diff mbox series

[mptcp-net,v2,26/37] selftests: mptcp: join: skip test if iptables/tc cmds fail

Message ID 20230406-mptcp-issue-368-selftests-old-kernels-v2-26-50313e4f83ab@tessares.net (mailing list archive)
State Superseded, archived
Delegated to: Paolo Abeni
Headers show
Series selftests: mptcp: skip tests when features are not supported | expand

Checks

Context Check Description
matttbe/build success Build and static analysis OK
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 155 lines checked
matttbe/KVM_Validation__normal__except_selftest_mptcp_join_ warning Unstable: 1 failed test(s): packetdrill_fastopen
matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ warning Unstable: 2 failed test(s): packetdrill_fastopen selftest_diag
matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ success Success! ✅
matttbe/KVM_Validation__normal__only_selftest_mptcp_join_ success Success! ✅

Commit Message

Matthieu Baerts May 22, 2023, 4:37 p.m. UTC 
Selftests are supposed to run on any kernels, including the old ones not
supporting all MPTCP features.

Some tests are using IPTables and/or TC commands to force some
behaviours. If one of these commands fails -- likely because some
features are not available -- we should intercept the error and skip the
tests requiring these features.

Note that if we expect to have these features available and if
SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES env var is set to 1, the tests
will be marked as failed instead of skipped.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/368
Fixes: 8d014eaa9254 ("selftests: mptcp: add ADD_ADDR timeout test case")
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 88 ++++++++++++++++---------
 1 file changed, 57 insertions(+), 31 deletions(-)

Comments

Paolo Abeni May 22, 2023, 5:42 p.m. UTC | #1 
On Mon, 2023-05-22 at 18:37 +0200, Matthieu Baerts wrote:
> Selftests are supposed to run on any kernels, including the old ones not
> supporting all MPTCP features.
> 
> Some tests are using IPTables and/or TC commands to force some
> behaviours. If one of these commands fails -- likely because some
> features are not available -- we should intercept the error and skip the
> tests requiring these features.
> 
> Note that if we expect to have these features available and if
> SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES env var is set to 1, the tests
> will be marked as failed instead of skipped.
> 
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/368
> Fixes: 8d014eaa9254 ("selftests: mptcp: add ADD_ADDR timeout test case")
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 88 ++++++++++++++++---------
>  1 file changed, 57 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 73c513f1e808..2ae6d99f3eb9 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -288,11 +288,15 @@ reset_with_add_addr_timeout()
>  	fi
>  
>  	ip netns exec $ns1 sysctl -q net.mptcp.add_addr_timeout=1
> -	ip netns exec $ns2 $tables -A OUTPUT -p tcp \
> -		-m tcp --tcp-option 30 \
> -		-m bpf --bytecode \
> -		"$CBPF_MPTCP_SUBOPTION_ADD_ADDR" \
> -		-j DROP
> +
> +	if ! ip netns exec $ns2 $tables -A OUTPUT -p tcp \
> +			-m tcp --tcp-option 30 \
> +			-m bpf --bytecode \
> +			"$CBPF_MPTCP_SUBOPTION_ADD_ADDR" \
> +			-j DROP; then
> +		mark_as_skipped "unable to set the 'add addr' rule"
> +		return 1
> +	fi
>  }
>  
>  # $1: test name
> @@ -336,17 +340,12 @@ reset_with_allow_join_id0()
>  #     tc action pedit offset 162 out of bounds
>  #
>  # Netfilter is used to mark packets with enough data.
> -reset_with_fail()
> +setup_fail_rules()
>  {
> -	reset "${1}" || return 1
> -
> -	ip netns exec $ns1 sysctl -q net.mptcp.checksum_enabled=1
> -	ip netns exec $ns2 sysctl -q net.mptcp.checksum_enabled=1
> -
>  	check_invert=1
>  	validate_checksum=1
> -	local i="$2"
> -	local ip="${3:-4}"
> +	local i="$1"
> +	local ip="${2:-4}"
>  	local tables
>  
>  	tables="${iptables}"
> @@ -361,15 +360,32 @@ reset_with_fail()
>  		-p tcp \
>  		-m length --length 150:9999 \
>  		-m statistic --mode nth --packet 1 --every 99999 \
> -		-j MARK --set-mark 42 || exit 1
> +		-j MARK --set-mark 42 || return ${ksft_skip}
>  
> -	tc -n $ns2 qdisc add dev ns2eth$i clsact || exit 1
> +	tc -n $ns2 qdisc add dev ns2eth$i clsact || return ${ksft_skip}
>  	tc -n $ns2 filter add dev ns2eth$i egress \
>  		protocol ip prio 1000 \
>  		handle 42 fw \
>  		action pedit munge offset 148 u8 invert \
>  		pipe csum tcp \
> -		index 100 || exit 1
> +		index 100 || return ${ksft_skip}
> +}
> +
> +reset_with_fail()
> +{
> +	reset "${1}" || return 1
> +	shift
> +
> +	ip netns exec $ns1 sysctl -q net.mptcp.checksum_enabled=1
> +	ip netns exec $ns2 sysctl -q net.mptcp.checksum_enabled=1
> +
> +	local rc=0
> +	setup_fail_rules "${@}" || rc=$?
> +
> +	if [ ${rc} -eq ${ksft_skip} ]; then
> +		mark_as_skipped "unable to set the 'fail' rules"
> +		return 1
> +	fi
>  }
>  
>  reset_with_events()
> @@ -384,6 +400,25 @@ reset_with_events()
>  	evts_ns2_pid=$!
>  }
>  
> +reset_with_tcp_filter()
> +{
> +	reset "${1}" || return 1
> +	shift
> +
> +	local ns="${!1}"
> +	local src="${2}"
> +	local target="${3}"
> +
> +	if ! ip netns exec "${ns}" ${iptables} \
> +			-A INPUT \
> +			-s "${src}" \
> +			-p tcp \
> +			-j "${target}"; then
> +		mark_as_skipped "unable to set the filter rules"
> +		return 1
> +	fi
> +}

