[PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

[PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

[Tyler Wear]

Need to modify the ds field to support upcoming Wifi QoS Alliance spec.
Instead of adding generic function for just modifying the ds field,
add skb_store_bytes for BPF_PROG_TYPE_CGROUP_SKB.
This allows other fields in the network and transport header to be
modified in the future.

Checksum API's also need to be added for completeness.

It is not possible to use CGROUP_(SET|GET)SOCKOPT since
the policy may change during runtime and would result
in a large number of entries with wildcards.

V4 patch fixes warnings and errors from checkpatch.

The existing check for bpf_try_make_writable() should mean that
skb_share_check() is not needed.

Signed-off-by: Tyler Wear <quic_twear@quicinc.com>
---
 net/core/filter.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index 6102f093d59a..f30d939cb4cf 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7299,6 +7299,18 @@ cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_sk_storage_delete_proto;
 	case BPF_FUNC_perf_event_output:
 		return &bpf_skb_event_output_proto;
+	case BPF_FUNC_skb_store_bytes:
+		return &bpf_skb_store_bytes_proto;
+	case BPF_FUNC_csum_update:
+		return &bpf_csum_update_proto;
+	case BPF_FUNC_csum_level:
+		return &bpf_csum_level_proto;
+	case BPF_FUNC_l3_csum_replace:
+		return &bpf_l3_csum_replace_proto;
+	case BPF_FUNC_l4_csum_replace:
+		return &bpf_l4_csum_replace_proto;
+	case BPF_FUNC_csum_diff:
+		return &bpf_csum_diff_proto;
 #ifdef CONFIG_SOCK_CGROUP_DATA
 	case BPF_FUNC_skb_cgroup_id:
 		return &bpf_skb_cgroup_id_proto;
-- 
2.25.1

[PATCH bpf-next v6 2/2] selftests/bpf: CGROUP_SKB test for skb_store_bytes()

[Tyler Wear]

Patch adds a test case to check that the source IP and Port of
packet are correctly changed for BPF_PROG_TYPE_CGROUP_SKB via
skb_store_bytes().

Test creates a client and server and checks that the packet
received on the server has the updated source IP and Port
that the bpf program modifies.

Signed-off-by: Tyler Wear <quic_twear@quicinc.com>
---
 .../bpf/prog_tests/cgroup_store_bytes.c       | 74 +++++++++++++++++++
 .../selftests/bpf/progs/cgroup_store_bytes.c  | 52 +++++++++++++
 2 files changed, 126 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c
 create mode 100644 tools/testing/selftests/bpf/progs/cgroup_store_bytes.c

diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c b/tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c
new file mode 100644
index 000000000000..4338f9db2f88
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <test_progs.h>
+#include <network_helpers.h>
+
+#include "cgroup_store_bytes.skel.h"
+
+static int duration;
+
+void test_cgroup_store_bytes(void)
+{
+	int server_fd, cgroup_fd, client_fd, err, bytes;
+	struct sockaddr server_addr;
+	socklen_t addrlen = sizeof(server_addr);
+	char buf[] = "testing";
+	struct sockaddr_storage ss;
+	socklen_t slen = sizeof(ss);
+	char recv_buf[BUFSIZ];
+	struct in_addr addr;
+	unsigned short port;
+	struct cgroup_store_bytes *skel;
+
+	cgroup_fd = test__join_cgroup("/cgroup_store_bytes");
+	if (!ASSERT_GE(cgroup_fd, 0, "cgroup_fd"))
+		return;
+
+	skel = cgroup_store_bytes__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel"))
+		goto close_cgroup_fd;
+	if (!ASSERT_OK_PTR(skel->bss, "check_bss"))
+		goto close_cgroup_fd;
+
+	skel->links.cgroup_store_bytes = bpf_program__attach_cgroup(
+			skel->progs.cgroup_store_bytes, cgroup_fd);
+	if (!ASSERT_OK_PTR(skel, "cgroup_store_bytes"))
+		goto close_skeleton;
+
+	server_fd = start_server(AF_INET, SOCK_DGRAM, NULL, 0, 0);
+	if (!ASSERT_GE(server_fd, 0, "server_fd"))
+		goto close_skeleton;
+
+	client_fd = start_server(AF_INET, SOCK_DGRAM, NULL, 0, 0);
+	if (!ASSERT_GE(client_fd, 0, "client_fd"))
+		goto close_server_fd;
+
+	err = getsockname(server_fd, &server_addr, &addrlen);
+	if (!ASSERT_OK(err, "getsockname"))
+		goto close_client_fd;
+
+	bytes = sendto(client_fd, buf, sizeof(buf), 0, &server_addr,
+			sizeof(server_addr));
+	if (!ASSERT_EQ(bytes, sizeof(buf), "sendto"))
+		goto close_client_fd;
+
+	bytes = recvfrom(server_fd, &recv_buf, sizeof(recv_buf), 0,
+			(struct sockaddr *)&ss, &slen);
+	if (!ASSERT_GE(bytes, 0, "recvfrom"))
+		goto close_client_fd;
+
+	addr = ((struct sockaddr_in *)&ss)->sin_addr;
+
+	ASSERT_EQ(skel->bss->test_result, 1, "bpf program returned failure");
+	port = ((struct sockaddr_in *)&ss)->sin_port;
+	ASSERT_EQ(port, 5555, "bpf program failed to change port");
+
+close_client_fd:
+	close(client_fd);
+close_server_fd:
+	close(server_fd);
+close_skeleton:
+	cgroup_store_bytes__destroy(skel);
+close_cgroup_fd:
+	close(cgroup_fd);
+}
diff --git a/tools/testing/selftests/bpf/progs/cgroup_store_bytes.c b/tools/testing/selftests/bpf/progs/cgroup_store_bytes.c
new file mode 100644
index 000000000000..c77b360ef4b0
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/cgroup_store_bytes.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <errno.h>
+#include <linux/bpf.h>
+#include <linux/if_ether.h>
+#include <linux/ip.h>
+#include <netinet/in.h>
+#include <netinet/udp.h>
+#include <bpf/bpf_helpers.h>
+
+#define IP_SRC_OFF offsetof(struct iphdr, saddr)
+#define UDP_SPORT_OFF (sizeof(struct iphdr) + offsetof(struct udphdr, source))
+
+#define IS_PSEUDO 0x10
+
+#define UDP_CSUM_OFF (sizeof(struct iphdr) + offsetof(struct udphdr, check))
+#define IP_CSUM_OFF offsetof(struct iphdr, check)
+
+int test_result;
+
+SEC("cgroup_skb/egress")
+int cgroup_store_bytes(struct __sk_buff *skb)
+{
+	struct ethhdr eth;
+	struct iphdr iph;
+	struct udphdr udph;
+
+	__u32 map_key = 0;
+	__u32 test_passed = 0;
+	__u16 new_port = 5555;
+	__u16 old_port;
+	__u32 old_ip;
+
+	if (bpf_skb_load_bytes_relative(skb, 0, &iph, sizeof(iph), BPF_HDR_START_NET))
+		goto fail;
+
+	if (bpf_skb_load_bytes_relative(skb, sizeof(iph), &udph, sizeof(udph), BPF_HDR_START_NET))
+		goto fail;
+
+	old_port = udph.source;
+	bpf_l4_csum_replace(skb, UDP_CSUM_OFF, old_port, new_port,
+						IS_PSEUDO | sizeof(new_port));
+	if (bpf_skb_store_bytes(skb, UDP_SPORT_OFF, &new_port, sizeof(new_port), 0) < 0)
+		goto fail;
+
+	test_passed = 1;
+
+fail:
+	test_result = test_passed;
+
+	return 1;
+}
-- 
2.25.1

Re: [PATCH bpf-next v6 2/2] selftests/bpf: CGROUP_SKB test for skb_store_bytes()

[Song Liu]

On Wed, Jan 12, 2022 at 4:07 PM Tyler Wear <quic_twear@quicinc.com> wrote:
>
> Patch adds a test case to check that the source IP and Port of
> packet are correctly changed for BPF_PROG_TYPE_CGROUP_SKB via
> skb_store_bytes().

Please revise the commit log based on guidance in
Documentation/process/submitting-patches.rst.

Specifically, please refer to this part:

Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour.

>
> Test creates a client and server and checks that the packet
> received on the server has the updated source IP and Port
> that the bpf program modifies.
>
> Signed-off-by: Tyler Wear <quic_twear@quicinc.com>
> ---
>  .../bpf/prog_tests/cgroup_store_bytes.c       | 74 +++++++++++++++++++
>  .../selftests/bpf/progs/cgroup_store_bytes.c  | 52 +++++++++++++
>  2 files changed, 126 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c
>  create mode 100644 tools/testing/selftests/bpf/progs/cgroup_store_bytes.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c b/tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c
> new file mode 100644
> index 000000000000..4338f9db2f88
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +
> +#include "cgroup_store_bytes.skel.h"
> +
> +static int duration;

I think duration is not needed.

> +
> +void test_cgroup_store_bytes(void)
> +{
> +       int server_fd, cgroup_fd, client_fd, err, bytes;
> +       struct sockaddr server_addr;
> +       socklen_t addrlen = sizeof(server_addr);
> +       char buf[] = "testing";
> +       struct sockaddr_storage ss;
> +       socklen_t slen = sizeof(ss);
> +       char recv_buf[BUFSIZ];
> +       struct in_addr addr;
> +       unsigned short port;
> +       struct cgroup_store_bytes *skel;
> +
> +       cgroup_fd = test__join_cgroup("/cgroup_store_bytes");
> +       if (!ASSERT_GE(cgroup_fd, 0, "cgroup_fd"))
> +               return;
> +
> +       skel = cgroup_store_bytes__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "skel"))
> +               goto close_cgroup_fd;
> +       if (!ASSERT_OK_PTR(skel->bss, "check_bss"))
> +               goto close_cgroup_fd;

No need to check skel->bss

> +
> +       skel->links.cgroup_store_bytes = bpf_program__attach_cgroup(
> +                       skel->progs.cgroup_store_bytes, cgroup_fd);
> +       if (!ASSERT_OK_PTR(skel, "cgroup_store_bytes"))
> +               goto close_skeleton;

I think we need to check skel->links.cgroup_store_bytes here, not skel.

> +
> +       server_fd = start_server(AF_INET, SOCK_DGRAM, NULL, 0, 0);
> +       if (!ASSERT_GE(server_fd, 0, "server_fd"))
> +               goto close_skeleton;
> +
> +       client_fd = start_server(AF_INET, SOCK_DGRAM, NULL, 0, 0);
> +       if (!ASSERT_GE(client_fd, 0, "client_fd"))
> +               goto close_server_fd;
> +
> +       err = getsockname(server_fd, &server_addr, &addrlen);
> +       if (!ASSERT_OK(err, "getsockname"))
> +               goto close_client_fd;
> +
> +       bytes = sendto(client_fd, buf, sizeof(buf), 0, &server_addr,
> +                       sizeof(server_addr));
> +       if (!ASSERT_EQ(bytes, sizeof(buf), "sendto"))
> +               goto close_client_fd;
> +
> +       bytes = recvfrom(server_fd, &recv_buf, sizeof(recv_buf), 0,
> +                       (struct sockaddr *)&ss, &slen);
> +       if (!ASSERT_GE(bytes, 0, "recvfrom"))
> +               goto close_client_fd;
> +
> +       addr = ((struct sockaddr_in *)&ss)->sin_addr;
> +
> +       ASSERT_EQ(skel->bss->test_result, 1, "bpf program returned failure");
> +       port = ((struct sockaddr_in *)&ss)->sin_port;
> +       ASSERT_EQ(port, 5555, "bpf program failed to change port");
> +
> +close_client_fd:
> +       close(client_fd);
> +close_server_fd:
> +       close(server_fd);
> +close_skeleton:
> +       cgroup_store_bytes__destroy(skel);
> +close_cgroup_fd:
> +       close(cgroup_fd);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/cgroup_store_bytes.c b/tools/testing/selftests/bpf/progs/cgroup_store_bytes.c
> new file mode 100644
> index 000000000000..c77b360ef4b0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/cgroup_store_bytes.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <errno.h>
> +#include <linux/bpf.h>
> +#include <linux/if_ether.h>
> +#include <linux/ip.h>
> +#include <netinet/in.h>
> +#include <netinet/udp.h>
> +#include <bpf/bpf_helpers.h>
> +
> +#define IP_SRC_OFF offsetof(struct iphdr, saddr)
> +#define UDP_SPORT_OFF (sizeof(struct iphdr) + offsetof(struct udphdr, source))
> +
> +#define IS_PSEUDO 0x10
> +
> +#define UDP_CSUM_OFF (sizeof(struct iphdr) + offsetof(struct udphdr, check))
> +#define IP_CSUM_OFF offsetof(struct iphdr, check)
> +
> +int test_result;

Let's use

int test_result = 0;

to make old clang happy.

> +
> +SEC("cgroup_skb/egress")
> +int cgroup_store_bytes(struct __sk_buff *skb)
> +{
> +       struct ethhdr eth;
> +       struct iphdr iph;
> +       struct udphdr udph;
> +
> +       __u32 map_key = 0;
> +       __u32 test_passed = 0;
> +       __u16 new_port = 5555;
> +       __u16 old_port;
> +       __u32 old_ip;
> +
> +       if (bpf_skb_load_bytes_relative(skb, 0, &iph, sizeof(iph), BPF_HDR_START_NET))
> +               goto fail;
> +
> +       if (bpf_skb_load_bytes_relative(skb, sizeof(iph), &udph, sizeof(udph), BPF_HDR_START_NET))
> +               goto fail;
> +
> +       old_port = udph.source;
> +       bpf_l4_csum_replace(skb, UDP_CSUM_OFF, old_port, new_port,
> +                                               IS_PSEUDO | sizeof(new_port));
> +       if (bpf_skb_store_bytes(skb, UDP_SPORT_OFF, &new_port, sizeof(new_port), 0) < 0)
> +               goto fail;
> +
> +       test_passed = 1;

We can just use test_result here, and remove test_passed, right?

> +
> +fail:
> +       test_result = test_passed;
> +
> +       return 1;
> +}
> --
> 2.25.1
>

Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

[Song Liu]

On Wed, Jan 12, 2022 at 4:07 PM Tyler Wear <quic_twear@quicinc.com> wrote:
>
> Need to modify the ds field to support upcoming Wifi QoS Alliance spec.
> Instead of adding generic function for just modifying the ds field,
> add skb_store_bytes for BPF_PROG_TYPE_CGROUP_SKB.
> This allows other fields in the network and transport header to be
> modified in the future.
>
> Checksum API's also need to be added for completeness.
>
> It is not possible to use CGROUP_(SET|GET)SOCKOPT since
> the policy may change during runtime and would result
> in a large number of entries with wildcards.
>
> V4 patch fixes warnings and errors from checkpatch.

I don't think we need "V4 patch ..." part in the commit log.
To keep some history of patch, we can add these notes after
an "---" line, like:

Signed-off-by:
---
v4 patch ...
v3 patch ...
---
  net/core/filter.c | 12 ++++++++++++
...

With this format, git-am will remove the content between the
two "---" lines.

>
> The existing check for bpf_try_make_writable() should mean that
> skb_share_check() is not needed.
>
> Signed-off-by: Tyler Wear <quic_twear@quicinc.com>
> ---
>  net/core/filter.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 6102f093d59a..f30d939cb4cf 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7299,6 +7299,18 @@ cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return &bpf_sk_storage_delete_proto;
>         case BPF_FUNC_perf_event_output:
>                 return &bpf_skb_event_output_proto;
> +       case BPF_FUNC_skb_store_bytes:
> +               return &bpf_skb_store_bytes_proto;
> +       case BPF_FUNC_csum_update:
> +               return &bpf_csum_update_proto;
> +       case BPF_FUNC_csum_level:
> +               return &bpf_csum_level_proto;
> +       case BPF_FUNC_l3_csum_replace:
> +               return &bpf_l3_csum_replace_proto;
> +       case BPF_FUNC_l4_csum_replace:
> +               return &bpf_l4_csum_replace_proto;
> +       case BPF_FUNC_csum_diff:
> +               return &bpf_csum_diff_proto;
>  #ifdef CONFIG_SOCK_CGROUP_DATA
>         case BPF_FUNC_skb_cgroup_id:
>                 return &bpf_skb_cgroup_id_proto;
> --
> 2.25.1
>

Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

[Alexei Starovoitov]

On Wed, Jan 12, 2022 at 5:15 PM Tyler Wear <quic_twear@quicinc.com> wrote:
>
> Need to modify the ds field to support upcoming Wifi QoS Alliance spec.
> Instead of adding generic function for just modifying the ds field,
> add skb_store_bytes for BPF_PROG_TYPE_CGROUP_SKB.
> This allows other fields in the network and transport header to be
> modified in the future.
>
> Checksum API's also need to be added for completeness.
>
> It is not possible to use CGROUP_(SET|GET)SOCKOPT since
> the policy may change during runtime and would result
> in a large number of entries with wildcards.
>
> V4 patch fixes warnings and errors from checkpatch.
>
> The existing check for bpf_try_make_writable() should mean that
> skb_share_check() is not needed.
>
> Signed-off-by: Tyler Wear <quic_twear@quicinc.com>
> ---
>  net/core/filter.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 6102f093d59a..f30d939cb4cf 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7299,6 +7299,18 @@ cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return &bpf_sk_storage_delete_proto;
>         case BPF_FUNC_perf_event_output:
>                 return &bpf_skb_event_output_proto;
> +       case BPF_FUNC_skb_store_bytes:
> +               return &bpf_skb_store_bytes_proto;
> +       case BPF_FUNC_csum_update:
> +               return &bpf_csum_update_proto;
> +       case BPF_FUNC_csum_level:
> +               return &bpf_csum_level_proto;
> +       case BPF_FUNC_l3_csum_replace:
> +               return &bpf_l3_csum_replace_proto;
> +       case BPF_FUNC_l4_csum_replace:
> +               return &bpf_l4_csum_replace_proto;
> +       case BPF_FUNC_csum_diff:
> +               return &bpf_csum_diff_proto;

This is wrong.
CGROUP_INET_EGRESS bpf prog cannot arbitrary change packet data.
The networking stack populated the IP header at that point.
If the prog changes it to something else it will be
confusing other layers of stack. neigh(L2) will be wrong, etc.
We can still change certain things in the packet, but not arbitrary bytes.

We cannot change the DS field directly in the packet either.
It can only be changed by changing its value in the socket.

TC layer is where packet modifications are allowed.

RE: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

[Tyler Wear]

> -----Original Message-----
> From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Sent: Wednesday, January 12, 2022 6:14 PM
> To: Tyler Wear (QUIC) <quic_twear@quicinc.com>
> Cc: Network Development <netdev@vger.kernel.org>; bpf
> <bpf@vger.kernel.org>; Maciej Żenczykowski <maze@google.com>;
> Yonghong Song <yhs@fb.com>; Martin KaFai Lau <kafai@fb.com>; Toke
> Høiland-Jørgensen <toke@redhat.com>; Daniel Borkmann
> <daniel@iogearbox.net>; Song Liu <song@kernel.org>
> Subject: Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for
> BPF_PROG_TYPE_CGROUP_SKB
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> On Wed, Jan 12, 2022 at 5:15 PM Tyler Wear <quic_twear@quicinc.com>
> wrote:
> >
> > Need to modify the ds field to support upcoming Wifi QoS Alliance spec.
> > Instead of adding generic function for just modifying the ds field,
> > add skb_store_bytes for BPF_PROG_TYPE_CGROUP_SKB.
> > This allows other fields in the network and transport header to be
> > modified in the future.
> >
> > Checksum API's also need to be added for completeness.
> >
> > It is not possible to use CGROUP_(SET|GET)SOCKOPT since the policy may
> > change during runtime and would result in a large number of entries
> > with wildcards.
> >
> > V4 patch fixes warnings and errors from checkpatch.
> >
> > The existing check for bpf_try_make_writable() should mean that
> > skb_share_check() is not needed.
> >
> > Signed-off-by: Tyler Wear <quic_twear@quicinc.com>
> > ---
> >  net/core/filter.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c index
> > 6102f093d59a..f30d939cb4cf 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -7299,6 +7299,18 @@ cg_skb_func_proto(enum bpf_func_id func_id,
> const struct bpf_prog *prog)
> >                 return &bpf_sk_storage_delete_proto;
> >         case BPF_FUNC_perf_event_output:
> >                 return &bpf_skb_event_output_proto;
> > +       case BPF_FUNC_skb_store_bytes:
> > +               return &bpf_skb_store_bytes_proto;
> > +       case BPF_FUNC_csum_update:
> > +               return &bpf_csum_update_proto;
> > +       case BPF_FUNC_csum_level:
> > +               return &bpf_csum_level_proto;
> > +       case BPF_FUNC_l3_csum_replace:
> > +               return &bpf_l3_csum_replace_proto;
> > +       case BPF_FUNC_l4_csum_replace:
> > +               return &bpf_l4_csum_replace_proto;
> > +       case BPF_FUNC_csum_diff:
> > +               return &bpf_csum_diff_proto;
> 
> This is wrong.
> CGROUP_INET_EGRESS bpf prog cannot arbitrary change packet data.
> The networking stack populated the IP header at that point.
> If the prog changes it to something else it will be confusing other layers of
> stack. neigh(L2) will be wrong, etc.
> We can still change certain things in the packet, but not arbitrary bytes.
> 
> We cannot change the DS field directly in the packet either.
> It can only be changed by changing its value in the socket.

Why is the DS field unchangeable, but ecn is changeable?

> 
> TC layer is where packet modifications are allowed.

RE: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

[Tyler Wear (QUIC)]

> -----Original Message-----
> From: Tyler Wear <twear@quicinc.com>
> Sent: Wednesday, January 12, 2022 9:13 PM
> To: Alexei Starovoitov <alexei.starovoitov@gmail.com>; Tyler Wear (QUIC)
> <quic_twear@quicinc.com>
> Cc: Network Development <netdev@vger.kernel.org>; bpf
> <bpf@vger.kernel.org>; Maciej Żenczykowski <maze@google.com>;
> Yonghong Song <yhs@fb.com>; Martin KaFai Lau <kafai@fb.com>; Toke
> Høiland-Jørgensen <toke@redhat.com>; Daniel Borkmann
> <daniel@iogearbox.net>; Song Liu <song@kernel.org>
> Subject: RE: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for
> BPF_PROG_TYPE_CGROUP_SKB
> 
> 
> 
> > -----Original Message-----
> > From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Sent: Wednesday, January 12, 2022 6:14 PM
> > To: Tyler Wear (QUIC) <quic_twear@quicinc.com>
> > Cc: Network Development <netdev@vger.kernel.org>; bpf
> > <bpf@vger.kernel.org>; Maciej Żenczykowski <maze@google.com>;
> Yonghong
> > Song <yhs@fb.com>; Martin KaFai Lau <kafai@fb.com>; Toke
> > Høiland-Jørgensen <toke@redhat.com>; Daniel Borkmann
> > <daniel@iogearbox.net>; Song Liu <song@kernel.org>
> > Subject: Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for
> > BPF_PROG_TYPE_CGROUP_SKB
> >
> > WARNING: This email originated from outside of Qualcomm. Please be
> > wary of any links or attachments, and do not enable macros.
> >
> > On Wed, Jan 12, 2022 at 5:15 PM Tyler Wear <quic_twear@quicinc.com>
> > wrote:
> > >
> > > Need to modify the ds field to support upcoming Wifi QoS Alliance spec.
> > > Instead of adding generic function for just modifying the ds field,
> > > add skb_store_bytes for BPF_PROG_TYPE_CGROUP_SKB.
> > > This allows other fields in the network and transport header to be
> > > modified in the future.
> > >
> > > Checksum API's also need to be added for completeness.
> > >
> > > It is not possible to use CGROUP_(SET|GET)SOCKOPT since the policy
> > > may change during runtime and would result in a large number of
> > > entries with wildcards.
> > >
> > > V4 patch fixes warnings and errors from checkpatch.
> > >
> > > The existing check for bpf_try_make_writable() should mean that
> > > skb_share_check() is not needed.
> > >
> > > Signed-off-by: Tyler Wear <quic_twear@quicinc.com>
> > > ---
> > >  net/core/filter.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/net/core/filter.c b/net/core/filter.c index
> > > 6102f093d59a..f30d939cb4cf 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> > > @@ -7299,6 +7299,18 @@ cg_skb_func_proto(enum bpf_func_id
> func_id,
> > const struct bpf_prog *prog)
> > >                 return &bpf_sk_storage_delete_proto;
> > >         case BPF_FUNC_perf_event_output:
> > >                 return &bpf_skb_event_output_proto;
> > > +       case BPF_FUNC_skb_store_bytes:
> > > +               return &bpf_skb_store_bytes_proto;
> > > +       case BPF_FUNC_csum_update:
> > > +               return &bpf_csum_update_proto;
> > > +       case BPF_FUNC_csum_level:
> > > +               return &bpf_csum_level_proto;
> > > +       case BPF_FUNC_l3_csum_replace:
> > > +               return &bpf_l3_csum_replace_proto;
> > > +       case BPF_FUNC_l4_csum_replace:
> > > +               return &bpf_l4_csum_replace_proto;
> > > +       case BPF_FUNC_csum_diff:
> > > +               return &bpf_csum_diff_proto;
> >
> > This is wrong.
> > CGROUP_INET_EGRESS bpf prog cannot arbitrary change packet data.
> > The networking stack populated the IP header at that point.
> > If the prog changes it to something else it will be confusing other
> > layers of stack. neigh(L2) will be wrong, etc.
> > We can still change certain things in the packet, but not arbitrary bytes.
> >
> > We cannot change the DS field directly in the packet either.
> > It can only be changed by changing its value in the socket.
> 
> Why is the DS field unchangeable, but ecn is changeable?

Per spec the requirement is to modify the ds field of egress packets with DSCP value. Setting ds field on socket will not suffice here.
Another case is where device is a middle-man and needs to modify the packets of a connected tethered client with the DSCP value, using a sock will not be able to change the packet here.

Spec doc can be found at: https://www.wi-fi.org/discover-wi-fi/wi-fi-qos-management

> 
> >
> > TC layer is where packet modifications are allowed.

Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

[Alexei Starovoitov]

On Thu, Jan 13, 2022 at 10:49 PM Tyler Wear (QUIC)
<quic_twear@quicinc.com> wrote:
>
> > > This is wrong.
> > > CGROUP_INET_EGRESS bpf prog cannot arbitrary change packet data.
> > > The networking stack populated the IP header at that point.
> > > If the prog changes it to something else it will be confusing other
> > > layers of stack. neigh(L2) will be wrong, etc.
> > > We can still change certain things in the packet, but not arbitrary bytes.
> > >
> > > We cannot change the DS field directly in the packet either.
> > > It can only be changed by changing its value in the socket.
> >
> > Why is the DS field unchangeable, but ecn is changeable?
>
> Per spec the requirement is to modify the ds field of egress packets with DSCP value. Setting ds field on socket will not suffice here.
> Another case is where device is a middle-man and needs to modify the packets of a connected tethered client with the DSCP value, using a sock will not be able to change the packet here.

If DS field needs to be changed differently for every packet
it's better to use TC layer for this task.
qdiscs may send packets with different DSs to different queues.

Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

[Maciej Żenczykowski]

> > > > This is wrong.
> > > > CGROUP_INET_EGRESS bpf prog cannot arbitrary change packet data.

I agree with this sentiment, which is why the original proposal was
simply to add a helper which is only capable of modifying the
tos/tclass/dscp field, and not any arbitrary bytes.  (note: there
already is such a helper to set the ECN congestion notification bits,
so there's somewhat of a precedent)

> > > > The networking stack populated the IP header at that point.
> > > > If the prog changes it to something else it will be confusing other
> > > > layers of stack. neigh(L2) will be wrong, etc.
> > > > We can still change certain things in the packet, but not arbitrary bytes.
> > > >
> > > > We cannot change the DS field directly in the packet either.

This part I won't agree with.  In most cases there is no DSCP based
routing decision, in which case it seems perfectly reasonable to
change the DSCP bits here.  Indeed last I checked (though this was a
few years ago) the ipv4 tos routing code wasn't even capable of making
sane decisions, because it looks at the bottom 4 bits of the TOS
field, instead of the top 6 bits, ie. you can route on ECN bits, but
you can't route on the full DSCP field.  Additionally afaik the ipv6
tclass routing simply wasn't implemented.  However, I last had to deal
with this probably half a decade ago, on even older kernels, so
perhaps the situation has changed.

Additionally DSCP bits may affect transmit queue selection (for
something like wifi qos / traffic prioritization across multiple
transmit queues with different air-time behaviours - which can use
dscp), so ideally we need dscp to be set *before* the mq qdisc /
dispatch.  I think this implies it needs to happen before tc (though
again, I'm not too certain of the ordering here).

> > > > It can only be changed by changing its value in the socket.

Changing it directly in the socket has two problems:
- it becomes visible to userspace which is undesirable (ie. I've run
across userspace code which will set tos to A, then read it back and
exit/fail/crash if it doesn't see A)
- if the tos bits themselves are an input to the decision about what
tos bits to actually use, then this becomes recursive and basically
impossible to get right.  (for example ssh sets tos to different
values for interactive/bulk (ie. copy) traffic, so using application
selected tos to select wire tos is perfectly reasonable)

> > > Why is the DS field unchangeable, but ecn is changeable?
> >
> > Per spec the requirement is to modify the ds field of egress packets with DSCP value. Setting ds field on socket will not suffice here.
> > Another case is where device is a middle-man and needs to modify the packets of a connected tethered client with the DSCP value, using a sock will not be able to change the packet here.
>
> If DS field needs to be changed differently for every packet
> it's better to use TC layer for this task.
> qdiscs may send packets with different DSs to different queues.

Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

[Alexei Starovoitov]

On Fri, Jan 14, 2022 at 1:18 PM Maciej Żenczykowski <maze@google.com> wrote:
>
> > > > > This is wrong.
> > > > > CGROUP_INET_EGRESS bpf prog cannot arbitrary change packet data.
>
> I agree with this sentiment, which is why the original proposal was
> simply to add a helper which is only capable of modifying the
> tos/tclass/dscp field, and not any arbitrary bytes.  (note: there
> already is such a helper to set the ECN congestion notification bits,
> so there's somewhat of a precedent)

True. bpf_skb_ecn_set_ce() is available to cg_skb progs.
An arbitrary tos rewriting helper would screw it up.

> > > > > The networking stack populated the IP header at that point.
> > > > > If the prog changes it to something else it will be confusing other
> > > > > layers of stack. neigh(L2) will be wrong, etc.
> > > > > We can still change certain things in the packet, but not arbitrary bytes.
> > > > >
> > > > > We cannot change the DS field directly in the packet either.
>
> This part I won't agree with.  In most cases there is no DSCP based
> routing decision, in which case it seems perfectly reasonable to
> change the DSCP bits here.  Indeed last I checked (though this was a
> few years ago) the ipv4 tos routing code wasn't even capable of making
> sane decisions, because it looks at the bottom 4 bits of the TOS
> field, instead of the top 6 bits, ie. you can route on ECN bits, but
> you can't route on the full DSCP field.  Additionally afaik the ipv6
> tclass routing simply wasn't implemented.  However, I last had to deal
> with this probably half a decade ago, on even older kernels, so
> perhaps the situation has changed.
>
> Additionally DSCP bits may affect transmit queue selection (for
> something like wifi qos / traffic prioritization across multiple
> transmit queues with different air-time behaviours - which can use
> dscp), so ideally we need dscp to be set *before* the mq qdisc /
> dispatch.  I think this implies it needs to happen before tc (though
> again, I'm not too certain of the ordering here).

and that's where tc bpf progs are running. Right before qdiscs.
They can change any byte of the packet.
So I still don't see a use case of adding a specific
tos/tclass/dscp rewriting helper to cg_skb when tc bpf prog
are already capable.

RE: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

[Tyler Wear]

> -----Original Message-----
> From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Sent: Tuesday, January 18, 2022 12:38 PM
> To: Maciej Żenczykowski <maze@google.com>
> Cc: Tyler Wear (QUIC) <quic_twear@quicinc.com>; Network Development
> <netdev@vger.kernel.org>; bpf <bpf@vger.kernel.org>; Yonghong Song
> <yhs@fb.com>; Martin KaFai Lau <kafai@fb.com>; Toke Høiland-Jørgensen
> <toke@redhat.com>; Daniel Borkmann <daniel@iogearbox.net>; Song Liu
> <song@kernel.org>
> Subject: Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for
> BPF_PROG_TYPE_CGROUP_SKB
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> On Fri, Jan 14, 2022 at 1:18 PM Maciej Żenczykowski <maze@google.com>
> wrote:
> >
> > > > > > This is wrong.
> > > > > > CGROUP_INET_EGRESS bpf prog cannot arbitrary change packet
> data.
> >
> > I agree with this sentiment, which is why the original proposal was
> > simply to add a helper which is only capable of modifying the
> > tos/tclass/dscp field, and not any arbitrary bytes.  (note: there
> > already is such a helper to set the ECN congestion notification bits,
> > so there's somewhat of a precedent)
> 
> True. bpf_skb_ecn_set_ce() is available to cg_skb progs.
> An arbitrary tos rewriting helper would screw it up.

Patch 1 was for a ds_field helper to modify the top 6 bits of TOS, not an arbitrary rewriting.
This should suffice since it doesn't interfere with CE.

Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB

[Alexei Starovoitov]

On Tue, Jan 18, 2022 at 2:23 PM Tyler Wear <twear@quicinc.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Sent: Tuesday, January 18, 2022 12:38 PM
> > To: Maciej Żenczykowski <maze@google.com>
> > Cc: Tyler Wear (QUIC) <quic_twear@quicinc.com>; Network Development
> > <netdev@vger.kernel.org>; bpf <bpf@vger.kernel.org>; Yonghong Song
> > <yhs@fb.com>; Martin KaFai Lau <kafai@fb.com>; Toke Høiland-Jørgensen
> > <toke@redhat.com>; Daniel Borkmann <daniel@iogearbox.net>; Song Liu
> > <song@kernel.org>
> > Subject: Re: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for
> > BPF_PROG_TYPE_CGROUP_SKB
> >
> > WARNING: This email originated from outside of Qualcomm. Please be wary
> > of any links or attachments, and do not enable macros.
> >
> > On Fri, Jan 14, 2022 at 1:18 PM Maciej Żenczykowski <maze@google.com>
> > wrote:
> > >
> > > > > > > This is wrong.
> > > > > > > CGROUP_INET_EGRESS bpf prog cannot arbitrary change packet
> > data.
> > >
> > > I agree with this sentiment, which is why the original proposal was
> > > simply to add a helper which is only capable of modifying the
> > > tos/tclass/dscp field, and not any arbitrary bytes.  (note: there
> > > already is such a helper to set the ECN congestion notification bits,
> > > so there's somewhat of a precedent)
> >
> > True. bpf_skb_ecn_set_ce() is available to cg_skb progs.
> > An arbitrary tos rewriting helper would screw it up.
>
> Patch 1 was for a ds_field helper to modify the top 6 bits of TOS, not an arbitrary rewriting.
> This should suffice since it doesn't interfere with CE.

I still don't hear the answer why tc bpf is not sufficient.