Re: [RFC PATCH 5/9] arm64/kprobes: Invoke pre/post handlers inside instrumentation

[Hongyan Xia <hongyan.xia@transsion.com>]

On 7/31/2026 11:44 PM, Mark Rutland wrote:
> On Mon, Jul 27, 2026 at 12:25:41PM +0000, Hongyan Xia wrote:
>> From: Hongyan Xia <hongyan.xia@transsion.com>
>>
>> The kprobe pre_handler() and post_handler() are arbitrary instrumentable
>> code and can themselves trace, fault, or hit other kprobes. They are
>> the only parts of the kprobe debug exception flow that legitimately
>> need instrumentation.
> 
> That doesn't explain what this patch changes, or why.
> 
> I'm fine with marking all the kprobe functions noinstr, but we don't
> need the intrumentation_{begin,end}() gunk.

Some of the helpers are quite difficult to deal with. The get_kprobe() 
has RCU debugging inside, just like kprobe_inc_nmissed_count(). We could 
further narrow down the instrumentation window, but making it completely 
noinstr might not be feasible.

> In general, it's not sound for pre_handler and post_handler to invoke
> arbitrary code, given that they could lead to recursion. We just hope
> people don't do stupid things with them.

I think this is still a big step forward. Before, we spent several weeks 
tracking down the Kprobe bug which eventually resulted in 879a6754 and 
23f851ac. It was super time-consuming to debug because 1) well, it's 
Kprobe and 2) the entire path could be instrumented and hard to narrow 
down. Now that we know such things could only happen in a couple of tiny 
helpers and the pre- and post-handlers, the surface is massively reduced 
and much easier to reason about, even if it's not 100% noinstr.

But of course, this is RFC. Would greatly appreciate better ideas. I'm 
also not keen on leaving instrumentation_{begin/end}() around.

> 
> Mark.
> 
>> Signed-off-by: Hongyan Xia <hongyan.xia@transsion.com>
>> ---
>>   arch/arm64/include/asm/kprobes.h   |  9 +++------
>>   arch/arm64/kernel/probes/kprobes.c | 22 ++++++++++++++++++----
>>   2 files changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
>> index 35ce2c94040e..a694f7d34f45 100644
>> --- a/arch/arm64/include/asm/kprobes.h
>> +++ b/arch/arm64/include/asm/kprobes.h
>> @@ -48,11 +48,8 @@ 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);
>> +int noinstr kprobe_brk_handler(struct pt_regs *regs, unsigned long esr);
>> +int noinstr kprobe_ss_brk_handler(struct pt_regs *regs, unsigned long esr);
>> +int noinstr kretprobe_brk_handler(struct pt_regs *regs, unsigned long esr);
>>
>>   #endif /* _ARM_KPROBES_H */
>> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
>> index 1b12341b2af3..e9fa66fa4217 100644
>> --- a/arch/arm64/kernel/probes/kprobes.c
>> +++ b/arch/arm64/kernel/probes/kprobes.c
>> @@ -301,8 +301,11 @@ post_kprobe_handler(struct kprobe *cur, struct kprobe_ctlblk *kcb, struct pt_reg
>>        }
>>        /* call post handler */
>>        kcb->kprobe_status = KPROBE_HIT_SSDONE;
>> +
>> +     instrumentation_begin();
>>        if (cur->post_handler)
>>                cur->post_handler(cur, regs, 0);
>> +     instrumentation_end();
>>
>>        reset_current_kprobe();
>>   }
>> @@ -359,24 +362,28 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
>>        return 0;
>>   }
>>
>> -int __kprobes
>> +int noinstr
>>   kprobe_brk_handler(struct pt_regs *regs, unsigned long esr)
>>   {
>>        struct kprobe *p, *cur_kprobe;
>>        struct kprobe_ctlblk *kcb;
>>        unsigned long addr = instruction_pointer(regs);
>> +     bool handled;
>>
>>        kcb = get_kprobe_ctlblk();
>>        cur_kprobe = kprobe_running();
>>
>> +     instrumentation_begin();
>>        p = get_kprobe((kprobe_opcode_t *) addr);
>>        if (WARN_ON_ONCE(!p)) {
>> +             instrumentation_end();
>>                /*
>>                 * Something went wrong. This BRK used an immediate reserved
>>                 * for kprobes, but we couldn't find any corresponding probe.
>>                 */
>>                return DBG_HOOK_ERROR;
>>        }
>> +     instrumentation_end();
>>
>>        if (cur_kprobe) {
>>                /* Hit a kprobe inside another kprobe */
>> @@ -387,6 +394,10 @@ kprobe_brk_handler(struct pt_regs *regs, unsigned long esr)
>>                set_current_kprobe(p);
>>                kcb->kprobe_status = KPROBE_HIT_ACTIVE;
>>
>> +             instrumentation_begin();
>> +             handled = p->pre_handler && p->pre_handler(p, regs);
>> +             instrumentation_end();
>> +
>>                /*
>>                 * If we have no pre-handler or it returned 0, we
>>                 * continue with normal processing.  If we have a
>> @@ -394,7 +405,7 @@ kprobe_brk_handler(struct pt_regs *regs, unsigned long esr)
>>                 * modify the execution path and not need to single-step
>>                 * Let's just reset current kprobe and exit.
>>                 */
>> -             if (!p->pre_handler || !p->pre_handler(p, regs))
>> +             if (!handled)
>>                        setup_singlestep(p, regs, kcb, 0);
>>                else
>>                        reset_current_kprobe();
>> @@ -403,7 +414,7 @@ kprobe_brk_handler(struct pt_regs *regs, unsigned long esr)
>>        return DBG_HOOK_HANDLED;
>>   }
>>
>> -int __kprobes
>> +int noinstr
>>   kprobe_ss_brk_handler(struct pt_regs *regs, unsigned long esr)
>>   {
>>        struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
>> @@ -422,13 +433,16 @@ kprobe_ss_brk_handler(struct pt_regs *regs, unsigned long esr)
>>        return DBG_HOOK_ERROR;
>>   }
>>
>> -int __kprobes
>> +int noinstr
>>   kretprobe_brk_handler(struct pt_regs *regs, unsigned long esr)
>>   {
>>        if (regs->pc != (unsigned long)__kretprobe_trampoline)
>>                return DBG_HOOK_ERROR;
>>
>> +     instrumentation_begin();
>>        regs->pc = kretprobe_trampoline_handler(regs, (void *)regs->regs[29]);
>> +     instrumentation_end();
>> +
>>        return DBG_HOOK_HANDLED;
>>   }
>>
>> --
>> 2.47.3
>>