[PATCH v3 00/13] arm64: debug: remove hook registration, split exception entry

[PATCH v3 00/13] 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 : related refactor of `aarch32_break_handler()`
3-5 : software breakpoints and single step handlers registration removal
6: preparatory function move that is made internal in patch 13
8: preparatory refactor of `reinstall_suspended_bps()`
7, 9-13 : debug exception splitting and handler registration removal

* Patch 3 copies and extends the `early_brk64` break handling for the
normal path, byassing the dynamic registration.
* Patch 4 does something similar for the single stepping handlers.
* Patches 7, and 9 through 13 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.
* Patch 8 refactors `reinstall_suspended_bps()` in preparation for the
single-step exception entry split, as it is moved out of the handler.
This could be squashed in patch 9 (the single-step exception split), but
I opted to separate it for clarity (and because the commit message
for patch 9 is already 45 lines long !).
* Patches 5 and 13 are clean-ups removing the code that has been replaced
and made redundant by the preceding patches.

PREEMPT_RT
===

Of note, this allows us to make the single exception handling mostly
preemptible coming from EL0 in patch 9, 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 as he did a lot of testing on v2 and originally
reported the issue.

Cc: "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>

We can do the same for the software break exception coming from EL0,
which works in a very similar way.

However, the hardware breakpoint and watchpoint exceptions also have
a sleeping while non-preemptible issue with PREEMPT_RT, but this is
much trickier to fix, so this won't be fixed in this series.
A bit more details in [1].

I hesitated to add comments within the BRK64 and single-step handlers,
on the EL0 paths, to highlight that they would be preemptible and to be
wary if modifying or adding to them, but I opted not to in the end.
(Partly as I didn't find a satisfactory way to do it !)
I would be happy to add some if anyone finds it would be beneficial.

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.

Note: `ssdd`[2], the test that highlighted the original bug, forks pairs
of tracer/tracee children. The tracer has a timeout waiting
for confirmation that its tracee process has been single stepped.
Given that the single step is now mostly preemptible, the step can be
delayed : possibly much more than what was usual previously.
Under heavy system load and by requesting the test to fork much more
children than its default, the test will start to fail unpredictably.
On a 4-core AMD Seattle board, I was able to see some inconsistent
failures with 32 (default 10) forks, becoming much more consistent
when the system load was above 16 (one or two failures per run).
Raising the timeout made the failures completely disappear, even with
a system load above 40, confirming that it is indeed a timeout issue.
Given that running `ssdd` with its default values does not seem to fail,
even with a system load of 21, so I do not feel like something
needs to be done to address this.


Based on v6.16-rc1.
Thanks,
Ada

v3 :
- Refactor `aarch32_break_handler()` to be consistent with other
  `el0_undef()` tentative handlers and the refactored
  `reinstall_suspended_bps()` later in the series.
- Drop the intermediate handlers for hardware breakpoint and watchpoint
  exceptions, as they both only return 0 and never end up calling
  `arm64_notify_die()`. Make the real handlers return void, rename them
  `do_{break,watch}point()` so that they can be called directly from
  `entry-common.c`. This is similar to what was done for the single-step
  handler. 
- Add static inline stand-ins for disabled stepping handlers. (Will)
- Don't duplicate `debug_exception_enter()` and `debug_exception_exit()`
  in patch 06 (Will)
  - Move them to `kernel/entry-common.c` and make them externally visible
    while `do_debug_exception()` in `mm/fault.c` needs them
  - Make them static again when cleaning up in patch 11.
- Refactor `reinstall_suspended_bps()` to make the logic cleaner and easier
  to understand (Will, Mark).
- Be more explicit in the commit messages about the goal and safety of
  moving `arm64_apply_bp_hardening()` to `kernel/entry-common.c` and
  calling it before `enter_from_user_mode` in patches 07 and 09 (Will)
- Enable preemption and unmask interrupts early for the brk64 handler,
  fixing a PREEMPT_RT sleeping while non-preemptible issue (Luis)
- Mention in patch 13 commit message that `bug_brk_handler()` is
  not called as a fall-through anymore in early boot, but that it doesn't
  change the actual behaviour (Will)
- Scope FAR_EL1 comment (Will)
- Fix typos (Will)
- Rebase on v6.16-rc1
  - Update the UBSAN BRK ESR check to `esr_is_ubsan_brk(esr)`.
v2 : https://lore.kernel.org/linux-arm-kernel/20250512174326.133905-1-ada.coupriediaz@arm.com
- 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/
[1]: https://lore.kernel.org/linux-arm-kernel/e86c5c3a-6666-46a7-b7ec-e803212a81a1@arm.com/
[2]: https://man.archlinux.org/man/ssdd.8.en

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 (13):
  arm64: debug: clean up single_step_handler logic
  arm64: refactor aarch32_break_handler()
  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 exception entry
  arm64: debug: refactor reinstall_suspended_bps()
  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       |  34 +--
 arch/arm64/include/asm/exception.h            |   6 +-
 arch/arm64/include/asm/kgdb.h                 |  12 ++
 arch/arm64/include/asm/kprobes.h              |   8 +
 arch/arm64/include/asm/system_misc.h          |   4 -
 arch/arm64/include/asm/traps.h                |   6 +
 arch/arm64/include/asm/uprobes.h              |  11 +
 arch/arm64/kernel/debug-monitors.c            | 204 +++++++-----------
 arch/arm64/kernel/entry-common.c              | 146 ++++++++++++-
 arch/arm64/kernel/hw_breakpoint.c             |  60 +++---
 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                     |  79 +------
 arch/arm64/mm/fault.c                         |  75 -------
 16 files changed, 308 insertions(+), 427 deletions(-)


base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.43.0

[PATCH v3 01/13] 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

base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.43.0

[PATCH v3 02/13] arm64: refactor aarch32_break_handler()

[Ada Couprie Diaz]

`aarch32_break_handler()` is called in `do_el0_undef()` when we
are trying to handle an exception whose Exception Syndrome is unknown.
It checks if the instruction hit might be a 32-bit arm break (be it
A32 or T2), and sends a SIGTRAP to userspace if it is so that it can
be handled.

However, this is badly represented in the naming of the function, and
is not consistent with the other functions called with the same logic
in `do_el0_undef()`.

Rename it `try_handle_aarch32_break()` and change the return value to
a boolean to align with the logic of the other tentative handlers in
`do_el0_undef()`, the previous error code being ignored anyway.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/debug-monitors.h |  2 +-
 arch/arm64/kernel/debug-monitors.c      | 10 +++++-----
 arch/arm64/kernel/traps.c               |  2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 8f6ba31b8658..e1caf6a8380c 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -116,7 +116,7 @@ static inline int reinstall_suspended_bps(struct pt_regs *regs)
 }
 #endif
 
-int aarch32_break_handler(struct pt_regs *regs);
+bool try_handle_aarch32_break(struct pt_regs *regs);
 
 void debug_traps_init(void);
 
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 676fa0231935..9e69f2e915e8 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -334,7 +334,7 @@ static int brk_handler(unsigned long unused, unsigned long esr,
 }
 NOKPROBE_SYMBOL(brk_handler);
 
-int aarch32_break_handler(struct pt_regs *regs)
+bool try_handle_aarch32_break(struct pt_regs *regs)
 {
 	u32 arm_instr;
 	u16 thumb_instr;
@@ -342,7 +342,7 @@ int aarch32_break_handler(struct pt_regs *regs)
 	void __user *pc = (void __user *)instruction_pointer(regs);
 
 	if (!compat_user_mode(regs))
-		return -EFAULT;
+		return false;
 
 	if (compat_thumb_mode(regs)) {
 		/* get 16-bit Thumb instruction */
@@ -366,12 +366,12 @@ int aarch32_break_handler(struct pt_regs *regs)
 	}
 
 	if (!bp)
-		return -EFAULT;
+		return false;
 
 	send_user_sigtrap(TRAP_BRKPT);
-	return 0;
+	return true;
 }
-NOKPROBE_SYMBOL(aarch32_break_handler);
+NOKPROBE_SYMBOL(try_handle_aarch32_break);
 
 void __init debug_traps_init(void)
 {
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 9bfa5c944379..685c11b2c553 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -454,7 +454,7 @@ void do_el0_undef(struct pt_regs *regs, unsigned long esr)
 	u32 insn;
 
 	/* check for AArch32 breakpoint instructions */
-	if (!aarch32_break_handler(regs))
+	if (try_handle_aarch32_break(regs))
 		return;
 
 	if (user_insn_read(regs, &insn))
-- 
2.43.0

[PATCH v3 03/13] 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              |  8 +++
 arch/arm64/include/asm/traps.h                |  6 ++
 arch/arm64/include/asm/uprobes.h              |  2 +
 arch/arm64/kernel/debug-monitors.c            | 54 +++++++++++++----
 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, 83 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..f2782560647b 100644
--- a/arch/arm64/include/asm/kprobes.h
+++ b/arch/arm64/include/asm/kprobes.h
@@ -41,4 +41,12 @@ 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);
+
 #endif /* _ARM_KPROBES_H */
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 9e69f2e915e8..15d7158a7548 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,47 @@ 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)) {
+		if (IS_ENABLED(CONFIG_UPROBES) &&
+			esr_brk_comment(esr) == UPROBES_BRK_IMM)
+			return uprobe_brk_handler(regs, esr);
+		return DBG_HOOK_ERROR;
 	}
 
