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

[Ada Couprie Diaz <ada.coupriediaz@arm.com>]

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

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

Thanks for the suggestion !
Ada