Re: [PATCH] arm64: dovetail: Fix undefinstr/break trap handling

[Jan Kiszka <jan.kiszka@siemens.com>]

On 25.08.23 14:58, Florian Bezdeka wrote:
> When running an compat RT application on arm64 the break trap is
> handled via the undefined instruction trap.
> 
> A possible call stack looks like this:
> 
> Call trace:
>   handle_inband_event+0x2d0/0x320
>   inband_event_notify+0x28/0x50
>   signal_wake_up_state+0x7c/0xa4
>   complete_signal+0x104/0x2d0
>   __send_signal_locked+0x1d0/0x3e4
>   send_signal_locked+0xf0/0x140
>   force_sig_info_to_task+0xa0/0x164
>   force_sig_fault+0x64/0x94
>   arm64_force_sig_fault+0x48/0x80
>   send_user_sigtrap+0x50/0x8c
>   aarch32_break_handler+0xac/0x1d0
>   do_undefinstr+0x6c/0x360
>   el0_undef+0x4c/0xd0
>   el0t_32_sync_handler+0xd0/0x140
>   el0t_32_sync+0x190/0x194
> 
> The trap is never reported to the companion core at that stage so
> running_oob() in do_undefinstr() will always return true. As the
> following bailout happens before calling the compat breakpoint
> detection (aarch32_break_handler()) debugging the compat 
> application does not work.
> 
> In addition aarch32_break_handler() has to report the trap entry to the
> companion core.
> 
> Reported-by: Clara Kowalsky <clara.kowalsky@siemens.com>
> Tested-by: Clara Kowalsky <clara.kowalsky@siemens.com>
> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> ---
>  arch/arm64/kernel/debug-monitors.c | 3 +++
>  arch/arm64/kernel/traps.c          | 7 -------
>  2 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 32271ed24ef5..ef7ac042a0a6 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -373,7 +373,10 @@ int aarch32_break_handler(struct pt_regs *regs)
>  	if (!bp)
>  		return -EFAULT;
>  
> +	mark_trap_entry(ARM64_TRAP_UNDI, regs);
>  	send_user_sigtrap(TRAP_BRKPT);
> +	mark_trap_exit(ARM64_TRAP_UNDI, regs);
> +
>  	return 0;
>  }
>  NOKPROBE_SYMBOL(aarch32_break_handler);
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index cc68be400244..9bf2f309248f 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -489,13 +489,6 @@ void arm64_notify_segfault(unsigned long addr)
>  
>  void do_undefinstr(struct pt_regs *regs, unsigned long esr)
>  {
> -	/*
> -	 * If the companion core did not switched us to in-band
> -	 * context, we may assume that it has handled the trap.
> -	 */
> -	if (running_oob())
> -		return;
> -
>  	/* check for AArch32 breakpoint instructions */
>  	if (!aarch32_break_handler(regs))
>  		return;

This is not against v6.5-dovetail-rebase, right? We likely need to start
from the top, then backport to stable.

Also note that this change came in via
https://source.denx.de/Xenomai/linux-dovetail/-/commit/2b2ccdaeb8116727cf4076960d664a3cedff0ac6,
so just dropping it will likely cause problems elsewhere. Should we
rather move that down in the handler?

Jan

-- 
Siemens AG, Technology
Linux Expert Center