Re: [PATCH bpf v1 RESEND] bpf: Fix timer_lockup deadlock using atomic_fetch_inc()

[bot+bpf-ci@kernel.org]

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

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index b3cc5c8fc875..7d89dc143883 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1591,9 +1591,7 @@ BPF_CALL_1(bpf_timer_cancel, struct bpf_async_kern *, async)
>  	 */
>  	if (!cur_t)
>  		goto drop;
> -	atomic_inc(&t->cancelling);
> -	/* Need full barrier after relaxed atomic_inc */
> -	smp_mb__after_atomic();
> +	atomic_fetch_inc(&t->cancelling);
>  	inc = true;
>  	if (atomic_read(&cur_t->cancelling)) {
>  		/* We're cancelling timer t, while some other timer callback is

The commit message states:

> Instead of relying on arch-specific macro implementations which may vary
> in strictness, fix this issue directly in the BPF core helper by replacing
> atomic_inc() and smp_mb__after_atomic() with a single atomic_fetch_inc()
> to provide full ordering natively.
> ...
> making the lockless deadlock detection logic immune to any
> platform-level barrier interpretation variations.

Is this rationale consistent with the atomic API documentation?
Documentation/atomic_t.txt:208-216 states that atomic_fetch_inc() is
equivalent to:

    smp_mb__before_atomic();
    atomic_fetch_add_relaxed();
    smp_mb__after_atomic();

The generic implementation in include/linux/atomic/atomic-arch-fallback.h
shows that atomic_fetch_inc() expands to __atomic_pre_full_fence() +
relaxed RMW + __atomic_post_full_fence(), where those fences are defined
as smp_mb__before_atomic() and smp_mb__after_atomic() in
include/linux/atomic.h:50-55.

On architectures that only supply the *_relaxed RMW variants, the new line
re-expands to exactly the sequence the patch deleted. The claim that this
makes the code "immune to any platform-level barrier interpretation
variations" and provides "full ordering natively" does not follow from the
API contract.

The patch does fix the reported LoongArch lockup, but only because
arch/loongarch/include/asm/atomic-amo.h emits the fully-ordered fetch
variant as amadd_db.w when CONFIG_CPU_HAS_AMO is set.

The actual contract violation appears to be in the architecture.
Documentation/atomic_t.txt:190-206 defines smp_mb__after_atomic() as a
barrier that augments the ordering inherent to the op, with
smp_mb__after_atomic() ordering all later accesses against the RMW op.
arch/loongarch/include/asm/barrier.h:60-61 defines both as plain
barrier(), while arch/loongarch/include/asm/atomic-amo.h's void-returning
ATOMIC_OP emits am<op>.w with no _db suffix - unordered. That combination
breaks the documented contract for every user, and there are approximately
270 smp_mb__after_atomic() call sites under kernel/, mm/, net/, fs/, lib/
and drivers/ that rely on it.

Fixing only this one BPF helper hides the arch bug rather than fixing it.
Should the fix go to arch/loongarch/include/asm/barrier.h instead, or does
the commit message need to justify why the arch macro is considered
permissibly weak?

The patch also deletes the only in-code record of the ordering requirement:

> -	atomic_inc(&t->cancelling);
> -	/* Need full barrier after relaxed atomic_inc */
> -	smp_mb__after_atomic();
> +	atomic_fetch_inc(&t->cancelling);

Nothing in the resulting code indicates that the value-returning variant
was chosen purely for its full-ordering semantics, so the line reads as a
plain refcount bump with a discarded return value. The store-load pairing
is the whole mechanism: t->cancelling must be globally visible before
cur_t->cancelling is read, otherwise two callbacks cancelling each other
both read 0 and both enter the unbounded wait in hrtimer_cancel()
(kernel/time/hrtimer.c hrtimer_cancel()).

A discarded-return-value RMW is an obvious target for a later unused
result cleanup back to atomic_inc(), which would silently reintroduce
exactly the lockup this commit fixes. Discarding a fetch op's result is
rare in core code - the only other instances are plain counters
(drivers/gpu/drm/drm_file.c:383, drivers/accel/drm_accel.c:129), none of
them ordering-motivated.

Would it make sense to keep a one-line comment on the new statement
explaining that atomic_fetch_inc() is used (return value ignored) for its
full ordering against the atomic_read() of cur_t->cancelling that follows?


---
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/33578169526