Re: [PATCH v3 4/4] openrisc: Add Kretprobes

[Sahil <sahilcdq0@gmail.com>]

On 11/8/26 1:28 PM, Stafford Horne wrote:
> On Tue, Oct 27, 2026 at 01:42:40AM +0530, Sahil Siddiq wrote:
>> Add Kretprobes support for OpenRISC. Kretprobes allow a probe to be
>> fired when a function returns. They are built on top of the generic
>> rethook framework and also require the KProbe mechanism introduced
>> in the previous commit. This work is primarily based on similar work
>> done for RISC-V.
>>
>> Kretprobes work by inserting a KProbe at a function's entry point.
>> When the function begins to execute, the KProbe is fired and its
>> pre-handler replaces the function's return address with that of the
>> rethook trampoline. The trampoline in turn calls the kretprobe handler.
>> Control is returned to the original caller following this.
>>
>> Signed-off-by: Sahil Siddiq <sahilcdq0@gmail.com>
>> ---
>> Changes v2 -> v3:
>> - arch/openrisc/kernel/rethook_trampoline.S:
>>    (save_all_base_regs): Save SR and SP registers
>>    (restore_all_base_regs): Restore SR register
>>
>> Changes v1 -> v2: New commit
>>
>> [...]
>> +#include <linux/kprobes.h>
>> +#include <linux/rethook.h>
>> +#include "rethook.h"
>> +
>> +unsigned long arch_rethook_trampoline_callback(struct pt_regs *regs)
>> +{
>> +	return rethook_trampoline_handler(regs, regs->gpr2);
>> +}
>> +NOKPROBE_SYMBOL(arch_rethook_trampoline_callback)
>> +
>> +void arch_rethook_prepare(struct rethook_node *rhn, struct pt_regs *regs, bool mcount)
>> +{
>> +	/* Store original return address and frame pointer */
>> +	rhn->ret_addr = regs->gpr9;
>> +	rhn->frame = regs->gpr2;
>> +
>> +	/* Set return address to trampoline */
>> +	regs->gpr9 = (unsigned long)arch_rethook_trampoline;
> 
> Though this will work, in the kernel code we prefer to regs->gpr[n] syntax
> for accessing gprs.  So to be consistent could you use that?  i.e. regs->gpr[9];

Sure, I'll change this.

>> +}
>> +NOKPROBE_SYMBOL(arch_rethook_prepare)
>> diff --git a/arch/openrisc/kernel/rethook.h b/arch/openrisc/kernel/rethook.h
>> new file mode 100644
>> index 000000000000..332306a5c380
>> --- /dev/null
>> +++ b/arch/openrisc/kernel/rethook.h
>> @@ -0,0 +1,8 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +#ifndef __OPENRISC_RETHOOK_H
>> +#define __OPENRISC_RETHOOK_H
>> +
>> +unsigned long arch_rethook_trampoline_callback(struct pt_regs *regs);
>> +void arch_rethook_prepare(struct rethook_node *rhn, struct pt_regs *regs, bool mcount);
>> +
>> +#endif
>> diff --git a/arch/openrisc/kernel/rethook_trampoline.S b/arch/openrisc/kernel/rethook_trampoline.S
>> new file mode 100644
>> index 000000000000..5e8fd5a65222
>> --- /dev/null
>> +++ b/arch/openrisc/kernel/rethook_trampoline.S
>> [...]
>> +SYM_CODE_START(arch_rethook_trampoline)
>> +	/* Allocate enough space on stack for pt_regs */
>> +	l.addi	r1,r1,-PT_SIZE
>> +	save_all_base_regs
>> +
>> +	l.addi	r3,r1,0	/* pt_regs */
>> +	l.jal	arch_rethook_trampoline_callback
>> +	l.nop
> 
> Could you indent the delay slot? For example:
> 
> 	l.jal	...
> 	 l.nop
> 
>> +
>> +	/* use the result as the return-address */
>> +	l.addi	r9,r11,0
>> +
>> +	restore_all_base_regs
>> +	l.addi	r1,r1,PT_SIZE
>> +
>> +	l.jr	r9
>> +	l.nop
> 
> Indent here too.

Sure, I'll format these as well.

>> +SYM_CODE_END(arch_rethook_trampoline)
> 
> Thanks,
> 
> -Stafford

Thanks,
Sahil