* [PATCH bpf-next v4] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB
@ 2022-01-07 20:06 Tyler Wear
2022-01-10 19:53 ` Song Liu
0 siblings, 1 reply; 2+ messages in thread
From: Tyler Wear @ 2022-01-07 20:06 UTC (permalink / raw)
To: netdev, bpf; +Cc: maze, yhs, kafai, toke, daniel, 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 | 10 ++
.../bpf/prog_tests/cgroup_store_bytes.c | 104 ++++++++++++++++++
.../selftests/bpf/progs/cgroup_store_bytes.c | 69 ++++++++++++
3 files changed, 183 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/net/core/filter.c b/net/core/filter.c
index 6102f093d59a..ce01a8036361 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7299,6 +7299,16 @@ 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;
#ifdef CONFIG_SOCK_CGROUP_DATA
case BPF_FUNC_skb_cgroup_id:
return &bpf_skb_cgroup_id_proto;
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..4b87ff003008
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <test_progs.h>
+#include <network_helpers.h>
+
+void test_cgroup_store_bytes(void)
+{
+ int server_fd, cgroup_fd, prog_fd, map_fd, client_fd;
+ int err;
+ struct bpf_object *obj;
+ struct bpf_program *prog;
+ struct bpf_map *test_result;
+ __u32 duration = 0;
+
+ __u32 map_key = 0;
+ __u32 map_value = 0;
+
+ cgroup_fd = test__join_cgroup("/cgroup_store_bytes");
+ if (CHECK_FAIL(cgroup_fd < 0))
+ return;
+
+ server_fd = start_server(AF_INET, SOCK_DGRAM, NULL, 0, 0);
+ if (CHECK_FAIL(server_fd < 0))
+ goto close_cgroup_fd;
+
+ err = bpf_prog_load("./cgroup_store_bytes.o", BPF_PROG_TYPE_CGROUP_SKB,
+ &obj, &prog_fd);
+
+ if (CHECK_FAIL(err))
+ goto close_server_fd;
+
+ test_result = bpf_object__find_map_by_name(obj, "test_result");
+ if (CHECK_FAIL(!test_result))
+ goto close_bpf_object;
+
+ map_fd = bpf_map__fd(test_result);
+ if (map_fd < 0)
+ goto close_bpf_object;
+
+ prog = bpf_object__find_program_by_name(obj, "cgroup_store_bytes");
+ if (CHECK_FAIL(!prog))
+ goto close_bpf_object;
+
+ err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_INET_EGRESS,
+ BPF_F_ALLOW_MULTI);
+ if (CHECK_FAIL(err))
+ goto close_bpf_object;
+
+ client_fd = start_server(AF_INET, SOCK_DGRAM, NULL, 0, 0);
+ if (CHECK_FAIL(client_fd < 0))
+ goto close_bpf_object;
+
+ struct sockaddr server_addr;
+ socklen_t addrlen = sizeof(server_addr);
+
+ if (getsockname(server_fd, &server_addr, &addrlen)) {
+ perror("Failed to get server addr");
+ return -1;
+ }
+
+ char buf[] = "testing";
+
+ if (CHECK_FAIL(sendto(client_fd, buf, sizeof(buf), 0, &server_addr,
+ sizeof(server_addr)) != sizeof(buf))) {
+ perror("Can't write on client");
+ goto close_client_fd;
+ }
+
+ struct sockaddr_storage ss;
+ char recv_buf[BUFSIZ];
+ socklen_t slen;
+
+ if (recvfrom(server_fd, &recv_buf, sizeof(recv_buf), 0,
+ (struct sockaddr *)&ss, &slen) <= 0) {
+ perror("Recvfrom received no packets");
+ goto close_client_fd;
+ }
+
+ struct in_addr addr = ((struct sockaddr_in *)&ss)->sin_addr;
+
+ CHECK(addr.s_addr != 0xac100164, "bpf", "bpf program failed to change saddr");
+
+ unsigned short port = ((struct sockaddr_in *)&ss)->sin_port;
+
+ CHECK(port != htons(5555), "bpf", "bpf program failed to change port");
+
+ err = bpf_map_lookup_elem(map_fd, &map_key, &map_value);
+ if (CHECK_FAIL(err))
+ goto close_client_fd;
+
+ CHECK(map_value != 1, "bpf", "bpf program returned failure");
+
+close_client_fd:
+ close(client_fd);
+
+close_bpf_object:
+ bpf_object__close(obj);
+
+close_server_fd:
+ close(server_fd);
+
+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..dc28e46c5069
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/cgroup_store_bytes.c
@@ -0,0 +1,69 @@
+// 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)
+#define TOS_OFF offsetof(struct iphdr, tos)
+
+struct {
+ __uint(type, BPF_MAP_TYPE_ARRAY);
+ __uint(max_entries, 1);
+ __type(key, __u32);
+ __type(value, __u32);
+} test_result SEC(".maps");
+
+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;
+
+ 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;
+
+ __u32 old_ip = htonl(iph.saddr);
+ __u32 new_ip = 0xac100164; //172.16.1.100
+
+ bpf_l4_csum_replace(skb, UDP_CSUM_OFF, old_ip, new_ip,
+ IS_PSEUDO | sizeof(new_ip));
+ bpf_l3_csum_replace(skb, IP_CSUM_OFF, old_ip, new_ip, sizeof(new_ip));
+ if (bpf_skb_store_bytes(skb, IP_SRC_OFF, &new_ip, sizeof(new_ip), 0) < 0)
+ goto fail;
+
+ __u16 old_port = udph.source;
+ __u16 new_port = 5555;
+
+ 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:
+ bpf_map_update_elem(&test_result, &map_key, &test_passed, BPF_ANY);
+
+ return 1;
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH bpf-next v4] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB
2022-01-07 20:06 [PATCH bpf-next v4] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB Tyler Wear
@ 2022-01-10 19:53 ` Song Liu
0 siblings, 0 replies; 2+ messages in thread
From: Song Liu @ 2022-01-10 19:53 UTC (permalink / raw)
To: Tyler Wear
Cc: Networking, bpf, Maciej Żenczykowski, Yonghong Song,
Martin KaFai Lau, Toke Høiland-Jørgensen,
Daniel Borkmann
On Fri, Jan 7, 2022 at 12: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.
>
> 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 | 10 ++
> .../bpf/prog_tests/cgroup_store_bytes.c | 104 ++++++++++++++++++
> .../selftests/bpf/progs/cgroup_store_bytes.c | 69 ++++++++++++
> 3 files changed, 183 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/net/core/filter.c b/net/core/filter.c
> index 6102f093d59a..ce01a8036361 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7299,6 +7299,16 @@ 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;
> #ifdef CONFIG_SOCK_CGROUP_DATA
> case BPF_FUNC_skb_cgroup_id:
> return &bpf_skb_cgroup_id_proto;
Please put changes to selftests in a separate patch.
> 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..4b87ff003008
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +
> +void test_cgroup_store_bytes(void)
> +{
> + int server_fd, cgroup_fd, prog_fd, map_fd, client_fd;
> + int err;
> + struct bpf_object *obj;
> + struct bpf_program *prog;
> + struct bpf_map *test_result;
> + __u32 duration = 0;
> +
> + __u32 map_key = 0;
> + __u32 map_value = 0;
> +
> + cgroup_fd = test__join_cgroup("/cgroup_store_bytes");
> + if (CHECK_FAIL(cgroup_fd < 0))
Please use ASSERT_* macros as much as possible, for example, use
ASSERT_GE for fd.
> + return;
> +
> + server_fd = start_server(AF_INET, SOCK_DGRAM, NULL, 0, 0);
> + if (CHECK_FAIL(server_fd < 0))
> + goto close_cgroup_fd;
> +
> + err = bpf_prog_load("./cgroup_store_bytes.o", BPF_PROG_TYPE_CGROUP_SKB,
> + &obj, &prog_fd);
Can we use bpf skeleton to simplify the code?
> +
> + if (CHECK_FAIL(err))
> + goto close_server_fd;
> +
> + test_result = bpf_object__find_map_by_name(obj, "test_result");
> + if (CHECK_FAIL(!test_result))
> + goto close_bpf_object;
> +
> + map_fd = bpf_map__fd(test_result);
> + if (map_fd < 0)
> + goto close_bpf_object;
> +
> + prog = bpf_object__find_program_by_name(obj, "cgroup_store_bytes");
> + if (CHECK_FAIL(!prog))
> + goto close_bpf_object;
> +
> + err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_INET_EGRESS,
> + BPF_F_ALLOW_MULTI);
> + if (CHECK_FAIL(err))
> + goto close_bpf_object;
> +
> + client_fd = start_server(AF_INET, SOCK_DGRAM, NULL, 0, 0);
> + if (CHECK_FAIL(client_fd < 0))
> + goto close_bpf_object;
> +
> + struct sockaddr server_addr;
Please put all variable declarations at the beginning of the function.
> + socklen_t addrlen = sizeof(server_addr);
> +
> + if (getsockname(server_fd, &server_addr, &addrlen)) {
> + perror("Failed to get server addr");
> + return -1;
> + }
> +
> + char buf[] = "testing";
> +
> + if (CHECK_FAIL(sendto(client_fd, buf, sizeof(buf), 0, &server_addr,
> + sizeof(server_addr)) != sizeof(buf))) {
> + perror("Can't write on client");
> + goto close_client_fd;
> + }
> +
> + struct sockaddr_storage ss;
> + char recv_buf[BUFSIZ];
> + socklen_t slen;
> +
> + if (recvfrom(server_fd, &recv_buf, sizeof(recv_buf), 0,
> + (struct sockaddr *)&ss, &slen) <= 0) {
> + perror("Recvfrom received no packets");
> + goto close_client_fd;
> + }
> +
> + struct in_addr addr = ((struct sockaddr_in *)&ss)->sin_addr;
> +
> + CHECK(addr.s_addr != 0xac100164, "bpf", "bpf program failed to change saddr");
> +
> + unsigned short port = ((struct sockaddr_in *)&ss)->sin_port;
> +
> + CHECK(port != htons(5555), "bpf", "bpf program failed to change port");
> +
> + err = bpf_map_lookup_elem(map_fd, &map_key, &map_value);
> + if (CHECK_FAIL(err))
> + goto close_client_fd;
> +
> + CHECK(map_value != 1, "bpf", "bpf program returned failure");
> +
> +close_client_fd:
> + close(client_fd);
> +
> +close_bpf_object:
> + bpf_object__close(obj);
> +
> +close_server_fd:
> + close(server_fd);
> +
> +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..dc28e46c5069
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/cgroup_store_bytes.c
> @@ -0,0 +1,69 @@
> +// 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)
> +#define TOS_OFF offsetof(struct iphdr, tos)
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_ARRAY);
> + __uint(max_entries, 1);
> + __type(key, __u32);
> + __type(value, __u32);
> +} test_result SEC(".maps");
We can just use a global variable here. The compiler will put it in
a map (bss or data).
> +
> +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;
> +
> + 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;
> +
> + __u32 old_ip = htonl(iph.saddr);
> + __u32 new_ip = 0xac100164; //172.16.1.100
> +
> + bpf_l4_csum_replace(skb, UDP_CSUM_OFF, old_ip, new_ip,
> + IS_PSEUDO | sizeof(new_ip));
> + bpf_l3_csum_replace(skb, IP_CSUM_OFF, old_ip, new_ip, sizeof(new_ip));
> + if (bpf_skb_store_bytes(skb, IP_SRC_OFF, &new_ip, sizeof(new_ip), 0) < 0)
> + goto fail;
> +
> + __u16 old_port = udph.source;
> + __u16 new_port = 5555;
> +
> + 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:
> + bpf_map_update_elem(&test_result, &map_key, &test_passed, BPF_ANY);
> +
> + return 1;
> +}
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-01-10 19:53 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-07 20:06 [PATCH bpf-next v4] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB Tyler Wear
2022-01-10 19:53 ` Song Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).