diff mbox series

[PATCHv2,net] selftests/net: use tc rule to filter the na packet

Message ID 20240517010327.2631319-1-liuhangbin@gmail.com
State Accepted
Commit ea63ac14292564eefc7dffe868ed354ff9ed6f4b
Headers show
Series [PATCHv2,net] selftests/net: use tc rule to filter the na packet | expand

Commit Message

Hangbin Liu May 17, 2024, 1:03 a.m. UTC 
Test arp_ndisc_untracked_subnets use tcpdump to filter the unsolicited
and untracked na messages. It set -e before calling tcpdump. But if
tcpdump filters 0 packet, it will return none zero, and cause the script
to exit.

Instead of using slow tcpdump to capture packets, let's using tc rule
to filter out the na message.

At the same time, fix function setup_v6 which only needs one parameter.
Move all the related helpers from forwarding lib.sh to net lib.sh.

Fixes: 0ea7b0a454ca ("selftests: net: arp_ndisc_untracked_subnets: test for arp_accept and accept_untracked_na")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---

v2: rebase to latest net main code. no update,
v1: https://lore.kernel.org/netdev/20240514071130.2121042-1-liuhangbin@gmail.com

---
 .../net/arp_ndisc_untracked_subnets.sh        | 53 ++++++-----------
 tools/testing/selftests/net/forwarding/lib.sh | 58 -------------------
 tools/testing/selftests/net/lib.sh            | 58 +++++++++++++++++++
 3 files changed, 75 insertions(+), 94 deletions(-)

Comments

Simon Horman May 17, 2024, 9:14 a.m. UTC | #1 
On Fri, May 17, 2024 at 09:03:27AM +0800, Hangbin Liu wrote:
> Test arp_ndisc_untracked_subnets use tcpdump to filter the unsolicited
> and untracked na messages. It set -e before calling tcpdump. But if
> tcpdump filters 0 packet, it will return none zero, and cause the script
> to exit.
> 
> Instead of using slow tcpdump to capture packets, let's using tc rule
> to filter out the na message.
> 
> At the same time, fix function setup_v6 which only needs one parameter.
> Move all the related helpers from forwarding lib.sh to net lib.sh.
> 
> Fixes: 0ea7b0a454ca ("selftests: net: arp_ndisc_untracked_subnets: test for arp_accept and accept_untracked_na")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

I see that, as of writing, the last two runs of this test have succeeded.
Which doesn't seem to have occurred since 9th May. So this does seem
positive, albeit perhaps a bit too soon to call.

Jakub, is there a way to tell how long a test took to execute?
Perhaps it's obvious, but I couldn't see it.

Code changes look good to me.

Reviewed-by: Simon Horman <horms@kernel.org>
Jakub Kicinski May 17, 2024, 7:04 p.m. UTC | #2 
On Fri, 17 May 2024 10:14:02 +0100 Simon Horman wrote:
> Jakub, is there a way to tell how long a test took to execute?
> Perhaps it's obvious, but I couldn't see it.

It's not obvious, and it was broken. There's an 'info' file with
extra metadata in the directory with results:

https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/599300/39-arp-ndisc-untracked-subnets-sh/info

but it's currently reporting fractional seconds rather than total
seconds:

https://github.com/linux-netdev/nipa/commit/fb7c45fd3b68b379b7bceb8f79c8df06aaf53ee0

Once we have a proper DB (any day now), I'll add it to the JSON output
so it appears in the web UI.
Simon Horman May 17, 2024, 7:16 p.m. UTC | #3 
On Fri, May 17, 2024 at 12:04:20PM -0700, Jakub Kicinski wrote:
> On Fri, 17 May 2024 10:14:02 +0100 Simon Horman wrote:
> > Jakub, is there a way to tell how long a test took to execute?
> > Perhaps it's obvious, but I couldn't see it.
> 
> It's not obvious, and it was broken. There's an 'info' file with
> extra metadata in the directory with results:
> 
> https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/599300/39-arp-ndisc-untracked-subnets-sh/info
> 
> but it's currently reporting fractional seconds rather than total
> seconds:
> 
> https://github.com/linux-netdev/nipa/commit/fb7c45fd3b68b379b7bceb8f79c8df06aaf53ee0
> 
> Once we have a proper DB (any day now), I'll add it to the JSON output
> so it appears in the web UI.

Great, thanks.
patchwork-bot+netdevbpf@kernel.org May 21, 2024, 11:30 a.m. UTC | #4 
Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 17 May 2024 09:03:27 +0800 you wrote:
> Test arp_ndisc_untracked_subnets use tcpdump to filter the unsolicited
> and untracked na messages. It set -e before calling tcpdump. But if
> tcpdump filters 0 packet, it will return none zero, and cause the script
> to exit.
> 
> Instead of using slow tcpdump to capture packets, let's using tc rule
> to filter out the na message.
> 
> [...]