+	if (esr_brk_comment(esr) == BUG_BRK_IMM)
+		return bug_brk_handler(regs, esr);
+
+	if (IS_ENABLED(CONFIG_CFI_CLANG) && esr_is_cfi_brk(esr))
+		return cfi_brk_handler(regs, esr);
+
+	if (esr_brk_comment(esr) == FAULT_BRK_IMM)
+		return reserved_fault_brk_handler(regs, esr);
+
+	if (IS_ENABLED(CONFIG_KASAN_SW_TAGS) &&
+		(esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
+		return kasan_brk_handler(regs, esr);
+
+	if (IS_ENABLED(CONFIG_UBSAN_TRAP) && esr_is_ubsan_brk(esr))
+		return ubsan_brk_handler(regs, esr);
+
+	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);
+
 	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 685c11b2c553..654c8ea46ec6 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(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_is_ubsan_brk(esr))
-		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 v3 04/13] 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.
Add a stand-in that returns DBG_HOOK_ERROR when the configuration
options are not 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      |  9 +++++++++
 arch/arm64/include/asm/uprobes.h   |  9 +++++++++
 arch/arm64/kernel/debug-monitors.c | 25 ++++++-------------------
 arch/arm64/kernel/kgdb.c           | 17 +++--------------
 arch/arm64/kernel/probes/uprobes.c |  9 +--------
 5 files changed, 28 insertions(+), 41 deletions(-)

diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
index 82a76b2102fb..fa0c1edb8a65 100644
--- a/arch/arm64/include/asm/kgdb.h
+++ b/arch/arm64/include/asm/kgdb.h
@@ -26,6 +26,15 @@ 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);
+#ifdef CONFIG_KGDB
+int kgdb_singlestep_handler(struct pt_regs *regs, unsigned long esr);
+#else
+static inline int kgdb_singlestep_handler(struct pt_regs *regs,
+	unsigned long esr)
+{
+	return DBG_HOOK_ERROR;
+}
+#endif
 
 #endif /* !__ASSEMBLY__ */
 
diff --git a/arch/arm64/include/asm/uprobes.h b/arch/arm64/include/asm/uprobes.h
index 3659a79a9f32..5858e436a5a7 100644
--- a/arch/arm64/include/asm/uprobes.h
+++ b/arch/arm64/include/asm/uprobes.h
@@ -29,5 +29,14 @@ struct arch_uprobe {
 };
 
 int uprobe_brk_handler(struct pt_regs *regs, unsigned long esr);
+#ifdef CONFIG_UPROBES
+int uprobe_singlestep_handler(struct pt_regs *regs, unsigned long esr);
+#else
+static inline int uprobe_singlestep_handler(struct pt_regs *regs,
+	unsigned long esr)
+{
+	return DBG_HOOK_ERROR;
+}
+#endif
 
 #endif
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 15d7158a7548..b156bef7f61e 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -200,30 +200,17 @@ 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.
  */
 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;
+	if (user_mode(regs))
+		return uprobe_singlestep_handler(regs, esr);
 
-	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;
-	}
-
-	return retval;
+	return kgdb_singlestep_handler(regs, esr);
 }
 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 v3 05/13] 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 e1caf6a8380c..caee1d923f9c 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 b156bef7f61e..02ba2c5e40ec 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.
@@ -264,29 +224,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 v3 06/13] arm64: entry: Add entry and exit functions for debug exceptions

[Ada Couprie Diaz]

Move the `debug_exception_enter()` 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.

Make them externally visible in include/asm/exception.h until
the caller in mm/fault.c is cleaned up.

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

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index d48fc16584cd..e54b5466fd2c 100644
--- a/arch/arm64/include/asm/exception.h
+++ b/arch/arm64/include/asm/exception.h
@@ -80,4 +80,8 @@ void do_serror(struct pt_regs *regs, unsigned long esr);
 void do_signal(struct pt_regs *regs);
 
 void __noreturn panic_bad_stack(struct pt_regs *regs, unsigned long esr, unsigned long far);
+
+void debug_exception_enter(struct pt_regs *regs);
+void debug_exception_exit(struct pt_regs *regs);
+
 #endif	/* __ASM_EXCEPTION_H */
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 7c1970b341b8..3bdfa5abaf7a 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -441,6 +441,28 @@ static __always_inline void fpsimd_syscall_exit(void)
 	__this_cpu_write(fpsimd_last_state.to_save, FP_STATE_CURRENT);
 }
 
+/*
+ * 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.
+ */
+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);
+
+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)
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index ec0a337891dd..d451d7d834f1 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -966,28 +966,6 @@ void __init hook_debug_fault_code(int nr,
 	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)
 {
-- 
2.43.0

[PATCH v3 07/13] arm64: debug: split hardware breakpoint 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 breakpoints exceptions are generated by the hardware after user
configuration. As such, they can be exploited when training branch
predictors outside of the userspace VA range: they still need to call
`arm64_apply_bp_hardening()` if needed to mitigate against this attack.

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.

As the hardware breakpoint handler only returns 0 and never triggers
the call to `arm64_notify_die()`, we can call it directly from
`entry-common.c`.
Split the hardware breakpoint exception entry, adjust
the function signature, and handling of the Cortex-A76 erratum to fit
the behaviour of the exception.

Move the call to `arm64_apply_bp_hardening()` to `entry-common.c` so that
we can do it as early as possible, and only for the exceptions coming
from EL0, where it is needed.
This is safe to do as it is `noinstr`, as are all the functions it
may call. `el0_ia()` and `el0_pc()` already call it this way.

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  | 16 ++++++----------
 3 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index e54b5466fd2c..926bad7b6704 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 3bdfa5abaf7a..be2add6b4ae3 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -526,6 +526,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);
