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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Flaky test assertion due to jiffy-based timeout resolution.
- [Medium] Infinite busy-loop in server thread on fatal socket error.
--

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

bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

This commit adds a selftest to ensure tcp_bpf_recvmsg() and
tcp_bpf_recvmsg_parser() correctly handle spurious wakeups in
tcp_msg_wait_data(). It verifies that recvfrom() doesn't return EAGAIN without
a timeout and properly respects SO_RCVTIMEO when configured.

> diff --git a/tools/testing/selftests/net/sockmap_recvfrom.c b/tools/testing/selftests/net/sockmap_recvfrom.c
> new file mode 100644
> index 0000000000000..25657fc3dbcd2
> --- /dev/null
> +++ b/tools/testing/selftests/net/sockmap_recvfrom.c

[ ... ]

> +static void process_client(int fd, atomic_int *running, bool *failed)
> +{

[ ... ]

> +	while (atomic_load(running)) {
> +		ssize_t len = recvfrom(fd, buf, sizeof(buf), 0, NULL, NULL);
> +
> +		if (len == -1) {
> +			if (errno != EAGAIN && errno != EINTR)
> +				*failed = true;
> +			continue;

[Severity: Medium]
Will this result in an infinite busy-loop if a fatal socket error occurs?

If recvfrom() encounters a fatal error like ECONNRESET, *failed is set to true
but the loop continues. Since the socket remains in an error state, subsequent
calls to recvfrom() will immediately return -1 without blocking. 

This creates a tight busy-wait loop that starves the CPU until the global test
timeout intervenes. Should this break out of the loop on fatal errors instead?

> +		}
> +
> +		if (len <= 0 || buf[len - 1] == PAYLOAD_END)
> +			break;
> +	}

[ ... ]

> +TEST_F(sockmap_recvfrom, with_timeout)
> +{

[ ... ]

> +	for (int i = 0; i < MAX_ITERATIONS; ++i) {
> +		struct timespec beg;
> +		struct timespec end;
> +		int err;
> +
> +		int fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> +
> +		ASSERT_NE(fd, -1);
> +		ASSERT_EQ(connect(fd, (struct sockaddr *)&self->addr, sizeof(self->addr)), 0);
> +
> +		ASSERT_EQ(send_payload(_metadata, fd, self->payload, self->payload_len), 0);
> +
> +		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 it safe to assume strict microsecond precision for SO_RCVTIMEO here?

The test expects the blocked duration to be strictly greater than or equal to
5ms. However, in the kernel, socket timeouts are converted to jiffies via
sk_rcvtimeo.

On systems with HZ=100, 5ms translates to less than a jiffy. Even on HZ=1000,
waiting 5 jiffies can complete in slightly over 4ms depending on tick
alignment. 

Because the test demands strict microsecond accuracy from a jiffy-based timer,
could this cause intermittent test failures?

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

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