Re: [PATCH bpf v3 2/2] selftests/bpf: Test FIONREAD on a sockmap socket without a verdict program

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The macro `IO_TIMEOUT_SEC` (30) is passed as the `timeout_ms` argument to `wait_for_fionread()`, resulting in a very short 30 millisecond timeout instead of 30 seconds.
- [Low] Multi-line comment does not follow the strict BPF subsystem comment style.
- [Low] Unnecessary creation and cleanup of an unused socket pair (`c0` and `p0`).
--

--- Patch [2]: [PATCH bpf v3 2/2] selftests/bpf: Test FIONREAD on a sockmap socket without a verdict program ---
commit ef6ce74d985414c924fd029034cc51c133f6ad26
Author: Mattia Meleleo <mattia.meleleo@coralogix.com>

selftests/bpf: Test FIONREAD on a sockmap socket without a verdict program

Add a test validating that FIONREAD on a TCP socket in a sockmap
without a verdict program reports data pending in sk_receive_queue.

> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index cb3229711f93a..f0f368201ddad 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -1373,6 +1373,43 @@ static void test_sockmap_multi_channels(int sotype)
>  	test_sockmap_pass_prog__destroy(skel);
>  }
>  
> +/* A socket in a sockmap without a verdict program keeps its ingress data
> + * in sk_receive_queue: FIONREAD must account for it.
> + */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the strict BPF
subsystem style guidelines? It appears the opening /* is expected to be
on its own line.

> +static void test_sockmap_no_verdict_fionread(void)
> +{
> +	int err, map, zero = 0, sent, avail;
> +	int c0 = -1, c1 = -1, p0 = -1, p1 = -1;
> +	struct test_sockmap_pass_prog *skel;
> +	char buf[256] = "0123456789";
> +
> +	skel = test_sockmap_pass_prog__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> +		return;
> +	map = bpf_map__fd(skel->maps.sock_map_rx);
> +
> +	err = create_socket_pairs(AF_INET, SOCK_STREAM, &c0, &c1, &p0, &p1);

[Severity: Low]
Is there a reason to allocate four socket file descriptors here?

It looks like the c0 and p0 sockets are created but never used in the test.
Could create_pair() be used instead to avoid unnecessarily creating and
closing the unused socket pair?

> +	if (!ASSERT_OK(err, "create_socket_pairs()"))
> +		goto out;
> +
> +	err = bpf_map_update_elem(map, &zero, &c1, BPF_NOEXIST);
> +	if (!ASSERT_OK(err, "bpf_map_update_elem(c1)"))
> +		goto out_close;
> +
> +	sent = xsend(p1, &buf, sizeof(buf), 0);
> +	ASSERT_EQ(sent, sizeof(buf), "xsend(p1)");
> +	avail = wait_for_fionread(c1, sizeof(buf), IO_TIMEOUT_SEC);

[Severity: Medium]
Will this result in a flaky test on slow or heavily loaded CI systems?

Looking at the wait_for_fionread() implementation, the third parameter is
timeout_ms, which dictates the number of 1 millisecond sleep iterations:

	while (elapsed < timeout_ms) {
		if (ioctl(fd, FIONREAD, &avail) < 0)
			return -errno;
		if (avail >= expected)
			return avail;
		usleep(1000);
		elapsed++;
	}

By passing IO_TIMEOUT_SEC (which is typically 30), it seems the test will only
wait 30 milliseconds instead of 30 seconds for the packet to traverse the local
network stack.

> +	ASSERT_EQ(avail, sizeof(buf), "ioctl(FIONREAD)");
> +
> +out_close:
> +	close(c0);
> +	close(p0);
> +	close(c1);
> +	close(p1);
> +out:
> +	test_sockmap_pass_prog__destroy(skel);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-fionread-no-verdict-v3-0-b4ee31b3af53@coralogix.com?part=2