| Message ID | 20250417074952.1315641-1-bjorn@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [fixes] riscv: uprobes: Add missing fence.i after building the XOL buffer | expand |
On Thu, Apr 17, 2025 at 3:50 PM Björn Töpel <bjorn@kernel.org> wrote: > > From: Björn Töpel <bjorn@rivosinc.com> > > The XOL (execute out-of-line) buffer is used to single-step the > replaced instruction(s) for uprobes. The RISC-V port was missing a > proper fence.i (i$ flushing) after constructing the XOL buffer, which > can result in incorrect execution of stale/broken instructions. > > This was found running the BPF selftests "test_progs: > uprobe_autoattach, attach_probe" on the Spacemit K1/X60, where the > uprobes tests randomly blew up. Good catch, Thx! Reviewed-by: Guo Ren <guoren@kernel.org> > > Fixes: 74784081aac8 ("riscv: Add uprobes supported") > Signed-off-by: Björn Töpel <bjorn@rivosinc.com> > --- > arch/riscv/kernel/probes/uprobes.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c > index 4b3dc8beaf77..4faef92dd771 100644 > --- a/arch/riscv/kernel/probes/uprobes.c > +++ b/arch/riscv/kernel/probes/uprobes.c > @@ -176,13 +176,6 @@ void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr, > *(uprobe_opcode_t *)dst = __BUG_INSN_32; > } > > + flush_icache_range((unsigned long)dst, (unsigned long)dst + len); > kunmap_atomic(kaddr); > - > - /* > - * We probably need flush_icache_user_page() but it needs vma. > - * This should work on most of architectures by default. If > - * architecture needs to do something different it can define > - * its own version of the function. > - */ > - flush_dcache_page(page); > } > > base-commit: 1a1d569a75f3ab2923cb62daf356d102e4df2b86 > -- > 2.45.2 >
Hi Björn, On 2025-04-17 2:49 AM, Björn Töpel wrote: > From: Björn Töpel <bjorn@rivosinc.com> > > The XOL (execute out-of-line) buffer is used to single-step the > replaced instruction(s) for uprobes. The RISC-V port was missing a > proper fence.i (i$ flushing) after constructing the XOL buffer, which > can result in incorrect execution of stale/broken instructions. > > This was found running the BPF selftests "test_progs: > uprobe_autoattach, attach_probe" on the Spacemit K1/X60, where the > uprobes tests randomly blew up. > > Fixes: 74784081aac8 ("riscv: Add uprobes supported") > Signed-off-by: Björn Töpel <bjorn@rivosinc.com> > --- > arch/riscv/kernel/probes/uprobes.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c > index 4b3dc8beaf77..4faef92dd771 100644 > --- a/arch/riscv/kernel/probes/uprobes.c > +++ b/arch/riscv/kernel/probes/uprobes.c > @@ -176,13 +176,6 @@ void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr, > *(uprobe_opcode_t *)dst = __BUG_INSN_32; > } > > + flush_icache_range((unsigned long)dst, (unsigned long)dst + len); This works because flush_icache_range currently ignores the range, but semantically is not quite right, because of the line just above the context that increments dst. If the range was respected, this would only flush the ebreak, not the preceding single-stepped instruction. Regards, Samuel > kunmap_atomic(kaddr); > - > - /* > - * We probably need flush_icache_user_page() but it needs vma. > - * This should work on most of architectures by default. If > - * architecture needs to do something different it can define > - * its own version of the function. > - */ > - flush_dcache_page(page); > } > > base-commit: 1a1d569a75f3ab2923cb62daf356d102e4df2b86
Samuel Holland <samuel.holland@sifive.com> writes: > Hi Björn, > > On 2025-04-17 2:49 AM, Björn Töpel wrote: >> From: Björn Töpel <bjorn@rivosinc.com> >> >> The XOL (execute out-of-line) buffer is used to single-step the >> replaced instruction(s) for uprobes. The RISC-V port was missing a >> proper fence.i (i$ flushing) after constructing the XOL buffer, which >> can result in incorrect execution of stale/broken instructions. >> >> This was found running the BPF selftests "test_progs: >> uprobe_autoattach, attach_probe" on the Spacemit K1/X60, where the >> uprobes tests randomly blew up. >> >> Fixes: 74784081aac8 ("riscv: Add uprobes supported") >> Signed-off-by: Björn Töpel <bjorn@rivosinc.com> >> --- >> arch/riscv/kernel/probes/uprobes.c | 9 +-------- >> 1 file changed, 1 insertion(+), 8 deletions(-) >> >> diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c >> index 4b3dc8beaf77..4faef92dd771 100644 >> --- a/arch/riscv/kernel/probes/uprobes.c >> +++ b/arch/riscv/kernel/probes/uprobes.c >> @@ -176,13 +176,6 @@ void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr, >> *(uprobe_opcode_t *)dst = __BUG_INSN_32; >> } >> >> + flush_icache_range((unsigned long)dst, (unsigned long)dst + len); > > This works because flush_icache_range currently ignores the range, but > semantically is not quite right, because of the line just above the context that > increments dst. If the range was respected, this would only flush the ebreak, > not the preceding single-stepped instruction. Indeed! That was sloppy! I'll spin a v2! Thank you! Björn
diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c index 4b3dc8beaf77..4faef92dd771 100644 --- a/arch/riscv/kernel/probes/uprobes.c +++ b/arch/riscv/kernel/probes/uprobes.c @@ -176,13 +176,6 @@ void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr, *(uprobe_opcode_t *)dst = __BUG_INSN_32; } + flush_icache_range((unsigned long)dst, (unsigned long)dst + len); kunmap_atomic(kaddr); - - /* - * We probably need flush_icache_user_page() but it needs vma. - * This should work on most of architectures by default. If - * architecture needs to do something different it can define - * its own version of the function. - */ - flush_dcache_page(page); }