Re: [PATCH bpf 2/2] selftests/bpf: Test user ringbuf backlog larger than ring size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] selftests/bpf: backlog check breaks
  test_user_ringbuf_post_larger_than_ringbuf_sz()
--

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

selftests/bpf: Test user ringbuf backlog larger than ring size

This commit adds a selftest verifying that bpf_user_ringbuf_drain() returns
-EINVAL and drains no samples when producer_pos exceeds consumer_pos by more
than the ring buffer size.

> diff --git a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> index 9fd3ae9873210..b455e35d616b5 100644
> --- a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> @@ -280,6 +280,22 @@ static void test_user_ringbuf_post_larger_than_ringbuf_sz(void)
>  	user_ringbuf_success__destroy(skel);
>  }
>  
> +static void test_user_ringbuf_post_backlog_larger_than_ringbuf_sz(void)
> +{
> +	struct user_ringbuf_success *skel;
> +	struct user_ring_buffer *ringbuf;
> +	int err;
> +	__u32 size = sizeof(struct sample);
> +
> +	err = load_skel_create_user_ringbuf(&skel, &ringbuf);
> +	if (!ASSERT_OK(err, "oversized_backlog_skel"))
> +		return;
> +
> +	manually_write_test_invalid_sample(skel, size, c_ringbuf_size * 2, -EINVAL);
> +	user_ring_buffer__free(ringbuf);
> +	user_ringbuf_success__destroy(skel);
> +}

[Severity: Medium]
Does the backlog check tested here break the preceding
test_user_ringbuf_post_larger_than_ringbuf_sz() test?

test_user_ringbuf_post_larger_than_ringbuf_sz() passes size = c_ringbuf_size
to manually_write_test_invalid_sample(), which sets producer_pos to
c_ringbuf_size + BPF_RINGBUF_HDR_SZ and expects bpf_user_ringbuf_drain() to
return -E2BIG:

tools/testing/selftests/bpf/prog_tests/user_ringbuf.c:test_user_ringbuf_post_larger_than_ringbuf_sz() {
	...
	__u32 size = c_ringbuf_size;
	...
	manually_write_test_invalid_sample(skel, size, size, -E2BIG);
	...
}

With commit "bpf: Reject user ringbuf backlogs larger than total ring size",
__bpf_user_ringbuf_peek() rejects prod_pos - cons_pos >
ringbuf_total_data_sz(rb) with -EINVAL before reading the sample header:

kernel/bpf/ringbuf.c:__bpf_user_ringbuf_peek() {
	...
	if (prod_pos - cons_pos > ringbuf_total_data_sz(rb))
		return -EINVAL;
	...
	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 both enforced first, the total_len >
ringbuf_total_data_sz(rb) check returning -E2BIG is now unreachable, and
test_user_ringbuf_post_larger_than_ringbuf_sz() fails with -EINVAL instead of
-E2BIG.

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