Re: [linux-6.6.y bugreport] riscv: kprobe crash as some patchs lost

[Nam Cao <namcao@linutronix.de>]

On Fri, Apr 25, 2025 at 11:29:17PM +0800, Kai Zhang wrote:
> On 4/25/2025 8:59 PM, Nam Cao wrote:
> > On Fri, Apr 25, 2025 at 02:49:52PM +0200, Nam Cao wrote:
> > > On Fri, Apr 25, 2025 at 08:09:21PM +0800, Kai Zhang wrote:
> > > > On 4/25/2025 4:07 PM, Greg Kroah-Hartman wrote:
> > > > > On Fri, Apr 25, 2025 at 04:03:41PM +0800, Kai Zhang wrote:
> > > > > > On 4/22/2025 4:46 PM, Greg Kroah-Hartman wrote:
> > > > > > > On Tue, Apr 22, 2025 at 10:58:42AM +0800, Kai Zhang wrote:
> > > > > > > > In most recent linux-6.6.y tree,
> > > > > > > > `arch/riscv/kernel/probes/kprobes.c::arch_prepare_ss_slot` still has the
> > > > > > > > obsolete code:
> > > > > > > > 
> > > > > > > >        u32 insn = __BUG_INSN_32;
> > > > > > > >        unsigned long offset = GET_INSN_LENGTH(p->opcode);
> > > > > > > >        p->ainsn.api.restore = (unsigned long)p->addr + offset;
> > > > > > > >        patch_text_nosync(p->ainsn.api.insn, &p->opcode, 1);
> > > > > > > >        patch_text_nosync((void *)p->ainsn.api.insn + offset, &insn, 1);
> > > > > > > > 
> > > > > > > > The last two 1s are wrong size of written instructions , which would lead to
> > > > > > > > kernel crash, like `insmod kprobe_example.ko` gives:
> > > > > > > > 
> > > > > > > > [  509.812815][ T2734] kprobe_init: Planted kprobe at 00000000c5c46130
> > > > > > > > [  509.837606][    C5] handler_pre: <kernel_clone> p->addr =
> > > > > > > > 0x00000000c5c46130, pc = 0xffffffff80032ee2, status = 0x200000120
> > > > > > > > [  509.839315][    C5] Oops - illegal instruction [#1]
> > > > > > > > 
> > > > > > > > 
> > > > > > > > I've tried two patchs from torvalds tree and it didn't crash again:
> > > > > > > > 
> > > > > > > > 51781ce8f448 riscv: Pass patch_text() the length in bytes (rebased)
> > > > > > > > 13134cc94914 riscv: kprobes: Fix incorrect address calculation
> > > 
> > > Please don't revert this patch. It fixes another issue.
> > > 
> > > You are correct that the sizes of the instructions are wrong. It can still
> > > happen to work if only one instruction is patched.
> > > 
> > > This bug is not specific to v6.6. It is in mainline as well. Therefore fix
> > > patch should be sent to mainline, and then backport to v6.6.
> > 
> > Sorry, I was confused. This is not in mainline. It has already been fixed
> > by 51781ce8f448 ("riscv: Pass patch_text() the length in bytes")
> 
> Indeed.
> 
> > But I wouldn't backport that patch, it is bigger than necessary. The patch
> > I sent in the previous email should be enough.
> 
> My suggested fixes are:
> 
> revert 03753bfacbc6(riscv: kprobes: Fix incorrect address calculation)
> apply  51781ce8f448(riscv: Pass patch_text() the length in bytes)
> apply  13134cc94914(riscv: kprobes: Fix incorrect address calculation)

This is probably fine. But I'm paranoid that as 51781ce8f448 ("riscv:
Pass patch_text() the length in bytes") does many things, it may break
something else in v6.6.

Also, just my preference, but I wouldn't revert a commit then apply it
again. I would only cherry-pick 51781ce8f448, and resolve conflicts.

> Because stable-only commit 03753bfacbc6 is actually rebased upstream
> 13134cc94914, and 13134cc94914 relied on 51781ce8f448. So I gave the above
> suggestion. But I'm ok with your previous email patch.

It was just my suggestion. Do as you think best, CC the relevant people
(RISCV maintainers and authors of those commits), see what they think.

Or I could send the diff I sent earlier as a proper patch. Up to you.

Best regards,
Nam