On Wed, Jan 12, 2022 at 4:07 PM Tyler Wear <quic_twear@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
> .../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
>