@@ -575,6 +584,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:
@@ -769,6 +780,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 */
@@ -848,6 +872,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:
@@ -968,6 +994,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..d7eede5d869c 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)
+void do_breakpoint(unsigned long esr, struct pt_regs *regs)
 {
 	int i, step = 0, *kernel_step;
 	u32 ctrl_reg;
@@ -662,7 +662,7 @@ static int breakpoint_handler(unsigned long unused, unsigned long esr,
 	}
 
 	if (!step)
-		return 0;
+		return;
 
 	if (user_mode(regs)) {
 		debug_info->bps_disabled = 1;
@@ -670,7 +670,7 @@ static int breakpoint_handler(unsigned long unused, unsigned long esr,
 
 		/* If we're already stepping a watchpoint, just return. */
 		if (debug_info->wps_disabled)
-			return 0;
+			return;
 
 		if (test_thread_flag(TIF_SINGLESTEP))
 			debug_info->suspended_step = 1;
@@ -681,7 +681,7 @@ static int breakpoint_handler(unsigned long unused, unsigned long esr,
 		kernel_step = this_cpu_ptr(&stepping_kernel_bp);
 
 		if (*kernel_step != ARM_KERNEL_STEP_NONE)
-			return 0;
+			return;
 
 		if (kernel_active_single_step()) {
 			*kernel_step = ARM_KERNEL_STEP_SUSPEND;
@@ -690,10 +690,8 @@ static int breakpoint_handler(unsigned long unused, unsigned long esr,
 			kernel_enable_single_step(regs);
 		}
 	}
-
-	return 0;
 }
-NOKPROBE_SYMBOL(breakpoint_handler);
+NOKPROBE_SYMBOL(do_breakpoint);
 
 /*
  * Arm64 hardware does not always report a watchpoint hit address that matches
@@ -988,8 +986,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 v3 08/13] arm64: debug: refactor reinstall_suspended_bps()

[Ada Couprie Diaz]

`reinstall_suspended_bps()` plays a key part in the stepping process
when we have hardware breakpoints and watchpoints enabled.
It checks if we need to step one, will re-enable it if it has
been handled and will return whether or not we need to proceed with
a single-step.

However, the current naming and return values make it harder to understand
the logic and goal of the function.

Rename it `try_step_suspended_breakpoints()` and change the return value
to a boolean, aligning it with similar functions used in
`do_el0_undef()` like `try_emulate_mrs()`, and making its behaviour
more obvious.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/debug-monitors.h |  6 +++---
 arch/arm64/kernel/debug-monitors.c      |  2 +-
 arch/arm64/kernel/hw_breakpoint.c       | 25 ++++++++++++-------------
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index caee1d923f9c..586290e95d87 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -84,11 +84,11 @@ void kernel_rewind_single_step(struct pt_regs *regs);
 void kernel_fastforward_single_step(struct pt_regs *regs);
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
-int reinstall_suspended_bps(struct pt_regs *regs);
+bool try_step_suspended_breakpoints(struct pt_regs *regs);
 #else
-static inline int reinstall_suspended_bps(struct pt_regs *regs)
+static inline bool try_step_suspended_breakpoints(struct pt_regs *regs)
 {
-	return -ENODEV;
+	return false;
 }
 #endif
 
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 02ba2c5e40ec..74ffdfeff76f 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -195,7 +195,7 @@ static int single_step_handler(unsigned long unused, unsigned long esr,
 	 * If we are stepping a pending breakpoint, call the hw_breakpoint
 	 * handler first.
 	 */
-	if (!reinstall_suspended_bps(regs))
+	if (try_step_suspended_breakpoints(regs))
 		return 0;
 
 	if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index d7eede5d869c..309ae24d4548 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -847,36 +847,35 @@ NOKPROBE_SYMBOL(watchpoint_handler);
 /*
  * Handle single-step exception.
  */
-int reinstall_suspended_bps(struct pt_regs *regs)
+bool try_step_suspended_breakpoints(struct pt_regs *regs)
 {
 	struct debug_info *debug_info = &current->thread.debug;
-	int handled_exception = 0, *kernel_step;
-
-	kernel_step = this_cpu_ptr(&stepping_kernel_bp);
+	int *kernel_step = this_cpu_ptr(&stepping_kernel_bp);
+	bool handled_exception = false;
 
 	/*
 	 * Called from single-step exception handler.
-	 * Return 0 if execution can resume, 1 if a SIGTRAP should be
-	 * reported.
+	 * Return true if we stepped a breakpoint and can resume execution,
+	 * false if we need to handle a single-step.
 	 */
 	if (user_mode(regs)) {
 		if (debug_info->bps_disabled) {
 			debug_info->bps_disabled = 0;
 			toggle_bp_registers(AARCH64_DBG_REG_BCR, DBG_ACTIVE_EL0, 1);
-			handled_exception = 1;
+			handled_exception = true;
 		}
 
 		if (debug_info->wps_disabled) {
 			debug_info->wps_disabled = 0;
 			toggle_bp_registers(AARCH64_DBG_REG_WCR, DBG_ACTIVE_EL0, 1);
-			handled_exception = 1;
+			handled_exception = true;
 		}
 
 		if (handled_exception) {
 			if (debug_info->suspended_step) {
 				debug_info->suspended_step = 0;
 				/* Allow exception handling to fall-through. */
-				handled_exception = 0;
+				handled_exception = false;
 			} else {
 				user_disable_single_step(current);
 			}
@@ -890,17 +889,17 @@ int reinstall_suspended_bps(struct pt_regs *regs)
 
 		if (*kernel_step != ARM_KERNEL_STEP_SUSPEND) {
 			kernel_disable_single_step();
-			handled_exception = 1;
+			handled_exception = true;
 		} else {
-			handled_exception = 0;
+			handled_exception = false;
 		}
 
 		*kernel_step = ARM_KERNEL_STEP_NONE;
 	}
 
-	return !handled_exception;
+	return handled_exception;
 }
-NOKPROBE_SYMBOL(reinstall_suspended_bps);
+NOKPROBE_SYMBOL(try_step_suspended_breakpoints);
 
 /*
  * Context-switcher for restoring suspended breakpoints.
-- 
2.43.0

[PATCH v3 09/13] 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.
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.

Move the call to `arm64_apply_bp_hardening()` to `entry-common.c` so that
we can do it as early as possible, and only for the exceptions coming
from EL0, where it is needed.
This is safe to do as it is `noinstr`, as are all the functions it
may call. `el0_ia()` and `el0_pc()` already call it this way.

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 `try_step_suspended_breakpoints()` 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  |  2 +-
 4 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index 926bad7b6704..d6648b68a4c3 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 74ffdfeff76f..3f5503d61aee 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>
@@ -188,18 +189,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 (try_step_suspended_breakpoints(regs))
-		return 0;
-
 	if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
-		return 0;
+		return;
 
 	if (user_mode(regs)) {
 		send_user_sigtrap(TRAP_TRACE);
@@ -219,10 +212,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)
 {
@@ -329,8 +320,6 @@ NOKPROBE_SYMBOL(try_handle_aarch32_break);
 
 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 be2add6b4ae3..5fb636efd554 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -535,6 +535,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 are stepping a suspended breakpoint there's nothing more to do:
+		 * the single-step is complete.
+		 */
+		if (!try_step_suspended_breakpoints(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);
@@ -587,6 +605,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);
@@ -793,6 +813,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 are stepping a suspended breakpoint there's nothing more to do:
+	 * the single-step is complete.
+	 */
+	if (!try_step_suspended_breakpoints(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 */
@@ -875,6 +914,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);
@@ -997,6 +1038,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 309ae24d4548..8a80e13347c8 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -854,7 +854,7 @@ bool try_step_suspended_breakpoints(struct pt_regs *regs)
 	bool handled_exception = false;
 
 	/*
-	 * Called from single-step exception handler.
+	 * Called from single-step exception entry.
 	 * Return true if we stepped a breakpoint and can resume execution,
 	 * false if we need to handle a single-step.
 	 */
-- 
2.43.0

[PATCH v3 10/13] 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.

As the hardware watchpoint handler only returns 0 and never triggers
the call to `arm64_notify_die()`, we can call it directly from
`entry-common.c`.
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  | 17 +++++-----------
 3 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index d6648b68a4c3..4c9779bdd05b 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 5fb636efd554..f302a235d8db 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -553,10 +553,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)
 {
+	/* Watchpoints are the only debug exception to 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);
@@ -608,6 +618,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;
@@ -832,6 +844,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)
+{
+	/* Watchpoints are the only debug exception to 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 */
@@ -917,6 +942,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;
@@ -1041,6 +1068,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 8a80e13347c8..ab76b36dce82 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -750,8 +750,7 @@ static int watchpoint_report(struct perf_event *wp, unsigned long addr,
 	return step;
 }
 
-static int watchpoint_handler(unsigned long addr, unsigned long esr,
-			      struct pt_regs *regs)
+void do_watchpoint(unsigned long addr, unsigned long esr, struct pt_regs *regs)
 {
 	int i, step = 0, *kernel_step, access, closest_match = 0;
 	u64 min_dist = -1, dist;
@@ -806,7 +805,7 @@ static int watchpoint_handler(unsigned long addr, unsigned long esr,
 	rcu_read_unlock();
 
 	if (!step)
-		return 0;
+		return;
 
 	/*
 	 * We always disable EL0 watchpoints because the kernel can
@@ -819,7 +818,7 @@ static int watchpoint_handler(unsigned long addr, unsigned long esr,
 
 		/* If we're already stepping a breakpoint, just return. */
 		if (debug_info->bps_disabled)
-			return 0;
+			return;
 
 		if (test_thread_flag(TIF_SINGLESTEP))
 			debug_info->suspended_step = 1;
@@ -830,7 +829,7 @@ static int watchpoint_handler(unsigned long addr, unsigned long esr,
 		kernel_step = this_cpu_ptr(&stepping_kernel_bp);
 
 		if (*kernel_step != ARM_KERNEL_STEP_NONE)
-			return 0;
+			return;
 
 		if (kernel_active_single_step()) {
 			*kernel_step = ARM_KERNEL_STEP_SUSPEND;
@@ -839,10 +838,8 @@ static int watchpoint_handler(unsigned long addr, unsigned long esr,
 			kernel_enable_single_step(regs);
 		}
 	}
-
-	return 0;
 }
-NOKPROBE_SYMBOL(watchpoint_handler);
+NOKPROBE_SYMBOL(do_watchpoint);
 
 /*
  * Handle single-step exception.
@@ -984,10 +981,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 v3 11/13] 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.

When taking a BRK64 exception from EL0, the exception handling is safely
preemptible : the only possible handler is `uprobe_brk_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.
Thus we can safely unmask interrupts and enable preemption before
handling the break itself, fixing a PREEMPT_RT issue where the handler
could call a sleeping function with preemption disabled.

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   | 19 ++++++++++++++-----
 3 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index 4c9779bdd05b..6500c378d2d3 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 3f5503d61aee..10f6a8a15954 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -262,8 +262,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;
@@ -279,6 +278,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);
+
 bool try_handle_aarch32_break(struct pt_regs *regs)
 {
 	u32 arm_instr;
@@ -319,10 +326,7 @@ bool try_handle_aarch32_break(struct pt_regs *regs)
 NOKPROBE_SYMBOL(try_handle_aarch32_break);
 
 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 f302a235d8db..c0dcca825383 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -565,11 +565,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);
 }
 
@@ -621,7 +622,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);
@@ -857,6 +858,14 @@ 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);
+	local_daif_restore(DAIF_PROCCTX);
+	do_brk64(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 */
@@ -945,7 +954,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 v3 12/13] 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 |  8 ++++++++
 arch/arm64/kernel/entry-common.c   | 21 +++++++++------------
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index 6500c378d2d3..1c41b6e3d3be 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 10f6a8a15954..5490a4f32e85 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -286,6 +286,14 @@ void do_brk64(unsigned long esr, struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(do_brk64);
 
+#ifdef CONFIG_COMPAT
+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 */
+
 bool try_handle_aarch32_break(struct pt_regs *regs)
 {
 	u32 arm_instr;
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index c0dcca825383..90ee140052c5 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -866,17 +866,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);
@@ -1037,6 +1026,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);
@@ -1080,7 +1077,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 v3 13/13] 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.

This removes the external caller for `debug_exception_enter()` and
`debug_exception_exit()`.
Make them static again and remove them from the header.

Remove `early_brk64()` as it has been made redundant by
(arm64: debug: split brk64 exception entry) and is not used anymore.
Note : in `early_brk64()` `bug_brk_handler()` is called unconditionally
as a fall-through, but now `call_break_hook()` only calls it if the
immediate matches.
This does not change the behaviour in early boot, as if
`bug_brk_handler()` was called on a non-BUG immediate it would return
DBG_HOOK_ERROR anyway, which `call_break_hook()` will do if no immediate
matches.

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

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 586290e95d87..4dfc37dd1d96 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -94,7 +94,5 @@ static inline bool try_step_suspended_breakpoints(struct pt_regs *regs)
 
 bool try_handle_aarch32_break(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 1c41b6e3d3be..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,
@@ -86,8 +84,4 @@ void do_serror(struct pt_regs *regs, unsigned long esr);
 void do_signal(struct pt_regs *regs);
 
 void __noreturn panic_bad_stack(struct pt_regs *regs, unsigned long esr, unsigned long far);
-
-void debug_exception_enter(struct pt_regs *regs);
-void debug_exception_exit(struct pt_regs *regs);
-
 #endif	/* __ASM_EXCEPTION_H */
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 5490a4f32e85..172be6b367f8 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -333,9 +333,6 @@ bool try_handle_aarch32_break(struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(try_handle_aarch32_break);
 
-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/entry-common.c b/arch/arm64/kernel/entry-common.c
index 90ee140052c5..879beba5c78b 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -448,7 +448,7 @@ static __always_inline void fpsimd_syscall_exit(void)
  * accidentally schedule in exception context and it will force a warning
  * if we somehow manage to schedule by accident.
  */
-void debug_exception_enter(struct pt_regs *regs)
+static void debug_exception_enter(struct pt_regs *regs)
 {
 	preempt_disable();
 
@@ -457,7 +457,7 @@ void debug_exception_enter(struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(debug_exception_enter);
 
-void debug_exception_exit(struct pt_regs *regs)
+static void debug_exception_exit(struct pt_regs *regs)
 {
 	preempt_enable_no_resched();
 }
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 654c8ea46ec6..d6c5268e6947 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_is_ubsan_brk(esr))
-		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 d451d7d834f1..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,53 +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;
-}
-
-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 v3 01/13] arm64: debug: clean up single_step_handler logic

[Anshuman Khandual]

On 09/06/25 11:04 PM, 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 {
>  		pr_warn("Unexpected kernel single-step exception at EL1\n");
>  		/*
>  		 * Re-enable stepping since we know that we will be
> 
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494

Existing semantics here is preserved.

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH v3 02/13] arm64: refactor aarch32_break_handler()

[Anshuman Khandual]

On 09/06/25 11:04 PM, Ada Couprie Diaz wrote:
> `aarch32_break_handler()` is called in `do_el0_undef()` when we
> are trying to handle an exception whose Exception Syndrome is unknown.
> It checks if the instruction hit might be a 32-bit arm break (be it
> A32 or T2), and sends a SIGTRAP to userspace if it is so that it can
> be handled.
> 
> However, this is badly represented in the naming of the function, and
> is not consistent with the other functions called with the same logic
> in `do_el0_undef()`.

Agreed.

> 
> Rename it `try_handle_aarch32_break()` and change the return value to
> a boolean to align with the logic of the other tentative handlers in
> `do_el0_undef()`, the previous error code being ignored anyway.

Makes sense.

> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>

LGTM.

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

> ---
>  arch/arm64/include/asm/debug-monitors.h |  2 +-
>  arch/arm64/kernel/debug-monitors.c      | 10 +++++-----
>  arch/arm64/kernel/traps.c               |  2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 8f6ba31b8658..e1caf6a8380c 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -116,7 +116,7 @@ static inline int reinstall_suspended_bps(struct pt_regs *regs)
>  }
>  #endif
>  
> -int aarch32_break_handler(struct pt_regs *regs);
> +bool try_handle_aarch32_break(struct pt_regs *regs);
>  
>  void debug_traps_init(void);
>  
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 676fa0231935..9e69f2e915e8 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -334,7 +334,7 @@ static int brk_handler(unsigned long unused, unsigned long esr,
>  }
>  NOKPROBE_SYMBOL(brk_handler);
>  
> -int aarch32_break_handler(struct pt_regs *regs)
> +bool try_handle_aarch32_break(struct pt_regs *regs)
>  {
>  	u32 arm_instr;
>  	u16 thumb_instr;
> @@ -342,7 +342,7 @@ int aarch32_break_handler(struct pt_regs *regs)
>  	void __user *pc = (void __user *)instruction_pointer(regs);
>  
>  	if (!compat_user_mode(regs))
> -		return -EFAULT;
> +		return false;
>  
>  	if (compat_thumb_mode(regs)) {
>  		/* get 16-bit Thumb instruction */
> @@ -366,12 +366,12 @@ int aarch32_break_handler(struct pt_regs *regs)
>  	}
>  
>  	if (!bp)
> -		return -EFAULT;
> +		return false;
>  
>  	send_user_sigtrap(TRAP_BRKPT);
> -	return 0;
> +	return true;
>  }
> -NOKPROBE_SYMBOL(aarch32_break_handler);
> +NOKPROBE_SYMBOL(try_handle_aarch32_break);
>  
>  void __init debug_traps_init(void)
>  {
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 9bfa5c944379..685c11b2c553 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -454,7 +454,7 @@ void do_el0_undef(struct pt_regs *regs, unsigned long esr)
>  	u32 insn;
>  
>  	/* check for AArch32 breakpoint instructions */
> -	if (!aarch32_break_handler(regs))
> +	if (try_handle_aarch32_break(regs))
>  		return;
>  
>  	if (user_insn_read(regs, &insn))

Re: [PATCH v3 01/13] arm64: debug: clean up single_step_handler logic

[Mark Rutland]

On Mon, Jun 09, 2025 at 06:34:01PM +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>

Nice cleanup!

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  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
> 
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> -- 
> 2.43.0
> 

Re: [PATCH v3 02/13] arm64: refactor aarch32_break_handler()

[Mark Rutland]

On Mon, Jun 09, 2025 at 06:34:02PM +0100, Ada Couprie Diaz wrote:
> `aarch32_break_handler()` is called in `do_el0_undef()` when we
> are trying to handle an exception whose Exception Syndrome is unknown.
> It checks if the instruction hit might be a 32-bit arm break (be it
> A32 or T2), and sends a SIGTRAP to userspace if it is so that it can
> be handled.
> 
> However, this is badly represented in the naming of the function, and
> is not consistent with the other functions called with the same logic
> in `do_el0_undef()`.
> 
> Rename it `try_handle_aarch32_break()` and change the return value to
> a boolean to align with the logic of the other tentative handlers in
> `do_el0_undef()`, the previous error code being ignored anyway.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/include/asm/debug-monitors.h |  2 +-
>  arch/arm64/kernel/debug-monitors.c      | 10 +++++-----
>  arch/arm64/kernel/traps.c               |  2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 8f6ba31b8658..e1caf6a8380c 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -116,7 +116,7 @@ static inline int reinstall_suspended_bps(struct pt_regs *regs)
>  }
>  #endif
>  
> -int aarch32_break_handler(struct pt_regs *regs);
> +bool try_handle_aarch32_break(struct pt_regs *regs);
>  
>  void debug_traps_init(void);
>  
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 676fa0231935..9e69f2e915e8 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -334,7 +334,7 @@ static int brk_handler(unsigned long unused, unsigned long esr,
>  }
>  NOKPROBE_SYMBOL(brk_handler);
[>  
> -int aarch32_break_handler(struct pt_regs *regs)
> +bool try_handle_aarch32_break(struct pt_regs *regs)
>  {
>  	u32 arm_instr;
>  	u16 thumb_instr;
> @@ -342,7 +342,7 @@ int aarch32_break_handler(struct pt_regs *regs)
>  	void __user *pc = (void __user *)instruction_pointer(regs);
>  
>  	if (!compat_user_mode(regs))
> -		return -EFAULT;
> +		return false;
>  
>  	if (compat_thumb_mode(regs)) {
>  		/* get 16-bit Thumb instruction */
> @@ -366,12 +366,12 @@ int aarch32_break_handler(struct pt_regs *regs)
>  	}
>  
>  	if (!bp)
> -		return -EFAULT;
> +		return false;
>  
>  	send_user_sigtrap(TRAP_BRKPT);
> -	return 0;
> +	return true;
>  }
> -NOKPROBE_SYMBOL(aarch32_break_handler);
> +NOKPROBE_SYMBOL(try_handle_aarch32_break);
>  
>  void __init debug_traps_init(void)
>  {
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 9bfa5c944379..685c11b2c553 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -454,7 +454,7 @@ void do_el0_undef(struct pt_regs *regs, unsigned long esr)
>  	u32 insn;
>  
>  	/* check for AArch32 breakpoint instructions */
> -	if (!aarch32_break_handler(regs))
> +	if (try_handle_aarch32_break(regs))
>  		return;
>  
>  	if (user_insn_read(regs, &insn))
> -- 
> 2.43.0
> 

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

[Luis Claudio R. Goncalves]

On Mon, Jun 09, 2025 at 06:34:00PM +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 : related refactor of `aarch32_break_handler()`
> 3-5 : software breakpoints and single step handlers registration removal
> 6: preparatory function move that is made internal in patch 13
> 8: preparatory refactor of `reinstall_suspended_bps()`
> 7, 9-13 : debug exception splitting and handler registration removal
> 
> * Patch 3 copies and extends the `early_brk64` break handling for the
> normal path, byassing the dynamic registration.
> * Patch 4 does something similar for the single stepping handlers.
> * Patches 7, and 9 through 13 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.
> * Patch 8 refactors `reinstall_suspended_bps()` in preparation for the
> single-step exception entry split, as it is moved out of the handler.
> This could be squashed in patch 9 (the single-step exception split), but
> I opted to separate it for clarity (and because the commit message
> for patch 9 is already 45 lines long !).
> * Patches 5 and 13 are clean-ups removing the code that has been replaced
> and made redundant by the preceding patches.
> 
> PREEMPT_RT
> ===
> 
> Of note, this allows us to make the single exception handling mostly
> preemptible coming from EL0 in patch 9, 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 as he did a lot of testing on v2 and originally
> reported the issue.
> 
> Cc: "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>
> 
> We can do the same for the software break exception coming from EL0,
> which works in a very similar way.
> 
> However, the hardware breakpoint and watchpoint exceptions also have
> a sleeping while non-preemptible issue with PREEMPT_RT, but this is
> much trickier to fix, so this won't be fixed in this series.
> A bit more details in [1].
> 
> I hesitated to add comments within the BRK64 and single-step handlers,
> on the EL0 paths, to highlight that they would be preemptible and to be
> wary if modifying or adding to them, but I opted not to in the end.
> (Partly as I didn't find a satisfactory way to do it !)
> I would be happy to add some if anyone finds it would be beneficial.

Tested-by: Luis Claudio R. Goncalves <lgoncalv@redhat.com>

Works as advertised, fixes the reported RT problem, fails on the specific
case Ada described above. :)

Best regards,
Luis

Re: [PATCH v3 03/13] arm64: debug: call software break handlers statically

[Anshuman Khandual]

A small nit - s/break handlers/break point handlers/

On 09/06/25 11:04 PM, 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.

Unless absolutely necessary - could we please move these renames into a
separate patch in itself instead ? That will reduce the churn and help
the reviewers see the functional changes more clearly.

> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/kgdb.h                 |  3 +
>  arch/arm64/include/asm/kprobes.h              |  8 +++
>  arch/arm64/include/asm/traps.h                |  6 ++
>  arch/arm64/include/asm/uprobes.h              |  2 +
>  arch/arm64/kernel/debug-monitors.c            | 54 +++++++++++++----
>  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, 83 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..f2782560647b 100644
> --- a/arch/arm64/include/asm/kprobes.h
> +++ b/arch/arm64/include/asm/kprobes.h
> @@ -41,4 +41,12 @@ 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);
> +
>  #endif /* _ARM_KPROBES_H */
> 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 9e69f2e915e8..15d7158a7548 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,47 @@ 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)) {
> +		if (IS_ENABLED(CONFIG_UPROBES) &&
> +			esr_brk_comment(esr) == UPROBES_BRK_IMM)
> +			return uprobe_brk_handler(regs, esr);
> +		return DBG_HOOK_ERROR;
>  	}
>  
> +	if (esr_brk_comment(esr) == BUG_BRK_IMM)
> +		return bug_brk_handler(regs, esr);
> +
> +	if (IS_ENABLED(CONFIG_CFI_CLANG) && esr_is_cfi_brk(esr))
> +		return cfi_brk_handler(regs, esr);
> +
> +	if (esr_brk_comment(esr) == FAULT_BRK_IMM)
> +		return reserved_fault_brk_handler(regs, esr);
> +
> +	if (IS_ENABLED(CONFIG_KASAN_SW_TAGS) &&
> +		(esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
> +		return kasan_brk_handler(regs, esr);
> +
> +	if (IS_ENABLED(CONFIG_UBSAN_TRAP) && esr_is_ubsan_brk(esr))
> +		return ubsan_brk_handler(regs, esr);
> +
> +	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);
> +
>  	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 685c11b2c553..654c8ea46ec6 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(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_is_ubsan_brk(esr))
> -		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();
>  }

debug_traps_init() can be renamed as trap_init() and just drop this
redundant indirection. All applicable comments can also be changed
as required there after.

Re: [PATCH v3 04/13] arm64: debug: call step handlers statically

[Anshuman Khandual]

On 09/06/25 11:04 PM, 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.
> Add a stand-in that returns DBG_HOOK_ERROR when the configuration
> options are not enabled.
> 
> Unify the naming of the handler to XXX_singlestep_handler(), making it
> clear they are related.

Makes sense.

> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/kgdb.h      |  9 +++++++++
>  arch/arm64/include/asm/uprobes.h   |  9 +++++++++
>  arch/arm64/kernel/debug-monitors.c | 25 ++++++-------------------
>  arch/arm64/kernel/kgdb.c           | 17 +++--------------
>  arch/arm64/kernel/probes/uprobes.c |  9 +--------
>  5 files changed, 28 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
> index 82a76b2102fb..fa0c1edb8a65 100644
> --- a/arch/arm64/include/asm/kgdb.h
> +++ b/arch/arm64/include/asm/kgdb.h
> @@ -26,6 +26,15 @@ 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);
> +#ifdef CONFIG_KGDB
> +int kgdb_singlestep_handler(struct pt_regs *regs, unsigned long esr);
> +#else
> +static inline int kgdb_singlestep_handler(struct pt_regs *regs,
> +	unsigned long esr)
> +{
> +	return DBG_HOOK_ERROR;
> +}
> +#endif
>  
>  #endif /* !__ASSEMBLY__ */
>  
> diff --git a/arch/arm64/include/asm/uprobes.h b/arch/arm64/include/asm/uprobes.h
> index 3659a79a9f32..5858e436a5a7 100644
> --- a/arch/arm64/include/asm/uprobes.h
> +++ b/arch/arm64/include/asm/uprobes.h
> @@ -29,5 +29,14 @@ struct arch_uprobe {
>  };
>  
>  int uprobe_brk_handler(struct pt_regs *regs, unsigned long esr);
> +#ifdef CONFIG_UPROBES
> +int uprobe_singlestep_handler(struct pt_regs *regs, unsigned long esr);
> +#else
> +static inline int uprobe_singlestep_handler(struct pt_regs *regs,
> +	unsigned long esr)
> +{
> +	return DBG_HOOK_ERROR;
> +}
> +#endif
>  
>  #endif
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 15d7158a7548..b156bef7f61e 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -200,30 +200,17 @@ 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.
>   */
>  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;
> +	if (user_mode(regs))
> +		return uprobe_singlestep_handler(regs, esr);
>  
> -	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;
> -	}
> -
> -	return retval;
> +	return kgdb_singlestep_handler(regs, esr);
>  }
>  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)

