[PATCH v2 00/11] arm64: debug: remove hook registration, split exception entry

[PATCH v2 00/11] arm64: debug: remove hook registration, split exception entry

[Ada Couprie Diaz]

Hi,

This series simplifies the debug exception entry path by removing handler
registration mechanisms for the debug exception handlers, a holdover from
the arm kernel, as well as the break and stepping handlers.
This moves much of the code related to debug exceptions outside of
`mm/fault.c` where it didn't make much sense.
This allows us to split the debug exception entries: going from one common
path per EL for all debug exceptions to a unique one per exception and EL.

The result is a much simpler and fully static exception entry path, which
we tailor to the different exceptions and their constraints.

The series is structured as such :
1 : related clean-up in the signle step handler
2-4 : software breakpoints and single step handlers registration removal
5: preparatory function duplication that gets cleaned-up in patch 11
6-11 : debug exception splitting and handler registration removal

* Patch 2 copies and extends the `early_brk64` break handling for the
normal path, byassing the dynamic registration.
* Patch 3 does something similar for the single stepping handlers.
* Patches 6 through 10 split each individual debug exception from
the unified path by creating specific handlers, adapting them to
their constraints and calling into them, bypassing the dynamically
registered handlers used before.
* Patches 4 and 11 are clean-ups removing the code that has been replaced
and made redundant by the preceding patches.

Single Step Exception
===

Of note, this allows us to make the single exception handling mostly
preemptible coming from EL0 in patch 7, fixing an issue with PREEMPT_RT[0].
The commit message details the reasoning on why this should be safe.
It is *definitely* not preemptible at EL1 in the current state, given
that the EL1 software step exception is handled by KGDB.

CC-ing Luis and Sebastian as they were active on the original bug report.

Cc: "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Testing
===

Testing EL1 debug exceptions can only be properly done with perf.
A simple test of hardware breakpoints, watchpoints, and software stepping
can be achieved by using `perf stat sleep 0.01` and adding hardware events
for `jiffies` and `hrtimer_nanosleep`, which guarantees a
hardware watchpoint and breakpoint.
Inserting a `WARN()` at a convenient place will allow testing both early
and late software breakpoints at EL1, or using KGDB to test without
changing code.

For EL0 debug exceptions, the easiest is to setup a basic program and use
GDB with a list of pre-programmed commands setting hardware and software
breakpoints as well as watchpoints. Setpping will occur naturally when
encountering breakpoints.

All tests maintained behaviour after the patches.

I also tested with KASAN on, with no apparent impact.

See below for some testing examples.

Regarding [0], I tested a PREEMPT_RT kernel with the patches, running
`ssdd` on loop as well as some spammy GDB stepping, and some
hardware watchpoints on a tight loop without any issues.


Based on v6.15-rc6.
Thanks,
Ada

v2 :
 - Move the BP hardening call outside of the handlers to `entry-common`,
   as they are not needed coming from EL1.
 - Make the EL0 software stepping exception mostly preemptible
   - Move `reinstall_hw_bps()` call to `entry-common`
   - Don't disable preemption in `el0_softstp()`
   - Unmask DAIF before `do_softstep()` in `el0_softstp()`
   - Update comments to make sense with the changes
 - Simplify the single step handler, as it always returns 0 and could not
   trigger the `arm64_notify_die()` call.
 - Fix some commit messages
v1 : https://lore.kernel.org/linux-arm-kernel/20250425153647.438508-1-ada.coupriediaz@arm.com

[0]: https://lore.kernel.org/linux-arm-kernel/Z6YW_Kx4S2tmj2BP@uudg.org/


Testing examples
===

Perf (for EL1):
~~~
Assuming that `perf` is on your $PATH and building with `kallsyms`

  #!/bin/bash
  
  watch_addr=$(sudo cat /proc/kallsyms | grep "D jiffies$" | cut -f1 -d\  )
  break_addr=$(sudo cat /proc/kallsyms | grep "clock_nanosleep$" | cut -f1 -d\  )
  
  cmd="sleep 0.01"
  
  sudo perf stat -a -e mem:0x${watch_addr}/8:w -e mem:0x${break_addr}:x ${cmd}

NB: This does /not/ test EL1 BRKs.


GDB commands (for EL0):
~~~
The following C example, compiled with `-g -O0`

  int main() {
          int add = 0xAA;
          int target = 0;

          target += add;

  #ifdef COMPAT
      __asm__("BKPT");
  #else
      __asm__("BRK 1");
  #endif
  
          return target;
  }

Combined with the following GDB command-list

  start
  hbreak 3
  watch target
  commands 2
  continue
  end
  commands 3
  continue
  end
  continue
  jump 11
  continue
  quit

Executed as such : `gdb -x ${COMMAND_LIST_FILE} ./a.out`
should go through the whole program, return 0252/170/0xAA, and
exercise all EL0 debug exception entries.
By using a cross-compiler and passing and additional `-DCOMPAT` argument
during compilation, the `BKPT32` path can also be tested.
NOTE: `BKPT` *will* make GDB loop infinitely, that is expected. Sending
SIGINT to GDB will break the loop and the execution should complete.

Ada Couprie Diaz (11):
  arm64: debug: clean up single_step_handler logic
  arm64: debug: call software break handlers statically
  arm64: debug: call step handlers statically
  arm64: debug: remove break/step handler registration infrastructure
  arm64: entry: Add entry and exit functions for debug exceptions
  arm64: debug: split hardware breakpoint exeception entry
  arm64: debug: split single stepping exception entry
  arm64: debug: split hardware watchpoint exception entry
  arm64: debug: split brk64 exception entry
  arm64: debug: split bkpt32 exception entry
  arm64: debug: remove debug exception registration infrastructure

 arch/arm64/include/asm/debug-monitors.h       |  26 ---
 arch/arm64/include/asm/exception.h            |   6 +-
 arch/arm64/include/asm/kgdb.h                 |   4 +
 arch/arm64/include/asm/kprobes.h              |   6 +
 arch/arm64/include/asm/system_misc.h          |   4 -
 arch/arm64/include/asm/traps.h                |   6 +
 arch/arm64/include/asm/uprobes.h              |   3 +
 arch/arm64/kernel/debug-monitors.c            | 205 +++++++-----------
 arch/arm64/kernel/entry-common.c              | 148 ++++++++++++-
 arch/arm64/kernel/hw_breakpoint.c             |  35 ++-
 arch/arm64/kernel/kgdb.c                      |  39 +---
 arch/arm64/kernel/probes/kprobes.c            |  31 +--
 arch/arm64/kernel/probes/kprobes_trampoline.S |   2 +-
 arch/arm64/kernel/probes/uprobes.c            |  18 +-
 arch/arm64/kernel/traps.c                     |  77 +------
 arch/arm64/mm/fault.c                         |  75 -------
 16 files changed, 294 insertions(+), 391 deletions(-)


base-commit: 82f2b0b97b36ee3fcddf0f0780a9a0825d52fec3
-- 
2.43.0

[PATCH v2 01/11] arm64: debug: clean up single_step_handler logic

[Ada Couprie Diaz]

Remove the unnecessary boolean which always checks if the handler was found
and return early instead.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/kernel/debug-monitors.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 58f047de3e1c..676fa0231935 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -241,8 +241,6 @@ static void send_user_sigtrap(int si_code)
 static int single_step_handler(unsigned long unused, unsigned long esr,
 			       struct pt_regs *regs)
 {
-	bool handler_found = false;
-
 	/*
 	 * If we are stepping a pending breakpoint, call the hw_breakpoint
 	 * handler first.
@@ -250,10 +248,10 @@ static int single_step_handler(unsigned long unused, unsigned long esr,
 	if (!reinstall_suspended_bps(regs))
 		return 0;
 
-	if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
-		handler_found = true;
+	if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
+		return 0;
 
-	if (!handler_found && user_mode(regs)) {
+	if (user_mode(regs)) {
 		send_user_sigtrap(TRAP_TRACE);
 
 		/*
@@ -263,7 +261,7 @@ static int single_step_handler(unsigned long unused, unsigned long esr,
 		 * to the active-not-pending state).
 		 */
 		user_rewind_single_step(current);
-	} else if (!handler_found) {
+	} else {
 		pr_warn("Unexpected kernel single-step exception at EL1\n");
 		/*
 		 * Re-enable stepping since we know that we will be
-- 
2.43.0

[PATCH v2 02/11] arm64: debug: call software break handlers statically

[Ada Couprie Diaz]

Software breakpoints pass an immediate value in ESR ("comment") that can
be used to call a specialized handler (KGDB, KASAN...).
We do so in two different ways :
 - During early boot, `early_brk64` statically checks against known
   immediates and calls the corresponding handler,
 - During init, handlers are dynamically registered into a list. When
   called, the generic software breakpoint handler will iterate over
   the list to find the appropriate handler.

The dynamic registration does not provide any benefit here as it is not
exported and all its uses are within the arm64 tree. It also depends on an
RCU list, whose safe access currently relies on the non-preemptible state
of `do_debug_exception`.

Replace the list iteration logic in `call_break_hooks` to call
the breakpoint handlers statically if they are enabled, like in
`early_brk64`.
Expose the handlers in their respective headers to be reachable from
`arch/arm64/kernel/debug-monitors.c` at link time.

Unify the naming of the software breakpoint handlers to XXX_brk_handler(),
making it clear they are related and to differentiate from the
hardware breakpoints.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/kgdb.h                 |  3 +
 arch/arm64/include/asm/kprobes.h              |  6 ++
 arch/arm64/include/asm/traps.h                |  6 ++
 arch/arm64/include/asm/uprobes.h              |  2 +
 arch/arm64/kernel/debug-monitors.c            | 57 ++++++++++++++----
 arch/arm64/kernel/kgdb.c                      | 22 ++-----
 arch/arm64/kernel/probes/kprobes.c            | 31 ++--------
 arch/arm64/kernel/probes/kprobes_trampoline.S |  2 +-
 arch/arm64/kernel/probes/uprobes.c            |  9 +--
 arch/arm64/kernel/traps.c                     | 59 ++++---------------
 10 files changed, 84 insertions(+), 113 deletions(-)

diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
index 21fc85e9d2be..82a76b2102fb 100644
--- a/arch/arm64/include/asm/kgdb.h
+++ b/arch/arm64/include/asm/kgdb.h
@@ -24,6 +24,9 @@ static inline void arch_kgdb_breakpoint(void)
 extern void kgdb_handle_bus_error(void);
 extern int kgdb_fault_expected;
 
+int kgdb_brk_handler(struct pt_regs *regs, unsigned long esr);
+int kgdb_compiled_brk_handler(struct pt_regs *regs, unsigned long esr);
+
 #endif /* !__ASSEMBLY__ */
 
 /*
diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
index be7a3680dadf..b27dd6028e6a 100644
--- a/arch/arm64/include/asm/kprobes.h
+++ b/arch/arm64/include/asm/kprobes.h
@@ -38,6 +38,12 @@ struct kprobe_ctlblk {
 void arch_remove_kprobe(struct kprobe *);
 int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
 void __kretprobe_trampoline(void);
+int __kprobes kprobe_brk_handler(struct pt_regs *regs,
+				 unsigned long esr);
+int __kprobes kprobe_ss_brk_handler(struct pt_regs *regs,
+				 unsigned long esr);
+int __kprobes kretprobe_brk_handler(struct pt_regs *regs,
+				 unsigned long esr);
 void __kprobes *trampoline_probe_handler(struct pt_regs *regs);
 
 #endif /* CONFIG_KPROBES */
diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
index 82cf1f879c61..e3e8944a71c3 100644
--- a/arch/arm64/include/asm/traps.h
+++ b/arch/arm64/include/asm/traps.h
@@ -29,6 +29,12 @@ void arm64_force_sig_fault_pkey(unsigned long far, const char *str, int pkey);
 void arm64_force_sig_mceerr(int code, unsigned long far, short lsb, const char *str);
 void arm64_force_sig_ptrace_errno_trap(int errno, unsigned long far, const char *str);
 
+int bug_brk_handler(struct pt_regs *regs, unsigned long esr);
+int cfi_brk_handler(struct pt_regs *regs, unsigned long esr);
+int reserved_fault_brk_handler(struct pt_regs *regs, unsigned long esr);
+int kasan_brk_handler(struct pt_regs *regs, unsigned long esr);
+int ubsan_brk_handler(struct pt_regs *regs, unsigned long esr);
+
 int early_brk64(unsigned long addr, unsigned long esr, struct pt_regs *regs);
 
 /*
diff --git a/arch/arm64/include/asm/uprobes.h b/arch/arm64/include/asm/uprobes.h
index 014b02897f8e..3659a79a9f32 100644
--- a/arch/arm64/include/asm/uprobes.h
+++ b/arch/arm64/include/asm/uprobes.h
@@ -28,4 +28,6 @@ struct arch_uprobe {
 	bool simulate;
 };
 
+int uprobe_brk_handler(struct pt_regs *regs, unsigned long esr);
+
 #endif
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 676fa0231935..4ece4a93b872 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -21,8 +21,11 @@
 #include <asm/cputype.h>
 #include <asm/daifflags.h>
 #include <asm/debug-monitors.h>
+#include <asm/kgdb.h>
+#include <asm/kprobes.h>
 #include <asm/system_misc.h>
 #include <asm/traps.h>
+#include <asm/uprobes.h>
 
 /* Determine debug architecture. */
 u8 debug_monitors_arch(void)
@@ -299,20 +302,50 @@ void unregister_kernel_break_hook(struct break_hook *hook)
 
 static int call_break_hook(struct pt_regs *regs, unsigned long esr)
 {
-	struct break_hook *hook;
-	struct list_head *list;
-
-	list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
-
-	/*
-	 * Since brk exception disables interrupt, this function is
-	 * entirely not preemptible, and we can use rcu list safely here.
-	 */
-	list_for_each_entry_rcu(hook, list, node) {
-		if ((esr_brk_comment(esr) & ~hook->mask) == hook->imm)
-			return hook->fn(regs, esr);
+	if (user_mode(regs)) {
+#ifdef CONFIG_UPROBES
+		if (esr_brk_comment(esr) == UPROBES_BRK_IMM)
+			return uprobe_brk_handler(regs, esr);
+#endif
+		return DBG_HOOK_ERROR;
 	}
 
+	if (esr_brk_comment(esr) == BUG_BRK_IMM)
+		return bug_brk_handler(regs, esr);
+
+#ifdef CONFIG_CFI_CLANG
+	if (esr_is_cfi_brk(esr))
+		return cfi_brk_handler(regs, esr);
+#endif
+
+	if (esr_brk_comment(esr) == FAULT_BRK_IMM)
+		return reserved_fault_brk_handler(regs, esr);
+
+#ifdef CONFIG_KASAN_SW_TAGS
+	if ((esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
+		return kasan_brk_handler(regs, esr);
+#endif
+#ifdef CONFIG_UBSAN_TRAP
+	if ((esr_brk_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
+		return ubsan_brk_handler(regs, esr);
+#endif
+#ifdef CONFIG_KGDB
+	if (esr_brk_comment(esr) == KGDB_DYN_DBG_BRK_IMM)
+		return kgdb_brk_handler(regs, esr);
+	if (esr_brk_comment(esr) == KGDB_COMPILED_DBG_BRK_IMM)
+		return kgdb_compiled_brk_handler(regs, esr);
+#endif
+#ifdef CONFIG_KPROBES
+	if (esr_brk_comment(esr) == KPROBES_BRK_IMM)
+		return kprobe_brk_handler(regs, esr);
+	if (esr_brk_comment(esr) == KPROBES_BRK_SS_IMM)
+		return kprobe_ss_brk_handler(regs, esr);
+#endif
+#ifdef CONFIG_KRETPROBES
+	if (esr_brk_comment(esr) == KRETPROBES_BRK_IMM)
+		return kretprobe_brk_handler(regs, esr);
+#endif
+
 	return DBG_HOOK_ERROR;
 }
 NOKPROBE_SYMBOL(call_break_hook);
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index f3c4d3a8a20f..b5a3b9c85a65 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -234,21 +234,21 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
 	return err;
 }
 
-static int kgdb_brk_fn(struct pt_regs *regs, unsigned long esr)
+int kgdb_brk_handler(struct pt_regs *regs, unsigned long esr)
 {
 	kgdb_handle_exception(1, SIGTRAP, 0, regs);
 	return DBG_HOOK_HANDLED;
 }
-NOKPROBE_SYMBOL(kgdb_brk_fn)
+NOKPROBE_SYMBOL(kgdb_brk_handler)
 
-static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned long esr)
+int kgdb_compiled_brk_handler(struct pt_regs *regs, unsigned long esr)
 {
 	compiled_break = 1;
 	kgdb_handle_exception(1, SIGTRAP, 0, regs);
 
 	return DBG_HOOK_HANDLED;
 }
-NOKPROBE_SYMBOL(kgdb_compiled_brk_fn);
+NOKPROBE_SYMBOL(kgdb_compiled_brk_handler);
 
 static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned long esr)
 {
@@ -260,16 +260,6 @@ static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned long esr)
 }
 NOKPROBE_SYMBOL(kgdb_step_brk_fn);
 
-static struct break_hook kgdb_brkpt_hook = {
-	.fn		= kgdb_brk_fn,
-	.imm		= KGDB_DYN_DBG_BRK_IMM,
-};
-
-static struct break_hook kgdb_compiled_brkpt_hook = {
-	.fn		= kgdb_compiled_brk_fn,
-	.imm		= KGDB_COMPILED_DBG_BRK_IMM,
-};
-
 static struct step_hook kgdb_step_hook = {
 	.fn		= kgdb_step_brk_fn
 };
@@ -316,8 +306,6 @@ int kgdb_arch_init(void)
 	if (ret != 0)
 		return ret;
 
-	register_kernel_break_hook(&kgdb_brkpt_hook);
-	register_kernel_break_hook(&kgdb_compiled_brkpt_hook);
 	register_kernel_step_hook(&kgdb_step_hook);
 	return 0;
 }
@@ -329,8 +317,6 @@ int kgdb_arch_init(void)
  */
 void kgdb_arch_exit(void)
 {
-	unregister_kernel_break_hook(&kgdb_brkpt_hook);
-	unregister_kernel_break_hook(&kgdb_compiled_brkpt_hook);
 	unregister_kernel_step_hook(&kgdb_step_hook);
 	unregister_die_notifier(&kgdb_notifier);
 }
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index d9e462eafb95..0c5d408afd95 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -292,8 +292,8 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
 	return 0;
 }
 
-static int __kprobes
-kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
+int __kprobes
+kprobe_brk_handler(struct pt_regs *regs, unsigned long esr)
 {
 	struct kprobe *p, *cur_kprobe;
 	struct kprobe_ctlblk *kcb;
@@ -336,13 +336,8 @@ kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
 	return DBG_HOOK_HANDLED;
 }
 
-static struct break_hook kprobes_break_hook = {
-	.imm = KPROBES_BRK_IMM,
-	.fn = kprobe_breakpoint_handler,
-};
-
-static int __kprobes
-kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned long esr)
+int __kprobes
+kprobe_ss_brk_handler(struct pt_regs *regs, unsigned long esr)
 {
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 	unsigned long addr = instruction_pointer(regs);
@@ -360,13 +355,8 @@ kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned long esr)
 	return DBG_HOOK_ERROR;
 }
 
-static struct break_hook kprobes_break_ss_hook = {
-	.imm = KPROBES_BRK_SS_IMM,
-	.fn = kprobe_breakpoint_ss_handler,
-};
-
-static int __kprobes
-kretprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
+int __kprobes
+kretprobe_brk_handler(struct pt_regs *regs, unsigned long esr)
 {
 	if (regs->pc != (unsigned long)__kretprobe_trampoline)
 		return DBG_HOOK_ERROR;
@@ -375,11 +365,6 @@ kretprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
 	return DBG_HOOK_HANDLED;
 }
 
