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 <[email protected]>
> ---
> 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
