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

[Kui-Feng Lee <sinquersw@gmail.com>]

On 6/21/23 21:15, Yonghong Song wrote:
> 
> 
> 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(...)

Sure!

> 
>> +    *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?

Sure! It make sense to me.

[...]