This rename makes sense but as mentioned later kgdb_single_step_handler()
might save some changes in uprobes callback function.

>  {
>  	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);
>  }

kgdb_arch_init()/_exit() now deals only with kgdb_notifier registration
and un-registration only.
 >  
> 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)

A small nit - if the kgdb handler be changed as kgdb_single_step_handler()
the above rename can be skipped.
 >  {
>  	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;
>  }
>  

The arch hook arch_init_uprobes() is redundant now and can be dropped.

static int __init arch_init_uprobes(void)
{
        return 0;
}
device_initcall(arch_init_uprobes);

git grep arch_init_uprobes
arch/arm64/kernel/probes/uprobes.c:static int __init arch_init_uprobes(void)
arch/arm64/kernel/probes/uprobes.c:device_initcall(arch_init_uprobes);

Otherwise LGTM.

Re: [PATCH v3 05/13] arm64: debug: remove break/step handler registration infrastructure

[Anshuman Khandual]

On 09/06/25 11:04 PM, 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(-)
> 
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index e1caf6a8380c..caee1d923f9c 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 b156bef7f61e..02ba2c5e40ec 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.
> @@ -264,29 +224,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)) {

LGTM, no residues remaining of these dropped infrastructure.

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH v3 06/13] arm64: entry: Add entry and exit functions for debug exceptions

