* [PATCH bpf-next v5 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB @ 2022-01-11 0:00 Tyler Wear 2022-01-11 0:00 ` [PATCH bpf-next v5 2/2] selftests/bpf: CGROUP_SKB test for skb_store_bytes() Tyler Wear 2022-01-12 19:57 ` [PATCH bpf-next v5 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB Martin KaFai Lau 0 siblings, 2 replies; 4+ messages in thread From: Tyler Wear @ 2022-01-11 0:00 UTC (permalink / raw) To: netdev, bpf; +Cc: maze, yhs, kafai, toke, daniel, song, 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 ++++++++++ 1 file changed, 10 insertions(+) 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; -- 2.25.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH bpf-next v5 2/2] selftests/bpf: CGROUP_SKB test for skb_store_bytes() 2022-01-11 0:00 [PATCH bpf-next v5 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB Tyler Wear @ 2022-01-11 0:00 ` Tyler Wear 2022-01-12 21:01 ` Martin KaFai Lau 2022-01-12 19:57 ` [PATCH bpf-next v5 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB Martin KaFai Lau 1 sibling, 1 reply; 4+ messages in thread From: Tyler Wear @ 2022-01-11 0:00 UTC (permalink / raw) To: netdev, bpf; +Cc: maze, yhs, kafai, toke, daniel, song, 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 | 81 +++++++++++++++++++ .../selftests/bpf/progs/cgroup_store_bytes.c | 63 +++++++++++++++ 2 files changed, 144 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..f492fdb2f31b --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c @@ -0,0 +1,81 @@ +// 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; + struct sockaddr server_addr; + socklen_t addrlen = sizeof(server_addr); + char buf[] = "testing"; + struct sockaddr_storage ss; + char recv_buf[BUFSIZ]; + socklen_t slen; + 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_cgroup_fd; + + client_fd = start_server(AF_INET, SOCK_DGRAM, NULL, 0, 0); + if (!ASSERT_GE(client_fd, 0, "client_fd")) + goto close_server_fd; + + if (getsockname(server_fd, &server_addr, &addrlen)) { + perror("Failed to get server addr"); + goto close_client_fd; + } + + 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; + } + + if (recvfrom(server_fd, &recv_buf, sizeof(recv_buf), 0, + (struct sockaddr *)&ss, &slen) <= 0) { + perror("Recvfrom received no packets"); + goto close_client_fd; + } + + addr = ((struct sockaddr_in *)&ss)->sin_addr; + + CHECK(addr.s_addr != 0xac100164, "bpf", "bpf program failed to change saddr"); + + port = ((struct sockaddr_in *)&ss)->sin_port; + + CHECK(port != htons(5555), "bpf", "bpf program failed to change port"); + + CHECK(skel->bss->test_result != 1, "bpf", "bpf program returned failure"); + +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..d81d39281526 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/cgroup_store_bytes.c @@ -0,0 +1,63 @@ +// 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; + + 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: + test_result = test_passed; + + return 1; +} -- 2.25.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next v5 2/2] selftests/bpf: CGROUP_SKB test for skb_store_bytes() 2022-01-11 0:00 ` [PATCH bpf-next v5 2/2] selftests/bpf: CGROUP_SKB test for skb_store_bytes() Tyler Wear @ 2022-01-12 21:01 ` Martin KaFai Lau 0 siblings, 0 replies; 4+ messages in thread From: Martin KaFai Lau @ 2022-01-12 21:01 UTC (permalink / raw) To: Tyler Wear; +Cc: netdev, bpf, maze, yhs, toke, daniel, song On Mon, Jan 10, 2022 at 04:00:01PM -0800, Tyler Wear 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(). > > 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 | 81 +++++++++++++++++++ > .../selftests/bpf/progs/cgroup_store_bytes.c | 63 +++++++++++++++ > 2 files changed, 144 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..f492fdb2f31b > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c > @@ -0,0 +1,81 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +#include <test_progs.h> > +#include <network_helpers.h> > + > +#include "cgroup_store_bytes.skel.h" > + > +static int duration; Replace all CHECK_* with ASSERT_* to avoid this. Song has already mentioned it in v4. > + > +void test_cgroup_store_bytes(void) > +{ > + int server_fd, cgroup_fd, client_fd; > + struct sockaddr server_addr; > + socklen_t addrlen = sizeof(server_addr); > + char buf[] = "testing"; > + struct sockaddr_storage ss; > + char recv_buf[BUFSIZ]; > + socklen_t slen; > + 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_cgroup_fd; Incorrect goto here. > + > + client_fd = start_server(AF_INET, SOCK_DGRAM, NULL, 0, 0); > + if (!ASSERT_GE(client_fd, 0, "client_fd")) > + goto close_server_fd; > + > + if (getsockname(server_fd, &server_addr, &addrlen)) { ASSERT_* test. > + perror("Failed to get server addr"); > + goto close_client_fd; > + } > + > + if (CHECK_FAIL(sendto(client_fd, buf, sizeof(buf), 0, &server_addr, > + sizeof(server_addr)) != sizeof(buf))) { Indentation is off. > + perror("Can't write on client"); > + goto close_client_fd; > + } > + > + if (recvfrom(server_fd, &recv_buf, sizeof(recv_buf), 0, > + (struct sockaddr *)&ss, &slen) <= 0) { Also missed ASSERT_* test. and "slen" is not init. At best slen could be 0. However, I am not sure how this test could pass considering "ss" is used in the following CHECK. If the test did PASSED, my best guess is recvfrom() did fail here but missing ASSERT_* test just makes it failed silently. Indentation is off also. > + perror("Recvfrom received no packets"); If recvfrom() did fail, running with -v will print this message, like ./test_progs -v -t cgroup_store_bytes > + goto close_client_fd; > + } > + > + addr = ((struct sockaddr_in *)&ss)->sin_addr; > + > + CHECK(addr.s_addr != 0xac100164, "bpf", "bpf program failed to change saddr"); > + > + port = ((struct sockaddr_in *)&ss)->sin_port; > + > + CHECK(port != htons(5555), "bpf", "bpf program failed to change port"); > + > + CHECK(skel->bss->test_result != 1, "bpf", "bpf program returned failure"); > + > +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..d81d39281526 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/cgroup_store_bytes.c > @@ -0,0 +1,63 @@ > +// 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; > + > + if (bpf_skb_load_bytes_relative(skb, 0, &iph, sizeof(iph), > + BPF_HDR_START_NET)) Indentation is off. > + goto fail; > + > + if (bpf_skb_load_bytes_relative(skb, sizeof(iph), &udph, sizeof(udph), > + BPF_HDR_START_NET)) Same here and a few other places below. > + goto fail; > + > + __u32 old_ip = htonl(iph.saddr); > + __u32 new_ip = 0xac100164; //172.16.1.100 Define all variables at the beginning of the function. Song has also mentioned that in v4. Please address them. and use C style comment /* */ instead of // > + > + 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: > + test_result = test_passed; > + > + return 1; > +} > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next v5 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB 2022-01-11 0:00 [PATCH bpf-next v5 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB Tyler Wear 2022-01-11 0:00 ` [PATCH bpf-next v5 2/2] selftests/bpf: CGROUP_SKB test for skb_store_bytes() Tyler Wear @ 2022-01-12 19:57 ` Martin KaFai Lau 1 sibling, 0 replies; 4+ messages in thread From: Martin KaFai Lau @ 2022-01-12 19:57 UTC (permalink / raw) To: Tyler Wear; +Cc: netdev, bpf, maze, yhs, toke, daniel, song On Mon, Jan 10, 2022 at 04:00:00PM -0800, Tyler Wear 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 ++++++++++ > 1 file changed, 10 insertions(+) > > 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; BPF_FUNC_csum_diff should also be added to support updating >4 bytes (e.g. ipv6 addr). ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-12 21:02 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-11 0:00 [PATCH bpf-next v5 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB Tyler Wear 2022-01-11 0:00 ` [PATCH bpf-next v5 2/2] selftests/bpf: CGROUP_SKB test for skb_store_bytes() Tyler Wear 2022-01-12 21:01 ` Martin KaFai Lau 2022-01-12 19:57 ` [PATCH bpf-next v5 1/2] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB Martin KaFai Lau
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).