Re: [PATCH v5 1/3] riscv: build kernel atomics from LR/SC when Zaamo is disabled

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 9757 bytes --]

> riscv: build kernel atomics from LR/SC when Zaamo is disabled

TL;DR:
- Unbounded futex LR/SC loop may starve, spinning with hash locks held
- kprobe on LR/SC atomics may livelock
- Zacas/Zabha still emit AMOs, contradicting docs
- redundant lr/sc ordering suffixes

> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig

[ ... ]

> +config RISCV_ISA_ZAAMO
> +	bool "Zaamo extension support for AMO atomics" if TOOLCHAIN_HAS_ZALRSC_ZAAMO
> +	default y
> +	help
> +	  Use AMO instructions for kernel atomic memory operations. When
> +	  disabled, those operations are built from LR/SC and the kernel
> +	  is compiled with zalrsc instead of the A extension.
> +
> [ ... ]
> +	  A kernel built without Zaamo runs on any CPU that implements the
> +	  A extension; it only does not use AMO.

The help text says a kernel built without Zaamo "only does not use AMO", and
the commit message says "On such a CPU the kernel must use LR/SC for every
atomic" and "every atomic is a constrained LR/SC loop".

Is that accurate when the hart reports Zacas or Zabha?  RISCV_ISA_ZACAS and
RISCV_ISA_ZABHA stay default y and do not depend on RISCV_ISA_ZAAMO, so a
ZAAMO=n kernel still issues AMO-class instructions on those harts:

__arch_cmpxchg():
    if (IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) && ...
        riscv_has_extension_unlikely(RISCV_ISA_EXT_ZACAS))
    -> amocas.w / amocas.d

__arch_cmpxchg_masked() -> amocas.b / amocas.h
__arch_xchg_masked()    -> amoswap.b / amoswap.h (Zabha)

riscv_spinlock_init() also selects the queued spinlock using Zabha when
Zabha and Zacas are both present, so the spinlock fast paths would run on
amocas/amoswap.h.

On a CPU that emulates AMOs with an M-mode trap and also reports
Zacas/Zabha, the hottest atomics would still trap.

Should RISCV_ISA_ZACAS and RISCV_ISA_ZABHA depend on RISCV_ISA_ZAAMO?  If
not, should the help text and changelog say that Zacas/Zabha AMOs are still
used when the hart reports them?

[ ... ]

> diff --git a/arch/riscv/include/asm/atomic.h b/arch/riscv/include/asm/atomic.h
> --- a/arch/riscv/include/asm/atomic.h
> +++ b/arch/riscv/include/asm/atomic.h

[ ... ]

> +#else
> +#define ATOMIC_OP(op, asm_op, I, asm_type, c_type, prefix)		\
> +static __always_inline							\
> +void arch_atomic##prefix##_##op(c_type i, atomic##prefix##_t *v)	\
> +{									\
> +	register c_type ret, temp;					\
> +	__asm__ __volatile__ (						\
> +		"1:	lr." #asm_type " %1, %0\n"			\
> +		"	" #asm_op " %2, %1, %3\n"			\
> +		"	sc." #asm_type " %2, %2, %0\n"			\
> +		"	bnez %2, 1b\n"					\
> +		: "+A" (v->counter), "=&r" (ret), "=&r" (temp)		\
> +		: "r" (I)						\
> +		: "memory");						\
> +}
> +#endif

Can a kprobe placed on one of these lr/sc loops livelock the CPU?  With
CONFIG_RISCV_ISA_ZAAMO=n, every inlined arch_atomic{,64}_<op>() and
arch_atomic{,64}_fetch_<op>{,_relaxed}() becomes an lr / op / sc / bnez
loop, where before it was a single amo<op> instruction that is safe to
probe.

arch_check_kprobe() only checks that the address is on an instruction
boundary, and riscv_probe_decode_insn() rejects only system, fence and
c.ebreak instructions, so a probe on the lr, the sc, or an instruction
between them is accepted.  The exception return path in entry.S then clears
the reservation on purpose:

    REG_L  a2, PT_EPC(sp)
    REG_SC x0, a2, PT_EPC(sp)

The trap for the single-step slot does the same, so the following sc always
fails, bnez goes back to lr, the probe fires again, and the loop never
completes.

This was already possible for the existing cmpxchg and conditional-atomic
LR/SC loops, but with ZAAMO=n it now reaches every atomic_inc(),
atomic_add() and atomic_fetch_*() site, including small out-of-line
functions whose first instruction is the lr.

