Re: [PATCH bpf 5/6] libbpf: ringbuf: Prevent missed wakeups

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Early returns in `ringbuf_process_ring()` bypass the newly added StoreLoad memory barrier, leaving bounded consumers vulnerable to missed wakeups.
- [Low] Multi-line comments introduced by the patch violate the BPF subsystem comment style.
--

commit 1b4f252cfd5df3b60a00d140737f0f9bc3b4ad87
Author: Tamir Duberstein <tamird@kernel.org>
Subject: libbpf: ringbuf: Prevent missed wakeups

This commit addresses missed wakeups in the BPF ring buffer by adding a full
StoreLoad barrier before checking for new data. It ensures either the
consumer observes the producer's update, or the producer observes the
consumer's update and sends a notification. It also updates documentation
and adds a selftest.

> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 9ba6b9ad34983..a3b8f606a91d0 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -1439,6 +1439,10 @@ struct ring_buffer;
>  struct ring;
>  struct user_ring_buffer;
>  
> +/* A negative return stops consumption; non-negative values continue. Stopping
> + * can leave records queued without a new readiness notification. Before
> + * waiting for readiness again, consume until no records remain.
> + */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the BPF subsystem
style? The guidelines prefer the opening /* to be on its own line.

[ ... ]

> diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> index 1c24a83f59d5c..ea8909fec4e98 100644
> --- a/tools/lib/bpf/ringbuf.c
> +++ b/tools/lib/bpf/ringbuf.c
[ ... ]
> @@ -267,14 +267,21 @@ static int64_t ringbuf_process_ring(struct ring *r, size_t n)
>  	cons_pos = __atomic_load_n(r->consumer_pos, __ATOMIC_ACQUIRE);
>  	do {
>  		got_new_data = false;
> +		if (needs_wakeup) {
> +			/* Ensure either this sees a new record or its producer sees
> +			 * the updated consumer position and sends a notification.
> +			 */

[Severity: Low]
This isn't a bug, but should this multi-line comment have the opening /* on
its own line to match the BPF subsystem comment style?

> +			__atomic_thread_fence(__ATOMIC_SEQ_CST);
> +			needs_wakeup = false;
> +		}
>  		prod_pos = __atomic_load_n(r->producer_pos, __ATOMIC_ACQUIRE);
>  		while (cons_pos != prod_pos) {
[ ... ]
> @@ -294,6 +301,7 @@ static int64_t ringbuf_process_ring(struct ring *r, size_t n)
>  
>  			__atomic_store_n(r->consumer_pos, cons_pos,
>  					 __ATOMIC_RELEASE);
> +			needs_wakeup = true;
>  
>  			if (cnt >= n)
>  				goto done;

[Severity: High]
If this loop terminates early by reaching the bound (cnt >= n) or due to a
negative callback return, consumer_pos is updated and the function returns,
discarding the local needs_wakeup variable.

When ring_buffer__consume() is called again to drain the remaining records
before sleeping, it will initialize needs_wakeup = false. This causes the
__ATOMIC_SEQ_CST fence to be skipped prior to loading producer_pos.

Could this allow the CPU to delay the visibility of the consumer_pos store
while reading a stale producer_pos, resulting in the consumer incorrectly
sleeping and the concurrent producer skipping the wakeup notification?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613-bpf-ringbuf-fixes-v1-0-e623481cb724@kernel.org?part=5