Unlike patch_text(), patch_text_nosync() takes the length in bytes, not
number of instructions. It is therefore wrong for arch_prepare_ss_slot() to
pass length=1 while patching one instruction.

This bug was introduced by commit b1756750a397 ("riscv: kprobes: Use
patch_text_nosync() for insn slots"). It has been fixed upstream by commit
51781ce8f448 ("riscv: Pass patch_text() the length in bytes"). However,
beside fixing this bug, this commit does many other things, making it
unsuitable for backporting.

Fix it by properly passing the lengths in bytes.

Fixes: b1756750a397 ("riscv: kprobes: Use patch_text_nosync() for insn slots")
Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 arch/riscv/kernel/probes/kprobes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

On Tue, Apr 29, 2025 at 06:14:18PM +0200, Nam Cao wrote:
> Unlike patch_text(), patch_text_nosync() takes the length in bytes, not
> number of instructions. It is therefore wrong for arch_prepare_ss_slot() to
> pass length=1 while patching one instruction.
> 
> This bug was introduced by commit b1756750a397 ("riscv: kprobes: Use
> patch_text_nosync() for insn slots"). It has been fixed upstream by commit
> 51781ce8f448 ("riscv: Pass patch_text() the length in bytes"). However,
> beside fixing this bug, this commit does many other things, making it
> unsuitable for backporting.

We would almost always want the original commit, why not just send that
instead?  What is wrong with it being in here as-is?

thanks,

greg k-h

On Tue, Apr 29, 2025 at 06:31:09PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 29, 2025 at 06:14:18PM +0200, Nam Cao wrote:
> > Unlike patch_text(), patch_text_nosync() takes the length in bytes, not
> > number of instructions. It is therefore wrong for arch_prepare_ss_slot() to
> > pass length=1 while patching one instruction.
> > 
> > This bug was introduced by commit b1756750a397 ("riscv: kprobes: Use
> > patch_text_nosync() for insn slots"). It has been fixed upstream by commit
> > 51781ce8f448 ("riscv: Pass patch_text() the length in bytes"). However,
> > beside fixing this bug, this commit does many other things, making it
> > unsuitable for backporting.
> 
> We would almost always want the original commit, why not just send that
> instead?  What is wrong with it being in here as-is?

The original commit is probably fine. But I'm paranoid, because it is not
completely obvious whether the original commit would break something else
in v6.6. Because, as mentioned, it does more than just fixing the bug.

Best regards,
Nam

On Tue, Apr 29, 2025 at 06:48:21PM +0200, Nam Cao wrote:
> On Tue, Apr 29, 2025 at 06:31:09PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 29, 2025 at 06:14:18PM +0200, Nam Cao wrote:
> > > Unlike patch_text(), patch_text_nosync() takes the length in bytes, not
> > > number of instructions. It is therefore wrong for arch_prepare_ss_slot() to
> > > pass length=1 while patching one instruction.
> > > 
> > > This bug was introduced by commit b1756750a397 ("riscv: kprobes: Use
> > > patch_text_nosync() for insn slots"). It has been fixed upstream by commit
> > > 51781ce8f448 ("riscv: Pass patch_text() the length in bytes"). However,
> > > beside fixing this bug, this commit does many other things, making it
> > > unsuitable for backporting.
> > 
> > We would almost always want the original commit, why not just send that
> > instead?  What is wrong with it being in here as-is?
> 
> The original commit is probably fine. But I'm paranoid, because it is not
> completely obvious whether the original commit would break something else
> in v6.6. Because, as mentioned, it does more than just fixing the bug.

You should be more paranoid about creating a one-off change that is NOT
upstream as in our experience, that almost always causes a new problem.

Please just backport the original and submit it after testing it out.

thanks,

greg k-h