Re: [PATCH v2 bpf 3/4] bpf: Read producer_pos before overwrite_pos in ringbuf_avail_data_sz()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Swapping the load order of producer_pos and overwrite_pos introduces a TOCTOU race that causes an integer underflow in `prod_pos - over_pos`, resulting in a massively overstated available data size.
--

--- Patch [3]: [PATCH v2 bpf 3/4] bpf: Read producer_pos before overwrite_pos in ringbuf_avail_data_sz() ---
commit efaae45af0e3891c65188df6e4677342c6fb0c41
Author: Israel Téllez García <[email protected]>

bpf: Read producer_pos before overwrite_pos in ringbuf_avail_data_sz()

This commit changes the order in which producer_pos and overwrite_pos are
read in ringbuf_avail_data_sz() to match the documented memory ordering
contract. It swaps the reads so that the acquire load of producer_pos
orders the subsequent load of overwrite_pos.

> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index 0fefa89039beb..89a9c15a260f3 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -319,8 +319,14 @@ static unsigned long ringbuf_avail_data_sz(struct bpf_ringbuf *rb)
>  	cons_pos = smp_load_acquire(&rb->consumer_pos);
>  
>  	if (unlikely(rb->overwrite_mode)) {
> -		over_pos = smp_load_acquire(&rb->overwrite_pos);
> +		/*
> +		 * Read producer_pos first: its release store in
> +		 * __bpf_ringbuf_reserve() is what publishes the matching
> +		 * overwrite_pos.
> +		 */
>  		prod_pos = smp_load_acquire(&rb->producer_pos);
> +		/* Ordered after the acquire load above, per that contract. */
> +		over_pos = smp_load_acquire(&rb->overwrite_pos);
>  		return min(prod_pos - cons_pos, prod_pos - over_pos);

[Severity: High]
Does this sequence allow an integer underflow in prod_pos - over_pos?

If the reader is preempted after reading prod_pos, and a concurrent producer
advances overwrite_pos beyond the stale prod_pos snapshot:

Reader:
    prod_pos = smp_load_acquire(&rb->producer_pos);
    <preempted>

Producer:
    // advances overwrite_pos beyond the snapshotted prod_pos
    rb->overwrite_pos = ...;
    smp_store_release(&rb->producer_pos, ...);

Reader resumes:
    over_pos = smp_load_acquire(&rb->overwrite_pos);
    return min(prod_pos - cons_pos, prod_pos - over_pos);

Could over_pos now be greater than prod_pos, causing the subtraction to
underflow to a massive unsigned value near ULONG_MAX?

This would result in min() returning prod_pos - cons_pos, which could be
extremely large and break EPOLLOUT for user-space producers by returning
incorrectly large available data sizes.

>  	} else {
>  		prod_pos = smp_load_acquire(&rb->producer_pos);
>  		return prod_pos - cons_pos;

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