Re: [PATCH v2 bpf 4/4] libbpf: Fix ring buffer consumer loop on 32-bit position wrap

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

New issues:
- [High] The loop condition `prod_pos - cons_pos > 0` uses unsigned arithmetic, turning it into a simple inequality check (`!=`) which causes an infinite loop if `cons_pos` oversteps `prod_pos`.

Pre-existing issues:
- [High] 32-bit user space on 64-bit kernel will experience permanent ringbuffer stall after 4GB of traffic.
- [High] The kernel's `__bpf_user_ringbuf_peek` implementation suffers from a 32-bit position wrap bug that permanently breaks the map on 32-bit architectures.
--

--- Patch [4]: [PATCH v2 bpf 4/4] libbpf: Fix ring buffer consumer loop on 32-bit position wrap ---
commit fb935226d411d583439a72026e8126519f36b208
Author: Israel Téllez García <[email protected]>

libbpf: Fix ring buffer consumer loop on 32-bit position wrap

ringbuf_process_ring() walks the records between the consumer and the
producer with an ordering comparison. This patch fixes a 32-bit position
wrap bug by comparing the distance instead, as the consumer never runs
ahead of the producer.

> diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> index 00ec4837a06de..c8fe57401a8b5 100644
> --- a/tools/lib/bpf/ringbuf.c
> +++ b/tools/lib/bpf/ringbuf.c
> @@ -244,7 +244,7 @@ static int64_t ringbuf_process_ring(struct ring *r, size_t n)
>  	do {
>  		got_new_data = false;
>  		prod_pos = smp_load_acquire(r->producer_pos);

[Severity: High]
This isn't a bug introduced by this patch, but on a 32-bit userspace running
on a 64-bit kernel, does this lead to a permanent ring buffer stall?

Because unsigned long is 32-bit in userspace but the kernel uses 64-bit
positions, won't producer_pos in the kernel exceed 2^32 after 4GB of
traffic, while the 32-bit userspace only updates the lower 32 bits of
consumer_pos?

Does this cause prod_pos - cons_pos in the kernel to permanently exceed the
ring buffer mask, causing bpf_ringbuf_has_space() to reject all future
records?

> -		while (cons_pos < prod_pos) {
> +		while (prod_pos - cons_pos > 0) {

[Severity: High]
Does this condition risk an infinite loop in the consumer?

Since prod_pos and cons_pos are unsigned long, their subtraction is
unsigned and strictly >= 0, making > 0 act identically to != 0.

If cons_pos oversteps prod_pos due to reading a torn or corrupted record
length (such as when the kernel overwrites unconsumed records in
BPF_F_RB_OVERWRITE mode), won't cons_pos bypass prod_pos?

Will the loop then spin endlessly, processing garbage memory as records
until cons_pos coincidentally aligns perfectly with prod_pos again?

>  			len_ptr = r->data + (cons_pos & r->mask);
>  			len = smp_load_acquire(len_ptr);
>  

While reviewing this 32-bit position wrap behavior, I noticed a similar
pre-existing issue in the kernel's USER_RINGBUF implementation.

[Severity: High]
This is a pre-existing issue, but does __bpf_user_ringbuf_peek() permanently
break maps on 32-bit architectures?

kernel/bpf/ringbuf.c:__bpf_user_ringbuf_peek() {
    ...
    u64 cons_pos, prod_pos;
    ...
    if (cons_pos >= prod_pos)
        return -ENODATA;
    ...
}

On 32-bit kernels, rb->producer_pos and rb->consumer_pos are 32-bit
variables read into 64-bit u64 variables. When producer_pos wraps past
32-bit, its zero-extended 64-bit value becomes small, while consumer_pos
remains large.

Does this absolute ordering check falsely evaluate to true, permanently
returning -ENODATA and stalling the map until user space pushes another 4GB
of data?

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