Re: [PATCH v2 bpf 2/4] bpf: Fix available-data accounting on 32-bit wrap in overwrite mode

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

Pre-existing issues:
- [High] 32-bit wrap-around bug in `__bpf_user_ringbuf_peek` deadlocks the user ringbuf.
--

--- Patch [2]: [PATCH v2 bpf 2/4] bpf: Fix available-data accounting on 32-bit wrap in overwrite mode ---
commit def0b55b3ee1635d80af8891542c5b83a8fe238e
Author: Israel Téllez García <[email protected]>

bpf: Fix available-data accounting on 32-bit wrap in overwrite mode

In overwrite mode ringbuf_avail_data_sz() picks the newer of the consumer
and overwrite positions before measuring how much data is available. This
commit fixes a 32-bit wrap issue by comparing distances rather than
positions.

> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index 06d3cc192601f..0fefa89039beb 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -321,7 +321,7 @@ static unsigned long ringbuf_avail_data_sz(struct bpf_ringbuf *rb)
>  	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);
> +		return min(prod_pos - cons_pos, prod_pos - over_pos);
>  	} else {
>  		prod_pos = smp_load_acquire(&rb->producer_pos);
>  		return prod_pos - cons_pos;

[Severity: High]
This is a pre-existing issue, but does a similar 32-bit wrap-around bug
exist in __bpf_user_ringbuf_peek() within the same file?

Looking at __bpf_user_ringbuf_peek(), it loads the 32-bit producer and
consumer positions into u64 variables:

kernel/bpf/ringbuf.c:__bpf_user_ringbuf_peek() {
    ...
    u64 cons_pos, prod_pos;
    ...
    prod_pos = smp_load_acquire(&rb->producer_pos);
    ...
    cons_pos = smp_load_acquire(&rb->consumer_pos);
    if (cons_pos >= prod_pos)
        return -ENODATA;
    ...
    if (total_len > prod_pos - cons_pos)
        return -EINVAL;
    ...
}

Because these are zero-extended into u64 on 32-bit architectures, does the
ordering comparison (cons_pos >= prod_pos) evaluate to true when the 32-bit
producer_pos wraps around to 0 (e.g., 0xFFFFFFF8 >= 0x00000008)?

If so, would this cause __bpf_user_ringbuf_peek() to incorrectly return
-ENODATA, failing to advance cons_pos and permanently deadlocking the user
ringbuffer once more than 4GB of data is pushed?

Additionally, does the u64 zero-extension break the modulo arithmetic in the
distance calculation (prod_pos - cons_pos)?

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