Re: [PATCH bpf 2/2] selftests/bpf: Add test for bpf_skb_change_head

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Apr 27, 2021 at 6:56 AM Jussi Maki <joamaki@gmail.com> 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@gmail.com>
> ---
>  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);
> +}
> +

[...]