netdev - Re: [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAKH8qBv5xvjxF2G78Nm3iOeC5OVygSZK3vVzZgOXT8EwyW+Obg@mail.gmail.com>
Date: Fri, 8 Sep 2023 10:17:20 -0700
From: Stanislav Fomichev <sdf@...gle.com>
To: Daniel Borkmann <daniel@...earbox.net>
Cc: bpf@...r.kernel.org, ast@...nel.org, martin.lau@...ux.dev, 
	netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 2/2] selftests/bpf: Make sure zero-len skbs
 aren't redirectable

On Fri, Sep 8, 2023 at 9:54 AM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> Hi Stan,
>
> Do you have some cycles to look into the below?

Sure, I'll take a look!

> On 11/21/22 7:03 PM, Stanislav Fomichev wrote:
> > LWT_XMIT to test L3 case, TC to test L2 case.
> >
> > v2:
> > - s/veth_ifindex/ipip_ifindex/ in two places (Martin)
> > - add comment about which condition triggers the rejection (Martin)
> >
> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> [...]
> > +             /* ETH_HLEN+1-sized packet should be redirected. */
> > +
> > +             {
> > +                     .msg = "veth ETH_HLEN+1 packet ingress",
> > +                     .data_in = eth_hlen_pp,
> > +                     .data_size_in = sizeof(eth_hlen_pp),
> > +                     .ifindex = &veth_ifindex,
> > +             },
> [...]
>
> This one is now failing in BPF CI on net/net-next ff after the veth driver changed
> it's drop error code in [0] from NETDEV_TX_OK (0) to NET_XMIT_DROP (1) :
>
> test_empty_skb:FAIL:ret: veth ETH_HLEN+1 packet ingress [redirect_egress] unexpected ret: veth ETH_HLEN+1 packet ingress [redirect_egress]: actual 1 != expected 0
> test_empty_skb:PASS:err: veth ETH_HLEN+1 packet ingress [tc_redirect_ingress] 0 nsec
> test_empty_skb:PASS:ret: veth ETH_HLEN+1 packet ingress [tc_redirect_ingress] 0 nsec
> test_empty_skb:PASS:err: veth ETH_HLEN+1 packet ingress [tc_redirect_egress] 0 nsec
> test_empty_skb:PASS:ret: veth ETH_HLEN+1 packet ingress [tc_redirect_egress] 0 nsec
> test_empty_skb:PASS:err: ipip ETH_HLEN+1 packet ingress [redirect_ingress] 0 nsec
> test_empty_skb:PASS:ret: ipip ETH_HLEN+1 packet ingress [redirect_ingress] 0 nsec
> test_empty_skb:PASS:err: ipip ETH_HLEN+1 packet ingress [redirect_egress] 0 nsec
> test_empty_skb:PASS:ret: ipip ETH_HLEN+1 packet ingress [redirect_egress] 0 nsec
> test_empty_skb:PASS:err: ipip ETH_HLEN+1 packet ingress [tc_redirect_ingress] 0 nsec
> test_empty_skb:PASS:ret: ipip ETH_HLEN+1 packet ingress [tc_redirect_ingress] 0 nsec
> test_empty_skb:PASS:err: ipip ETH_HLEN+1 packet ingress [tc_redirect_egress] 0 nsec
> test_empty_skb:PASS:ret: ipip ETH_HLEN+1 packet ingress [tc_redirect_egress] 0 nsec
> close_netns:PASS:setns 0 nsec
> #71      empty_skb:FAIL
>
> The test was testing bpf_clone_redirect which is still okay, just that for the
> xmit sides it propagates the error code now into ret and hence the assert fails.
> Perhaps we would need to tweak the test case to test for 0 or 1 ... 0 in case
> bpf_clone_redirect pushes to ingress, 1 in case it pushes to egress and reaches
> veth..
>
> Thanks,
> Daniel
>
>    [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=151e887d8ff97e2e42110ffa1fb1e6a2128fb364
>
> > +     };
> > +
> > +     SYS("ip netns add empty_skb");
> > +     tok = open_netns("empty_skb");
> > +     SYS("ip link add veth0 type veth peer veth1");
> > +     SYS("ip link set dev veth0 up");
> > +     SYS("ip link set dev veth1 up");
> > +     SYS("ip addr add 10.0.0.1/8 dev veth0");
> > +     SYS("ip addr add 10.0.0.2/8 dev veth1");
> > +     veth_ifindex = if_nametoindex("veth0");
> > +
> > +     SYS("ip link add ipip0 type ipip local 10.0.0.1 remote 10.0.0.2");
> > +     SYS("ip link set ipip0 up");
> > +     SYS("ip addr add 192.168.1.1/16 dev ipip0");
> > +     ipip_ifindex = if_nametoindex("ipip0");
> > +
> > +     bpf_obj = empty_skb__open_and_load();
> > +     if (!ASSERT_OK_PTR(bpf_obj, "open skeleton"))
> > +             goto out;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > +             bpf_object__for_each_program(prog, bpf_obj->obj) {
> > +                     char buf[128];
> > +                     bool at_tc = !strncmp(bpf_program__section_name(prog), "tc", 2);
> > +
> > +                     tattr.data_in = tests[i].data_in;
> > +                     tattr.data_size_in = tests[i].data_size_in;
> > +
> > +                     tattr.data_size_out = 0;
> > +                     bpf_obj->bss->ifindex = *tests[i].ifindex;
> > +                     bpf_obj->bss->ret = 0;
> > +                     err = bpf_prog_test_run_opts(bpf_program__fd(prog), &tattr);
> > +                     sprintf(buf, "err: %s [%s]", tests[i].msg, bpf_program__name(prog));
> > +
> > +                     if (at_tc && tests[i].success_on_tc)
> > +                             ASSERT_GE(err, 0, buf);
> > +                     else
> > +                             ASSERT_EQ(err, tests[i].err, buf);
> > +                     sprintf(buf, "ret: %s [%s]", tests[i].msg, bpf_program__name(prog));
> > +                     if (at_tc && tests[i].success_on_tc)
> > +                             ASSERT_GE(bpf_obj->bss->ret, 0, buf);
> > +                     else
> > +                             ASSERT_EQ(bpf_obj->bss->ret, tests[i].ret, buf);
>