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

[Sahil <sahilcdq0@gmail.com>]

Hi,

I have two queries in this commit.

On 10/27/26 1:42 AM, Sahil Siddiq wrote:
> Add KProbes support for OpenRISC. This work is primarily based
> on similar work done for LoongArch, MIPS and RISC-V.
> 
> KProbes make it possible to trap at almost any address in the
> kernel to collect performance/debugging info.
>
> [...]
> diff --git a/arch/openrisc/include/asm/kprobes.h b/arch/openrisc/include/asm/kprobes.h
> new file mode 100644
> index 000000000000..2b5847002141
> --- /dev/null
> +++ b/arch/openrisc/include/asm/kprobes.h
> @@ -0,0 +1,77 @@
> [...]
> +
> +struct prev_kprobe {
> +	struct kprobe *kp;
> +	unsigned int status;
> +	unsigned long restore_pc;
> +};
> +
> +/* per-cpu kprobe control block */
> +struct kprobe_ctlblk {
> +	unsigned int kprobe_status;
> +	unsigned long irq_flags;
> +	/* address of next instruction in case of SSOL */
> +	unsigned long restore_pc;
> +	struct prev_kprobe prev_kprobe;
> +};
> +

There was a review comment from Sashiko about saving "irq_flags" in
"struct prev_kprobe". I am not sure if that's actually required.

My understanding is that a KProbe will be reentered if a probe is hit
before single-stepping (e.g. a probe in the pre-handler) or after it
(e.g. a probe in the post-handler). If a KProbe is reentered before
single-stepping, nothing will be present in kprobe_ctlblk.irq_flags that
requires restoration. So, it doesn't need to be saved in prev_kprobe.
And if the KProbe is reentered after single-stepping, by that point the
value in kprobe_ctlblk.irq_flags should have already been restored (i.e.
IRQs should be reenabled by then) and so it shouldn't need saving in
prev_kprobe.

Could you please let me know if I am missing something, or if my
reasoning is sound?

> [...]
> diff --git a/arch/openrisc/kernel/kprobes.c b/arch/openrisc/kernel/kprobes.c
> new file mode 100644
> index 000000000000..0b8762e340d5
> --- /dev/null
> +++ b/arch/openrisc/kernel/kprobes.c
> @@ -0,0 +1,426 @@
> [...]
> +
> +int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
> +{
> +	struct kprobe *cur = kprobe_running();
> +	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> +
> +	switch (kcb->kprobe_status) {
> +	case KPROBE_HIT_SS:
> +	case KPROBE_REENTER:
> +		/*
> +		 * The instruction being single-stepped caused a page fault.
> +		 * Reset the current KProbe and set PC to the probe's address.
> +		 * Then allow the page fault handler to continue as usual.
> +		 */
> +		instruction_pointer_set(regs, (unsigned long)cur->addr);
> +

There was another review comment from Sashiko. It stated that in the case
of a page fault which is triggered in the delay slot of a jump-and-link
instruction, resetting PC to the branch instruction can corrupt the state.
It theorized that this is due to the modification made to regs->gpr[9]
(the link register) in "simulate_branch()".

I haven't really understood why the state would get corrupted in this case.
Rewinding PC to the branch instruction implies it'll get executed again.
If it gets re-executed, the value in regs->gpr[9] should also get re-evaluated
and it should hold the correct value after re-execution.

> +		if (kcb->kprobe_status == KPROBE_REENTER)
> +			restore_previous_kprobe(kcb);
> +		else
> +			reset_current_kprobe();
> +
> +		kprobe_local_irq_restore(kcb, regs);
> +		preempt_enable_no_resched();
> +		break;
> +	}
> +
> +	return 0;
> +}
> [...]

Thanks,
Sahil

[1] https://sashiko.dev/#/patchset/20260407185650.79816-1-sahilcdq0%40gmail.com?part=2 (1st comment)
[2] https://sashiko.dev/#/patchset/20260930203129.51048-1-sahilcdq0%40gmail.com?part=2 (6th comment)