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

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

Pre-existing issues:
- [High] Missing wrap-around handling in BPF_MAP_TYPE_USER_RINGBUF kernel consumer
- [High] Missing wrap-around handling in libbpf ringbuffer consumer
- [Medium] Incorrect wrap-around handling and memory barrier usage in `ringbuf_avail_data_sz`
--

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

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

The reservation path caches the position of the oldest not-yet-committed
record in rb->pending_pos and advances it past already committed records
on every reservation. Consumer, producer, and pending positions are 32-bit on
32-bit architectures and can wrap around, which causes an ordered comparison
to incorrectly stall reservations permanently. This patch changes the
ordered comparison to an unsigned difference, which stays correct across the
wrap-around.

> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index 35ae64ade36b8..06d3cc192601f 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 (prod_pos - pend_pos > 0) {

[Severity: High]
This isn't a bug introduced by this patch, but since you are fixing the
32-bit wrap around issues, does this code in __bpf_user_ringbuf_peek() also
have the same problem?

kernel/bpf/ringbuf.c:__bpf_user_ringbuf_peek() {
    ...
    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 cons_pos and prod_pos are read from 32-bit unsigned longs into 64-bit
u64 variables, will the implicit zero-extension cause cons_pos >= prod_pos
to incorrectly evaluate to true when prod_pos wraps around 4GB?

Also, wouldn't the subtraction total_len > prod_pos - cons_pos underflow to a
massive positive value on 32-bit architecture wrap-around, bypassing the size
check?


[Severity: Medium]
This is a pre-existing issue, but is the wrap-around handling in
ringbuf_avail_data_sz() also affected?

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);
    }
    ...
}

Could the max() macro unsafely select the logically older cons_pos over
over_pos during a 32-bit wrap-around, because max() uses an ordered comparison
rather than modular arithmetic?

Additionally, is it safe to read overwrite_pos before producer_pos here,
or does this break the acquire-release pairing and allow reading a stale
overwrite_pos relative to producer_pos?


[Severity: High]
This is a pre-existing issue, but does the libbpf consumer loop have a similar
wrap-around problem on 32-bit architectures?

tools/lib/bpf/ringbuf.c:ringbuf_process_ring() {
    ...
    cons_pos = smp_load_acquire(r->consumer_pos);
    do {
        got_new_data = false;
        prod_pos = smp_load_acquire(r->producer_pos);
        while (cons_pos < prod_pos) {
    ...
}

If producer_pos wraps around the 32-bit boundary and becomes smaller than
consumer_pos, will the ordered loop condition cons_pos < prod_pos become false
and permanently stall the libbpf consumer?

>  		hdr = (void *)rb->data + (pend_pos & rb->mask);
>  		hdr_len = READ_ONCE(hdr->len);
>  		if (hdr_len & BPF_RINGBUF_BUSY_BIT)

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