Re: [PATCH bpf-next v4 4/6] selftests/bpf: add icmp_send_unreach kfunc tests

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [Medium] Race condition leading to test flakiness due to missing `poll()` before `recvmsg(MSG_ERRQUEUE)`.
- [Medium] Hardcoded server port can cause test collisions during parallel execution.
- [Low] Improper use of `ASSERT_EQ` for flow control and swapped assertion tags in `read_icmp_errqueue`.
- [Low] Factually incorrect comment regarding `SERVER_IP` byte order.
--

commit 954da1124af043d103f7e4f4435358fccf26853c
Author: Mahe Tardy <mahe.tardy@gmail.com>

selftests/bpf: add icmp_send_unreach kfunc tests

This commit adds tests for the icmp_send_unreach kfunc. It sets up a client
and server, uses a cgroup egress program to trigger an ICMP unreachable
message, and reads the error queue to confirm the correct code is received.

> diff --git a/tools/testing/selftests/bpf/prog_tests/icmp_send_unreach_kfunc.c b/tools/testing/selftests/bpf/prog_tests/icmp_send_unreach_kfunc.c
> new file mode 100644
> index 0000000000000..24d5e01cfe804
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/icmp_send_unreach_kfunc.c
[ ... ]
> @@ -0,0 +1,136 @@
[ ... ]
> +#define TIMEOUT_MS 1000
> +#define SRV_PORT 54321

Can this cause intermittent bind() failures when selftests are run in
parallel? Since tests often execute concurrently, hardcoding a fixed port
like 54321 might collide with other tests or system processes. Could we
bind to port 0 to allow the OS to assign an ephemeral port, retrieve it via
getsockname(), and pass it to the BPF program dynamically?

[ ... ]
> +static void read_icmp_errqueue(int sockfd, int expected_code)
> +{
[ ... ]
> +	struct msghdr msg = {
> +		.msg_control = ctrl_buf,
> +		.msg_controllen = sizeof(ctrl_buf),
> +	};
> +
> +	n = recvmsg(sockfd, &msg, MSG_ERRQUEUE);

Could this read the error queue before the ICMP message has been processed?

The non-blocking connect() returns -EINPROGRESS, and the ICMP unreachable
message is generated by the BPF program and processed asynchronously by
softirqs. Under heavy load, if the softirq processing is deferred, could
recvmsg() execute before the error queue is populated and cause a spurious
-EAGAIN failure?

Would it be safer to use poll() or select() to wait for the POLLERR event
on the socket before attempting the read?

> +	if (!ASSERT_GE(n, 0, "recvmsg_errqueue"))
> +		return;
> +
> +	cm = CMSG_FIRSTHDR(&msg);
> +	if (!ASSERT_NEQ(cm, NULL, "cm_firsthdr_null"))
> +		return;
> +
> +	for (; cm; cm = CMSG_NXTHDR(&msg, cm)) {
> +		if (!ASSERT_EQ(cm->cmsg_level, IPPROTO_IP, "cmsg_type") ||
> +		    !ASSERT_EQ(cm->cmsg_type, IP_RECVERR, "cmsg_level"))
> +			continue;

Does using ASSERT_EQ here cause the test to fail immediately if an unexpected
but benign control message is encountered? In BPF selftests, if ASSERT_EQ
evaluates to false, it logs a failure and marks the test as failed instead
of just skipping the message.

Also, it looks like the tags might be swapped. "cmsg_type" is used to label
the cm->cmsg_level check, and "cmsg_level" is used for the cm->cmsg_type
check.

[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/icmp_send_unreach.c b/tools/testing/selftests/bpf/progs/icmp_send_unreach.c
> new file mode 100644
> index 0000000000000..6fc5595f08aa9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/icmp_send_unreach.c
[ ... ]
> @@ -0,0 +1,36 @@
[ ... ]
> +#define SERVER_PORT 54321
> +/* 127.0.0.1 in network byte order */
> +#define SERVER_IP 0x7F000001

This isn't a bug since the code correctly calls bpf_htonl(SERVER_IP) later,
but the comment appears to be incorrect. The value 0x7F000001 represents
127.0.0.1 in host byte order, not network byte order.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420105816.72168-1-mahe.tardy@gmail.com?part=4