Re: [PATCH v5 3/3] riscv: probes: reject probes inside an LR/SC sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] riscv: probes: arch_check_kprobe() falsely rejects probes after __cmpwait()
--

commit 4dd5cb2e2ba32786ac861a461730851e4daa6b77
Author: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>

riscv: probes: reject probes inside an LR/SC sequence

This commit updates the RISC-V kprobes and uprobes instruction decoders and
validators to reject probes placed on or between load-reserved (lr) and
store-conditional (sc) instructions, preventing infinite retry loops caused
by breakpoint traps clearing the hart's reservation.

> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> index 9e2afabf94e80..9c2284eafae90 100644
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
[ ... ]
> @@ -61,9 +70,18 @@ static bool __kprobes arch_check_kprobe(unsigned long addr)
>  
>  	while (tmp <= addr) {
>  		if (tmp == addr)
> -			return true;
> +			return !in_lrsc;
> +
> +		insn = *(u16 *)tmp;
> +		if (GET_INSN_LENGTH(insn) == 4) {
> +			insn |= (u32)*(u16 *)(tmp + 2) << 16;
> +			if (riscv_insn_is_lr(insn))
> +				in_lrsc = true;
> +			else if (riscv_insn_is_sc(insn))
> +				in_lrsc = false;
> +		}
>  
> -		tmp += GET_INSN_LENGTH(*(u16 *)tmp);
> +		tmp += GET_INSN_LENGTH(insn);
>  	}

[Severity: Medium]
Can this leave in_lrsc stuck at true for the rest of a function after an
inlined __cmpwait() sequence?

In arch_check_kprobe(), in_lrsc is set to true whenever riscv_insn_is_lr()
matches an lr.w or lr.d instruction, and is only cleared when
riscv_insn_is_sc() matches an sc.w or sc.d instruction.

However, when CONFIG_RISCV_ISA_ZAWRS is enabled (which is default y),
__cmpwait() in arch/riscv/include/asm/cmpxchg.h uses lr.w or lr.d paired with
wrs.nto and no sc instruction to implement smp_cond_load_relaxed(),
smp_cond_load_acquire(), and atomic_cond_read_*():

	case 4:
		asm volatile(
		"	lr.w	%0, %1\n"
		"	xor	%0, %0, %2\n"
		"	bnez	%0, 1f\n"
			ZAWRS_WRS_NTO "\n"
		"1:"
		: "=&r" (tmp), "+A" (*(u32 *)ptr)
		: "r" (val));
		break;

When register_kprobe() -> arch_prepare_kprobe() -> arch_check_kprobe() walks
past an inlined __cmpwait() in functions such as try_to_wake_up(),
queued_spin_lock_slowpath(), or osq_lock(), won't in_lrsc remain true after
the wrs.nto and cause arch_check_kprobe() to falsely reject kprobes on all
subsequent instructions in that function (unless an unrelated sc happens to
appear before addr)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-lrsc-only-v5-0-f0648f57b2e1@mobileye.com?part=3