* [PATCH bpf-next 0/2] Add support for bpf_setsockopt and bpf_getsockopt from BPF setsockopt @ 2021-08-16 23:17 Prankur gupta 2021-08-16 23:17 ` [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF Prankur gupta 2021-08-16 23:17 ` [PATCH bpf-next 2/2] selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program Prankur gupta 0 siblings, 2 replies; 10+ messages in thread From: Prankur gupta @ 2021-08-16 23:17 UTC (permalink / raw) To: bpf, ast, andrii, daniel; +Cc: kernel-team, prankur.07 This patch contains support to set and get socket options from setsockopt bpf program. This enables us to set multiple socket option when the user changes a particular socket option. Example use case, when the user sets the IPV6_TCLASS socket option we would also like to change the tcp-cc for that socket. We don't have any use case for calling bpf_setsockopt from supposedly read-only sys_getsockopt, so it is made available to BPF_CGROUP_SETSOCKOPT only. Prankur gupta (2): bpf: Add support for {set|get} socket options from setsockopt BPF selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program kernel/bpf/cgroup.c | 8 +++ 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 +++++++++++ 4 files changed, 135 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 -- 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF 2021-08-16 23:17 [PATCH bpf-next 0/2] Add support for bpf_setsockopt and bpf_getsockopt from BPF setsockopt Prankur gupta @ 2021-08-16 23:17 ` Prankur gupta 2021-08-17 0:02 ` Song Liu 2021-08-17 2:23 ` Andrii Nakryiko 2021-08-16 23:17 ` [PATCH bpf-next 2/2] selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program Prankur gupta 1 sibling, 2 replies; 10+ messages in thread From: Prankur gupta @ 2021-08-16 23:17 UTC (permalink / raw) To: bpf, ast, andrii, daniel; +Cc: kernel-team, prankur.07 Add logic to call bpf_setsockopt and bpf_getsockopt from setsockopt BPF programs. Example use case, when the user sets the IPV6_TCLASS socket option we would also like to change the tcp-cc for that socket. We don't have any use case for calling bpf_setsockopt from supposedly read-only sys_getsockopti, so it is made available to BPF_CGROUP_SETSOCKOPT only. Signed-off-by: Prankur gupta <prankgup@fb.com> --- kernel/bpf/cgroup.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c index a1dedba4c174..9c92eff9af95 100644 --- a/kernel/bpf/cgroup.c +++ b/kernel/bpf/cgroup.c @@ -1873,6 +1873,14 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_sk_storage_get_proto; case BPF_FUNC_sk_storage_delete: return &bpf_sk_storage_delete_proto; + case BPF_FUNC_setsockopt: + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) + return &bpf_sk_setsockopt_proto; + return NULL; + case BPF_FUNC_getsockopt: + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) + return &bpf_sk_getsockopt_proto; + return NULL; #endif #ifdef CONFIG_INET case BPF_FUNC_tcp_sock: -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF 2021-08-16 23:17 ` [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF Prankur gupta @ 2021-08-17 0:02 ` Song Liu 2021-08-17 2:23 ` Andrii Nakryiko 1 sibling, 0 replies; 10+ messages in thread From: Song Liu @ 2021-08-17 0:02 UTC (permalink / raw) To: Prankur gupta Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Kernel Team, prankur.07 On Mon, Aug 16, 2021 at 4:17 PM Prankur gupta <prankgup@fb.com> wrote: > > Add logic to call bpf_setsockopt and bpf_getsockopt from > setsockopt BPF programs. > Example use case, when the user sets the IPV6_TCLASS socket option > we would also like to change the tcp-cc for that socket. > We don't have any use case for calling bpf_setsockopt from > supposedly read-only sys_getsockopti, so it is made available to > BPF_CGROUP_SETSOCKOPT only. > > Signed-off-by: Prankur gupta <prankgup@fb.com> Acked-by: Song Liu <songliubraving@fb.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF 2021-08-16 23:17 ` [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF Prankur gupta 2021-08-17 0:02 ` Song Liu @ 2021-08-17 2:23 ` Andrii Nakryiko 2021-08-17 19:49 ` Prankur gupta 1 sibling, 1 reply; 10+ messages in thread From: Andrii Nakryiko @ 2021-08-17 2:23 UTC (permalink / raw) To: Prankur gupta Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Kernel Team, prankur.07 On Mon, Aug 16, 2021 at 4:17 PM Prankur gupta <prankgup@fb.com> wrote: > > Add logic to call bpf_setsockopt and bpf_getsockopt from > setsockopt BPF programs. > Example use case, when the user sets the IPV6_TCLASS socket option > we would also like to change the tcp-cc for that socket. > We don't have any use case for calling bpf_setsockopt from > supposedly read-only sys_getsockopti, so it is made available to > BPF_CGROUP_SETSOCKOPT only. > > Signed-off-by: Prankur gupta <prankgup@fb.com> > --- > kernel/bpf/cgroup.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c > index a1dedba4c174..9c92eff9af95 100644 > --- a/kernel/bpf/cgroup.c > +++ b/kernel/bpf/cgroup.c > @@ -1873,6 +1873,14 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return &bpf_sk_storage_get_proto; > case BPF_FUNC_sk_storage_delete: > return &bpf_sk_storage_delete_proto; > + case BPF_FUNC_setsockopt: > + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) > + return &bpf_sk_setsockopt_proto; > + return NULL; > + case BPF_FUNC_getsockopt: > + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) > + return &bpf_sk_getsockopt_proto; Is there any problem enabling bpf_getsockopt() for BPF_CGROUP_GETSOCKOPT program type? > + return NULL; > #endif > #ifdef CONFIG_INET > case BPF_FUNC_tcp_sock: > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF 2021-08-17 2:23 ` Andrii Nakryiko @ 2021-08-17 19:49 ` Prankur gupta 2021-08-17 20:31 ` Martin KaFai Lau 0 siblings, 1 reply; 10+ messages in thread From: Prankur gupta @ 2021-08-17 19:49 UTC (permalink / raw) To: andrii.nakryiko Cc: andrii, ast, bpf, daniel, kernel-team, prankgup, prankur.07 >> >> Add logic to call bpf_setsockopt and bpf_getsockopt from >> setsockopt BPF programs. >> Example use case, when the user sets the IPV6_TCLASS socket option >> we would also like to change the tcp-cc for that socket. >> We don't have any use case for calling bpf_setsockopt from >> supposedly read-only sys_getsockopti, so it is made available to >> BPF_CGROUP_SETSOCKOPT only. >> >> Signed-off-by: Prankur gupta <prankgup@fb.com> >> --- >> kernel/bpf/cgroup.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c >> index a1dedba4c174..9c92eff9af95 100644 >> --- a/kernel/bpf/cgroup.c >> +++ b/kernel/bpf/cgroup.c >> @@ -1873,6 +1873,14 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) >> return &bpf_sk_storage_get_proto; >> case BPF_FUNC_sk_storage_delete: >> return &bpf_sk_storage_delete_proto; >> + case BPF_FUNC_setsockopt: >> + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) >> + return &bpf_sk_setsockopt_proto; >> + return NULL; >> + case BPF_FUNC_getsockopt: >> + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) >> + return &bpf_sk_getsockopt_proto; > >Is there any problem enabling bpf_getsockopt() for >BPF_CGROUP_GETSOCKOPT program type? > No, there's no problem but there's no usecase (which i can think of) where a user wants to set or get some socket option for a getsockopt call >> + return NULL; >> #endif >> #ifdef CONFIG_INET >> case BPF_FUNC_tcp_sock: >> -- >> 2.30.2 >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF 2021-08-17 19:49 ` Prankur gupta @ 2021-08-17 20:31 ` Martin KaFai Lau 0 siblings, 0 replies; 10+ messages in thread From: Martin KaFai Lau @ 2021-08-17 20:31 UTC (permalink / raw) To: Prankur gupta, andrii Cc: andrii.nakryiko, ast, bpf, daniel, kernel-team, prankur.07 On Tue, Aug 17, 2021 at 12:49:21PM -0700, Prankur gupta wrote: > >> > >> Add logic to call bpf_setsockopt and bpf_getsockopt from > >> setsockopt BPF programs. > >> Example use case, when the user sets the IPV6_TCLASS socket option > >> we would also like to change the tcp-cc for that socket. > >> We don't have any use case for calling bpf_setsockopt from > >> supposedly read-only sys_getsockopti, so it is made available to > >> BPF_CGROUP_SETSOCKOPT only. > >> > >> Signed-off-by: Prankur gupta <prankgup@fb.com> > >> --- > >> kernel/bpf/cgroup.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c > >> index a1dedba4c174..9c92eff9af95 100644 > >> --- a/kernel/bpf/cgroup.c > >> +++ b/kernel/bpf/cgroup.c > >> @@ -1873,6 +1873,14 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > >> return &bpf_sk_storage_get_proto; > >> case BPF_FUNC_sk_storage_delete: > >> return &bpf_sk_storage_delete_proto; > >> + case BPF_FUNC_setsockopt: > >> + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) > >> + return &bpf_sk_setsockopt_proto; > >> + return NULL; > >> + case BPF_FUNC_getsockopt: > >> + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) > >> + return &bpf_sk_getsockopt_proto; > > > >Is there any problem enabling bpf_getsockopt() for > >BPF_CGROUP_GETSOCKOPT program type? > > > > No, there's no problem but there's no usecase (which i can think of) > where a user wants to set or get some socket option for a getsockopt call imo, user usually expects that bpf_getsockopt() and bpf_setsockopt() are available together. It is pretty much the only reason bpf_getsockopt() is made available together with bpf_setsockopt() in this patch. It may actually create usage surprises if it only allows one but not another. To read members from a sk, it is more useful to make the bpf_skc_to_*_sock() helpers available instead of bpf_getsockopt(). Then it can read the sk's members from the returned PTR_TO_BTF_ID. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program 2021-08-16 23:17 [PATCH bpf-next 0/2] Add support for bpf_setsockopt and bpf_getsockopt from BPF setsockopt Prankur gupta 2021-08-16 23:17 ` [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF Prankur gupta @ 2021-08-16 23:17 ` Prankur gupta 2021-08-17 0:20 ` Song Liu 1 sibling, 1 reply; 10+ messages in thread From: Prankur gupta @ 2021-08-16 23:17 UTC (permalink / raw) To: bpf, ast, andrii, daniel; +Cc: kernel-team, prankur.07 Adding selftests for new added functionality to call bpf_setsockopt and bpf_getsockopt from setsockopt BPF programs Signed-off-by: Prankur gupta <prankgup@fb.com> --- 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; +} + 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; + + buf = 0x2D; + 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program 2021-08-16 23:17 ` [PATCH bpf-next 2/2] selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program Prankur gupta @ 2021-08-17 0:20 ` Song Liu 2021-08-17 3:56 ` Song Liu 0 siblings, 1 reply; 10+ messages in thread From: Song Liu @ 2021-08-17 0:20 UTC (permalink / raw) To: Prankur gupta Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Kernel Team, prankur.07 On Mon, Aug 16, 2021 at 4:18 PM Prankur gupta <prankgup@fb.com> wrote: > > Adding selftests for new added functionality to call bpf_setsockopt and > bpf_getsockopt from setsockopt BPF programs > > Signed-off-by: Prankur gupta <prankgup@fb.com> > --- > 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? > + > 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program 2021-08-17 0:20 ` Song Liu @ 2021-08-17 3:56 ` Song Liu 0 siblings, 0 replies; 10+ messages in thread From: Song Liu @ 2021-08-17 3:56 UTC (permalink / raw) To: Prankur gupta Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Kernel Team, prankur.07 On Mon, Aug 16, 2021 at 5:20 PM Song Liu <song@kernel.org> wrote: > > On Mon, Aug 16, 2021 at 4:18 PM Prankur gupta <prankgup@fb.com> wrote: > > > > Adding selftests for new added functionality to call bpf_setsockopt and > > bpf_getsockopt from setsockopt BPF programs > > > > Signed-off-by: Prankur gupta <prankgup@fb.com> > > --- > > 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 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 bpf-next 0/2] Add support for bpf_setsockopt and @ 2021-08-17 22:42 Prankur gupta 2021-08-17 22:42 ` [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF Prankur gupta 0 siblings, 1 reply; 10+ messages in thread From: Prankur gupta @ 2021-08-17 22:42 UTC (permalink / raw) To: bpf, ast, andrii, daniel; +Cc: kernel-team, prankur.07 v2: Added details about the test in commit log This patch contains support to set and get socket options from setsockopt bpf program. This enables us to set multiple socket option when the user changes a particular socket option. Example use case, when the user sets the IPV6_TCLASS socket option we would also like to change the tcp-cc for that socket. We don't have any use case for calling bpf_setsockopt from supposedly read-only sys_getsockopt, so it is made available to BPF_CGROUP_SETSOCKOPT only. Prankur gupta (2): bpf: Add support for {set|get} socket options from setsockopt BPF selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program kernel/bpf/cgroup.c | 8 +++ 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 +++++++++++ 4 files changed, 135 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 -- 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF 2021-08-17 22:42 [PATCH v2 bpf-next 0/2] Add support for bpf_setsockopt and Prankur gupta @ 2021-08-17 22:42 ` Prankur gupta 0 siblings, 0 replies; 10+ messages in thread From: Prankur gupta @ 2021-08-17 22:42 UTC (permalink / raw) To: bpf, ast, andrii, daniel; +Cc: kernel-team, prankur.07, Song Liu Add logic to call bpf_setsockopt and bpf_getsockopt from setsockopt BPF programs. Example use case, when the user sets the IPV6_TCLASS socket option we would also like to change the tcp-cc for that socket. We don't have any use case for calling bpf_setsockopt from supposedly read-only sys_getsockopti, so it is made available to BPF_CGROUP_SETSOCKOPT only. Signed-off-by: Prankur gupta <prankgup@fb.com> Acked-by: Song Liu <songliubraving@fb.com> --- kernel/bpf/cgroup.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c index 9f35928bab0a..8e9d99e2ade4 100644 --- a/kernel/bpf/cgroup.c +++ b/kernel/bpf/cgroup.c @@ -1873,6 +1873,14 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_sk_storage_get_proto; case BPF_FUNC_sk_storage_delete: return &bpf_sk_storage_delete_proto; + case BPF_FUNC_setsockopt: + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) + return &bpf_sk_setsockopt_proto; + return NULL; + case BPF_FUNC_getsockopt: + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) + return &bpf_sk_getsockopt_proto; + return NULL; #endif #ifdef CONFIG_INET case BPF_FUNC_tcp_sock: -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-08-17 22:42 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-16 23:17 [PATCH bpf-next 0/2] Add support for bpf_setsockopt and bpf_getsockopt from BPF setsockopt Prankur gupta 2021-08-16 23:17 ` [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF Prankur gupta 2021-08-17 0:02 ` Song Liu 2021-08-17 2:23 ` Andrii Nakryiko 2021-08-17 19:49 ` Prankur gupta 2021-08-17 20:31 ` Martin KaFai Lau 2021-08-16 23:17 ` [PATCH bpf-next 2/2] selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program Prankur gupta 2021-08-17 0:20 ` Song Liu 2021-08-17 3:56 ` Song Liu 2021-08-17 22:42 [PATCH v2 bpf-next 0/2] Add support for bpf_setsockopt and Prankur gupta 2021-08-17 22:42 ` [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF Prankur gupta
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.