[Anshuman Khandual]

On 09/06/25 11:04 PM, Ada Couprie Diaz wrote:
> Move the `debug_exception_enter()` 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.
> 
> Make them externally visible in include/asm/exception.h until
> the caller in mm/fault.c is cleaned up.
Makes sense.

> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/exception.h |  4 ++++
>  arch/arm64/kernel/entry-common.c   | 22 ++++++++++++++++++++++
>  arch/arm64/mm/fault.c              | 22 ----------------------
>  3 files changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
> index d48fc16584cd..e54b5466fd2c 100644
> --- a/arch/arm64/include/asm/exception.h
> +++ b/arch/arm64/include/asm/exception.h
> @@ -80,4 +80,8 @@ void do_serror(struct pt_regs *regs, unsigned long esr);
>  void do_signal(struct pt_regs *regs);
>  
>  void __noreturn panic_bad_stack(struct pt_regs *regs, unsigned long esr, unsigned long far);
> +
> +void debug_exception_enter(struct pt_regs *regs);
> +void debug_exception_exit(struct pt_regs *regs);
> +
>  #endif	/* __ASM_EXCEPTION_H */
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 7c1970b341b8..3bdfa5abaf7a 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -441,6 +441,28 @@ static __always_inline void fpsimd_syscall_exit(void)
>  	__this_cpu_write(fpsimd_last_state.to_save, FP_STATE_CURRENT);
>  }
>  
> +/*
> + * 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.
> + */
> +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);
> +
> +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)
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index ec0a337891dd..d451d7d834f1 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -966,28 +966,6 @@ void __init hook_debug_fault_code(int nr,
>  	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)
>  {Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH v3 08/13] arm64: debug: refactor reinstall_suspended_bps()

