[PATCH v3][Dovetail 6.6] arm64: dovetail: Fix undefinstr/break trap handling

[PATCH v3][Dovetail 6.6] arm64: dovetail: Fix undefinstr/break trap handling

[Clara Kowalsky]

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 the emulation of the deprecated armv8 SWP{B} instruction
(try_emulate_armv8_deprecated()) cannot be done from the out-of-band
stage.

Therefore do_undefinstr()/do_el0_undef reports the trap entry to the
companion core. If the companion core handles the undefined instruction,
running_oob returns true and the bailout occurs. Otherwise, switching to
the in-band stage takes place and the undefined instruction handler
continues with the compat breakpoint detection and the SWP{B}
instruction emulation.

Signed-off-by: Clara Kowalsky <clara.kowalsky@siemens.com>
---
 arch/arm64/kernel/traps.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index b0db35eda8f5..0811aa219ea7 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -456,29 +456,32 @@ void do_el0_undef(struct pt_regs *regs, unsigned long esr)
 {
 	u32 insn;
 
+	mark_trap_entry(ARM64_TRAP_UNDI, regs);
+
 	/*
 	 * 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;
+		goto out_exit;
 
 	/* check for AArch32 breakpoint instructions */
 	if (!aarch32_break_handler(regs))
-		return;
+		goto out_exit;
 
 	if (user_insn_read(regs, &insn))
 		goto out_err;
 
 	if (try_emulate_mrs(regs, insn))
-		return;
+		goto out_exit;
 
 	if (try_emulate_armv8_deprecated(regs, insn))
-		return;
+		goto out_exit;
 
 out_err:
-	mark_trap_entry(ARM64_TRAP_UNDI, regs);
 	force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc, 0);
+
+out_exit:
 	mark_trap_exit(ARM64_TRAP_UNDI, regs);
 }
 
-- 
2.39.2

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

[Philippe Gerum]

Clara Kowalsky <clara.kowalsky@siemens.com> writes:

> 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 the emulation of the deprecated armv8 SWP{B} instruction
> (try_emulate_armv8_deprecated()) cannot be done from the out-of-band
> stage.
>
> Therefore do_undefinstr()/do_el0_undef reports the trap entry to the
> companion core. If the companion core handles the undefined instruction,
> running_oob returns true and the bailout occurs. Otherwise, switching to
> the in-band stage takes place and the undefined instruction handler
> continues with the compat breakpoint detection and the SWP{B}
> instruction emulation.
>
> Signed-off-by: Clara Kowalsky <clara.kowalsky@siemens.com>
> ---
>  arch/arm64/kernel/traps.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index b0db35eda8f5..0811aa219ea7 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -456,29 +456,32 @@ void do_el0_undef(struct pt_regs *regs, unsigned long esr)
>  {
>  	u32 insn;
>  
> +	mark_trap_entry(ARM64_TRAP_UNDI, regs);
> +
>  	/*
>  	 * 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;
> +		goto out_exit;
>  
>  	/* check for AArch32 breakpoint instructions */
>  	if (!aarch32_break_handler(regs))
> -		return;
> +		goto out_exit;
>  
>  	if (user_insn_read(regs, &insn))
>  		goto out_err;
>  
>  	if (try_emulate_mrs(regs, insn))
> -		return;
> +		goto out_exit;
>  
>  	if (try_emulate_armv8_deprecated(regs, insn))
> -		return;
> +		goto out_exit;
>  
>  out_err:
> -	mark_trap_entry(ARM64_TRAP_UNDI, regs);
>  	force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc, 0);
> +
> +out_exit:
>  	mark_trap_exit(ARM64_TRAP_UNDI, regs);
>  }

Ack. I initially thought we could postpone the in-band switch until
after some of the emulation code paths have been considered (typically
the MRS stuff), but that would involve convoluted logic for no obvious
gain, so let's take the straightforward approach instead. The assumption
here is that issuing an (emulated) MRS read request from a userland
application should not happen from a time-critical context anyway, so
doing this in-band should be acceptable. We'll see from complains if its
not..