A later commit in this series, 01f0e5fb499d ("riscv: probes: reject probes
inside an LR/SC sequence"), fixes this, but it is still reachable at this
commit, which matters for bisection and backports.

[ ... ]

> diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h
> --- a/arch/riscv/include/asm/bitops.h
> +++ b/arch/riscv/include/asm/bitops.h

[ ... ]

> +#else
> +#define __test_and_op_bit_ord(op, mod, nr, addr, ord)		\
> +({								\
> +	unsigned long __res, __mask, __temp;			\
> +	__mask = BIT_MASK(nr);					\
> +	__asm__ __volatile__ (					\
> +		"1: " __LR #ord " %0, %1\n"			\
> +		#op " %2, %0, %3\n"				\
> +		__SC #ord " %2, %2, %1\n"			\
> +		"bnez %2, 1b\n"					\
> +		: "=&r" (__res), "+A" (addr[BIT_WORD(nr)]), "=&r" (__temp)	\
> +		: "r" (mod(__mask))				\
> +		: "memory");					\
> +	((__res & __mask) != 0);				\
> +})
> +
> +#define __op_bit_ord(op, mod, nr, addr, ord)			\
> +do {								\
> +	unsigned long __res, __temp;				\
> +	__asm__ __volatile__ (					\
> +		"1: " __LR #ord " %0, %1\n"			\
> +		#op " %2, %0, %3\n"				\
> +		__SC #ord " %2, %2, %1\n"			\
> +		"bnez %2, 1b\n"					\
> +		: "=&r" (__res), "+A" (addr[BIT_WORD(nr)]), "=&r" (__temp)	\
> +		: "r" (mod(BIT_MASK(nr)))			\
> +		: "memory");					\
> +} while (0)
> +#endif

Both macros put the same ordering suffix on the lr and on the sc.  That
gives:

arch_test_and_set_bit_lock()   (ord = .aq)  -> lr.d.aq  ... sc.d.aq
arch_clear_bit_unlock()        (ord = .rl)  -> lr.d.rl  ... sc.d.rl
arch___clear_bit_unlock()      (ord = .rl)  -> lr.d.rl  ... sc.d.rl

arch_xor_unlock_is_negative_byte() hard-codes the same pattern with
__LR ".rl" and __SC ".rl".

The A extension spec says software should not set rl on an LR unless aq is
also set, and should not set aq on an SC unless rl is also set, and that
LR.rl and SC.aq are not guaranteed to provide any stronger ordering than
having both bits clear but may result in lower performance.

The ordering that is wanted still comes from lr.aq and sc.rl, so the
semantics hold, but the other annotations do nothing useful and may be
slower on the hardware this option targets.  They also differ from the rest
of arch/riscv, where LR/SC ordering is a plain lr plus sc.rl and fences (see
arch_cmpxchg_acquire/_release and __arch_cmpxchg(), whose lr has no
ordering suffix).

Could these macros take separate lr and sc suffixes, as the cmpxchg macros
do with lr_sfx/sc_sfx?

[ ... ]

> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h

[ ... ]

This is the same issue as the Kconfig comment above.  The #ifdef
CONFIG_RISCV_ISA_ZAAMO in this header does not touch these paths, which
still emit AMO instructions when the hart advertises Zabha or Zacas:

__arch_xchg_masked()      -> amoswap.b/.h (Zabha), including xchg_relaxed()
                             on the 16-bit qspinlock tail
__arch_cmpxchg()          -> amocas.w/.d (Zacas), i.e. every cmpxchg and
                             try_cmpxchg, including the spinlock fast paths
__arch_cmpxchg_masked()   -> amocas.b/.h (Zabha + Zacas)
__arch_cmpxchg128()       -> amocas.q

These still build because each one enables the extension with
".option arch, +zabha" or ".option arch, +zacas".

This contradicts the commit message statement "A kernel built this way runs
on any hart that implements A; it just does not use AMO".  Should
RISCV_ISA_ZACAS and RISCV_ISA_ZABHA depend on RISCV_ISA_ZAAMO?

[ ... ]

> diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h
> --- a/arch/riscv/include/asm/futex.h
> +++ b/arch/riscv/include/asm/futex.h

[ ... ]

> +#else
> +#define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)	\
> +{								\
> +	int temp;						\
> +								\
> +	__enable_user_access();					\
> +	__asm__ __volatile__ (					\
> +	"1:	lr.w.aqrl %[ov], %[u]			\n"	\
> +	"	" insn "				\n"	\
> +	"2:	sc.w.aqrl %[t], %[t], %[u]		\n"	\
> +	"	bnez %[t], 1b				\n"	\

Can this loop run unbounded while the futex hash-bucket locks are held?
With CONFIG_RISCV_ISA_ZAAMO=n, FUTEX_WAKE_OP now runs an LR/SC loop on user
memory:

futex(FUTEX_WAKE_OP)
  futex_wake_op()                    /* kernel/futex/waitwake.c */
    double_lock_hb(hb1, hb2)
    futex_atomic_op_inuser(op, uaddr2)
      pagefault_disable()
      arch_futex_atomic_op_inuser()
        __futex_atomic_op()

The loop exits only when sc.w.aqrl succeeds or faults.  The RISC-V
eventual-success guarantee for constrained LR/SC loops (the one Ziccrse
provides) does not hold while other harts keep writing to the reservation
set; the spec says that in that case no hart is guaranteed to leave its
LR/SC loop.

A process can do exactly that: one thread calls FUTEX_WAKE_OP on uaddr2
while a second thread on another hart stores to *uaddr2, or anything else
in the same reservation granule, in a tight loop.  The kernel then spins
with preemption disabled and both hash-bucket locks held.  The old
amo*.w.aqrl version cannot be starved this way because an AMO always
completes.

arm64 bounds its LDXR/STXR futex loop: LLSC_FUTEX_ATOMIC_OP() in
arch/arm64/include/asm/futex.h counts down FUTEX_MAX_LOOPS and returns
-EAGAIN, and futex_wake_op() already handles that:

    if (!IS_ENABLED(CONFIG_MMU) ||
        unlikely(op_ret != -EFAULT && op_ret != -EAGAIN))

by dropping the locks, calling cond_resched() and retrying.

The commit message says Ziccrse is "the forward-progress guarantee for such
loops", which does not cover this case.  The existing riscv
futex_atomic_cmpxchg_inatomic() has the same unbounded pattern, but that
does not make the new instance correct.

Should both loops be bounded the way arm64 does it?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42069736200