Re: [PATCH net-next v3 4/4] selftests/bpf: Ensure dst addr/port are preserved after socket abort

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] The test silently passes if the socket abort mechanism fails or if the target socket is not found, rendering it ineffective.
- [Medium] Uninitialized variable `out` is read and evaluated if `read()` returns an error.
- [Low] Passing a NULL pointer to the `%s` format specifier in `snprintf` causes undefined behavior.
--

commit 8c3285f5a0be84b220c0ac15975fdc9d109ad136
Author: Jordan Rife <jrife@google.com>

selftests/bpf: Ensure dst addr/port are preserved after socket abort

This commit adds a test to ensure that sock_release hooks see the original
dst_ip4, dst_ip6, and dst_port values for connected UDP and TCP sockets
following a socket abort.

diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy_release.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy_release.c
new file mode 100644
index 0000000000000..022031332b839
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy_release.c
[ ... ]
> +static void destroy(struct sock_destroy_release *skel, int fd, int sock_type)
> +{
[ ... ]
> +	/* Delete matching socket. */
> +	nread = read(iter_fd, &out, sizeof(out));
> +	ASSERT_GE(nread, 0, "nread");
> +	if (nread)
> +		ASSERT_EQ(out, cookie, "cookie matches");

If the iterator fails to find the target socket, read() returns 0. The test
tolerates this because ASSERT_GE(0, 0) passes and if (nread) simply skips
the ASSERT_EQ.

Since the test then proceeds to call close(connect_fd), which performs a
normal socket closure, won't the sock_release hook naturally see the intact
dst_ip and dst_port values, causing the test to silently pass without the
socket ever being aborted? Should this assert that nread == sizeof(out)?

Additionally, if read() fails and returns -1, the ASSERT_GE() macro logs a
test failure but does not return early.

Because -1 evaluates to true in C, could this lead to executing
ASSERT_EQ(out, cookie, ...) where out is an uninitialized stack variable?

[ ... ]
> +static void do_tests(struct sock_destroy_release *skel, int sock_type,
> +		     int family, const char * const *bind_addrs,
> +		     size_t bind_addrs_len, const int *bind_ports,
> +		     size_t bind_ports_len)
> +{
> +	const char *protocol_name = sock_type == SOCK_STREAM ? "tcp" : "udp";
> +	const char *family_name = family == AF_INET ? "ipv4" : "ipv6";
> +	char name[256];
> +
> +	for (size_t i = 0; i < bind_addrs_len; i++) {
> +		for (size_t j = 0; j < bind_ports_len; j++) {
> +			snprintf(name, sizeof(name), "%s/%s/destroy/%s:%d",
> +				 protocol_name, family_name, bind_addrs[i],
> +				 bind_ports[j]);

Looking at test_sock_destroy_release(), the bind4_addresses and
bind6_addresses arrays contain NULL as their first element.

Could passing a NULL pointer to the %s format specifier cause undefined
behavior here?

While some libc implementations like glibc gracefully handle this by
outputting (null), others might crash or trigger sanitizer warnings.
Would it be safer to explicitly guard the argument, such as checking for
NULL and substituting it with a placeholder string?

diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_release.c b/tools/testing/selftests/bpf/progs/sock_destroy_release.c
new file mode 100644
index 0000000000000..5389f79226f99
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/sock_destroy_release.c
[ ... ]
> +void maybe_abort(struct sock_common *sk, struct seq_file *seq)
> +{
> +	__u64 sock_cookie;
> +
> +	if (!sk)
> +		return;
> +
> +	sock_cookie = bpf_get_socket_cookie(sk);
> +	if (sock_cookie != abort_cookie)
> +		return;
> +
> +	bpf_sock_destroy(sk);
> +	bpf_seq_write(seq, &sock_cookie, sizeof(sock_cookie));
> +}

Is it possible for bpf_sock_destroy() to fail and return an error code?

If it does, the iterator will still return the cookie to userspace, which
will receive it and incorrectly assume the socket was aborted.

Would it make sense to only call bpf_seq_write() if bpf_sock_destroy()
succeeds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260330215707.2374657-1-jrife@google.com?part=4