Messages in this thread | | | Date | Wed, 27 May 2020 13:06:59 +0100 | From | Will Deacon <> | Subject | Re: [PATCH] arm64: ptrace: Fix PTRACE_SINGLESTEP into signal handler |
| |
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
| |