Here is the summary with links:
  - [PATCHv2,net] selftests/net: use tc rule to filter the na packet
    https://git.kernel.org/netdev/net/c/ea63ac142925

You are awesome, thank you!
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/arp_ndisc_untracked_subnets.sh b/tools/testing/selftests/net/arp_ndisc_untracked_subnets.sh
index a40c0e9bd023..eef5cbf6eecc 100755
--- a/tools/testing/selftests/net/arp_ndisc_untracked_subnets.sh
+++ b/tools/testing/selftests/net/arp_ndisc_untracked_subnets.sh
@@ -73,25 +73,19 @@  setup_v6() {
 	# namespaces. veth0 is veth-router, veth1 is veth-host.
 	# first, set up the inteface's link to the namespace
 	# then, set the interface "up"
-	ip -6 -netns ${ROUTER_NS_V6} link add name ${ROUTER_INTF} \
-		type veth peer name ${HOST_INTF}
-
-	ip -6 -netns ${ROUTER_NS_V6} link set dev ${ROUTER_INTF} up
-	ip -6 -netns ${ROUTER_NS_V6} link set dev ${HOST_INTF} netns \
-		${HOST_NS_V6}
+	ip -n ${ROUTER_NS_V6} link add name ${ROUTER_INTF} \
+		type veth peer name ${HOST_INTF} netns ${HOST_NS_V6}
 
-	ip -6 -netns ${HOST_NS_V6} link set dev ${HOST_INTF} up
-	ip -6 -netns ${ROUTER_NS_V6} addr add \
-		${ROUTER_ADDR_V6}/${PREFIX_WIDTH_V6} dev ${ROUTER_INTF} nodad
+	# Add tc rule to filter out host na message
+	tc -n ${ROUTER_NS_V6} qdisc add dev ${ROUTER_INTF} clsact
+	tc -n ${ROUTER_NS_V6} filter add dev ${ROUTER_INTF} \
+		ingress protocol ipv6 pref 1 handle 101 \
+		flower src_ip ${HOST_ADDR_V6} ip_proto icmpv6 type 136 skip_hw action pass
 
 	HOST_CONF=net.ipv6.conf.${HOST_INTF}
 	ip netns exec ${HOST_NS_V6} sysctl -qw ${HOST_CONF}.ndisc_notify=1
 	ip netns exec ${HOST_NS_V6} sysctl -qw ${HOST_CONF}.disable_ipv6=0
-	ip -6 -netns ${HOST_NS_V6} addr add ${HOST_ADDR_V6}/${PREFIX_WIDTH_V6} \
-		dev ${HOST_INTF}
-
 	ROUTER_CONF=net.ipv6.conf.${ROUTER_INTF}
-
 	ip netns exec ${ROUTER_NS_V6} sysctl -w \
 		${ROUTER_CONF}.forwarding=1 >/dev/null 2>&1
 	ip netns exec ${ROUTER_NS_V6} sysctl -w \
@@ -99,6 +93,13 @@  setup_v6() {
 	ip netns exec ${ROUTER_NS_V6} sysctl -w \
 		${ROUTER_CONF}.accept_untracked_na=${accept_untracked_na} \
 		>/dev/null 2>&1
+
+	ip -n ${ROUTER_NS_V6} link set dev ${ROUTER_INTF} up
+	ip -n ${HOST_NS_V6} link set dev ${HOST_INTF} up
+	ip -n ${ROUTER_NS_V6} addr add ${ROUTER_ADDR_V6}/${PREFIX_WIDTH_V6} \
+		dev ${ROUTER_INTF} nodad
+	ip -n ${HOST_NS_V6} addr add ${HOST_ADDR_V6}/${PREFIX_WIDTH_V6} \
+		dev ${HOST_INTF}
 	set +e
 }
 
@@ -162,26 +163,6 @@  arp_test_gratuitous_combinations() {
 	arp_test_gratuitous 2 1
 }
 
-cleanup_tcpdump() {
-	set -e
-	[[ ! -z  ${tcpdump_stdout} ]] && rm -f ${tcpdump_stdout}
-	[[ ! -z  ${tcpdump_stderr} ]] && rm -f ${tcpdump_stderr}
-	tcpdump_stdout=
-	tcpdump_stderr=
-	set +e
-}
-
-start_tcpdump() {
-	set -e
-	tcpdump_stdout=`mktemp`
-	tcpdump_stderr=`mktemp`
-	ip netns exec ${ROUTER_NS_V6} timeout 15s \
-		tcpdump --immediate-mode -tpni ${ROUTER_INTF} -c 1 \
-		"icmp6 && icmp6[0] == 136 && src ${HOST_ADDR_V6}" \
-		> ${tcpdump_stdout} 2> /dev/null
-	set +e
-}
-
 verify_ndisc() {
 	local accept_untracked_na=$1
 	local same_subnet=$2
@@ -222,8 +203,9 @@  ndisc_test_untracked_advertisements() {
 			HOST_ADDR_V6=2001:db8:abcd:0012::3
 		fi
 	fi
-	setup_v6 $1 $2
-	start_tcpdump
+	setup_v6 $1
+	slowwait_for_counter 15 1 \
+		tc_rule_handle_stats_get "dev ${ROUTER_INTF} ingress" 101 ".packets" "-n ${ROUTER_NS_V6}"
 
 	if verify_ndisc $1 $2; then
 		printf "    TEST: %-60s  [ OK ]\n" "${test_msg[*]}"
@@ -231,7 +213,6 @@  ndisc_test_untracked_advertisements() {
 		printf "    TEST: %-60s  [FAIL]\n" "${test_msg[*]}"
 	fi
 
-	cleanup_tcpdump
 	cleanup_v6
 	set +e
 }
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 112c85c35092..eabbdf00d8ca 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -129,14 +129,6 @@  fi
 
 source "$net_forwarding_dir/../lib.sh"
 
-# timeout in seconds
-slowwait()
-{
-	local timeout_sec=$1; shift
-
-	loopy_wait "sleep 0.1" "$((timeout_sec * 1000))" "$@"
-}
-
 ##############################################################################
 # Sanity checks
 
@@ -678,33 +670,6 @@  wait_for_trap()
 	"$@" | grep -q trap
 }
 
-until_counter_is()
-{
-	local expr=$1; shift
-	local current=$("$@")
-
-	echo $((current))
-	((current $expr))
-}
-
-busywait_for_counter()
-{
-	local timeout=$1; shift
-	local delta=$1; shift
-
-	local base=$("$@")
-	busywait "$timeout" until_counter_is ">= $((base + delta))" "$@"
-}
-
-slowwait_for_counter()
-{
-	local timeout=$1; shift
-	local delta=$1; shift
-
-	local base=$("$@")
-	slowwait "$timeout" until_counter_is ">= $((base + delta))" "$@"
-}
-
 setup_wait_dev()
 {
 	local dev=$1; shift
@@ -1023,29 +988,6 @@  link_stats_rx_errors_get()
 	link_stats_get $1 rx errors
 }
 
-tc_rule_stats_get()
-{
-	local dev=$1; shift
-	local pref=$1; shift
-	local dir=$1; shift
-	local selector=${1:-.packets}; shift
-
-	tc -j -s filter show dev $dev ${dir:-ingress} pref $pref \
-	    | jq ".[1].options.actions[].stats$selector"
-}
-
-tc_rule_handle_stats_get()
-{
-	local id=$1; shift
-	local handle=$1; shift
-	local selector=${1:-.packets}; shift
-	local netns=${1:-""}; shift
-
-	tc $netns -j -s filter show $id \
-	    | jq ".[] | select(.options.handle == $handle) | \
-		  .options.actions[0].stats$selector"
-}
-
 ethtool_stats_get()
 {
 	local dev=$1; shift
diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index 72b191e4e064..edc030e81a46 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -91,6 +91,41 @@  busywait()
 	loopy_wait : "$timeout_ms" "$@"
 }
 
+# timeout in seconds
+slowwait()
+{
+	local timeout_sec=$1; shift
+
+	loopy_wait "sleep 0.1" "$((timeout_sec * 1000))" "$@"
+}
+
+until_counter_is()
+{
+	local expr=$1; shift
+	local current=$("$@")
+
+	echo $((current))
+	((current $expr))
+}
+
+busywait_for_counter()
+{
+	local timeout=$1; shift
+	local delta=$1; shift
+
+	local base=$("$@")
+	busywait "$timeout" until_counter_is ">= $((base + delta))" "$@"
+}
+
+slowwait_for_counter()
+{
+	local timeout=$1; shift
+	local delta=$1; shift
+
+	local base=$("$@")
+	slowwait "$timeout" until_counter_is ">= $((base + delta))" "$@"
+}
+
 cleanup_ns()
 {
 	local ns=""
@@ -150,3 +185,26 @@  setup_ns()
 	done
 	NS_LIST="$NS_LIST $ns_list"
 }
+
+tc_rule_stats_get()
+{
+	local dev=$1; shift
+	local pref=$1; shift
+	local dir=$1; shift
+	local selector=${1:-.packets}; shift
+
+	tc -j -s filter show dev $dev ${dir:-ingress} pref $pref \
+	    | jq ".[1].options.actions[].stats$selector"
+}
+
+tc_rule_handle_stats_get()
+{
+	local id=$1; shift
+	local handle=$1; shift
+	local selector=${1:-.packets}; shift
+	local netns=${1:-""}; shift
+
+	tc $netns -j -s filter show $id \
+	    | jq ".[] | select(.options.handle == $handle) | \
+		  .options.actions[0].stats$selector"
+}