IOW, patch merged to v6.6, thanks.

PS: There are three Dovetail branches to maintain concurrently ATM,
twice that number when including the EVL trees. So please always mention
the kernel release any patch sent my way applies to, this may help
speeding up the process on my end (e.g. this patch conflicts, needs
manual backport to v5.10, v6.1.y).

-- 
Philippe.

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

[Florian Bezdeka]

On Tue, 2023-10-31 at 11:46 +0100, Philippe Gerum wrote:
> Ack. I initially thought we could postpone the in-band switch until
> after some of the emulation code paths have been considered (typically
> the MRS stuff), but that would involve convoluted logic for no obvious
> gain, so let's take the straightforward approach instead. The assumption
> here is that issuing an (emulated) MRS read request from a userland
> application should not happen from a time-critical context anyway, so
> doing this in-band should be acceptable. We'll see from complains if its
> not..
> 
> IOW, patch merged to v6.6, thanks.

Nice! Thanks!

> 
> PS: There are three Dovetail branches to maintain concurrently ATM,
> twice that number when including the EVL trees. So please always mention
> the kernel release any patch sent my way applies to, this may help
> speeding up the process on my end (e.g. this patch conflicts, needs
> manual backport to v5.10, v6.1.y).

We had [Dovetail 6.6] in the subject. I thought that should be
sufficient. Seems it's not. How should we make that clear?

Best regards,
Florian

> 
> -- 
> Philippe.

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

[Philippe Gerum]

Florian Bezdeka <florian.bezdeka@siemens.com> writes:

> On Tue, 2023-10-31 at 11:46 +0100, Philippe Gerum wrote:
>> Ack. I initially thought we could postpone the in-band switch until
>> after some of the emulation code paths have been considered (typically
>> the MRS stuff), but that would involve convoluted logic for no obvious
>> gain, so let's take the straightforward approach instead. The assumption
>> here is that issuing an (emulated) MRS read request from a userland
>> application should not happen from a time-critical context anyway, so
>> doing this in-band should be acceptable. We'll see from complains if its
>> not..
>> 
>> IOW, patch merged to v6.6, thanks.
>
> Nice! Thanks!
>
>> 
>> PS: There are three Dovetail branches to maintain concurrently ATM,
>> twice that number when including the EVL trees. So please always mention
>> the kernel release any patch sent my way applies to, this may help
>> speeding up the process on my end (e.g. this patch conflicts, needs
>> manual backport to v5.10, v6.1.y).
>
> We had [Dovetail 6.6] in the subject. I thought that should be
> sufficient. Seems it's not. How should we make that clear?

No, that's perfect. PEBKAC detected locally.

-- 
Philippe.

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

[Florian Bezdeka]

On Tue, 2023-10-31 at 12:23 +0100, Philippe Gerum wrote:
> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
> 
> > On Tue, 2023-10-31 at 11:46 +0100, Philippe Gerum wrote:
> > > Ack. I initially thought we could postpone the in-band switch until
> > > after some of the emulation code paths have been considered (typically
> > > the MRS stuff), but that would involve convoluted logic for no obvious
> > > gain, so let's take the straightforward approach instead. The assumption
> > > here is that issuing an (emulated) MRS read request from a userland
> > > application should not happen from a time-critical context anyway, so
> > > doing this in-band should be acceptable. We'll see from complains if its
> > > not..
> > > 
> > > IOW, patch merged to v6.6, thanks.
> > 
> > Nice! Thanks!
> > 
> > > 
> > > PS: There are three Dovetail branches to maintain concurrently ATM,
> > > twice that number when including the EVL trees. So please always mention
> > > the kernel release any patch sent my way applies to, this may help
> > > speeding up the process on my end (e.g. this patch conflicts, needs
> > > manual backport to v5.10, v6.1.y).
> > 
> > We had [Dovetail 6.6] in the subject. I thought that should be
> > sufficient. Seems it's not. How should we make that clear?
> 
> No, that's perfect. PEBKAC detected locally.