-static struct break_hook kretprobes_break_hook = {
-	.imm = KRETPROBES_BRK_IMM,
-	.fn = kretprobe_breakpoint_handler,
-};
-
 /*
  * Provide a blacklist of symbols identifying ranges which cannot be kprobed.
  * This blacklist is exposed to userspace via debugfs (kprobes/blacklist).
@@ -422,9 +407,5 @@ int __kprobes arch_trampoline_kprobe(struct kprobe *p)
 
 int __init arch_init_kprobes(void)
 {
-	register_kernel_break_hook(&kprobes_break_hook);
-	register_kernel_break_hook(&kprobes_break_ss_hook);
-	register_kernel_break_hook(&kretprobes_break_hook);
-
 	return 0;
 }
diff --git a/arch/arm64/kernel/probes/kprobes_trampoline.S b/arch/arm64/kernel/probes/kprobes_trampoline.S
index a362f3dbb3d1..b60739d3983f 100644
--- a/arch/arm64/kernel/probes/kprobes_trampoline.S
+++ b/arch/arm64/kernel/probes/kprobes_trampoline.S
@@ -12,7 +12,7 @@
 SYM_CODE_START(__kretprobe_trampoline)
 	/*
 	 * Trigger a breakpoint exception. The PC will be adjusted by
-	 * kretprobe_breakpoint_handler(), and no subsequent instructions will
+	 * kretprobe_brk_handler(), and no subsequent instructions will
 	 * be executed from the trampoline.
 	 */
 	brk #KRETPROBES_BRK_IMM
diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index cb3d05af36e3..ad68b4a5974d 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -173,7 +173,7 @@ int arch_uprobe_exception_notify(struct notifier_block *self,
 	return NOTIFY_DONE;
 }
 
