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@xxxxxxxxxx>
>> ---
>> .../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?
> Create another test_nonstandard_opt() in prog_tests/setget_sockopt.c and
> separate it from the existing test_{tcp, udp...} which is mainly
> exercising set/getsockopt(sol_*_tests[]) on different hooks (right now
> it has lsm_cgroup/socket_post_create and sockops). It doesn't fit
> testing the user syscall set/getsockopt on the nonstandard_opt.
Sounds good. I'll also drop the test in patch 3 as you suggest for v2.
Thanks again!
Alan
