[v4,bpf-next,4/4] selftests/bpf: Add tests for bpf_sock_destroy

Commit Message

Aditi Ghag March 23, 2023, 8:06 p.m. UTC

The test cases for destroying sockets mirror the intended usages of the
bpf_sock_destroy kfunc using iterators.

The destroy helpers set `ECONNABORTED` error code that we can validate in
the test code with client sockets. But UDP sockets have an overriding error
code from the disconnect called during abort, so the error code the
validation is only done for TCP sockets.

Signed-off-by: Aditi Ghag <[e-mail]>
---
 .../selftests/bpf/prog_tests/sock_destroy.c   | 195 ++++++++++++++++++
 .../selftests/bpf/progs/sock_destroy_prog.c   | 151 ++++++++++++++
 2 files changed, 346 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c
 create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c

Comments

Stanislav Fomichev March 24, 2023, 9:52 p.m. UTC | #1

On 03/23, Aditi Ghag wrote:
> The test cases for destroying sockets mirror the intended usages of the
> bpf_sock_destroy kfunc using iterators.

> The destroy helpers set `ECONNABORTED` error code that we can validate in
> the test code with client sockets. But UDP sockets have an overriding  
> error
> code from the disconnect called during abort, so the error code the
> validation is only done for TCP sockets.

> Signed-off-by: Aditi Ghag <[e-mail]>
> ---
>   .../selftests/bpf/prog_tests/sock_destroy.c   | 195 ++++++++++++++++++
>   .../selftests/bpf/progs/sock_destroy_prog.c   | 151 ++++++++++++++
>   2 files changed, 346 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>   create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c

> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c  
> b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> new file mode 100644
> index 000000000000..cbce966af568
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +
> +#include "sock_destroy_prog.skel.h"
> +#include "network_helpers.h"
> +
> +#define SERVER_PORT 6062
> +
> +static void start_iter_sockets(struct bpf_program *prog)
> +{
> +	struct bpf_link *link;
> +	char buf[50] = {};
> +	int iter_fd, len;
> +
> +	link = bpf_program__attach_iter(prog, NULL);
> +	if (!ASSERT_OK_PTR(link, "attach_iter"))
> +		return;
> +
> +	iter_fd = bpf_iter_create(bpf_link__fd(link));
> +	if (!ASSERT_GE(iter_fd, 0, "create_iter"))
> +		goto free_link;
> +
> +	while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
> +		;
> +	ASSERT_GE(len, 0, "read");
> +
> +	close(iter_fd);
> +
> +free_link:
> +	bpf_link__destroy(link);
> +}
> +
> +static void test_tcp_client(struct sock_destroy_prog *skel)
> +{
> +	int serv = -1, clien = -1, n = 0;
> +
> +	serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
> +	if (!ASSERT_GE(serv, 0, "start_server"))
> +		goto cleanup_serv;
> +
> +	clien = connect_to_fd(serv, 0);
> +	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> +		goto cleanup_serv;
> +
> +	serv = accept(serv, NULL, NULL);
> +	if (!ASSERT_GE(serv, 0, "serv accept"))
> +		goto cleanup;
> +
> +	n = send(clien, "t", 1, 0);
> +	if (!ASSERT_GE(n, 0, "client send"))
> +		goto cleanup;
> +
> +	/* Run iterator program that destroys connected client sockets. */
> +	start_iter_sockets(skel->progs.iter_tcp6_client);
> +
> +	n = send(clien, "t", 1, 0);
> +	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> +		goto cleanup;
> +	ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket");
> +
> +
> +cleanup:
> +	close(clien);
> +cleanup_serv:
> +	close(serv);
> +}
> +
> +static void test_tcp_server(struct sock_destroy_prog *skel)
> +{
> +	int serv = -1, clien = -1, n = 0;
> +
> +	serv = start_server(AF_INET6, SOCK_STREAM, NULL, SERVER_PORT, 0);
> +	if (!ASSERT_GE(serv, 0, "start_server"))
> +		goto cleanup_serv;
> +
> +	clien = connect_to_fd(serv, 0);
> +	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> +		goto cleanup_serv;
> +
> +	serv = accept(serv, NULL, NULL);
> +	if (!ASSERT_GE(serv, 0, "serv accept"))
> +		goto cleanup;
> +
> +	n = send(clien, "t", 1, 0);
> +	if (!ASSERT_GE(n, 0, "client send"))
> +		goto cleanup;
> +
> +	/* Run iterator program that destroys server sockets. */
> +	start_iter_sockets(skel->progs.iter_tcp6_server);
> +
> +	n = send(clien, "t", 1, 0);
> +	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> +		goto cleanup;
> +	ASSERT_EQ(errno, ECONNRESET, "error code on destroyed socket");
> +
> +
> +cleanup:
> +	close(clien);
> +cleanup_serv:
> +	close(serv);
> +}
> +
> +
> +static void test_udp_client(struct sock_destroy_prog *skel)
> +{
> +	int serv = -1, clien = -1, n = 0;
> +
> +	serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0);
> +	if (!ASSERT_GE(serv, 0, "start_server"))
> +		goto cleanup_serv;
> +
> +	clien = connect_to_fd(serv, 0);
> +	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> +		goto cleanup_serv;
> +
> +	n = send(clien, "t", 1, 0);
> +	if (!ASSERT_GE(n, 0, "client send"))
> +		goto cleanup;
> +
> +	/* Run iterator program that destroys sockets. */
> +	start_iter_sockets(skel->progs.iter_udp6_client);
> +
> +	n = send(clien, "t", 1, 0);
> +	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> +		goto cleanup;
> +	/* UDP sockets have an overriding error code after they are  
> disconnected,
> +	 * so we don't check for ECONNABORTED error code.
> +	 */
> +
> +cleanup:
> +	close(clien);
> +cleanup_serv:
> +	close(serv);
> +}
> +
> +static void test_udp_server(struct sock_destroy_prog *skel)
> +{
> +	int *listen_fds = NULL, n, i;
> +	unsigned int num_listens = 5;
> +	char buf[1];
> +
> +	/* Start reuseport servers. */
> +	listen_fds = start_reuseport_server(AF_INET6, SOCK_DGRAM,
> +					    "::1", SERVER_PORT, 0,
> +					    num_listens);
> +	if (!ASSERT_OK_PTR(listen_fds, "start_reuseport_server"))
> +		goto cleanup;
> +
> +	/* Run iterator program that destroys server sockets. */
> +	start_iter_sockets(skel->progs.iter_udp6_server);
> +
> +	for (i = 0; i < num_listens; ++i) {
> +		n = read(listen_fds[i], buf, sizeof(buf));
> +		if (!ASSERT_EQ(n, -1, "read") ||
> +		    !ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket"))
> +			break;
> +	}
> +	ASSERT_EQ(i, num_listens, "server socket");
> +
> +cleanup:
> +	free_fds(listen_fds, num_listens);
> +}
> +
> +void test_sock_destroy(void)
> +{
> +	int cgroup_fd = 0;
> +	struct sock_destroy_prog *skel;
> +
> +	skel = sock_destroy_prog__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "skel_open"))
> +		return;
> +
> +	cgroup_fd = test__join_cgroup("/sock_destroy");
> +	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
> +		goto close_cgroup_fd;
> +
> +	skel->links.sock_connect = bpf_program__attach_cgroup(
> +		skel->progs.sock_connect, cgroup_fd);
> +	if (!ASSERT_OK_PTR(skel->links.sock_connect, "prog_attach"))
> +		goto close_cgroup_fd;
> +
> +	if (test__start_subtest("tcp_client"))
> +		test_tcp_client(skel);
> +	if (test__start_subtest("tcp_server"))
> +		test_tcp_server(skel);
> +	if (test__start_subtest("udp_client"))
> +		test_udp_client(skel);
> +	if (test__start_subtest("udp_server"))
> +		test_udp_server(skel);
> +
> +
> +close_cgroup_fd:
> +	close(cgroup_fd);
> +	sock_destroy_prog__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c  
> b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
> new file mode 100644
> index 000000000000..8e09d82c50f3
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
> @@ -0,0 +1,151 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "vmlinux.h"
> +
> +#include "bpf_tracing_net.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_endian.h>
> +
> +#define AF_INET6 10

[..]

> +/* Keep it in sync with prog_test/sock_destroy. */
> +#define SERVER_PORT 6062

The test looks good, one optional unrelated nit maybe:

I've been guilty of these hard-coded ports in the past, but maybe
we should stop hard-coding them? Getting the address of the listener (bound  
to
port 0) and passing it to the bpf program via global variable should be  
super
easy now (with the skeletons and network_helpers).

And, unrelated, maybe also fix a bunch of places where the reverse christmas
tree doesn't look reverse anymore?