All the above tc/nf features look quite old. I thought even older them
mptcp itself. Do we really need to check for them?!?

/P
Matthieu Baerts May 23, 2023, 10:10 a.m. UTC | #2 
Hi Paolo,

On 22/05/2023 19:42, Paolo Abeni wrote:
> On Mon, 2023-05-22 at 18:37 +0200, Matthieu Baerts wrote:
>> Selftests are supposed to run on any kernels, including the old ones not
>> supporting all MPTCP features.
>>
>> Some tests are using IPTables and/or TC commands to force some
>> behaviours. If one of these commands fails -- likely because some
>> features are not available -- we should intercept the error and skip the
>> tests requiring these features.

(...)

> All the above tc/nf features look quite old. I thought even older them
> mptcp itself. Do we really need to check for them?!?

It should indeed not be required but when doing the validation on older
kernels, I had issues with 'iptables-nft'. We could also have similar
issues if a kconfig is missing or if there is something wrong with the
Netfilter stack.

The consequence is that either the test was finishing in the middle
(because of the "exit 1" next to some commands) or the errors were
ignored and it was not clear why.

What is mainly interesting here is that we check we can add the specific
rules before starting a test and we stop only the subtest in case of
issue. If it doesn't hurt, maybe safer to keep this?

Cheers,
Matt
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 73c513f1e808..2ae6d99f3eb9 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -288,11 +288,15 @@  reset_with_add_addr_timeout()
 	fi
 
 	ip netns exec $ns1 sysctl -q net.mptcp.add_addr_timeout=1
-	ip netns exec $ns2 $tables -A OUTPUT -p tcp \
-		-m tcp --tcp-option 30 \
-		-m bpf --bytecode \
-		"$CBPF_MPTCP_SUBOPTION_ADD_ADDR" \
-		-j DROP
+
+	if ! ip netns exec $ns2 $tables -A OUTPUT -p tcp \
+			-m tcp --tcp-option 30 \
+			-m bpf --bytecode \
+			"$CBPF_MPTCP_SUBOPTION_ADD_ADDR" \
+			-j DROP; then
+		mark_as_skipped "unable to set the 'add addr' rule"
+		return 1
+	fi
 }
 
 # $1: test name
@@ -336,17 +340,12 @@  reset_with_allow_join_id0()
 #     tc action pedit offset 162 out of bounds
 #
 # Netfilter is used to mark packets with enough data.
