Date:   Thu, 06 Jan 2022 21:20:51 +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 10:21 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>>
>> 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.
>
> Got it. Please make it clear in the commit log.

Ah, sorry, already hit send on v6 before I saw this. If you want to fix
up the commit message while applying, how about a paragraph at the end
like:


The new mode reuses the setup code from the existing bpf_prog_run() for
XDP. This means that userspace can set the ingress ifindex and RXQ
number as part of the context object being passed to the kernel, in
which case packets will look like they arrived on that interface when
the test program returns XDP_PASS and the packets go up the stack.

>> 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 :)
>
> Got it. You mean a tc ingress prog attached to veth_src ? That should work.

Yup, exactly!

-Toke

Powered by blists - more mailing lists