| Message ID | afe50d1db63a10fde9547ea08fe1fa68b0638aba.1624618157.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] powerpc/signal: Fix handling of SA_RESTORER sigaction flag | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (dc4225b1afe8424b3fdf56599cdac283be683c69) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 30 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch is tagged for stable |
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > powerpc advertises support of SA_RESTORER sigaction flag. > > Make it the truth. > > Cc: stable@vger.kernel.org > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/kernel/signal_32.c | 8 ++++++-- > arch/powerpc/kernel/signal_64.c | 4 +++- > 2 files changed, 9 insertions(+), 3 deletions(-) Hi Christophe, I dug into the history a bit on this. The 32-bit port originally did not define SA_RESTORER in include/asm-ppc/signal.h, but it was added in 2.1.79. https://github.com/mpe/linux-fullhistory/commit/4e7e9c0d54ff5725a73d2210a950f8bc0f225073 That commit added SA_RESTORER to the header, added code to get/set it in sys_sigaction(), but didn't add any code to use it for signal delivery. The 64-bit port was merged with SA_RESTORER already defined in include/asm-ppc64/signal.h: https://github.com/mpe/linux-fullhistory/commit/c3aa9878533e724f639852c3d951e6a169e04081 Similarly there was code to set/get it in sys_sigaction(), but no code to use it for signal delivery. Later the two ports were merged, and the headers were moved and disintegrated into uapi, so we end up today with SA_RESTORER defined in arch/powerpc/include/uapi/asm/signal.h, but no code to use it. So essentially we've had SA_RESTORER defined since ancient kernels, but never actually supported using it for anything. One problem with enabling it now is there's no way for userspace to determine if it's on a fixed kernel or not. That makes it unusable for userspace, unless it does version checks, or is happy to break on all old kernels (not likely). We could solve that by defining SA_RESTORER_FIXED or something, but that's slightly gross. It's also described in the man page as "Not intended for application use", ie. it's intended for use by libc. I'm not sure there's any value in adding support for it to the kernel unless we know there's interest from glibc/musl in using it. So my inclination is that we should *not* add support for it, rather we should leave it unimplemented and remove SA_RESTORER from the header. There's precedent in riscv for not supporting it at all. cheers > diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c > index 0608581967f0..cf3da1386595 100644 > --- a/arch/powerpc/kernel/signal_32.c > +++ b/arch/powerpc/kernel/signal_32.c > @@ -769,7 +769,9 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset, > } > > /* Save user registers on the stack */ > - if (tsk->mm->context.vdso) { > + if (ksig->ka.sa.sa_flags & SA_RESTORER) { > + tramp = (unsigned long)ksig->ka.sa.sa_restorer; > + } else if (tsk->mm->context.vdso) { > tramp = VDSO32_SYMBOL(tsk->mm->context.vdso, sigtramp_rt32); > } else { > tramp = (unsigned long)mctx->mc_pad; > @@ -865,7 +867,9 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset, > else > unsafe_save_user_regs(regs, mctx, tm_mctx, 1, failed); > > - if (tsk->mm->context.vdso) { > + if (ksig->ka.sa.sa_flags & SA_RESTORER) { > + tramp = (unsigned long)ksig->ka.sa.sa_restorer; > + } else if (tsk->mm->context.vdso) { > tramp = VDSO32_SYMBOL(tsk->mm->context.vdso, sigtramp32); > } else { > tramp = (unsigned long)mctx->mc_pad; > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c > index 1831bba0582e..fb31a334aca6 100644 > --- a/arch/powerpc/kernel/signal_64.c > +++ b/arch/powerpc/kernel/signal_64.c > @@ -910,7 +910,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > tsk->thread.fp_state.fpscr = 0; > > /* Set up to return from userspace. */ > - if (tsk->mm->context.vdso) { > + if (ksig->ka.sa.sa_flags & SA_RESTORER) { > + regs_set_return_ip(regs, (unsigned long)ksig->ka.sa.sa_restorer); > + } else if (tsk->mm->context.vdso) { > regs_set_return_ip(regs, VDSO64_SYMBOL(tsk->mm->context.vdso, sigtramp_rt64)); > } else { > err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]); > -- > 2.25.0
Le 04/02/2022 à 11:22, Michael Ellerman a écrit : > Christophe Leroy <christophe.leroy@csgroup.eu> writes: >> powerpc advertises support of SA_RESTORER sigaction flag. >> >> Make it the truth. >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> arch/powerpc/kernel/signal_32.c | 8 ++++++-- >> arch/powerpc/kernel/signal_64.c | 4 +++- >> 2 files changed, 9 insertions(+), 3 deletions(-) > > Hi Christophe, > > I dug into the history a bit on this. > > The 32-bit port originally did not define SA_RESTORER in > include/asm-ppc/signal.h, but it was added in 2.1.79. > > https://github.com/mpe/linux-fullhistory/commit/4e7e9c0d54ff5725a73d2210a950f8bc0f225073 > > That commit added SA_RESTORER to the header, added code to get/set it in > sys_sigaction(), but didn't add any code to use it for signal delivery. > > > The 64-bit port was merged with SA_RESTORER already defined in > include/asm-ppc64/signal.h: > > https://github.com/mpe/linux-fullhistory/commit/c3aa9878533e724f639852c3d951e6a169e04081 > > Similarly there was code to set/get it in sys_sigaction(), but no code > to use it for signal delivery. > > > Later the two ports were merged, and the headers were moved and > disintegrated into uapi, so we end up today with SA_RESTORER defined in > arch/powerpc/include/uapi/asm/signal.h, but no code to use it. > > So essentially we've had SA_RESTORER defined since ancient kernels, but > never actually supported using it for anything. > > > One problem with enabling it now is there's no way for userspace to > determine if it's on a fixed kernel or not. That makes it unusable for > userspace, unless it does version checks, or is happy to break on all > old kernels (not likely). We could solve that by defining > SA_RESTORER_FIXED or something, but that's slightly gross. > > It's also described in the man page as "Not intended for application > use", ie. it's intended for use by libc. I'm not sure there's any value > in adding support for it to the kernel unless we know there's interest > from glibc/musl in using it. > > So my inclination is that we should *not* add support for it, rather we > should leave it unimplemented and remove SA_RESTORER from the header. > There's precedent in riscv for not supporting it at all. > Nowadays, stacks are mapped noexec, so the fallback stack trampoline cannot work anymore. If a process doesn't want exec stack and doesn't want to map the VDSO, the SA_RESTORER is the only alternative to get signal working. On several architectures including arm64 and s390 only VDSO and SA_RESTORER are supported, on stack signal trampoline is not supported anymore. So my idea was to first implement SA_RESTORER and then as a second step to retire the on stack signal trampoline which has become useless with noexec stacks. See https://elixir.bootlin.com/linux/v5.17-rc1/source/arch/arm64/kernel/signal.c#L761 or https://elixir.bootlin.com/linux/v5.17-rc1/source/arch/s390/kernel/signal.c#L337 Christophe
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c index 0608581967f0..cf3da1386595 100644 --- a/arch/powerpc/kernel/signal_32.c +++ b/arch/powerpc/kernel/signal_32.c @@ -769,7 +769,9 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset, } /* Save user registers on the stack */ - if (tsk->mm->context.vdso) { + if (ksig->ka.sa.sa_flags & SA_RESTORER) { + tramp = (unsigned long)ksig->ka.sa.sa_restorer; + } else if (tsk->mm->context.vdso) { tramp = VDSO32_SYMBOL(tsk->mm->context.vdso, sigtramp_rt32); } else { tramp = (unsigned long)mctx->mc_pad; @@ -865,7 +867,9 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset, else unsafe_save_user_regs(regs, mctx, tm_mctx, 1, failed); - if (tsk->mm->context.vdso) { + if (ksig->ka.sa.sa_flags & SA_RESTORER) { + tramp = (unsigned long)ksig->ka.sa.sa_restorer; + } else if (tsk->mm->context.vdso) { tramp = VDSO32_SYMBOL(tsk->mm->context.vdso, sigtramp32); } else { tramp = (unsigned long)mctx->mc_pad; diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c index 1831bba0582e..fb31a334aca6 100644 --- a/arch/powerpc/kernel/signal_64.c +++ b/arch/powerpc/kernel/signal_64.c @@ -910,7 +910,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, tsk->thread.fp_state.fpscr = 0; /* Set up to return from userspace. */ - if (tsk->mm->context.vdso) { + if (ksig->ka.sa.sa_flags & SA_RESTORER) { + regs_set_return_ip(regs, (unsigned long)ksig->ka.sa.sa_restorer); + } else if (tsk->mm->context.vdso) { regs_set_return_ip(regs, VDSO64_SYMBOL(tsk->mm->context.vdso, sigtramp_rt64)); } else { err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
powerpc advertises support of SA_RESTORER sigaction flag. Make it the truth. Cc: stable@vger.kernel.org Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- arch/powerpc/kernel/signal_32.c | 8 ++++++-- arch/powerpc/kernel/signal_64.c | 4 +++- 2 files changed, 9 insertions(+), 3 deletions(-)