Re: [PATCH bpf-next v3 2/2] selftests/bpf: Verify that the cgroup_skb filters receive expected packets.

[Yonghong Song <yhs@meta.com>]

On 6/20/23 10:14 AM, Kui-Feng Lee wrote:
> This test case includes four scenarios:
> 1. Connect to the server from outside the cgroup and close the connection
>     from outside the cgroup.
> 2. Connect to the server from outside the cgroup and close the connection
>     from inside the cgroup.
> 3. Connect to the server from inside the cgroup and close the connection
>     from outside the cgroup.
> 4. Connect to the server from inside the cgroup and close the connection
>     from inside the cgroup.
> 
> The test case is to verify that cgroup_skb/{egress, ingress} filters
> receive expected packets including SYN, SYN/ACK, ACK, FIN, and FIN/ACK.
> 
> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
> ---
>   tools/testing/selftests/bpf/cgroup_helpers.c  |  12 +
>   tools/testing/selftests/bpf/cgroup_helpers.h  |   1 +
>   tools/testing/selftests/bpf/cgroup_tcp_skb.h  |  35 ++
>   .../selftests/bpf/prog_tests/cgroup_tcp_skb.c | 399 ++++++++++++++++++
>   .../selftests/bpf/progs/cgroup_tcp_skb.c      | 382 +++++++++++++++++
>   5 files changed, 829 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/cgroup_tcp_skb.h
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
>   create mode 100644 tools/testing/selftests/bpf/progs/cgroup_tcp_skb.c
> 
> diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
> index 9e95b37a7dff..2caee8423ee0 100644
> --- a/tools/testing/selftests/bpf/cgroup_helpers.c
> +++ b/tools/testing/selftests/bpf/cgroup_helpers.c
> @@ -277,6 +277,18 @@ int join_cgroup(const char *relative_path)
>   	return join_cgroup_from_top(cgroup_path);
>   }
>   
> +/**
> + * join_root_cgroup() - Join the root cgroup
> + *
> + * This function joins the root cgroup.
> + *
> + * On success, it returns 0, otherwise on failure it returns 1.
> + */
> +int join_root_cgroup(void)
> +{
> +	return join_cgroup_from_top(CGROUP_MOUNT_PATH);
> +}
> +
>   /**
>    * join_parent_cgroup() - Join a cgroup in the parent process workdir
>    * @relative_path: The cgroup path, relative to parent process workdir, to join
> diff --git a/tools/testing/selftests/bpf/cgroup_helpers.h b/tools/testing/selftests/bpf/cgroup_helpers.h
> index f099a166c94d..5c2cb9c8b546 100644
> --- a/tools/testing/selftests/bpf/cgroup_helpers.h
> +++ b/tools/testing/selftests/bpf/cgroup_helpers.h
> @@ -22,6 +22,7 @@ void remove_cgroup(const char *relative_path);
>   unsigned long long get_cgroup_id(const char *relative_path);
>   
>   int join_cgroup(const char *relative_path);
> +int join_root_cgroup(void);
>   int join_parent_cgroup(const char *relative_path);
>   
>   int setup_cgroup_environment(void);
> diff --git a/tools/testing/selftests/bpf/cgroup_tcp_skb.h b/tools/testing/selftests/bpf/cgroup_tcp_skb.h
> new file mode 100644
> index 000000000000..1054b3633983
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/cgroup_tcp_skb.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2023 Facebook */

/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */

