Re: [PATCH v3 2/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The test attaches its BPF cgroup program directly to the root cgroup (`/sys/fs/cgroup`), lacking isolation and risking system-wide side-effects.
- [Medium] Unchecked `pthread_create` failure causes the test suite to hang indefinitely.
- [Medium] The test incorrectly validates the return values of libbpf attach APIs using `ASSERT_NE(..., NULL)`, which fails to detect error pointers (`ERR_PTR`).
- [Medium] The assertion checking the elapsed time against `timeo.tv_usec * 1000` is flaky due to timer granularity and jiffy alignment.
--

--- Patch [2]: [PATCH v3 2/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup ---
commit b46e4566e9f7fd3ddd12a149005b03389686104c
Author: Nnamdi Onyeyiri <nnamdio@gmail.com>

bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

This selftest exercises the tcp_bpf_recvmsg() and tcp_bpf_recvmsg_parser()
functions, to ensure that they are properly handling spurious wakeups in
tcp_msg_wait_data().

> diff --git a/tools/testing/selftests/net/sockmap_recvfrom.c b/tools/testing/selftests/net/sockmap_recvfrom.c
> new file mode 100644
> index 0000000000000..f96b4673c977c
> --- /dev/null
> +++ b/tools/testing/selftests/net/sockmap_recvfrom.c
[ ... ]
> +static void setup_bpf(struct __test_metadata *_metadata, const char *path, bool recv,
> +						struct bpf_t *bpf)
> +{
[ ... ]
> +	cgroup = open("/sys/fs/cgroup", O_RDONLY);
> +	ASSERT_NE(cgroup, -1);
> +
> +	bpf->on_sockops = bpf_program__attach_cgroup(prog, cgroup);

[Severity: Medium]
Is it safe to attach the BPF program directly to the global root cgroup here?

Since this is a test, attaching globally to /sys/fs/cgroup impacts the
entire system and subjects unrelated background connections to the test's
stream verdict logic. This could disrupt host networking and easily
overflow the 1024 max_entries of map_socks.

Should the test create and attach to an isolated test cgroup instead?

> +	close(cgroup);
> +
> +	ASSERT_NE(bpf->on_sockops, NULL);

[Severity: Medium]
Does ASSERT_NE() properly handle libbpf error pointers here?

Modern libbpf functions like bpf_program__attach_cgroup() return an ERR_PTR,
not NULL, on failure.

If the attachment fails, ASSERT_NE(link, NULL) evaluates to true, masking
the failure. The test will proceed without the BPF programs attached and
pass vacuously.

Would ASSERT_OK_PTR() be more appropriate for checking this return value?

> +
> +	if (recv) {
> +		struct bpf_map *map = bpf_object__find_map_by_name(bpf->obj, "map_socks");
> +
> +		ASSERT_NE(map, NULL);
> +
> +		prog = bpf_object__find_program_by_name(bpf->obj, "on_recv");
> +		ASSERT_NE(prog, NULL);
> +
> +		bpf->on_recv = bpf_program__attach_sockmap(prog, bpf_map__fd(map));
> +		ASSERT_NE(bpf->on_recv, NULL);

[Severity: Medium]
Will this assertion correctly catch a failure in bpf_program__attach_sockmap()?

Like bpf_program__attach_cgroup(), this function returns an ERR_PTR on
failure, so checking against NULL will allow error pointers to bypass
this check.

> +	}
> +}
[ ... ]
> +FIXTURE_SETUP(sockmap_recvfrom)
> +{
> +	memset(&self->addr, 0, sizeof(self->addr));
> +
> +	self->payload_len = 1024 * 1024 * 25;
> +	self->payload = malloc(self->payload_len);
> +	ASSERT_NE(self->payload, NULL);
> +
> +	memset(self->payload, 0, self->payload_len);
> +	self->payload[self->payload_len - 1] = PAYLOAD_END;
> +
> +	setup_bpf(_metadata, "sockmap_recvfrom.bpf.o", variant->with_recv, &self->bpf);
> +	atomic_store(&self->server.running, 1);
> +	self->server.fd = start_listening(_metadata, &self->addr.sin_port);
> +	self->server.metadata = _metadata;
> +
> +	self->addr.sin_family = AF_INET;
> +	self->addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> +
> +	pthread_create(&self->server.thread, NULL, &run_server, &self->server);

[Severity: Medium]
What happens if pthread_create() fails here?

If the server thread fails to start, it looks like the main thread will
subsequently call send_payload() with a 25MB payload. Because no server
thread is calling accept() or reading the data, the TCP receive window
will fill up and the main thread's write() will block forever.

Should we check the return value of pthread_create() to prevent the
test suite from hanging indefinitely on resource limits?

> +}
[ ... ]
> +TEST_F(sockmap_recvfrom, with_timeout)
> +{
[ ... ]
> +		ASSERT_EQ(setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo)), 0);
> +
> +		clock_gettime(CLOCK_MONOTONIC, &beg);
> +		ASSERT_EQ(recvfrom(fd, ignored, sizeof(ignored), 0, NULL, NULL), -1);
> +		err = errno;
> +		clock_gettime(CLOCK_MONOTONIC, &end);
> +
> +		ASSERT_EQ(err, EAGAIN);
> +		ASSERT_GE(to_nanos(&end) - to_nanos(&beg), timeo.tv_usec * 1000);

[Severity: Medium]
Is this strict time assertion susceptible to jiffy alignment flakiness?

The kernel handles SO_RCVTIMEO timeouts in units of jiffies. When
requesting a 5000us timeout on a system with HZ=1000, it converts to 5
ticks. Because the sleep can start just prior to a tick boundary, the
actual elapsed time can be slightly less than the absolute 5000us requested.

Could this cause intermittent test failures where the elapsed time is just
under the requested 5000000ns?

> +
> +		close(fd);
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714203927.32289-1-nnamdio@gmail.com?part=2