[PATCH bpf-next 0/2] Add support for bpf_setsockopt and bpf_getsockopt from BPF setsockopt

[PATCH bpf-next 0/2] Add support for bpf_setsockopt and bpf_getsockopt from BPF setsockopt

[Prankur gupta]

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

[PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF

[Prankur gupta]

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

[PATCH bpf-next 2/2] selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program

[Prankur gupta]

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

Re: [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF

[Song Liu]

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>

Re: [PATCH bpf-next 2/2] selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program

[Song Liu]

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
>

Re: [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF

[Andrii Nakryiko]

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
>

Re: [PATCH bpf-next 2/2] selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program

[Song Liu]

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
> >

Re: [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF

[Prankur gupta]

>>
>> 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
>>

Re: [PATCH bpf-next 1/2] bpf: Add support for {set|get} socket options from setsockopt BPF

[Martin KaFai Lau]

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.

Re: [PATCH bpf-next 2/2] selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program

[Song Liu]

On Tue, Aug 17, 2021 at 3:43 PM Prankur gupta <prankgup@fb.com> wrote:
>
> Adding selftests for new added functionality to call bpf_setsockopt and
> bpf_getsockopt from setsockopt BPF programs
>
> Test Details:
> 1. BPF Program
>    Checks for changes in IPV6_TCLASS(SOL_IPV6) via setsockopt
>    If the cca for the socket is not cubic do nothing
>    If the newly set value for IPV6_TCLASS is 45 (0x2d) (as per our usecase)
>    then change the cc from cubic to reno
>
> 2. User Space Program
>    Creates an AF_INET6 socket and set the cca for that to be "cubic"
>    Attach the program and set the IPV6_TCLASS to 0x2d using setsockopt
>    Verify the cca for the socket changed to reno
>
> Signed-off-by: Prankur gupta <prankgup@fb.com>

Acked-by: Song Liu <songliubraving@fb.com>

[PATCH bpf-next 2/2] selftests/bpf: Add test for {set|get} socket option from setsockopt BPF program

[Prankur gupta]

Adding selftests for new added functionality to call bpf_setsockopt and
bpf_getsockopt from setsockopt BPF programs

Test Details:
1. BPF Program
   Checks for changes in IPV6_TCLASS(SOL_IPV6) via setsockopt
   If the cca for the socket is not cubic do nothing
   If the newly set value for IPV6_TCLASS is 45 (0x2d) (as per our usecase)
   then change the cc from cubic to reno

2. User Space Program
   Creates an AF_INET6 socket and set the cca for that to be "cubic"
   Attach the program and set the IPV6_TCLASS to 0x2d using setsockopt
   Verify the cca for the socket changed to reno

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