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

[Stafford Horne <shorne@gmail.com>]

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
> 
>  arch/openrisc/Kconfig                     |   2 +
>  arch/openrisc/kernel/Makefile             |   1 +
>  arch/openrisc/kernel/rethook.c            |  32 +++++++
>  arch/openrisc/kernel/rethook.h            |   8 ++
>  arch/openrisc/kernel/rethook_trampoline.S | 111 ++++++++++++++++++++++
>  5 files changed, 154 insertions(+)
>  create mode 100644 arch/openrisc/kernel/rethook.c
>  create mode 100644 arch/openrisc/kernel/rethook.h
>  create mode 100644 arch/openrisc/kernel/rethook_trampoline.S
> 
> diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
> index d240533b424b..82365eb8f574 100644
> --- a/arch/openrisc/Kconfig
> +++ b/arch/openrisc/Kconfig
> @@ -28,6 +28,8 @@ config OPENRISC
>  	select HAVE_ARCH_JUMP_LABEL_RELATIVE
>  	select HAVE_PCI
>  	select HAVE_KPROBES
> +	select HAVE_KRETPROBES
> +	select HAVE_RETHOOK
>  	select HAVE_UID16
>  	select HAVE_PAGE_SIZE_8KB
>  	select HAVE_REGS_AND_STACK_ACCESS_API
> diff --git a/arch/openrisc/kernel/Makefile b/arch/openrisc/kernel/Makefile
> index 0c1cade09e0c..e36537cea319 100644
> --- a/arch/openrisc/kernel/Makefile
> +++ b/arch/openrisc/kernel/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
>  obj-$(CONFIG_MODULES)		+= module.o
>  obj-$(CONFIG_OF)		+= prom.o
>  obj-$(CONFIG_KPROBES)		+= kprobes.o
> +obj-$(CONFIG_RETHOOK)		+= rethook.o rethook_trampoline.o
>  obj-$(CONFIG_OR_KPROBES_KUNIT)	+= tests/
>  obj-y	+= patching.o
>  
> diff --git a/arch/openrisc/kernel/rethook.c b/arch/openrisc/kernel/rethook.c
> new file mode 100644
> index 000000000000..50ef6715d044
> --- /dev/null
> +++ b/arch/openrisc/kernel/rethook.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Generic return hook for OpenRISC. Depends on KProbes.
> + *
> + * Linux architectural port borrowing liberally from similar works of
> + * others.  All original copyrights apply as per the original source
> + * declaration.
> + *
> + * OpenRISC implementation:
> + * Copyright (C) 2026 Sahil Siddiq <sahilcdq0@gmail.com>
> + */
> +
> +#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];

> +}
> +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
> @@ -0,0 +1,111 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Author: Sahil Siddiq <sahilcdq0@gmail.com>
> + */
> +
> +#include <linux/linkage.h>
> +
> +#include <asm/asm.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/ptrace.h>
> +
> +	.text
> +
> +	.macro save_all_base_regs
> +	l.sw	PT_GPR2(r1),r2
> +	l.sw	PT_GPR3(r1),r3
> +	l.sw	PT_GPR4(r1),r4
> +	l.sw	PT_GPR5(r1),r5
> +	l.sw	PT_GPR6(r1),r6
> +	l.sw	PT_GPR7(r1),r7
> +	l.sw	PT_GPR8(r1),r8
> +	l.sw	PT_GPR9(r1),r9
> +	l.sw	PT_GPR10(r1),r10
> +	l.sw	PT_GPR11(r1),r11
> +	l.sw	PT_GPR12(r1),r12
> +	l.sw	PT_GPR13(r1),r13
> +	l.sw	PT_GPR14(r1),r14
> +	l.sw	PT_GPR15(r1),r15
> +	l.sw	PT_GPR16(r1),r16
> +	l.sw	PT_GPR17(r1),r17
> +	l.sw	PT_GPR18(r1),r18
> +	l.sw	PT_GPR19(r1),r19
> +	l.sw	PT_GPR20(r1),r20
> +	l.sw	PT_GPR21(r1),r21
> +	l.sw	PT_GPR22(r1),r22
> +	l.sw	PT_GPR23(r1),r23
> +	l.sw	PT_GPR24(r1),r24
> +	l.sw	PT_GPR25(r1),r25
> +	l.sw	PT_GPR26(r1),r26
> +	l.sw	PT_GPR27(r1),r27
> +	l.sw	PT_GPR28(r1),r28
> +	l.sw	PT_GPR29(r1),r29
> +	l.sw	PT_GPR30(r1),r30
> +	l.sw	PT_GPR31(r1),r31
> +	/* Save SR in pt_regs */
> +	l.mfspr	r13,r0,SPR_SR
> +	l.sw	PT_SR(r1),r13
> +	/* Save SP in pt_regs as it was prior to setting up this frame */
> +	l.addi	r13,r1,PT_SIZE
> +	l.sw	PT_SP(r1),r13
> +	.endm
> +
> +	.macro restore_all_base_regs
> +	/* Restore SR from pt_regs */
> +	l.lwz	r13,PT_SR(r1)
> +	l.mtspr	r0,r13,SPR_SR
> +	/*
> +	 * r1 doesn't need to be explicitly restored from pt_regs. When
> +	 * arch_rethook_trampoline_callback finishes, it restores SP to
> +	 * what it was before its frame was set up.
> +	 */
> +	l.lwz	r2,PT_GPR2(r1)
> +	l.lwz	r3,PT_GPR3(r1)
> +	l.lwz	r4,PT_GPR4(r1)
> +	l.lwz	r5,PT_GPR5(r1)
> +	l.lwz	r6,PT_GPR6(r1)
> +	l.lwz	r7,PT_GPR7(r1)
> +	l.lwz	r8,PT_GPR8(r1)
> +	/* Return address in r9 is stale, so don't restore it */
> +	l.lwz	r10,PT_GPR10(r1)
> +	l.lwz	r11,PT_GPR11(r1)
> +	l.lwz	r12,PT_GPR12(r1)
> +	l.lwz	r13,PT_GPR13(r1)
> +	l.lwz	r14,PT_GPR14(r1)
> +	l.lwz	r15,PT_GPR15(r1)
> +	l.lwz	r16,PT_GPR16(r1)
> +	l.lwz	r17,PT_GPR17(r1)
> +	l.lwz	r18,PT_GPR18(r1)
> +	l.lwz	r19,PT_GPR19(r1)
> +	l.lwz	r20,PT_GPR20(r1)
> +	l.lwz	r21,PT_GPR21(r1)
> +	l.lwz	r22,PT_GPR22(r1)
> +	l.lwz	r23,PT_GPR23(r1)
> +	l.lwz	r24,PT_GPR24(r1)
> +	l.lwz	r25,PT_GPR25(r1)
> +	l.lwz	r26,PT_GPR26(r1)
> +	l.lwz	r27,PT_GPR27(r1)
> +	l.lwz	r28,PT_GPR28(r1)
> +	l.lwz	r29,PT_GPR29(r1)
> +	l.lwz	r30,PT_GPR30(r1)
> +	l.lwz	r31,PT_GPR31(r1)
> +	.endm
> +
> +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.

> +SYM_CODE_END(arch_rethook_trampoline)

Thanks,

-Stafford