diff mbox series

[bpf-next] selftests/bpf: Fix tc_redirect_dtime

Message ID 20220601234050.2572671-1-kafai@fb.com (mailing list archive)
State Accepted
Commit f7dd4cfb82db28291fb0a3358751e0c7e6733446
Delegated to: BPF
Headers show
Series [bpf-next] selftests/bpf: Fix tc_redirect_dtime | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers fail 1 blamed authors not CCed: davem@davemloft.net; 9 maintainers not CCed: netdev@vger.kernel.org songliubraving@fb.com linux-kselftest@vger.kernel.org yhs@fb.com toke@redhat.com john.fastabend@gmail.com davem@davemloft.net shuah@kernel.org kpsingh@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 126 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-1 success Logs for Kernel LATEST on ubuntu-latest with gcc
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Kernel LATEST on ubuntu-latest with llvm-15
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Kernel LATEST on z15 with gcc

Commit Message

Martin KaFai Lau June 1, 2022, 11:40 p.m. UTC 
tc_redirect_dtime was reported flaky from time to time.  It
always fails at the udp test and complains about the bpf@tc-ingress
got a skb->tstamp when handling udp packet.  It is unexpected
because the skb->tstamp should have been cleared when crossing
different netns.

The most likely cause is that the skb is actually a tcp packet
from the earlier tcp test.  It could be the final TCP_FIN handling.

This patch tightens the skb->tstamp check in the bpf prog.  It ensures
the skb is the current testing traffic.  First, it checks that skb
matches the IPPROTO of the running test (i.e. tcp vs udp).
Second, it checks the server port (dst_ns_port).  The server
port is unique for each test (50000 + test_enum).

Also fixed a typo in test_udp_dtime(): s/P100/P101/

Fixes: c803475fd8dd ("bpf: selftests: test skb->tstamp in redirect_neigh")
Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 .../selftests/bpf/prog_tests/tc_redirect.c    |  8 +--
 .../selftests/bpf/progs/test_tc_dtime.c       | 53 ++++++++++++++++++-
 2 files changed, 55 insertions(+), 6 deletions(-)

Comments

Song Liu June 2, 2022, 5:43 p.m. UTC | #1 
On Wed, Jun 1, 2022 at 4:40 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> tc_redirect_dtime was reported flaky from time to time.  It
> always fails at the udp test and complains about the bpf@tc-ingress
> got a skb->tstamp when handling udp packet.  It is unexpected
> because the skb->tstamp should have been cleared when crossing
> different netns.
>
> The most likely cause is that the skb is actually a tcp packet
> from the earlier tcp test.  It could be the final TCP_FIN handling.
>
> This patch tightens the skb->tstamp check in the bpf prog.  It ensures
> the skb is the current testing traffic.  First, it checks that skb
> matches the IPPROTO of the running test (i.e. tcp vs udp).
> Second, it checks the server port (dst_ns_port).  The server
> port is unique for each test (50000 + test_enum).
>
> Also fixed a typo in test_udp_dtime(): s/P100/P101/
>
> Fixes: c803475fd8dd ("bpf: selftests: test skb->tstamp in redirect_neigh")
> Reported-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>

Acked-by: Song Liu <songliubraving@fb.com>
patchwork-bot+netdevbpf@kernel.org June 2, 2022, 8:30 p.m. UTC | #2 
Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Wed, 1 Jun 2022 16:40:50 -0700 you wrote:
> tc_redirect_dtime was reported flaky from time to time.  It
> always fails at the udp test and complains about the bpf@tc-ingress
> got a skb->tstamp when handling udp packet.  It is unexpected
> because the skb->tstamp should have been cleared when crossing
> different netns.
> 
> The most likely cause is that the skb is actually a tcp packet
> from the earlier tcp test.  It could be the final TCP_FIN handling.
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests/bpf: Fix tc_redirect_dtime
    https://git.kernel.org/bpf/bpf-next/c/f7dd4cfb82db

You are awesome, thank you!
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
index 958dae769c52..cb6a53b3e023 100644
--- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
+++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
@@ -646,7 +646,7 @@  static void test_tcp_clear_dtime(struct test_tc_dtime *skel)
 	__u32 *errs = skel->bss->errs[t];
 
 	skel->bss->test = t;
-	test_inet_dtime(AF_INET6, SOCK_STREAM, IP6_DST, 0);
+	test_inet_dtime(AF_INET6, SOCK_STREAM, IP6_DST, 50000 + t);
 
 	ASSERT_EQ(dtimes[INGRESS_FWDNS_P100], 0,
 		  dtime_cnt_str(t, INGRESS_FWDNS_P100));
@@ -683,7 +683,7 @@  static void test_tcp_dtime(struct test_tc_dtime *skel, int family, bool bpf_fwd)
 	errs = skel->bss->errs[t];
 
 	skel->bss->test = t;
