On Mon, Aug 16, 2021 at 5:20 PM Song Liu <song@xxxxxxxxxx> wrote:
>
> On Mon, Aug 16, 2021 at 4:18 PM Prankur gupta <prankgup@xxxxxx> wrote:
> >
> > Adding selftests for new added functionality to call bpf_setsockopt and
> > bpf_getsockopt from setsockopt BPF programs
> >
> > Signed-off-by: Prankur gupta <prankgup@xxxxxx>
> > ---
> > tools/testing/selftests/bpf/bpf_tcp_helpers.h | 18 +++++
> > .../bpf/prog_tests/sockopt_qos_to_cc.c | 70 +++++++++++++++++++
> > .../selftests/bpf/progs/sockopt_qos_to_cc.c | 39 +++++++++++
> > 3 files changed, 127 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/sockopt_qos_to_cc.c
> > create mode 100644 tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c
> >
> > diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> > index 029589c008c9..c9f9bdad60c7 100644
> > --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> > +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> > @@ -12,6 +12,10 @@
> > SEC("struct_ops/"#name) \
> > BPF_PROG(name, args)
> >
> > +#ifndef SOL_TCP
> > +#define SOL_TCP 6
> > +#endif
> > +
> > #define tcp_jiffies32 ((__u32)bpf_jiffies64())
> >
> > struct sock_common {
> > @@ -203,6 +207,20 @@ static __always_inline bool tcp_is_cwnd_limited(const struct sock *sk)
> > return !!BPF_CORE_READ_BITFIELD(tp, is_cwnd_limited);
> > }
> >
> > +static __always_inline bool tcp_cc_eq(const char *a, const char *b)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < TCP_CA_NAME_MAX; i++) {
> > + if (a[i] != b[i])
> > + return false;
> > + if (!a[i])
> > + break;
> > + }
> > +
> > + return true;
> > +}
>
> IIUC, this is copied from bpf_iter_setsockopt.c. I think we should be
> more careful with it
> as this is a header. With current code,
>
> tcp_cc_eq("ABC", "ABCD") = true;
> tcp_cc_eq("ABCD", "ABC") = false;
>
> Is this expected?
Ooops, I misread the code. Please ignore the comment above.
>
> > +
> > extern __u32 tcp_slow_start(struct tcp_sock *tp, __u32 acked) __ksym;
> > extern void tcp_cong_avoid_ai(struct tcp_sock *tp, __u32 w, __u32 acked) __ksym;
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_qos_to_cc.c b/tools/testing/selftests/bpf/prog_tests/sockopt_qos_to_cc.c
> > new file mode 100644
> > index 000000000000..6b53b3cb8dad
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockopt_qos_to_cc.c
> > @@ -0,0 +1,70 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2021 Facebook */
> > +#include <test_progs.h>
> > +#include <netinet/tcp.h>
> > +#include "sockopt_qos_to_cc.skel.h"
> > +
> > +static void run_setsockopt_test(int cg_fd, int sock_fd)
> > +{
> > + socklen_t optlen;
> > + char cc[16]; /* TCP_CA_NAME_MAX */
> > + int buf;
> > + int err = -1;
> nit: this err = -1 is not needed.
>
> > +
> > + buf = 0x2D;
>
> Please explain the test case in the commit log. Otherwise, it is not
> easy to follow
> the test case. For example, why we use 0x2d ('-'?) here?
>
> > + err = setsockopt(sock_fd, SOL_IPV6, IPV6_TCLASS, &buf, sizeof(buf));
> > + if (!ASSERT_OK(err, "setsockopt(sock_fd, IPV6_TCLASS)"))
> > + return;
> > +
> > + /* Verify the setsockopt cc change */
> > + optlen = sizeof(cc);
> > + err = getsockopt(sock_fd, SOL_TCP, TCP_CONGESTION, cc, &optlen);
> > + if (!ASSERT_OK(err, "getsockopt(sock_fd, TCP_CONGESTION)"))
> > + return;
> > +
> > + if (!ASSERT_STREQ(cc, "reno", "getsockopt(sock_fd, TCP_CONGESTION)"))
> > + return;
> > +}
> > +
> > +void test_sockopt_qos_to_cc(void)
> > +{
> > + struct sockopt_qos_to_cc *skel;
> > + char cc_cubic[16] = "cubic"; /* TCP_CA_NAME_MAX */
> > + int cg_fd = -1;
> > + int sock_fd = -1;
> > + int err;
> > +
> > + cg_fd = test__join_cgroup("/sockopt_qos_to_cc");
> > + if (!ASSERT_GE(cg_fd, 0, "cg-join(sockopt_qos_to_cc)"))
> > + return;
> > +
> > + skel = sockopt_qos_to_cc__open_and_load();
> > + if (!ASSERT_OK_PTR(skel, "skel"))
> > + goto done;
> > +
> > + sock_fd = socket(AF_INET6, SOCK_STREAM, 0);
> > + if (!ASSERT_GE(sock_fd, 0, "v6 socket open"))
> > + goto done;
> > +
> > + err = setsockopt(sock_fd, SOL_TCP, TCP_CONGESTION, &cc_cubic,
> > + sizeof(cc_cubic));
> > + if (!ASSERT_OK(err, "setsockopt(sock_fd, TCP_CONGESTION)"))
> > + goto done;
> > +
> > + skel->links.sockopt_qos_to_cc =
> > + bpf_program__attach_cgroup(skel->progs.sockopt_qos_to_cc,
> > + cg_fd);
> > + if (!ASSERT_OK_PTR(skel->links.sockopt_qos_to_cc,
> > + "prog_attach(sockopt_qos_to_cc)"))
> > + goto done;
> > +
> > + run_setsockopt_test(cg_fd, sock_fd);
> > +
> > +done:
> > + if (sock_fd != -1)
> > + close(sock_fd);
> > + if (cg_fd != -1)
> > + close(cg_fd);
> > + /* destroy can take null and error pointer */
> > + sockopt_qos_to_cc__destroy(skel);
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c b/tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c
> > new file mode 100644
> > index 000000000000..1bce83b6e3a7
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c
> > @@ -0,0 +1,39 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2021 Facebook */
> > +#include <string.h>
> > +#include <linux/tcp.h>
> > +#include <netinet/in.h>
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include "bpf_tcp_helpers.h"
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +SEC("cgroup/setsockopt")
> > +int sockopt_qos_to_cc(struct bpf_sockopt *ctx)
> > +{
> > + void *optval_end = ctx->optval_end;
> > + int *optval = ctx->optval;
> > + char buf[TCP_CA_NAME_MAX];
> > + char cc_reno[TCP_CA_NAME_MAX] = "reno";
> > + char cc_cubic[TCP_CA_NAME_MAX] = "cubic";
> > +
> > + if (ctx->level != SOL_IPV6 || ctx->optname != IPV6_TCLASS)
> > + return 1;
> > +
> > + if (optval + 1 > optval_end)
> > + return 0; /* EPERM, bounds check */
> > +
> > + if (bpf_getsockopt(ctx->sk, SOL_TCP, TCP_CONGESTION, &buf, sizeof(buf)))
> > + return 0;
> > +
> > + if (!tcp_cc_eq(buf, cc_cubic))
> > + return 0;
> > +
> > + if (*optval == 0x2d) {
> > + if (bpf_setsockopt(ctx->sk, SOL_TCP, TCP_CONGESTION, &cc_reno,
> > + sizeof(cc_reno)))
> > + return 0;
> > + }
> > + return 1;
> > +}
> > --
> > 2.30.2
> >
