Re: [net-next,v1,2/2] selftests/bpf: Ensure dst addr/port are preserved after socket abort

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
selftests/bpf: Ensure dst addr/port are preserved after socket abort

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 000000000000..9bd2be4d45a2
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy_release.c
> @@ -0,0 +1,135 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "network_helpers.h"
> +#include "sock_destroy_release.skel.h"
> +
> +#define TEST_NS "sock_destroy_release_netns"
> +
> +static __u64 socket_cookie(int fd)
> +{
> +	__u64 cookie;
> +	socklen_t cookie_len = sizeof(cookie);
> +
> +	if (!ASSERT_OK(getsockopt(fd, SOL_SOCKET, SO_COOKIE, &cookie,
> +				  &cookie_len), "getsockopt(SO_COOKIE)"))
> +		return 0;
> +	return cookie;
> +}
> +
> +static void destroy(struct sock_destroy_release *skel, int fd, int sock_type)
> +{
> +	__u64 cookie = socket_cookie(fd);
> +	struct bpf_link *link = NULL;
> +	int iter_fd = -1;
> +	int nread;
> +	__u64 out;
> +
> +	skel->bss->abort_cookie = cookie;
> +
> +	link = bpf_program__attach_iter(sock_type == SOCK_STREAM ?
> +					skel->progs.abort_tcp :
> +					skel->progs.abort_udp, NULL);
> +	if (!ASSERT_OK_PTR(link, "bpf_program__attach_iter"))
> +		goto done;
> +
> +	iter_fd = bpf_iter_create(bpf_link__fd(link));
> +	if (!ASSERT_OK_FD(iter_fd, "bpf_iter_create"))
> +		goto done;
> +
> +	/* Delete matching socket. */
> +	nread = read(iter_fd, &out, sizeof(out));
> +	ASSERT_GE(nread, 0, "nread");
> +	if (nread)
> +		ASSERT_EQ(out, cookie, "cookie matches");
> +done:
> +	if (iter_fd >= 0)
> +		close(iter_fd);
> +	bpf_link__destroy(link);
> +	close(fd);
> +}
> +
> +static void do_test(struct sock_destroy_release *skel, int sock_type,
> +		    int family)
> +{
> +	const char *addr = family == AF_INET ? "127.0.0.1" : "::1";
> +	int listen_fd = -1, connect_fd = -1;
> +	static const int port = 10001;
> +
> +	listen_fd = start_server(family, sock_type, addr, port, 0);
> +	if (!ASSERT_OK_FD(listen_fd, "start_server"))
> +		goto cleanup;
> +
> +	connect_fd = connect_to_fd(listen_fd, 0);
> +	if (!ASSERT_OK_FD(connect_fd, "connect_to_fd"))
> +		goto cleanup;
> +
> +	memset(&skel->bss->sk, 0, sizeof(skel->bss->sk));
> +	destroy(skel, connect_fd, sock_type);

Does this cause a double close of connect_fd?

The destroy() function closes the fd parameter at line 49:

    close(fd);

But do_test() doesn't update its local connect_fd variable after calling
destroy().

Later in the cleanup code:

> +cleanup:
> +	if (connect_fd >= 0)
> +		close(connect_fd);

connect_fd still holds the closed value and gets closed again.

While this usually results in a benign EBADF error, there's a small window
where another operation could create a new fd with the same number, and the
second close would incorrectly close the reused fd.

Should connect_fd be set to -1 after calling destroy() to prevent the double
close?

[ ... ]

> 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 000000000000..add322ab1303
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/sock_destroy_release.c
> @@ -0,0 +1,59 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_core_read.h>
> +#include <bpf/bpf_endian.h>
> +#include "bpf_kfuncs.h"
> +
> +volatile __u64 abort_cookie;
> +
> +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));
> +}
> +
> +SEC("iter/udp")
> +int abort_udp(struct bpf_iter__udp *ctx)
> +{
> +	maybe_abort((struct sock_common *)ctx->udp_sk,
> +		    ctx->meta->seq);
> +
> +	return 0;
> +}
> +
> +SEC("iter/tcp")
> +int abort_tcp(struct bpf_iter__tcp *ctx)
> +{
> +	maybe_abort((struct sock_common *)ctx->sk_common,
> +		    ctx->meta->seq);
> +
> +	return 0;
> +}
> +
> +struct bpf_sock sk = {};
> +
> +SEC("cgroup/sock_release")
> +int sock_release(struct bpf_sock *ctx)
> +{
> +	sk.dst_ip4 = ctx->dst_ip4;
> +	sk.dst_ip6[0] = ctx->dst_ip6[0];
> +	sk.dst_ip6[1] = ctx->dst_ip6[1];
> +	sk.dst_ip6[2] = ctx->dst_ip6[2];
> +	sk.dst_ip6[3] = ctx->dst_ip6[3];
> +	sk.dst_port = ctx->dst_port;
> +
> +	return 1;
> +}
> +
> +char _license[] SEC("license") = "GPL";
-- 
pw-bot: cr