-reset_with_fail()
+setup_fail_rules()
 {
-	reset "${1}" || return 1
-
-	ip netns exec $ns1 sysctl -q net.mptcp.checksum_enabled=1
-	ip netns exec $ns2 sysctl -q net.mptcp.checksum_enabled=1
-
 	check_invert=1
 	validate_checksum=1
-	local i="$2"
-	local ip="${3:-4}"
+	local i="$1"
+	local ip="${2:-4}"
 	local tables
 
 	tables="${iptables}"
@@ -361,15 +360,32 @@  reset_with_fail()
 		-p tcp \
 		-m length --length 150:9999 \
 		-m statistic --mode nth --packet 1 --every 99999 \
-		-j MARK --set-mark 42 || exit 1
+		-j MARK --set-mark 42 || return ${ksft_skip}
 
-	tc -n $ns2 qdisc add dev ns2eth$i clsact || exit 1
+	tc -n $ns2 qdisc add dev ns2eth$i clsact || return ${ksft_skip}
 	tc -n $ns2 filter add dev ns2eth$i egress \
 		protocol ip prio 1000 \
 		handle 42 fw \
 		action pedit munge offset 148 u8 invert \
 		pipe csum tcp \
-		index 100 || exit 1
+		index 100 || return ${ksft_skip}
+}
+
+reset_with_fail()
+{
+	reset "${1}" || return 1
+	shift
+
+	ip netns exec $ns1 sysctl -q net.mptcp.checksum_enabled=1
+	ip netns exec $ns2 sysctl -q net.mptcp.checksum_enabled=1
+
+	local rc=0
+	setup_fail_rules "${@}" || rc=$?
+
+	if [ ${rc} -eq ${ksft_skip} ]; then
+		mark_as_skipped "unable to set the 'fail' rules"
+		return 1
+	fi
 }
 
 reset_with_events()
@@ -384,6 +400,25 @@  reset_with_events()
 	evts_ns2_pid=$!
 }
 
+reset_with_tcp_filter()
+{
+	reset "${1}" || return 1
+	shift
+
+	local ns="${!1}"
+	local src="${2}"
+	local target="${3}"
+
+	if ! ip netns exec "${ns}" ${iptables} \
+			-A INPUT \
+			-s "${src}" \
+			-p tcp \
+			-j "${target}"; then
+		mark_as_skipped "unable to set the filter rules"
+		return 1
+	fi
+}
+
 fail_test()
 {
 	ret=1
@@ -750,15 +785,6 @@  pm_nl_check_endpoint()
 	fi
 }
 
-filter_tcp_from()
-{
-	local ns="${1}"
-	local src="${2}"
-	local target="${3}"
-
-	ip netns exec "${ns}" ${iptables} -A INPUT -s "${src}" -p tcp -j "${target}"
-}
-
 do_transfer()
 {
 	local listener_ns="$1"
@@ -2029,23 +2055,23 @@  subflows_error_tests()
 	fi
 
 	# multiple subflows, with subflow creation error
-	if reset "multi subflows, with failing subflow"; then
+	if reset_with_tcp_filter "multi subflows, with failing subflow" ns1 10.0.3.2 REJECT &&
+	   continue_if mptcp_lib_kallsyms_has "mptcp_pm_subflow_check_next$"; then
 		pm_nl_set_limits $ns1 0 2
 		pm_nl_set_limits $ns2 0 2
 		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
 		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
-		filter_tcp_from $ns1 10.0.3.2 REJECT
 		run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow
 		chk_join_nr 1 1 1
 	fi
 
 	# multiple subflows, with subflow timeout on MPJ
-	if reset "multi subflows, with subflow timeout"; then
+	if reset_with_tcp_filter "multi subflows, with subflow timeout" ns1 10.0.3.2 DROP &&
+	   continue_if mptcp_lib_kallsyms_has "mptcp_pm_subflow_check_next$"; then
 		pm_nl_set_limits $ns1 0 2
 		pm_nl_set_limits $ns2 0 2
 		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
 		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
-		filter_tcp_from $ns1 10.0.3.2 DROP
 		run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow
 		chk_join_nr 1 1 1
 	fi
@@ -2053,11 +2079,11 @@  subflows_error_tests()
 	# multiple subflows, check that the endpoint corresponding to
 	# closed subflow (due to reset) is not reused if additional
 	# subflows are added later
-	if reset "multi subflows, fair usage on close"; then
+	if reset_with_tcp_filter "multi subflows, fair usage on close" ns1 10.0.3.2 REJECT &&
+	   continue_if mptcp_lib_kallsyms_has "mptcp_pm_subflow_check_next$"; then
 		pm_nl_set_limits $ns1 0 1
 		pm_nl_set_limits $ns2 0 1
 		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
-		filter_tcp_from $ns1 10.0.3.2 REJECT
 		run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow &
 
 		# mpj subflow will be in TW after the reset