Re: [PATCH bpf-next v3 5/5] selftests/bpf: cover stream capacity and partial read edge cases

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `test_stream_oversize` test fails to verify its stated goal because it uses a completely different BPF program to verify the capacity rollback.
--

--- Patch [5]: [PATCH bpf-next v3 5/5] selftests/bpf: cover stream capacity and partial read edge cases ---
commit 168aa19f7a74d7d1bc5cd6a9913bc5c68df1058d
Author: Jianlin Shi <[email protected]>

selftests/bpf: cover stream capacity and partial read edge cases

This commit adds test coverage for stream capacity rollback after oversized
writes and partial reads when buffers straddle unmapped pages.

> diff --git a/tools/testing/selftests/bpf/prog_tests/stream.c b/tools/testing/selftests/bpf/prog_tests/stream.c
> index e4e9374309e26..fefe6431dc7c9 100644
> --- a/tools/testing/selftests/bpf/prog_tests/stream.c
> +++ b/tools/testing/selftests/bpf/prog_tests/stream.c
[ ... ]
> @@ -58,6 +58,76 @@ void test_stream_syscall(void)
>  	stream__destroy(skel);
>  }
>  
> +void test_stream_oversize(void)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, opts);
> +	struct stream *skel;
> +	int ret, prog_fd;
> +	char buf[8] = {};
> +
> +	skel = stream__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "stream__open_and_load"))
> +		return;
> +
> +	prog_fd = bpf_program__fd(skel->progs.stream_oversize);
> +	ret = bpf_prog_test_run_opts(prog_fd, &opts);
> +	ASSERT_OK(ret, "oversize run");
> +	ASSERT_EQ(opts.retval, -E2BIG, "oversize retval");
> +
> +	/* Oversized push must not permanently consume capacity. */
> +	prog_fd = bpf_program__fd(skel->progs.stream_syscall);

[Severity: Medium]
Does this code verify the capacity rollback correctly?

Because BPF streams are maintained on a per-program basis, switching to
the stream_syscall program means it tests a completely different, pristine
stream that was never affected by the oversized push.

Should the test verify a subsequent successful push using the same program
that performed the oversized push?

> +	ret = bpf_prog_test_run_opts(prog_fd, &opts);
> +	ASSERT_OK(ret, "syscall run");
> +	ASSERT_OK(opts.retval, "syscall retval");
> +
> +	ret = bpf_prog_stream_read(prog_fd, BPF_STREAM_STDOUT, buf, sizeof(buf), NULL);
> +	ASSERT_EQ(ret, 3, "bytes after oversize");
> +	ASSERT_OK(memcmp(buf, "foo", 3), "payload after oversize");
> +
> +	stream__destroy(skel);
> +}
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/stream.c b/tools/testing/selftests/bpf/progs/stream.c
> index 00a37933e411d..6aa9a67483500 100644
> --- a/tools/testing/selftests/bpf/progs/stream.c
> +++ b/tools/testing/selftests/bpf/progs/stream.c
[ ... ]
> @@ -118,6 +123,13 @@ int stream_syscall(void *ctx)
>  	return 0;
>  }
>  
> +SEC("syscall")
> +__success __retval(-E2BIG)
> +int stream_oversize(void *ctx)
> +{
> +	return bpf_stream_printk(BPF_STDOUT, _BIG_STR);
> +}

[Severity: Medium]
Since this program returns immediately after the oversized push, it leaves
no way to verify the capacity rollback on the same stream from userspace.

Could this program be updated to perform a subsequent successful push to the
same stream, to correctly validate that the capacity was not permanently
consumed?

> +
>  SEC("syscall")
>  __arch_x86_64
>  __arch_arm64

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