Re: [PATCH bpf-next 2/3] selftests/bpf: add tests for TCP_BPF_SOCK_OPS_CB_FLAGS

[Martin KaFai Lau <martin.lau@linux.dev>]

On 8/7/24 10:58 AM, Alan Maguire wrote:
> On 06/08/2024 22:27, Martin KaFai Lau wrote:
>> On 8/2/24 8:29 AM, Alan Maguire wrote:
>>> Add tests to set/get TCP sockopt TCP_BPF_SOCK_OPS_CB_FLAGS via
>>> bpf_setsockopt() and also add a cgroup/setsockopt program that
>>> catches setsockopt() for this option and uses bpf_setsockopt()
>>> to set it.  The latter allows us to support modifying sockops
>>> cb flags on a per-socket basis via setsockopt() without adding
>>> support into core setsockopt() itself.
>>>
>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>> ---
>>>    .../selftests/bpf/prog_tests/setget_sockopt.c | 11 ++++++
>>>    .../selftests/bpf/progs/setget_sockopt.c      | 37 +++++++++++++++++--
>>>    2 files changed, 45 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/setget_sockopt.c
>>> b/tools/testing/selftests/bpf/prog_tests/setget_sockopt.c
>>> index 7d4a9b3d3722..b9c54217a489 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/setget_sockopt.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/setget_sockopt.c
>>> @@ -42,6 +42,7 @@ static int create_netns(void)
>>>    static void test_tcp(int family)
>>>    {
>>>        struct setget_sockopt__bss *bss = skel->bss;
>>> +    int cb_flags = BPF_SOCK_OPS_STATE_CB_FLAG |
>>> BPF_SOCK_OPS_RTO_CB_FLAG;
>>>        int sfd, cfd;
>>>          memset(bss, 0, sizeof(*bss));
>>> @@ -56,6 +57,9 @@ static void test_tcp(int family)
>>>            close(sfd);
>>>            return;
>>>        }
>>> +    ASSERT_EQ(setsockopt(sfd, SOL_TCP, TCP_BPF_SOCK_OPS_CB_FLAGS,
>>> +                 &cb_flags, sizeof(cb_flags)),
>>> +          0, "setsockopt cb_flags");
>>
>> The sfd here is the listen fd. The setsockopt here is after the
>> connection is established and the connection won't be affected by this
>> setsockopt...
>>
>> I don't think this test belongs to test_tcp() here, more on this below...
>>
>>>        close(sfd);
>>>        close(cfd);
>>>    @@ -65,6 +69,8 @@ static void test_tcp(int family)
>>>        ASSERT_EQ(bss->nr_passive, 1, "nr_passive");
>>>        ASSERT_EQ(bss->nr_socket_post_create, 2, "nr_socket_post_create");
>>>        ASSERT_EQ(bss->nr_binddev, 2, "nr_bind");
>>> +    ASSERT_GE(bss->nr_state, 1, "nr_state");
>>> +    ASSERT_EQ(bss->nr_setsockopt, 1, "nr_setsockopt");
>>>    }
>>>      static void test_udp(int family)
>>> @@ -185,6 +191,11 @@ void test_setget_sockopt(void)
>>>        if (!ASSERT_OK_PTR(skel->links.socket_post_create,
>>> "attach_cgroup"))
>>>            goto done;
>>>    +    skel->links.tcp_setsockopt =
>>> +        bpf_program__attach_cgroup(skel->progs.tcp_setsockopt, cg_fd);
>>> +    if (!ASSERT_OK_PTR(skel->links.tcp_setsockopt, "attach_setsockopt"))
>>> +        goto done;
>>> +
>>>        test_tcp(AF_INET6);
>>>        test_tcp(AF_INET);
>>>        test_udp(AF_INET6);
>>> diff --git a/tools/testing/selftests/bpf/progs/setget_sockopt.c
>>> b/tools/testing/selftests/bpf/progs/setget_sockopt.c
>>> index 60518aed1ffc..920af9e21e84 100644
>>> --- a/tools/testing/selftests/bpf/progs/setget_sockopt.c
>>> +++ b/tools/testing/selftests/bpf/progs/setget_sockopt.c
>>> @@ -20,6 +20,8 @@ int nr_connect;
>>>    int nr_binddev;
>>>    int nr_socket_post_create;
>>>    int nr_fin_wait1;
>>> +int nr_state;
>>> +int nr_setsockopt;
>>>      struct sockopt_test {
>>>        int opt;
>>> @@ -59,6 +61,8 @@ static const struct sockopt_test sol_tcp_tests[] = {
>>>        { .opt = TCP_THIN_LINEAR_TIMEOUTS, .flip = 1, },
>>>        { .opt = TCP_USER_TIMEOUT, .new = 123400, .expected = 123400, },
>>>        { .opt = TCP_NOTSENT_LOWAT, .new = 1314, .expected = 1314, },
>>> +    { .opt = TCP_BPF_SOCK_OPS_CB_FLAGS, .new =
>>> BPF_SOCK_OPS_ALL_CB_FLAGS,
>>> +      .expected = BPF_SOCK_OPS_ALL_CB_FLAGS, .restore =
>>> BPF_SOCK_OPS_STATE_CB_FLAG, },
>>>        { .opt = 0, },
>>>    };
>>>    @@ -124,6 +128,7 @@ static int bpf_test_sockopt_int(void *ctx,
>>> struct sock *sk,
>>>          if (bpf_setsockopt(ctx, level, opt, &new, sizeof(new)))
>>>            return 1;
>>> +
>>>        if (bpf_getsockopt(ctx, level, opt, &tmp, sizeof(tmp)) ||
>>>            tmp != expected)
>>>            return 1;
>>> @@ -384,11 +389,14 @@ int skops_sockopt(struct bpf_sock_ops *skops)
>>>            nr_passive += !(bpf_test_sockopt(skops, sk) ||
>>>                    test_tcp_maxseg(skops, sk) ||
>>>                    test_tcp_saved_syn(skops, sk));
>>> -        bpf_sock_ops_cb_flags_set(skops,
>>> -                      skops->bpf_sock_ops_cb_flags |
>>> -                      BPF_SOCK_OPS_STATE_CB_FLAG);
>>> +
>>> +        /* no need to set sockops cb flags here as sockopt
>>> +         * tests and user-space originated setsockopt() will
>>> +         * set flags to include BPF_SOCK_OPS_STATE_CB.
>>> +         */
>>
>> I don't think the "user-space originated..." comment is accruate here.
>> The user-space originated setsockopt() from the above test_tcp() won't
>> affect the passively established sk here. This took me a while to digest...
>>
>> iiuc, the removed bpf_sock_ops_cb_flags_set() for the passive connection
>> is now implicitly done (and hidden) in the newly added
>> sol_tcp_tests[].restore.
>>
>> How about keeping the explicit bpf_sock_ops_cb_flags_set() and removing
>> the ".restore".
>>
>> The existing bpf_sock_ops_cb_flags_set() can be changed to
>> bpf_setsockopt(TCP_BPF_SOCK_OPS_CB_FLAGS) if it helps to test if it is
>> effective.
> 
> Sounds good; I'll make that change and avoid changing test_tcp() etc.
>>
>>>            break;
>>>        case BPF_SOCK_OPS_STATE_CB:
>>> +        nr_state++;
>>
>> How about removing the nr_state addition and adding a
>> SEC("cgroup/getsockopt") bpf prog to test the
>> getsockopt(TCP_BPF_SOCK_OPS_CB_FLAGS).
>>
> 
> Will do. Given that currently we cannot call bpf_getsockopt() from
> cgroup/getsockopt progs it might make sense to use per-socket storage to
> store the cb flags value that we set via bpf_setsockopt() in the sock
> ops program, and retrieve it in the cgroup/getsockopt prog. Does that
> sound ok?

The bpf_sock_ops_cb_flags can be directly inspected in cgroup/getsockopt
with the help of bpf_core_cast(). The following is based on the patch3's
_getsockopt (untested):

#include <vmlinux.h>
#include <bpf/bpf_core_read.h>

SEC("cgroup/getsockopt")
int _getsockopt(struct bpf_sockopt *ctx)
{
	struct bpf_sock *sk = ctx->sk;
	int *optval = ctx->optval;
	struct tcp_sock *tp;

	if (!sk || ctx->level != SOL_TCP || ctx->optname != TCP_BPF_SOCK_OPS_CB_FLAGS || sk->protocol != IPPROTO_TCP ||
	    ctx->optval + sizeof(int) > ctx->optval_end)
		return 1;

	tp = bpf_core_cast(sk, struct tcp_sock);
	*optval = tp->bpf_sock_ops_cb_flags;
	ctx->retval = 0;
	return 1;
}