On 31/01/2024 05:32, Manu Bretelle wrote:
> After a recent change in the vmtest runner, this test started failing
> sporadically.
>
> Investigation showed that this test was subject to race condition which
> got exacerbated after the vm runner change. The symptoms being that the
> logic that waited for an ICMPv4 packet is naive and will break if 5 or
> more non-ICMPv4 packets make it to tap0.
> When ICMPv6 is enabled, the kernel will generate traffic such as ICMPv6
> router solicitation...
> On a system with good performance, the expected ICMPv4 packet would very
> likely make it to the network interface promptly, but on a system with
> poor performance, those "guarantees" do not hold true anymore.
>
> Given that the test is IPv4 only, this change disable IPv6 in the test
> netns by setting `net.ipv6.conf.all.disable_ipv6` to 1.
> This essentially leaves "ping" as the sole generator of traffic in the
> network namespace.
> If this test was to be made IPv6 compatible, the logic in
> `wait_for_packet` would need to be modified.
>
Great to fix test flakiness like this; I was curious if you tried
modifying things from the bpf side; would something like this in
progs/test_lwt_redirect.c help?
(haven't been able to test because I can't reproduce the failure):
static int get_redirect_target(struct __sk_buff *skb)
{
struct iphdr *iph = NULL;
void *start = (void *)(long)skb->data;
void *end = (void *)(long)skb->data_end;
+ if (skb->protocol == __bpf_constant_htons(ETH_P_IPV6))
+ return -1;
I _think_ that would skip redirection and might solve the problem
from the bpf side. Might be worth testing, but not a big deal..
> In more details...
>
> At a high level, the test does:
> - create a new namespace
> - in `setup_redirect_target` set up lo, tap0, and link_err interfaces as
> well as add 2 routes that attaches ingress/egress sections of
> `test_lwt_redirect.bpf.o` to the xmit path.
> - in `send_and_capture_test_packets` send an ICMP packet and read off
> the tap interface (using `wait_for_packet`) to check that a ICMP packet
> with the right size is read.
>
> `wait_for_packet` will try to read `max_retry` (5) times from the tap0
> fd looking for an ICMPv4 packet matching some criteria.
>
> The problem is that when we set up the `tap0` interface, because IPv6 is
> enabled by default, traffic such as Router solicitation is sent through
> tap0, as in:
>
> # tcpdump -r /tmp/lwt_redirect.pc
> reading from file /tmp/lwt_redirect.pcap, link-type EN10MB (Ethernet)
> 04:46:23.578352 IP6 :: > ff02::1:ffc0:4427: ICMP6, neighbor solicitation, who has fe80::fcba:dff:fec0:4427, length 32
> 04:46:23.659522 IP6 :: > ff02::16: HBH ICMP6, multicast listener report v2, 1 group record(s), length 28
> 04:46:24.389169 IP 10.0.0.1 > 20.0.0.9: ICMP echo request, id 122, seq 1, length 108
> 04:46:24.618599 IP6 fe80::fcba:dff:fec0:4427 > ff02::16: HBH ICMP6, multicast listener report v2, 1 group record(s), length 28
> 04:46:24.619985 IP6 fe80::fcba:dff:fec0:4427 > ff02::2: ICMP6, router solicitation, length 16
> 04:46:24.767326 IP6 fe80::fcba:dff:fec0:4427 > ff02::16: HBH ICMP6, multicast listener report v2, 1 group record(s), length 28
> 04:46:28.936402 IP6 fe80::fcba:dff:fec0:4427 > ff02::2: ICMP6, router solicitation, length 16
>
> If `wait_for_packet` sees 5 non-ICMPv4 packets, it will return 0, which is what we see in:
>
> 2024-01-31T03:51:25.0336992Z test_lwt_redirect_run:PASS:netns_create 0 nsec
> 2024-01-31T03:51:25.0341309Z open_netns:PASS:malloc token 0 nsec
> 2024-01-31T03:51:25.0344844Z open_netns:PASS:open /proc/self/ns/net 0 nsec
> 2024-01-31T03:51:25.0350071Z open_netns:PASS:open netns fd 0 nsec
> 2024-01-31T03:51:25.0353516Z open_netns:PASS:setns 0 nsec
> 2024-01-31T03:51:25.0356560Z test_lwt_redirect_run:PASS:setns 0 nsec
> 2024-01-31T03:51:25.0360140Z open_tuntap:PASS:open(/dev/net/tun) 0 nsec
> 2024-01-31T03:51:25.0363822Z open_tuntap:PASS:ioctl(TUNSETIFF) 0 nsec
> 2024-01-31T03:51:25.0367402Z open_tuntap:PASS:fcntl(O_NONBLOCK) 0 nsec
> 2024-01-31T03:51:25.0371167Z setup_redirect_target:PASS:open_tuntap 0 nsec
> 2024-01-31T03:51:25.0375180Z setup_redirect_target:PASS:if_nametoindex 0 nsec
> 2024-01-31T03:51:25.0379929Z setup_redirect_target:PASS:ip link add link_err type dummy 0 nsec
> 2024-01-31T03:51:25.0384874Z setup_redirect_target:PASS:ip link set lo up 0 nsec
> 2024-01-31T03:51:25.0389678Z setup_redirect_target:PASS:ip addr add dev lo 10.0.0.1/32 0 nsec
> 2024-01-31T03:51:25.0394814Z setup_redirect_target:PASS:ip link set link_err up 0 nsec
> 2024-01-31T03:51:25.0399874Z setup_redirect_target:PASS:ip link set tap0 up 0 nsec
> 2024-01-31T03:51:25.0407731Z setup_redirect_target:PASS:ip route add 10.0.0.0/24 dev link_err encap bpf xmit obj test_lwt_redirect.bpf.o sec redir_ingress 0 nsec
> 2024-01-31T03:51:25.0419105Z setup_redirect_target:PASS:ip route add 20.0.0.0/24 dev link_err encap bpf xmit obj test_lwt_redirect.bpf.o sec redir_egress 0 nsec
> 2024-01-31T03:51:25.0427209Z test_lwt_redirect_normal:PASS:setup_redirect_target 0 nsec
> 2024-01-31T03:51:25.0431424Z ping_dev:PASS:if_nametoindex 0 nsec
> 2024-01-31T03:51:25.0437222Z send_and_capture_test_packets:FAIL:wait_for_epacket unexpected wait_for_epacket: actual 0 != expected 1
> 2024-01-31T03:51:25.0448298Z (/tmp/work/bpf/bpf/tools/testing/selftests/bpf/prog_tests/lwt_redirect.c:175: errno: Success) test_lwt_redirect_normal egress test fails
> 2024-01-31T03:51:25.0457124Z close_netns:PASS:setns 0 nsec
>
> When running in a VM which potential resource contrains, the odds that calling
> `ping` is not scheduled very soon after bringing `tap0` up increases,
> and with this the chances to get our ICMP packet pushed to position 6+
> in the network trace.
>
> To confirm this indeed solves the issue, I ran the test 100 times in a
> row with:
>
> errors=0
> successes=0
> for i in `seq 1 100`
> do
> ./test_progs -t lwt_redirect/lwt_redirect_normal
> if [ $? -eq 0 ]; then
> successes=$((successes+1))
> else
> errors=$((errors+1))
> fi
> done
> echo "successes: $successes/errors: $errors"
>
> While this test would at least fail a couple of time every 10 runs, here
> it ran 100 times with no error.
>
> Fixes: 43a7c3ef8a15 ("selftests/bpf: Add lwt_xmit tests for BPF_REDIRECT")
> Signed-off-by: Manu Bretelle <chantr4@xxxxxxxxx>
Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> ---
> tools/testing/selftests/bpf/prog_tests/lwt_redirect.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/lwt_redirect.c b/tools/testing/selftests/bpf/prog_tests/lwt_redirect.c
> index beeb3ac1c361..b5b9e74b1044 100644
> --- a/tools/testing/selftests/bpf/prog_tests/lwt_redirect.c
> +++ b/tools/testing/selftests/bpf/prog_tests/lwt_redirect.c
> @@ -203,6 +203,7 @@ static int setup_redirect_target(const char *target_dev, bool need_mac)
> if (!ASSERT_GE(target_index, 0, "if_nametoindex"))
> goto fail;
>
> + SYS(fail, "sysctl -w net.ipv6.conf.all.disable_ipv6=1");
> SYS(fail, "ip link add link_err type dummy");
> SYS(fail, "ip link set lo up");
> SYS(fail, "ip addr add dev lo " LOCAL_SRC "/32");