-static int uprobe_breakpoint_handler(struct pt_regs *regs,
+int uprobe_brk_handler(struct pt_regs *regs,
 				     unsigned long esr)
 {
 	if (uprobe_pre_sstep_notifier(regs))
@@ -194,12 +194,6 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
 	return DBG_HOOK_ERROR;
 }
 
-/* uprobe breakpoint handler hook */
-static struct break_hook uprobes_break_hook = {
-	.imm = UPROBES_BRK_IMM,
-	.fn = uprobe_breakpoint_handler,
-};
-
 /* uprobe single step handler hook */
 static struct step_hook uprobes_step_hook = {
 	.fn = uprobe_single_step_handler,
@@ -207,7 +201,6 @@ static struct step_hook uprobes_step_hook = {
 
 static int __init arch_init_uprobes(void)
 {
-	register_user_break_hook(&uprobes_break_hook);
 	register_user_step_hook(&uprobes_step_hook);
 
 	return 0;
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 529cff825531..50d7a6a75f45 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -987,7 +987,7 @@ void do_serror(struct pt_regs *regs, unsigned long esr)
 int is_valid_bugaddr(unsigned long addr)
 {
 	/*
-	 * bug_handler() only called for BRK #BUG_BRK_IMM.
+	 * bug_brk_handler() only called for BRK #BUG_BRK_IMM.
 	 * So the answer is trivial -- any spurious instances with no
 	 * bug table entry will be rejected by report_bug() and passed
 	 * back to the debug-monitors code and handled as a fatal
@@ -997,7 +997,7 @@ int is_valid_bugaddr(unsigned long addr)
 }
 #endif
 
-static int bug_handler(struct pt_regs *regs, unsigned long esr)
+int bug_brk_handler(struct pt_regs *regs, unsigned long esr)
 {
 	switch (report_bug(regs->pc, regs)) {
 	case BUG_TRAP_TYPE_BUG:
@@ -1017,13 +1017,8 @@ static int bug_handler(struct pt_regs *regs, unsigned long esr)
 	return DBG_HOOK_HANDLED;
 }
 
-static struct break_hook bug_break_hook = {
-	.fn = bug_handler,
-	.imm = BUG_BRK_IMM,
-};
-
 #ifdef CONFIG_CFI_CLANG
-static int cfi_handler(struct pt_regs *regs, unsigned long esr)
+int cfi_brk_handler(struct pt_regs *regs, unsigned long esr)
 {
 	unsigned long target;
 	u32 type;
@@ -1046,15 +1041,9 @@ static int cfi_handler(struct pt_regs *regs, unsigned long esr)
 	arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);
 	return DBG_HOOK_HANDLED;
 }
-
-static struct break_hook cfi_break_hook = {
-	.fn = cfi_handler,
-	.imm = CFI_BRK_IMM_BASE,
-	.mask = CFI_BRK_IMM_MASK,
-};
 #endif /* CONFIG_CFI_CLANG */
 
-static int reserved_fault_handler(struct pt_regs *regs, unsigned long esr)
+int reserved_fault_brk_handler(struct pt_regs *regs, unsigned long esr)
 {
 	pr_err("%s generated an invalid instruction at %pS!\n",
 		"Kernel text patching",
@@ -1064,11 +1053,6 @@ static int reserved_fault_handler(struct pt_regs *regs, unsigned long esr)
 	return DBG_HOOK_ERROR;
 }
 
-static struct break_hook fault_break_hook = {
-	.fn = reserved_fault_handler,
-	.imm = FAULT_BRK_IMM,
-};
-
 #ifdef CONFIG_KASAN_SW_TAGS
 
 #define KASAN_ESR_RECOVER	0x20
@@ -1076,7 +1060,7 @@ static struct break_hook fault_break_hook = {
 #define KASAN_ESR_SIZE_MASK	0x0f
 #define KASAN_ESR_SIZE(esr)	(1 << ((esr) & KASAN_ESR_SIZE_MASK))
 
-static int kasan_handler(struct pt_regs *regs, unsigned long esr)
+int kasan_brk_handler(struct pt_regs *regs, unsigned long esr)
 {
 	bool recover = esr & KASAN_ESR_RECOVER;
 	bool write = esr & KASAN_ESR_WRITE;
@@ -1107,26 +1091,14 @@ static int kasan_handler(struct pt_regs *regs, unsigned long esr)
 	arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);
 	return DBG_HOOK_HANDLED;
 }
-
-static struct break_hook kasan_break_hook = {
-	.fn	= kasan_handler,
-	.imm	= KASAN_BRK_IMM,
-	.mask	= KASAN_BRK_MASK,
-};
 #endif
 
 #ifdef CONFIG_UBSAN_TRAP
-static int ubsan_handler(struct pt_regs *regs, unsigned long esr)
+int ubsan_brk_handler(struct pt_regs *regs, unsigned long esr)
 {
 	die(report_ubsan_failure(regs, esr & UBSAN_BRK_MASK), regs, esr);
 	return DBG_HOOK_HANDLED;
 }
-
-static struct break_hook ubsan_break_hook = {
-	.fn	= ubsan_handler,
-	.imm	= UBSAN_BRK_IMM,
-	.mask	= UBSAN_BRK_MASK,
-};
 #endif
 
 /*
@@ -1138,31 +1110,20 @@ int __init early_brk64(unsigned long addr, unsigned long esr,
 {
 #ifdef CONFIG_CFI_CLANG
 	if (esr_is_cfi_brk(esr))
-		return cfi_handler(regs, esr) != DBG_HOOK_HANDLED;
+		return cfi_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
 #endif
 #ifdef CONFIG_KASAN_SW_TAGS
 	if ((esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
-		return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
+		return kasan_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
 #endif
 #ifdef CONFIG_UBSAN_TRAP
 	if ((esr_brk_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
-		return ubsan_handler(regs, esr) != DBG_HOOK_HANDLED;
+		return ubsan_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
 #endif
-	return bug_handler(regs, esr) != DBG_HOOK_HANDLED;
+	return bug_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
 }
 
 void __init trap_init(void)
 {
-	register_kernel_break_hook(&bug_break_hook);
-#ifdef CONFIG_CFI_CLANG
-	register_kernel_break_hook(&cfi_break_hook);
-#endif
-	register_kernel_break_hook(&fault_break_hook);
-#ifdef CONFIG_KASAN_SW_TAGS
-	register_kernel_break_hook(&kasan_break_hook);
-#endif
-#ifdef CONFIG_UBSAN_TRAP
-	register_kernel_break_hook(&ubsan_break_hook);
-#endif
 	debug_traps_init();
 }
-- 
2.43.0

[PATCH v2 03/11] arm64: debug: call step handlers statically

[Ada Couprie Diaz]

Software stepping checks for the correct handler by iterating over a list
of dynamically registered handlers and calling all of them until one
handles the exception.

This is the only generic way to handle software stepping handlers in arm64
as the exception does not provide an immediate that could be checked,
contrary to software breakpoints.

However, the registration mechanism is not exported and has only
two current users : the KGDB stepping handler, and the uprobe single step
handler.
Given that one comes from user mode and the other from kernel mode, call
the appropriate one by checking the source EL of the exception, if it
is enabled.

Unify the naming of the handler to XXX_singlestep_handler(), making it
clear they are related.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/kgdb.h      |  1 +
 arch/arm64/include/asm/uprobes.h   |  1 +
 arch/arm64/kernel/debug-monitors.c | 32 +++++++++++++-----------------
 arch/arm64/kernel/kgdb.c           | 17 +++-------------
 arch/arm64/kernel/probes/uprobes.c |  9 +--------
 5 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
index 82a76b2102fb..fd287ec38bb7 100644
--- a/arch/arm64/include/asm/kgdb.h
+++ b/arch/arm64/include/asm/kgdb.h
@@ -26,6 +26,7 @@ extern int kgdb_fault_expected;
 
 int kgdb_brk_handler(struct pt_regs *regs, unsigned long esr);
 int kgdb_compiled_brk_handler(struct pt_regs *regs, unsigned long esr);
+int kgdb_singlestep_handler(struct pt_regs *regs, unsigned long esr);
 
 #endif /* !__ASSEMBLY__ */
 
diff --git a/arch/arm64/include/asm/uprobes.h b/arch/arm64/include/asm/uprobes.h
index 3659a79a9f32..e44bbef40eca 100644
--- a/arch/arm64/include/asm/uprobes.h
+++ b/arch/arm64/include/asm/uprobes.h
@@ -29,5 +29,6 @@ struct arch_uprobe {
 };
 
 int uprobe_brk_handler(struct pt_regs *regs, unsigned long esr);
+int uprobe_singlestep_handler(struct pt_regs *regs, unsigned long esr);
 
 #endif
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 4ece4a93b872..81b813e16842 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -200,30 +200,26 @@ void unregister_kernel_step_hook(struct step_hook *hook)
 }
 
 /*
- * Call registered single step handlers
+ * Call single step handlers
  * There is no Syndrome info to check for determining the handler.
- * So we call all the registered handlers, until the right handler is
- * found which returns zero.
+ * However, there is only one possible handler for user and kernel modes, so
+ * check and call the appropriate one if it is enabled.
  */
 static int call_step_hook(struct pt_regs *regs, unsigned long esr)
 {
-	struct step_hook *hook;
-	struct list_head *list;
-	int retval = DBG_HOOK_ERROR;
-
-	list = user_mode(regs) ? &user_step_hook : &kernel_step_hook;
-
-	/*
-	 * Since single-step exception disables interrupt, this function is
-	 * entirely not preemptible, and we can use rcu list safely here.
-	 */
-	list_for_each_entry_rcu(hook, list, node)	{
-		retval = hook->fn(regs, esr);
-		if (retval == DBG_HOOK_HANDLED)
-			break;
+	if (user_mode(regs)) {
+#if CONFIG_UPROBES
+		return uprobe_singlestep_handler(regs, esr);
+#else
+		return DBG_HOOK_ERROR;
+#endif
 	}
 
-	return retval;
+#ifdef CONFIG_KGDB
+	return kgdb_singlestep_handler(regs, esr);
+#else
+	return DBG_HOOK_ERROR;
+#endif
 }
 NOKPROBE_SYMBOL(call_step_hook);
 
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index b5a3b9c85a65..8f6ce2ea005c 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -250,7 +250,7 @@ int kgdb_compiled_brk_handler(struct pt_regs *regs, unsigned long esr)
 }
 NOKPROBE_SYMBOL(kgdb_compiled_brk_handler);
 
-static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned long esr)
+int kgdb_singlestep_handler(struct pt_regs *regs, unsigned long esr)
 {
 	if (!kgdb_single_step)
 		return DBG_HOOK_ERROR;
@@ -258,11 +258,7 @@ static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned long esr)
 	kgdb_handle_exception(0, SIGTRAP, 0, regs);
 	return DBG_HOOK_HANDLED;
 }
-NOKPROBE_SYMBOL(kgdb_step_brk_fn);
-
-static struct step_hook kgdb_step_hook = {
-	.fn		= kgdb_step_brk_fn
-};
+NOKPROBE_SYMBOL(kgdb_singlestep_handler);
 
 static int __kgdb_notify(struct die_args *args, unsigned long cmd)
 {
@@ -301,13 +297,7 @@ static struct notifier_block kgdb_notifier = {
  */
 int kgdb_arch_init(void)
 {
-	int ret = register_die_notifier(&kgdb_notifier);
-
-	if (ret != 0)
-		return ret;
-
-	register_kernel_step_hook(&kgdb_step_hook);
-	return 0;
+	return register_die_notifier(&kgdb_notifier);
 }
 
 /*
@@ -317,7 +307,6 @@ int kgdb_arch_init(void)
  */
 void kgdb_arch_exit(void)
 {
-	unregister_kernel_step_hook(&kgdb_step_hook);
 	unregister_die_notifier(&kgdb_notifier);
 }
 
diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index ad68b4a5974d..fefc990860bc 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -182,7 +182,7 @@ int uprobe_brk_handler(struct pt_regs *regs,
 	return DBG_HOOK_ERROR;
 }
 
-static int uprobe_single_step_handler(struct pt_regs *regs,
+int uprobe_singlestep_handler(struct pt_regs *regs,
 				      unsigned long esr)
 {
 	struct uprobe_task *utask = current->utask;
@@ -194,15 +194,8 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
 	return DBG_HOOK_ERROR;
 }
 
-/* uprobe single step handler hook */
-static struct step_hook uprobes_step_hook = {
-	.fn = uprobe_single_step_handler,
-};
-
 static int __init arch_init_uprobes(void)
 {
-	register_user_step_hook(&uprobes_step_hook);
-
 	return 0;
 }
 
-- 
2.43.0

[PATCH v2 04/11] arm64: debug: remove break/step handler registration infrastructure

[Ada Couprie Diaz]

Remove all infrastructure for the dynamic registration previously used by
software breakpoints and stepping handlers.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/debug-monitors.h | 24 ----------
 arch/arm64/kernel/debug-monitors.c      | 63 -------------------------
 2 files changed, 87 deletions(-)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 8f6ba31b8658..9aabf65de693 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -62,30 +62,6 @@ struct task_struct;
 #define DBG_HOOK_HANDLED	0
 #define DBG_HOOK_ERROR		1
 
-struct step_hook {
-	struct list_head node;
-	int (*fn)(struct pt_regs *regs, unsigned long esr);
-};
-
-void register_user_step_hook(struct step_hook *hook);
-void unregister_user_step_hook(struct step_hook *hook);
-
-void register_kernel_step_hook(struct step_hook *hook);
-void unregister_kernel_step_hook(struct step_hook *hook);
-
-struct break_hook {
-	struct list_head node;
-	int (*fn)(struct pt_regs *regs, unsigned long esr);
-	u16 imm;
-	u16 mask; /* These bits are ignored when comparing with imm */
-};
-
-void register_user_break_hook(struct break_hook *hook);
-void unregister_user_break_hook(struct break_hook *hook);
-
-void register_kernel_break_hook(struct break_hook *hook);
-void unregister_kernel_break_hook(struct break_hook *hook);
-
 u8 debug_monitors_arch(void);
 
 enum dbg_active_el {
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 81b813e16842..55bca019ef5c 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -159,46 +159,6 @@ NOKPROBE_SYMBOL(clear_user_regs_spsr_ss);
 #define set_regs_spsr_ss(r)	set_user_regs_spsr_ss(&(r)->user_regs)
 #define clear_regs_spsr_ss(r)	clear_user_regs_spsr_ss(&(r)->user_regs)
 
-static DEFINE_SPINLOCK(debug_hook_lock);
-static LIST_HEAD(user_step_hook);
-static LIST_HEAD(kernel_step_hook);
-
-static void register_debug_hook(struct list_head *node, struct list_head *list)
-{
-	spin_lock(&debug_hook_lock);
-	list_add_rcu(node, list);
-	spin_unlock(&debug_hook_lock);
-
-}
-
-static void unregister_debug_hook(struct list_head *node)
-{
-	spin_lock(&debug_hook_lock);
-	list_del_rcu(node);
-	spin_unlock(&debug_hook_lock);
-	synchronize_rcu();
-}
-
-void register_user_step_hook(struct step_hook *hook)
-{
-	register_debug_hook(&hook->node, &user_step_hook);
-}
-
-void unregister_user_step_hook(struct step_hook *hook)
-{
-	unregister_debug_hook(&hook->node);
-}
-
-void register_kernel_step_hook(struct step_hook *hook)
-{
-	register_debug_hook(&hook->node, &kernel_step_hook);
-}
-
-void unregister_kernel_step_hook(struct step_hook *hook)
-{
-	unregister_debug_hook(&hook->node);
-}
-
 /*
  * Call single step handlers
  * There is no Syndrome info to check for determining the handler.
@@ -273,29 +233,6 @@ static int single_step_handler(unsigned long unused, unsigned long esr,
 }
 NOKPROBE_SYMBOL(single_step_handler);
 
-static LIST_HEAD(user_break_hook);
-static LIST_HEAD(kernel_break_hook);
-
-void register_user_break_hook(struct break_hook *hook)
-{
-	register_debug_hook(&hook->node, &user_break_hook);
-}
-
-void unregister_user_break_hook(struct break_hook *hook)
-{
-	unregister_debug_hook(&hook->node);
-}
-
-void register_kernel_break_hook(struct break_hook *hook)
-{
-	register_debug_hook(&hook->node, &kernel_break_hook);
-}
-
-void unregister_kernel_break_hook(struct break_hook *hook)
-{
-	unregister_debug_hook(&hook->node);
-}
-
 static int call_break_hook(struct pt_regs *regs, unsigned long esr)
 {
 	if (user_mode(regs)) {
-- 
2.43.0

[PATCH v2 05/11] arm64: entry: Add entry and exit functions for debug exceptions

[Ada Couprie Diaz]

Duplicate the `debug_exception_entry()` and `debug_exception_exit()`
functions from mm/fault.c, as they are needed to split
the debug exceptions entry paths from the current unified one.
The original will be removed in a cleanup patch.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/kernel/entry-common.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index b260ddc4d3e9..92d78b329e67 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -418,6 +418,28 @@ static inline void fp_user_discard(void)
 	}
 }
 
+/*
+ * In debug exception context, we explicitly disable preemption despite
+ * having interrupts disabled.
+ * This serves two purposes: it makes it much less likely that we would
+ * accidentally schedule in exception context and it will force a warning
+ * if we somehow manage to schedule by accident.
+ */
+static void debug_exception_enter(struct pt_regs *regs)
+{
+	preempt_disable();
+
+	/* This code is a bit fragile.  Test it. */
+	RCU_LOCKDEP_WARN(!rcu_is_watching(), "exception_enter didn't work");
+}
+NOKPROBE_SYMBOL(debug_exception_enter);
+
+static void debug_exception_exit(struct pt_regs *regs)
+{
+	preempt_enable_no_resched();
+}
+NOKPROBE_SYMBOL(debug_exception_exit);
+
 UNHANDLED(el1t, 64, sync)
 UNHANDLED(el1t, 64, irq)
 UNHANDLED(el1t, 64, fiq)
-- 
2.43.0

[PATCH v2 06/11] arm64: debug: split hardware breakpoint exeception entry

[Ada Couprie Diaz]

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.

Hardware breakpoints exceptions are generated by the hardware after user
configuration. As such, they can be exploited when training branch
predictors outisde of the userspace VA range: they still need to call
`arm64_apply_bp_hardening()` if needed to mitigate against this attack.
Move the call to `arm64_apply_bp_hardening()` to `entry-common.c` as
it is needed for exceptions coming from EL0 only.

However, they do not need to handle the Cortex-A76 erratum #1463225 as
it only applies to single stepping exceptions.
It does not set an address in FAR_EL1 either, only the hardware
watchpoint does.

Split the hardware breakpoint exception entry, adjust
the function signature, and handling of the Cortex-A76 erratum to fit
the behaviour of the exception.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/exception.h |  1 +
 arch/arm64/kernel/entry-common.c   | 28 ++++++++++++++++++++++++++++
 arch/arm64/kernel/hw_breakpoint.c  | 15 +++++++++++----
 3 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index d48fc16584cd..c593fe639697 100644
--- a/arch/arm64/include/asm/exception.h
+++ b/arch/arm64/include/asm/exception.h
@@ -61,6 +61,7 @@ void do_el0_gcs(struct pt_regs *regs, unsigned long esr);
 void do_el1_gcs(struct pt_regs *regs, unsigned long esr);
 void do_debug_exception(unsigned long addr_if_watchpoint, unsigned long esr,
 			struct pt_regs *regs);
+void do_breakpoint(unsigned long esr, struct pt_regs *regs);
 void do_fpsimd_acc(unsigned long esr, struct pt_regs *regs);
 void do_sve_acc(unsigned long esr, struct pt_regs *regs);
 void do_sme_acc(unsigned long esr, struct pt_regs *regs);
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 92d78b329e67..6ff52fc94da7 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -503,6 +503,15 @@ static void noinstr el1_mops(struct pt_regs *regs, unsigned long esr)
 	exit_to_kernel_mode(regs);
 }
 
+static void noinstr el1_breakpt(struct pt_regs *regs, unsigned long esr)
+{
+	arm64_enter_el1_dbg(regs);
+	debug_exception_enter(regs);
+	do_breakpoint(esr, regs);
+	debug_exception_exit(regs);
+	arm64_exit_el1_dbg(regs);
+}
+
 static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
 {
 	unsigned long far = read_sysreg(far_el1);
@@ -552,6 +561,8 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
 		el1_mops(regs, esr);
 		break;
 	case ESR_ELx_EC_BREAKPT_CUR:
+		el1_breakpt(regs, esr);
+		break;
 	case ESR_ELx_EC_SOFTSTP_CUR:
 	case ESR_ELx_EC_WATCHPT_CUR:
 	case ESR_ELx_EC_BRK64:
@@ -746,6 +757,19 @@ static void noinstr el0_inv(struct pt_regs *regs, unsigned long esr)
 	exit_to_user_mode(regs);
 }
 
+static void noinstr el0_breakpt(struct pt_regs *regs, unsigned long esr)
+{
+	if (!is_ttbr0_addr(regs->pc))
+		arm64_apply_bp_hardening();
+
+	enter_from_user_mode(regs);
+	debug_exception_enter(regs);
+	do_breakpoint(esr, regs);
+	debug_exception_exit(regs);
+	local_daif_restore(DAIF_PROCCTX);
+	exit_to_user_mode(regs);
+}
+
 static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
 {
 	/* Only watchpoints write FAR_EL1, otherwise its UNKNOWN */
@@ -824,6 +848,8 @@ asmlinkage void noinstr el0t_64_sync_handler(struct pt_regs *regs)
 		el0_gcs(regs, esr);
 		break;
 	case ESR_ELx_EC_BREAKPT_LOW:
+		el0_breakpt(regs, esr);
+		break;
 	case ESR_ELx_EC_SOFTSTP_LOW:
 	case ESR_ELx_EC_WATCHPT_LOW:
 	case ESR_ELx_EC_BRK64:
@@ -944,6 +970,8 @@ asmlinkage void noinstr el0t_32_sync_handler(struct pt_regs *regs)
 		el0_cp15(regs, esr);
 		break;
 	case ESR_ELx_EC_BREAKPT_LOW:
+		el0_breakpt(regs, esr);
+		break;
 	case ESR_ELx_EC_SOFTSTP_LOW:
 	case ESR_ELx_EC_WATCHPT_LOW:
 	case ESR_ELx_EC_BKPT32:
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index 722ac45f9f7b..b7253ddac230 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -22,6 +22,7 @@
 #include <asm/current.h>
 #include <asm/debug-monitors.h>
 #include <asm/esr.h>
+#include <asm/exception.h>
 #include <asm/hw_breakpoint.h>
 #include <asm/traps.h>
 #include <asm/cputype.h>
@@ -618,8 +619,7 @@ NOKPROBE_SYMBOL(toggle_bp_registers);
 /*
  * Debug exception handlers.
  */
-static int breakpoint_handler(unsigned long unused, unsigned long esr,
-			      struct pt_regs *regs)
+static int breakpoint_handler(unsigned long esr, struct pt_regs *regs)
 {
 	int i, step = 0, *kernel_step;
 	u32 ctrl_reg;
@@ -695,6 +695,15 @@ static int breakpoint_handler(unsigned long unused, unsigned long esr,
 }
 NOKPROBE_SYMBOL(breakpoint_handler);
 
+void do_breakpoint(unsigned long esr, struct pt_regs *regs)
+{
+	if (breakpoint_handler(esr, regs)) {
+		arm64_notify_die("hw-breakpoint handler", regs, SIGTRAP, TRAP_HWBKPT,
+					regs->pc, esr);
+	}
+}
+NOKPROBE_SYMBOL(do_breakpoint);
+
 /*
  * Arm64 hardware does not always report a watchpoint hit address that matches
  * one of the watchpoints set. It can also report an address "near" the
@@ -988,8 +997,6 @@ static int __init arch_hw_breakpoint_init(void)
 		core_num_brps, core_num_wrps);
 
 	/* Register debug fault handlers. */
-	hook_debug_fault_code(DBG_ESR_EVT_HWBP, breakpoint_handler, SIGTRAP,
-			      TRAP_HWBKPT, "hw-breakpoint handler");
 	hook_debug_fault_code(DBG_ESR_EVT_HWWP, watchpoint_handler, SIGTRAP,
 			      TRAP_HWBKPT, "hw-watchpoint handler");
 
-- 
2.43.0

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

[Ada Couprie Diaz]

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(-)

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index c593fe639697..cbcd832bf58e 100644
--- a/arch/arm64/include/asm/exception.h
+++ b/arch/arm64/include/asm/exception.h
@@ -62,6 +62,7 @@ void do_el1_gcs(struct pt_regs *regs, unsigned long esr);
 void do_debug_exception(unsigned long addr_if_watchpoint, unsigned long esr,
 			struct pt_regs *regs);
 void do_breakpoint(unsigned long esr, struct pt_regs *regs);
+void do_softstep(unsigned long esr, struct pt_regs *regs);
 void do_fpsimd_acc(unsigned long esr, struct pt_regs *regs);
 void do_sve_acc(unsigned long esr, struct pt_regs *regs);
 void do_sme_acc(unsigned long esr, struct pt_regs *regs);
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 55bca019ef5c..d3cb8f5da51b 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -21,6 +21,7 @@
 #include <asm/cputype.h>
 #include <asm/daifflags.h>
 #include <asm/debug-monitors.h>
+#include <asm/exception.h>
 #include <asm/kgdb.h>
 #include <asm/kprobes.h>
 #include <asm/system_misc.h>
@@ -197,18 +198,10 @@ static void send_user_sigtrap(int si_code)
 			      "User debug trap");
 }
 
-static int single_step_handler(unsigned long unused, unsigned long esr,
-			       struct pt_regs *regs)
+void do_softstep(unsigned long esr, struct pt_regs *regs)
 {
-	/*
-	 * If we are stepping a pending breakpoint, call the hw_breakpoint
-	 * handler first.
-	 */
-	if (!reinstall_suspended_bps(regs))
-		return 0;
-
 	if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
-		return 0;
+		return;
 
 	if (user_mode(regs)) {
 		send_user_sigtrap(TRAP_TRACE);
@@ -228,10 +221,8 @@ static int single_step_handler(unsigned long unused, unsigned long esr,
 		 */
 		set_regs_spsr_ss(regs);
 	}
-
-	return 0;
 }
-NOKPROBE_SYMBOL(single_step_handler);
+NOKPROBE_SYMBOL(do_softstep);
 
 static int call_break_hook(struct pt_regs *regs, unsigned long esr)
 {
@@ -341,8 +332,6 @@ NOKPROBE_SYMBOL(aarch32_break_handler);
 
 void __init debug_traps_init(void)
 {
-	hook_debug_fault_code(DBG_ESR_EVT_HWSS, single_step_handler, SIGTRAP,
-			      TRAP_TRACE, "single-step handler");
 	hook_debug_fault_code(DBG_ESR_EVT_BRK, brk_handler, SIGTRAP,
 			      TRAP_BRKPT, "BRK handler");
 }
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 6ff52fc94da7..8814ad24e707 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -512,6 +512,24 @@ static void noinstr el1_breakpt(struct pt_regs *regs, unsigned long esr)
 	arm64_exit_el1_dbg(regs);
 }
 
+static void noinstr el1_softstp(struct pt_regs *regs, unsigned long esr)
+{
+	arm64_enter_el1_dbg(regs);
+	if (!cortex_a76_erratum_1463225_debug_handler(regs)) {
+		debug_exception_enter(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))
+			do_softstep(esr, regs);
+		debug_exception_exit(regs);
+	}
+	arm64_exit_el1_dbg(regs);
+}
+
 static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
 {
 	unsigned long far = read_sysreg(far_el1);
@@ -564,6 +582,8 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
 		el1_breakpt(regs, esr);
 		break;
 	case ESR_ELx_EC_SOFTSTP_CUR:
+		el1_softstp(regs, esr);
+		break;
 	case ESR_ELx_EC_WATCHPT_CUR:
 	case ESR_ELx_EC_BRK64:
 		el1_dbg(regs, esr);
@@ -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();
+
+	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);
+}
+
 static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
 {
 	/* Only watchpoints write FAR_EL1, otherwise its UNKNOWN */
@@ -851,6 +890,8 @@ asmlinkage void noinstr el0t_64_sync_handler(struct pt_regs *regs)
 		el0_breakpt(regs, esr);
 		break;
 	case ESR_ELx_EC_SOFTSTP_LOW:
+		el0_softstp(regs, esr);
+		break;
 	case ESR_ELx_EC_WATCHPT_LOW:
 	case ESR_ELx_EC_BRK64:
 		el0_dbg(regs, esr);
@@ -973,6 +1014,8 @@ asmlinkage void noinstr el0t_32_sync_handler(struct pt_regs *regs)
 		el0_breakpt(regs, esr);
 		break;
 	case ESR_ELx_EC_SOFTSTP_LOW:
+		el0_softstp(regs, esr);
+		break;
 	case ESR_ELx_EC_WATCHPT_LOW:
 	case ESR_ELx_EC_BKPT32:
 		el0_dbg(regs, esr);
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index b7253ddac230..1ab37d6561cb 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -866,9 +866,9 @@ int reinstall_suspended_bps(struct pt_regs *regs)
 	kernel_step = this_cpu_ptr(&stepping_kernel_bp);
 
 	/*
-	 * Called from single-step exception handler.
-	 * Return 0 if execution can resume, 1 if a SIGTRAP should be
-	 * reported.
+	 * Called from single-step exception entry.
+	 * Return 0 if execution can resume, 1 if the single step
+	 * should be handled.
 	 */
 	if (user_mode(regs)) {
 		if (debug_info->bps_disabled) {
-- 
2.43.0

[PATCH v2 08/11] arm64: debug: split hardware watchpoint exception entry

[Ada Couprie Diaz]

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.

Hardware watchpoints are the only debug exceptions that will write
FAR_EL1, so we need to preserve it and pass it down.
However, they cannot be used to maliciously train branch predictors, so
we can omit calling `arm64_bp_hardening()`, nor do they need to handle
the Cortex-A76 erratum #1463225, as it only applies to single stepping
exceptions.

Split the hardware watchpoint exception entry and adjust the behaviour
to match the lack of needed mitigations.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/exception.h |  2 ++
 arch/arm64/kernel/entry-common.c   | 31 +++++++++++++++++++++++++++++-
 arch/arm64/kernel/hw_breakpoint.c  | 14 ++++++++++----
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index cbcd832bf58e..eb465c375d34 100644
--- a/arch/arm64/include/asm/exception.h
+++ b/arch/arm64/include/asm/exception.h
@@ -63,6 +63,8 @@ void do_debug_exception(unsigned long addr_if_watchpoint, unsigned long esr,
 			struct pt_regs *regs);
 void do_breakpoint(unsigned long esr, struct pt_regs *regs);
 void do_softstep(unsigned long esr, struct pt_regs *regs);
+void do_watchpoint(unsigned long addr, unsigned long esr,
+			struct pt_regs *regs);
 void do_fpsimd_acc(unsigned long esr, struct pt_regs *regs);
 void do_sve_acc(unsigned long esr, struct pt_regs *regs);
 void do_sme_acc(unsigned long esr, struct pt_regs *regs);
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 8814ad24e707..6e70130d2741 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -530,10 +530,20 @@ static void noinstr el1_softstp(struct pt_regs *regs, unsigned long esr)
 	arm64_exit_el1_dbg(regs);
 }
 
-static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
+static void noinstr el1_watchpt(struct pt_regs *regs, unsigned long esr)
 {
+	/* Only watchpoints write FAR_EL1 */
 	unsigned long far = read_sysreg(far_el1);
 
+	arm64_enter_el1_dbg(regs);
+	debug_exception_enter(regs);
+	do_watchpoint(far, esr, regs);
+	debug_exception_exit(regs);
+	arm64_exit_el1_dbg(regs);
+}
+
+static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
+{
 	arm64_enter_el1_dbg(regs);
 	if (!cortex_a76_erratum_1463225_debug_handler(regs))
 		do_debug_exception(far, esr, regs);
@@ -585,6 +595,8 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
 		el1_softstp(regs, esr);
 		break;
 	case ESR_ELx_EC_WATCHPT_CUR:
+		el1_watchpt(regs, esr);
+		break;
 	case ESR_ELx_EC_BRK64:
 		el1_dbg(regs, esr);
 		break;
@@ -809,6 +821,19 @@ static void noinstr el0_softstp(struct pt_regs *regs, unsigned long esr)
 	exit_to_user_mode(regs);
 }
 
+static void noinstr el0_watchpt(struct pt_regs *regs, unsigned long esr)
+{
+	/* Only watchpoints write FAR_EL1 */
+	unsigned long far = read_sysreg(far_el1);
+
+	enter_from_user_mode(regs);
+	debug_exception_enter(regs);
+	do_watchpoint(far, esr, regs);
+	debug_exception_exit(regs);
+	local_daif_restore(DAIF_PROCCTX);
+	exit_to_user_mode(regs);
+}
+
 static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
 {
 	/* Only watchpoints write FAR_EL1, otherwise its UNKNOWN */
@@ -893,6 +918,8 @@ asmlinkage void noinstr el0t_64_sync_handler(struct pt_regs *regs)
 		el0_softstp(regs, esr);
 		break;
 	case ESR_ELx_EC_WATCHPT_LOW:
+		el0_watchpt(regs, esr);
+		break;
 	case ESR_ELx_EC_BRK64:
 		el0_dbg(regs, esr);
 		break;
@@ -1017,6 +1044,8 @@ asmlinkage void noinstr el0t_32_sync_handler(struct pt_regs *regs)
 		el0_softstp(regs, esr);
 		break;
 	case ESR_ELx_EC_WATCHPT_LOW:
+		el0_watchpt(regs, esr);
+		break;
 	case ESR_ELx_EC_BKPT32:
 		el0_dbg(regs, esr);
 		break;
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index 1ab37d6561cb..f329ef0363e0 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -855,6 +855,16 @@ static int watchpoint_handler(unsigned long addr, unsigned long esr,
 }
 NOKPROBE_SYMBOL(watchpoint_handler);
 
+void do_watchpoint(unsigned long addr, unsigned long esr,
+	struct pt_regs *regs)
+{
+	if (watchpoint_handler(addr, esr, regs)) {
+		arm64_notify_die("hw-watchpoint handler", regs, SIGTRAP, TRAP_HWBKPT,
+					regs->pc, esr);
+	}
+}
+NOKPROBE_SYMBOL(do_watchpoint);
+
 /*
  * Handle single-step exception.
  */
@@ -996,10 +1006,6 @@ static int __init arch_hw_breakpoint_init(void)
 	pr_info("found %d breakpoint and %d watchpoint registers.\n",
 		core_num_brps, core_num_wrps);
 
-	/* Register debug fault handlers. */
-	hook_debug_fault_code(DBG_ESR_EVT_HWWP, watchpoint_handler, SIGTRAP,
-			      TRAP_HWBKPT, "hw-watchpoint handler");
-
 	/*
 	 * Reset the breakpoint resources. We assume that a halting
 	 * debugger will leave the world in a nice state for us.
-- 
2.43.0

[PATCH v2 09/11] arm64: debug: split brk64 exception entry

[Ada Couprie Diaz]

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 BRK64 instruction can only be triggered by a BRK instruction. Thus,
we know that the PC is a legitimate address and isn't being used to train
a branch predictor with a bogus address : we don't need to call
`arm64_apply_bp_hardening()`.

We do not need to handle the Cortex-A76 erratum #1463225 either, as it
only relevant for single stepping at EL1.
BRK64 does not write FAR_EL1 either, as only hardware watchpoints do so.

Split the BRK64 exception entry, adjust the function signature, and its
behaviour to match the lack of needed mitigations.

Given that the break hook registration is handled statically in
`call_break_hook` since
(arm64: debug: call software break handlers statically)
and that we now bypass the exception handler registration, this change
renders `early_brk64` redundant : its functionality is now handled through
the post-init path.

This also removes the last usage of `el1_dbg()`.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/exception.h |  1 +
 arch/arm64/kernel/debug-monitors.c | 16 ++++++++++------
 arch/arm64/kernel/entry-common.c   | 21 ++++++++++++++++-----
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index eb465c375d34..af9b521e35f9 100644
--- a/arch/arm64/include/asm/exception.h
+++ b/arch/arm64/include/asm/exception.h
@@ -65,6 +65,7 @@ void do_breakpoint(unsigned long esr, struct pt_regs *regs);
 void do_softstep(unsigned long esr, struct pt_regs *regs);
 void do_watchpoint(unsigned long addr, unsigned long esr,
 			struct pt_regs *regs);
+void do_brk64(unsigned long esr, struct pt_regs *regs);
 void do_fpsimd_acc(unsigned long esr, struct pt_regs *regs);
 void do_sve_acc(unsigned long esr, struct pt_regs *regs);
 void do_sme_acc(unsigned long esr, struct pt_regs *regs);
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index d3cb8f5da51b..94779c957b70 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -274,8 +274,7 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr)
 }
 NOKPROBE_SYMBOL(call_break_hook);
 
-static int brk_handler(unsigned long unused, unsigned long esr,
-		       struct pt_regs *regs)
+static int brk_handler(unsigned long esr, struct pt_regs *regs)
 {
 	if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
 		return 0;
@@ -291,6 +290,14 @@ static int brk_handler(unsigned long unused, unsigned long esr,
 }
 NOKPROBE_SYMBOL(brk_handler);
 
+void do_brk64(unsigned long esr, struct pt_regs *regs)
+{
+	if (brk_handler(esr, regs))
+		arm64_notify_die("BRK handler", regs, SIGTRAP, TRAP_BRKPT, regs->pc,
+			esr);
+}
+NOKPROBE_SYMBOL(do_brk64);
+
 int aarch32_break_handler(struct pt_regs *regs)
 {
 	u32 arm_instr;
@@ -331,10 +338,7 @@ int aarch32_break_handler(struct pt_regs *regs)
 NOKPROBE_SYMBOL(aarch32_break_handler);
 
 void __init debug_traps_init(void)
-{
-	hook_debug_fault_code(DBG_ESR_EVT_BRK, brk_handler, SIGTRAP,
-			      TRAP_BRKPT, "BRK handler");
-}
+{}
 
 /* Re-enable single step for syscall restarting. */
 void user_rewind_single_step(struct task_struct *task)
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 6e70130d2741..424d072659b5 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -542,11 +542,12 @@ static void noinstr el1_watchpt(struct pt_regs *regs, unsigned long esr)
 	arm64_exit_el1_dbg(regs);
 }
 
-static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
+static void noinstr el1_brk64(struct pt_regs *regs, unsigned long esr)
 {
 	arm64_enter_el1_dbg(regs);
-	if (!cortex_a76_erratum_1463225_debug_handler(regs))
-		do_debug_exception(far, esr, regs);
+	debug_exception_enter(regs);
+	do_brk64(esr, regs);
+	debug_exception_exit(regs);
 	arm64_exit_el1_dbg(regs);
 }
 
@@ -598,7 +599,7 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
 		el1_watchpt(regs, esr);
 		break;
 	case ESR_ELx_EC_BRK64:
-		el1_dbg(regs, esr);
+		el1_brk64(regs, esr);
 		break;
 	case ESR_ELx_EC_FPAC:
 		el1_fpac(regs, esr);
@@ -834,6 +835,16 @@ static void noinstr el0_watchpt(struct pt_regs *regs, unsigned long esr)
 	exit_to_user_mode(regs);
 }
 
+static void noinstr el0_brk64(struct pt_regs *regs, unsigned long esr)
+{
+	enter_from_user_mode(regs);
+	debug_exception_enter(regs);
+	do_brk64(esr, regs);
+	debug_exception_exit(regs);
+	local_daif_restore(DAIF_PROCCTX);
+	exit_to_user_mode(regs);
+}
+
 static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
 {
 	/* Only watchpoints write FAR_EL1, otherwise its UNKNOWN */
@@ -921,7 +932,7 @@ asmlinkage void noinstr el0t_64_sync_handler(struct pt_regs *regs)
 		el0_watchpt(regs, esr);
 		break;
 	case ESR_ELx_EC_BRK64:
-		el0_dbg(regs, esr);
+		el0_brk64(regs, esr);
 		break;
 	case ESR_ELx_EC_FPAC:
 		el0_fpac(regs, esr);
-- 
2.43.0

[PATCH v2 10/11] arm64: debug: split bkpt32 exception entry

[Ada Couprie Diaz]

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 BKPT32 exception can only be triggered by a BKPT instruction. Thus,
we know that the PC is a legitimate address and isn't being used to train
a branch predictor with a bogus address : we don't need to call
`arm64_apply_bp_hardening()`.

The handler for this exception only pends a signal and doesn't depend
on any per-CPU state : we don't need to inhibit preemption, nor do we
need to keep the DAIF exceptions masked, so we can unmask them earlier.

Split the BKPT32 exception entry and adjust function signatures and its
behaviour to match its relaxed constraints compared to other
debug exceptions.

This replaces the last usage of `el0_dbg()`, so remove it.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/exception.h |  1 +
 arch/arm64/kernel/debug-monitors.c |  9 +++++++++
 arch/arm64/kernel/entry-common.c   | 21 +++++++++------------
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index af9b521e35f9..4ca43d4eaf98 100644
--- a/arch/arm64/include/asm/exception.h
+++ b/arch/arm64/include/asm/exception.h
@@ -66,6 +66,7 @@ void do_softstep(unsigned long esr, struct pt_regs *regs);
 void do_watchpoint(unsigned long addr, unsigned long esr,
 			struct pt_regs *regs);
 void do_brk64(unsigned long esr, struct pt_regs *regs);
+void do_bkpt32(unsigned long esr, struct pt_regs *regs);
 void do_fpsimd_acc(unsigned long esr, struct pt_regs *regs);
 void do_sve_acc(unsigned long esr, struct pt_regs *regs);
 void do_sme_acc(unsigned long esr, struct pt_regs *regs);
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 94779c957b70..53f0b2281f14 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -298,6 +298,15 @@ void do_brk64(unsigned long esr, struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(do_brk64);
 
+#ifdef CONFIG_COMPAT
+/* BRKPT exception always traps to be handled. */
+void do_bkpt32(unsigned long esr, struct pt_regs *regs)
+{
+	arm64_notify_die("aarch32 BKPT", regs, SIGTRAP, TRAP_BRKPT, regs->pc, esr);
+}
+NOKPROBE_SYMBOL(do_bkpt32);
+#endif /* CONFIG_COMPAT */
+
 int aarch32_break_handler(struct pt_regs *regs)
 {
 	u32 arm_instr;
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 424d072659b5..d7bc0b731424 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -845,17 +845,6 @@ static void noinstr el0_brk64(struct pt_regs *regs, unsigned long esr)
 	exit_to_user_mode(regs);
 }
 
-static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
-{
-	/* Only watchpoints write FAR_EL1, otherwise its UNKNOWN */
-	unsigned long far = read_sysreg(far_el1);
-
-	enter_from_user_mode(regs);
-	do_debug_exception(far, esr, regs);
-	local_daif_restore(DAIF_PROCCTX);
-	exit_to_user_mode(regs);
-}
-
 static void noinstr el0_svc(struct pt_regs *regs)
 {
 	enter_from_user_mode(regs);
@@ -1015,6 +1004,14 @@ static void noinstr el0_svc_compat(struct pt_regs *regs)
 	exit_to_user_mode(regs);
 }
 
+static void noinstr el0_bkpt32(struct pt_regs *regs, unsigned long esr)
+{
+	enter_from_user_mode(regs);
+	local_daif_restore(DAIF_PROCCTX);
+	do_bkpt32(esr, regs);
+	exit_to_user_mode(regs);
+}
+
 asmlinkage void noinstr el0t_32_sync_handler(struct pt_regs *regs)
 {
 	unsigned long esr = read_sysreg(esr_el1);
@@ -1058,7 +1055,7 @@ asmlinkage void noinstr el0t_32_sync_handler(struct pt_regs *regs)
 		el0_watchpt(regs, esr);
 		break;
 	case ESR_ELx_EC_BKPT32:
-		el0_dbg(regs, esr);
+		el0_bkpt32(regs, esr);
 		break;
 	default:
 		el0_inv(regs, esr);
-- 
2.43.0

[PATCH v2 11/11] arm64: debug: remove debug exception registration infrastructure

[Ada Couprie Diaz]

Now that debug exceptions are handled individually and without the need
for dynamic registration, remove the unused registration infrastructure.

Remove `early_brk64` as it has been made redundant by
(arm64: debug: split brk64 exception entry) and is not used anymore.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/debug-monitors.h |  2 -
 arch/arm64/include/asm/exception.h      |  2 -
 arch/arm64/include/asm/system_misc.h    |  4 --
 arch/arm64/kernel/debug-monitors.c      |  3 -
 arch/arm64/kernel/traps.c               | 26 +--------
 arch/arm64/mm/fault.c                   | 75 -------------------------
 6 files changed, 1 insertion(+), 111 deletions(-)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 9aabf65de693..fedcf78fb04d 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -94,7 +94,5 @@ static inline int reinstall_suspended_bps(struct pt_regs *regs)
 
 int aarch32_break_handler(struct pt_regs *regs);
 
-void debug_traps_init(void);
-
 #endif	/* __ASSEMBLY */
 #endif	/* __ASM_DEBUG_MONITORS_H */
diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index 4ca43d4eaf98..f7f772d75860 100644
--- a/arch/arm64/include/asm/exception.h
+++ b/arch/arm64/include/asm/exception.h
@@ -59,8 +59,6 @@ void do_el0_bti(struct pt_regs *regs);
 void do_el1_bti(struct pt_regs *regs, unsigned long esr);
 void do_el0_gcs(struct pt_regs *regs, unsigned long esr);
 void do_el1_gcs(struct pt_regs *regs, unsigned long esr);
-void do_debug_exception(unsigned long addr_if_watchpoint, unsigned long esr,
-			struct pt_regs *regs);
 void do_breakpoint(unsigned long esr, struct pt_regs *regs);
 void do_softstep(unsigned long esr, struct pt_regs *regs);
 void do_watchpoint(unsigned long addr, unsigned long esr,
diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
index c34344256762..344b1c1a4bbb 100644
--- a/arch/arm64/include/asm/system_misc.h
+++ b/arch/arm64/include/asm/system_misc.h
@@ -25,10 +25,6 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
 		      int signo, int sicode, unsigned long far,
 		      unsigned long err);
 
-void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned long,
-					     struct pt_regs *),
-			   int sig, int code, const char *name);
-
 struct mm_struct;
 extern void __show_regs(struct pt_regs *);
 
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 53f0b2281f14..a5f344e556df 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -346,9 +346,6 @@ int aarch32_break_handler(struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(aarch32_break_handler);
 
-void __init debug_traps_init(void)
-{}
-
 /* Re-enable single step for syscall restarting. */
 void user_rewind_single_step(struct task_struct *task)
 {
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 50d7a6a75f45..ce20f46e08cb 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -1101,29 +1101,5 @@ int ubsan_brk_handler(struct pt_regs *regs, unsigned long esr)
 }
 #endif
 
-/*
- * Initial handler for AArch64 BRK exceptions
- * This handler only used until debug_traps_init().
- */
-int __init early_brk64(unsigned long addr, unsigned long esr,
-		struct pt_regs *regs)
-{
-#ifdef CONFIG_CFI_CLANG
-	if (esr_is_cfi_brk(esr))
-		return cfi_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
-#endif
-#ifdef CONFIG_KASAN_SW_TAGS
-	if ((esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
-		return kasan_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
-#endif
-#ifdef CONFIG_UBSAN_TRAP
-	if ((esr_brk_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
-		return ubsan_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
-#endif
-	return bug_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
-}
-
 void __init trap_init(void)
-{
-	debug_traps_init();
-}
+{}
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index ec0a337891dd..bc7f554dea9f 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -53,18 +53,12 @@ struct fault_info {
 };
 
 static const struct fault_info fault_info[];
-static struct fault_info debug_fault_info[];
 
 static inline const struct fault_info *esr_to_fault_info(unsigned long esr)
 {
 	return fault_info + (esr & ESR_ELx_FSC);
 }
 
-static inline const struct fault_info *esr_to_debug_fault_info(unsigned long esr)
-{
-	return debug_fault_info + DBG_ESR_EVT(esr);
-}
-
 static void data_abort_decode(unsigned long esr)
 {
 	unsigned long iss2 = ESR_ELx_ISS2(esr);
@@ -938,75 +932,6 @@ void do_sp_pc_abort(unsigned long addr, unsigned long esr, struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(do_sp_pc_abort);
 
-/*
- * __refdata because early_brk64 is __init, but the reference to it is
- * clobbered at arch_initcall time.
- * See traps.c and debug-monitors.c:debug_traps_init().
- */
-static struct fault_info __refdata debug_fault_info[] = {
-	{ do_bad,	SIGTRAP,	TRAP_HWBKPT,	"hardware breakpoint"	},
-	{ do_bad,	SIGTRAP,	TRAP_HWBKPT,	"hardware single-step"	},
-	{ do_bad,	SIGTRAP,	TRAP_HWBKPT,	"hardware watchpoint"	},
-	{ do_bad,	SIGKILL,	SI_KERNEL,	"unknown 3"		},
-	{ do_bad,	SIGTRAP,	TRAP_BRKPT,	"aarch32 BKPT"		},
-	{ do_bad,	SIGKILL,	SI_KERNEL,	"aarch32 vector catch"	},
-	{ early_brk64,	SIGTRAP,	TRAP_BRKPT,	"aarch64 BRK"		},
-	{ do_bad,	SIGKILL,	SI_KERNEL,	"unknown 7"		},
-};
-
-void __init hook_debug_fault_code(int nr,
-				  int (*fn)(unsigned long, unsigned long, struct pt_regs *),
-				  int sig, int code, const char *name)
-{
-	BUG_ON(nr < 0 || nr >= ARRAY_SIZE(debug_fault_info));
-
-	debug_fault_info[nr].fn		= fn;
-	debug_fault_info[nr].sig	= sig;
-	debug_fault_info[nr].code	= code;
-	debug_fault_info[nr].name	= name;
-}
-
-/*
- * In debug exception context, we explicitly disable preemption despite
- * having interrupts disabled.
- * This serves two purposes: it makes it much less likely that we would
- * accidentally schedule in exception context and it will force a warning
- * if we somehow manage to schedule by accident.
- */
-static void debug_exception_enter(struct pt_regs *regs)
-{
-	preempt_disable();
-
-	/* This code is a bit fragile.  Test it. */
-	RCU_LOCKDEP_WARN(!rcu_is_watching(), "exception_enter didn't work");
-}
-NOKPROBE_SYMBOL(debug_exception_enter);
-
-static void debug_exception_exit(struct pt_regs *regs)
-{
-	preempt_enable_no_resched();
-}
-NOKPROBE_SYMBOL(debug_exception_exit);
-
-void do_debug_exception(unsigned long addr_if_watchpoint, unsigned long esr,
-			struct pt_regs *regs)
-{
-	const struct fault_info *inf = esr_to_debug_fault_info(esr);
-	unsigned long pc = instruction_pointer(regs);
-
-	debug_exception_enter(regs);
-
-	if (user_mode(regs) && !is_ttbr0_addr(pc))
-		arm64_apply_bp_hardening();
-
-	if (inf->fn(addr_if_watchpoint, esr, regs)) {
-		arm64_notify_die(inf->name, regs, inf->sig, inf->code, pc, esr);
-	}
-
-	debug_exception_exit(regs);
-}
-NOKPROBE_SYMBOL(do_debug_exception);
-
 /*
  * Used during anonymous page fault handling.
  */
-- 
2.43.0

Re: [PATCH v2 00/11] arm64: debug: remove hook registration, split exception entry

[Luis Claudio R. Goncalves]

On Mon, May 12, 2025 at 06:43:15PM +0100, Ada Couprie Diaz wrote:
> Hi,
> 
> This series simplifies the debug exception entry path by removing handler
> registration mechanisms for the debug exception handlers, a holdover from
> the arm kernel, as well as the break and stepping handlers.
> This moves much of the code related to debug exceptions outside of
> `mm/fault.c` where it didn't make much sense.
> This allows us to split the debug exception entries: going from one common
> path per EL for all debug exceptions to a unique one per exception and EL.
> 
> The result is a much simpler and fully static exception entry path, which
> we tailor to the different exceptions and their constraints.
> 
> The series is structured as such :
> 1 : related clean-up in the signle step handler
> 2-4 : software breakpoints and single step handlers registration removal
> 5: preparatory function duplication that gets cleaned-up in patch 11
> 6-11 : debug exception splitting and handler registration removal
> 
> * Patch 2 copies and extends the `early_brk64` break handling for the
> normal path, byassing the dynamic registration.
> * Patch 3 does something similar for the single stepping handlers.
> * Patches 6 through 10 split each individual debug exception from
> the unified path by creating specific handlers, adapting them to
> their constraints and calling into them, bypassing the dynamically
> registered handlers used before.
> * Patches 4 and 11 are clean-ups removing the code that has been replaced
> and made redundant by the preceding patches.
> 
> Single Step Exception
> ===
> 
> Of note, this allows us to make the single exception handling mostly
> preemptible coming from EL0 in patch 7, fixing an issue with PREEMPT_RT[0].
> The commit message details the reasoning on why this should be safe.
> It is *definitely* not preemptible at EL1 in the current state, given
> that the EL1 software step exception is handled by KGDB.
> 
> CC-ing Luis and Sebastian as they were active on the original bug report.

I have been running the ssdd test in a tight loop on a kernel with your
patches and PREEMPT_RT enabled, on two different aarch64 boxes (8 cores and
144 cores). So far, so good. The patch series seems to have solved the
problem I reported.

For the first 10h of tests the kernel also had LOCKDEP and locking debug
features enabled. Now I am running the test with a production-like
configuration. As soon as these tests are done I will run a few more sanity
tests and reply here with my test ACK.

Is there any specific test you would like me to run on that test setup I
have?

As for the code review, I am not that well versed on ARM debug exceptions,
I may take a bit longer to complete the task with the required accuracy.

Best regards,
Luis

 
> Cc: "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Testing
> ===
> 
> Testing EL1 debug exceptions can only be properly done with perf.
> A simple test of hardware breakpoints, watchpoints, and software stepping
> can be achieved by using `perf stat sleep 0.01` and adding hardware events
> for `jiffies` and `hrtimer_nanosleep`, which guarantees a
> hardware watchpoint and breakpoint.
> Inserting a `WARN()` at a convenient place will allow testing both early
> and late software breakpoints at EL1, or using KGDB to test without
> changing code.
> 
> For EL0 debug exceptions, the easiest is to setup a basic program and use
> GDB with a list of pre-programmed commands setting hardware and software
> breakpoints as well as watchpoints. Setpping will occur naturally when
> encountering breakpoints.
> 
> All tests maintained behaviour after the patches.
> 
> I also tested with KASAN on, with no apparent impact.
> 
> See below for some testing examples.
> 
> Regarding [0], I tested a PREEMPT_RT kernel with the patches, running
> `ssdd` on loop as well as some spammy GDB stepping, and some
> hardware watchpoints on a tight loop without any issues.
> 
> 
> Based on v6.15-rc6.
> Thanks,
> Ada
> 
> v2 :
>  - Move the BP hardening call outside of the handlers to `entry-common`,
>    as they are not needed coming from EL1.
>  - Make the EL0 software stepping exception mostly preemptible
>    - Move `reinstall_hw_bps()` call to `entry-common`
>    - Don't disable preemption in `el0_softstp()`
>    - Unmask DAIF before `do_softstep()` in `el0_softstp()`
>    - Update comments to make sense with the changes
>  - Simplify the single step handler, as it always returns 0 and could not
>    trigger the `arm64_notify_die()` call.
>  - Fix some commit messages
> v1 : https://lore.kernel.org/linux-arm-kernel/20250425153647.438508-1-ada.coupriediaz@arm.com
> 
> [0]: https://lore.kernel.org/linux-arm-kernel/Z6YW_Kx4S2tmj2BP@uudg.org/
> 
> 
> Testing examples
> ===
> 
> Perf (for EL1):
> ~~~
> Assuming that `perf` is on your $PATH and building with `kallsyms`
> 
>   #!/bin/bash
>   
>   watch_addr=$(sudo cat /proc/kallsyms | grep "D jiffies$" | cut -f1 -d\  )
>   break_addr=$(sudo cat /proc/kallsyms | grep "clock_nanosleep$" | cut -f1 -d\  )
>   
>   cmd="sleep 0.01"
>   
>   sudo perf stat -a -e mem:0x${watch_addr}/8:w -e mem:0x${break_addr}:x ${cmd}
> 
> NB: This does /not/ test EL1 BRKs.
> 
> 
> GDB commands (for EL0):
> ~~~
> The following C example, compiled with `-g -O0`
> 
>   int main() {
>           int add = 0xAA;
>           int target = 0;
> 
>           target += add;
> 
>   #ifdef COMPAT
>       __asm__("BKPT");
>   #else
>       __asm__("BRK 1");
>   #endif
>   
>           return target;
>   }
> 
> Combined with the following GDB command-list
> 
>   start
>   hbreak 3
>   watch target
>   commands 2
>   continue
>   end
>   commands 3
>   continue
>   end
>   continue
>   jump 11
>   continue
>   quit
> 
> Executed as such : `gdb -x ${COMMAND_LIST_FILE} ./a.out`
> should go through the whole program, return 0252/170/0xAA, and
> exercise all EL0 debug exception entries.
> By using a cross-compiler and passing and additional `-DCOMPAT` argument
> during compilation, the `BKPT32` path can also be tested.
> NOTE: `BKPT` *will* make GDB loop infinitely, that is expected. Sending
> SIGINT to GDB will break the loop and the execution should complete.
> 
> Ada Couprie Diaz (11):
>   arm64: debug: clean up single_step_handler logic
>   arm64: debug: call software break handlers statically
>   arm64: debug: call step handlers statically
>   arm64: debug: remove break/step handler registration infrastructure
>   arm64: entry: Add entry and exit functions for debug exceptions
>   arm64: debug: split hardware breakpoint exeception entry
>   arm64: debug: split single stepping exception entry
>   arm64: debug: split hardware watchpoint exception entry
>   arm64: debug: split brk64 exception entry
>   arm64: debug: split bkpt32 exception entry
>   arm64: debug: remove debug exception registration infrastructure
> 
>  arch/arm64/include/asm/debug-monitors.h       |  26 ---
>  arch/arm64/include/asm/exception.h            |   6 +-
>  arch/arm64/include/asm/kgdb.h                 |   4 +
>  arch/arm64/include/asm/kprobes.h              |   6 +
>  arch/arm64/include/asm/system_misc.h          |   4 -
>  arch/arm64/include/asm/traps.h                |   6 +
>  arch/arm64/include/asm/uprobes.h              |   3 +
>  arch/arm64/kernel/debug-monitors.c            | 205 +++++++-----------
>  arch/arm64/kernel/entry-common.c              | 148 ++++++++++++-
>  arch/arm64/kernel/hw_breakpoint.c             |  35 ++-
>  arch/arm64/kernel/kgdb.c                      |  39 +---
>  arch/arm64/kernel/probes/kprobes.c            |  31 +--
>  arch/arm64/kernel/probes/kprobes_trampoline.S |   2 +-
>  arch/arm64/kernel/probes/uprobes.c            |  18 +-
>  arch/arm64/kernel/traps.c                     |  77 +------
>  arch/arm64/mm/fault.c                         |  75 -------
>  16 files changed, 294 insertions(+), 391 deletions(-)
> 
> 
> base-commit: 82f2b0b97b36ee3fcddf0f0780a9a0825d52fec3
> -- 
> 2.43.0
> 
---end quoted text---

Re: [PATCH v2 00/11] arm64: debug: remove hook registration, split exception entry

[Ada Couprie Diaz]

Re-sending with proper text format, apologies for the noise...

On 13/05/2025 13:25, Luis Claudio R. Goncalves wrote:

> On Mon, May 12, 2025 at 06:43:15PM +0100, Ada Couprie Diaz wrote:
>> [...]
>>
>> Single Step Exception
>> ===
>>
>> Of note, this allows us to make the single exception handling mostly
>> preemptible coming from EL0 in patch 7, fixing an issue with PREEMPT_RT[0].
>> The commit message details the reasoning on why this should be safe.
>> It is *definitely* not preemptible at EL1 in the current state, given
>> that the EL1 software step exception is handled by KGDB.
>>
>> CC-ing Luis and Sebastian as they were active on the original bug report.
> I have been running the ssdd test in a tight loop on a kernel with your
> patches and PREEMPT_RT enabled, on two different aarch64 boxes (8 cores and
> 144 cores). So far, so good. The patch series seems to have solved the
> problem I reported.
>
> For the first 10h of tests the kernel also had LOCKDEP and locking debug
> features enabled. Now I am running the test with a production-like
> configuration. As soon as these tests are done I will run a few more sanity
> tests and reply here with my test ACK.
Hi Luis,

Thanks for taking the time to test, I'm glad it seems OK for now.
> Is there any specific test you would like me to run on that test setup I
> have?

There are a couple of edge-cases that might be problematic if my 
conclusions are wrong : 1. Race between a step exception being taken, 
and the related hardware breakpoint/watchpoint being removed 2. 
Migration of a task stepping a CPU-bound breakpoint/watchpoint

I have been stress testing them on an AMD Seattle board with 4 cores, 
but more extensive testing is always welcome.

I'll describe my testing below, but it is a bit messy and might be 
unclear, my apologies.

I have been using the following very rough program (compiled with -O0) :

	#include <sys/mman.h>
	#include <stdio.h>
	#include <errno.h>
	#include <string.h>
	#include <unistd.h>
	#include <signal.h>
	
	#define TEST_ADDR (void*)0x6000000000
	
	volatile int exit_flag = 0;
	
	void signal_exit(int signal) {
		exit_flag = 1;
	}
	
	int main() {
		void *p = mmap(TEST_ADDR, sizeof(long), PROT_READ|PROT_WRITE,
			MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0);
		int map_err = errno;
	
		printf("r : %p\np : %p\n", TEST_ADDR, p);
		// Reference to make TEST_ADDR appropriate for the address space if it fails
		printf("main : %p\n", (void *)&main);
	
		if (p == MAP_FAILED) {
			printf(strerror(map_err));
			return -1;
		}
	
		struct sigaction interrupt_handler = {
			.sa_handler = &signal_exit,
			.sa_mask = 0,
			.sa_flags = SA_RESETHAND
		};
	
		sigaction(SIGINT, &interrupt_handler, NULL);
	
		unsigned long *data = (unsigned long *)p;
		while (!exit_flag) {
			*data += 1;
		}
	
		printf("Exiting gracefully\n");
	
		munmap(p, sizeof(long));
		return 0;
	}


Which runs continuously, repeatedly changing a fixed addressed, so that 
a hardware watchpoint can be set externally via perf and be CPU-bound :

	perf stat -C $CPU -emem:0x6000000000/8:w


So to test 1. I run perf in a loop, with --timeout 10 so that it 
adds/removes the watchpoint repeatedly, one for each CPU.

	while true; do perf stat --timeout 10 -C $CPU -emem:0x6000000000/8:w ; done


My machine has 4 hardware watchpoints, so I can cover all cores and see 
that the counts are consistent, even if the target task switches cores.
(It is never 0 on all cores, no errors are produced, it is consistent 
with the count when perf is ran on all-cores rather than core-by-core 
(-a) or with the task PID (-p) )

To test 2. I again set one perf monitor per CPU, this time without 
timeout, and then load the system to try to force preemption (with ssdd 
for example),
similarly waiting for inconsistencies, errors, or the count stopping.

However, this might be more difficult if the number of cores is much 
greater than the number of hardware watchpoints.
For 1. the task could be pinned to a core, but for 2. the task could be 
limited to as many cores as the system has hardware watchpoints.


Hopefully that makes sense, but I understand it's a bit involved.

> As for the code review, I am not that well versed on ARM debug exceptions,
> I may take a bit longer to complete the task with the required accuracy.
No worries, any input would be appreciated !
> Best regards,
> Luis
Thanks,
Best regards
Ada
>> Cc: "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>
>> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>>
>> [...]
>>
>> Testing examples
>> ===
>>
>> Perf (for EL1):
>> ~~~
>> Assuming that `perf` is on your $PATH and building with `kallsyms`
>>
>>    #!/bin/bash
>>    
>>    watch_addr=$(sudo cat /proc/kallsyms | grep "D jiffies$" | cut -f1 -d\  )
>>    break_addr=$(sudo cat /proc/kallsyms | grep "clock_nanosleep$" | cut -f1 -d\  )
>>    
>>    cmd="sleep 0.01"
>>    
>>    sudo perf stat -a -e mem:0x${watch_addr}/8:w -e mem:0x${break_addr}:x ${cmd}
>>
>> NB: This does /not/ test EL1 BRKs.
>>
>>
>> GDB commands (for EL0):
>> ~~~
>> The following C example, compiled with `-g -O0`
>>
>>    int main() {
>>            int add = 0xAA;
>>            int target = 0;
>>
>>            target += add;
>>
>>    #ifdef COMPAT
>>        __asm__("BKPT");
>>    #else
>>        __asm__("BRK 1");
>>    #endif
>>    
>>            return target;
>>    }
>>
>> Combined with the following GDB command-list
>>
>>    start
>>    hbreak 3
>>    watch target
>>    commands 2
>>    continue
>>    end
>>    commands 3
>>    continue
>>    end
>>    continue
>>    jump 11
>>    continue
>>    quit
>>
>> Executed as such : `gdb -x ${COMMAND_LIST_FILE} ./a.out`
>> should go through the whole program, return 0252/170/0xAA, and
>> exercise all EL0 debug exception entries.
>> By using a cross-compiler and passing and additional `-DCOMPAT` argument
>> during compilation, the `BKPT32` path can also be tested.
>> NOTE: `BKPT` *will* make GDB loop infinitely, that is expected. Sending
>> SIGINT to GDB will break the loop and the execution should complete.
>>
>> [...]

Re: [PATCH v2 00/11] arm64: debug: remove hook registration, split exception entry

[Luis Claudio R. Goncalves]

On Tue, May 13, 2025 at 04:19:26PM +0100, Ada Couprie Diaz wrote:
> Re-sending with proper text format, apologies for the noise...
> 
> On 13/05/2025 13:25, Luis Claudio R. Goncalves wrote:
> 
> > On Mon, May 12, 2025 at 06:43:15PM +0100, Ada Couprie Diaz wrote:
> > > [...]
> > > 
> > > Single Step Exception
> > > ===
...
> Hi Luis,
> 
> Thanks for taking the time to test, I'm glad it seems OK for now.
> > Is there any specific test you would like me to run on that test setup I
> > have?
> 
> There are a couple of edge-cases that might be problematic if my conclusions
> are wrong : 1. Race between a step exception being taken, and the related
> hardware breakpoint/watchpoint being removed 2. Migration of a task stepping
> a CPU-bound breakpoint/watchpoint
> 
> I have been stress testing them on an AMD Seattle board with 4 cores, but
> more extensive testing is always welcome.
> 
> I'll describe my testing below, but it is a bit messy and might be unclear,
> my apologies.
> 
> I have been using the following very rough program (compiled with -O0) :
> 
...
> 
> Which runs continuously, repeatedly changing a fixed addressed, so that a
> hardware watchpoint can be set externally via perf and be CPU-bound :
> 
> 	perf stat -C $CPU -emem:0x6000000000/8:w
> 
> 
> So to test 1. I run perf in a loop, with --timeout 10 so that it
> adds/removes the watchpoint repeatedly, one for each CPU.
> 
> 	while true; do perf stat --timeout 10 -C $CPU -emem:0x6000000000/8:w ; done
> 
> 
> My machine has 4 hardware watchpoints, so I can cover all cores and see that
> the counts are consistent, even if the target task switches cores.
> (It is never 0 on all cores, no errors are produced, it is consistent with
> the count when perf is ran on all-cores rather than core-by-core (-a) or
> with the task PID (-p) )
> 
> To test 2. I again set one perf monitor per CPU, this time without timeout,
> and then load the system to try to force preemption (with ssdd for example),
> similarly waiting for inconsistencies, errors, or the count stopping.
> 
> However, this might be more difficult if the number of cores is much greater
> than the number of hardware watchpoints.
> For 1. the task could be pinned to a core, but for 2. the task could be
> limited to as many cores as the system has hardware watchpoints.

I ran the two tests you listed above, along with some variations just to
make sure I got the details right, and all those tests completed flawlessly
on both machines, on the 4 kernel configurations I tests (all with
PREEMPT_RT enabled, with and without LOCKDEP and assorted debug features).

> Hopefully that makes sense, but I understand it's a bit involved.
> 
...
> > > Testing examples
> > > ===
> > > 
> > > Perf (for EL1):
> > > ~~~
> > > Assuming that `perf` is on your $PATH and building with `kallsyms`
> > > 
> > >    #!/bin/bash
> > >    watch_addr=$(sudo cat /proc/kallsyms | grep "D jiffies$" | cut -f1 -d\  )
> > >    break_addr=$(sudo cat /proc/kallsyms | grep "clock_nanosleep$" | cut -f1 -d\  )
> > >    cmd="sleep 0.01"
> > >    sudo perf stat -a -e mem:0x${watch_addr}/8:w -e mem:0x${break_addr}:x ${cmd}
> > > 
> > > NB: This does /not/ test EL1 BRKs.
> > > 
> > > 
> > > GDB commands (for EL0):
> > > ~~~
> > > The following C example, compiled with `-g -O0`
> > > 
> > >    int main() {
> > >            int add = 0xAA;
> > >            int target = 0;
> > > 
> > >            target += add;
> > > 
> > >    #ifdef COMPAT
> > >        __asm__("BKPT");
> > >    #else
> > >        __asm__("BRK 1");
> > >    #endif
> > >            return target;
> > >    }
> > > 
> > > Combined with the following GDB command-list
> > > 
> > >    start
> > >    hbreak 3
> > >    watch target
> > >    commands 2
> > >    continue
> > >    end
> > >    commands 3
> > >    continue
> > >    end
> > >    continue
> > >    jump 11
> > >    continue
> > >    quit
> > > 
> > > Executed as such : `gdb -x ${COMMAND_LIST_FILE} ./a.out`
> > > should go through the whole program, return 0252/170/0xAA, and
> > > exercise all EL0 debug exception entries.

This is the only test where I (consistently) hit backtraces. If I run the
test with "gdb -x ${COMMAND_LIST_FILE} ..." I get a single backtrace, every
time:

[  263.890424] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
[  263.890444] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 5744, name: gdb_prog1
[  263.890445] preempt_count: 1, expected: 0
[  263.890446] RCU nest depth: 0, expected: 0
[  263.890447] 1 lock held by gdb_prog1/5744:
[  263.890448]  #0: ffff100028496f58 (&sighand->siglock){+.+.}-{3:3}, at: force_sig_info_to_task+0x30/0x150
[  263.890468] Preemption disabled at:
[  263.890469] [<ffff8000800391a8>] debug_exception_enter+0x18/0x78
[  263.890484] CPU: 114 UID: 0 PID: 5744 Comm: gdb_prog1 Tainted: G        W           6.15.0-rc6-rt1__dbg #2 PREEMPT_{RT,(lazy)}
[  263.890487] Tainted: [W]=WARN
[  263.890488] Hardware name: Supermicro ARS-221GL-NR-01/G1SMH, BIOS 2.0 07/12/2024
[  263.890490] Call trace:
[  263.890492]  show_stack+0x30/0x88 (C)
[  263.890495]  dump_stack_lvl+0xa0/0xe0
[  263.890498]  dump_stack+0x14/0x2c
[  263.890499]  __might_resched+0x170/0x240
[  263.890506]  rt_spin_lock+0x6c/0x1a0
[  263.890512]  force_sig_info_to_task+0x30/0x150
[  263.890513]  force_sig_fault+0x68/0xa0
[  263.890515]  arm64_force_sig_fault+0x44/0x80
[  263.890518]  send_user_sigtrap+0x60/0xa8
[  263.890520]  do_brk64+0x40/0x88
[  263.890522]  el0_brk64+0x50/0x1c0
[  263.890526]  el0t_64_sync_handler+0x60/0xe0
[  263.890528]  el0t_64_sync+0x184/0x188

Quite similar to the problem originally reported, where sending signals
with preemption disabled could trigger the "rtlock_might_resched();" check
if CONFIG_DEBUG_ATOMIC_SLEEP is enabled.

If I call gdb and run manually the sequence of commands you described, I
get the backtrace above three times. The only difference is that on the
second backtrace I get these extra elements on the header:

  [48052.129422] RCU nest depth: 1, expected: 1
  [48052.129424] 2 locks held by gdb_prog1/27451:
  [48052.129425]  #0: ffff8000828315c8 (rcu_read_lock){....}-{1:3}, at: breakpoint_handler+0xd8/0x318
  [48052.129439]  #1: ffff00008abd92d8 (&sighand->siglock){+.+.}-{3:3}, at: force_sig_info_to_task+0x30/0x150

So, when I enter manually the GDB command you suggested, the result is:

    start           <--- Backtrace#1:   preempt_count: 1
    hbreak 3
    watch target
    commands 2
    continue
    end
    commands 3
    continue
    end
    continue        <--- Backtrace#2:   preempt_count: 1   RCU nest depth: 1
    jump 11         <--- Backtrace#3:   preempt_count: 1
    continue
    quit

I hope this report is helpful.

IMHO, even with these backtraces, there was a considerable enhancement when
compared to the original scenario we reported.

Best regards,
Luis

> > > By using a cross-compiler and passing and additional `-DCOMPAT` argument
> > > during compilation, the `BKPT32` path can also be tested.
> > > NOTE: `BKPT` *will* make GDB loop infinitely, that is expected. Sending
> > > SIGINT to GDB will break the loop and the execution should complete.
> > > 
> > > [...]
> 
---end quoted text---

Re: [PATCH v2 01/11] arm64: debug: clean up single_step_handler logic

[Will Deacon]

On Mon, May 12, 2025 at 06:43:16PM +0100, Ada Couprie Diaz wrote:
> Remove the unnecessary boolean which always checks if the handler was found
> and return early instead.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/kernel/debug-monitors.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 58f047de3e1c..676fa0231935 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -241,8 +241,6 @@ static void send_user_sigtrap(int si_code)
>  static int single_step_handler(unsigned long unused, unsigned long esr,
>  			       struct pt_regs *regs)
>  {
> -	bool handler_found = false;
> -
>  	/*
>  	 * If we are stepping a pending breakpoint, call the hw_breakpoint
>  	 * handler first.
> @@ -250,10 +248,10 @@ static int single_step_handler(unsigned long unused, unsigned long esr,
>  	if (!reinstall_suspended_bps(regs))
>  		return 0;
>  
> -	if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
> -		handler_found = true;
> +	if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
> +		return 0;
>  
> -	if (!handler_found && user_mode(regs)) {
> +	if (user_mode(regs)) {
>  		send_user_sigtrap(TRAP_TRACE);
>  
>  		/*
> @@ -263,7 +261,7 @@ static int single_step_handler(unsigned long unused, unsigned long esr,
>  		 * to the active-not-pending state).
>  		 */
>  		user_rewind_single_step(current);
> -	} else if (!handler_found) {
> +	} else {

Crikey, this code is totally bizarre! Thanks for cleaning it up.

Will

Re: [PATCH v2 02/11] arm64: debug: call software break handlers statically

[Will Deacon]

On Mon, May 12, 2025 at 06:43:17PM +0100, Ada Couprie Diaz wrote:
> Software breakpoints pass an immediate value in ESR ("comment") that can
> be used to call a specialized handler (KGDB, KASAN...).
> We do so in two different ways :
>  - During early boot, `early_brk64` statically checks against known
>    immediates and calls the corresponding handler,
>  - During init, handlers are dynamically registered into a list. When
>    called, the generic software breakpoint handler will iterate over
>    the list to find the appropriate handler.
> 
> The dynamic registration does not provide any benefit here as it is not
> exported and all its uses are within the arm64 tree. It also depends on an
> RCU list, whose safe access currently relies on the non-preemptible state
> of `do_debug_exception`.
> 
> Replace the list iteration logic in `call_break_hooks` to call
> the breakpoint handlers statically if they are enabled, like in
> `early_brk64`.
> Expose the handlers in their respective headers to be reachable from
> `arch/arm64/kernel/debug-monitors.c` at link time.
> 
> Unify the naming of the software breakpoint handlers to XXX_brk_handler(),
> making it clear they are related and to differentiate from the
> hardware breakpoints.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/kgdb.h                 |  3 +
>  arch/arm64/include/asm/kprobes.h              |  6 ++
>  arch/arm64/include/asm/traps.h                |  6 ++
>  arch/arm64/include/asm/uprobes.h              |  2 +
>  arch/arm64/kernel/debug-monitors.c            | 57 ++++++++++++++----
>  arch/arm64/kernel/kgdb.c                      | 22 ++-----
>  arch/arm64/kernel/probes/kprobes.c            | 31 ++--------
>  arch/arm64/kernel/probes/kprobes_trampoline.S |  2 +-
>  arch/arm64/kernel/probes/uprobes.c            |  9 +--
>  arch/arm64/kernel/traps.c                     | 59 ++++---------------
>  10 files changed, 84 insertions(+), 113 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
> index 21fc85e9d2be..82a76b2102fb 100644
> --- a/arch/arm64/include/asm/kgdb.h
> +++ b/arch/arm64/include/asm/kgdb.h
> @@ -24,6 +24,9 @@ static inline void arch_kgdb_breakpoint(void)
>  extern void kgdb_handle_bus_error(void);
>  extern int kgdb_fault_expected;
>  
> +int kgdb_brk_handler(struct pt_regs *regs, unsigned long esr);
> +int kgdb_compiled_brk_handler(struct pt_regs *regs, unsigned long esr);
> +
>  #endif /* !__ASSEMBLY__ */
>  
>  /*
> diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
> index be7a3680dadf..b27dd6028e6a 100644
> --- a/arch/arm64/include/asm/kprobes.h
> +++ b/arch/arm64/include/asm/kprobes.h
> @@ -38,6 +38,12 @@ struct kprobe_ctlblk {
>  void arch_remove_kprobe(struct kprobe *);
>  int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
>  void __kretprobe_trampoline(void);
> +int __kprobes kprobe_brk_handler(struct pt_regs *regs,
> +				 unsigned long esr);
> +int __kprobes kprobe_ss_brk_handler(struct pt_regs *regs,
> +				 unsigned long esr);
> +int __kprobes kretprobe_brk_handler(struct pt_regs *regs,
> +				 unsigned long esr);
>  void __kprobes *trampoline_probe_handler(struct pt_regs *regs);
>  
>  #endif /* CONFIG_KPROBES */

If you add these _after_ the #endif...

> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 676fa0231935..4ece4a93b872 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -21,8 +21,11 @@
>  #include <asm/cputype.h>
>  #include <asm/daifflags.h>
>  #include <asm/debug-monitors.h>
> +#include <asm/kgdb.h>
> +#include <asm/kprobes.h>
>  #include <asm/system_misc.h>
>  #include <asm/traps.h>
> +#include <asm/uprobes.h>
>  
>  /* Determine debug architecture. */
>  u8 debug_monitors_arch(void)
> @@ -299,20 +302,50 @@ void unregister_kernel_break_hook(struct break_hook *hook)
>  
>  static int call_break_hook(struct pt_regs *regs, unsigned long esr)
>  {
> -	struct break_hook *hook;
> -	struct list_head *list;
> -
> -	list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
> -
> -	/*
> -	 * Since brk exception disables interrupt, this function is
> -	 * entirely not preemptible, and we can use rcu list safely here.
> -	 */
> -	list_for_each_entry_rcu(hook, list, node) {
> -		if ((esr_brk_comment(esr) & ~hook->mask) == hook->imm)
> -			return hook->fn(regs, esr);
> +	if (user_mode(regs)) {
> +#ifdef CONFIG_UPROBES
> +		if (esr_brk_comment(esr) == UPROBES_BRK_IMM)
> +			return uprobe_brk_handler(regs, esr);
> +#endif
> +		return DBG_HOOK_ERROR;
>  	}
>  
> +	if (esr_brk_comment(esr) == BUG_BRK_IMM)
> +		return bug_brk_handler(regs, esr);
> +
> +#ifdef CONFIG_CFI_CLANG
> +	if (esr_is_cfi_brk(esr))
> +		return cfi_brk_handler(regs, esr);
> +#endif
> +
> +	if (esr_brk_comment(esr) == FAULT_BRK_IMM)
> +		return reserved_fault_brk_handler(regs, esr);
> +
> +#ifdef CONFIG_KASAN_SW_TAGS
> +	if ((esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
> +		return kasan_brk_handler(regs, esr);
> +#endif
> +#ifdef CONFIG_UBSAN_TRAP
> +	if ((esr_brk_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
> +		return ubsan_brk_handler(regs, esr);
> +#endif
> +#ifdef CONFIG_KGDB
> +	if (esr_brk_comment(esr) == KGDB_DYN_DBG_BRK_IMM)
> +		return kgdb_brk_handler(regs, esr);
> +	if (esr_brk_comment(esr) == KGDB_COMPILED_DBG_BRK_IMM)
> +		return kgdb_compiled_brk_handler(regs, esr);
> +#endif
> +#ifdef CONFIG_KPROBES
> +	if (esr_brk_comment(esr) == KPROBES_BRK_IMM)
> +		return kprobe_brk_handler(regs, esr);
> +	if (esr_brk_comment(esr) == KPROBES_BRK_SS_IMM)
> +		return kprobe_ss_brk_handler(regs, esr);
> +#endif
> +#ifdef CONFIG_KRETPROBES
> +	if (esr_brk_comment(esr) == KRETPROBES_BRK_IMM)
> +		return kretprobe_brk_handler(regs, esr);
> +#endif

... then I think you can use IS_ENABLED() here instead of the #ifdefs.
I didn't check everything, but the diff I was hacking on is below.

Will

--->8

diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
index b27dd6028e6a..0d41fbbffb06 100644
--- a/arch/arm64/include/asm/kprobes.h
+++ b/arch/arm64/include/asm/kprobes.h
@@ -38,13 +38,14 @@ struct kprobe_ctlblk {
 void arch_remove_kprobe(struct kprobe *);
 int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
 void __kretprobe_trampoline(void);
+void __kprobes *trampoline_probe_handler(struct pt_regs *regs);
+#endif /* CONFIG_KPROBES */
+
 int __kprobes kprobe_brk_handler(struct pt_regs *regs,
 				 unsigned long esr);
 int __kprobes kprobe_ss_brk_handler(struct pt_regs *regs,
 				 unsigned long esr);
 int __kprobes kretprobe_brk_handler(struct pt_regs *regs,
 				 unsigned long esr);
-void __kprobes *trampoline_probe_handler(struct pt_regs *regs);
 
-#endif /* CONFIG_KPROBES */
 #endif /* _ARM_KPROBES_H */
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index a5f344e556df..5b199ac70476 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -227,48 +227,50 @@ NOKPROBE_SYMBOL(do_softstep);
 static int call_break_hook(struct pt_regs *regs, unsigned long esr)
 {
 	if (user_mode(regs)) {
-#ifdef CONFIG_UPROBES
-		if (esr_brk_comment(esr) == UPROBES_BRK_IMM)
+		if (IS_ENABLED(CONFIG_UPROBES) &&
+		    esr_brk_comment(esr) == UPROBES_BRK_IMM) {
 			return uprobe_brk_handler(regs, esr);
-#endif
+		}
+
 		return DBG_HOOK_ERROR;
 	}
 
 	if (esr_brk_comment(esr) == BUG_BRK_IMM)
 		return bug_brk_handler(regs, esr);
 
-#ifdef CONFIG_CFI_CLANG
-	if (esr_is_cfi_brk(esr))
+	if (IS_ENABLED(CONFIG_CFI_CLANG) && esr_is_cfi_brk(esr))
 		return cfi_brk_handler(regs, esr);
-#endif
 
 	if (esr_brk_comment(esr) == FAULT_BRK_IMM)
 		return reserved_fault_brk_handler(regs, esr);
 
-#ifdef CONFIG_KASAN_SW_TAGS
-	if ((esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
+	if (IS_ENABLED(CONFIG_KASAN_SW_TAGS) &&
+	    (esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM) {
 		return kasan_brk_handler(regs, esr);
-#endif
-#ifdef CONFIG_UBSAN_TRAP
-	if ((esr_brk_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
+	}
+
+	if (IS_ENABLED(CONFIG_UBSAN_TRAP) &&
+	    (esr_brk_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM) {
 		return ubsan_brk_handler(regs, esr);
-#endif
-#ifdef CONFIG_KGDB
-	if (esr_brk_comment(esr) == KGDB_DYN_DBG_BRK_IMM)
-		return kgdb_brk_handler(regs, esr);
-	if (esr_brk_comment(esr) == KGDB_COMPILED_DBG_BRK_IMM)
-		return kgdb_compiled_brk_handler(regs, esr);
-#endif
-#ifdef CONFIG_KPROBES
-	if (esr_brk_comment(esr) == KPROBES_BRK_IMM)
-		return kprobe_brk_handler(regs, esr);
-	if (esr_brk_comment(esr) == KPROBES_BRK_SS_IMM)
-		return kprobe_ss_brk_handler(regs, esr);
-#endif
-#ifdef CONFIG_KRETPROBES
-	if (esr_brk_comment(esr) == KRETPROBES_BRK_IMM)
+	}
+
+	if (IS_ENABLED(CONFIG_KGDB)) {
+		if (esr_brk_comment(esr) == KGDB_DYN_DBG_BRK_IMM)
+			return kgdb_brk_handler(regs, esr);
+		if (esr_brk_comment(esr) == KGDB_COMPILED_DBG_BRK_IMM)
+			return kgdb_compiled_brk_handler(regs, esr);
+	}
+
+	if (IS_ENABLED(CONFIG_KPROBES)) {
+		if (esr_brk_comment(esr) == KPROBES_BRK_IMM)
+			return kprobe_brk_handler(regs, esr);
+		if (esr_brk_comment(esr) == KPROBES_BRK_SS_IMM)
+			return kprobe_ss_brk_handler(regs, esr);
+	}
+
+	if (IS_ENABLED(CONFIG_KRETPROBES) &&
+	    esr_brk_comment(esr) == KRETPROBES_BRK_IMM)
 		return kretprobe_brk_handler(regs, esr);
-#endif
 
 	return DBG_HOOK_ERROR;
 }

Re: [PATCH v2 03/11] arm64: debug: call step handlers statically

[Will Deacon]

On Mon, May 12, 2025 at 06:43:18PM +0100, Ada Couprie Diaz wrote:
> Software stepping checks for the correct handler by iterating over a list
> of dynamically registered handlers and calling all of them until one
> handles the exception.
> 
> This is the only generic way to handle software stepping handlers in arm64
> as the exception does not provide an immediate that could be checked,
> contrary to software breakpoints.
> 
> However, the registration mechanism is not exported and has only
> two current users : the KGDB stepping handler, and the uprobe single step
> handler.
> Given that one comes from user mode and the other from kernel mode, call
> the appropriate one by checking the source EL of the exception, if it
> is enabled.
> 
> Unify the naming of the handler to XXX_singlestep_handler(), making it
> clear they are related.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/kgdb.h      |  1 +
>  arch/arm64/include/asm/uprobes.h   |  1 +
>  arch/arm64/kernel/debug-monitors.c | 32 +++++++++++++-----------------
>  arch/arm64/kernel/kgdb.c           | 17 +++-------------
>  arch/arm64/kernel/probes/uprobes.c |  9 +--------
>  5 files changed, 20 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
> index 82a76b2102fb..fd287ec38bb7 100644
> --- a/arch/arm64/include/asm/kgdb.h
> +++ b/arch/arm64/include/asm/kgdb.h
> @@ -26,6 +26,7 @@ extern int kgdb_fault_expected;
>  
>  int kgdb_brk_handler(struct pt_regs *regs, unsigned long esr);
>  int kgdb_compiled_brk_handler(struct pt_regs *regs, unsigned long esr);
> +int kgdb_singlestep_handler(struct pt_regs *regs, unsigned long esr);
>  
>  #endif /* !__ASSEMBLY__ */
>  
> diff --git a/arch/arm64/include/asm/uprobes.h b/arch/arm64/include/asm/uprobes.h
> index 3659a79a9f32..e44bbef40eca 100644
> --- a/arch/arm64/include/asm/uprobes.h
> +++ b/arch/arm64/include/asm/uprobes.h
> @@ -29,5 +29,6 @@ struct arch_uprobe {
>  };
>  
>  int uprobe_brk_handler(struct pt_regs *regs, unsigned long esr);
> +int uprobe_singlestep_handler(struct pt_regs *regs, unsigned long esr);
>  
>  #endif
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 4ece4a93b872..81b813e16842 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -200,30 +200,26 @@ void unregister_kernel_step_hook(struct step_hook *hook)
>  }
>  
>  /*
> - * Call registered single step handlers
> + * Call single step handlers
>   * There is no Syndrome info to check for determining the handler.
> - * So we call all the registered handlers, until the right handler is
> - * found which returns zero.
> + * However, there is only one possible handler for user and kernel modes, so
> + * check and call the appropriate one if it is enabled.
>   */
>  static int call_step_hook(struct pt_regs *regs, unsigned long esr)
>  {
> -	struct step_hook *hook;
> -	struct list_head *list;
> -	int retval = DBG_HOOK_ERROR;
> -
> -	list = user_mode(regs) ? &user_step_hook : &kernel_step_hook;
> -
> -	/*
> -	 * Since single-step exception disables interrupt, this function is
> -	 * entirely not preemptible, and we can use rcu list safely here.
> -	 */
> -	list_for_each_entry_rcu(hook, list, node)	{
> -		retval = hook->fn(regs, esr);
> -		if (retval == DBG_HOOK_HANDLED)
> -			break;
> +	if (user_mode(regs)) {
> +#if CONFIG_UPROBES

nit: this should be #ifdef and my compiler complains about that:

  | arch/arm64/kernel/debug-monitors.c:172:5: warning: 'CONFIG_UPROBES' is not defined, evaluates to 0 [-Wundef]

> +		return uprobe_singlestep_handler(regs, esr);
> +#else
> +		return DBG_HOOK_ERROR;
> +#endif

It would probably be cleaner to have a static inline definition of
uprobe_singlestep_handler() when !CONFIG_UPROBES that just returns
DBG_HOOK_ERROR.

>  	}
>  
> -	return retval;
> +#ifdef CONFIG_KGDB
> +	return kgdb_singlestep_handler(regs, esr);
> +#else
> +	return DBG_HOOK_ERROR;
> +#endif

Similarly here.

Will

Re: [PATCH v2 04/11] arm64: debug: remove break/step handler registration infrastructure

[Will Deacon]

On Mon, May 12, 2025 at 06:43:19PM +0100, Ada Couprie Diaz wrote:
> Remove all infrastructure for the dynamic registration previously used by
> software breakpoints and stepping handlers.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/debug-monitors.h | 24 ----------
>  arch/arm64/kernel/debug-monitors.c      | 63 -------------------------
>  2 files changed, 87 deletions(-)

Nice!

Will

Re: [PATCH v2 05/11] arm64: entry: Add entry and exit functions for debug exceptions

[Will Deacon]

On Mon, May 12, 2025 at 06:43:20PM +0100, Ada Couprie Diaz wrote:
> Duplicate the `debug_exception_entry()` and `debug_exception_exit()`

nit: it's debug_exception_enter().

> functions from mm/fault.c, as they are needed to split
> the debug exceptions entry paths from the current unified one.
> The original will be removed in a cleanup patch.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/kernel/entry-common.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index b260ddc4d3e9..92d78b329e67 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -418,6 +418,28 @@ static inline void fp_user_discard(void)
>  	}
>  }
>  
> +/*
> + * In debug exception context, we explicitly disable preemption despite
> + * having interrupts disabled.
> + * This serves two purposes: it makes it much less likely that we would
> + * accidentally schedule in exception context and it will force a warning
> + * if we somehow manage to schedule by accident.
> + */
> +static void debug_exception_enter(struct pt_regs *regs)
> +{
> +	preempt_disable();
> +
> +	/* This code is a bit fragile.  Test it. */
> +	RCU_LOCKDEP_WARN(!rcu_is_watching(), "exception_enter didn't work");
> +}
> +NOKPROBE_SYMBOL(debug_exception_enter);
> +
> +static void debug_exception_exit(struct pt_regs *regs)
> +{
> +	preempt_enable_no_resched();
> +}
> +NOKPROBE_SYMBOL(debug_exception_exit);

Could you make these externally visible (i.e. drop the 'static') so that
you can remove the definitions from mm/fault.c at the same time? Then
you could add the 'static' back at the end when there are no more
external callers.

Will

Re: [PATCH v2 06/11] arm64: debug: split hardware breakpoint exeception entry

[Will Deacon]

nit: typo in $subject ("exeception").

On Mon, May 12, 2025 at 06:43:21PM +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.
> 
> Hardware breakpoints exceptions are generated by the hardware after user
> configuration. As such, they can be exploited when training branch
> predictors outisde of the userspace VA range: they still need to call

"outisde"

> `arm64_apply_bp_hardening()` if needed to mitigate against this attack.
> Move the call to `arm64_apply_bp_hardening()` to `entry-common.c` as
> it is needed for exceptions coming from EL0 only.
> 
> However, they do not need to handle the Cortex-A76 erratum #1463225 as
> it only applies to single stepping exceptions.
> It does not set an address in FAR_EL1 either, only the hardware
> watchpoint does.
>
> Split the hardware breakpoint exception entry, adjust
> the function signature, and handling of the Cortex-A76 erratum to fit
> the behaviour of the exception.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/exception.h |  1 +
>  arch/arm64/kernel/entry-common.c   | 28 ++++++++++++++++++++++++++++
>  arch/arm64/kernel/hw_breakpoint.c  | 15 +++++++++++----
>  3 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
> index d48fc16584cd..c593fe639697 100644
> --- a/arch/arm64/include/asm/exception.h
> +++ b/arch/arm64/include/asm/exception.h
> @@ -61,6 +61,7 @@ void do_el0_gcs(struct pt_regs *regs, unsigned long esr);
>  void do_el1_gcs(struct pt_regs *regs, unsigned long esr);
>  void do_debug_exception(unsigned long addr_if_watchpoint, unsigned long esr,
>  			struct pt_regs *regs);
> +void do_breakpoint(unsigned long esr, struct pt_regs *regs);
>  void do_fpsimd_acc(unsigned long esr, struct pt_regs *regs);
>  void do_sve_acc(unsigned long esr, struct pt_regs *regs);
>  void do_sme_acc(unsigned long esr, struct pt_regs *regs);
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 92d78b329e67..6ff52fc94da7 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -503,6 +503,15 @@ static void noinstr el1_mops(struct pt_regs *regs, unsigned long esr)
>  	exit_to_kernel_mode(regs);
>  }
>  
> +static void noinstr el1_breakpt(struct pt_regs *regs, unsigned long esr)
> +{
> +	arm64_enter_el1_dbg(regs);
> +	debug_exception_enter(regs);
> +	do_breakpoint(esr, regs);
> +	debug_exception_exit(regs);
> +	arm64_exit_el1_dbg(regs);
> +}
> +
>  static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
>  {
>  	unsigned long far = read_sysreg(far_el1);
> @@ -552,6 +561,8 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
>  		el1_mops(regs, esr);
>  		break;
>  	case ESR_ELx_EC_BREAKPT_CUR:
> +		el1_breakpt(regs, esr);
> +		break;
>  	case ESR_ELx_EC_SOFTSTP_CUR:
>  	case ESR_ELx_EC_WATCHPT_CUR:
>  	case ESR_ELx_EC_BRK64:
> @@ -746,6 +757,19 @@ static void noinstr el0_inv(struct pt_regs *regs, unsigned long esr)
>  	exit_to_user_mode(regs);
>  }
>  
> +static void noinstr el0_breakpt(struct pt_regs *regs, unsigned long esr)
> +{
> +	if (!is_ttbr0_addr(regs->pc))
> +		arm64_apply_bp_hardening();

I think this is a change in behaviour, as arm64_apply_bp_hardening() is
now called before enter_from_user_mode() and debug_exception_enter().
Is that safe and intentional?

Will

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

[Will Deacon]

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

Re: [PATCH v2 08/11] arm64: debug: split hardware watchpoint exception entry

[Will Deacon]

On Mon, May 12, 2025 at 06:43:23PM +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.
> 
> Hardware watchpoints are the only debug exceptions that will write
> FAR_EL1, so we need to preserve it and pass it down.
> However, they cannot be used to maliciously train branch predictors, so
> we can omit calling `arm64_bp_hardening()`, nor do they need to handle
> the Cortex-A76 erratum #1463225, as it only applies to single stepping
> exceptions.
> 
> Split the hardware watchpoint exception entry and adjust the behaviour
> to match the lack of needed mitigations.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/exception.h |  2 ++
>  arch/arm64/kernel/entry-common.c   | 31 +++++++++++++++++++++++++++++-
>  arch/arm64/kernel/hw_breakpoint.c  | 14 ++++++++++----
>  3 files changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
> index cbcd832bf58e..eb465c375d34 100644
> --- a/arch/arm64/include/asm/exception.h
> +++ b/arch/arm64/include/asm/exception.h
> @@ -63,6 +63,8 @@ void do_debug_exception(unsigned long addr_if_watchpoint, unsigned long esr,
>  			struct pt_regs *regs);
>  void do_breakpoint(unsigned long esr, struct pt_regs *regs);
>  void do_softstep(unsigned long esr, struct pt_regs *regs);
> +void do_watchpoint(unsigned long addr, unsigned long esr,
> +			struct pt_regs *regs);
>  void do_fpsimd_acc(unsigned long esr, struct pt_regs *regs);
>  void do_sve_acc(unsigned long esr, struct pt_regs *regs);
>  void do_sme_acc(unsigned long esr, struct pt_regs *regs);
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 8814ad24e707..6e70130d2741 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -530,10 +530,20 @@ static void noinstr el1_softstp(struct pt_regs *regs, unsigned long esr)
>  	arm64_exit_el1_dbg(regs);
>  }
>  
> -static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
> +static void noinstr el1_watchpt(struct pt_regs *regs, unsigned long esr)
>  {
> +	/* Only watchpoints write FAR_EL1 */

nit: But maybe scope the comment (here and in the el0 handler) for debug
exceptions?
e.g.

	/* Watchpoints are the only debug exception to write FAR_EL1 */

?

Will

Re: [PATCH v2 10/11] arm64: debug: split bkpt32 exception entry

[Will Deacon]

On Mon, May 12, 2025 at 06:43:25PM +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 BKPT32 exception can only be triggered by a BKPT instruction. Thus,
> we know that the PC is a legitimate address and isn't being used to train
> a branch predictor with a bogus address : we don't need to call
> `arm64_apply_bp_hardening()`.
> 
> The handler for this exception only pends a signal and doesn't depend
> on any per-CPU state : we don't need to inhibit preemption, nor do we
> need to keep the DAIF exceptions masked, so we can unmask them earlier.
> 
> Split the BKPT32 exception entry and adjust function signatures and its
> behaviour to match its relaxed constraints compared to other
> debug exceptions.
> 
> This replaces the last usage of `el0_dbg()`, so remove it.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/exception.h |  1 +
>  arch/arm64/kernel/debug-monitors.c |  9 +++++++++
>  arch/arm64/kernel/entry-common.c   | 21 +++++++++------------
>  3 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
> index af9b521e35f9..4ca43d4eaf98 100644
> --- a/arch/arm64/include/asm/exception.h
> +++ b/arch/arm64/include/asm/exception.h
> @@ -66,6 +66,7 @@ void do_softstep(unsigned long esr, struct pt_regs *regs);
>  void do_watchpoint(unsigned long addr, unsigned long esr,
>  			struct pt_regs *regs);
>  void do_brk64(unsigned long esr, struct pt_regs *regs);
> +void do_bkpt32(unsigned long esr, struct pt_regs *regs);
>  void do_fpsimd_acc(unsigned long esr, struct pt_regs *regs);
>  void do_sve_acc(unsigned long esr, struct pt_regs *regs);
>  void do_sme_acc(unsigned long esr, struct pt_regs *regs);
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 94779c957b70..53f0b2281f14 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -298,6 +298,15 @@ void do_brk64(unsigned long esr, struct pt_regs *regs)
>  }
>  NOKPROBE_SYMBOL(do_brk64);
>  
> +#ifdef CONFIG_COMPAT
> +/* BRKPT exception always traps to be handled. */

nit: I don't understand this comment

Will

Re: [PATCH v2 11/11] arm64: debug: remove debug exception registration infrastructure

[Will Deacon]

On Mon, May 12, 2025 at 06:43:26PM +0100, Ada Couprie Diaz wrote:
> Now that debug exceptions are handled individually and without the need
> for dynamic registration, remove the unused registration infrastructure.
> 
> Remove `early_brk64` as it has been made redundant by
> (arm64: debug: split brk64 exception entry) and is not used anymore.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/debug-monitors.h |  2 -
>  arch/arm64/include/asm/exception.h      |  2 -
>  arch/arm64/include/asm/system_misc.h    |  4 --
>  arch/arm64/kernel/debug-monitors.c      |  3 -
>  arch/arm64/kernel/traps.c               | 26 +--------
>  arch/arm64/mm/fault.c                   | 75 -------------------------
>  6 files changed, 1 insertion(+), 111 deletions(-)

[...]

> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 53f0b2281f14..a5f344e556df 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -346,9 +346,6 @@ int aarch32_break_handler(struct pt_regs *regs)
>  }
>  NOKPROBE_SYMBOL(aarch32_break_handler);
>  
> -void __init debug_traps_init(void)
> -{}
> -
>  /* Re-enable single step for syscall restarting. */
>  void user_rewind_single_step(struct task_struct *task)
>  {
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 50d7a6a75f45..ce20f46e08cb 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -1101,29 +1101,5 @@ int ubsan_brk_handler(struct pt_regs *regs, unsigned long esr)
>  }
>  #endif
>  
> -/*
> - * Initial handler for AArch64 BRK exceptions
> - * This handler only used until debug_traps_init().
> - */
> -int __init early_brk64(unsigned long addr, unsigned long esr,
> -		struct pt_regs *regs)
> -{
> -#ifdef CONFIG_CFI_CLANG
> -	if (esr_is_cfi_brk(esr))
> -		return cfi_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
> -#endif
> -#ifdef CONFIG_KASAN_SW_TAGS
> -	if ((esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
> -		return kasan_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
> -#endif
> -#ifdef CONFIG_UBSAN_TRAP
> -	if ((esr_brk_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
> -		return ubsan_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
> -#endif
> -	return bug_brk_handler(regs, esr) != DBG_HOOK_HANDLED;

The early bug handler is now only called if the brk immediate matches,
but I couldn't spot any problems with that.

Will

Re: [PATCH v2 00/11] arm64: debug: remove hook registration, split exception entry

[Ada Couprie Diaz]

On 16/05/2025 12:57, Luis Claudio R. Goncalves wrote:

> On Tue, May 13, 2025 at 04:19:26PM +0100, Ada Couprie Diaz wrote:
>> Re-sending with proper text format, apologies for the noise...
>>
>> On 13/05/2025 13:25, Luis Claudio R. Goncalves wrote:
>>
>>> On Mon, May 12, 2025 at 06:43:15PM +0100, Ada Couprie Diaz wrote:
>>>> [...]
>>>>
>>>> Single Step Exception
>>>> ===
>>>>
>> Hi Luis,
>>
>> Thanks for taking the time to test, I'm glad it seems OK for now.
>>> Is there any specific test you would like me to run on that test setup I
>>> have?
>> There are a couple of edge-cases that might be problematic if my conclusions
>> are wrong : 1. Race between a step exception being taken, and the related
>> hardware breakpoint/watchpoint being removed 2. Migration of a task stepping
>> a CPU-bound breakpoint/watchpoint
>> [...]
> I ran the two tests you listed above, along with some variations just to
> make sure I got the details right, and all those tests completed flawlessly
> on both machines, on the 4 kernel configurations I tests (all with
> PREEMPT_RT enabled, with and without LOCKDEP and assorted debug features).
Thanks a lot for taking the time to test so exhaustively ! I'm happy to 
hear that this part is holding up : I am confident it should be OK.
>>>> Testing examples
>>>> ===
>>>> [...]
>>>>
>>>> GDB commands (for EL0):
>>>> ~~~
>>>> [...]
> This is the only test where I (consistently) hit backtraces. If I run the
> test with "gdb -x ${COMMAND_LIST_FILE} ..." I get a single backtrace, every
> time:
>
> [  263.890424] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
> [  263.890444] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 5744, name: gdb_prog1
> [  263.890445] preempt_count: 1, expected: 0
> [  263.890446] RCU nest depth: 0, expected: 0
> [  263.890447] 1 lock held by gdb_prog1/5744:
> [  263.890448]  #0: ffff100028496f58 (&sighand->siglock){+.+.}-{3:3}, at: force_sig_info_to_task+0x30/0x150
> [  263.890468] Preemption disabled at:
> [  263.890469] [<ffff8000800391a8>] debug_exception_enter+0x18/0x78
> [  263.890484] CPU: 114 UID: 0 PID: 5744 Comm: gdb_prog1 Tainted: G        W           6.15.0-rc6-rt1__dbg #2 PREEMPT_{RT,(lazy)}
> [  263.890487] Tainted: [W]=WARN
> [  263.890488] Hardware name: Supermicro ARS-221GL-NR-01/G1SMH, BIOS 2.0 07/12/2024
> [  263.890490] Call trace:
> [  263.890492]  show_stack+0x30/0x88 (C)
> [  263.890495]  dump_stack_lvl+0xa0/0xe0
> [  263.890498]  dump_stack+0x14/0x2c
> [  263.890499]  __might_resched+0x170/0x240
> [  263.890506]  rt_spin_lock+0x6c/0x1a0
> [  263.890512]  force_sig_info_to_task+0x30/0x150
> [  263.890513]  force_sig_fault+0x68/0xa0
> [  263.890515]  arm64_force_sig_fault+0x44/0x80
> [  263.890518]  send_user_sigtrap+0x60/0xa8
> [  263.890520]  do_brk64+0x40/0x88
> [  263.890522]  el0_brk64+0x50/0x1c0
> [  263.890526]  el0t_64_sync_handler+0x60/0xe0
> [  263.890528]  el0t_64_sync+0x184/0x188
>
> Quite similar to the problem originally reported, where sending signals
> with preemption disabled could trigger the "rtlock_might_resched();" check
> if CONFIG_DEBUG_ATOMIC_SLEEP is enabled.

Oh, indeed : I can confirm that this happens both with my series and on 
mainline tags v6.15-rc6, v6.15.

I didn't see it originally, but as you point out it shows up 
consistently with CONFIG_DEBUG_ATOMIC_SLEEP enabled.

> If I call gdb and run manually the sequence of commands you described, I
> get the backtrace above three times. The only difference is that on the
> second backtrace I get these extra elements on the header:
>
>    [48052.129422] RCU nest depth: 1, expected: 1
>    [48052.129424] 2 locks held by gdb_prog1/27451:
>    [48052.129425]  #0: ffff8000828315c8 (rcu_read_lock){....}-{1:3}, at: breakpoint_handler+0xd8/0x318
>    [48052.129439]  #1: ffff00008abd92d8 (&sighand->siglock){+.+.}-{3:3}, at: force_sig_info_to_task+0x30/0x150
>
> So, when I enter manually the GDB command you suggested, the result is:
>
>      start           <--- Backtrace#1:   preempt_count: 1
>      hbreak 3
>      watch target
>      commands 2
>      continue
>      end
>      commands 3
>      continue
>      end
>      continue        <--- Backtrace#2:   preempt_count: 1   RCU nest depth: 1
>      jump 11         <--- Backtrace#3:   preempt_count: 1
>      continue
>      quit
>
> I hope this report is helpful.

Very much so, thanks !

I am looking into fixing this in v3, I feel this series is a good 
opportunity to do it.

> IMHO, even with these backtraces, there was a considerable enhancement when
> compared to the original scenario we reported.
>
> Best regards,
> Luis

I'm glad that the fix works well under more heavy testing.

Best regards,
Ada

Re: [PATCH v2 08/11] arm64: debug: split hardware watchpoint exception entry

[Ada Couprie Diaz]

On 20/05/2025 17:59, Will Deacon wrote:

> On Mon, May 12, 2025 at 06:43:23PM +0100, Ada Couprie Diaz wrote:
>> [...]
>> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
>> index 8814ad24e707..6e70130d2741 100644
>> --- a/arch/arm64/kernel/entry-common.c
>> +++ b/arch/arm64/kernel/entry-common.c
>> @@ -530,10 +530,20 @@ static void noinstr el1_softstp(struct pt_regs *regs, unsigned long esr)
>>   	arm64_exit_el1_dbg(regs);
>>   }
>>   
>> -static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
>> +static void noinstr el1_watchpt(struct pt_regs *regs, unsigned long esr)
>>   {
>> +	/* Only watchpoints write FAR_EL1 */
> nit: But maybe scope the comment (here and in the el0 handler) for debug
> exceptions?
> e.g.
>
> 	/* Watchpoints are the only debug exception to write FAR_EL1 */
>
> ?
>
> Will

Good point. The comment felt somewhat off to me and that's exactly why.

Updated the wording for v3, thanks.

Ada

Re: [PATCH v2 05/11] arm64: entry: Add entry and exit functions for debug exceptions

[Ada Couprie Diaz]

On 20/05/2025 16:36, Will Deacon wrote:

> On Mon, May 12, 2025 at 06:43:20PM +0100, Ada Couprie Diaz wrote:
>> Duplicate the `debug_exception_entry()` and `debug_exception_exit()`
> nit: it's debug_exception_enter().
Thanks,fixed for v3.
>> functions from mm/fault.c, as they are needed to split
>> the debug exceptions entry paths from the current unified one.
>> The original will be removed in a cleanup patch.
>>
>> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
>> ---
>>   arch/arm64/kernel/entry-common.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
>> index b260ddc4d3e9..92d78b329e67 100644
>> --- a/arch/arm64/kernel/entry-common.c
>> +++ b/arch/arm64/kernel/entry-common.c
>> @@ -418,6 +418,28 @@ static inline void fp_user_discard(void)
>>   	}
>>   }
>>   
>> +/*
>> + * In debug exception context, we explicitly disable preemption despite
>> + * having interrupts disabled.
>> + * This serves two purposes: it makes it much less likely that we would
>> + * accidentally schedule in exception context and it will force a warning
>> + * if we somehow manage to schedule by accident.
>> + */
>> +static void debug_exception_enter(struct pt_regs *regs)
>> +{
>> +	preempt_disable();
>> +
>> +	/* This code is a bit fragile.  Test it. */
>> +	RCU_LOCKDEP_WARN(!rcu_is_watching(), "exception_enter didn't work");
>> +}
>> +NOKPROBE_SYMBOL(debug_exception_enter);
>> +
>> +static void debug_exception_exit(struct pt_regs *regs)
>> +{
>> +	preempt_enable_no_resched();
>> +}
>> +NOKPROBE_SYMBOL(debug_exception_exit);
> Could you make these externally visible (i.e. drop the 'static') so that
> you can remove the definitions from mm/fault.c at the same time? Then
> you could add the 'static' back at the end when there are no more
> external callers.
>
> Will
If you are happy with some header noise, adding `debug_exception_enter()`
and `debug_exception_exit()` to include/asm/exception.h` so that they can
be visible in `mm/fault.c` during the clean up and removing them when
done, I would much prefer doing it that way !

It felt strange to just have some duplicate code, but I didn't know
what would be acceptable.
I will make the change for v3, unless the header noise mentioned above is
an issue.

Thanks,
Ada

Re: [PATCH v2 06/11] arm64: debug: split hardware breakpoint exeception entry

[Mark Rutland]

On Tue, May 20, 2025 at 04:36:39PM +0100, Will Deacon wrote:
> On Mon, May 12, 2025 at 06:43:21PM +0100, Ada Couprie Diaz wrote:
> > +static void noinstr el0_breakpt(struct pt_regs *regs, unsigned long esr)
> > +{
> > +	if (!is_ttbr0_addr(regs->pc))
> > +		arm64_apply_bp_hardening();
> 
> I think this is a change in behaviour, as arm64_apply_bp_hardening() is
> now called before enter_from_user_mode() and debug_exception_enter().
> Is that safe and intentional?

Yes on both counts:

* It's safe. The arm64_apply_bp_hardening() helper, and the callbacks
  that it may call are all noinstr, and are written to be safe to call
  in this environment.

* It's intentional. The goal was to do this as soon as reasonably
  possible, at least before unmasking exceptions, without incurring the
  cost for exceptions where this didn't matter.

  We've already executed a bunch of code to get here, and moving this
  after enter_from_user_mode() should be fine.

This is inteneded to look the same as el0_ia() and el0_pc(), which both
call arm64_apply_bp_hardening() before enter_from_user_mode().

Mark.

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

[Mark Rutland]

On Tue, May 20, 2025 at 05:29:14PM +0100, Will Deacon wrote:
> On Mon, May 12, 2025 at 06:43:22PM +0100, Ada Couprie Diaz wrote:
> > +	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?

How about:

	if (!try_step_suspended_breakpoints(regs))

... that'd match the naming in do_el0_undef() and do_el1_undef() in
traps.c, where we have try_${HANDLE_POTENTIAL_CASE}() for a few cases,
e.g.

| void do_el0_undef(struct pt_regs *regs, unsigned long esr) 
| {
|         u32 insn;
| 
|         /* check for AArch32 breakpoint instructions */
|         if (!aarch32_break_handler(regs))
|                 return;
| 
|         if (user_insn_read(regs, &insn))
|                 goto out_err;
| 
|         if (try_emulate_mrs(regs, insn))
|                 return;
| 
|         if (try_emulate_armv8_deprecated(regs, insn))
|                 return;
| 
| out_err:
|         force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc, 0);
| }

Mark.

Re: [PATCH v2 08/11] arm64: debug: split hardware watchpoint exception entry

[Mark Rutland]

On Wed, May 28, 2025 at 02:47:52PM +0100, Ada Couprie Diaz wrote:
> On 20/05/2025 17:59, Will Deacon wrote:
> 
> > On Mon, May 12, 2025 at 06:43:23PM +0100, Ada Couprie Diaz wrote:
> > > [...]
> > > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > > index 8814ad24e707..6e70130d2741 100644
> > > --- a/arch/arm64/kernel/entry-common.c
> > > +++ b/arch/arm64/kernel/entry-common.c
> > > @@ -530,10 +530,20 @@ static void noinstr el1_softstp(struct pt_regs *regs, unsigned long esr)
> > >   	arm64_exit_el1_dbg(regs);
> > >   }
> > > -static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
> > > +static void noinstr el1_watchpt(struct pt_regs *regs, unsigned long esr)
> > >   {
> > > +	/* Only watchpoints write FAR_EL1 */
> > nit: But maybe scope the comment (here and in the el0 handler) for debug
> > exceptions?
> > e.g.
> > 
> > 	/* Watchpoints are the only debug exception to write FAR_EL1 */
> > 
> > ?
> > 
> > Will
> 
> Good point. The comment felt somewhat off to me and that's exactly why.
> 
> Updated the wording for v3, thanks.

More of a question for Will, but could we drop the comment entirely?

Historically the same comment in the common el0_dbg() function was a
useful warning because we were forced to read FAR even for
non-watchpoints, but as of this restructuring that awkwardness is gone.

As of this series, for the other debug exceptions, the
el{0,1}_${EXCEPTIONNAME}() handlers don't currently read FAR, and their
callees don't have a 'far' argument.

I'm happy either way, in case you'd prefer that it stays.

Mark.

Re: [PATCH v2 03/11] arm64: debug: call step handlers statically

[Ada Couprie Diaz]

On 20/05/2025 16:35, Will Deacon wrote:

> On Mon, May 12, 2025 at 06:43:18PM +0100, Ada Couprie Diaz wrote:
>> +#if CONFIG_UPROBES
> nit: this should be #ifdef and my compiler complains about that:
>
>    | arch/arm64/kernel/debug-monitors.c:172:5: warning: 'CONFIG_UPROBES' is not defined, evaluates to 0 [-Wundef]
Indeed, that's my bad.
Replacing it in v3 with an #ifndef for the stand-in.
>> +		return uprobe_singlestep_handler(regs, esr);
>> +#else
>> +		return DBG_HOOK_ERROR;
>> +#endif
> It would probably be cleaner to have a static inline definition of
> uprobe_singlestep_handler() when !CONFIG_UPROBES that just returns
> DBG_HOOK_ERROR.
>
>>   	}
>>   
>> -	return retval;
>> +#ifdef CONFIG_KGDB
>> +	return kgdb_singlestep_handler(regs, esr);
>> +#else
>> +	return DBG_HOOK_ERROR;
>> +#endif
> Similarly here.

That's fair, it does make the function itself very clear !

Replaced the #ifdefs in the function with static inline stand-ins
when the configs are disabled for v3.

> Will

Thanks,

Ada

Re: [PATCH v2 06/11] arm64: debug: split hardware breakpoint exeception entry

[Ada Couprie Diaz]

On 28/05/2025 16:17, Mark Rutland wrote:

> On Tue, May 20, 2025 at 04:36:39PM +0100, Will Deacon wrote:
>> On Mon, May 12, 2025 at 06:43:21PM +0100, Ada Couprie Diaz wrote:
>>> +static void noinstr el0_breakpt(struct pt_regs *regs, unsigned long esr)
>>> +{
>>> +	if (!is_ttbr0_addr(regs->pc))
>>> +		arm64_apply_bp_hardening();
>> I think this is a change in behaviour, as arm64_apply_bp_hardening() is
>> now called before enter_from_user_mode() and debug_exception_enter().
>> Is that safe and intentional?
> Yes on both counts:
>
> * It's safe. The arm64_apply_bp_hardening() helper, and the callbacks
>    that it may call are all noinstr, and are written to be safe to call
>    in this environment.
>
> * It's intentional. The goal was to do this as soon as reasonably
>    possible, at least before unmasking exceptions, without incurring the
>    cost for exceptions where this didn't matter.
>
>    We've already executed a bunch of code to get here, and moving this
>    after enter_from_user_mode() should be fine.
>
> This is inteneded to look the same as el0_ia() and el0_pc(), which both
> call arm64_apply_bp_hardening() before enter_from_user_mode().
>
> Mark.
A you mention in the other patch Will, I can definitely highlight
those points in the commit message for v3, especially that it is safe
and already done by `el0_ia()` and `el0_pc()`.

Thanks both,
Ada

Re: [PATCH v2 11/11] arm64: debug: remove debug exception registration infrastructure

[Ada Couprie Diaz]

On 21/05/2025 10:38, Will Deacon wrote:

> On Mon, May 12, 2025 at 06:43:26PM +0100, Ada Couprie Diaz wrote:
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index 50d7a6a75f45..ce20f46e08cb 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -1101,29 +1101,5 @@ int ubsan_brk_handler(struct pt_regs *regs, unsigned long esr)
>>   }
>>   #endif
>>   
>> -/*
>> - * Initial handler for AArch64 BRK exceptions
>> - * This handler only used until debug_traps_init().
>> - */
>> -int __init early_brk64(unsigned long addr, unsigned long esr,
>> -		struct pt_regs *regs)
>> -{
>> -#ifdef CONFIG_CFI_CLANG
>> -	if (esr_is_cfi_brk(esr))
>> -		return cfi_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
>> -#endif
>> -#ifdef CONFIG_KASAN_SW_TAGS
>> -	if ((esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
>> -		return kasan_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
>> -#endif
>> -#ifdef CONFIG_UBSAN_TRAP
>> -	if ((esr_brk_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
>> -		return ubsan_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
>> -#endif
>> -	return bug_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
> The early bug handler is now only called if the brk immediate matches,
> but I couldn't spot any problems with that.

Correct, as far as I can tell the behaviour is unchanged for a 
fall-through :
`bug_brk_handler()` would have called `report_bug()`, which would
not have found a non-BUG BRK and bailed early with BUG_TRAP_TYPE_NONE,
leading `bug_brk_handler()` to return `DBG_HOOK_ERROR`.

Now, we would return `DBG_HOOK_ERROR` anyway if it's not an immediate we 
know how to handle. (This also seems to match better with the 
expectations of the comment in `is_valid_bugaddr()`, in `kernel/traps.c` !)

> Will

Happy to have my logic checked or add a mention in a commit message
(either this one, or patch 02 ?)

Thanks,
Ada

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

[Will Deacon]

On Wed, May 28, 2025 at 04:22:05PM +0100, Mark Rutland wrote:
> On Tue, May 20, 2025 at 05:29:14PM +0100, Will Deacon wrote:
> > On Mon, May 12, 2025 at 06:43:22PM +0100, Ada Couprie Diaz wrote:
> > > +	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?
> 
> How about:
> 
> 	if (!try_step_suspended_breakpoints(regs))
> 
> ... that'd match the naming in do_el0_undef() and do_el1_undef() in
> traps.c, where we have try_${HANDLE_POTENTIAL_CASE}() for a few cases,
> e.g.

That's even better, thanks.

Will

Re: [PATCH v2 05/11] arm64: entry: Add entry and exit functions for debug exceptions

[Will Deacon]

On Wed, May 28, 2025 at 03:08:35PM +0100, Ada Couprie Diaz wrote:
> On 20/05/2025 16:36, Will Deacon wrote:
> 
> > On Mon, May 12, 2025 at 06:43:20PM +0100, Ada Couprie Diaz wrote:
> > > Duplicate the `debug_exception_entry()` and `debug_exception_exit()`
> > nit: it's debug_exception_enter().
> Thanks,fixed for v3.
> > > functions from mm/fault.c, as they are needed to split
> > > the debug exceptions entry paths from the current unified one.
> > > The original will be removed in a cleanup patch.
> > > 
> > > Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> > > ---
> > >   arch/arm64/kernel/entry-common.c | 22 ++++++++++++++++++++++
> > >   1 file changed, 22 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > > index b260ddc4d3e9..92d78b329e67 100644
> > > --- a/arch/arm64/kernel/entry-common.c
> > > +++ b/arch/arm64/kernel/entry-common.c
> > > @@ -418,6 +418,28 @@ static inline void fp_user_discard(void)
> > >   	}
> > >   }
> > > +/*
> > > + * In debug exception context, we explicitly disable preemption despite
> > > + * having interrupts disabled.
> > > + * This serves two purposes: it makes it much less likely that we would
> > > + * accidentally schedule in exception context and it will force a warning
> > > + * if we somehow manage to schedule by accident.
> > > + */
> > > +static void debug_exception_enter(struct pt_regs *regs)
> > > +{
> > > +	preempt_disable();
> > > +
> > > +	/* This code is a bit fragile.  Test it. */
> > > +	RCU_LOCKDEP_WARN(!rcu_is_watching(), "exception_enter didn't work");
> > > +}
> > > +NOKPROBE_SYMBOL(debug_exception_enter);
> > > +
> > > +static void debug_exception_exit(struct pt_regs *regs)
> > > +{
> > > +	preempt_enable_no_resched();
> > > +}
> > > +NOKPROBE_SYMBOL(debug_exception_exit);
> > Could you make these externally visible (i.e. drop the 'static') so that
> > you can remove the definitions from mm/fault.c at the same time? Then
> > you could add the 'static' back at the end when there are no more
> > external callers.
> > 
> If you are happy with some header noise, adding `debug_exception_enter()`
> and `debug_exception_exit()` to include/asm/exception.h` so that they can
> be visible in `mm/fault.c` during the clean up and removing them when
> done, I would much prefer doing it that way !
> 
> It felt strange to just have some duplicate code, but I didn't know
> what would be acceptable.
> I will make the change for v3, unless the header noise mentioned above is
> an issue.

Thanks, that sounds great.

Will

Re: [PATCH v2 08/11] arm64: debug: split hardware watchpoint exception entry

[Will Deacon]

On Wed, May 28, 2025 at 04:42:46PM +0100, Mark Rutland wrote:
> On Wed, May 28, 2025 at 02:47:52PM +0100, Ada Couprie Diaz wrote:
> > On 20/05/2025 17:59, Will Deacon wrote:
> > 
> > > On Mon, May 12, 2025 at 06:43:23PM +0100, Ada Couprie Diaz wrote:
> > > > [...]
> > > > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > > > index 8814ad24e707..6e70130d2741 100644
> > > > --- a/arch/arm64/kernel/entry-common.c
> > > > +++ b/arch/arm64/kernel/entry-common.c
> > > > @@ -530,10 +530,20 @@ static void noinstr el1_softstp(struct pt_regs *regs, unsigned long esr)
> > > >   	arm64_exit_el1_dbg(regs);
> > > >   }
> > > > -static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
> > > > +static void noinstr el1_watchpt(struct pt_regs *regs, unsigned long esr)
> > > >   {
> > > > +	/* Only watchpoints write FAR_EL1 */
> > > nit: But maybe scope the comment (here and in the el0 handler) for debug
> > > exceptions?
> > > e.g.
> > > 
> > > 	/* Watchpoints are the only debug exception to write FAR_EL1 */
> > > 
> > > ?
> > > 
> > 
> > Good point. The comment felt somewhat off to me and that's exactly why.
> > 
> > Updated the wording for v3, thanks.
> 
> More of a question for Will, but could we drop the comment entirely?
> 
> Historically the same comment in the common el0_dbg() function was a
> useful warning because we were forced to read FAR even for
> non-watchpoints, but as of this restructuring that awkwardness is gone.
> 
> As of this series, for the other debug exceptions, the
> el{0,1}_${EXCEPTIONNAME}() handlers don't currently read FAR, and their
> callees don't have a 'far' argument.
> 
> I'm happy either way, in case you'd prefer that it stays.

I'd prefer to keep it, just in case somebody smart reworks the code and
we end up trying to use the FAR for other debug exceptions.

Will

Re: [PATCH v2 11/11] arm64: debug: remove debug exception registration infrastructure

[Will Deacon]

On Wed, May 28, 2025 at 05:41:43PM +0100, Ada Couprie Diaz wrote:
> On 21/05/2025 10:38, Will Deacon wrote:
> 
> > On Mon, May 12, 2025 at 06:43:26PM +0100, Ada Couprie Diaz wrote:
> > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > > index 50d7a6a75f45..ce20f46e08cb 100644
> > > --- a/arch/arm64/kernel/traps.c
> > > +++ b/arch/arm64/kernel/traps.c
> > > @@ -1101,29 +1101,5 @@ int ubsan_brk_handler(struct pt_regs *regs, unsigned long esr)
> > >   }
> > >   #endif
> > > -/*
> > > - * Initial handler for AArch64 BRK exceptions
> > > - * This handler only used until debug_traps_init().
> > > - */
> > > -int __init early_brk64(unsigned long addr, unsigned long esr,
> > > -		struct pt_regs *regs)
> > > -{
> > > -#ifdef CONFIG_CFI_CLANG
> > > -	if (esr_is_cfi_brk(esr))
> > > -		return cfi_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
> > > -#endif
> > > -#ifdef CONFIG_KASAN_SW_TAGS
> > > -	if ((esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
> > > -		return kasan_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
> > > -#endif
> > > -#ifdef CONFIG_UBSAN_TRAP
> > > -	if ((esr_brk_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
> > > -		return ubsan_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
> > > -#endif
> > > -	return bug_brk_handler(regs, esr) != DBG_HOOK_HANDLED;
> > The early bug handler is now only called if the brk immediate matches,
> > but I couldn't spot any problems with that.
> 
> Correct, as far as I can tell the behaviour is unchanged for a fall-through
> :
> `bug_brk_handler()` would have called `report_bug()`, which would
> not have found a non-BUG BRK and bailed early with BUG_TRAP_TYPE_NONE,
> leading `bug_brk_handler()` to return `DBG_HOOK_ERROR`.
> 
> Now, we would return `DBG_HOOK_ERROR` anyway if it's not an immediate we
> know how to handle. (This also seems to match better with the expectations
> of the comment in `is_valid_bugaddr()`, in `kernel/traps.c` !)
> 
> Happy to have my logic checked or add a mention in a commit message
> (either this one, or patch 02 ?)

It wouldn't hurt to have a note there, but I don't mind either way. I
just highlighted it to check that we agreed.

Will

Re: [PATCH v2 10/11] arm64: debug: split bkpt32 exception entry

[Ada Couprie Diaz]

On 21/05/2025 10:07, Will Deacon wrote:

> On Mon, May 12, 2025 at 06:43:25PM +0100, Ada Couprie Diaz wrote:
>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>> index 94779c957b70..53f0b2281f14 100644
>> --- a/arch/arm64/kernel/debug-monitors.c
>> +++ b/arch/arm64/kernel/debug-monitors.c
>> @@ -298,6 +298,15 @@ void do_brk64(unsigned long esr, struct pt_regs *regs)
>>   }
>>   NOKPROBE_SYMBOL(do_brk64);
>>   
>> +#ifdef CONFIG_COMPAT
>> +/* BRKPT exception always traps to be handled. */
> nit: I don't understand this comment
>
> Will
We don't do any handling of BKPT in the kernel and let userspace do it, 
so I think I am stretching the meaning of "trap" a bit here to mean 
"send a SIGTRAP to userspace".
I'll keep that in mind in the future and be more precise.

However, given that `do_sp_pc_abort()` is a very similar handler and we 
don't make a note of it, I'll remove the comment : it doesn't feel 
really useful.

Thanks,
Ada

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

[Ada Couprie Diaz]

On 29/05/2025 11:10, Will Deacon wrote:
> On Wed, May 28, 2025 at 04:22:05PM +0100, Mark Rutland wrote:
>> On Tue, May 20, 2025 at 05:29:14PM +0100, Will Deacon wrote:
>>> On Mon, May 12, 2025 at 06:43:22PM +0100, Ada Couprie Diaz wrote:
>>>> +	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?
>> How about:
>>
>> 	if (!try_step_suspended_breakpoints(regs))
>>
>> ... that'd match the naming in do_el0_undef() and do_el1_undef() in
>> traps.c, where we have try_${HANDLE_POTENTIAL_CASE}() for a few cases,
>> e.g.
> That's even better, thanks.
>
> Will

Happy to rename ! I think it will make the whole stepping logic easier 
to understand.

Thanks both,
Ada

Re: [PATCH v2 02/11] arm64: debug: call software break handlers statically

[Ada Couprie Diaz]

On 20/05/2025 16:35, Will Deacon wrote:

> On Mon, May 12, 2025 at 06:43:17PM +0100, Ada Couprie Diaz wrote:
>> diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
>> index be7a3680dadf..b27dd6028e6a 100644
>> --- a/arch/arm64/include/asm/kprobes.h
>> +++ b/arch/arm64/include/asm/kprobes.h
>> @@ -38,6 +38,12 @@ struct kprobe_ctlblk {
>>   void arch_remove_kprobe(struct kprobe *);
>>   int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
>>   void __kretprobe_trampoline(void);
>> +int __kprobes kprobe_brk_handler(struct pt_regs *regs,
>> +				 unsigned long esr);
>> +int __kprobes kprobe_ss_brk_handler(struct pt_regs *regs,
>> +				 unsigned long esr);
>> +int __kprobes kretprobe_brk_handler(struct pt_regs *regs,
>> +				 unsigned long esr);
>>   void __kprobes *trampoline_probe_handler(struct pt_regs *regs);
>>   
>>   #endif /* CONFIG_KPROBES */
> If you add these _after_ the #endif...
>
>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>> index 676fa0231935..4ece4a93b872 100644
>> --- a/arch/arm64/kernel/debug-monitors.c
>> +++ b/arch/arm64/kernel/debug-monitors.c
>> @@ -21,8 +21,11 @@
>>   #include <asm/cputype.h>
>>   #include <asm/daifflags.h>
>>   #include <asm/debug-monitors.h>
>> +#include <asm/kgdb.h>
>> +#include <asm/kprobes.h>
>>   #include <asm/system_misc.h>
>>   #include <asm/traps.h>
>> +#include <asm/uprobes.h>
>>   
>>   /* Determine debug architecture. */
>>   u8 debug_monitors_arch(void)
>> @@ -299,20 +302,50 @@ void unregister_kernel_break_hook(struct break_hook *hook)
>>   
>>   static int call_break_hook(struct pt_regs *regs, unsigned long esr)
>>   {
>> -	struct break_hook *hook;
>> -	struct list_head *list;
>> -
>> -	list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
>> -
>> -	/*
>> -	 * Since brk exception disables interrupt, this function is
>> -	 * entirely not preemptible, and we can use rcu list safely here.
>> -	 */
>> -	list_for_each_entry_rcu(hook, list, node) {
>> -		if ((esr_brk_comment(esr) & ~hook->mask) == hook->imm)
>> -			return hook->fn(regs, esr);
>> +	if (user_mode(regs)) {
>> +#ifdef CONFIG_UPROBES
>> +		if (esr_brk_comment(esr) == UPROBES_BRK_IMM)
>> +			return uprobe_brk_handler(regs, esr);
>> +#endif
>> +		return DBG_HOOK_ERROR;
>>   	}
>>   
>> +	if (esr_brk_comment(esr) == BUG_BRK_IMM)
>> +		return bug_brk_handler(regs, esr);
>> +
>> +#ifdef CONFIG_CFI_CLANG
>> +	if (esr_is_cfi_brk(esr))
>> +		return cfi_brk_handler(regs, esr);
>> +#endif
>> +
>> +	if (esr_brk_comment(esr) == FAULT_BRK_IMM)
>> +		return reserved_fault_brk_handler(regs, esr);
>> +
>> +#ifdef CONFIG_KASAN_SW_TAGS
>> +	if ((esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
>> +		return kasan_brk_handler(regs, esr);
>> +#endif
>> +#ifdef CONFIG_UBSAN_TRAP
>> +	if ((esr_brk_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
>> +		return ubsan_brk_handler(regs, esr);
>> +#endif
>> +#ifdef CONFIG_KGDB
>> +	if (esr_brk_comment(esr) == KGDB_DYN_DBG_BRK_IMM)
>> +		return kgdb_brk_handler(regs, esr);
>> +	if (esr_brk_comment(esr) == KGDB_COMPILED_DBG_BRK_IMM)
>> +		return kgdb_compiled_brk_handler(regs, esr);
>> +#endif
>> +#ifdef CONFIG_KPROBES
>> +	if (esr_brk_comment(esr) == KPROBES_BRK_IMM)
>> +		return kprobe_brk_handler(regs, esr);
>> +	if (esr_brk_comment(esr) == KPROBES_BRK_SS_IMM)
>> +		return kprobe_ss_brk_handler(regs, esr);
>> +#endif
>> +#ifdef CONFIG_KRETPROBES
>> +	if (esr_brk_comment(esr) == KRETPROBES_BRK_IMM)
>> +		return kretprobe_brk_handler(regs, esr);
>> +#endif
> ... then I think you can use IS_ENABLED() here instead of the #ifdefs.
> I didn't check everything, but the diff I was hacking on is below.
>
> Will
Oh, I didn't know about `IS_ENABLED()` !
I took the time to check that all the other functions are 
unconditionally declared
and test a few configs : it seems to be all good and it is much more 
readable than with the #ifdefs.

Thanks for the suggestion !
Ada

Re: [PATCH v2 00/11] arm64: debug: remove hook registration, split exception entry

[Ada Couprie Diaz]

On 28/05/2025 11:38, Ada Couprie Diaz wrote:

> On 16/05/2025 12:57, Luis Claudio R. Goncalves wrote:
>> On Tue, May 13, 2025 at 04:19:26PM +0100, Ada Couprie Diaz wrote:
>>
>> This is the only test where I (consistently) hit backtraces. If I run 
>> the
>> test with "gdb -x ${COMMAND_LIST_FILE} ..." I get a single backtrace, 
>> every
>> time:
>>
>> [  263.890424] BUG: sleeping function called from invalid context at 
>> kernel/locking/spinlock_rt.c:48
>> [  263.890444] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 
>> 5744, name: gdb_prog1
>> [  263.890445] preempt_count: 1, expected: 0
>> [  263.890446] RCU nest depth: 0, expected: 0
>> [  263.890447] 1 lock held by gdb_prog1/5744:
>> [  263.890448]  #0: ffff100028496f58 (&sighand->siglock){+.+.}-{3:3}, 
>> at: force_sig_info_to_task+0x30/0x150
>> [  263.890468] Preemption disabled at:
>> [  263.890469] [<ffff8000800391a8>] debug_exception_enter+0x18/0x78
>> [  263.890484] CPU: 114 UID: 0 PID: 5744 Comm: gdb_prog1 Tainted: 
>> G        W           6.15.0-rc6-rt1__dbg #2 PREEMPT_{RT,(lazy)}
>> [  263.890487] Tainted: [W]=WARN
>> [  263.890488] Hardware name: Supermicro ARS-221GL-NR-01/G1SMH, BIOS 
>> 2.0 07/12/2024
>> [  263.890490] Call trace:
>> [  263.890492]  show_stack+0x30/0x88 (C)
>> [  263.890495]  dump_stack_lvl+0xa0/0xe0
>> [  263.890498]  dump_stack+0x14/0x2c
>> [  263.890499]  __might_resched+0x170/0x240
>> [  263.890506]  rt_spin_lock+0x6c/0x1a0
>> [  263.890512]  force_sig_info_to_task+0x30/0x150
>> [  263.890513]  force_sig_fault+0x68/0xa0
>> [  263.890515]  arm64_force_sig_fault+0x44/0x80
>> [  263.890518]  send_user_sigtrap+0x60/0xa8
>> [  263.890520]  do_brk64+0x40/0x88
>> [  263.890522]  el0_brk64+0x50/0x1c0
>> [  263.890526]  el0t_64_sync_handler+0x60/0xe0
>> [  263.890528]  el0t_64_sync+0x184/0x188
>>
>> Quite similar to the problem originally reported, where sending signals
>> with preemption disabled could trigger the "rtlock_might_resched();" 
>> check
>> if CONFIG_DEBUG_ATOMIC_SLEEP is enabled.
>
> Oh, indeed : I can confirm that this happens both with my series and 
> on mainline tags v6.15-rc6, v6.15.
>
> I didn't see it originally, but as you point out it shows up 
> consistently with CONFIG_DEBUG_ATOMIC_SLEEP enabled.
>
> [...]
>
> I am looking into fixing this in v3, I feel this series is a good 
> opportunity to do it.
>
As you mentioned the issue is very similar to the single-step one,
and we already have done most of the work for the fix.
Indeed, the handling of the software breakpoint instruction
from EL0 is very similar to the single step exception.
In fact : the exact same logic applies and we can safely
enable preemption before calling `do_brk64()`, given that the only
possible path from EL0 is the uprobe handler, which sets a TIF
to be handled on exit, as with the single step.

So this is fixed in v3.


However, as the other traces you hit suggested : there is also an issue
with the hardware breakpoint and watchpoint handlers.
Here it is much more painful though.
Indeed : the signal is sent from within the ptrace handler
`ptrace_hbptriggered()`, itself called from the `perf_bp_event()`, called
in a loop in the handlers to find matching {break,watch}points.
This makes it *very* impractical to enable preemption at any point
for those debug exceptions.

Looking into how x86 handles it, they do not seem to have the issue
as they do not send a signal in their hardware breakpoint handler,
rather they set a "virtual register" that should be seen by userspace.
(If my understanding is correct !)

However, they use a `notify_die()` callback to call their hardware
breakpoint handler : `hw_breakpoint_exceptions_notify()`.
We do have it for arm64, but it is a stub.
We might be able to use this callback to delay the signal until we
are done operating on the hardware registers, which definitely needs
preemption disabled.


However, this is becoming a much larger change whose scope I am
not sure of yet, so I will not be looking into fixing it in this series.
Hopefully it can be done at a later date !

Best,
Ada