Re: [PATCH fixes] riscv: uprobes: Add missing fence.i after building the XOL buffer

[Samuel Holland <samuel.holland@sifive.com>]

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