| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 12 Jan 2022 13:01:41 -0800
From: Martin KaFai Lau <kafai@...com>
To: Tyler Wear <quic_twear@...cinc.com>
CC: <netdev@...r.kernel.org>, <bpf@...r.kernel.org>, <maze@...gle.com>,
<yhs@...com>, <toke@...hat.com>, <daniel@...earbox.net>,
<song@...nel.org>
Subject: Re: [PATCH bpf-next v5 2/2] selftests/bpf: CGROUP_SKB test for
skb_store_bytes()
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@...cinc.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
>
Powered by blists - more mailing lists