Re: [PATCH bpf-next v7 3/3] selftests/bpf: Add selftest for XDP_REDIRECT in bpf_prog_run()

["Toke Høiland-Jørgensen" <toke@redhat.com>]

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

> On Fri, Jan 7, 2022 at 1:54 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> +
>> +#define NUM_PKTS 1000000
>
> It takes 7 seconds on my kvm with kasan and lockdep
> and will take much longer in BPF CI.
> So it needs to be lower otherwise CI will struggle.

OK, I'll lower it.

>> +       /* The XDP program we run with bpf_prog_run() will cycle through all
>> +        * three xmit (PASS/TX/REDIRECT) return codes starting from above, and
>> +        * ending up with PASS, so we should end up with two packets on the dst
>> +        * iface and NUM_PKTS-2 in the TC hook. We match the packets on the UDP
>> +        * payload.
>> +        */
>
> could you keep cycling through all return codes?
> That should make the test stronger.

Can do.

>> +
>> +       /* We enable forwarding in the test namespace because that will cause
>> +        * the packets that go through the kernel stack (with XDP_PASS) to be
>> +        * forwarded back out the same interface (because of the packet dst
>> +        * combined with the interface addresses). When this happens, the
>> +        * regular forwarding path will end up going through the same
>> +        * veth_xdp_xmit() call as the XDP_REDIRECT code, which can cause a
>> +        * deadlock if it happens on the same CPU. There's a local_bh_disable()
>> +        * in the test_run code to prevent this, but an earlier version of the
>> +        * code didn't have this, so we keep the test behaviour to make sure the
>> +        * bug doesn't resurface.
>> +        */
>> +       SYS("sysctl -qw net.ipv6.conf.all.forwarding=1");
>
> Does it mean that without forwarding=1 the kernel will dead lock ?!

No, the deadlock is referring to the lockdep warning you posted. Which I
fixed by moving around the local_bh_disable(); that comment is just
meant to explain why the forwarding sysctl is set (so that the code path
is still exercised even though it's no longer faulty). Reading it again
now I can see that this was not entirely clear, will try to improve the
wording :)

-Toke