> +
> +int bpf_sock_destroy(struct sock_common *sk) __ksym;
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_ARRAY);
> +	__uint(max_entries, 1);
> +	__type(key, __u32);
> +	__type(value, __u64);
> +} tcp_conn_sockets SEC(".maps");
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_ARRAY);
> +	__uint(max_entries, 1);
> +	__type(key, __u32);
> +	__type(value, __u64);
> +} udp_conn_sockets SEC(".maps");
> +
> +SEC("cgroup/connect6")
> +int sock_connect(struct bpf_sock_addr *ctx)
> +{
> +	int key = 0;
> +	__u64 sock_cookie = 0;
> +	__u32 keyc = 0;
> +
> +	if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
> +		return 1;
> +
> +	sock_cookie = bpf_get_socket_cookie(ctx);
> +	if (ctx->protocol == IPPROTO_TCP)
> +		bpf_map_update_elem(&tcp_conn_sockets, &key, &sock_cookie, 0);
> +	else if (ctx->protocol == IPPROTO_UDP)
> +		bpf_map_update_elem(&udp_conn_sockets, &keyc, &sock_cookie, 0);
> +	else
> +		return 1;
> +
> +	return 1;
> +}
> +
> +SEC("iter/tcp")
> +int iter_tcp6_client(struct bpf_iter__tcp *ctx)
> +{
> +	struct sock_common *sk_common = ctx->sk_common;
> +	struct seq_file *seq = ctx->meta->seq;
> +	__u64 sock_cookie = 0;
> +	__u64 *val;
> +	int key = 0;
> +
> +	if (!sk_common)
> +		return 0;
> +
> +	if (sk_common->skc_family != AF_INET6)
> +		return 0;
> +
> +	sock_cookie  = bpf_get_socket_cookie(sk_common);
> +	val = bpf_map_lookup_elem(&tcp_conn_sockets, &key);
> +	if (!val)
> +		return 0;
> +	/* Destroy connected client sockets. */
> +	if (sock_cookie == *val)
> +		bpf_sock_destroy(sk_common);
> +
> +	return 0;
> +}
> +
> +SEC("iter/tcp")
> +int iter_tcp6_server(struct bpf_iter__tcp *ctx)
> +{
> +	struct sock_common *sk_common = ctx->sk_common;
> +	struct seq_file *seq = ctx->meta->seq;
> +	struct tcp6_sock *tcp_sk;
> +	const struct inet_connection_sock *icsk;
> +	const struct inet_sock *inet;
> +	__u16 srcp;
> +
> +	if (!sk_common)
> +		return 0;
> +
> +	if (sk_common->skc_family != AF_INET6)
> +		return 0;
> +
> +	tcp_sk = bpf_skc_to_tcp6_sock(sk_common);
> +	if (!tcp_sk)
> +		return 0;
> +
> +	icsk = &tcp_sk->tcp.inet_conn;
> +	inet = &icsk->icsk_inet;
> +	srcp = bpf_ntohs(inet->inet_sport);
> +
> +	/* Destroy server sockets. */
> +	if (srcp == SERVER_PORT)
> +		bpf_sock_destroy(sk_common);
> +
> +	return 0;
> +}
> +
> +
> +SEC("iter/udp")
> +int iter_udp6_client(struct bpf_iter__udp *ctx)
> +{
> +	struct seq_file *seq = ctx->meta->seq;
> +	struct udp_sock *udp_sk = ctx->udp_sk;
> +	struct sock *sk = (struct sock *) udp_sk;
> +	__u64 sock_cookie = 0, *val;
> +	int key = 0;
> +
> +	if (!sk)
> +		return 0;
> +
> +	sock_cookie  = bpf_get_socket_cookie(sk);
> +	val = bpf_map_lookup_elem(&udp_conn_sockets, &key);
> +	if (!val)
> +		return 0;
> +	/* Destroy connected client sockets. */
> +	if (sock_cookie == *val)
> +		bpf_sock_destroy((struct sock_common *)sk);
> +
> +	return 0;
> +}
> +
> +SEC("iter/udp")
> +int iter_udp6_server(struct bpf_iter__udp *ctx)
> +{
> +	struct seq_file *seq = ctx->meta->seq;
> +	struct udp_sock *udp_sk = ctx->udp_sk;
> +	struct sock *sk = (struct sock *) udp_sk;
> +	__u16 srcp;
> +	struct inet_sock *inet;
> +
> +	if (!sk)
> +		return 0;
> +
> +	inet = &udp_sk->inet;
> +	srcp = bpf_ntohs(inet->inet_sport);
> +	if (srcp == SERVER_PORT)
> +		bpf_sock_destroy((struct sock_common *)sk);
> +
> +	return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.34.1

Aditi Ghag March 27, 2023, 3:57 p.m. UTC | #2

> On Mar 24, 2023, at 2:52 PM, Stanislav Fomichev <[e-mail]> wrote:
> 
> On 03/23, Aditi Ghag wrote:
>> The test cases for destroying sockets mirror the intended usages of the
>> bpf_sock_destroy kfunc using iterators.
> 
>> The destroy helpers set `ECONNABORTED` error code that we can validate in
>> the test code with client sockets. But UDP sockets have an overriding error
>> code from the disconnect called during abort, so the error code the
>> validation is only done for TCP sockets.
> 
>> Signed-off-by: Aditi Ghag <[e-mail]>
>> ---
>>  .../selftests/bpf/prog_tests/sock_destroy.c   | 195 ++++++++++++++++++
>>  .../selftests/bpf/progs/sock_destroy_prog.c   | 151 ++++++++++++++
>>  2 files changed, 346 insertions(+)
>>  create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>>  create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c
> 
>> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>> new file mode 100644
>> index 000000000000..cbce966af568
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>> @@ -0,0 +1,195 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#include <test_progs.h>
>> +
>> +#include "sock_destroy_prog.skel.h"
>> +#include "network_helpers.h"
>> +
>> +#define SERVER_PORT 6062
>> +
>> +static void start_iter_sockets(struct bpf_program *prog)
>> +{
>> +	struct bpf_link *link;
>> +	char buf[50] = {};
>> +	int iter_fd, len;
>> +
>> +	link = bpf_program__attach_iter(prog, NULL);
>> +	if (!ASSERT_OK_PTR(link, "attach_iter"))
>> +		return;
>> +
>> +	iter_fd = bpf_iter_create(bpf_link__fd(link));
>> +	if (!ASSERT_GE(iter_fd, 0, "create_iter"))
>> +		goto free_link;
>> +
>> +	while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
>> +		;
>> +	ASSERT_GE(len, 0, "read");
>> +
>> +	close(iter_fd);
>> +
>> +free_link:
>> +	bpf_link__destroy(link);
>> +}
>> +
>> +static void test_tcp_client(struct sock_destroy_prog *skel)
>> +{
>> +	int serv = -1, clien = -1, n = 0;
>> +
>> +	serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
>> +	if (!ASSERT_GE(serv, 0, "start_server"))
>> +		goto cleanup_serv;
>> +
>> +	clien = connect_to_fd(serv, 0);
>> +	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
>> +		goto cleanup_serv;
>> +
>> +	serv = accept(serv, NULL, NULL);
>> +	if (!ASSERT_GE(serv, 0, "serv accept"))
>> +		goto cleanup;
>> +
>> +	n = send(clien, "t", 1, 0);
>> +	if (!ASSERT_GE(n, 0, "client send"))
>> +		goto cleanup;
>> +
>> +	/* Run iterator program that destroys connected client sockets. */
>> +	start_iter_sockets(skel->progs.iter_tcp6_client);
>> +
>> +	n = send(clien, "t", 1, 0);
>> +	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
>> +		goto cleanup;
>> +	ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket");
>> +
>> +
>> +cleanup:
>> +	close(clien);
>> +cleanup_serv:
>> +	close(serv);
>> +}
>> +
>> +static void test_tcp_server(struct sock_destroy_prog *skel)
>> +{
>> +	int serv = -1, clien = -1, n = 0;
>> +
>> +	serv = start_server(AF_INET6, SOCK_STREAM, NULL, SERVER_PORT, 0);
>> +	if (!ASSERT_GE(serv, 0, "start_server"))
>> +		goto cleanup_serv;
>> +
>> +	clien = connect_to_fd(serv, 0);
>> +	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
>> +		goto cleanup_serv;
>> +
>> +	serv = accept(serv, NULL, NULL);
>> +	if (!ASSERT_GE(serv, 0, "serv accept"))
>> +		goto cleanup;
>> +
>> +	n = send(clien, "t", 1, 0);
>> +	if (!ASSERT_GE(n, 0, "client send"))
>> +		goto cleanup;
>> +
>> +	/* Run iterator program that destroys server sockets. */
>> +	start_iter_sockets(skel->progs.iter_tcp6_server);
>> +
>> +	n = send(clien, "t", 1, 0);
>> +	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
>> +		goto cleanup;
>> +	ASSERT_EQ(errno, ECONNRESET, "error code on destroyed socket");
>> +
>> +
>> +cleanup:
>> +	close(clien);
>> +cleanup_serv:
>> +	close(serv);
>> +}
>> +
>> +
>> +static void test_udp_client(struct sock_destroy_prog *skel)
>> +{
>> +	int serv = -1, clien = -1, n = 0;
>> +
>> +	serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0);
>> +	if (!ASSERT_GE(serv, 0, "start_server"))
>> +		goto cleanup_serv;
>> +
>> +	clien = connect_to_fd(serv, 0);
>> +	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
>> +		goto cleanup_serv;
>> +
>> +	n = send(clien, "t", 1, 0);
>> +	if (!ASSERT_GE(n, 0, "client send"))
>> +		goto cleanup;
>> +
>> +	/* Run iterator program that destroys sockets. */
>> +	start_iter_sockets(skel->progs.iter_udp6_client);
>> +
>> +	n = send(clien, "t", 1, 0);
>> +	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
>> +		goto cleanup;
>> +	/* UDP sockets have an overriding error code after they are disconnected,
>> +	 * so we don't check for ECONNABORTED error code.
>> +	 */
>> +
>> +cleanup:
>> +	close(clien);
>> +cleanup_serv:
>> +	close(serv);
>> +}
>> +
>> +static void test_udp_server(struct sock_destroy_prog *skel)
>> +{
>> +	int *listen_fds = NULL, n, i;
>> +	unsigned int num_listens = 5;
>> +	char buf[1];
>> +
>> +	/* Start reuseport servers. */
>> +	listen_fds = start_reuseport_server(AF_INET6, SOCK_DGRAM,
>> +					    "::1", SERVER_PORT, 0,
>> +					    num_listens);
>> +	if (!ASSERT_OK_PTR(listen_fds, "start_reuseport_server"))
>> +		goto cleanup;
>> +
>> +	/* Run iterator program that destroys server sockets. */
>> +	start_iter_sockets(skel->progs.iter_udp6_server);
>> +
>> +	for (i = 0; i < num_listens; ++i) {
>> +		n = read(listen_fds[i], buf, sizeof(buf));
>> +		if (!ASSERT_EQ(n, -1, "read") ||
>> +		    !ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket"))
>> +			break;
>> +	}
>> +	ASSERT_EQ(i, num_listens, "server socket");
>> +
>> +cleanup:
>> +	free_fds(listen_fds, num_listens);
>> +}
>> +
>> +void test_sock_destroy(void)
>> +{
>> +	int cgroup_fd = 0;
>> +	struct sock_destroy_prog *skel;
>> +
>> +	skel = sock_destroy_prog__open_and_load();
>> +	if (!ASSERT_OK_PTR(skel, "skel_open"))
>> +		return;
>> +
>> +	cgroup_fd = test__join_cgroup("/sock_destroy");
>> +	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
>> +		goto close_cgroup_fd;
>> +
>> +	skel->links.sock_connect = bpf_program__attach_cgroup(
>> +		skel->progs.sock_connect, cgroup_fd);
>> +	if (!ASSERT_OK_PTR(skel->links.sock_connect, "prog_attach"))
>> +		goto close_cgroup_fd;
>> +
>> +	if (test__start_subtest("tcp_client"))
>> +		test_tcp_client(skel);
>> +	if (test__start_subtest("tcp_server"))
>> +		test_tcp_server(skel);
>> +	if (test__start_subtest("udp_client"))
>> +		test_udp_client(skel);
>> +	if (test__start_subtest("udp_server"))
>> +		test_udp_server(skel);
>> +
>> +
>> +close_cgroup_fd:
>> +	close(cgroup_fd);
>> +	sock_destroy_prog__destroy(skel);
>> +}
>> diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>> new file mode 100644
>> index 000000000000..8e09d82c50f3
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>> @@ -0,0 +1,151 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include "vmlinux.h"
>> +
>> +#include "bpf_tracing_net.h"
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_endian.h>
>> +
>> +#define AF_INET6 10
> 
> [..]
> 
>> +/* Keep it in sync with prog_test/sock_destroy. */
>> +#define SERVER_PORT 6062
> 
> The test looks good, one optional unrelated nit maybe:
> 
> I've been guilty of these hard-coded ports in the past, but maybe
> we should stop hard-coding them? Getting the address of the listener (bound to
> port 0) and passing it to the bpf program via global variable should be super
> easy now (with the skeletons and network_helpers).


I briefly considered adding the ports in a map, and retrieving them in the test. But it didn't seem worthwhile as the tests should fail clearly when there is a mismatch. 

> 
> And, unrelated, maybe also fix a bunch of places where the reverse christmas
> tree doesn't look reverse anymore?

Ok. The checks should be part of tooling (e.g., checkpatch) though if they are meant to be enforced consistently, no?

> 
>> +
>> +int bpf_sock_destroy(struct sock_common *sk) __ksym;
>> +
>> +struct {
>> +	__uint(type, BPF_MAP_TYPE_ARRAY);
>> +	__uint(max_entries, 1);
>> +	__type(key, __u32);
>> +	__type(value, __u64);
>> +} tcp_conn_sockets SEC(".maps");
>> +
>> +struct {
>> +	__uint(type, BPF_MAP_TYPE_ARRAY);
>> +	__uint(max_entries, 1);
>> +	__type(key, __u32);
>> +	__type(value, __u64);
>> +} udp_conn_sockets SEC(".maps");
>> +
>> +SEC("cgroup/connect6")
>> +int sock_connect(struct bpf_sock_addr *ctx)
>> +{
>> +	int key = 0;
>> +	__u64 sock_cookie = 0;
>> +	__u32 keyc = 0;
>> +
>> +	if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
>> +		return 1;
>> +
>> +	sock_cookie = bpf_get_socket_cookie(ctx);
>> +	if (ctx->protocol == IPPROTO_TCP)
>> +		bpf_map_update_elem(&tcp_conn_sockets, &key, &sock_cookie, 0);
>> +	else if (ctx->protocol == IPPROTO_UDP)
>> +		bpf_map_update_elem(&udp_conn_sockets, &keyc, &sock_cookie, 0);
>> +	else
>> +		return 1;
>> +
>> +	return 1;
>> +}
>> +
>> +SEC("iter/tcp")
>> +int iter_tcp6_client(struct bpf_iter__tcp *ctx)
>> +{
>> +	struct sock_common *sk_common = ctx->sk_common;
>> +	struct seq_file *seq = ctx->meta->seq;
>> +	__u64 sock_cookie = 0;
>> +	__u64 *val;
>> +	int key = 0;
>> +
>> +	if (!sk_common)
>> +		return 0;
>> +
>> +	if (sk_common->skc_family != AF_INET6)
>> +		return 0;
>> +
>> +	sock_cookie  = bpf_get_socket_cookie(sk_common);
>> +	val = bpf_map_lookup_elem(&tcp_conn_sockets, &key);
>> +	if (!val)
>> +		return 0;
>> +	/* Destroy connected client sockets. */
>> +	if (sock_cookie == *val)
>> +		bpf_sock_destroy(sk_common);
>> +
>> +	return 0;
>> +}
>> +
>> +SEC("iter/tcp")
>> +int iter_tcp6_server(struct bpf_iter__tcp *ctx)
>> +{
>> +	struct sock_common *sk_common = ctx->sk_common;
>> +	struct seq_file *seq = ctx->meta->seq;
>> +	struct tcp6_sock *tcp_sk;
>> +	const struct inet_connection_sock *icsk;
>> +	const struct inet_sock *inet;
>> +	__u16 srcp;
>> +
>> +	if (!sk_common)
>> +		return 0;
>> +
>> +	if (sk_common->skc_family != AF_INET6)
>> +		return 0;
>> +
>> +	tcp_sk = bpf_skc_to_tcp6_sock(sk_common);
>> +	if (!tcp_sk)
>> +		return 0;
>> +
>> +	icsk = &tcp_sk->tcp.inet_conn;
>> +	inet = &icsk->icsk_inet;
>> +	srcp = bpf_ntohs(inet->inet_sport);
>> +
>> +	/* Destroy server sockets. */
>> +	if (srcp == SERVER_PORT)
>> +		bpf_sock_destroy(sk_common);
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +SEC("iter/udp")
>> +int iter_udp6_client(struct bpf_iter__udp *ctx)
>> +{
>> +	struct seq_file *seq = ctx->meta->seq;
>> +	struct udp_sock *udp_sk = ctx->udp_sk;
>> +	struct sock *sk = (struct sock *) udp_sk;
>> +	__u64 sock_cookie = 0, *val;
>> +	int key = 0;
>> +
>> +	if (!sk)
>> +		return 0;
>> +
>> +	sock_cookie  = bpf_get_socket_cookie(sk);
>> +	val = bpf_map_lookup_elem(&udp_conn_sockets, &key);
>> +	if (!val)
>> +		return 0;
>> +	/* Destroy connected client sockets. */
>> +	if (sock_cookie == *val)
>> +		bpf_sock_destroy((struct sock_common *)sk);
>> +
>> +	return 0;
>> +}
>> +
>> +SEC("iter/udp")
>> +int iter_udp6_server(struct bpf_iter__udp *ctx)
>> +{
>> +	struct seq_file *seq = ctx->meta->seq;
>> +	struct udp_sock *udp_sk = ctx->udp_sk;
>> +	struct sock *sk = (struct sock *) udp_sk;
>> +	__u16 srcp;
>> +	struct inet_sock *inet;
>> +
>> +	if (!sk)
>> +		return 0;
>> +
>> +	inet = &udp_sk->inet;
>> +	srcp = bpf_ntohs(inet->inet_sport);
>> +	if (srcp == SERVER_PORT)
>> +		bpf_sock_destroy((struct sock_common *)sk);
>> +
>> +	return 0;
>> +}
>> +
>> +char _license[] SEC("license") = "GPL";
>> --
>> 2.34.1

Stanislav Fomichev March 27, 2023, 4:54 p.m. UTC | #3

On 03/27, Aditi Ghag wrote:


> > On Mar 24, 2023, at 2:52 PM, Stanislav Fomichev <[e-mail]> wrote:
> >
> > On 03/23, Aditi Ghag wrote:
> >> The test cases for destroying sockets mirror the intended usages of the
> >> bpf_sock_destroy kfunc using iterators.
> >
> >> The destroy helpers set `ECONNABORTED` error code that we can validate  
> in
> >> the test code with client sockets. But UDP sockets have an overriding  
> error
> >> code from the disconnect called during abort, so the error code the
> >> validation is only done for TCP sockets.
> >
> >> Signed-off-by: Aditi Ghag <[e-mail]>
> >> ---
> >>  .../selftests/bpf/prog_tests/sock_destroy.c   | 195 ++++++++++++++++++
> >>  .../selftests/bpf/progs/sock_destroy_prog.c   | 151 ++++++++++++++
> >>  2 files changed, 346 insertions(+)
> >>  create mode 100644  
> tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> >>  create mode 100644  
> tools/testing/selftests/bpf/progs/sock_destroy_prog.c
> >
> >> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c  
> b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> >> new file mode 100644
> >> index 000000000000..cbce966af568
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> >> @@ -0,0 +1,195 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +#include <test_progs.h>
> >> +
> >> +#include "sock_destroy_prog.skel.h"
> >> +#include "network_helpers.h"
> >> +
> >> +#define SERVER_PORT 6062
> >> +
> >> +static void start_iter_sockets(struct bpf_program *prog)
> >> +{
> >> +	struct bpf_link *link;
> >> +	char buf[50] = {};
> >> +	int iter_fd, len;
> >> +
> >> +	link = bpf_program__attach_iter(prog, NULL);
> >> +	if (!ASSERT_OK_PTR(link, "attach_iter"))
> >> +		return;
> >> +
> >> +	iter_fd = bpf_iter_create(bpf_link__fd(link));
> >> +	if (!ASSERT_GE(iter_fd, 0, "create_iter"))
> >> +		goto free_link;
> >> +
> >> +	while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
> >> +		;
> >> +	ASSERT_GE(len, 0, "read");
> >> +
> >> +	close(iter_fd);
> >> +
> >> +free_link:
> >> +	bpf_link__destroy(link);
> >> +}
> >> +
> >> +static void test_tcp_client(struct sock_destroy_prog *skel)
> >> +{
> >> +	int serv = -1, clien = -1, n = 0;
> >> +
> >> +	serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
> >> +	if (!ASSERT_GE(serv, 0, "start_server"))
> >> +		goto cleanup_serv;
> >> +
> >> +	clien = connect_to_fd(serv, 0);
> >> +	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> >> +		goto cleanup_serv;
> >> +
> >> +	serv = accept(serv, NULL, NULL);
> >> +	if (!ASSERT_GE(serv, 0, "serv accept"))
> >> +		goto cleanup;
> >> +
> >> +	n = send(clien, "t", 1, 0);
> >> +	if (!ASSERT_GE(n, 0, "client send"))
> >> +		goto cleanup;
> >> +
> >> +	/* Run iterator program that destroys connected client sockets. */
> >> +	start_iter_sockets(skel->progs.iter_tcp6_client);
> >> +
> >> +	n = send(clien, "t", 1, 0);
> >> +	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> >> +		goto cleanup;
> >> +	ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket");
> >> +
> >> +
> >> +cleanup:
> >> +	close(clien);
> >> +cleanup_serv:
> >> +	close(serv);
> >> +}
> >> +
> >> +static void test_tcp_server(struct sock_destroy_prog *skel)
> >> +{
> >> +	int serv = -1, clien = -1, n = 0;
> >> +
> >> +	serv = start_server(AF_INET6, SOCK_STREAM, NULL, SERVER_PORT, 0);
> >> +	if (!ASSERT_GE(serv, 0, "start_server"))
> >> +		goto cleanup_serv;
> >> +
> >> +	clien = connect_to_fd(serv, 0);
> >> +	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> >> +		goto cleanup_serv;
> >> +
> >> +	serv = accept(serv, NULL, NULL);
> >> +	if (!ASSERT_GE(serv, 0, "serv accept"))
> >> +		goto cleanup;
> >> +
> >> +	n = send(clien, "t", 1, 0);
> >> +	if (!ASSERT_GE(n, 0, "client send"))
> >> +		goto cleanup;
> >> +
> >> +	/* Run iterator program that destroys server sockets. */
> >> +	start_iter_sockets(skel->progs.iter_tcp6_server);
> >> +
> >> +	n = send(clien, "t", 1, 0);
> >> +	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> >> +		goto cleanup;
> >> +	ASSERT_EQ(errno, ECONNRESET, "error code on destroyed socket");
> >> +
> >> +
> >> +cleanup:
> >> +	close(clien);
> >> +cleanup_serv:
> >> +	close(serv);
> >> +}
> >> +
> >> +
> >> +static void test_udp_client(struct sock_destroy_prog *skel)
> >> +{
> >> +	int serv = -1, clien = -1, n = 0;
> >> +
> >> +	serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0);
> >> +	if (!ASSERT_GE(serv, 0, "start_server"))
> >> +		goto cleanup_serv;
> >> +
> >> +	clien = connect_to_fd(serv, 0);
> >> +	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> >> +		goto cleanup_serv;
> >> +
> >> +	n = send(clien, "t", 1, 0);
> >> +	if (!ASSERT_GE(n, 0, "client send"))
> >> +		goto cleanup;
> >> +
> >> +	/* Run iterator program that destroys sockets. */
> >> +	start_iter_sockets(skel->progs.iter_udp6_client);
> >> +
> >> +	n = send(clien, "t", 1, 0);
> >> +	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> >> +		goto cleanup;
> >> +	/* UDP sockets have an overriding error code after they are  
> disconnected,
> >> +	 * so we don't check for ECONNABORTED error code.
> >> +	 */
> >> +
> >> +cleanup:
> >> +	close(clien);
> >> +cleanup_serv:
> >> +	close(serv);
> >> +}
> >> +
> >> +static void test_udp_server(struct sock_destroy_prog *skel)
> >> +{
> >> +	int *listen_fds = NULL, n, i;
> >> +	unsigned int num_listens = 5;
> >> +	char buf[1];
> >> +
> >> +	/* Start reuseport servers. */
> >> +	listen_fds = start_reuseport_server(AF_INET6, SOCK_DGRAM,
> >> +					    "::1", SERVER_PORT, 0,
> >> +					    num_listens);
> >> +	if (!ASSERT_OK_PTR(listen_fds, "start_reuseport_server"))
> >> +		goto cleanup;
> >> +
> >> +	/* Run iterator program that destroys server sockets. */
> >> +	start_iter_sockets(skel->progs.iter_udp6_server);
> >> +
> >> +	for (i = 0; i < num_listens; ++i) {
> >> +		n = read(listen_fds[i], buf, sizeof(buf));
> >> +		if (!ASSERT_EQ(n, -1, "read") ||
> >> +		    !ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed  
> socket"))
> >> +			break;
> >> +	}
> >> +	ASSERT_EQ(i, num_listens, "server socket");
> >> +
> >> +cleanup:
> >> +	free_fds(listen_fds, num_listens);
> >> +}
> >> +
> >> +void test_sock_destroy(void)
> >> +{
> >> +	int cgroup_fd = 0;
> >> +	struct sock_destroy_prog *skel;
> >> +
> >> +	skel = sock_destroy_prog__open_and_load();
> >> +	if (!ASSERT_OK_PTR(skel, "skel_open"))
> >> +		return;
> >> +
> >> +	cgroup_fd = test__join_cgroup("/sock_destroy");
> >> +	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
> >> +		goto close_cgroup_fd;
> >> +
> >> +	skel->links.sock_connect = bpf_program__attach_cgroup(
> >> +		skel->progs.sock_connect, cgroup_fd);
> >> +	if (!ASSERT_OK_PTR(skel->links.sock_connect, "prog_attach"))
> >> +		goto close_cgroup_fd;
> >> +
> >> +	if (test__start_subtest("tcp_client"))
> >> +		test_tcp_client(skel);
> >> +	if (test__start_subtest("tcp_server"))
> >> +		test_tcp_server(skel);
> >> +	if (test__start_subtest("udp_client"))
> >> +		test_udp_client(skel);
> >> +	if (test__start_subtest("udp_server"))
> >> +		test_udp_server(skel);
> >> +
> >> +
> >> +close_cgroup_fd:
> >> +	close(cgroup_fd);
> >> +	sock_destroy_prog__destroy(skel);
> >> +}
> >> diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c  
> b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
> >> new file mode 100644
> >> index 000000000000..8e09d82c50f3
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
> >> @@ -0,0 +1,151 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +#include "vmlinux.h"
> >> +
> >> +#include "bpf_tracing_net.h"
> >> +#include <bpf/bpf_helpers.h>
> >> +#include <bpf/bpf_endian.h>
> >> +
> >> +#define AF_INET6 10
> >
> > [..]
> >
> >> +/* Keep it in sync with prog_test/sock_destroy. */
> >> +#define SERVER_PORT 6062
> >
> > The test looks good, one optional unrelated nit maybe:
> >
> > I've been guilty of these hard-coded ports in the past, but maybe
> > we should stop hard-coding them? Getting the address of the listener  
> (bound to
> > port 0) and passing it to the bpf program via global variable should be  
> super
> > easy now (with the skeletons and network_helpers).


> I briefly considered adding the ports in a map, and retrieving them in  
> the test. But it didn't seem worthwhile as the tests should fail clearly  
> when there is a mismatch.

My worry is that the amount of those tests that have a hard-coded port
grows and at some point somebody will clash with somebody else.
And it might not be 100% apparent because test_progs is now multi-threaded
and racy..

> >
> > And, unrelated, maybe also fix a bunch of places where the reverse  
> christmas
> > tree doesn't look reverse anymore?

> Ok. The checks should be part of tooling (e.g., checkpatch) though if  
> they are meant to be enforced consistently, no?

They are networking specific, so they are not part of a checkpath :-(
I won't say they are consistently enforced, but we try to keep then
whenever possible.

> >
> >> +
> >> +int bpf_sock_destroy(struct sock_common *sk) __ksym;
> >> +
> >> +struct {
> >> +	__uint(type, BPF_MAP_TYPE_ARRAY);
> >> +	__uint(max_entries, 1);
> >> +	__type(key, __u32);
> >> +	__type(value, __u64);
> >> +} tcp_conn_sockets SEC(".maps");
> >> +
> >> +struct {
> >> +	__uint(type, BPF_MAP_TYPE_ARRAY);
> >> +	__uint(max_entries, 1);
> >> +	__type(key, __u32);
> >> +	__type(value, __u64);
> >> +} udp_conn_sockets SEC(".maps");
> >> +
> >> +SEC("cgroup/connect6")
> >> +int sock_connect(struct bpf_sock_addr *ctx)
> >> +{
> >> +	int key = 0;
> >> +	__u64 sock_cookie = 0;
> >> +	__u32 keyc = 0;
> >> +
> >> +	if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
> >> +		return 1;
> >> +
> >> +	sock_cookie = bpf_get_socket_cookie(ctx);
> >> +	if (ctx->protocol == IPPROTO_TCP)
> >> +		bpf_map_update_elem(&tcp_conn_sockets, &key, &sock_cookie, 0);
> >> +	else if (ctx->protocol == IPPROTO_UDP)
> >> +		bpf_map_update_elem(&udp_conn_sockets, &keyc, &sock_cookie, 0);
> >> +	else
> >> +		return 1;
> >> +
> >> +	return 1;
> >> +}
> >> +
> >> +SEC("iter/tcp")
> >> +int iter_tcp6_client(struct bpf_iter__tcp *ctx)
> >> +{
> >> +	struct sock_common *sk_common = ctx->sk_common;
> >> +	struct seq_file *seq = ctx->meta->seq;
> >> +	__u64 sock_cookie = 0;
> >> +	__u64 *val;
> >> +	int key = 0;
> >> +
> >> +	if (!sk_common)
> >> +		return 0;
> >> +
> >> +	if (sk_common->skc_family != AF_INET6)
> >> +		return 0;
> >> +
> >> +	sock_cookie  = bpf_get_socket_cookie(sk_common);
> >> +	val = bpf_map_lookup_elem(&tcp_conn_sockets, &key);
> >> +	if (!val)
> >> +		return 0;
> >> +	/* Destroy connected client sockets. */
> >> +	if (sock_cookie == *val)
> >> +		bpf_sock_destroy(sk_common);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +SEC("iter/tcp")
> >> +int iter_tcp6_server(struct bpf_iter__tcp *ctx)
> >> +{
> >> +	struct sock_common *sk_common = ctx->sk_common;
> >> +	struct seq_file *seq = ctx->meta->seq;
> >> +	struct tcp6_sock *tcp_sk;
> >> +	const struct inet_connection_sock *icsk;
> >> +	const struct inet_sock *inet;
> >> +	__u16 srcp;
> >> +
> >> +	if (!sk_common)
> >> +		return 0;
> >> +
> >> +	if (sk_common->skc_family != AF_INET6)
> >> +		return 0;
> >> +
> >> +	tcp_sk = bpf_skc_to_tcp6_sock(sk_common);
> >> +	if (!tcp_sk)
> >> +		return 0;
> >> +
> >> +	icsk = &tcp_sk->tcp.inet_conn;
> >> +	inet = &icsk->icsk_inet;
> >> +	srcp = bpf_ntohs(inet->inet_sport);
> >> +
> >> +	/* Destroy server sockets. */
> >> +	if (srcp == SERVER_PORT)
> >> +		bpf_sock_destroy(sk_common);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +
> >> +SEC("iter/udp")
> >> +int iter_udp6_client(struct bpf_iter__udp *ctx)
> >> +{
> >> +	struct seq_file *seq = ctx->meta->seq;
> >> +	struct udp_sock *udp_sk = ctx->udp_sk;
> >> +	struct sock *sk = (struct sock *) udp_sk;
> >> +	__u64 sock_cookie = 0, *val;
> >> +	int key = 0;
> >> +
> >> +	if (!sk)
> >> +		return 0;
> >> +
> >> +	sock_cookie  = bpf_get_socket_cookie(sk);
> >> +	val = bpf_map_lookup_elem(&udp_conn_sockets, &key);
> >> +	if (!val)
> >> +		return 0;
> >> +	/* Destroy connected client sockets. */
> >> +	if (sock_cookie == *val)
> >> +		bpf_sock_destroy((struct sock_common *)sk);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +SEC("iter/udp")
> >> +int iter_udp6_server(struct bpf_iter__udp *ctx)
> >> +{
> >> +	struct seq_file *seq = ctx->meta->seq;
> >> +	struct udp_sock *udp_sk = ctx->udp_sk;
> >> +	struct sock *sk = (struct sock *) udp_sk;
> >> +	__u16 srcp;
> >> +	struct inet_sock *inet;
> >> +
> >> +	if (!sk)
> >> +		return 0;
> >> +
> >> +	inet = &udp_sk->inet;
> >> +	srcp = bpf_ntohs(inet->inet_sport);
> >> +	if (srcp == SERVER_PORT)
> >> +		bpf_sock_destroy((struct sock_common *)sk);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +char _license[] SEC("license") = "GPL";
> >> --
> >> 2.34.1

Aditi Ghag March 28, 2023, 5:50 p.m. UTC | #4

> On Mar 27, 2023, at 9:54 AM, Stanislav Fomichev <[e-mail]> wrote:
> 
> On 03/27, Aditi Ghag wrote:
> 
> 
>> > On Mar 24, 2023, at 2:52 PM, Stanislav Fomichev <[e-mail]> wrote:
>> >
>> > On 03/23, Aditi Ghag wrote:
>> >> The test cases for destroying sockets mirror the intended usages of the
>> >> bpf_sock_destroy kfunc using iterators.
>> >
>> >> The destroy helpers set `ECONNABORTED` error code that we can validate in
>> >> the test code with client sockets. But UDP sockets have an overriding error
>> >> code from the disconnect called during abort, so the error code the
>> >> validation is only done for TCP sockets.
>> >
>> >> Signed-off-by: Aditi Ghag <[e-mail]>
>> >> ---
>> >>  .../selftests/bpf/prog_tests/sock_destroy.c   | 195 ++++++++++++++++++
>> >>  .../selftests/bpf/progs/sock_destroy_prog.c   | 151 ++++++++++++++
>> >>  2 files changed, 346 insertions(+)
>> >>  create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>> >>  create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>> >
>> >> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>> >> new file mode 100644
>> >> index 000000000000..cbce966af568
>> >> --- /dev/null
>> >> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>> >> @@ -0,0 +1,195 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +#include <test_progs.h>
>> >> +
>> >> +#include "sock_destroy_prog.skel.h"
>> >> +#include "network_helpers.h"
>> >> +
>> >> +#define SERVER_PORT 6062
>> >> +
>> >> +static void start_iter_sockets(struct bpf_program *prog)
>> >> +{
>> >> +	struct bpf_link *link;
>> >> +	char buf[50] = {};
>> >> +	int iter_fd, len;
>> >> +
>> >> +	link = bpf_program__attach_iter(prog, NULL);
>> >> +	if (!ASSERT_OK_PTR(link, "attach_iter"))
>> >> +		return;
>> >> +
>> >> +	iter_fd = bpf_iter_create(bpf_link__fd(link));
>> >> +	if (!ASSERT_GE(iter_fd, 0, "create_iter"))
>> >> +		goto free_link;
>> >> +
>> >> +	while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
>> >> +		;
>> >> +	ASSERT_GE(len, 0, "read");
>> >> +
>> >> +	close(iter_fd);
>> >> +
>> >> +free_link:
>> >> +	bpf_link__destroy(link);
>> >> +}
>> >> +
>> >> +static void test_tcp_client(struct sock_destroy_prog *skel)
>> >> +{
>> >> +	int serv = -1, clien = -1, n = 0;
>> >> +
>> >> +	serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
>> >> +	if (!ASSERT_GE(serv, 0, "start_server"))
>> >> +		goto cleanup_serv;
>> >> +
>> >> +	clien = connect_to_fd(serv, 0);
>> >> +	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
>> >> +		goto cleanup_serv;
>> >> +
>> >> +	serv = accept(serv, NULL, NULL);
>> >> +	if (!ASSERT_GE(serv, 0, "serv accept"))
>> >> +		goto cleanup;
>> >> +
>> >> +	n = send(clien, "t", 1, 0);
>> >> +	if (!ASSERT_GE(n, 0, "client send"))
>> >> +		goto cleanup;
>> >> +
>> >> +	/* Run iterator program that destroys connected client sockets. */
>> >> +	start_iter_sockets(skel->progs.iter_tcp6_client);
>> >> +
>> >> +	n = send(clien, "t", 1, 0);
>> >> +	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
>> >> +		goto cleanup;
>> >> +	ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket");
>> >> +
>> >> +
>> >> +cleanup:
>> >> +	close(clien);
>> >> +cleanup_serv:
>> >> +	close(serv);
>> >> +}
>> >> +
>> >> +static void test_tcp_server(struct sock_destroy_prog *skel)
>> >> +{
>> >> +	int serv = -1, clien = -1, n = 0;
>> >> +
>> >> +	serv = start_server(AF_INET6, SOCK_STREAM, NULL, SERVER_PORT, 0);
>> >> +	if (!ASSERT_GE(serv, 0, "start_server"))
>> >> +		goto cleanup_serv;
>> >> +
>> >> +	clien = connect_to_fd(serv, 0);
>> >> +	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
>> >> +		goto cleanup_serv;
>> >> +
>> >> +	serv = accept(serv, NULL, NULL);
>> >> +	if (!ASSERT_GE(serv, 0, "serv accept"))
>> >> +		goto cleanup;
>> >> +
>> >> +	n = send(clien, "t", 1, 0);
>> >> +	if (!ASSERT_GE(n, 0, "client send"))
>> >> +		goto cleanup;
>> >> +
>> >> +	/* Run iterator program that destroys server sockets. */
>> >> +	start_iter_sockets(skel->progs.iter_tcp6_server);
>> >> +
>> >> +	n = send(clien, "t", 1, 0);
>> >> +	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
>> >> +		goto cleanup;
>> >> +	ASSERT_EQ(errno, ECONNRESET, "error code on destroyed socket");
>> >> +
>> >> +
>> >> +cleanup:
>> >> +	close(clien);
>> >> +cleanup_serv:
>> >> +	close(serv);
>> >> +}
>> >> +
>> >> +
>> >> +static void test_udp_client(struct sock_destroy_prog *skel)
>> >> +{
>> >> +	int serv = -1, clien = -1, n = 0;
>> >> +
>> >> +	serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0);
>> >> +	if (!ASSERT_GE(serv, 0, "start_server"))
>> >> +		goto cleanup_serv;
>> >> +
>> >> +	clien = connect_to_fd(serv, 0);
>> >> +	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
>> >> +		goto cleanup_serv;
>> >> +
>> >> +	n = send(clien, "t", 1, 0);
>> >> +	if (!ASSERT_GE(n, 0, "client send"))
>> >> +		goto cleanup;
>> >> +
>> >> +	/* Run iterator program that destroys sockets. */
>> >> +	start_iter_sockets(skel->progs.iter_udp6_client);
>> >> +
>> >> +	n = send(clien, "t", 1, 0);
>> >> +	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
>> >> +		goto cleanup;
>> >> +	/* UDP sockets have an overriding error code after they are disconnected,
>> >> +	 * so we don't check for ECONNABORTED error code.
>> >> +	 */
>> >> +
>> >> +cleanup:
>> >> +	close(clien);
>> >> +cleanup_serv:
>> >> +	close(serv);
>> >> +}
>> >> +
>> >> +static void test_udp_server(struct sock_destroy_prog *skel)
>> >> +{
>> >> +	int *listen_fds = NULL, n, i;
>> >> +	unsigned int num_listens = 5;
>> >> +	char buf[1];
>> >> +
>> >> +	/* Start reuseport servers. */
>> >> +	listen_fds = start_reuseport_server(AF_INET6, SOCK_DGRAM,
>> >> +					    "::1", SERVER_PORT, 0,
>> >> +					    num_listens);
>> >> +	if (!ASSERT_OK_PTR(listen_fds, "start_reuseport_server"))
>> >> +		goto cleanup;
>> >> +
>> >> +	/* Run iterator program that destroys server sockets. */
>> >> +	start_iter_sockets(skel->progs.iter_udp6_server);
>> >> +
>> >> +	for (i = 0; i < num_listens; ++i) {
>> >> +		n = read(listen_fds[i], buf, sizeof(buf));
>> >> +		if (!ASSERT_EQ(n, -1, "read") ||
>> >> +		    !ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket"))
>> >> +			break;
>> >> +	}
>> >> +	ASSERT_EQ(i, num_listens, "server socket");
>> >> +
>> >> +cleanup:
>> >> +	free_fds(listen_fds, num_listens);
>> >> +}
>> >> +
>> >> +void test_sock_destroy(void)
>> >> +{
>> >> +	int cgroup_fd = 0;
>> >> +	struct sock_destroy_prog *skel;
>> >> +
>> >> +	skel = sock_destroy_prog__open_and_load();
>> >> +	if (!ASSERT_OK_PTR(skel, "skel_open"))
>> >> +		return;
>> >> +
>> >> +	cgroup_fd = test__join_cgroup("/sock_destroy");
>> >> +	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
>> >> +		goto close_cgroup_fd;
>> >> +
>> >> +	skel->links.sock_connect = bpf_program__attach_cgroup(
>> >> +		skel->progs.sock_connect, cgroup_fd);
>> >> +	if (!ASSERT_OK_PTR(skel->links.sock_connect, "prog_attach"))
>> >> +		goto close_cgroup_fd;
>> >> +
>> >> +	if (test__start_subtest("tcp_client"))
>> >> +		test_tcp_client(skel);
>> >> +	if (test__start_subtest("tcp_server"))
>> >> +		test_tcp_server(skel);
>> >> +	if (test__start_subtest("udp_client"))
>> >> +		test_udp_client(skel);
>> >> +	if (test__start_subtest("udp_server"))
>> >> +		test_udp_server(skel);
>> >> +
>> >> +
>> >> +close_cgroup_fd:
>> >> +	close(cgroup_fd);
>> >> +	sock_destroy_prog__destroy(skel);
>> >> +}
>> >> diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>> >> new file mode 100644
>> >> index 000000000000..8e09d82c50f3
>> >> --- /dev/null
>> >> +++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>> >> @@ -0,0 +1,151 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +
>> >> +#include "vmlinux.h"
>> >> +
>> >> +#include "bpf_tracing_net.h"
>> >> +#include <bpf/bpf_helpers.h>
>> >> +#include <bpf/bpf_endian.h>
>> >> +
>> >> +#define AF_INET6 10
>> >
>> > [..]
>> >
>> >> +/* Keep it in sync with prog_test/sock_destroy. */
>> >> +#define SERVER_PORT 6062
>> >
>> > The test looks good, one optional unrelated nit maybe:
>> >
>> > I've been guilty of these hard-coded ports in the past, but maybe
>> > we should stop hard-coding them? Getting the address of the listener (bound to
>> > port 0) and passing it to the bpf program via global variable should be super
>> > easy now (with the skeletons and network_helpers).
> 
> 
>> I briefly considered adding the ports in a map, and retrieving them in the test. But it didn't seem worthwhile as the tests should fail clearly when there is a mismatch.
> 
> My worry is that the amount of those tests that have a hard-coded port
> grows and at some point somebody will clash with somebody else.
> And it might not be 100% apparent because test_progs is now multi-threaded
> and racy..
> 

So you would like the ports to be unique across all the tests. 

>Getting the address of the listener (bound to
> port 0) and passing it to the bpf program via global variable should be super
> easy now (with the skeletons and network_helpers).

Just so that we are on the same page, could you point to which network helpers are you referring to here for passing global variables?


>> >
>> > And, unrelated, maybe also fix a bunch of places where the reverse christmas
>> > tree doesn't look reverse anymore?
> 
>> Ok. The checks should be part of tooling (e.g., checkpatch) though if they are meant to be enforced consistently, no?
> 
> They are networking specific, so they are not part of a checkpath :-(
> I won't say they are consistently enforced, but we try to keep then
> whenever possible.
> 
>> >
>> >> +
>> >> +int bpf_sock_destroy(struct sock_common *sk) __ksym;
>> >> +
>> >> +struct {
>> >> +	__uint(type, BPF_MAP_TYPE_ARRAY);
>> >> +	__uint(max_entries, 1);
>> >> +	__type(key, __u32);
>> >> +	__type(value, __u64);
>> >> +} tcp_conn_sockets SEC(".maps");
>> >> +
>> >> +struct {
>> >> +	__uint(type, BPF_MAP_TYPE_ARRAY);
>> >> +	__uint(max_entries, 1);
>> >> +	__type(key, __u32);
>> >> +	__type(value, __u64);
>> >> +} udp_conn_sockets SEC(".maps");
>> >> +
>> >> +SEC("cgroup/connect6")
>> >> +int sock_connect(struct bpf_sock_addr *ctx)
>> >> +{
>> >> +	int key = 0;
>> >> +	__u64 sock_cookie = 0;
>> >> +	__u32 keyc = 0;
>> >> +
>> >> +	if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
>> >> +		return 1;
>> >> +
>> >> +	sock_cookie = bpf_get_socket_cookie(ctx);
>> >> +	if (ctx->protocol == IPPROTO_TCP)
>> >> +		bpf_map_update_elem(&tcp_conn_sockets, &key, &sock_cookie, 0);
>> >> +	else if (ctx->protocol == IPPROTO_UDP)
>> >> +		bpf_map_update_elem(&udp_conn_sockets, &keyc, &sock_cookie, 0);
>> >> +	else
>> >> +		return 1;
>> >> +
>> >> +	return 1;
>> >> +}
>> >> +
>> >> +SEC("iter/tcp")
>> >> +int iter_tcp6_client(struct bpf_iter__tcp *ctx)
>> >> +{
>> >> +	struct sock_common *sk_common = ctx->sk_common;
>> >> +	struct seq_file *seq = ctx->meta->seq;
>> >> +	__u64 sock_cookie = 0;
>> >> +	__u64 *val;
>> >> +	int key = 0;
>> >> +
>> >> +	if (!sk_common)
>> >> +		return 0;
>> >> +
>> >> +	if (sk_common->skc_family != AF_INET6)
>> >> +		return 0;
>> >> +
>> >> +	sock_cookie  = bpf_get_socket_cookie(sk_common);
>> >> +	val = bpf_map_lookup_elem(&tcp_conn_sockets, &key);
>> >> +	if (!val)
>> >> +		return 0;
>> >> +	/* Destroy connected client sockets. */
>> >> +	if (sock_cookie == *val)
>> >> +		bpf_sock_destroy(sk_common);
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +SEC("iter/tcp")
>> >> +int iter_tcp6_server(struct bpf_iter__tcp *ctx)
>> >> +{
>> >> +	struct sock_common *sk_common = ctx->sk_common;
>> >> +	struct seq_file *seq = ctx->meta->seq;
>> >> +	struct tcp6_sock *tcp_sk;
>> >> +	const struct inet_connection_sock *icsk;
>> >> +	const struct inet_sock *inet;
>> >> +	__u16 srcp;
>> >> +
>> >> +	if (!sk_common)
>> >> +		return 0;
>> >> +
>> >> +	if (sk_common->skc_family != AF_INET6)
>> >> +		return 0;
>> >> +
>> >> +	tcp_sk = bpf_skc_to_tcp6_sock(sk_common);
>> >> +	if (!tcp_sk)
>> >> +		return 0;
>> >> +
>> >> +	icsk = &tcp_sk->tcp.inet_conn;
>> >> +	inet = &icsk->icsk_inet;
>> >> +	srcp = bpf_ntohs(inet->inet_sport);
>> >> +
>> >> +	/* Destroy server sockets. */
>> >> +	if (srcp == SERVER_PORT)
>> >> +		bpf_sock_destroy(sk_common);
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +
>> >> +SEC("iter/udp")
>> >> +int iter_udp6_client(struct bpf_iter__udp *ctx)
>> >> +{
>> >> +	struct seq_file *seq = ctx->meta->seq;
>> >> +	struct udp_sock *udp_sk = ctx->udp_sk;
>> >> +	struct sock *sk = (struct sock *) udp_sk;
>> >> +	__u64 sock_cookie = 0, *val;
>> >> +	int key = 0;
>> >> +
>> >> +	if (!sk)
>> >> +		return 0;
>> >> +
>> >> +	sock_cookie  = bpf_get_socket_cookie(sk);
>> >> +	val = bpf_map_lookup_elem(&udp_conn_sockets, &key);
>> >> +	if (!val)
>> >> +		return 0;
>> >> +	/* Destroy connected client sockets. */
>> >> +	if (sock_cookie == *val)
>> >> +		bpf_sock_destroy((struct sock_common *)sk);
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +SEC("iter/udp")
>> >> +int iter_udp6_server(struct bpf_iter__udp *ctx)
>> >> +{
>> >> +	struct seq_file *seq = ctx->meta->seq;
>> >> +	struct udp_sock *udp_sk = ctx->udp_sk;
>> >> +	struct sock *sk = (struct sock *) udp_sk;
>> >> +	__u16 srcp;
>> >> +	struct inet_sock *inet;
>> >> +
>> >> +	if (!sk)
>> >> +		return 0;
>> >> +
>> >> +	inet = &udp_sk->inet;
>> >> +	srcp = bpf_ntohs(inet->inet_sport);
>> >> +	if (srcp == SERVER_PORT)
>> >> +		bpf_sock_destroy((struct sock_common *)sk);
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +char _license[] SEC("license") = "GPL";
>> >> --
>> >> 2.34.1

Stanislav Fomichev March 28, 2023, 6:35 p.m. UTC | #5

On Tue, Mar 28, 2023 at 10:51 AM Aditi Ghag <[e-mail]> wrote:
>
>
>
> > On Mar 27, 2023, at 9:54 AM, Stanislav Fomichev <[e-mail]> wrote:
> >
> > On 03/27, Aditi Ghag wrote:
> >
> >
> >> > On Mar 24, 2023, at 2:52 PM, Stanislav Fomichev <[e-mail]> wrote:
> >> >
> >> > On 03/23, Aditi Ghag wrote:
> >> >> The test cases for destroying sockets mirror the intended usages of the
> >> >> bpf_sock_destroy kfunc using iterators.
> >> >
> >> >> The destroy helpers set `ECONNABORTED` error code that we can validate in
> >> >> the test code with client sockets. But UDP sockets have an overriding error
> >> >> code from the disconnect called during abort, so the error code the
> >> >> validation is only done for TCP sockets.
> >> >
> >> >> Signed-off-by: Aditi Ghag <[e-mail]>
> >> >> ---
> >> >>  .../selftests/bpf/prog_tests/sock_destroy.c   | 195 ++++++++++++++++++
> >> >>  .../selftests/bpf/progs/sock_destroy_prog.c   | 151 ++++++++++++++
> >> >>  2 files changed, 346 insertions(+)
> >> >>  create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> >> >>  create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c
> >> >
> >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> >> >> new file mode 100644
> >> >> index 000000000000..cbce966af568
> >> >> --- /dev/null
> >> >> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> >> >> @@ -0,0 +1,195 @@
> >> >> +// SPDX-License-Identifier: GPL-2.0
> >> >> +#include <test_progs.h>
> >> >> +
> >> >> +#include "sock_destroy_prog.skel.h"
> >> >> +#include "network_helpers.h"
> >> >> +
> >> >> +#define SERVER_PORT 6062
> >> >> +
> >> >> +static void start_iter_sockets(struct bpf_program *prog)
> >> >> +{
> >> >> + struct bpf_link *link;
> >> >> + char buf[50] = {};
> >> >> + int iter_fd, len;
> >> >> +
> >> >> + link = bpf_program__attach_iter(prog, NULL);
> >> >> + if (!ASSERT_OK_PTR(link, "attach_iter"))
> >> >> +         return;
> >> >> +
> >> >> + iter_fd = bpf_iter_create(bpf_link__fd(link));
> >> >> + if (!ASSERT_GE(iter_fd, 0, "create_iter"))
> >> >> +         goto free_link;
> >> >> +
> >> >> + while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
> >> >> +         ;
> >> >> + ASSERT_GE(len, 0, "read");
> >> >> +
> >> >> + close(iter_fd);
> >> >> +
> >> >> +free_link:
> >> >> + bpf_link__destroy(link);
> >> >> +}
> >> >> +
> >> >> +static void test_tcp_client(struct sock_destroy_prog *skel)
> >> >> +{
> >> >> + int serv = -1, clien = -1, n = 0;
> >> >> +
> >> >> + serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
> >> >> + if (!ASSERT_GE(serv, 0, "start_server"))
> >> >> +         goto cleanup_serv;
> >> >> +
> >> >> + clien = connect_to_fd(serv, 0);
> >> >> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> >> >> +         goto cleanup_serv;
> >> >> +
> >> >> + serv = accept(serv, NULL, NULL);
> >> >> + if (!ASSERT_GE(serv, 0, "serv accept"))
> >> >> +         goto cleanup;
> >> >> +
> >> >> + n = send(clien, "t", 1, 0);
> >> >> + if (!ASSERT_GE(n, 0, "client send"))
> >> >> +         goto cleanup;
> >> >> +
> >> >> + /* Run iterator program that destroys connected client sockets. */
> >> >> + start_iter_sockets(skel->progs.iter_tcp6_client);
> >> >> +
> >> >> + n = send(clien, "t", 1, 0);
> >> >> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> >> >> +         goto cleanup;
> >> >> + ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket");
> >> >> +
> >> >> +
> >> >> +cleanup:
> >> >> + close(clien);
> >> >> +cleanup_serv:
> >> >> + close(serv);
> >> >> +}
> >> >> +
> >> >> +static void test_tcp_server(struct sock_destroy_prog *skel)
> >> >> +{
> >> >> + int serv = -1, clien = -1, n = 0;
> >> >> +
> >> >> + serv = start_server(AF_INET6, SOCK_STREAM, NULL, SERVER_PORT, 0);
> >> >> + if (!ASSERT_GE(serv, 0, "start_server"))
> >> >> +         goto cleanup_serv;
> >> >> +
> >> >> + clien = connect_to_fd(serv, 0);
> >> >> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> >> >> +         goto cleanup_serv;
> >> >> +
> >> >> + serv = accept(serv, NULL, NULL);
> >> >> + if (!ASSERT_GE(serv, 0, "serv accept"))
> >> >> +         goto cleanup;
> >> >> +
> >> >> + n = send(clien, "t", 1, 0);
> >> >> + if (!ASSERT_GE(n, 0, "client send"))
> >> >> +         goto cleanup;
> >> >> +
> >> >> + /* Run iterator program that destroys server sockets. */
> >> >> + start_iter_sockets(skel->progs.iter_tcp6_server);
> >> >> +
> >> >> + n = send(clien, "t", 1, 0);
> >> >> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> >> >> +         goto cleanup;
> >> >> + ASSERT_EQ(errno, ECONNRESET, "error code on destroyed socket");
> >> >> +
> >> >> +
> >> >> +cleanup:
> >> >> + close(clien);
> >> >> +cleanup_serv:
> >> >> + close(serv);
> >> >> +}
> >> >> +
> >> >> +
> >> >> +static void test_udp_client(struct sock_destroy_prog *skel)
> >> >> +{
> >> >> + int serv = -1, clien = -1, n = 0;
> >> >> +
> >> >> + serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0);
> >> >> + if (!ASSERT_GE(serv, 0, "start_server"))
> >> >> +         goto cleanup_serv;
> >> >> +
> >> >> + clien = connect_to_fd(serv, 0);
> >> >> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> >> >> +         goto cleanup_serv;
> >> >> +
> >> >> + n = send(clien, "t", 1, 0);
> >> >> + if (!ASSERT_GE(n, 0, "client send"))
> >> >> +         goto cleanup;
> >> >> +
> >> >> + /* Run iterator program that destroys sockets. */
> >> >> + start_iter_sockets(skel->progs.iter_udp6_client);
> >> >> +
> >> >> + n = send(clien, "t", 1, 0);
> >> >> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> >> >> +         goto cleanup;
> >> >> + /* UDP sockets have an overriding error code after they are disconnected,
> >> >> +  * so we don't check for ECONNABORTED error code.
> >> >> +  */
> >> >> +
> >> >> +cleanup:
> >> >> + close(clien);
> >> >> +cleanup_serv:
> >> >> + close(serv);
> >> >> +}
> >> >> +
> >> >> +static void test_udp_server(struct sock_destroy_prog *skel)
> >> >> +{
> >> >> + int *listen_fds = NULL, n, i;
> >> >> + unsigned int num_listens = 5;
> >> >> + char buf[1];
> >> >> +
> >> >> + /* Start reuseport servers. */
> >> >> + listen_fds = start_reuseport_server(AF_INET6, SOCK_DGRAM,
> >> >> +                                     "::1", SERVER_PORT, 0,
> >> >> +                                     num_listens);
> >> >> + if (!ASSERT_OK_PTR(listen_fds, "start_reuseport_server"))
> >> >> +         goto cleanup;
> >> >> +
> >> >> + /* Run iterator program that destroys server sockets. */
> >> >> + start_iter_sockets(skel->progs.iter_udp6_server);
> >> >> +
> >> >> + for (i = 0; i < num_listens; ++i) {
> >> >> +         n = read(listen_fds[i], buf, sizeof(buf));
> >> >> +         if (!ASSERT_EQ(n, -1, "read") ||
> >> >> +             !ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket"))
> >> >> +                 break;
> >> >> + }
> >> >> + ASSERT_EQ(i, num_listens, "server socket");
> >> >> +
> >> >> +cleanup:
> >> >> + free_fds(listen_fds, num_listens);
> >> >> +}
> >> >> +
> >> >> +void test_sock_destroy(void)
> >> >> +{
> >> >> + int cgroup_fd = 0;
> >> >> + struct sock_destroy_prog *skel;
> >> >> +
> >> >> + skel = sock_destroy_prog__open_and_load();
> >> >> + if (!ASSERT_OK_PTR(skel, "skel_open"))
> >> >> +         return;
> >> >> +
> >> >> + cgroup_fd = test__join_cgroup("/sock_destroy");
> >> >> + if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
> >> >> +         goto close_cgroup_fd;
> >> >> +
> >> >> + skel->links.sock_connect = bpf_program__attach_cgroup(
> >> >> +         skel->progs.sock_connect, cgroup_fd);
> >> >> + if (!ASSERT_OK_PTR(skel->links.sock_connect, "prog_attach"))
> >> >> +         goto close_cgroup_fd;
> >> >> +
> >> >> + if (test__start_subtest("tcp_client"))
> >> >> +         test_tcp_client(skel);
> >> >> + if (test__start_subtest("tcp_server"))
> >> >> +         test_tcp_server(skel);
> >> >> + if (test__start_subtest("udp_client"))
> >> >> +         test_udp_client(skel);
> >> >> + if (test__start_subtest("udp_server"))
> >> >> +         test_udp_server(skel);
> >> >> +
> >> >> +
> >> >> +close_cgroup_fd:
> >> >> + close(cgroup_fd);
> >> >> + sock_destroy_prog__destroy(skel);
> >> >> +}
> >> >> diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
> >> >> new file mode 100644
> >> >> index 000000000000..8e09d82c50f3
> >> >> --- /dev/null
> >> >> +++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
> >> >> @@ -0,0 +1,151 @@
> >> >> +// SPDX-License-Identifier: GPL-2.0
> >> >> +
> >> >> +#include "vmlinux.h"
> >> >> +
> >> >> +#include "bpf_tracing_net.h"
> >> >> +#include <bpf/bpf_helpers.h>
> >> >> +#include <bpf/bpf_endian.h>
> >> >> +
> >> >> +#define AF_INET6 10
> >> >
> >> > [..]
> >> >
> >> >> +/* Keep it in sync with prog_test/sock_destroy. */
> >> >> +#define SERVER_PORT 6062
> >> >
> >> > The test looks good, one optional unrelated nit maybe:
> >> >
> >> > I've been guilty of these hard-coded ports in the past, but maybe
> >> > we should stop hard-coding them? Getting the address of the listener (bound to
> >> > port 0) and passing it to the bpf program via global variable should be super
> >> > easy now (with the skeletons and network_helpers).
> >
> >
> >> I briefly considered adding the ports in a map, and retrieving them in the test. But it didn't seem worthwhile as the tests should fail clearly when there is a mismatch.
> >
> > My worry is that the amount of those tests that have a hard-coded port
> > grows and at some point somebody will clash with somebody else.
> > And it might not be 100% apparent because test_progs is now multi-threaded
> > and racy..
> >
>
> So you would like the ports to be unique across all the tests.

Yeah, but it's hard without having some kind of global registry. Take
a look at the following:

$ grep -Iri _port tools/testing/selftests/bpf/ | grep -P '\d{4}'

tools/testing/selftests/bpf/progs/connect_force_port4.c:
sa.sin_port = bpf_htons(22222);
tools/testing/selftests/bpf/progs/connect_force_port4.c:        if
(ctx->user_port == bpf_htons(60000)) {
tools/testing/selftests/bpf/progs/connect_force_port4.c:
 ctx->user_port = bpf_htons(60123);
tools/testing/selftests/bpf/progs/connect_force_port4.c:        if
(ctx->user_port == bpf_htons(60123)) {
tools/testing/selftests/bpf/progs/connect_force_port4.c:
 ctx->user_port = bpf_htons(60000);
tools/testing/selftests/bpf/progs/connect_force_port4.c:        if
(ctx->user_port == bpf_htons(60123)) {
tools/testing/selftests/bpf/progs/connect6_prog.c:#define
DST_REWRITE_PORT6     6666
tools/testing/selftests/bpf/progs/test_sk_lookup.c:static const __u16
SRC_PORT = bpf_htons(8008);
tools/testing/selftests/bpf/progs/test_sk_lookup.c:static const __u16
DST_PORT = 7007; /* Host byte order */
tools/testing/selftests/bpf/progs/test_tc_dtime.c:      __u16
dst_ns_port = __bpf_htons(50000 + test);
tools/testing/selftests/bpf/progs/connect4_dropper.c:   if
(ctx->user_port == bpf_htons(60120))
tools/testing/selftests/bpf/progs/connect_force_port6.c:
sa.sin6_port = bpf_htons(22223);
tools/testing/selftests/bpf/progs/connect_force_port6.c:        if
(ctx->user_port == bpf_htons(60000)) {
tools/testing/selftests/bpf/progs/connect_force_port6.c:
 ctx->user_port = bpf_htons(60124);
tools/testing/selftests/bpf/progs/connect_force_port6.c:        if
(ctx->user_port == bpf_htons(60124)) {
tools/testing/selftests/bpf/progs/connect_force_port6.c:
 ctx->user_port = bpf_htons(60000);
tools/testing/selftests/bpf/progs/connect_force_port6.c:        if
(ctx->user_port == bpf_htons(60124)) {
tools/testing/selftests/bpf/progs/test_tunnel_kern.c:#define
VXLAN_UDP_PORT 4789
tools/testing/selftests/bpf/progs/sendmsg4_prog.c:#define DST_PORT
         4040
tools/testing/selftests/bpf/progs/sendmsg4_prog.c:#define
DST_REWRITE_PORT4     4444
tools/testing/selftests/bpf/progs/connect4_prog.c:#define
DST_REWRITE_PORT4     4444
tools/testing/selftests/bpf/progs/bind6_prog.c:#define SERV6_PORT
         6060
tools/testing/selftests/bpf/progs/bind6_prog.c:#define
SERV6_REWRITE_PORT       6666
tools/testing/selftests/bpf/progs/sendmsg6_prog.c:#define
DST_REWRITE_PORT6     6666
tools/testing/selftests/bpf/progs/recvmsg4_prog.c:#define SERV4_PORT
         4040
<cut>

.... there is much more ...

> >Getting the address of the listener (bound to
> > port 0) and passing it to the bpf program via global variable should be super
> > easy now (with the skeletons and network_helpers).
>
> Just so that we are on the same page, could you point to which network helpers are you referring to here for passing global variables?

Take a look at the following existing tests:
* prog_tests/cgroup_skb_sk_lookup.c
  * run_lookup_test(&skel->bss->g_serv_port, out_sk);
* progs/cgroup_skb_sk_lookup_kern.c
  * g_serv_port

Fundamentally, here is what's preferable to have:

fd = start_server(..., port=0, ...);
listener_port = get_port(fd); /* new network_helpers.h helper that
calls getsockname */
obj->bss->port = listener_port; /* populate the port in the BPF program */

Does it make sense?

> >> >
> >> > And, unrelated, maybe also fix a bunch of places where the reverse christmas
> >> > tree doesn't look reverse anymore?
> >
> >> Ok. The checks should be part of tooling (e.g., checkpatch) though if they are meant to be enforced consistently, no?
> >
> > They are networking specific, so they are not part of a checkpath :-(
> > I won't say they are consistently enforced, but we try to keep then
> > whenever possible.
> >
> >> >
> >> >> +
> >> >> +int bpf_sock_destroy(struct sock_common *sk) __ksym;
> >> >> +
> >> >> +struct {
> >> >> + __uint(type, BPF_MAP_TYPE_ARRAY);
> >> >> + __uint(max_entries, 1);
> >> >> + __type(key, __u32);
> >> >> + __type(value, __u64);
> >> >> +} tcp_conn_sockets SEC(".maps");
> >> >> +
> >> >> +struct {
> >> >> + __uint(type, BPF_MAP_TYPE_ARRAY);
> >> >> + __uint(max_entries, 1);
> >> >> + __type(key, __u32);
> >> >> + __type(value, __u64);
> >> >> +} udp_conn_sockets SEC(".maps");
> >> >> +
> >> >> +SEC("cgroup/connect6")
> >> >> +int sock_connect(struct bpf_sock_addr *ctx)
> >> >> +{
> >> >> + int key = 0;
> >> >> + __u64 sock_cookie = 0;
> >> >> + __u32 keyc = 0;
> >> >> +
> >> >> + if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
> >> >> +         return 1;
> >> >> +
> >> >> + sock_cookie = bpf_get_socket_cookie(ctx);
> >> >> + if (ctx->protocol == IPPROTO_TCP)
> >> >> +         bpf_map_update_elem(&tcp_conn_sockets, &key, &sock_cookie, 0);
> >> >> + else if (ctx->protocol == IPPROTO_UDP)
> >> >> +         bpf_map_update_elem(&udp_conn_sockets, &keyc, &sock_cookie, 0);
> >> >> + else
> >> >> +         return 1;
> >> >> +
> >> >> + return 1;
> >> >> +}
> >> >> +
> >> >> +SEC("iter/tcp")
> >> >> +int iter_tcp6_client(struct bpf_iter__tcp *ctx)
> >> >> +{
> >> >> + struct sock_common *sk_common = ctx->sk_common;
> >> >> + struct seq_file *seq = ctx->meta->seq;
> >> >> + __u64 sock_cookie = 0;
> >> >> + __u64 *val;
> >> >> + int key = 0;
> >> >> +
> >> >> + if (!sk_common)
> >> >> +         return 0;
> >> >> +
> >> >> + if (sk_common->skc_family != AF_INET6)
> >> >> +         return 0;
> >> >> +
> >> >> + sock_cookie  = bpf_get_socket_cookie(sk_common);
> >> >> + val = bpf_map_lookup_elem(&tcp_conn_sockets, &key);
> >> >> + if (!val)
> >> >> +         return 0;
> >> >> + /* Destroy connected client sockets. */
> >> >> + if (sock_cookie == *val)
> >> >> +         bpf_sock_destroy(sk_common);
> >> >> +
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +SEC("iter/tcp")
> >> >> +int iter_tcp6_server(struct bpf_iter__tcp *ctx)
> >> >> +{
> >> >> + struct sock_common *sk_common = ctx->sk_common;
> >> >> + struct seq_file *seq = ctx->meta->seq;
> >> >> + struct tcp6_sock *tcp_sk;
> >> >> + const struct inet_connection_sock *icsk;
> >> >> + const struct inet_sock *inet;
> >> >> + __u16 srcp;
> >> >> +
> >> >> + if (!sk_common)
> >> >> +         return 0;
> >> >> +
> >> >> + if (sk_common->skc_family != AF_INET6)
> >> >> +         return 0;
> >> >> +
> >> >> + tcp_sk = bpf_skc_to_tcp6_sock(sk_common);
> >> >> + if (!tcp_sk)
> >> >> +         return 0;
> >> >> +
> >> >> + icsk = &tcp_sk->tcp.inet_conn;
> >> >> + inet = &icsk->icsk_inet;
> >> >> + srcp = bpf_ntohs(inet->inet_sport);
> >> >> +
> >> >> + /* Destroy server sockets. */
> >> >> + if (srcp == SERVER_PORT)
> >> >> +         bpf_sock_destroy(sk_common);
> >> >> +
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +
> >> >> +SEC("iter/udp")
> >> >> +int iter_udp6_client(struct bpf_iter__udp *ctx)
> >> >> +{
> >> >> + struct seq_file *seq = ctx->meta->seq;
> >> >> + struct udp_sock *udp_sk = ctx->udp_sk;
> >> >> + struct sock *sk = (struct sock *) udp_sk;
> >> >> + __u64 sock_cookie = 0, *val;
> >> >> + int key = 0;
> >> >> +
> >> >> + if (!sk)
> >> >> +         return 0;
> >> >> +
> >> >> + sock_cookie  = bpf_get_socket_cookie(sk);
> >> >> + val = bpf_map_lookup_elem(&udp_conn_sockets, &key);
> >> >> + if (!val)
> >> >> +         return 0;
> >> >> + /* Destroy connected client sockets. */
> >> >> + if (sock_cookie == *val)
> >> >> +         bpf_sock_destroy((struct sock_common *)sk);
> >> >> +
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +SEC("iter/udp")
> >> >> +int iter_udp6_server(struct bpf_iter__udp *ctx)
> >> >> +{
> >> >> + struct seq_file *seq = ctx->meta->seq;
> >> >> + struct udp_sock *udp_sk = ctx->udp_sk;
> >> >> + struct sock *sk = (struct sock *) udp_sk;
> >> >> + __u16 srcp;
> >> >> + struct inet_sock *inet;
> >> >> +
> >> >> + if (!sk)
> >> >> +         return 0;
> >> >> +
> >> >> + inet = &udp_sk->inet;
> >> >> + srcp = bpf_ntohs(inet->inet_sport);
> >> >> + if (srcp == SERVER_PORT)
> >> >> +         bpf_sock_destroy((struct sock_common *)sk);
> >> >> +
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +char _license[] SEC("license") = "GPL";
> >> >> --
> >> >> 2.34.1
>

Aditi Ghag March 29, 2023, 11:13 p.m. UTC | #6

> On Mar 28, 2023, at 11:35 AM, Stanislav Fomichev <[e-mail]> wrote:
> 
> On Tue, Mar 28, 2023 at 10:51 AM Aditi Ghag <[e-mail]> wrote:
>> 
>> 
>> 
>>> On Mar 27, 2023, at 9:54 AM, Stanislav Fomichev <[e-mail]> wrote:
>>> 
>>> On 03/27, Aditi Ghag wrote:
>>> 
>>> 
>>>>> On Mar 24, 2023, at 2:52 PM, Stanislav Fomichev <[e-mail]> wrote:
>>>>> 
>>>>> On 03/23, Aditi Ghag wrote:
>>>>>> The test cases for destroying sockets mirror the intended usages of the
>>>>>> bpf_sock_destroy kfunc using iterators.
>>>>> 
>>>>>> The destroy helpers set `ECONNABORTED` error code that we can validate in
>>>>>> the test code with client sockets. But UDP sockets have an overriding error
>>>>>> code from the disconnect called during abort, so the error code the
>>>>>> validation is only done for TCP sockets.
>>>>> 
>>>>>> Signed-off-by: Aditi Ghag <[e-mail]>
>>>>>> ---
>>>>>> .../selftests/bpf/prog_tests/sock_destroy.c   | 195 ++++++++++++++++++
>>>>>> .../selftests/bpf/progs/sock_destroy_prog.c   | 151 ++++++++++++++
>>>>>> 2 files changed, 346 insertions(+)
>>>>>> create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>>>>>> create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>>>>> 
>>>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..cbce966af568
>>>>>> --- /dev/null
>>>>>> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>>>>>> @@ -0,0 +1,195 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +#include <test_progs.h>
>>>>>> +
>>>>>> +#include "sock_destroy_prog.skel.h"
>>>>>> +#include "network_helpers.h"
>>>>>> +
>>>>>> +#define SERVER_PORT 6062
>>>>>> +
>>>>>> +static void start_iter_sockets(struct bpf_program *prog)
>>>>>> +{
>>>>>> + struct bpf_link *link;
>>>>>> + char buf[50] = {};
>>>>>> + int iter_fd, len;
>>>>>> +
>>>>>> + link = bpf_program__attach_iter(prog, NULL);
>>>>>> + if (!ASSERT_OK_PTR(link, "attach_iter"))
>>>>>> +         return;
>>>>>> +
>>>>>> + iter_fd = bpf_iter_create(bpf_link__fd(link));
>>>>>> + if (!ASSERT_GE(iter_fd, 0, "create_iter"))
>>>>>> +         goto free_link;
>>>>>> +
>>>>>> + while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
>>>>>> +         ;
>>>>>> + ASSERT_GE(len, 0, "read");
>>>>>> +
>>>>>> + close(iter_fd);
>>>>>> +
>>>>>> +free_link:
>>>>>> + bpf_link__destroy(link);
>>>>>> +}
>>>>>> +
>>>>>> +static void test_tcp_client(struct sock_destroy_prog *skel)
>>>>>> +{
>>>>>> + int serv = -1, clien = -1, n = 0;
>>>>>> +
>>>>>> + serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
>>>>>> + if (!ASSERT_GE(serv, 0, "start_server"))
>>>>>> +         goto cleanup_serv;
>>>>>> +
>>>>>> + clien = connect_to_fd(serv, 0);
>>>>>> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
>>>>>> +         goto cleanup_serv;
>>>>>> +
>>>>>> + serv = accept(serv, NULL, NULL);
>>>>>> + if (!ASSERT_GE(serv, 0, "serv accept"))
>>>>>> +         goto cleanup;
>>>>>> +
>>>>>> + n = send(clien, "t", 1, 0);
>>>>>> + if (!ASSERT_GE(n, 0, "client send"))
>>>>>> +         goto cleanup;
>>>>>> +
>>>>>> + /* Run iterator program that destroys connected client sockets. */
>>>>>> + start_iter_sockets(skel->progs.iter_tcp6_client);
>>>>>> +
>>>>>> + n = send(clien, "t", 1, 0);
>>>>>> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
>>>>>> +         goto cleanup;
>>>>>> + ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket");
>>>>>> +
>>>>>> +
>>>>>> +cleanup:
>>>>>> + close(clien);
>>>>>> +cleanup_serv:
>>>>>> + close(serv);
>>>>>> +}
>>>>>> +
>>>>>> +static void test_tcp_server(struct sock_destroy_prog *skel)
>>>>>> +{
>>>>>> + int serv = -1, clien = -1, n = 0;
>>>>>> +
>>>>>> + serv = start_server(AF_INET6, SOCK_STREAM, NULL, SERVER_PORT, 0);
>>>>>> + if (!ASSERT_GE(serv, 0, "start_server"))
>>>>>> +         goto cleanup_serv;
>>>>>> +
>>>>>> + clien = connect_to_fd(serv, 0);
>>>>>> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
>>>>>> +         goto cleanup_serv;
>>>>>> +
>>>>>> + serv = accept(serv, NULL, NULL);
>>>>>> + if (!ASSERT_GE(serv, 0, "serv accept"))
>>>>>> +         goto cleanup;
>>>>>> +
>>>>>> + n = send(clien, "t", 1, 0);
>>>>>> + if (!ASSERT_GE(n, 0, "client send"))
>>>>>> +         goto cleanup;
>>>>>> +
>>>>>> + /* Run iterator program that destroys server sockets. */
>>>>>> + start_iter_sockets(skel->progs.iter_tcp6_server);
>>>>>> +
>>>>>> + n = send(clien, "t", 1, 0);
>>>>>> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
>>>>>> +         goto cleanup;
>>>>>> + ASSERT_EQ(errno, ECONNRESET, "error code on destroyed socket");
>>>>>> +
>>>>>> +
>>>>>> +cleanup:
>>>>>> + close(clien);
>>>>>> +cleanup_serv:
>>>>>> + close(serv);
>>>>>> +}
>>>>>> +
>>>>>> +
>>>>>> +static void test_udp_client(struct sock_destroy_prog *skel)
>>>>>> +{
>>>>>> + int serv = -1, clien = -1, n = 0;
>>>>>> +
>>>>>> + serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0);
>>>>>> + if (!ASSERT_GE(serv, 0, "start_server"))
>>>>>> +         goto cleanup_serv;
>>>>>> +
>>>>>> + clien = connect_to_fd(serv, 0);
>>>>>> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
>>>>>> +         goto cleanup_serv;
>>>>>> +
>>>>>> + n = send(clien, "t", 1, 0);
>>>>>> + if (!ASSERT_GE(n, 0, "client send"))
>>>>>> +         goto cleanup;
>>>>>> +
>>>>>> + /* Run iterator program that destroys sockets. */
>>>>>> + start_iter_sockets(skel->progs.iter_udp6_client);
>>>>>> +
>>>>>> + n = send(clien, "t", 1, 0);
>>>>>> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
>>>>>> +         goto cleanup;
>>>>>> + /* UDP sockets have an overriding error code after they are disconnected,
>>>>>> +  * so we don't check for ECONNABORTED error code.
>>>>>> +  */
>>>>>> +
>>>>>> +cleanup:
>>>>>> + close(clien);
>>>>>> +cleanup_serv:
>>>>>> + close(serv);
>>>>>> +}
>>>>>> +
>>>>>> +static void test_udp_server(struct sock_destroy_prog *skel)
>>>>>> +{
>>>>>> + int *listen_fds = NULL, n, i;
>>>>>> + unsigned int num_listens = 5;
>>>>>> + char buf[1];
>>>>>> +
>>>>>> + /* Start reuseport servers. */
>>>>>> + listen_fds = start_reuseport_server(AF_INET6, SOCK_DGRAM,
>>>>>> +                                     "::1", SERVER_PORT, 0,
>>>>>> +                                     num_listens);
>>>>>> + if (!ASSERT_OK_PTR(listen_fds, "start_reuseport_server"))
>>>>>> +         goto cleanup;
>>>>>> +
>>>>>> + /* Run iterator program that destroys server sockets. */
>>>>>> + start_iter_sockets(skel->progs.iter_udp6_server);
>>>>>> +
>>>>>> + for (i = 0; i < num_listens; ++i) {
>>>>>> +         n = read(listen_fds[i], buf, sizeof(buf));
>>>>>> +         if (!ASSERT_EQ(n, -1, "read") ||
>>>>>> +             !ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket"))
>>>>>> +                 break;
>>>>>> + }
>>>>>> + ASSERT_EQ(i, num_listens, "server socket");
>>>>>> +
>>>>>> +cleanup:
>>>>>> + free_fds(listen_fds, num_listens);
>>>>>> +}
>>>>>> +
>>>>>> +void test_sock_destroy(void)
>>>>>> +{
>>>>>> + int cgroup_fd = 0;
>>>>>> + struct sock_destroy_prog *skel;
>>>>>> +
>>>>>> + skel = sock_destroy_prog__open_and_load();
>>>>>> + if (!ASSERT_OK_PTR(skel, "skel_open"))
>>>>>> +         return;
>>>>>> +
>>>>>> + cgroup_fd = test__join_cgroup("/sock_destroy");
>>>>>> + if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
>>>>>> +         goto close_cgroup_fd;
>>>>>> +
>>>>>> + skel->links.sock_connect = bpf_program__attach_cgroup(
>>>>>> +         skel->progs.sock_connect, cgroup_fd);
>>>>>> + if (!ASSERT_OK_PTR(skel->links.sock_connect, "prog_attach"))
>>>>>> +         goto close_cgroup_fd;
>>>>>> +
>>>>>> + if (test__start_subtest("tcp_client"))
>>>>>> +         test_tcp_client(skel);
>>>>>> + if (test__start_subtest("tcp_server"))
>>>>>> +         test_tcp_server(skel);
>>>>>> + if (test__start_subtest("udp_client"))
>>>>>> +         test_udp_client(skel);
>>>>>> + if (test__start_subtest("udp_server"))
>>>>>> +         test_udp_server(skel);
>>>>>> +
>>>>>> +
>>>>>> +close_cgroup_fd:
>>>>>> + close(cgroup_fd);
>>>>>> + sock_destroy_prog__destroy(skel);
>>>>>> +}
>>>>>> diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..8e09d82c50f3
>>>>>> --- /dev/null
>>>>>> +++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>>>>>> @@ -0,0 +1,151 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +
>>>>>> +#include "vmlinux.h"
>>>>>> +
>>>>>> +#include "bpf_tracing_net.h"
>>>>>> +#include <bpf/bpf_helpers.h>
>>>>>> +#include <bpf/bpf_endian.h>
>>>>>> +
>>>>>> +#define AF_INET6 10
>>>>> 
>>>>> [..]
>>>>> 
>>>>>> +/* Keep it in sync with prog_test/sock_destroy. */
>>>>>> +#define SERVER_PORT 6062
>>>>> 
>>>>> The test looks good, one optional unrelated nit maybe:
>>>>> 
>>>>> I've been guilty of these hard-coded ports in the past, but maybe
>>>>> we should stop hard-coding them? Getting the address of the listener (bound to
>>>>> port 0) and passing it to the bpf program via global variable should be super
>>>>> easy now (with the skeletons and network_helpers).
>>> 
>>> 
>>>> I briefly considered adding the ports in a map, and retrieving them in the test. But it didn't seem worthwhile as the tests should fail clearly when there is a mismatch.
>>> 
>>> My worry is that the amount of those tests that have a hard-coded port
>>> grows and at some point somebody will clash with somebody else.
>>> And it might not be 100% apparent because test_progs is now multi-threaded
>>> and racy..
>>> 
>> 
>> So you would like the ports to be unique across all the tests.
> 
> Yeah, but it's hard without having some kind of global registry. Take
> a look at the following:
> 
> $ grep -Iri _port tools/testing/selftests/bpf/ | grep -P '\d{4}'
> 
> tools/testing/selftests/bpf/progs/connect_force_port4.c:
> sa.sin_port = bpf_htons(22222);
> tools/testing/selftests/bpf/progs/connect_force_port4.c:        if
> (ctx->user_port == bpf_htons(60000)) {
> tools/testing/selftests/bpf/progs/connect_force_port4.c:
> ctx->user_port = bpf_htons(60123);
> tools/testing/selftests/bpf/progs/connect_force_port4.c:        if
> (ctx->user_port == bpf_htons(60123)) {
> tools/testing/selftests/bpf/progs/connect_force_port4.c:
> ctx->user_port = bpf_htons(60000);
> tools/testing/selftests/bpf/progs/connect_force_port4.c:        if
> (ctx->user_port == bpf_htons(60123)) {
> tools/testing/selftests/bpf/progs/connect6_prog.c:#define
> DST_REWRITE_PORT6     6666
> tools/testing/selftests/bpf/progs/test_sk_lookup.c:static const __u16
> SRC_PORT = bpf_htons(8008);
> tools/testing/selftests/bpf/progs/test_sk_lookup.c:static const __u16
> DST_PORT = 7007; /* Host byte order */
> tools/testing/selftests/bpf/progs/test_tc_dtime.c:      __u16
> dst_ns_port = __bpf_htons(50000 + test);
> tools/testing/selftests/bpf/progs/connect4_dropper.c:   if
> (ctx->user_port == bpf_htons(60120))
> tools/testing/selftests/bpf/progs/connect_force_port6.c:
> sa.sin6_port = bpf_htons(22223);
> tools/testing/selftests/bpf/progs/connect_force_port6.c:        if
> (ctx->user_port == bpf_htons(60000)) {
> tools/testing/selftests/bpf/progs/connect_force_port6.c:
> ctx->user_port = bpf_htons(60124);
> tools/testing/selftests/bpf/progs/connect_force_port6.c:        if
> (ctx->user_port == bpf_htons(60124)) {
> tools/testing/selftests/bpf/progs/connect_force_port6.c:
> ctx->user_port = bpf_htons(60000);
> tools/testing/selftests/bpf/progs/connect_force_port6.c:        if
> (ctx->user_port == bpf_htons(60124)) {
> tools/testing/selftests/bpf/progs/test_tunnel_kern.c:#define
> VXLAN_UDP_PORT 4789
> tools/testing/selftests/bpf/progs/sendmsg4_prog.c:#define DST_PORT
>         4040
> tools/testing/selftests/bpf/progs/sendmsg4_prog.c:#define
> DST_REWRITE_PORT4     4444
> tools/testing/selftests/bpf/progs/connect4_prog.c:#define
> DST_REWRITE_PORT4     4444
> tools/testing/selftests/bpf/progs/bind6_prog.c:#define SERV6_PORT
>         6060
> tools/testing/selftests/bpf/progs/bind6_prog.c:#define
> SERV6_REWRITE_PORT       6666
> tools/testing/selftests/bpf/progs/sendmsg6_prog.c:#define
> DST_REWRITE_PORT6     6666
> tools/testing/selftests/bpf/progs/recvmsg4_prog.c:#define SERV4_PORT
>         4040
> <cut>
> 
> .... there is much more ...
> 
>>> Getting the address of the listener (bound to
>>> port 0) and passing it to the bpf program via global variable should be super
>>> easy now (with the skeletons and network_helpers).
>> 
>> Just so that we are on the same page, could you point to which network helpers are you referring to here for passing global variables?
> 
> Take a look at the following existing tests:
> * prog_tests/cgroup_skb_sk_lookup.c
>  * run_lookup_test(&skel->bss->g_serv_port, out_sk);
> * progs/cgroup_skb_sk_lookup_kern.c
>  * g_serv_port
> 
> Fundamentally, here is what's preferable to have:
> 
> fd = start_server(..., port=0, ...);
> listener_port = get_port(fd); /* new network_helpers.h helper that
> calls getsockname */
> obj->bss->port = listener_port; /* populate the port in the BPF program */
> 
> Does it make sense?

That makes sense. Good to know for future references. The client tests don't have hard-coded ports anyway, only the server tests do as they are using the so_resuseport option. You did mention that this was an optional nit, so I'll leave the hard-coded ports for the server tests for now. Hope that's reasonable.

> 
>>>>> 
>>>>> And, unrelated, maybe also fix a bunch of places where the reverse christmas
>>>>> tree doesn't look reverse anymore?
>>> 
>>>> Ok. The checks should be part of tooling (e.g., checkpatch) though if they are meant to be enforced consistently, no?
>>> 
>>> They are networking specific, so they are not part of a checkpath :-(
>>> I won't say they are consistently enforced, but we try to keep then
>>> whenever possible.
>>> 
>>>>> 
>>>>>> +
>>>>>> +int bpf_sock_destroy(struct sock_common *sk) __ksym;
>>>>>> +
>>>>>> +struct {
>>>>>> + __uint(type, BPF_MAP_TYPE_ARRAY);
>>>>>> + __uint(max_entries, 1);
>>>>>> + __type(key, __u32);
>>>>>> + __type(value, __u64);
>>>>>> +} tcp_conn_sockets SEC(".maps");
>>>>>> +
>>>>>> +struct {
>>>>>> + __uint(type, BPF_MAP_TYPE_ARRAY);
>>>>>> + __uint(max_entries, 1);
>>>>>> + __type(key, __u32);
>>>>>> + __type(value, __u64);
>>>>>> +} udp_conn_sockets SEC(".maps");
>>>>>> +
>>>>>> +SEC("cgroup/connect6")
>>>>>> +int sock_connect(struct bpf_sock_addr *ctx)
>>>>>> +{
>>>>>> + int key = 0;
>>>>>> + __u64 sock_cookie = 0;
>>>>>> + __u32 keyc = 0;
>>>>>> +
>>>>>> + if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
>>>>>> +         return 1;
>>>>>> +
>>>>>> + sock_cookie = bpf_get_socket_cookie(ctx);
>>>>>> + if (ctx->protocol == IPPROTO_TCP)
>>>>>> +         bpf_map_update_elem(&tcp_conn_sockets, &key, &sock_cookie, 0);
>>>>>> + else if (ctx->protocol == IPPROTO_UDP)
>>>>>> +         bpf_map_update_elem(&udp_conn_sockets, &keyc, &sock_cookie, 0);
>>>>>> + else
>>>>>> +         return 1;
>>>>>> +
>>>>>> + return 1;
>>>>>> +}
>>>>>> +
>>>>>> +SEC("iter/tcp")
>>>>>> +int iter_tcp6_client(struct bpf_iter__tcp *ctx)
>>>>>> +{
>>>>>> + struct sock_common *sk_common = ctx->sk_common;
>>>>>> + struct seq_file *seq = ctx->meta->seq;
>>>>>> + __u64 sock_cookie = 0;
>>>>>> + __u64 *val;
>>>>>> + int key = 0;
>>>>>> +
>>>>>> + if (!sk_common)
>>>>>> +         return 0;
>>>>>> +
>>>>>> + if (sk_common->skc_family != AF_INET6)
>>>>>> +         return 0;
>>>>>> +
>>>>>> + sock_cookie  = bpf_get_socket_cookie(sk_common);
>>>>>> + val = bpf_map_lookup_elem(&tcp_conn_sockets, &key);
>>>>>> + if (!val)
>>>>>> +         return 0;
>>>>>> + /* Destroy connected client sockets. */
>>>>>> + if (sock_cookie == *val)
>>>>>> +         bpf_sock_destroy(sk_common);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +SEC("iter/tcp")
>>>>>> +int iter_tcp6_server(struct bpf_iter__tcp *ctx)
>>>>>> +{
>>>>>> + struct sock_common *sk_common = ctx->sk_common;
>>>>>> + struct seq_file *seq = ctx->meta->seq;
>>>>>> + struct tcp6_sock *tcp_sk;
>>>>>> + const struct inet_connection_sock *icsk;
>>>>>> + const struct inet_sock *inet;
>>>>>> + __u16 srcp;
>>>>>> +
>>>>>> + if (!sk_common)
>>>>>> +         return 0;
>>>>>> +
>>>>>> + if (sk_common->skc_family != AF_INET6)
>>>>>> +         return 0;
>>>>>> +
>>>>>> + tcp_sk = bpf_skc_to_tcp6_sock(sk_common);
>>>>>> + if (!tcp_sk)
>>>>>> +         return 0;
>>>>>> +
>>>>>> + icsk = &tcp_sk->tcp.inet_conn;
>>>>>> + inet = &icsk->icsk_inet;
>>>>>> + srcp = bpf_ntohs(inet->inet_sport);
>>>>>> +
>>>>>> + /* Destroy server sockets. */
>>>>>> + if (srcp == SERVER_PORT)
>>>>>> +         bpf_sock_destroy(sk_common);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +
>>>>>> +SEC("iter/udp")
>>>>>> +int iter_udp6_client(struct bpf_iter__udp *ctx)
>>>>>> +{
>>>>>> + struct seq_file *seq = ctx->meta->seq;
>>>>>> + struct udp_sock *udp_sk = ctx->udp_sk;
>>>>>> + struct sock *sk = (struct sock *) udp_sk;
>>>>>> + __u64 sock_cookie = 0, *val;
>>>>>> + int key = 0;
>>>>>> +
>>>>>> + if (!sk)
>>>>>> +         return 0;
>>>>>> +
>>>>>> + sock_cookie  = bpf_get_socket_cookie(sk);
>>>>>> + val = bpf_map_lookup_elem(&udp_conn_sockets, &key);
>>>>>> + if (!val)
>>>>>> +         return 0;
>>>>>> + /* Destroy connected client sockets. */
>>>>>> + if (sock_cookie == *val)
>>>>>> +         bpf_sock_destroy((struct sock_common *)sk);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +SEC("iter/udp")
>>>>>> +int iter_udp6_server(struct bpf_iter__udp *ctx)
>>>>>> +{
>>>>>> + struct seq_file *seq = ctx->meta->seq;
>>>>>> + struct udp_sock *udp_sk = ctx->udp_sk;
>>>>>> + struct sock *sk = (struct sock *) udp_sk;
>>>>>> + __u16 srcp;
>>>>>> + struct inet_sock *inet;
>>>>>> +
>>>>>> + if (!sk)
>>>>>> +         return 0;
>>>>>> +
>>>>>> + inet = &udp_sk->inet;
>>>>>> + srcp = bpf_ntohs(inet->inet_sport);
>>>>>> + if (srcp == SERVER_PORT)
>>>>>> +         bpf_sock_destroy((struct sock_common *)sk);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +char _license[] SEC("license") = "GPL";
>>>>>> --
>>>>>> 2.34.1

Aditi Ghag March 29, 2023, 11:25 p.m. UTC | #7

> On Mar 29, 2023, at 4:13 PM, Aditi Ghag <[e-mail]> wrote:
> 
> 
> 
>> On Mar 28, 2023, at 11:35 AM, Stanislav Fomichev <[e-mail]> wrote:
>> 
>> On Tue, Mar 28, 2023 at 10:51 AM Aditi Ghag <[e-mail]> wrote:
>>> 
>>> 
>>> 
>>>> On Mar 27, 2023, at 9:54 AM, Stanislav Fomichev <[e-mail]> wrote:
>>>> 
>>>> On 03/27, Aditi Ghag wrote:
>>>> 
>>>> 
>>>>>> On Mar 24, 2023, at 2:52 PM, Stanislav Fomichev <[e-mail]> wrote:
>>>>>> 
>>>>>> On 03/23, Aditi Ghag wrote:
>>>>>>> The test cases for destroying sockets mirror the intended usages of the
>>>>>>> bpf_sock_destroy kfunc using iterators.
>>>>>> 
>>>>>>> The destroy helpers set `ECONNABORTED` error code that we can validate in
>>>>>>> the test code with client sockets. But UDP sockets have an overriding error
>>>>>>> code from the disconnect called during abort, so the error code the
>>>>>>> validation is only done for TCP sockets.
>>>>>> 
>>>>>>> Signed-off-by: Aditi Ghag <[e-mail]>
>>>>>>> ---
>>>>>>> .../selftests/bpf/prog_tests/sock_destroy.c   | 195 ++++++++++++++++++
>>>>>>> .../selftests/bpf/progs/sock_destroy_prog.c   | 151 ++++++++++++++
>>>>>>> 2 files changed, 346 insertions(+)
>>>>>>> create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>>>>>>> create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>>>>>> 
>>>>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..cbce966af568
>>>>>>> --- /dev/null
>>>>>>> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>>>>>>> @@ -0,0 +1,195 @@
>>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>>> +#include <test_progs.h>
>>>>>>> +
>>>>>>> +#include "sock_destroy_prog.skel.h"
>>>>>>> +#include "network_helpers.h"
>>>>>>> +
>>>>>>> +#define SERVER_PORT 6062
>>>>>>> +
>>>>>>> +static void start_iter_sockets(struct bpf_program *prog)
>>>>>>> +{
>>>>>>> + struct bpf_link *link;
>>>>>>> + char buf[50] = {};
>>>>>>> + int iter_fd, len;
>>>>>>> +
>>>>>>> + link = bpf_program__attach_iter(prog, NULL);
>>>>>>> + if (!ASSERT_OK_PTR(link, "attach_iter"))
>>>>>>> +         return;
>>>>>>> +
>>>>>>> + iter_fd = bpf_iter_create(bpf_link__fd(link));
>>>>>>> + if (!ASSERT_GE(iter_fd, 0, "create_iter"))
>>>>>>> +         goto free_link;
>>>>>>> +
>>>>>>> + while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
>>>>>>> +         ;
>>>>>>> + ASSERT_GE(len, 0, "read");
>>>>>>> +
>>>>>>> + close(iter_fd);
>>>>>>> +
>>>>>>> +free_link:
>>>>>>> + bpf_link__destroy(link);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void test_tcp_client(struct sock_destroy_prog *skel)
>>>>>>> +{
>>>>>>> + int serv = -1, clien = -1, n = 0;
>>>>>>> +
>>>>>>> + serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
>>>>>>> + if (!ASSERT_GE(serv, 0, "start_server"))
>>>>>>> +         goto cleanup_serv;
>>>>>>> +
>>>>>>> + clien = connect_to_fd(serv, 0);
>>>>>>> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
>>>>>>> +         goto cleanup_serv;
>>>>>>> +
>>>>>>> + serv = accept(serv, NULL, NULL);
>>>>>>> + if (!ASSERT_GE(serv, 0, "serv accept"))
>>>>>>> +         goto cleanup;
>>>>>>> +
>>>>>>> + n = send(clien, "t", 1, 0);
>>>>>>> + if (!ASSERT_GE(n, 0, "client send"))
>>>>>>> +         goto cleanup;
>>>>>>> +
>>>>>>> + /* Run iterator program that destroys connected client sockets. */
>>>>>>> + start_iter_sockets(skel->progs.iter_tcp6_client);
>>>>>>> +
>>>>>>> + n = send(clien, "t", 1, 0);
>>>>>>> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
>>>>>>> +         goto cleanup;
>>>>>>> + ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket");
>>>>>>> +
>>>>>>> +
>>>>>>> +cleanup:
>>>>>>> + close(clien);
>>>>>>> +cleanup_serv:
>>>>>>> + close(serv);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void test_tcp_server(struct sock_destroy_prog *skel)
>>>>>>> +{
>>>>>>> + int serv = -1, clien = -1, n = 0;
>>>>>>> +
>>>>>>> + serv = start_server(AF_INET6, SOCK_STREAM, NULL, SERVER_PORT, 0);
>>>>>>> + if (!ASSERT_GE(serv, 0, "start_server"))
>>>>>>> +         goto cleanup_serv;
>>>>>>> +
>>>>>>> + clien = connect_to_fd(serv, 0);
>>>>>>> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
>>>>>>> +         goto cleanup_serv;
>>>>>>> +
>>>>>>> + serv = accept(serv, NULL, NULL);
>>>>>>> + if (!ASSERT_GE(serv, 0, "serv accept"))
>>>>>>> +         goto cleanup;
>>>>>>> +
>>>>>>> + n = send(clien, "t", 1, 0);
>>>>>>> + if (!ASSERT_GE(n, 0, "client send"))
>>>>>>> +         goto cleanup;
>>>>>>> +
>>>>>>> + /* Run iterator program that destroys server sockets. */
>>>>>>> + start_iter_sockets(skel->progs.iter_tcp6_server);
>>>>>>> +
>>>>>>> + n = send(clien, "t", 1, 0);
>>>>>>> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
>>>>>>> +         goto cleanup;
>>>>>>> + ASSERT_EQ(errno, ECONNRESET, "error code on destroyed socket");
>>>>>>> +
>>>>>>> +
>>>>>>> +cleanup:
>>>>>>> + close(clien);
>>>>>>> +cleanup_serv:
>>>>>>> + close(serv);
>>>>>>> +}
>>>>>>> +
>>>>>>> +
>>>>>>> +static void test_udp_client(struct sock_destroy_prog *skel)
>>>>>>> +{
>>>>>>> + int serv = -1, clien = -1, n = 0;
>>>>>>> +
>>>>>>> + serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0);
>>>>>>> + if (!ASSERT_GE(serv, 0, "start_server"))
>>>>>>> +         goto cleanup_serv;
>>>>>>> +
>>>>>>> + clien = connect_to_fd(serv, 0);
>>>>>>> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
>>>>>>> +         goto cleanup_serv;
>>>>>>> +
>>>>>>> + n = send(clien, "t", 1, 0);
>>>>>>> + if (!ASSERT_GE(n, 0, "client send"))
>>>>>>> +         goto cleanup;
>>>>>>> +
>>>>>>> + /* Run iterator program that destroys sockets. */
>>>>>>> + start_iter_sockets(skel->progs.iter_udp6_client);
>>>>>>> +
>>>>>>> + n = send(clien, "t", 1, 0);
>>>>>>> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
>>>>>>> +         goto cleanup;
>>>>>>> + /* UDP sockets have an overriding error code after they are disconnected,
>>>>>>> +  * so we don't check for ECONNABORTED error code.
>>>>>>> +  */
>>>>>>> +
>>>>>>> +cleanup:
>>>>>>> + close(clien);
>>>>>>> +cleanup_serv:
>>>>>>> + close(serv);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void test_udp_server(struct sock_destroy_prog *skel)
>>>>>>> +{
>>>>>>> + int *listen_fds = NULL, n, i;
>>>>>>> + unsigned int num_listens = 5;
>>>>>>> + char buf[1];
>>>>>>> +
>>>>>>> + /* Start reuseport servers. */
>>>>>>> + listen_fds = start_reuseport_server(AF_INET6, SOCK_DGRAM,
>>>>>>> +                                     "::1", SERVER_PORT, 0,
>>>>>>> +                                     num_listens);
>>>>>>> + if (!ASSERT_OK_PTR(listen_fds, "start_reuseport_server"))
>>>>>>> +         goto cleanup;
>>>>>>> +
>>>>>>> + /* Run iterator program that destroys server sockets. */
>>>>>>> + start_iter_sockets(skel->progs.iter_udp6_server);
>>>>>>> +
>>>>>>> + for (i = 0; i < num_listens; ++i) {
>>>>>>> +         n = read(listen_fds[i], buf, sizeof(buf));
>>>>>>> +         if (!ASSERT_EQ(n, -1, "read") ||
>>>>>>> +             !ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket"))
>>>>>>> +                 break;
>>>>>>> + }
>>>>>>> + ASSERT_EQ(i, num_listens, "server socket");
>>>>>>> +
>>>>>>> +cleanup:
>>>>>>> + free_fds(listen_fds, num_listens);
>>>>>>> +}
>>>>>>> +
>>>>>>> +void test_sock_destroy(void)
>>>>>>> +{
>>>>>>> + int cgroup_fd = 0;
>>>>>>> + struct sock_destroy_prog *skel;
>>>>>>> +
>>>>>>> + skel = sock_destroy_prog__open_and_load();
>>>>>>> + if (!ASSERT_OK_PTR(skel, "skel_open"))
>>>>>>> +         return;
>>>>>>> +
>>>>>>> + cgroup_fd = test__join_cgroup("/sock_destroy");
>>>>>>> + if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
>>>>>>> +         goto close_cgroup_fd;
>>>>>>> +
>>>>>>> + skel->links.sock_connect = bpf_program__attach_cgroup(
>>>>>>> +         skel->progs.sock_connect, cgroup_fd);
>>>>>>> + if (!ASSERT_OK_PTR(skel->links.sock_connect, "prog_attach"))
>>>>>>> +         goto close_cgroup_fd;
>>>>>>> +
>>>>>>> + if (test__start_subtest("tcp_client"))
>>>>>>> +         test_tcp_client(skel);
>>>>>>> + if (test__start_subtest("tcp_server"))
>>>>>>> +         test_tcp_server(skel);
>>>>>>> + if (test__start_subtest("udp_client"))
>>>>>>> +         test_udp_client(skel);
>>>>>>> + if (test__start_subtest("udp_server"))
>>>>>>> +         test_udp_server(skel);
>>>>>>> +
>>>>>>> +
>>>>>>> +close_cgroup_fd:
>>>>>>> + close(cgroup_fd);
>>>>>>> + sock_destroy_prog__destroy(skel);
>>>>>>> +}
>>>>>>> diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..8e09d82c50f3
>>>>>>> --- /dev/null
>>>>>>> +++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>>>>>>> @@ -0,0 +1,151 @@
>>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>>> +
>>>>>>> +#include "vmlinux.h"
>>>>>>> +
>>>>>>> +#include "bpf_tracing_net.h"
>>>>>>> +#include <bpf/bpf_helpers.h>
>>>>>>> +#include <bpf/bpf_endian.h>
>>>>>>> +
>>>>>>> +#define AF_INET6 10
>>>>>> 
>>>>>> [..]
>>>>>> 
>>>>>>> +/* Keep it in sync with prog_test/sock_destroy. */
>>>>>>> +#define SERVER_PORT 6062
>>>>>> 
>>>>>> The test looks good, one optional unrelated nit maybe:
>>>>>> 
>>>>>> I've been guilty of these hard-coded ports in the past, but maybe
>>>>>> we should stop hard-coding them? Getting the address of the listener (bound to
>>>>>> port 0) and passing it to the bpf program via global variable should be super
>>>>>> easy now (with the skeletons and network_helpers).
>>>> 
>>>> 
>>>>> I briefly considered adding the ports in a map, and retrieving them in the test. But it didn't seem worthwhile as the tests should fail clearly when there is a mismatch.
>>>> 
>>>> My worry is that the amount of those tests that have a hard-coded port
>>>> grows and at some point somebody will clash with somebody else.
>>>> And it might not be 100% apparent because test_progs is now multi-threaded
>>>> and racy..
>>>> 
>>> 
>>> So you would like the ports to be unique across all the tests.
>> 
>> Yeah, but it's hard without having some kind of global registry. Take
>> a look at the following:
>> 
>> $ grep -Iri _port tools/testing/selftests/bpf/ | grep -P '\d{4}'
>> 
>> tools/testing/selftests/bpf/progs/connect_force_port4.c:
>> sa.sin_port = bpf_htons(22222);
>> tools/testing/selftests/bpf/progs/connect_force_port4.c:        if
>> (ctx->user_port == bpf_htons(60000)) {
>> tools/testing/selftests/bpf/progs/connect_force_port4.c:
>> ctx->user_port = bpf_htons(60123);
>> tools/testing/selftests/bpf/progs/connect_force_port4.c:        if
>> (ctx->user_port == bpf_htons(60123)) {
>> tools/testing/selftests/bpf/progs/connect_force_port4.c:
>> ctx->user_port = bpf_htons(60000);
>> tools/testing/selftests/bpf/progs/connect_force_port4.c:        if
>> (ctx->user_port == bpf_htons(60123)) {
>> tools/testing/selftests/bpf/progs/connect6_prog.c:#define
>> DST_REWRITE_PORT6     6666
>> tools/testing/selftests/bpf/progs/test_sk_lookup.c:static const __u16
>> SRC_PORT = bpf_htons(8008);
>> tools/testing/selftests/bpf/progs/test_sk_lookup.c:static const __u16
>> DST_PORT = 7007; /* Host byte order */
>> tools/testing/selftests/bpf/progs/test_tc_dtime.c:      __u16
>> dst_ns_port = __bpf_htons(50000 + test);
>> tools/testing/selftests/bpf/progs/connect4_dropper.c:   if
>> (ctx->user_port == bpf_htons(60120))
>> tools/testing/selftests/bpf/progs/connect_force_port6.c:
>> sa.sin6_port = bpf_htons(22223);
>> tools/testing/selftests/bpf/progs/connect_force_port6.c:        if
>> (ctx->user_port == bpf_htons(60000)) {
>> tools/testing/selftests/bpf/progs/connect_force_port6.c:
>> ctx->user_port = bpf_htons(60124);
>> tools/testing/selftests/bpf/progs/connect_force_port6.c:        if
>> (ctx->user_port == bpf_htons(60124)) {
>> tools/testing/selftests/bpf/progs/connect_force_port6.c:
>> ctx->user_port = bpf_htons(60000);
>> tools/testing/selftests/bpf/progs/connect_force_port6.c:        if
>> (ctx->user_port == bpf_htons(60124)) {
>> tools/testing/selftests/bpf/progs/test_tunnel_kern.c:#define
>> VXLAN_UDP_PORT 4789
>> tools/testing/selftests/bpf/progs/sendmsg4_prog.c:#define DST_PORT
>>        4040
>> tools/testing/selftests/bpf/progs/sendmsg4_prog.c:#define
>> DST_REWRITE_PORT4     4444
>> tools/testing/selftests/bpf/progs/connect4_prog.c:#define
>> DST_REWRITE_PORT4     4444
>> tools/testing/selftests/bpf/progs/bind6_prog.c:#define SERV6_PORT
>>        6060
>> tools/testing/selftests/bpf/progs/bind6_prog.c:#define
>> SERV6_REWRITE_PORT       6666
>> tools/testing/selftests/bpf/progs/sendmsg6_prog.c:#define
>> DST_REWRITE_PORT6     6666
>> tools/testing/selftests/bpf/progs/recvmsg4_prog.c:#define SERV4_PORT
>>        4040
>> <cut>
>> 
>> .... there is much more ...
>> 
>>>> Getting the address of the listener (bound to
>>>> port 0) and passing it to the bpf program via global variable should be super
>>>> easy now (with the skeletons and network_helpers).
>>> 
>>> Just so that we are on the same page, could you point to which network helpers are you referring to here for passing global variables?
>> 
>> Take a look at the following existing tests:
>> * prog_tests/cgroup_skb_sk_lookup.c
>> * run_lookup_test(&skel->bss->g_serv_port, out_sk);
>> * progs/cgroup_skb_sk_lookup_kern.c
>> * g_serv_port
>> 
>> Fundamentally, here is what's preferable to have:
>> 
>> fd = start_server(..., port=0, ...);
>> listener_port = get_port(fd); /* new network_helpers.h helper that
>> calls getsockname */
>> obj->bss->port = listener_port; /* populate the port in the BPF program */
>> 
>> Does it make sense?
> 
> That makes sense. Good to know for future references. The client tests don't have hard-coded ports anyway, only the server tests do as they are using the so_resuseport option. You did mention that this was an optional nit, so I'll leave the hard-coded ports for the server tests for now. Hope that's reasonable.
> 

Looks like that shouldn't be a problem as you can pass port 0 to start_reuseport_server. I'll give this a shot, as it looks like a better option than contributing to the long list of hard-coded ports that you pointed out above. 

>> 
>>>>>> 
>>>>>> And, unrelated, maybe also fix a bunch of places where the reverse christmas
>>>>>> tree doesn't look reverse anymore?
>>>> 
>>>>> Ok. The checks should be part of tooling (e.g., checkpatch) though if they are meant to be enforced consistently, no?
>>>> 
>>>> They are networking specific, so they are not part of a checkpath :-(
>>>> I won't say they are consistently enforced, but we try to keep then
>>>> whenever possible.
>>>> 
>>>>>> 
>>>>>>> +
>>>>>>> +int bpf_sock_destroy(struct sock_common *sk) __ksym;
>>>>>>> +
>>>>>>> +struct {
>>>>>>> + __uint(type, BPF_MAP_TYPE_ARRAY);
>>>>>>> + __uint(max_entries, 1);
>>>>>>> + __type(key, __u32);
>>>>>>> + __type(value, __u64);
>>>>>>> +} tcp_conn_sockets SEC(".maps");
>>>>>>> +
>>>>>>> +struct {
>>>>>>> + __uint(type, BPF_MAP_TYPE_ARRAY);
>>>>>>> + __uint(max_entries, 1);
>>>>>>> + __type(key, __u32);
>>>>>>> + __type(value, __u64);
>>>>>>> +} udp_conn_sockets SEC(".maps");
>>>>>>> +
>>>>>>> +SEC("cgroup/connect6")
>>>>>>> +int sock_connect(struct bpf_sock_addr *ctx)
>>>>>>> +{
>>>>>>> + int key = 0;
>>>>>>> + __u64 sock_cookie = 0;
>>>>>>> + __u32 keyc = 0;
>>>>>>> +
>>>>>>> + if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
>>>>>>> +         return 1;
>>>>>>> +
>>>>>>> + sock_cookie = bpf_get_socket_cookie(ctx);
>>>>>>> + if (ctx->protocol == IPPROTO_TCP)
>>>>>>> +         bpf_map_update_elem(&tcp_conn_sockets, &key, &sock_cookie, 0);
>>>>>>> + else if (ctx->protocol == IPPROTO_UDP)
>>>>>>> +         bpf_map_update_elem(&udp_conn_sockets, &keyc, &sock_cookie, 0);
>>>>>>> + else
>>>>>>> +         return 1;
>>>>>>> +
>>>>>>> + return 1;
>>>>>>> +}
>>>>>>> +
>>>>>>> +SEC("iter/tcp")
>>>>>>> +int iter_tcp6_client(struct bpf_iter__tcp *ctx)
>>>>>>> +{
>>>>>>> + struct sock_common *sk_common = ctx->sk_common;
>>>>>>> + struct seq_file *seq = ctx->meta->seq;
>>>>>>> + __u64 sock_cookie = 0;
>>>>>>> + __u64 *val;
>>>>>>> + int key = 0;
>>>>>>> +
>>>>>>> + if (!sk_common)
>>>>>>> +         return 0;
>>>>>>> +
>>>>>>> + if (sk_common->skc_family != AF_INET6)
>>>>>>> +         return 0;
>>>>>>> +
>>>>>>> + sock_cookie  = bpf_get_socket_cookie(sk_common);
>>>>>>> + val = bpf_map_lookup_elem(&tcp_conn_sockets, &key);
>>>>>>> + if (!val)
>>>>>>> +         return 0;
>>>>>>> + /* Destroy connected client sockets. */
>>>>>>> + if (sock_cookie == *val)
>>>>>>> +         bpf_sock_destroy(sk_common);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +SEC("iter/tcp")
>>>>>>> +int iter_tcp6_server(struct bpf_iter__tcp *ctx)
>>>>>>> +{
>>>>>>> + struct sock_common *sk_common = ctx->sk_common;
>>>>>>> + struct seq_file *seq = ctx->meta->seq;
>>>>>>> + struct tcp6_sock *tcp_sk;
>>>>>>> + const struct inet_connection_sock *icsk;
>>>>>>> + const struct inet_sock *inet;
>>>>>>> + __u16 srcp;
>>>>>>> +
>>>>>>> + if (!sk_common)
>>>>>>> +         return 0;
>>>>>>> +
>>>>>>> + if (sk_common->skc_family != AF_INET6)
>>>>>>> +         return 0;
>>>>>>> +
>>>>>>> + tcp_sk = bpf_skc_to_tcp6_sock(sk_common);
>>>>>>> + if (!tcp_sk)
>>>>>>> +         return 0;
>>>>>>> +
>>>>>>> + icsk = &tcp_sk->tcp.inet_conn;
>>>>>>> + inet = &icsk->icsk_inet;
>>>>>>> + srcp = bpf_ntohs(inet->inet_sport);
>>>>>>> +
>>>>>>> + /* Destroy server sockets. */
>>>>>>> + if (srcp == SERVER_PORT)
>>>>>>> +         bpf_sock_destroy(sk_common);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +
>>>>>>> +SEC("iter/udp")
>>>>>>> +int iter_udp6_client(struct bpf_iter__udp *ctx)
>>>>>>> +{
>>>>>>> + struct seq_file *seq = ctx->meta->seq;
>>>>>>> + struct udp_sock *udp_sk = ctx->udp_sk;
>>>>>>> + struct sock *sk = (struct sock *) udp_sk;
>>>>>>> + __u64 sock_cookie = 0, *val;
>>>>>>> + int key = 0;
>>>>>>> +
>>>>>>> + if (!sk)
>>>>>>> +         return 0;
>>>>>>> +
>>>>>>> + sock_cookie  = bpf_get_socket_cookie(sk);
>>>>>>> + val = bpf_map_lookup_elem(&udp_conn_sockets, &key);
>>>>>>> + if (!val)
>>>>>>> +         return 0;
>>>>>>> + /* Destroy connected client sockets. */
>>>>>>> + if (sock_cookie == *val)
>>>>>>> +         bpf_sock_destroy((struct sock_common *)sk);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +SEC("iter/udp")
>>>>>>> +int iter_udp6_server(struct bpf_iter__udp *ctx)
>>>>>>> +{
>>>>>>> + struct seq_file *seq = ctx->meta->seq;
>>>>>>> + struct udp_sock *udp_sk = ctx->udp_sk;
>>>>>>> + struct sock *sk = (struct sock *) udp_sk;
>>>>>>> + __u16 srcp;
>>>>>>> + struct inet_sock *inet;
>>>>>>> +
>>>>>>> + if (!sk)
>>>>>>> +         return 0;
>>>>>>> +
>>>>>>> + inet = &udp_sk->inet;
>>>>>>> + srcp = bpf_ntohs(inet->inet_sport);
>>>>>>> + if (srcp == SERVER_PORT)
>>>>>>> +         bpf_sock_destroy((struct sock_common *)sk);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +char _license[] SEC("license") = "GPL";
>>>>>>> --
>>>>>>> 2.34.1

Stanislav Fomichev March 29, 2023, 11:25 p.m. UTC | #8

On Wed, Mar 29, 2023 at 4:13 PM Aditi Ghag <[e-mail]> wrote:
>
>
>
> > On Mar 28, 2023, at 11:35 AM, Stanislav Fomichev <[e-mail]> wrote:
> >
> > On Tue, Mar 28, 2023 at 10:51 AM Aditi Ghag <[e-mail]> wrote:
> >>
> >>
> >>
> >>> On Mar 27, 2023, at 9:54 AM, Stanislav Fomichev <[e-mail]> wrote:
> >>>
> >>> On 03/27, Aditi Ghag wrote:
> >>>
> >>>
> >>>>> On Mar 24, 2023, at 2:52 PM, Stanislav Fomichev <[e-mail]> wrote:
> >>>>>
> >>>>> On 03/23, Aditi Ghag wrote:
> >>>>>> The test cases for destroying sockets mirror the intended usages of the
> >>>>>> bpf_sock_destroy kfunc using iterators.
> >>>>>
> >>>>>> The destroy helpers set `ECONNABORTED` error code that we can validate in
> >>>>>> the test code with client sockets. But UDP sockets have an overriding error
> >>>>>> code from the disconnect called during abort, so the error code the
> >>>>>> validation is only done for TCP sockets.
> >>>>>
> >>>>>> Signed-off-by: Aditi Ghag <[e-mail]>
> >>>>>> ---
> >>>>>> .../selftests/bpf/prog_tests/sock_destroy.c   | 195 ++++++++++++++++++
> >>>>>> .../selftests/bpf/progs/sock_destroy_prog.c   | 151 ++++++++++++++
> >>>>>> 2 files changed, 346 insertions(+)
> >>>>>> create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> >>>>>> create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c
> >>>>>
> >>>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..cbce966af568
> >>>>>> --- /dev/null
> >>>>>> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> >>>>>> @@ -0,0 +1,195 @@
> >>>>>> +// SPDX-License-Identifier: GPL-2.0
> >>>>>> +#include <test_progs.h>
> >>>>>> +
> >>>>>> +#include "sock_destroy_prog.skel.h"
> >>>>>> +#include "network_helpers.h"
> >>>>>> +
> >>>>>> +#define SERVER_PORT 6062
> >>>>>> +
> >>>>>> +static void start_iter_sockets(struct bpf_program *prog)
> >>>>>> +{
> >>>>>> + struct bpf_link *link;
> >>>>>> + char buf[50] = {};
> >>>>>> + int iter_fd, len;
> >>>>>> +
> >>>>>> + link = bpf_program__attach_iter(prog, NULL);
> >>>>>> + if (!ASSERT_OK_PTR(link, "attach_iter"))
> >>>>>> +         return;
> >>>>>> +
> >>>>>> + iter_fd = bpf_iter_create(bpf_link__fd(link));
> >>>>>> + if (!ASSERT_GE(iter_fd, 0, "create_iter"))
> >>>>>> +         goto free_link;
> >>>>>> +
> >>>>>> + while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
> >>>>>> +         ;
> >>>>>> + ASSERT_GE(len, 0, "read");
> >>>>>> +
> >>>>>> + close(iter_fd);
> >>>>>> +
> >>>>>> +free_link:
> >>>>>> + bpf_link__destroy(link);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void test_tcp_client(struct sock_destroy_prog *skel)
> >>>>>> +{
> >>>>>> + int serv = -1, clien = -1, n = 0;
> >>>>>> +
> >>>>>> + serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
> >>>>>> + if (!ASSERT_GE(serv, 0, "start_server"))
> >>>>>> +         goto cleanup_serv;
> >>>>>> +
> >>>>>> + clien = connect_to_fd(serv, 0);
> >>>>>> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> >>>>>> +         goto cleanup_serv;
> >>>>>> +
> >>>>>> + serv = accept(serv, NULL, NULL);
> >>>>>> + if (!ASSERT_GE(serv, 0, "serv accept"))
> >>>>>> +         goto cleanup;
> >>>>>> +
> >>>>>> + n = send(clien, "t", 1, 0);
> >>>>>> + if (!ASSERT_GE(n, 0, "client send"))
> >>>>>> +         goto cleanup;
> >>>>>> +
> >>>>>> + /* Run iterator program that destroys connected client sockets. */
> >>>>>> + start_iter_sockets(skel->progs.iter_tcp6_client);
> >>>>>> +
> >>>>>> + n = send(clien, "t", 1, 0);
> >>>>>> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> >>>>>> +         goto cleanup;
> >>>>>> + ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket");
> >>>>>> +
> >>>>>> +
> >>>>>> +cleanup:
> >>>>>> + close(clien);
> >>>>>> +cleanup_serv:
> >>>>>> + close(serv);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void test_tcp_server(struct sock_destroy_prog *skel)
> >>>>>> +{
> >>>>>> + int serv = -1, clien = -1, n = 0;
> >>>>>> +
> >>>>>> + serv = start_server(AF_INET6, SOCK_STREAM, NULL, SERVER_PORT, 0);
> >>>>>> + if (!ASSERT_GE(serv, 0, "start_server"))
> >>>>>> +         goto cleanup_serv;
> >>>>>> +
> >>>>>> + clien = connect_to_fd(serv, 0);
> >>>>>> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> >>>>>> +         goto cleanup_serv;
> >>>>>> +
> >>>>>> + serv = accept(serv, NULL, NULL);
> >>>>>> + if (!ASSERT_GE(serv, 0, "serv accept"))
> >>>>>> +         goto cleanup;
> >>>>>> +
> >>>>>> + n = send(clien, "t", 1, 0);
> >>>>>> + if (!ASSERT_GE(n, 0, "client send"))
> >>>>>> +         goto cleanup;
> >>>>>> +
> >>>>>> + /* Run iterator program that destroys server sockets. */
> >>>>>> + start_iter_sockets(skel->progs.iter_tcp6_server);
> >>>>>> +
> >>>>>> + n = send(clien, "t", 1, 0);
> >>>>>> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> >>>>>> +         goto cleanup;
> >>>>>> + ASSERT_EQ(errno, ECONNRESET, "error code on destroyed socket");
> >>>>>> +
> >>>>>> +
> >>>>>> +cleanup:
> >>>>>> + close(clien);
> >>>>>> +cleanup_serv:
> >>>>>> + close(serv);
> >>>>>> +}
> >>>>>> +
> >>>>>> +
> >>>>>> +static void test_udp_client(struct sock_destroy_prog *skel)
> >>>>>> +{
> >>>>>> + int serv = -1, clien = -1, n = 0;
> >>>>>> +
> >>>>>> + serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0);
> >>>>>> + if (!ASSERT_GE(serv, 0, "start_server"))
> >>>>>> +         goto cleanup_serv;
> >>>>>> +
> >>>>>> + clien = connect_to_fd(serv, 0);
> >>>>>> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> >>>>>> +         goto cleanup_serv;
> >>>>>> +
> >>>>>> + n = send(clien, "t", 1, 0);
> >>>>>> + if (!ASSERT_GE(n, 0, "client send"))
> >>>>>> +         goto cleanup;
> >>>>>> +
> >>>>>> + /* Run iterator program that destroys sockets. */
> >>>>>> + start_iter_sockets(skel->progs.iter_udp6_client);
> >>>>>> +
> >>>>>> + n = send(clien, "t", 1, 0);
> >>>>>> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> >>>>>> +         goto cleanup;
> >>>>>> + /* UDP sockets have an overriding error code after they are disconnected,
> >>>>>> +  * so we don't check for ECONNABORTED error code.
> >>>>>> +  */
> >>>>>> +
> >>>>>> +cleanup:
> >>>>>> + close(clien);
> >>>>>> +cleanup_serv:
> >>>>>> + close(serv);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void test_udp_server(struct sock_destroy_prog *skel)
> >>>>>> +{
> >>>>>> + int *listen_fds = NULL, n, i;
> >>>>>> + unsigned int num_listens = 5;
> >>>>>> + char buf[1];
> >>>>>> +
> >>>>>> + /* Start reuseport servers. */
> >>>>>> + listen_fds = start_reuseport_server(AF_INET6, SOCK_DGRAM,
> >>>>>> +                                     "::1", SERVER_PORT, 0,
> >>>>>> +                                     num_listens);
> >>>>>> + if (!ASSERT_OK_PTR(listen_fds, "start_reuseport_server"))
> >>>>>> +         goto cleanup;
> >>>>>> +
> >>>>>> + /* Run iterator program that destroys server sockets. */
> >>>>>> + start_iter_sockets(skel->progs.iter_udp6_server);
> >>>>>> +
> >>>>>> + for (i = 0; i < num_listens; ++i) {
> >>>>>> +         n = read(listen_fds[i], buf, sizeof(buf));
> >>>>>> +         if (!ASSERT_EQ(n, -1, "read") ||
> >>>>>> +             !ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket"))
> >>>>>> +                 break;
> >>>>>> + }
> >>>>>> + ASSERT_EQ(i, num_listens, "server socket");
> >>>>>> +
> >>>>>> +cleanup:
> >>>>>> + free_fds(listen_fds, num_listens);
> >>>>>> +}
> >>>>>> +
> >>>>>> +void test_sock_destroy(void)
> >>>>>> +{
> >>>>>> + int cgroup_fd = 0;
> >>>>>> + struct sock_destroy_prog *skel;
> >>>>>> +
> >>>>>> + skel = sock_destroy_prog__open_and_load();
> >>>>>> + if (!ASSERT_OK_PTR(skel, "skel_open"))
> >>>>>> +         return;
> >>>>>> +
> >>>>>> + cgroup_fd = test__join_cgroup("/sock_destroy");
> >>>>>> + if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
> >>>>>> +         goto close_cgroup_fd;
> >>>>>> +
> >>>>>> + skel->links.sock_connect = bpf_program__attach_cgroup(
> >>>>>> +         skel->progs.sock_connect, cgroup_fd);
> >>>>>> + if (!ASSERT_OK_PTR(skel->links.sock_connect, "prog_attach"))
> >>>>>> +         goto close_cgroup_fd;
> >>>>>> +
> >>>>>> + if (test__start_subtest("tcp_client"))
> >>>>>> +         test_tcp_client(skel);
> >>>>>> + if (test__start_subtest("tcp_server"))
> >>>>>> +         test_tcp_server(skel);
> >>>>>> + if (test__start_subtest("udp_client"))
> >>>>>> +         test_udp_client(skel);
> >>>>>> + if (test__start_subtest("udp_server"))
> >>>>>> +         test_udp_server(skel);
> >>>>>> +
> >>>>>> +
> >>>>>> +close_cgroup_fd:
> >>>>>> + close(cgroup_fd);
> >>>>>> + sock_destroy_prog__destroy(skel);
> >>>>>> +}
> >>>>>> diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..8e09d82c50f3
> >>>>>> --- /dev/null
> >>>>>> +++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
> >>>>>> @@ -0,0 +1,151 @@
> >>>>>> +// SPDX-License-Identifier: GPL-2.0
> >>>>>> +
> >>>>>> +#include "vmlinux.h"
> >>>>>> +
> >>>>>> +#include "bpf_tracing_net.h"
> >>>>>> +#include <bpf/bpf_helpers.h>
> >>>>>> +#include <bpf/bpf_endian.h>
> >>>>>> +
> >>>>>> +#define AF_INET6 10
> >>>>>
> >>>>> [..]
> >>>>>
> >>>>>> +/* Keep it in sync with prog_test/sock_destroy. */
> >>>>>> +#define SERVER_PORT 6062
> >>>>>
> >>>>> The test looks good, one optional unrelated nit maybe:
> >>>>>
> >>>>> I've been guilty of these hard-coded ports in the past, but maybe
> >>>>> we should stop hard-coding them? Getting the address of the listener (bound to
> >>>>> port 0) and passing it to the bpf program via global variable should be super
> >>>>> easy now (with the skeletons and network_helpers).
> >>>
> >>>
> >>>> I briefly considered adding the ports in a map, and retrieving them in the test. But it didn't seem worthwhile as the tests should fail clearly when there is a mismatch.
> >>>
> >>> My worry is that the amount of those tests that have a hard-coded port
> >>> grows and at some point somebody will clash with somebody else.
> >>> And it might not be 100% apparent because test_progs is now multi-threaded
> >>> and racy..
> >>>
> >>
> >> So you would like the ports to be unique across all the tests.
> >
> > Yeah, but it's hard without having some kind of global registry. Take
> > a look at the following:
> >
> > $ grep -Iri _port tools/testing/selftests/bpf/ | grep -P '\d{4}'
> >
> > tools/testing/selftests/bpf/progs/connect_force_port4.c:
> > sa.sin_port = bpf_htons(22222);
> > tools/testing/selftests/bpf/progs/connect_force_port4.c:        if
> > (ctx->user_port == bpf_htons(60000)) {
> > tools/testing/selftests/bpf/progs/connect_force_port4.c:
> > ctx->user_port = bpf_htons(60123);
> > tools/testing/selftests/bpf/progs/connect_force_port4.c:        if
> > (ctx->user_port == bpf_htons(60123)) {
> > tools/testing/selftests/bpf/progs/connect_force_port4.c:
> > ctx->user_port = bpf_htons(60000);
> > tools/testing/selftests/bpf/progs/connect_force_port4.c:        if
> > (ctx->user_port == bpf_htons(60123)) {
> > tools/testing/selftests/bpf/progs/connect6_prog.c:#define
> > DST_REWRITE_PORT6     6666
> > tools/testing/selftests/bpf/progs/test_sk_lookup.c:static const __u16
> > SRC_PORT = bpf_htons(8008);
> > tools/testing/selftests/bpf/progs/test_sk_lookup.c:static const __u16
> > DST_PORT = 7007; /* Host byte order */
> > tools/testing/selftests/bpf/progs/test_tc_dtime.c:      __u16
> > dst_ns_port = __bpf_htons(50000 + test);
> > tools/testing/selftests/bpf/progs/connect4_dropper.c:   if
> > (ctx->user_port == bpf_htons(60120))
> > tools/testing/selftests/bpf/progs/connect_force_port6.c:
> > sa.sin6_port = bpf_htons(22223);
> > tools/testing/selftests/bpf/progs/connect_force_port6.c:        if
> > (ctx->user_port == bpf_htons(60000)) {
> > tools/testing/selftests/bpf/progs/connect_force_port6.c:
> > ctx->user_port = bpf_htons(60124);
> > tools/testing/selftests/bpf/progs/connect_force_port6.c:        if
> > (ctx->user_port == bpf_htons(60124)) {
> > tools/testing/selftests/bpf/progs/connect_force_port6.c:
> > ctx->user_port = bpf_htons(60000);
> > tools/testing/selftests/bpf/progs/connect_force_port6.c:        if
> > (ctx->user_port == bpf_htons(60124)) {
> > tools/testing/selftests/bpf/progs/test_tunnel_kern.c:#define
> > VXLAN_UDP_PORT 4789
> > tools/testing/selftests/bpf/progs/sendmsg4_prog.c:#define DST_PORT
> >         4040
> > tools/testing/selftests/bpf/progs/sendmsg4_prog.c:#define
> > DST_REWRITE_PORT4     4444
> > tools/testing/selftests/bpf/progs/connect4_prog.c:#define
> > DST_REWRITE_PORT4     4444
> > tools/testing/selftests/bpf/progs/bind6_prog.c:#define SERV6_PORT
> >         6060
> > tools/testing/selftests/bpf/progs/bind6_prog.c:#define
> > SERV6_REWRITE_PORT       6666
> > tools/testing/selftests/bpf/progs/sendmsg6_prog.c:#define
> > DST_REWRITE_PORT6     6666
> > tools/testing/selftests/bpf/progs/recvmsg4_prog.c:#define SERV4_PORT
> >         4040
> > <cut>
> >
> > .... there is much more ...
> >
> >>> Getting the address of the listener (bound to
> >>> port 0) and passing it to the bpf program via global variable should be super
> >>> easy now (with the skeletons and network_helpers).
> >>
> >> Just so that we are on the same page, could you point to which network helpers are you referring to here for passing global variables?
> >
> > Take a look at the following existing tests:
> > * prog_tests/cgroup_skb_sk_lookup.c
> >  * run_lookup_test(&skel->bss->g_serv_port, out_sk);
> > * progs/cgroup_skb_sk_lookup_kern.c
> >  * g_serv_port
> >
> > Fundamentally, here is what's preferable to have:
> >
> > fd = start_server(..., port=0, ...);
> > listener_port = get_port(fd); /* new network_helpers.h helper that
> > calls getsockname */
> > obj->bss->port = listener_port; /* populate the port in the BPF program */
> >
> > Does it make sense?
>
> That makes sense. Good to know for future references. The client tests don't have hard-coded ports anyway, only the server tests do as they are using the so_resuseport option. You did mention that this was an optional nit, so I'll leave the hard-coded ports for the server tests for now. Hope that's reasonable.

Sure, up to you, but to clarify. You have the following:

+static void test_tcp_server(struct sock_destroy_prog *skel)
+{
+ int serv = -1, clien = -1, n = 0;
+
+ serv = start_server(AF_INET6, SOCK_STREAM, NULL, SERVER_PORT, 0);

And the following:

+static void test_udp_client(struct sock_destroy_prog *skel)
+{
+ int serv = -1, clien = -1, n = 0;
+
+ serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0);

Both have hard-coded ports and not using reuseport?

> >
> >>>>>
> >>>>> And, unrelated, maybe also fix a bunch of places where the reverse christmas
> >>>>> tree doesn't look reverse anymore?
> >>>
> >>>> Ok. The checks should be part of tooling (e.g., checkpatch) though if they are meant to be enforced consistently, no?
> >>>
> >>> They are networking specific, so they are not part of a checkpath :-(
> >>> I won't say they are consistently enforced, but we try to keep then
> >>> whenever possible.
> >>>
> >>>>>
> >>>>>> +
> >>>>>> +int bpf_sock_destroy(struct sock_common *sk) __ksym;
> >>>>>> +
> >>>>>> +struct {
> >>>>>> + __uint(type, BPF_MAP_TYPE_ARRAY);
> >>>>>> + __uint(max_entries, 1);
> >>>>>> + __type(key, __u32);
> >>>>>> + __type(value, __u64);
> >>>>>> +} tcp_conn_sockets SEC(".maps");
> >>>>>> +
> >>>>>> +struct {
> >>>>>> + __uint(type, BPF_MAP_TYPE_ARRAY);
> >>>>>> + __uint(max_entries, 1);
> >>>>>> + __type(key, __u32);
> >>>>>> + __type(value, __u64);
> >>>>>> +} udp_conn_sockets SEC(".maps");
> >>>>>> +
> >>>>>> +SEC("cgroup/connect6")
> >>>>>> +int sock_connect(struct bpf_sock_addr *ctx)
> >>>>>> +{
> >>>>>> + int key = 0;
> >>>>>> + __u64 sock_cookie = 0;
> >>>>>> + __u32 keyc = 0;
> >>>>>> +
> >>>>>> + if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
> >>>>>> +         return 1;
> >>>>>> +
> >>>>>> + sock_cookie = bpf_get_socket_cookie(ctx);
> >>>>>> + if (ctx->protocol == IPPROTO_TCP)
> >>>>>> +         bpf_map_update_elem(&tcp_conn_sockets, &key, &sock_cookie, 0);
> >>>>>> + else if (ctx->protocol == IPPROTO_UDP)
> >>>>>> +         bpf_map_update_elem(&udp_conn_sockets, &keyc, &sock_cookie, 0);
> >>>>>> + else
> >>>>>> +         return 1;
> >>>>>> +
> >>>>>> + return 1;
> >>>>>> +}
> >>>>>> +
> >>>>>> +SEC("iter/tcp")
> >>>>>> +int iter_tcp6_client(struct bpf_iter__tcp *ctx)
> >>>>>> +{
> >>>>>> + struct sock_common *sk_common = ctx->sk_common;
> >>>>>> + struct seq_file *seq = ctx->meta->seq;
> >>>>>> + __u64 sock_cookie = 0;
> >>>>>> + __u64 *val;
> >>>>>> + int key = 0;
> >>>>>> +
> >>>>>> + if (!sk_common)
> >>>>>> +         return 0;
> >>>>>> +
> >>>>>> + if (sk_common->skc_family != AF_INET6)
> >>>>>> +         return 0;
> >>>>>> +
> >>>>>> + sock_cookie  = bpf_get_socket_cookie(sk_common);
> >>>>>> + val = bpf_map_lookup_elem(&tcp_conn_sockets, &key);
> >>>>>> + if (!val)
> >>>>>> +         return 0;
> >>>>>> + /* Destroy connected client sockets. */
> >>>>>> + if (sock_cookie == *val)
> >>>>>> +         bpf_sock_destroy(sk_common);
> >>>>>> +
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +SEC("iter/tcp")
> >>>>>> +int iter_tcp6_server(struct bpf_iter__tcp *ctx)
> >>>>>> +{
> >>>>>> + struct sock_common *sk_common = ctx->sk_common;
> >>>>>> + struct seq_file *seq = ctx->meta->seq;
> >>>>>> + struct tcp6_sock *tcp_sk;
> >>>>>> + const struct inet_connection_sock *icsk;
> >>>>>> + const struct inet_sock *inet;
> >>>>>> + __u16 srcp;
> >>>>>> +
> >>>>>> + if (!sk_common)
> >>>>>> +         return 0;
> >>>>>> +
> >>>>>> + if (sk_common->skc_family != AF_INET6)
> >>>>>> +         return 0;
> >>>>>> +
> >>>>>> + tcp_sk = bpf_skc_to_tcp6_sock(sk_common);
> >>>>>> + if (!tcp_sk)
> >>>>>> +         return 0;
> >>>>>> +
> >>>>>> + icsk = &tcp_sk->tcp.inet_conn;
> >>>>>> + inet = &icsk->icsk_inet;
> >>>>>> + srcp = bpf_ntohs(inet->inet_sport);
> >>>>>> +
> >>>>>> + /* Destroy server sockets. */
> >>>>>> + if (srcp == SERVER_PORT)
> >>>>>> +         bpf_sock_destroy(sk_common);
> >>>>>> +
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +
> >>>>>> +SEC("iter/udp")
> >>>>>> +int iter_udp6_client(struct bpf_iter__udp *ctx)
> >>>>>> +{
> >>>>>> + struct seq_file *seq = ctx->meta->seq;
> >>>>>> + struct udp_sock *udp_sk = ctx->udp_sk;
> >>>>>> + struct sock *sk = (struct sock *) udp_sk;
> >>>>>> + __u64 sock_cookie = 0, *val;
> >>>>>> + int key = 0;
> >>>>>> +
> >>>>>> + if (!sk)
> >>>>>> +         return 0;
> >>>>>> +
> >>>>>> + sock_cookie  = bpf_get_socket_cookie(sk);
> >>>>>> + val = bpf_map_lookup_elem(&udp_conn_sockets, &key);
> >>>>>> + if (!val)
> >>>>>> +         return 0;
> >>>>>> + /* Destroy connected client sockets. */
> >>>>>> + if (sock_cookie == *val)
> >>>>>> +         bpf_sock_destroy((struct sock_common *)sk);
> >>>>>> +
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +SEC("iter/udp")
> >>>>>> +int iter_udp6_server(struct bpf_iter__udp *ctx)
> >>>>>> +{
> >>>>>> + struct seq_file *seq = ctx->meta->seq;
> >>>>>> + struct udp_sock *udp_sk = ctx->udp_sk;
> >>>>>> + struct sock *sk = (struct sock *) udp_sk;
> >>>>>> + __u16 srcp;
> >>>>>> + struct inet_sock *inet;
> >>>>>> +
> >>>>>> + if (!sk)
> >>>>>> +         return 0;
> >>>>>> +
> >>>>>> + inet = &udp_sk->inet;
> >>>>>> + srcp = bpf_ntohs(inet->inet_sport);
> >>>>>> + if (srcp == SERVER_PORT)
> >>>>>> +         bpf_sock_destroy((struct sock_common *)sk);
> >>>>>> +
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +char _license[] SEC("license") = "GPL";
> >>>>>> --
> >>>>>> 2.34.1
>

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
new file mode 100644
index 000000000000..cbce966af568
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
@@ -0,0 +1,195 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+#include "sock_destroy_prog.skel.h"
+#include "network_helpers.h"
+
+#define SERVER_PORT 6062
+
+static void start_iter_sockets(struct bpf_program *prog)
+{
+	struct bpf_link *link;
+	char buf[50] = {};
+	int iter_fd, len;
+
+	link = bpf_program__attach_iter(prog, NULL);
+	if (!ASSERT_OK_PTR(link, "attach_iter"))
+		return;
+
+	iter_fd = bpf_iter_create(bpf_link__fd(link));
+	if (!ASSERT_GE(iter_fd, 0, "create_iter"))
+		goto free_link;
+
+	while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
+		;
+	ASSERT_GE(len, 0, "read");
+
+	close(iter_fd);
+
+free_link:
+	bpf_link__destroy(link);
+}
+
+static void test_tcp_client(struct sock_destroy_prog *skel)
+{
+	int serv = -1, clien = -1, n = 0;
+
+	serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
+	if (!ASSERT_GE(serv, 0, "start_server"))
+		goto cleanup_serv;
+
+	clien = connect_to_fd(serv, 0);
+	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
+		goto cleanup_serv;
+
+	serv = accept(serv, NULL, NULL);
+	if (!ASSERT_GE(serv, 0, "serv accept"))
+		goto cleanup;
+
+	n = send(clien, "t", 1, 0);
+	if (!ASSERT_GE(n, 0, "client send"))
+		goto cleanup;
+
+	/* Run iterator program that destroys connected client sockets. */
+	start_iter_sockets(skel->progs.iter_tcp6_client);
+
+	n = send(clien, "t", 1, 0);
+	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
+		goto cleanup;
+	ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket");
+
+
+cleanup:
+	close(clien);
+cleanup_serv:
+	close(serv);
+}
+
+static void test_tcp_server(struct sock_destroy_prog *skel)
+{
+	int serv = -1, clien = -1, n = 0;
+
+	serv = start_server(AF_INET6, SOCK_STREAM, NULL, SERVER_PORT, 0);
+	if (!ASSERT_GE(serv, 0, "start_server"))
+		goto cleanup_serv;
+
+	clien = connect_to_fd(serv, 0);
+	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
+		goto cleanup_serv;
+
+	serv = accept(serv, NULL, NULL);
+	if (!ASSERT_GE(serv, 0, "serv accept"))
+		goto cleanup;
+
+	n = send(clien, "t", 1, 0);
+	if (!ASSERT_GE(n, 0, "client send"))
+		goto cleanup;
+
+	/* Run iterator program that destroys server sockets. */
+	start_iter_sockets(skel->progs.iter_tcp6_server);
+
+	n = send(clien, "t", 1, 0);
+	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
+		goto cleanup;
+	ASSERT_EQ(errno, ECONNRESET, "error code on destroyed socket");
+
+
+cleanup:
+	close(clien);
+cleanup_serv:
+	close(serv);
+}
+
+
+static void test_udp_client(struct sock_destroy_prog *skel)
+{
+	int serv = -1, clien = -1, n = 0;
+
+	serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 6161, 0);
+	if (!ASSERT_GE(serv, 0, "start_server"))
+		goto cleanup_serv;
+
+	clien = connect_to_fd(serv, 0);
+	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
+		goto cleanup_serv;
+
+	n = send(clien, "t", 1, 0);
+	if (!ASSERT_GE(n, 0, "client send"))
+		goto cleanup;
+
+	/* Run iterator program that destroys sockets. */
+	start_iter_sockets(skel->progs.iter_udp6_client);
+
+	n = send(clien, "t", 1, 0);
+	if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
+		goto cleanup;
+	/* UDP sockets have an overriding error code after they are disconnected,
+	 * so we don't check for ECONNABORTED error code.
+	 */
+
+cleanup:
+	close(clien);
+cleanup_serv:
+	close(serv);
+}
+
+static void test_udp_server(struct sock_destroy_prog *skel)
+{
+	int *listen_fds = NULL, n, i;
+	unsigned int num_listens = 5;
+	char buf[1];
+
+	/* Start reuseport servers. */
+	listen_fds = start_reuseport_server(AF_INET6, SOCK_DGRAM,
+					    "::1", SERVER_PORT, 0,
+					    num_listens);
+	if (!ASSERT_OK_PTR(listen_fds, "start_reuseport_server"))
+		goto cleanup;
+
+	/* Run iterator program that destroys server sockets. */
+	start_iter_sockets(skel->progs.iter_udp6_server);
+
+	for (i = 0; i < num_listens; ++i) {
+		n = read(listen_fds[i], buf, sizeof(buf));
+		if (!ASSERT_EQ(n, -1, "read") ||
+		    !ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket"))
+			break;
+	}
+	ASSERT_EQ(i, num_listens, "server socket");
+
+cleanup:
+	free_fds(listen_fds, num_listens);
+}
+
+void test_sock_destroy(void)
+{
+	int cgroup_fd = 0;
+	struct sock_destroy_prog *skel;
+
+	skel = sock_destroy_prog__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open"))
+		return;
+
+	cgroup_fd = test__join_cgroup("/sock_destroy");
+	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
+		goto close_cgroup_fd;
+
+	skel->links.sock_connect = bpf_program__attach_cgroup(
+		skel->progs.sock_connect, cgroup_fd);
+	if (!ASSERT_OK_PTR(skel->links.sock_connect, "prog_attach"))
+		goto close_cgroup_fd;
+
+	if (test__start_subtest("tcp_client"))
+		test_tcp_client(skel);
+	if (test__start_subtest("tcp_server"))
+		test_tcp_server(skel);
+	if (test__start_subtest("udp_client"))
+		test_udp_client(skel);
+	if (test__start_subtest("udp_server"))
+		test_udp_server(skel);
+
+
+close_cgroup_fd:
+	close(cgroup_fd);
+	sock_destroy_prog__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
new file mode 100644
index 000000000000..8e09d82c50f3
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
@@ -0,0 +1,151 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+
+#include "bpf_tracing_net.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+#define AF_INET6 10
+/* Keep it in sync with prog_test/sock_destroy. */
+#define SERVER_PORT 6062
+
+int bpf_sock_destroy(struct sock_common *sk) __ksym;
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, __u64);
+} tcp_conn_sockets SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, __u64);
+} udp_conn_sockets SEC(".maps");
+
+SEC("cgroup/connect6")
+int sock_connect(struct bpf_sock_addr *ctx)
+{
+	int key = 0;
+	__u64 sock_cookie = 0;
+	__u32 keyc = 0;
+
+	if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
+		return 1;
+
+	sock_cookie = bpf_get_socket_cookie(ctx);
+	if (ctx->protocol == IPPROTO_TCP)
+		bpf_map_update_elem(&tcp_conn_sockets, &key, &sock_cookie, 0);
+	else if (ctx->protocol == IPPROTO_UDP)
+		bpf_map_update_elem(&udp_conn_sockets, &keyc, &sock_cookie, 0);
+	else
+		return 1;
+
+	return 1;
+}
+
+SEC("iter/tcp")
+int iter_tcp6_client(struct bpf_iter__tcp *ctx)
+{
+	struct sock_common *sk_common = ctx->sk_common;
+	struct seq_file *seq = ctx->meta->seq;
+	__u64 sock_cookie = 0;
+	__u64 *val;
+	int key = 0;
+
+	if (!sk_common)
+		return 0;
+
+	if (sk_common->skc_family != AF_INET6)
+		return 0;
+
+	sock_cookie  = bpf_get_socket_cookie(sk_common);
+	val = bpf_map_lookup_elem(&tcp_conn_sockets, &key);
+	if (!val)
+		return 0;
+	/* Destroy connected client sockets. */
+	if (sock_cookie == *val)
+		bpf_sock_destroy(sk_common);
+
+	return 0;
+}
+
+SEC("iter/tcp")
+int iter_tcp6_server(struct bpf_iter__tcp *ctx)
+{
+	struct sock_common *sk_common = ctx->sk_common;
+	struct seq_file *seq = ctx->meta->seq;
+	struct tcp6_sock *tcp_sk;
+	const struct inet_connection_sock *icsk;
+	const struct inet_sock *inet;
+	__u16 srcp;
+
+	if (!sk_common)
+		return 0;
+
+	if (sk_common->skc_family != AF_INET6)
+		return 0;
+
+	tcp_sk = bpf_skc_to_tcp6_sock(sk_common);
+	if (!tcp_sk)
+		return 0;
+
+	icsk = &tcp_sk->tcp.inet_conn;
+	inet = &icsk->icsk_inet;
+	srcp = bpf_ntohs(inet->inet_sport);
+
+	/* Destroy server sockets. */
+	if (srcp == SERVER_PORT)
+		bpf_sock_destroy(sk_common);
+
+	return 0;
+}
+
+
+SEC("iter/udp")
+int iter_udp6_client(struct bpf_iter__udp *ctx)
+{
+	struct seq_file *seq = ctx->meta->seq;
+	struct udp_sock *udp_sk = ctx->udp_sk;
+	struct sock *sk = (struct sock *) udp_sk;
+	__u64 sock_cookie = 0, *val;
+	int key = 0;
+
+	if (!sk)
+		return 0;
+
+	sock_cookie  = bpf_get_socket_cookie(sk);
+	val = bpf_map_lookup_elem(&udp_conn_sockets, &key);
+	if (!val)
+		return 0;
+	/* Destroy connected client sockets. */
+	if (sock_cookie == *val)
+		bpf_sock_destroy((struct sock_common *)sk);
+
+	return 0;
+}
+
+SEC("iter/udp")
+int iter_udp6_server(struct bpf_iter__udp *ctx)
+{
+	struct seq_file *seq = ctx->meta->seq;
+	struct udp_sock *udp_sk = ctx->udp_sk;
+	struct sock *sk = (struct sock *) udp_sk;
+	__u16 srcp;
+	struct inet_sock *inet;
+
+	if (!sk)
+		return 0;
+
+	inet = &udp_sk->inet;
+	srcp = bpf_ntohs(inet->inet_sport);
+	if (srcp == SERVER_PORT)
+		bpf_sock_destroy((struct sock_common *)sk);
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";