Happens to the best ;-)

Let us know if we can help backporting. Clara might have a look
especially at 6.1 - our preferred target right now.

> 
> -- 
> Philippe.

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

[Philippe Gerum]

Florian Bezdeka <florian.bezdeka@siemens.com> writes:

> On Tue, 2023-10-31 at 12:23 +0100, Philippe Gerum wrote:
>> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
>> 
>> > On Tue, 2023-10-31 at 11:46 +0100, Philippe Gerum wrote:
>> > > Ack. I initially thought we could postpone the in-band switch until
>> > > after some of the emulation code paths have been considered (typically
>> > > the MRS stuff), but that would involve convoluted logic for no obvious
>> > > gain, so let's take the straightforward approach instead. The assumption
>> > > here is that issuing an (emulated) MRS read request from a userland
>> > > application should not happen from a time-critical context anyway, so
>> > > doing this in-band should be acceptable. We'll see from complains if its
>> > > not..
>> > > 
>> > > IOW, patch merged to v6.6, thanks.
>> > 
>> > Nice! Thanks!
>> > 
>> > > 
>> > > PS: There are three Dovetail branches to maintain concurrently ATM,
>> > > twice that number when including the EVL trees. So please always mention
>> > > the kernel release any patch sent my way applies to, this may help
>> > > speeding up the process on my end (e.g. this patch conflicts, needs
>> > > manual backport to v5.10, v6.1.y).
>> > 
>> > We had [Dovetail 6.6] in the subject. I thought that should be
>> > sufficient. Seems it's not. How should we make that clear?
>> 
>> No, that's perfect. PEBKAC detected locally.
>
> Happens to the best ;-)
>
> Let us know if we can help backporting. Clara might have a look
> especially at 6.1 - our preferred target right now.
>

That would be great. I would take care of 5.10 then.

-- 
Philippe.

[PATCH v3][Dovetail 6.6] arm64: dovetail: Fix undefinstr/break trap handling

[Clara Kowalsky]

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 the emulation of the deprecated armv8 SWP{B} instruction
(try_emulate_armv8_deprecated()) cannot be done from the out-of-band
stage.

Therefore do_undefinstr()/do_el0_undef reports the trap entry to the
companion core. If the companion core handles the undefined instruction,
running_oob returns true and the bailout occurs. Otherwise, switching to
the in-band stage takes place and the undefined instruction handler
continues with the compat breakpoint detection and the SWP{B}
instruction emulation.

Signed-off-by: Clara Kowalsky <clara.kowalsky@siemens.com>
---
 arch/arm64/kernel/traps.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index b0db35eda8f5..0811aa219ea7 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -456,29 +456,32 @@ void do_el0_undef(struct pt_regs *regs, unsigned long esr)
 {
 	u32 insn;
 
+	mark_trap_entry(ARM64_TRAP_UNDI, regs);
+
 	/*
 	 * 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;
+		goto out_exit;
 
 	/* check for AArch32 breakpoint instructions */
 	if (!aarch32_break_handler(regs))
-		return;
+		goto out_exit;
 
 	if (user_insn_read(regs, &insn))
 		goto out_err;
 
 	if (try_emulate_mrs(regs, insn))
-		return;
+		goto out_exit;
 
 	if (try_emulate_armv8_deprecated(regs, insn))
-		return;
+		goto out_exit;
 
 out_err:
-	mark_trap_entry(ARM64_TRAP_UNDI, regs);
 	force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc, 0);
+
+out_exit:
 	mark_trap_exit(ARM64_TRAP_UNDI, regs);
 }
 
-- 
2.39.2