Re: [PATCH v3 2/4] openrisc: Add KProbes

[Stafford Horne <shorne@gmail.com>]

On Fri, Nov 13, 2026 at 10:43:55PM +0530, Sahil wrote:
> On 11/8/26 1:16 PM, Stafford Horne wrote:
> > On Tue, Oct 27, 2026 at 01:42:38AM +0530, Sahil Siddiq wrote:
> > > Add KProbes support for OpenRISC. This work is primarily based
> > > on similar work done for LoongArch, MIPS and RISC-V.

[snip]

> > > +++ b/arch/openrisc/include/asm/kprobes.h

[snip]

> > > +#define flush_insn_slot(p)		do { } while (0)
> > 
> > On openrisc I think we need to implement flush_insn_slot.  As our
> > copy's only affect data caches instruction caches may end up being
> > stale.  This is really hard to detect as context switches invalidate
> > a lot of the instruction cache lines, but not all.
> > 
> > I fixed this for text patching recently with commit aca063c90245
> > ("openrisc: Fix jump_label smp syncing")
> 
> Understood. In the same commit [1], you have also mentioned that
> invalidating all the cache lines is less expensive than keeping
> track of which line needs to be invalidated.
> 
> In case of KProbes, where a KProbe at a specific instruction is
> armed or disarmed:
> 
> void arch_arm_kprobe(struct kprobe *p) {
>     patch_insn_write(p->addr, KPROBE_BP_INSN);
>     flush_insn_slot(p);
> }
> 
> void arch_disarm_kprobe(struct kprobe *p) {
>     patch_insn_write(p->addr, p->opcode);
>     flush_insn_slot(p);
> }
> 
> Should only that page be invalidated? Or is it ok to invalidate all
> lines?

I think you only need to flush the part of the kprobe data structure that has
code that is updated, for example p->addr.  Since you have all of that
information tracked for you in the kprobe structures it should be pretty simple.
That was not the case for text patching.

Note, I had a look this is needed for init_aggr_kprobe() which copies code from
one prope to another.

Note 2, for arch_arm_kprobe/arch_disarm_kprobe we use patch_insn_write which
internally does a icache invalidation alredy.  So I think the call to
flush_insn_slot() in arch_disarm_kprobe()/arch_arm_kprobe() is not needed.

> > > +#define kretprobe_blacklist_size	0
> > > +
> > > +void arch_remove_kprobe(struct kprobe *p);
> > > +int kprobe_fault_handler(struct pt_regs *regs, int trapnr);
> > > +bool kprobe_breakpoint_handler(struct pt_regs *regs);
> > > +bool kprobe_singlestep_handler(struct pt_regs *regs);
> > > +#else /* !CONFIG_KPROBES */
> > > +static inline bool kprobe_breakpoint_handler(struct pt_regs *regs)
> > > +{
> > > +	return false;
> > > +}
> > > +
> > > +static inline bool kprobe_singlestep_handler(struct pt_regs *regs)
> > > +{
> > > +	return false;
> > > +}
> > > +#endif /* CONFIG_KPROBES */
> > > +#endif /* __ASM_OPENRISC_KPROBES_H */
> > > diff --git a/arch/openrisc/kernel/Makefile b/arch/openrisc/kernel/Makefile
> > > index 150779fbf010..2ac824867963 100644
> > > --- a/arch/openrisc/kernel/Makefile
> > > +++ b/arch/openrisc/kernel/Makefile
> > > @@ -14,6 +14,7 @@ obj-$(CONFIG_SMP)		+= smp.o sync-timer.o
> > >   obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
> > >   obj-$(CONFIG_MODULES)		+= module.o
> > >   obj-$(CONFIG_OF)		+= prom.o
> > > +obj-$(CONFIG_KPROBES)		+= kprobes.o
> > >   obj-y	+= patching.o
> > >   clean:
> > > diff --git a/arch/openrisc/kernel/entry.S b/arch/openrisc/kernel/entry.S
> > > index c7e90b09645e..cd28bf1f7a3b 100644
> > > --- a/arch/openrisc/kernel/entry.S
> > > +++ b/arch/openrisc/kernel/entry.S
> > > @@ -15,6 +15,7 @@
> > >   #include <linux/linkage.h>
> > >   #include <linux/pgtable.h>
> > > +#include <asm/asm.h>
> > >   #include <asm/processor.h>
> > >   #include <asm/unistd.h>
> > >   #include <asm/thread_info.h>
> > > @@ -640,6 +641,7 @@ ENTRY(_sys_call_handler)
> > >   	/* r30 is the only register we clobber in the fast path */
> > >   	/* r30 already saved */
> > >   /*	l.sw    PT_GPR30(r1),r30 */
> > > +_ASM_NOKPROBE(_sys_call_handler)
> > 
> > How is this working for these not function symbols?  _ASM_NOKPROBE registers a
> > sybmol (in this case just a macro address) into the _kprobe_blacklist.  I think
> > withing kprobe.c it uses these _kprobe_blacklist entries to detect ranges that
> > cannot be probed.  However, in this case I think you are not able to get range
> > information.
> 
> _ASM_NOKPROBE and NOKPROBE_SYMBOL (for functions in C) expand to the
> same thing functionally. The kernel treats these labels as symbols as
> well. I verified this using "nm".
> 
> When a KProbe is registered using an address instead of the symbol_name,
> "register_kprobe()" [2] at kernel/kprobes.c indirectly calls
> "within_kprobe_blacklist()" [3] which performs "kprobe_lookup_name()".
> It then checks if that name is in the blacklist. This blacklist
> includes the names of the asm functions as well.
> 
> I wrote this test module [4] to verify that an address in memcpy's range,
> for example, is also blacklisted.

Right, I think for C functions this is fine.  But for OpenRISC assembly I think
we do not provide enough info for this to work correctly.

For example above _sys_call_handler, is just a label.

In other architectures they do something like this:

    SYM_CODE_START(handle_syscall)
	    UNWIND_HINT_UNDEFINED
	    csrrd           t0, PERCPU_BASE_KS
...
	    bl              do_syscall

	    STACKLEAK_ERASE
	    RESTORE_ALL_AND_RET
    SYM_CODE_END(handle_syscall)
    _ASM_NOKPROBE(handle_syscall)

The SYM_CODE_START/SYM_CODE_END macros or similar are not used in OpenRISC, I
think we need something to define the beginning and end of our asm macros
in order for this to work.

-Stafford

[snip]
 
> Thanks,
> Sahil
> 
> [1] https://github.com/torvalds/linux/commit/1be031de802ba486a7605b52eda825f128b026e2
> [2] https://github.com/torvalds/linux/blob/master/kernel/kprobes.c#L2002
> [3] https://github.com/torvalds/linux/blob/master/kernel/kprobes.c#L1730
> [4] https://github.com/valdaarhun/or-dev/blob/main/home/kprobes_blacklist.c
>