| Message ID | 20200524043827.GA33001@juliacomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: ptrace: Fix PTRACE_SINGLESTEP into signal handler | expand |
On Sun, May 24, 2020 at 12:38:27AM -0400, Keno Fischer wrote: > Executing PTRACE_SINGLESTEP at a signal stop is special. It > is supposed to step merely the signal setup work that the > kernel does, but not any instructions in user space. Since > not all architectures have the ability to generate a > single-step exception directly upon return from user-space, > there is a generic pseudo-single-step-stop that may be used > for this purpose (tracehook_signal_handler). Now, arm64 does > have the ability to generate single-step exceptions directly > upon return to userspace and was using this capability (rather > than the generic pseudo-trap) to obtain a similar effect. However, > there is actually a subtle difference that becomes noticeable > when the signal handler in question attempts to block SIGTRAP > (either because it is set in sa_mask, or because it is a handler > for SIGTRAP itself and SA_NODEFER is not set). In such a > situation, a real single step exception will cause the SIGTRAP > signal to be forcibly unblocked and the signal disposition > to be reset to SIG_DFL. The generic pseudo-single-step does > not suffer from this problem, because the SIGTRAP it delivers > is not real. The arm64 behavior is problematic, because a forced > reset of the signal disposition can be quite disruptive to the > userspace program. This patch brings the arm64 behavior in line > with the other major architectures by using the generic > pseudo-single-step-stop, avoiding the problematic interaction > with SIGTRAP masks. > > Fixes: 2c020ed8 ("arm64: Signal handling support") nit: please use a 12-character ID here. > Signed-off-by: Keno Fischer <keno@juliacomputing.com> > --- > arch/arm64/kernel/signal.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c > index 339882db5a91..cf237ee9443b 100644 > --- a/arch/arm64/kernel/signal.c > +++ b/arch/arm64/kernel/signal.c > @@ -808,14 +808,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs) > */ > ret |= !valid_user_regs(®s->user_regs, current); > > - /* > - * Fast forward the stepping logic so we step into the signal > - * handler. > - */ > - if (!ret) > - user_fastforward_single_step(tsk); > - > - signal_setup_done(ret, ksig, 0); > + signal_setup_done(ret, ksig, test_thread_flag(TIF_SINGLESTEP)); another nit: tsk is now unused, so this generates a compiler warning: arch/arm64/kernel/signal.c:787:22: warning: unused variable 'tsk' [-Wunused-variable] struct task_struct *tsk = current; ^ 1 warning generated. Also, the si_code used by signal_setup_done seems to be SIGTRAP, whereas we usually set TRAP_TRACE. What's the correct behaviour here? Looks like x86 uses TRAP_BRKPT... :/ Will
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 339882db5a91..cf237ee9443b 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -808,14 +808,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs) */ ret |= !valid_user_regs(®s->user_regs, current); - /* - * Fast forward the stepping logic so we step into the signal - * handler. - */ - if (!ret) - user_fastforward_single_step(tsk); - - signal_setup_done(ret, ksig, 0); + signal_setup_done(ret, ksig, test_thread_flag(TIF_SINGLESTEP)); } /*
Executing PTRACE_SINGLESTEP at a signal stop is special. It is supposed to step merely the signal setup work that the kernel does, but not any instructions in user space. Since not all architectures have the ability to generate a single-step exception directly upon return from user-space, there is a generic pseudo-single-step-stop that may be used for this purpose (tracehook_signal_handler). Now, arm64 does have the ability to generate single-step exceptions directly upon return to userspace and was using this capability (rather than the generic pseudo-trap) to obtain a similar effect. However, there is actually a subtle difference that becomes noticeable when the signal handler in question attempts to block SIGTRAP (either because it is set in sa_mask, or because it is a handler for SIGTRAP itself and SA_NODEFER is not set). In such a situation, a real single step exception will cause the SIGTRAP signal to be forcibly unblocked and the signal disposition to be reset to SIG_DFL. The generic pseudo-single-step does not suffer from this problem, because the SIGTRAP it delivers is not real. The arm64 behavior is problematic, because a forced reset of the signal disposition can be quite disruptive to the userspace program. This patch brings the arm64 behavior in line with the other major architectures by using the generic pseudo-single-step-stop, avoiding the problematic interaction with SIGTRAP masks. Fixes: 2c020ed8 ("arm64: Signal handling support") Signed-off-by: Keno Fischer <keno@juliacomputing.com> --- arch/arm64/kernel/signal.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)