Re: [RFC PATCH 3/9] arm64/debug-monitors: Make do_el1_brk64()/do_el1_softstep() noinstr

[Hongyan Xia]

On 7/31/2026 11:25 PM, Mark Rutland wrote:
> [Some people who received this message don't often get email from mark.rutland@xxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Mon, Jul 27, 2026 at 12:25:38PM +0000, Hongyan Xia wrote:
>> From: Hongyan Xia <hongyan.xia@xxxxxxxxxxxxx>
>>
>> Convert do_el1_brk64(), do_el1_softstep() and call_el1_break_hook() to
>> noinstr. The kprobe and kretprobe BRK handlers (converted to noinstr in
>> the following patches) are dispatched directly. Every other BRK handler
>> are ordinary instrumentable code and now run bounded by
>> instrumentation_begin()/end().
> 
> Why is it necessary to change do_el1_softstep()?
> 
> Neither kprobes nor kretprobes uses software stepping since commit:
> 
>    7ee31a3aa8f4 ("arm64: kprobes: Use BRK instead of single-step when executing instructions out-of-line")
> 
> ... so either that shouldn't be necessary, or there's a problem that
> needs to be described in this commit message.
> 
>> With this, everything on the el1 debug exception path from the vectors
>> down to the kprobe handlers is noinstr, and instrumentation only runs
>> inside explicit instrumentation windows.
>>
>> Signed-off-by: Hongyan Xia <hongyan.xia@xxxxxxxxxxxxx>
>> ---
>>   arch/arm64/kernel/debug-monitors.c | 74 +++++++++++++++++-------------
>>   1 file changed, 41 insertions(+), 33 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>> index 29307642f4c9..a970ab6327cd 100644
>> --- a/arch/arm64/kernel/debug-monitors.c
>> +++ b/arch/arm64/kernel/debug-monitors.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/debugfs.h>
>>   #include <linux/hardirq.h>
>>   #include <linux/init.h>
>> +#include <linux/instrumentation.h>
>>   #include <linux/ptrace.h>
>>   #include <linux/kprobes.h>
>>   #include <linux/stat.h>
>> @@ -193,59 +194,65 @@ void do_el0_softstep(unsigned long esr, struct pt_regs *regs)
>>        user_rewind_single_step(current);
>>   }
>>
>> -void do_el1_softstep(unsigned long esr, struct pt_regs *regs)
>> +void noinstr do_el1_softstep(unsigned long esr, struct pt_regs *regs)
>>   {
>> -     if (kgdb_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
>> +     int handled;
>> +
>> +     instrumentation_begin();
>> +     handled = kgdb_single_step_handler(regs, esr);
>> +     instrumentation_end();
>> +
>> +     if (handled == DBG_HOOK_HANDLED)
>>                return;
>>
>> +     instrumentation_begin();
>>        pr_warn("Unexpected kernel single-step exception at EL1\n");
>> +     instrumentation_end();
>>        /*
>>         * Re-enable stepping since we know that we will be
>>         * returning to regs.
>>         */
>>        set_regs_spsr_ss(regs);
>>   }
>> -NOKPROBE_SYMBOL(do_el1_softstep);
> 
> As above, I don't think it's necessary to change do_el1_softstep(), but
> I might be missing something that you haven't described in the commit
> message.

We have a noinstr verifier locally. To make it happy I started out this 
series trying to make the whole debug_exception() noinstr, but I see it 
has caused enough confusion (like in 1/9) and I'll limit the scope to 
only Kprobe noinstr in the next revision.

> Is the existing NOKPROBE_SYMBOL() annotation actually necessary? It
> looks like that dates from before commit 7ee31a3aa8f4, and I suspect we
> can delete it even without making this noinstr.
> 
> I don't think you need to make structural changes here.  Given the first
> thing the function does is an unconditional call to an instrumented
> function, we're not gaining anything by litering this with
> instrumentation_{begin,end}().
> 
>> -static int call_el1_break_hook(struct pt_regs *regs, unsigned long esr)
>> +static int noinstr call_el1_break_hook(struct pt_regs *regs, unsigned long esr)
>>   {
>> -     if (esr_brk_comment(esr) == BUG_BRK_IMM)
>> -             return bug_brk_handler(regs, esr);
>> -
>> -     if (IS_ENABLED(CONFIG_CFI) && 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);
>> -     }
>> +     unsigned long comment = esr_brk_comment(esr);
>> +     int ret = DBG_HOOK_ERROR;
>>
>>        if (IS_ENABLED(CONFIG_KPROBES)) {
>> -             if (esr_brk_comment(esr) == KPROBES_BRK_IMM)
>> +             if (comment == KPROBES_BRK_IMM)
>>                        return kprobe_brk_handler(regs, esr);
>> -             if (esr_brk_comment(esr) == KPROBES_BRK_SS_IMM)
>> +             if (comment == KPROBES_BRK_SS_IMM)
>>                        return kprobe_ss_brk_handler(regs, esr);
>>        }
>>
>>        if (IS_ENABLED(CONFIG_KRETPROBES) &&
>> -             esr_brk_comment(esr) == KRETPROBES_BRK_IMM)
>> +         comment == KRETPROBES_BRK_IMM)
>>                return kretprobe_brk_handler(regs, esr);
>>
>> -     return DBG_HOOK_ERROR;
>> +     instrumentation_begin();
>> +     if (comment == BUG_BRK_IMM)
>> +             ret = bug_brk_handler(regs, esr);
>> +     else if (IS_ENABLED(CONFIG_CFI) && esr_is_cfi_brk(esr))
>> +             ret = cfi_brk_handler(regs, esr);
>> +     else if (comment == FAULT_BRK_IMM)
>> +             ret = reserved_fault_brk_handler(regs, esr);
>> +     else if (IS_ENABLED(CONFIG_KASAN_SW_TAGS) &&
>> +              (comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
>> +             ret = kasan_brk_handler(regs, esr);
>> +     else if (IS_ENABLED(CONFIG_UBSAN_TRAP) && esr_is_ubsan_brk(esr))
>> +             ret = ubsan_brk_handler(regs, esr);
>> +     else if (IS_ENABLED(CONFIG_KGDB)) {
>> +             if (comment == KGDB_DYN_DBG_BRK_IMM)
>> +                     ret = kgdb_brk_handler(regs, esr);
>> +             else if (comment == KGDB_COMPILED_DBG_BRK_IMM)
>> +                     ret = kgdb_compiled_brk_handler(regs, esr);
>> +     }
>> +     instrumentation_end();
>> +
>> +     return ret;
>>   }
>> -NOKPROBE_SYMBOL(call_el1_break_hook);
> 
> I don't think you need to make any structural changes to
> call_el1_break_hook(). Just mark it as noinstr, and remove the
> NOKPROBE_SYMBOL() annotation. The existing control flow will be safe.
> 
>>   /*
>>    * We have already unmasked interrupts and enabled preemption
>> @@ -261,14 +268,15 @@ void do_el0_brk64(unsigned long esr, struct pt_regs *regs)
>>        send_user_sigtrap(TRAP_BRKPT);
>>   }
>>
>> -void do_el1_brk64(unsigned long esr, struct pt_regs *regs)
>> +void noinstr do_el1_brk64(unsigned long esr, struct pt_regs *regs)
>>   {
>>        if (call_el1_break_hook(regs, esr) == DBG_HOOK_HANDLED)
>>                return;
>>
>> +     instrumentation_begin();
>>        die("Oops - BRK", regs, esr);
>> +     instrumentation_end();
>>   }
>> -NOKPROBE_SYMBOL(do_el1_brk64);
> 
> Likewise, just mark do_el1_brk64() as noinstr and remove the
> NOKPROBE_SYMBOL() annotation, without the instrumentation_{begin,end}()
> calls.

I'll just drop this patch and get Kprobe properly done first.

> 
> Mark.