Re: [PATCH bpf] bpf: Fix pending_pos walk on 32-bit ring position wrap

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] `ringbuf_avail_data_sz` incorrectly handles `overwrite_mode` on 32-bit architectures because `max(cons_pos, over_pos)` fails on wrapped sequence numbers.
- [High] `__bpf_user_ringbuf_peek` permanently stalls on 32-bit architectures (and for 32-bit userspace on 64-bit kernels) when the ringbuffer hits 4GB of traffic.
- [High] The user-space ringbuffer consumer loop in `libbpf` uses `cons_pos < prod_pos`, which permanently hangs processing after 4GB of data on 32-bit architectures.
- [Medium] `ringbuf_avail_data_sz()` loads `overwrite_pos` before `producer_pos`, violating the explicit memory barrier contract and risking reads of stale state.
--

--- Patch [1]: [PATCH bpf] bpf: Fix pending_pos walk on 32-bit ring position wrap ---
commit 617dac82211278dfe2a6441e138c7025d95f169b
Author: Israel Téllez García <[email protected]>

bpf: Fix pending_pos walk on 32-bit ring position wrap

This patch fixes a 32-bit wrap-around bug in __bpf_ringbuf_reserve() by
changing an absolute ordering comparison to a signed difference. This ensures
the pending position is correctly advanced even when the counters wrap past 4GB.

> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index 35ae64ade36b8..2c08a98370d5e 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -482,7 +482,7 @@ static void *__bpf_ringbuf_reserve(struct bpf_ringbuf *rb, u64 size)
>  	prod_pos = rb->producer_pos;
>  	new_prod_pos = prod_pos + len;
>  
> -	while (pend_pos < prod_pos) {
> +	while ((long)(prod_pos - pend_pos) > 0) {
>  		hdr = (void *)rb->data + (pend_pos & rb->mask);
>  		hdr_len = READ_ONCE(hdr->len);
>  		if (hdr_len & BPF_RINGBUF_BUSY_BIT)

While reviewing this fix for the 32-bit wrap-around in __bpf_ringbuf_reserve(),
I noticed a few other similar pre-existing 32-bit wrap-around and concurrency
issues in the ringbuf code.

This is a pre-existing issue, but there's a similar wrap issue in
ringbuf_avail_data_sz():

kernel/bpf/ringbuf.c:ringbuf_avail_data_sz() {
    ...
	if (unlikely(rb->overwrite_mode)) {
		over_pos = smp_load_acquire(&rb->overwrite_pos);
		prod_pos = smp_load_acquire(&rb->producer_pos);
		return prod_pos - max(cons_pos, over_pos);
    ...
}

[Severity: High]
Will this code incorrectly handle overwrite_mode on 32-bit architectures?

The max() macro performs an absolute comparison. If over_pos wraps while
cons_pos has not, max() will incorrectly select cons_pos.

This calculation then evaluates modularly to a value larger than the ring size,
returning an artificially inflated available data size that permanently starves
pollers.

[Severity: Medium]
Also in ringbuf_avail_data_sz(), could this load order violate the producer's
barrier semantics?

The producer updates overwrite_pos and then executes
smp_store_release(&rb->producer_pos, ...). The consumer must read producer_pos
with smp_load_acquire() first to guarantee it observes the new overwrite_pos.

By reading overwrite_pos first, a race window allows observing a stale, smaller
overwrite_pos alongside the new producer_pos, throwing off the size calculation.

This isn't a bug introduced by this patch, but __bpf_user_ringbuf_peek() also
permanently stalls on 32-bit architectures:

kernel/bpf/ringbuf.c:__bpf_user_ringbuf_peek() {
    ...
	u64 cons_pos, prod_pos;

	/* Synchronizes with smp_store_release() in user-space producer. */
	prod_pos = smp_load_acquire(&rb->producer_pos);
	if (prod_pos % 8)
		return -EINVAL;

	/* Synchronizes with smp_store_release() in __bpf_user_ringbuf_sample_release() */
	cons_pos = smp_load_acquire(&rb->consumer_pos);
	if (cons_pos >= prod_pos)
		return -ENODATA;
    ...
}

[Severity: High]
Can the zero-extension of 32-bit wrapping counters to 64-bit variables break
modular arithmetic here?

When producer_pos wraps past 4GB to a small value while consumer_pos is large,
the condition evaluates as true (e.g., 4294967200ULL >= 10ULL).

The consumer stops, preventing consumer_pos from ever wrapping to recover,
which causes a permanent stall.

This is a pre-existing issue, but I found a similar wrap issue in the libbpf
user-space consumer:

tools/lib/bpf/ringbuf.c:ringbuf_process_ring() {
    ...
	unsigned long cons_pos, prod_pos;
    ...
		prod_pos = smp_load_acquire(r->producer_pos);
		while (cons_pos < prod_pos) {
			len_ptr = r->data + (cons_pos & r->mask);
    ...
}

[Severity: High]
Would this absolute comparison permanently hang event consumption after 4GB of
data on 32-bit architectures?

When prod_pos wraps past 4GB, it becomes smaller than the non-wrapped cons_pos.

The absolute comparison fails, the loop terminates prematurely, and since no
data is consumed, cons_pos never wraps to recover.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1