Re: [PATCH bpf 1/2] bpf: Reject user ringbuf backlogs larger than total ring size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpf: early backlog check in __bpf_user_ringbuf_peek() breaks
  -E2BIG return
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] bpf: compat mode ring buffer hang in __bpf_user_ringbuf_peek()
  [1]
--

commit c14bce3c5f895ee2ea691c51e1942bac34af6108
Author: Luxiao Xu <rakukuip@gmail.com>

bpf: Reject user ringbuf backlogs larger than total ring size

This commit rejects user ring buffer drain operations with -EINVAL when
the unread window between producer_pos and consumer_pos exceeds the total
data size of the ring buffer.

> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index 447863ece26a3..cfd4fe4e85ee6 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -761,6 +761,9 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
>  	if (cons_pos >= prod_pos)
>  		return -ENODATA;

[Severity: High]
This problem wasn't introduced by this patch, but can cons_pos >= prod_pos
permanently stall sample processing for a 32-bit compat user-space
producer on a 64-bit kernel [1]?

A 32-bit compat application updates rb->producer_pos with a 32-bit store
that wraps to zero at 4 GB, while the 64-bit kernel increments
rb->consumer_pos past 4 GB using 64-bit arithmetic.  Once producer_pos
wraps, cons_pos >= prod_pos evaluates to true and __bpf_user_ringbuf_peek()
returns -ENODATA on every subsequent drain.

>  
> +	if (prod_pos - cons_pos > ringbuf_total_data_sz(rb))
> +		return -EINVAL;

[Severity: Medium]
Does checking prod_pos - cons_pos > ringbuf_total_data_sz(rb) here break
the -E2BIG return for a single oversized sample?

Later in __bpf_user_ringbuf_peek(), total_len is checked against both
prod_pos - cons_pos and ringbuf_total_data_sz(rb):

kernel/bpf/ringbuf.c:__bpf_user_ringbuf_peek() {
	...
	/* The sample must fit within the region advertised by the producer position. */
	if (total_len > prod_pos - cons_pos)
		return -EINVAL;

	/* The sample must fit within the data region of the ring buffer. */
	if (total_len > ringbuf_total_data_sz(rb))
		return -E2BIG;
	...
}

Because prod_pos - cons_pos <= ringbuf_total_data_sz(rb) and
total_len <= prod_pos - cons_pos are already enforced before reaching
total_len > ringbuf_total_data_sz(rb), the -E2BIG branch becomes
unreachable.  When user space publishes a single sample larger than the
ring buffer (setting producer_pos to at least ringbuf_total_data_sz(rb) +
BPF_RINGBUF_HDR_SZ), bpf_user_ringbuf_drain() now returns -EINVAL instead
of the -E2BIG documented in include/uapi/linux/bpf.h and expected by
test_user_ringbuf_post_larger_than_ringbuf_sz() in
tools/testing/selftests/bpf/prog_tests/user_ringbuf.c.

[1] https://sashiko.dev/#/bug/linux-262d24f9-bac4-4511-aaed-6698163eb6f5

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793951856.git.rakukuip@gmail.com?part=1