-	test_inet_dtime(family, SOCK_STREAM, addr, 0);
+	test_inet_dtime(family, SOCK_STREAM, addr, 50000 + t);
 
 	/* fwdns_prio100 prog does not read delivery_time_type, so
 	 * kernel puts the (rcv) timetamp in __sk_buff->tstamp
@@ -715,13 +715,13 @@  static void test_udp_dtime(struct test_tc_dtime *skel, int family, bool bpf_fwd)
 	errs = skel->bss->errs[t];
 
 	skel->bss->test = t;
-	test_inet_dtime(family, SOCK_DGRAM, addr, 0);
+	test_inet_dtime(family, SOCK_DGRAM, addr, 50000 + t);
 
 	ASSERT_EQ(dtimes[INGRESS_FWDNS_P100], 0,
 		  dtime_cnt_str(t, INGRESS_FWDNS_P100));
 	/* non mono delivery time is not forwarded */
 	ASSERT_EQ(dtimes[INGRESS_FWDNS_P101], 0,
-		  dtime_cnt_str(t, INGRESS_FWDNS_P100));
+		  dtime_cnt_str(t, INGRESS_FWDNS_P101));
 	for (i = EGRESS_FWDNS_P100; i < SET_DTIME; i++)
 		ASSERT_GT(dtimes[i], 0, dtime_cnt_str(t, i));
 
diff --git a/tools/testing/selftests/bpf/progs/test_tc_dtime.c b/tools/testing/selftests/bpf/progs/test_tc_dtime.c
index 06f300d06dbd..b596479a9ebe 100644
--- a/tools/testing/selftests/bpf/progs/test_tc_dtime.c
+++ b/tools/testing/selftests/bpf/progs/test_tc_dtime.c
@@ -11,6 +11,8 @@ 
 #include <linux/in.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_endian.h>
 #include <sys/socket.h>
@@ -115,6 +117,19 @@  static bool bpf_fwd(void)
 	return test < TCP_IP4_RT_FWD;
 }
 
+static __u8 get_proto(void)
+{
+	switch (test) {
+	case UDP_IP4:
+	case UDP_IP6:
+	case UDP_IP4_RT_FWD:
+	case UDP_IP6_RT_FWD:
+		return IPPROTO_UDP;
+	default:
+		return IPPROTO_TCP;
+	}
+}
+
 /* -1: parse error: TC_ACT_SHOT
  *  0: not testing traffic: TC_ACT_OK
  * >0: first byte is the inet_proto, second byte has the netns
@@ -122,11 +137,16 @@  static bool bpf_fwd(void)
  */
 static int skb_get_type(struct __sk_buff *skb)
 {
+	__u16 dst_ns_port = __bpf_htons(50000 + test);
 	void *data_end = ctx_ptr(skb->data_end);
 	void *data = ctx_ptr(skb->data);
 	__u8 inet_proto = 0, ns = 0;
 	struct ipv6hdr *ip6h;
+	__u16 sport, dport;
 	struct iphdr *iph;
+	struct tcphdr *th;
+	struct udphdr *uh;
+	void *trans;
 
 	switch (skb->protocol) {
 	case __bpf_htons(ETH_P_IP):
@@ -138,6 +158,7 @@  static int skb_get_type(struct __sk_buff *skb)
 		else if (iph->saddr == ip4_dst)
 			ns = DST_NS;
 		inet_proto = iph->protocol;
+		trans = iph + 1;
 		break;
 	case __bpf_htons(ETH_P_IPV6):
 		ip6h = data + sizeof(struct ethhdr);
@@ -148,15 +169,43 @@  static int skb_get_type(struct __sk_buff *skb)
 		else if (v6_equal(ip6h->saddr, (struct in6_addr)ip6_dst))
 			ns = DST_NS;
 		inet_proto = ip6h->nexthdr;
+		trans = ip6h + 1;
 		break;
 	default:
 		return 0;
 	}
 
-	if ((inet_proto != IPPROTO_TCP && inet_proto != IPPROTO_UDP) || !ns)
+	/* skb is not from src_ns or dst_ns.
+	 * skb is not the testing IPPROTO.
+	 */
+	if (!ns || inet_proto != get_proto())
 		return 0;
 
-	return (ns << 8 | inet_proto);
+	switch (inet_proto) {
+	case IPPROTO_TCP:
+		th = trans;
+		if (th + 1 > data_end)
+			return -1;
+		sport = th->source;
+		dport = th->dest;
+		break;
+	case IPPROTO_UDP:
+		uh = trans;
+		if (uh + 1 > data_end)
+			return -1;
+		sport = uh->source;
+		dport = uh->dest;
+		break;
+	default:
+		return 0;
+	}
+
+	/* The skb is the testing traffic */
+	if ((ns == SRC_NS && dport == dst_ns_port) ||
+	    (ns == DST_NS && sport == dst_ns_port))
+		return (ns << 8 | inet_proto);
+
+	return 0;
 }
 
 /* format: direction@iface@netns