[RFC PATCH v2 0/2] Fix trap related handling

[RFC PATCH v2 0/2] Fix trap related handling

[Clara Kowalsky]

Hi all,

the series provides a fix for the issue discussed in:
Link: https://lore.kernel.org/xenomai/87edjiuifo.fsf@xenomai.org/T/#mbe75bcddd368954dbdb7bbcc00e9fcfa71f7ea8a

It does two things:
- Fix the break trap handling in the undefined instruction trap
- Add a notification before the emulation of the armv8 SWP{B} instruction

Signed-off-by: Clara Kowalsky <clara.kowalsky@siemens.com>
---
Clara Kowalsky (2):
  arm64: dovetail: Fix undefinstr/break trap handling
  arm64: dovetail: Fix armv8 SWP{B} instruction handling

 arch/arm64/include/asm/dovetail.h    | 1 +
 arch/arm64/kernel/armv8_deprecated.c | 8 +++++++-
 arch/arm64/kernel/debug-monitors.c   | 2 ++
 arch/arm64/kernel/traps.c            | 7 -------
 4 files changed, 10 insertions(+), 8 deletions(-)

-- 
2.39.2

[RFC PATCH v2 1/2] arm64: dovetail: Fix undefinstr/break trap handling

[Clara Kowalsky]

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

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 | 2 ++
 arch/arm64/kernel/traps.c          | 7 -------
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 32271ed24ef5..8157496bea62 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -373,7 +373,9 @@ 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_entry(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 b0db35eda8f5..a8c3642c61dc 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -456,13 +456,6 @@ void do_el0_undef(struct pt_regs *regs, unsigned long esr)
 {
 	u32 insn;
 
-	/*
-	 * 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;
-- 
2.39.2

[RFC PATCH v2 2/2] arm64: dovetail: Fix armv8 SWP{B} instruction handling

[Clara Kowalsky]

The emulation of the deprecated armv8 SWP{B} instruction cannot be done
from the out-of-band stage. The core is notified before handling the SWP
emulation.

Reported-by: Philippe Gerum <rpm@xenomai.org>
Signed-off-by: Clara Kowalsky <clara.kowalsky@siemens.com>
---
 arch/arm64/include/asm/dovetail.h    | 1 +
 arch/arm64/kernel/armv8_deprecated.c | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/dovetail.h b/arch/arm64/include/asm/dovetail.h
index f7a672c750a9..f83f736747f8 100644
--- a/arch/arm64/include/asm/dovetail.h
+++ b/arch/arm64/include/asm/dovetail.h
@@ -19,6 +19,7 @@
 #define ARM64_TRAP_SVE		7	/* SVE access trap */
 #define ARM64_TRAP_BTI		8	/* Branch target identification */
 #define ARM64_TRAP_SME		9	/* SME access trap */
+#define ARM64_TRAP_SWP		10	/* Emulation of deprecated armv8 SWP{B} instruction */
 
 #ifdef CONFIG_DOVETAIL
 
diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
index e459cfd33711..755e0785fcf4 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -232,6 +232,8 @@ static int swp_handler(struct pt_regs *regs, u32 instr)
 
 static bool try_emulate_swp(struct pt_regs *regs, u32 insn)
 {
+	int rc;
+
 	/* SWP{B} only exists in ARM state and does not exist in Thumb */
 	if (!compat_user_mode(regs) || compat_thumb_mode(regs))
 		return false;
@@ -239,7 +241,11 @@ static bool try_emulate_swp(struct pt_regs *regs, u32 insn)
 	if ((insn & 0x0fb00ff0) != 0x01000090)
 		return false;
 
-	return swp_handler(regs, insn) == 0;
+	mark_trap_entry(ARM64_TRAP_SWP, regs);
+	rc = swp_handler(regs, insn) == 0;
+	mark_trap_exit(ARM64_TRAP_SWP, regs);
+
+	return rc;
 }
 
 static struct insn_emulation insn_swp = {
-- 
2.39.2

RE: [RFC PATCH v2 1/2] arm64: dovetail: Fix undefinstr/break trap handling

[Johannes Kirchmair]

Hey Clara,

> -----Original Message-----
> From: Clara Kowalsky <clara.kowalsky@siemens.com>
> Sent: Freitag, 6. Oktober 2023 12:43
> To: xenomai@lists.linux.dev
> Cc: jan.kiszka@siemens.com; florian.bezdeka@siemens.com; Clara Kowalsky
> <clara.kowalsky@siemens.com>
> Subject: [RFC PATCH v2 1/2] arm64: dovetail: Fix undefinstr/break trap handling
> 
> [Sie erhalten nicht häufig E-Mails von clara.kowalsky@siemens.com. Weitere
> Informationen, warum dies wichtig ist, finden Sie unter
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> CAUTION: External E-Mail !
> 
> From: Florian Bezdeka <florian.bezdeka@siemens.com>
> 
> 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 | 2 ++
>  arch/arm64/kernel/traps.c          | 7 -------
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-
> monitors.c
> index 32271ed24ef5..8157496bea62 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -373,7 +373,9 @@ 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_entry(ARM64_TRAP_UNDI, regs);
Should this be a mark_trap_exit?


>         return 0;
>  }
>  NOKPROBE_SYMBOL(aarch32_break_handler);
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index b0db35eda8f5..a8c3642c61dc 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -456,13 +456,6 @@ void do_el0_undef(struct pt_regs *regs, unsigned long
> esr)
>  {
>         u32 insn;
> 
> -       /*
> -        * 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;
Shouldn't this be moved below the mark_trap_entry?
If the companion core decides to handle the undefined instructions, we will be running_oob after mark_trap_entry and we should not emit an signal in that case.

Best regards
Johannes
> -
>         /* check for AArch32 breakpoint instructions */
>         if (!aarch32_break_handler(regs))
>                 return;
> --
> 2.39.2
> 

Re: [RFC PATCH v2 1/2] arm64: dovetail: Fix undefinstr/break trap handling

[Florian Bezdeka]

On Mon, 2023-10-09 at 12:44 +0000, Johannes Kirchmair wrote:
> Hey Clara,
> 
> > -----Original Message-----
> > From: Clara Kowalsky <clara.kowalsky@siemens.com>
> > Sent: Freitag, 6. Oktober 2023 12:43
> > To: xenomai@lists.linux.dev
> > Cc: jan.kiszka@siemens.com; florian.bezdeka@siemens.com; Clara Kowalsky
> > <clara.kowalsky@siemens.com>
> > Subject: [RFC PATCH v2 1/2] arm64: dovetail: Fix undefinstr/break trap handling
> > 
> > [Sie erhalten nicht häufig E-Mails von clara.kowalsky@siemens.com. Weitere
> > Informationen, warum dies wichtig ist, finden Sie unter
> > https://aka.ms/LearnAboutSenderIdentification ]
> > 
> > CAUTION: External E-Mail !
> > 
> > From: Florian Bezdeka <florian.bezdeka@siemens.com>
> > 
> > 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 | 2 ++
> >  arch/arm64/kernel/traps.c          | 7 -------
> >  2 files changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-
> > monitors.c
> > index 32271ed24ef5..8157496bea62 100644
> > --- a/arch/arm64/kernel/debug-monitors.c
> > +++ b/arch/arm64/kernel/debug-monitors.c
> > @@ -373,7 +373,9 @@ 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_entry(ARM64_TRAP_UNDI, regs);
> Should this be a mark_trap_exit?

It should (and it was in v1). That means that you manually modified the
patch... Seems like a broken workflow on your end.

> 
> 
> >         return 0;
> >  }
> >  NOKPROBE_SYMBOL(aarch32_break_handler);
> > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > index b0db35eda8f5..a8c3642c61dc 100644
> > --- a/arch/arm64/kernel/traps.c
> > +++ b/arch/arm64/kernel/traps.c
> > @@ -456,13 +456,6 @@ void do_el0_undef(struct pt_regs *regs, unsigned long
> > esr)
> >  {
> >         u32 insn;
> > 
> > -       /*
> > -        * 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;
> Shouldn't this be moved below the mark_trap_entry?
> If the companion core decides to handle the undefined instructions, we will be running_oob after mark_trap_entry and we should not emit an signal in that case.
> 
> Best regards
> Johannes
> > -
> >         /* check for AArch32 breakpoint instructions */
> >         if (!aarch32_break_handler(regs))
> >                 return;
> > --
> > 2.39.2
> > 
>