Re: [PATCH v2 07/11] arm64: debug: split single stepping exception entry

[Will Deacon <will@kernel.org>]

On Mon, May 12, 2025 at 06:43:22PM +0100, Ada Couprie Diaz wrote:
> Currently all debug exceptions share common entry code and are routed
> to `do_debug_exception()`, which calls dynamically-registered
> handlers for each specific debug exception. This is unfortunate as
> different debug exceptions have different entry handling requirements,
> and it would be better to handle these distinct requirements earlier.
> 
> The single stepping exception has the most constraints : it can be
> exploited to train branch predictors and it needs special handling at EL1
> for the Cortex-A76 erratum #1463225. We need to conserve all those
> mitigations.
> Move the call to `arm64_apply_bp_hardening()` to `entry-common.c` as
> it is needed for exceptions coming from EL0 only.
> However, it does not write an address at FAR_EL1, as only hardware
> watchpoints do so.
> 
> The single-step handler does its own signaling if it needs to and only
> returns 0, so we can call it directly from `entry-common.c`.
> 
> Split the single stepping exception entry, adjust the function signature,
> keep the security mitigation and erratum handling.
> 
> When taking a soft-step exception from EL0, most of the single stepping
> handling is safely preemptible : the only possible handler is
> `uprobe_singlestep_handler()`. It only operates on task-local data and
> properly checks its validity, then raises a Thread Information Flag,
> processed before returning to userspace in `do_notify_resume()`, which
> is already preemptible.
> However, the soft-step handler first calls `reinstall_suspended_bps()`
> to check if there is any hardware breakpoint or watchpoint pending
> or already stepped through.
> This cannot be preempted as it manipulates the hardware breakpoint and
> watchpoint registers.
> 
> Move the call to `reinstall_suspended_bps()` to `entry-common.c` and
> adjust the relevant comments.
> We can now safely unmask interrupts before handling the step itself,
> fixing a PREEMPT_RT issue where the handler could call a sleeping function
> with preemption disabled.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> Closes: https://lore.kernel.org/linux-arm-kernel/Z6YW_Kx4S2tmj2BP@uudg.org/
> ---
>  arch/arm64/include/asm/exception.h |  1 +
>  arch/arm64/kernel/debug-monitors.c | 19 +++----------
>  arch/arm64/kernel/entry-common.c   | 43 ++++++++++++++++++++++++++++++
>  arch/arm64/kernel/hw_breakpoint.c  |  6 ++---
>  4 files changed, 51 insertions(+), 18 deletions(-)

[...]

> @@ -770,6 +790,25 @@ static void noinstr el0_breakpt(struct pt_regs *regs, unsigned long esr)
>  	exit_to_user_mode(regs);
>  }
>  
> +static void noinstr el0_softstp(struct pt_regs *regs, unsigned long esr)
> +{
> +	if (!is_ttbr0_addr(regs->pc))
> +		arm64_apply_bp_hardening();

Similar to the other patch, I think this is a functional change. It
might be fine, but it should be called out in the commit message if it's
intentional.

> +	enter_from_user_mode(regs);
> +	/*
> +	 * After handling a breakpoint, we suspend the breakpoint
> +	 * and use single-step to move to the next instruction.
> +	 * If we have a suspended breakpoint there's nothing more to do:
> +	 * complete the single-step.
> +	 */
> +	if (reinstall_suspended_bps(regs)) {
> +		local_daif_restore(DAIF_PROCCTX);
> +		do_softstep(esr, regs);
> +	}
> +	exit_to_user_mode(regs);

I quite like the look of this now, but perhaps we could rename
reinstall_suspended_bps() and change the return value to make things a
bit more readable? For example, 'if (!stepped_suspended_breakpt(regs))'
or something like that? What do you think?

Will