netdev - Re: [PATCH bpf-next v5 7/7] selftests/bpf: Add selftest for XDP_REDIRECT in bpf_prog

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAADnVQ+j=DO8fMCcpoHmAjrW5sTbhHp_OA4eVpcKcwwRzsvKTA@mail.gmail.com>
Date:   Thu, 6 Jan 2022 09:18:45 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        KP Singh <kpsingh@...nel.org>, Shuah Khan <shuah@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v5 7/7] selftests/bpf: Add selftest for
 XDP_REDIRECT in bpf_prog_run()

On Thu, Jan 6, 2022 at 6:34 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Alexei Starovoitov <alexei.starovoitov@...il.com> writes:
>
> > On Mon, Jan 03, 2022 at 04:08:12PM +0100, Toke Høiland-Jørgensen wrote:
> >> +
> >> +#define NUM_PKTS 3
> >
> > May be send a bit more than 3 packets?
> > Just to test skb_list logic for XDP_PASS.
>
> OK, can do.
>
> >> +
> >> +    /* We setup a veth pair that we can not only XDP_REDIRECT packets
> >> +     * between, but also route them. The test packet (defined above) has
> >> +     * address information so it will be routed back out the same interface
> >> +     * after it has been received, which will allow it to be picked up by
> >> +     * the XDP program on the destination interface.
> >> +     *
> >> +     * The XDP program we run with bpf_prog_run() will cycle through all
> >> +     * four return codes (DROP/PASS/TX/REDIRECT), so we should end up with
> >> +     * NUM_PKTS - 1 packets seen on the dst iface. We match the packets on
> >> +     * the UDP payload.
> >> +     */
> >> +    SYS("ip link add veth_src type veth peer name veth_dst");
> >> +    SYS("ip link set dev veth_src address 00:11:22:33:44:55");
> >> +    SYS("ip link set dev veth_dst address 66:77:88:99:aa:bb");
> >> +    SYS("ip link set dev veth_src up");
> >> +    SYS("ip link set dev veth_dst up");
> >> +    SYS("ip addr add dev veth_src fc00::1/64");
> >> +    SYS("ip addr add dev veth_dst fc00::2/64");
> >> +    SYS("ip neigh add fc00::2 dev veth_src lladdr 66:77:88:99:aa:bb");
> >> +    SYS("sysctl -w net.ipv6.conf.all.forwarding=1");
> >
> > These commands pollute current netns. The test has to create its own netns
> > like other tests do.
>
> Right, will fix.
>
> > The forwarding=1 is odd. Nothing in the comments or commit logs
> > talks about it.
>
> Hmm, yeah, should probably have added an explanation, sorry about that :)
>
> > I'm guessing it's due to patch 6 limitation of picking loopback
> > for XDP_PASS and XDP_TX, right?
> > There is ingress_ifindex field in struct xdp_md.
> > May be use that to setup dev and rxq in test_run in patch 6?
> > Then there will be no need to hack through forwarding=1 ?
>
> No, as you note there's already ingress_ifindex to set the device, and
> the test does use that:
>
> +       memcpy(skel->rodata->expect_dst, &pkt_udp.eth.h_dest, ETH_ALEN);
> +       skel->rodata->ifindex_out = ifindex_src;
> +       ctx_in.ingress_ifindex = ifindex_src;

My point is that this ingress_ifindex should be used instead of loopback.
Otherwise the test_run infra is lying to the xdp program.

> I enable forwarding because the XDP program that counts the packets is
> running on the other end of the veth pair (on veth_dst), while the
> traffic gen is using veth_src as its ingress ifindex. So for XDP_TX and
> XDP_REDIRECT we send the frame back out the veth device, and it ends up
> being processed by the XDP program on veth_dst, and counted.

Not for XDP_TX. If I'm reading patch 6 correctly it gets xmited
out of loopback.

> But when
> the test program returns XDP_PASS, the packet will go up the frame; so
> to get it back to the counting program I enable forwarding and set the
> packet dst IP so that the stack routes it back out the same interface.
>
> I'll admit this is a bit hacky; I guess I can add a second TC ingress
> program that will count the packets being XDP_PASS'ed instead...

No. Please figure out how to XDP_PASS and XDP_TX without enabling forward
and counting in different places.
imo the forwarding hides the issue in the design that should be addressed.
When rx ifindex is an actual ifindex given by user space instead of
loopback all problems go away.