Message-ID: <87tuegafnw.fsf@toke.dk>
Date:   Thu, 06 Jan 2022 19:21:39 +0100
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.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()

Alexei Starovoitov <alexei.starovoitov@...il.com> writes:

> 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.

But it is already using that! There is just no explicit code in patch 6
to do that because that was already part of the XDP prog_run
functionality.

Specifically, the existing bpf_prog_test_run_xdp() will pass the context
through xdp_convert_md_to_buff() which will resolve the ifindex and get
a dev reference. So the xdp_buff object being passed to the new
bpf_test_run_xdp_live() function already has the right device in
ctx->rxq.

I'll add a check for this to the selftest to make it explicit.

>> 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.

See above.

>> 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.

No the problem of XDP_PASS going in the opposite direction of XDP_TX and
XDP_REDIRECT remains. This is just like on a physical interface: if you
XDP_TX a packet it goes back out, if you XDP_PASS it, it goes up the
stack. To intercept both after the fact, you need to look in two
different places.

Anyhow, just using a TC hook for XDP_PASS works fine and gets rid of the
forwarding hack; I'll send a v6 with that just as soon as I verify that
I didn't break anything when running the traffic generator on bare metal :)

-Toke

Powered by blists - more mailing lists