On Tue, Apr 27, 2021 at 6:56 AM Jussi Maki <joamaki@xxxxxxxxx> wrote:
>
> Adds test to check that bpf_skb_change_head can be used
> in combination with bpf_redirect_peer to redirect a packet
> from L3 device to veth.
>
> Fixes: a426d97e970d ("bpf: Set mac_len in bpf_skb_change_head")
> Signed-off-by: Jussi Maki <joamaki@xxxxxxxxx>
> ---
> tools/testing/selftests/bpf/.gitignore | 1 +
> tools/testing/selftests/bpf/Makefile | 2 +-
> .../selftests/bpf/progs/test_tc_peer.c | 24 ++++
> .../testing/selftests/bpf/test_tc_peer_user.c | 127 ++++++++++++++++++
> .../testing/selftests/bpf/test_tc_redirect.sh | 95 ++++++++++---
> 5 files changed, 233 insertions(+), 16 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/test_tc_peer_user.c
>
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index 4866f6a21901..49f3f050be4d 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -27,6 +27,7 @@ test_tcpnotify_user
> test_libbpf
> test_tcp_check_syncookie_user
> test_sysctl
> +test_tc_peer_user
can we make it into a reasonable test inside test_progs? that way it
will be executed regularly
> xdping
> test_cpp
> *.skel.h
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 283e5ad8385e..0e05fefe2333 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -84,7 +84,7 @@ TEST_PROGS_EXTENDED := with_addr.sh \
> TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
> flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
> test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
> - xdpxceiver
> + xdpxceiver test_tc_peer_user
>
> TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read
>
> diff --git a/tools/testing/selftests/bpf/progs/test_tc_peer.c b/tools/testing/selftests/bpf/progs/test_tc_peer.c
> index fc84a7685aa2..49f0eefd58e7 100644
> --- a/tools/testing/selftests/bpf/progs/test_tc_peer.c
> +++ b/tools/testing/selftests/bpf/progs/test_tc_peer.c
> @@ -5,8 +5,11 @@
> #include <linux/bpf.h>
> #include <linux/stddef.h>
> #include <linux/pkt_cls.h>
> +#include <linux/if_ether.h>
> +#include <linux/ip.h>
>
> #include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_endian.h>
>
> enum {
> dev_src,
> @@ -42,4 +45,25 @@ SEC("src_ingress") int tc_src(struct __sk_buff *skb)
> return bpf_redirect_peer(get_dev_ifindex(dev_dst), 0);
> }
>
> +SEC("src_ingress_l3") int tc_src_l3(struct __sk_buff *skb)
please keep SEC() on separate line
also, is this a BPF_PROG_TYPE_SCHED_CLS program? Can you usee libbpf's
"classifier/src_ingress_l3" naming convention?
> +{
> + __u16 proto = skb->protocol;
> +
> + if (bpf_skb_change_head(skb, ETH_HLEN, 0) != 0)
> + return TC_ACT_SHOT;
> +
> + __u8 src_mac[] = {0x00, 0x11, 0x22, 0x33, 0x44, 0x55};
we try to keep BPF code compliant with C89, so please move all the
variable declaration into a single block at the top of a function
> + if (bpf_skb_store_bytes(skb, 0, &src_mac, ETH_ALEN, 0) != 0)
> + return TC_ACT_SHOT;
> +
> + __u8 dst_mac[] = {0x00, 0x22, 0x33, 0x44, 0x55, 0x66};
> + if (bpf_skb_store_bytes(skb, ETH_ALEN, &dst_mac, ETH_ALEN, 0) != 0)
> + return TC_ACT_SHOT;
> +
> + if (bpf_skb_store_bytes(skb, ETH_ALEN + ETH_ALEN, &proto, sizeof(__u16), 0) != 0)
> + return TC_ACT_SHOT;
> +
> + return bpf_redirect_peer(get_dev_ifindex(dev_dst), 0);
> +}
> +
[...]
> +#define MAX(a, b) ((a) > (b) ? (a) : (b))
> +
> +enum {
> + SRC_TO_TARGET = 0,
> + TARGET_TO_SRC = 1,
> +};
> +
> +void setns_by_name(char *name) {
{ should be on a new line
please run scripts/checkpatch.pl on these files, it will point out
trivial issues like this
> + int nsfd;
> + char nspath[PATH_MAX];
> +
> + snprintf(nspath, sizeof(nspath), "%s/%s", "/var/run/netns", name);
> + nsfd = open(nspath, O_RDONLY | O_CLOEXEC);
> + if (nsfd < 0) {
> + fprintf(stderr, "failed to open net namespace %s: %s\n", name, strerror(errno));
> + exit(1);
> + }
here seems to be a mix of tabs and spaces
> + setns(nsfd, CLONE_NEWNET);
> + close(nsfd);
> +}
> +
[...]