> +
> +/* Define states of a socket to tracking messages sending to and from the
> + * socket.
> + *
> + * These states are based on rfc9293 with some modifications to support
> + * tracking of messages sent out from a socket. For example, when a SYN is
> + * received, a new socket is transiting to the SYN_RECV state defined in
> + * rfc9293. But, we put it in SYN_RECV_SENDING_SYN_ACK state and when
> + * SYN-ACK is sent out, it moves to SYN_RECV state. With this modification,
> + * we can track the message sent out from a socket.
> + */
> +
> +#ifndef __CGROUP_TCP_SKB_H__
> +#define __CGROUP_TCP_SKB_H__
> +
> +enum {
> +	INIT,
> +	CLOSED,
> +	SYN_SENT,
> +	SYN_RECV_SENDING_SYN_ACK,
> +	SYN_RECV,
> +	ESTABLISHED,
> +	FIN_WAIT1,
> +	FIN_WAIT2,
> +	CLOSE_WAIT_SENDING_ACK,
> +	CLOSE_WAIT,
> +	CLOSING,
> +	LAST_ACK,
> +	TIME_WAIT_SENDING_ACK,
> +	TIME_WAIT,
> +};
> +
> +#endif /* __CGROUP_TCP_SKB_H__ */
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c b/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
> new file mode 100644
> index 000000000000..1b78e8ab3f02
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
> @@ -0,0 +1,399 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023 Facebook */
> +#include <test_progs.h>
> +#include <linux/in6.h>
> +#include <sys/socket.h>
> +#include <sched.h>
> +#include <unistd.h>
> +#include "cgroup_helpers.h"
> +#include "testing_helpers.h"
> +#include "cgroup_tcp_skb.skel.h"
> +#include "cgroup_tcp_skb.h"
> +
> +#define CGROUP_TCP_SKB_PATH "/test_cgroup_tcp_skb"
> +
> +static int install_filters(int cgroup_fd,
> +			   struct bpf_link **egress_link,
> +			   struct bpf_link **ingress_link,
> +			   struct bpf_program *egress_prog,
> +			   struct bpf_program *ingress_prog,
> +			   struct cgroup_tcp_skb *skel)
> +{
> +	/* Prepare filters */
> +	skel->bss->g_sock_state = 0;
> +	skel->bss->g_unexpected = 0;
> +	*egress_link =
> +		bpf_program__attach_cgroup(egress_prog,
> +					   cgroup_fd);
> +	if (!ASSERT_NEQ(*egress_link, NULL, "egress_link"))
> +		return -1;

!ASSERT_OK_PTR(...)

> +	*ingress_link =
> +		bpf_program__attach_cgroup(ingress_prog,
> +					   cgroup_fd);
> +	if (!ASSERT_NEQ(*ingress_link, NULL, "ingress_link"))
> +		return -1;

!ASSERT_OK_PTR(...)

> +
> +	return 0;
> +}
> +
> +static void uninstall_filters(struct bpf_link **egress_link,
> +			      struct bpf_link **ingress_link)
> +{
> +	bpf_link__destroy(*egress_link);
> +	*egress_link = NULL;
> +	bpf_link__destroy(*ingress_link);
> +	*ingress_link = NULL;
> +}
> +
[...]
> +
> +static int close_connection(int *closing_fd, int *peer_fd, int *listen_fd,
> +			    struct cgroup_tcp_skb *skel)
> +{
> +	__u32 saved_packet_count = 0;
> +	int err;
> +	int i;
> +
> +	/* Wait for ACKs to be sent */
> +	saved_packet_count = skel->bss->g_packet_count;
> +	usleep(100000);		/* 0.1s */
> +	while (skel->bss->g_packet_count != saved_packet_count) {
> +		saved_packet_count = skel->bss->g_packet_count;
> +		usleep(100000);	/* 0.1s */
> +	}

Should we put a limitation in the number of loop iterations
just in case that something went wrong or too slow?

> +
> +	skel->bss->g_packet_count = 0;
> +	saved_packet_count = 0;
> +
> +	/* Half shutdown to make sure the closing socket having a chance to
> +	 * receive a FIN from the peer.
> +	 */
> +	err = shutdown(*closing_fd, SHUT_WR);
> +	if (!ASSERT_OK(err, "shutdown closing_fd"))
> +		return -1;
> +
> +	/* Wait for FIN and the ACK of the FIN to be observed */
> +	for (i = 0;
> +	     skel->bss->g_packet_count < saved_packet_count + 2 && i < 10;
> +	     i++) {
> +		usleep(100000);	/* 0.1s */
> +	}

'{...}' is not necessary.

> +	if (!ASSERT_GE(skel->bss->g_packet_count, saved_packet_count + 2,
> +		       "packet_count"))
> +		return -1;
> +
> +	saved_packet_count = skel->bss->g_packet_count;
> +
> +	/* Fully shutdown the connection */
> +	err = close(*peer_fd);
> +	if (!ASSERT_OK(err, "close peer_fd"))
> +		return -1;
> +	*peer_fd = -1;
> +
> +	/* Wait for FIN and the ACK of the FIN to be observed */
> +	for (i = 0;
> +	     skel->bss->g_packet_count < saved_packet_count + 2 && i < 10;
> +	     i++) {
> +		usleep(100000);	/* 0.1s */
> +	}

'{...}' is not necessary.

> +	if (!ASSERT_GE(skel->bss->g_packet_count, saved_packet_count + 2,
> +		       "packet_count"))
> +		return -1;
> +
> +	err = close(*closing_fd);
> +	if (!ASSERT_OK(err, "close closing_fd"))
> +		return -1;
> +	*closing_fd = -1;
> +
> +	close(*listen_fd);
> +	*listen_fd = -1;
> +
> +	return 0;
> +}
> +
[...]
> diff --git a/tools/testing/selftests/bpf/progs/cgroup_tcp_skb.c b/tools/testing/selftests/bpf/progs/cgroup_tcp_skb.c
> new file mode 100644
> index 000000000000..372a1548798c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/cgroup_tcp_skb.c
> @@ -0,0 +1,382 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023 Facebook */

/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */

> +#include <linux/bpf.h>
> +#include <bpf/bpf_endian.h>
> +#include <bpf/bpf_helpers.h>
> +
> +#include <linux/if_ether.h>
> +#include <linux/in.h>
> +#include <linux/in6.h>
> +#include <linux/ipv6.h>
> +#include <linux/tcp.h>
> +
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +
> +#include "cgroup_tcp_skb.h"
[...]