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

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

[Kai Zhang]

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

Regards,
laokz

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

[Greg Kroah-Hartman]

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

Neither of these apply cleanly.  Please provide working backports if you
wish to see them added to the tree.

thanks,

greg k-h

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

[Kai Zhang]

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
> 
> Neither of these apply cleanly.  Please provide working backports if you
> wish to see them added to the tree.
> 
> thanks,
> 
> greg k-h

revert 03753bfacbc6
apply  51781ce8f448
apply  13134cc94914

Regards,
laokz

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

[Greg Kroah-Hartman]

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
> > 
> > Neither of these apply cleanly.  Please provide working backports if you
> > wish to see them added to the tree.
> > 
> > thanks,
> > 
> > greg k-h
> 
> revert 03753bfacbc6
> apply  51781ce8f448
> apply  13134cc94914

Thanks, but that's not how we take patches for the stable tree.  Please
submit these all in a tested patch series and we will be glad to queue
them up.

greg k-h

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

[Kai Zhang]

Hi Nam,

I reported a riscv kprobe bug of linux-6.6.y. It seems that 
03753bfacbc6(riscv: kprobes: Fix incorrect address calculation) should 
be reverted. There are a lot of changes of riscv kprobe in upstream. I'm 
not all in sure of my suggested fix. Will you kind to help?

Thanks,
laokz

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
>>>
>>> Neither of these apply cleanly.  Please provide working backports if you
>>> wish to see them added to the tree.
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> revert 03753bfacbc6
>> apply  51781ce8f448
>> apply  13134cc94914
> 
> Thanks, but that's not how we take patches for the stable tree.  Please
> submit these all in a tested patch series and we will be glad to queue
> them up.
> 
> greg k-h

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

[Nam Cao]

On Fri, Apr 25, 2025 at 08:09:21PM +0800, Kai Zhang wrote:
> Hi Nam,
> 
> I reported a riscv kprobe bug of linux-6.6.y. It seems that
> 03753bfacbc6(riscv: kprobes: Fix incorrect address calculation) should be
> reverted. There are a lot of changes of riscv kprobe in upstream. I'm not
> all in sure of my suggested fix. Will you kind to help?

Certainly.

> Thanks,
> laokz
> 
> 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.

Can you please verify if the below patch fixes your crash?

Best regards,
Nam

diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
index 4fbc70e823f0..dc431b965bc3 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -28,8 +28,8 @@ static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
 
 	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);
+	patch_text_nosync(p->ainsn.api.insn, &p->opcode, offset);
+	patch_text_nosync((void *)p->ainsn.api.insn + offset, &insn, sizeof(insn));
 }
 
 static void __kprobes arch_prepare_simulate(struct kprobe *p)

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

[Nam Cao]

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")

But I wouldn't backport that patch, it is bigger than necessary. The patch
I sent in the previous email should be enough.

Nam

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

[Kai Zhang]

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)

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.

Thanks a lot,
laokz

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

[Nam Cao]

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

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

[Kai Zhang]

On 4/28/2025 3:22 PM, Nam Cao wrote:
> 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.

Great. I'd like you could help fix the problem. Thanks again.

laokz

> Best regards,
> Nam

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

[Kai Zhang]

On 4/25/2025 8:49 PM, Nam Cao wrote:
> On Fri, Apr 25, 2025 at 08:09:21PM +0800, Kai Zhang wrote:
>> Hi Nam,
>>
>> I reported a riscv kprobe bug of linux-6.6.y. It seems that
>> 03753bfacbc6(riscv: kprobes: Fix incorrect address calculation) should be
>> reverted. There are a lot of changes of riscv kprobe in upstream. I'm not
>> all in sure of my suggested fix. Will you kind to help?
> 
> Certainly.

Thank you very much!

>> Thanks,
>> laokz
>>
>> 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.

No. Because patch_text_nosync will write only one byte instead of one 
instruction(2 or 4 bytes).

> 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.
> 
> Can you please verify if the below patch fixes your crash?

Sure. I already confirmed that the following patch indeed fixed my 
crash. But there is still something else I'll talk about in the next mail.

> Best regards,
> Nam
> 
> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> index 4fbc70e823f0..dc431b965bc3 100644
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
> @@ -28,8 +28,8 @@ static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
>   
>   	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);
> +	patch_text_nosync(p->ainsn.api.insn, &p->opcode, offset);
> +	patch_text_nosync((void *)p->ainsn.api.insn + offset, &insn, sizeof(insn));
>   }
>   
>   static void __kprobes arch_prepare_simulate(struct kprobe *p)

Regards,
laokz