[Anshuman Khandual]

On 09/06/25 11:04 PM, Ada Couprie Diaz wrote:
> `reinstall_suspended_bps()` plays a key part in the stepping process
> when we have hardware breakpoints and watchpoints enabled.
> It checks if we need to step one, will re-enable it if it has
> been handled and will return whether or not we need to proceed with
> a single-step.
> 
> However, the current naming and return values make it harder to understand
> the logic and goal of the function.
> 
> Rename it `try_step_suspended_breakpoints()` and change the return value
> to a boolean, aligning it with similar functions used in
> `do_el0_undef()` like `try_emulate_mrs()`, and making its behaviour
> more obvious.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/debug-monitors.h |  6 +++---
>  arch/arm64/kernel/debug-monitors.c      |  2 +-
>  arch/arm64/kernel/hw_breakpoint.c       | 25 ++++++++++++-------------
>  3 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index caee1d923f9c..586290e95d87 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -84,11 +84,11 @@ void kernel_rewind_single_step(struct pt_regs *regs);
>  void kernel_fastforward_single_step(struct pt_regs *regs);
>  
>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
> -int reinstall_suspended_bps(struct pt_regs *regs);
> +bool try_step_suspended_breakpoints(struct pt_regs *regs);
>  #else
> -static inline int reinstall_suspended_bps(struct pt_regs *regs)
> +static inline bool try_step_suspended_breakpoints(struct pt_regs *regs)
>  {
> -	return -ENODEV;
> +	return false;
>  }
>  #endif
>  
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 02ba2c5e40ec..74ffdfeff76f 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -195,7 +195,7 @@ static int single_step_handler(unsigned long unused, unsigned long esr,
>  	 * If we are stepping a pending breakpoint, call the hw_breakpoint
>  	 * handler first.
>  	 */
> -	if (!reinstall_suspended_bps(regs))
> +	if (try_step_suspended_breakpoints(regs))
>  		return 0;
>  
>  	if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index d7eede5d869c..309ae24d4548 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -847,36 +847,35 @@ NOKPROBE_SYMBOL(watchpoint_handler);
>  /*
>   * Handle single-step exception.
>   */
> -int reinstall_suspended_bps(struct pt_regs *regs)
> +bool try_step_suspended_breakpoints(struct pt_regs *regs)
>  {
>  	struct debug_info *debug_info = &current->thread.debug;
> -	int handled_exception = 0, *kernel_step;
> -
> -	kernel_step = this_cpu_ptr(&stepping_kernel_bp);
> +	int *kernel_step = this_cpu_ptr(&stepping_kernel_bp);
> +	bool handled_exception = false;
>  
>  	/*
>  	 * Called from single-step exception handler.
> -	 * Return 0 if execution can resume, 1 if a SIGTRAP should be
> -	 * reported.
> +	 * Return true if we stepped a breakpoint and can resume execution,
> +	 * false if we need to handle a single-step.
>  	 */
>  	if (user_mode(regs)) {
>  		if (debug_info->bps_disabled) {
>  			debug_info->bps_disabled = 0;
>  			toggle_bp_registers(AARCH64_DBG_REG_BCR, DBG_ACTIVE_EL0, 1);
> -			handled_exception = 1;
> +			handled_exception = true;
>  		}
>  
>  		if (debug_info->wps_disabled) {
>  			debug_info->wps_disabled = 0;
>  			toggle_bp_registers(AARCH64_DBG_REG_WCR, DBG_ACTIVE_EL0, 1);
> -			handled_exception = 1;
> +			handled_exception = true;
>  		}
>  
>  		if (handled_exception) {
>  			if (debug_info->suspended_step) {
>  				debug_info->suspended_step = 0;
>  				/* Allow exception handling to fall-through. */
> -				handled_exception = 0;
> +				handled_exception = false;
>  			} else {
>  				user_disable_single_step(current);
>  			}
> @@ -890,17 +889,17 @@ int reinstall_suspended_bps(struct pt_regs *regs)
>  
>  		if (*kernel_step != ARM_KERNEL_STEP_SUSPEND) {
>  			kernel_disable_single_step();
> -			handled_exception = 1;
> +			handled_exception = true;
>  		} else {
> -			handled_exception = 0;
> +			handled_exception = false;
>  		}
>  
>  		*kernel_step = ARM_KERNEL_STEP_NONE;
>  	}
>  
> -	return !handled_exception;
> +	return handled_exception;
>  }
> -NOKPROBE_SYMBOL(reinstall_suspended_bps);
> +NOKPROBE_SYMBOL(try_step_suspended_breakpoints);
>  
>  /*
>   * Context-switcher for restoring suspended breakpoints.

This makes sense.

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH v3 03/13] arm64: debug: call software break handlers statically

[Ada Couprie Diaz]

On 13/06/2025 07:11, Anshuman Khandual wrote:

> A small nit - s/break handlers/break point handlers/
You are right, checking again I was using too small a character limit 
for the summary line (55), which is not relevant. I will write 
`breakpoint` in full, I wasn't happy about leaving it out !
> On 09/06/25 11:04 PM, Ada Couprie Diaz wrote:
>> [...]
>>
>> 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.
> Unless absolutely necessary - could we please move these renames into a
> separate patch in itself instead ? That will reduce the churn and help
> the reviewers see the functional changes more clearly.
Fair enough, I can move the renames to a later patch to avoid renaming 
in all the places
that get removed in this patch.

Would it make sense to combine it with the single step handler renames 
in this case,
or would it be better to have two independent commits ?
>> [...]
>>    
>>   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();
>>   }
> debug_traps_init() can be renamed as trap_init() and just drop this
> redundant indirection. All applicable comments can also be changed
> as required there after.
I understand what you mean, but I would be tempted to not change it,
with the following reasons :
  - `debug_traps_init()` gets removed entirely in the last commit,
  - having `trap_init()` in `traps.c` makes more sense than 
`debug-monitors.c` to me
  - `trap_init()` ends up empty at the end of the series, it could make 
sense to simply
    remove it entirely, given there is an empty weak definition in 
`init/main.c` already.

What do you think ?

Thanks,
Ada

Re: [PATCH v3 04/13] arm64: debug: call step handlers statically

[Ada Couprie Diaz]

On 13/06/2025 08:47, Anshuman Khandual wrote:

> On 09/06/25 11:04 PM, Ada Couprie Diaz wrote:
>> [...]
>>
>> 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)
> This rename makes sense but as mentioned later kgdb_single_step_handler()
> might save some changes in uprobes callback function.

That's fair. I think I would prefer the `_single_step_` version now as 
well, so I'll go for it.

As per the other patch, would it make sense to split the rename here as 
well ? Would it be OK if it were in the same commit as the breakpoint 
exception handlers ?

>>   {
>>   	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);
>>   }
> kgdb_arch_init()/_exit() now deals only with kgdb_notifier registration
> and un-registration only.
That is correct, however is there something you want me to do/change 
regarding this ? It feels like those would still be the best places for 
the `kgdb_notifier` (un)registration.
>> 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)
> A small nit - if the kgdb handler be changed as kgdb_single_step_handler()
> the above rename can be skipped.
ACK above.
>>   {
>>
>>   	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;
>>   }
>>   
> The arch hook arch_init_uprobes() is redundant now and can be dropped.
>
> static int __init arch_init_uprobes(void)
> {
>          return 0;
> }
> device_initcall(arch_init_uprobes);
>
> git grep arch_init_uprobes
> arch/arm64/kernel/probes/uprobes.c:static int __init arch_init_uprobes(void)
> arch/arm64/kernel/probes/uprobes.c:device_initcall(arch_init_uprobes);
Absolutely right, I did miss that. I will clean up in v4.
> Otherwise LGTM.
Thanks for the comments,
Ada

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

[Ada Couprie Diaz]

On 12/06/2025 19:18, Luis Claudio R. Goncalves wrote:

> On Mon, Jun 09, 2025 at 06:34:00PM +0100, Ada Couprie Diaz wrote:
>> [...]
>>
>> PREEMPT_RT
>> ===
>>
>> Of note, this allows us to make the single exception handling mostly
>> preemptible coming from EL0 in patch 9, 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 as he did a lot of testing on v2 and originally
>> reported the issue.
>>
>> Cc: "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>
>>
>> We can do the same for the software break exception coming from EL0,
>> which works in a very similar way.
>>
>> However, the hardware breakpoint and watchpoint exceptions also have
>> a sleeping while non-preemptible issue with PREEMPT_RT, but this is
>> much trickier to fix, so this won't be fixed in this series.
>> A bit more details in [1].
>>
>> I hesitated to add comments within the BRK64 and single-step handlers,
>> on the EL0 paths, to highlight that they would be preemptible and to be
>> wary if modifying or adding to them, but I opted not to in the end.
>> (Partly as I didn't find a satisfactory way to do it !)
>> I would be happy to add some if anyone finds it would be beneficial.
> Tested-by: Luis Claudio R. Goncalves <lgoncalv@redhat.com>
>
> Works as advertised, fixes the reported RT problem, fails on the specific
> case Ada described above. :)
>
> Best regards,
> Luis
>
Thanks again Luis for the thorough testing and reports !

Best regards,
Ada

Re: [PATCH v3 09/13] arm64: debug: split single stepping exception entry

[Anshuman Khandual]

On 09/06/25 11:04 PM, 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.

I guess makes sense to mention this in the commit message as this is the
common motivation for these dynamic to static exception handler changes.

> 
> 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.
> 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.
> 
> Move the call to `arm64_apply_bp_hardening()` to `entry-common.c` so that
> we can do it as early as possible, and only for the exceptions coming
> from EL0, where it is needed.
> This is safe to do as it is `noinstr`, as are all the functions it
> may call. `el0_ia()` and `el0_pc()` already call it this way.

> 
> 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 `try_step_suspended_breakpoints()` 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  |  2 +-
>  4 files changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
> index 926bad7b6704..d6648b68a4c3 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 74ffdfeff76f..3f5503d61aee 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>
> @@ -188,18 +189,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)
do_softstep() has been chosen here just to match the corresponding ESR
macros ESR_ELx_EC_SOFTSTP_[LOW|CUR] - although it does make sense and
also consistent.

>  {
> -	/*
> -	 * If we are stepping a pending breakpoint, call the hw_breakpoint
> -	 * handler first.
> -	 */
> -	if (try_step_suspended_breakpoints(regs))
> -		return 0;

This check has been moved individual exception handling functions.

> -
>  	if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
> -		return 0;
> +		return;
>  
>  	if (user_mode(regs)) {
>  		send_user_sigtrap(TRAP_TRACE);
> @@ -219,10 +212,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)
>  {
> @@ -329,8 +320,6 @@ NOKPROBE_SYMBOL(try_handle_aarch32_break);
>  
>  void __init debug_traps_init(void)
>  {
> -	hook_debug_fault_code(DBG_ESR_EVT_HWSS, single_step_handler, SIGTRAP,
> -			      TRAP_TRACE, "single-step handler");
Right, this dynamic hook registration is no longer required.

>  	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 be2add6b4ae3..5fb636efd554 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -535,6 +535,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)) {

Although mentioned in the commit message, this constraint is not
there in the present code though.

> +		debug_exception_enter(regs);
> +		/*
> +		 * After handling a breakpoint, we suspend the breakpoint
> +		 * and use single-step to move to the next instruction.
> +		 * If we are stepping a suspended breakpoint there's nothing more to do:
> +		 * the single-step is complete.
> +		 */
> +		if (!try_step_suspended_breakpoints(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);
> @@ -587,6 +605,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;Changes to EL1 exception handling as required.

>  	case ESR_ELx_EC_WATCHPT_CUR:
>  	case ESR_ELx_EC_BRK64:
>  		el1_dbg(regs, esr);
> @@ -793,6 +813,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 are stepping a suspended breakpoint there's nothing more to do:
> +	 * the single-step is complete.
> +	 */
> +	if (!try_step_suspended_breakpoints(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 */
> @@ -875,6 +914,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);
> @@ -997,6 +1038,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;

Changes to EL0 exception handling as required.

>  	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 309ae24d4548..8a80e13347c8 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -854,7 +854,7 @@ bool try_step_suspended_breakpoints(struct pt_regs *regs)
>  	bool handled_exception = false;
>  
>  	/*
> -	 * Called from single-step exception handler.
> +	 * Called from single-step exception entry.Hmm, alright but does not make much difference through.

>  	 * Return true if we stepped a breakpoint and can resume execution,
>  	 * false if we need to handle a single-step.
>  	 */

Re: [PATCH v3 09/13] arm64: debug: split single stepping exception entry

[Ada Couprie Diaz]

On 18/06/2025 07:25, Anshuman Khandual wrote:

> On 09/06/25 11:04 PM, Ada Couprie Diaz wrote:
>> [...]
>>
>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>> index 74ffdfeff76f..3f5503d61aee 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>
>> @@ -188,18 +189,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)
> do_softstep() has been chosen here just to match the corresponding ESR
> macros ESR_ELx_EC_SOFTSTP_[LOW|CUR] - although it does make sense and
> also consistent.
Exactly, that was the thinking !
>>   {
>> -	/*
>> -	 * If we are stepping a pending breakpoint, call the hw_breakpoint
>> -	 * handler first.
>> -	 */
>> -	if (try_step_suspended_breakpoints(regs))
>> -		return 0;
> This check has been moved individual exception handling functions.
Correct, as per explanation in the commit message
>> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
>> index be2add6b4ae3..5fb636efd554 100644
>> --- a/arch/arm64/kernel/entry-common.c
>> +++ b/arch/arm64/kernel/entry-common.c
>> @@ -535,6 +535,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)) {
> Although mentioned in the commit message, this constraint is not
> there in the present code though.
I believe it is : in mainline it is done for all EL1 debug exceptions in 
`el1_dbg()`[0], unless I misunderstood your comment ?

Thanks for the review Anshuman !
Ada

[0]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/entry-common.c#n507

Re: [PATCH v3 03/13] arm64: debug: call software break handlers statically

[Mark Rutland]

On Mon, Jun 16, 2025 at 02:29:37PM +0100, Ada Couprie Diaz wrote:
> On 13/06/2025 07:11, Anshuman Khandual wrote:
> 
> > A small nit - s/break handlers/break point handlers/
> You are right, checking again I was using too small a character limit for
> the summary line (55), which is not relevant. I will write `breakpoint` in
> full, I wasn't happy about leaving it out !

FWIW, saying "software breakpoint" sounds good to me.

[...]

> > On 09/06/25 11:04 PM, Ada Couprie Diaz wrote:
> > > 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.
> > Unless absolutely necessary - could we please move these renames into a
> > separate patch in itself instead ? That will reduce the churn and help
> > the reviewers see the functional changes more clearly.
> Fair enough, I can move the renames to a later patch to avoid renaming in
> all the places
> that get removed in this patch.
> Would it make sense to combine it with the single step handler renames in
> this case, or would it be better to have two independent commits ?

TBH I think this is fine as-is and doesn't need to be split out. The
renames aid clarity, and the churn to early_brk64() is small.

[...]

> > >   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();
> > >   }
> > debug_traps_init() can be renamed as trap_init() and just drop this
> > redundant indirection. All applicable comments can also be changed
> > as required there after.
> I understand what you mean, but I would be tempted to not change it,
> with the following reasons :
>  - `debug_traps_init()` gets removed entirely in the last commit,
>  - having `trap_init()` in `traps.c` makes more sense than
> `debug-monitors.c` to me
>  - `trap_init()` ends up empty at the end of the series, it could make sense
> to simply
>    remove it entirely, given there is an empty weak definition in
> `init/main.c` already.
> 
> What do you think ?

I agree with your reasoning, please leave it as-is!

Mark.

Re: [PATCH v3 04/13] arm64: debug: call step handlers statically

[Mark Rutland]

On Mon, Jun 16, 2025 at 02:39:43PM +0100, Ada Couprie Diaz wrote:
> On 13/06/2025 08:47, Anshuman Khandual wrote:
> 
> > On 09/06/25 11:04 PM, Ada Couprie Diaz wrote:
> > > [...]
> > > 
> > > 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)
> > This rename makes sense but as mentioned later kgdb_single_step_handler()
> > might save some changes in uprobes callback function.
> 
> That's fair. I think I would prefer the `_single_step_` version now as well,
> so I'll go for it.

FWIW, that sounds good to me.

> As per the other patch, would it make sense to split the rename here as well
> ? Would it be OK if it were in the same commit as the breakpoint exception
> handlers ?

I think it makes sense for the rename to be in this patch, at the point
we expose the functions in header files. No need to split that out into
a separate patch.

Thanks,
Mark.

Re: [PATCH v3 09/13] arm64: debug: split single stepping exception entry

[Mark Rutland]

This looks good; one comment below.

On Mon, Jun 09, 2025 at 06:34:09PM +0100, Ada Couprie Diaz wrote:
> +void do_softstep(unsigned long esr, struct pt_regs *regs)
>  {
> -	/*
> -	 * If we are stepping a pending breakpoint, call the hw_breakpoint
> -	 * handler first.
> -	 */
> -	if (try_step_suspended_breakpoints(regs))
> -		return 0;
> -
>  	if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
> -		return 0;
> +		return;
>  
>  	if (user_mode(regs)) {
>  		send_user_sigtrap(TRAP_TRACE);
> @@ -219,10 +212,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);

With the EL0/EL1 entry paths split up, it would be nice to split this
into separate do_el{0,1}_softstep() handlers, like we do for
do_el{0,1}_undef(). With the relevant portions of call_step_hook()
folded in, that'd leave us with:

| void do_el0_softstep(struct pt_regs *regs, unsigned long esr)
| {
| 	if (uprobe_singlestep_handler(regs, esr) == DBG_HOOK_HANDLED)
| 		return;
| 	
| 	send_user_sigtrap(TRAP_TRACE);
| 	/*
| 	 * ptrace will disable single step unless explicitly asked to
| 	 * re-enable it. For other clients, it makes sense to leave it
| 	 * enabled (i.e. rewind the controls to the active-not-pending
| 	 * state).
| 	 */
| 	 user_rewind_single_step(current);
| }
| 
| void do_el1_softstep(struct pt_regs *regs, unsigned long esr)
| {
| 	if (kgdb_singlestep_handler(regs, esr) == DBG_HOOK_HANDLED)
| 		return;
| 	
| 	pr_warn("Unexpected kernel single-step exception at EL1\n");
| 	/*
| 	 * Re-enable stepping since we know that we will be
| 	 * returning to regs.
| 	 */
| 	set_regs_spsr_ss(regs);
| }
| NOKPROBE_SYMBOL(do_el1_softstep):

Doing that would remove some indirection, and make this a bit easier to
follow. That would also permit the EL0 handler to be kprobed (which is
safe since there's no risk of recursion in the EL0 handler).

Mark.

Re: [PATCH v3 11/13] arm64: debug: split brk64 exception entry

[Mark Rutland]

This generally looks good; 

On Mon, Jun 09, 2025 at 06:34:11PM +0100, Ada Couprie Diaz wrote:
> +static int brk_handler(unsigned long esr, struct pt_regs *regs)
>  {
>  	if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
>  		return 0;
> @@ -279,6 +278,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);

Could we please split this into separate do_el{0,1}_brk64() helpers?

e.g. rename call_break_hook() to call_el1_break_hook(), remove
brk_handler(), and have:

| void do_el0_brk64(struct pt_regs *regs, unsigned long esr)
| {
|         if (IS_ENABLED(CONFIG_UPROBES) &&
|             esr_brk_comment(esr) == UPROBES_BRK_IMM &&
|             uprobe_brk_handler(regs, esr) == DBG_HOOK_HANDLED)
|                 return;
| 
|         send_user_sigtrap(TRAP_BRKPT);
| }
|
| void do_el1_brk64(unsigned long esr, struct pt_regs *regs)
| {
|         if (call_el1_break_hook(regs, esr) == DBG_HOOK_HANDLED)
|                 return;
| 
|         pr_warn("Unexpected kernel BRK exception at EL1\n");
|         arm64_notify_die("BRK handler", regs, SIGTRAP, TRAP_BRKPT, regs->pc,
|                          esr);
| }
| NOKPROBE_SYMBOL(do_brk64);

... and we could probably simplify the latter to:

| void do_el1_brk64(unsigned long esr, struct pt_regs *regs)
| {
|         if (call_el1_break_hook(regs, esr) == DBG_HOOK_HANDLED)
|                 return;
| 
|	  die("Oops - BRK", regs, esr);
| }
| NOKPROBE_SYMBOL(do_brk64);

... matching do_el1_undef() and do_el1_bti().

Mark.

Re: [PATCH v3 12/13] arm64: debug: split bkpt32 exception entry

[Mark Rutland]

On Mon, Jun 09, 2025 at 06:34:12PM +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>

> +#ifdef CONFIG_COMPAT
> +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 */

Trivial nit: we can drop NOKPROBE_SYMBOL() here, since do_bkpt32() can't
be recurse within kprobes.

Otherwise this all looks good to me!

Mark.

Re: [PATCH v3 09/13] arm64: debug: split single stepping exception entry

[Ada Couprie Diaz]

On 18/06/2025 17:02, Mark Rutland wrote:

> This looks good; one comment below.
>
> On Mon, Jun 09, 2025 at 06:34:09PM +0100, Ada Couprie Diaz wrote:
>> +void do_softstep(unsigned long esr, struct pt_regs *regs)
>>   {
>> -	/*
>> -	 * If we are stepping a pending breakpoint, call the hw_breakpoint
>> -	 * handler first.
>> -	 */
>> -	if (try_step_suspended_breakpoints(regs))
>> -		return 0;
>> -
>>   	if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
>> -		return 0;
>> +		return;
>>   
>>   	if (user_mode(regs)) {
>>   		send_user_sigtrap(TRAP_TRACE);
>> @@ -219,10 +212,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);
> With the EL0/EL1 entry paths split up, it would be nice to split this
> into separate do_el{0,1}_softstep() handlers, like we do for
> do_el{0,1}_undef(). With the relevant portions of call_step_hook()
> folded in, that'd leave us with:
>
> | void do_el0_softstep(struct pt_regs *regs, unsigned long esr)
> | {
> | 	if (uprobe_singlestep_handler(regs, esr) == DBG_HOOK_HANDLED)
> | 		return;
> | 	
> | 	send_user_sigtrap(TRAP_TRACE);
> | 	/*
> | 	 * ptrace will disable single step unless explicitly asked to
> | 	 * re-enable it. For other clients, it makes sense to leave it
> | 	 * enabled (i.e. rewind the controls to the active-not-pending
> | 	 * state).
> | 	 */
> | 	 user_rewind_single_step(current);
> | }
> |
> | void do_el1_softstep(struct pt_regs *regs, unsigned long esr)
> | {
> | 	if (kgdb_singlestep_handler(regs, esr) == DBG_HOOK_HANDLED)
> | 		return;
> | 	
> | 	pr_warn("Unexpected kernel single-step exception at EL1\n");
> | 	/*
> | 	 * Re-enable stepping since we know that we will be
> | 	 * returning to regs.
> | 	 */
> | 	set_regs_spsr_ss(regs);
> | }
> | NOKPROBE_SYMBOL(do_el1_softstep):
>
> Doing that would remove some indirection, and make this a bit easier to
> follow. That would also permit the EL0 handler to be kprobed (which is
> safe since there's no risk of recursion in the EL0 handler).
>
> Mark.
Agree, that's really nice and being able to probe a bit more is quite 
useful !

I split the EL0/EL1 handlers for the soft step and BRK64 in v4.

Thanks Mark,
Ada