[PATCH mptcp-next] selftests: mptcp: add mp_fail testcases

Geliang Tang posted 1 patch 1 month, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/98ce131530c3ae91bf2ca29e6566ca1f7debe202.1635330390.git.geliang.tang@suse.com
Maintainers: Shuah Khan <shuah@kernel.org>, Mat Martineau <mathew.j.martineau@linux.intel.com>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Matthieu Baerts <matthieu.baerts@tessares.net>
tools/testing/selftests/net/mptcp/config      |  8 +++
.../testing/selftests/net/mptcp/mptcp_join.sh | 50 ++++++++++++++++---
2 files changed, 51 insertions(+), 7 deletions(-)

[PATCH mptcp-next] selftests: mptcp: add mp_fail testcases

Posted by Geliang Tang 1 month, 1 week ago
Added the test cases for MP_FAIL, use 'tc' command to trigger the
checksum failure.

Suggested-by: Davide Caratti <dcaratti@redhat.com>
Suggested-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/net/mptcp/config      |  8 +++
 .../testing/selftests/net/mptcp/mptcp_join.sh | 50 ++++++++++++++++---
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
index 0faaccd21447..dc7d5d1d5861 100644
--- a/tools/testing/selftests/net/mptcp/config
+++ b/tools/testing/selftests/net/mptcp/config
@@ -15,3 +15,11 @@ CONFIG_NETFILTER_XTABLES=m
 CONFIG_NETFILTER_XT_MATCH_BPF=m
 CONFIG_NF_TABLES_IPV4=y
 CONFIG_NF_TABLES_IPV6=y
+CONFIG_GACT_PROB=y
+CONFIG_NET_CLS_FLOWER=m
+CONFIG_NET_CLS_ACT=m
+CONFIG_NET_SCH_INGRESS=m
+CONFIG_NET_SCH_HTB=m
+CONFIG_NET_ACT_CSUM=m
+CONFIG_NET_ACT_GACT=m
+CONFIG_NET_ACT_PEDIT=m
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 7ef639a9d4a6..8bf38db1d54c 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -232,6 +232,20 @@ link_failure()
 	done
 }
 
+checksum_failure()
+{
+	tc -n $ns2 qdisc add dev ns2eth2 clsact
+	tc -n $ns2 filter add dev ns2eth2 egress \
+		protocol ip prio 1000 \
+		flower ip_proto tcp \
+		action pedit munge offset 148 u32 invert \
+		pipe csum tcp
+
+	sleep 1
+
+	tc -n $ns2 qdisc del dev ns2eth2 clsact
+}
+
 # $1: IP address
 is_v6()
 {
@@ -419,6 +433,8 @@ do_transfer()
 			fi
 			sleep 1
 			ip netns exec ${connector_ns} ./pm_nl_ctl del 0 $addr
+		elif [ $rm_nr_ns2 -eq 10 ]; then
+			checksum_failure
 		fi
 	fi
 
@@ -542,6 +558,8 @@ run_tests()
 chk_csum_nr()
 {
 	local msg=${1:-""}
+	local csum_1=${2:-0}
+	local csum_2=${3:-0}
 	local count
 	local dump_stats
 
@@ -553,8 +571,8 @@ chk_csum_nr()
 	printf " %-36s %s" "$msg" "sum"
 	count=`ip netns exec $ns1 nstat -as | grep MPTcpExtDataCsumErr | awk '{print $2}'`
 	[ -z "$count" ] && count=0
-	if [ "$count" != 0 ]; then
-		echo "[fail] got $count data checksum error[s] expected 0"
+	if [ "$count" != $csum_1 ]; then
+		echo "[fail] got $count data checksum error[s] expected $csum_1"
 		ret=1
 		dump_stats=1
 	else
@@ -563,8 +581,8 @@ chk_csum_nr()
 	echo -n " - csum  "
 	count=`ip netns exec $ns2 nstat -as | grep MPTcpExtDataCsumErr | awk '{print $2}'`
 	[ -z "$count" ] && count=0
-	if [ "$count" != 0 ]; then
-		echo "[fail] got $count data checksum error[s] expected 0"
+	if [ "$count" != $csum_2 ]; then
+		echo "[fail] got $count data checksum error[s] expected $csum_2"
 		ret=1
 		dump_stats=1
 	else
@@ -621,6 +639,7 @@ chk_join_nr()
 	local syn_nr=$2
 	local syn_ack_nr=$3
 	local ack_nr=$4
+	local fail_nr=${5:-0}
 	local count
 	local dump_stats
 
@@ -663,8 +682,8 @@ chk_join_nr()
 		ip netns exec $ns2 nstat -as | grep MPTcp
 	fi
 	if [ $checksum -eq 1 ]; then
-		chk_csum_nr
-		chk_fail_nr 0 0
+		chk_csum_nr "" $fail_nr
+		chk_fail_nr $fail_nr $fail_nr
 	fi
 }
 
@@ -1799,6 +1818,18 @@ fullmesh_tests()
 	chk_add_nr 1 1
 }
 
+fail_tests()
+{
+       # multiple subflows
+       reset
+       ip netns exec $ns1 ./pm_nl_ctl limits 0 2
+       ip netns exec $ns2 ./pm_nl_ctl limits 0 2
+       ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 dev ns2eth3 flags subflow
+       ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 dev ns2eth2 flags subflow
+       run_tests $ns1 $ns2 10.0.1.1 2 0 -10 fast
+       chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
+}
+
 all_tests()
 {
 	subflows_tests
@@ -1815,6 +1846,7 @@ all_tests()
 	checksum_tests
 	deny_join_id0_tests
 	fullmesh_tests
+	fail_tests
 }
 
 usage()
@@ -1834,6 +1866,7 @@ usage()
 	echo "  -S checksum_tests"
 	echo "  -d deny_join_id0_tests"
 	echo "  -m fullmesh_tests"
+	echo "  -F fail_tests"
 	echo "  -c capture pcap files"
 	echo "  -C enable data checksum"
 	echo "  -h help"
@@ -1869,7 +1902,7 @@ if [ $do_all_tests -eq 1 ]; then
 	exit $ret
 fi
 
-while getopts 'fsltra64bpkdmchCS' opt; do
+while getopts 'fsltra64bpkdmchCSF' opt; do
 	case $opt in
 		f)
 			subflows_tests
@@ -1913,6 +1946,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
 		m)
 			fullmesh_tests
 			;;
+		F)
+			fail_tests
+			;;
 		c)
 			;;
 		C)
-- 
2.26.2


Re: [PATCH mptcp-next] selftests: mptcp: add mp_fail testcases

Posted by Matthieu Baerts 1 month, 1 week ago
Hi Geliang,

On 27/10/2021 12:26, Geliang Tang wrote:
> Added the test cases for MP_FAIL, use 'tc' command to trigger the
> checksum failure.

Thank you for looking at that!

> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> index 0faaccd21447..dc7d5d1d5861 100644
> --- a/tools/testing/selftests/net/mptcp/config
> +++ b/tools/testing/selftests/net/mptcp/config
> @@ -15,3 +15,11 @@ CONFIG_NETFILTER_XTABLES=m
>  CONFIG_NETFILTER_XT_MATCH_BPF=m
>  CONFIG_NF_TABLES_IPV4=y
>  CONFIG_NF_TABLES_IPV6=y
> +CONFIG_GACT_PROB=y
> +CONFIG_NET_CLS_FLOWER=m
> +CONFIG_NET_CLS_ACT=m
> +CONFIG_NET_SCH_INGRESS=m
> +CONFIG_NET_SCH_HTB=m

I guess we no longer need HTB with the TC commands you are using.

> +CONFIG_NET_ACT_CSUM=m
> +CONFIG_NET_ACT_GACT=m

I don't think we need gact anymore if you don't need to filter some
packets, e.g; one out of 10.
Same for CONFIG_GACT_PROB.

> +CONFIG_NET_ACT_PEDIT=m> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 7ef639a9d4a6..8bf38db1d54c 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -232,6 +232,20 @@ link_failure()
>  	done
>  }
>  
> +checksum_failure()
> +{
> +	tc -n $ns2 qdisc add dev ns2eth2 clsact
> +	tc -n $ns2 filter add dev ns2eth2 egress \
> +		protocol ip prio 1000 \
> +		flower ip_proto tcp \
> +		action pedit munge offset 148 u32 invert \
> +		pipe csum tcp
> +
> +	sleep 1

This looks dangerous to me. We should probably check tc counters to see
if the pedit action did something, no?

Maybe on slow environments, one second will not be enough (or way too
much). Maybe iterating every .1 second on the counters to see if
something happened might help.

> +
> +	tc -n $ns2 qdisc del dev ns2eth2 clsact
> +}
> +
>  # $1: IP address
>  is_v6()
>  {
> @@ -419,6 +433,8 @@ do_transfer()
>  			fi
>  			sleep 1
>  			ip netns exec ${connector_ns} ./pm_nl_ctl del 0 $addr
> +		elif [ $rm_nr_ns2 -eq 10 ]; then

I'm really not a big fan of giving a number representing something but I
see we are already doing that before. Also when you use it later:

  run_tests $ns1 $ns2 10.0.1.1 2 0 -10 fast

With the IP and "fast" it is clear but not the rest. Using something
like what we do with the fullmesh (fullmesh_1: 'fullmesh' mode with one
add_addr) seems clearer to me than giving "-10" or replacing "-10" by a
variable (e.g. ${CORRUPT_PACKETS}) but OK → we should do more
modifications around that I guess.
> +fail_tests()
> +{
> +       # multiple subflows
> +       reset
> +       ip netns exec $ns1 ./pm_nl_ctl limits 0 2
> +       ip netns exec $ns2 ./pm_nl_ctl limits 0 2
> +       ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 dev ns2eth3 flags subflow
> +       ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 dev ns2eth2 flags subflow
> +       run_tests $ns1 $ns2 10.0.1.1 2 0 -10 fast
> +       chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
> +}

It looks like you are using spaces instead of tabs here.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] selftests: mptcp: add mp_fail testcases

Posted by Geliang Tang 1 month, 1 week ago
Hi Matt,

Thanks for your review.

On Wed, Oct 27, 2021 at 01:07:20PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 27/10/2021 12:26, Geliang Tang wrote:
> > Added the test cases for MP_FAIL, use 'tc' command to trigger the
> > checksum failure.
> 
> Thank you for looking at that!
> 
> > diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> > index 0faaccd21447..dc7d5d1d5861 100644
> > --- a/tools/testing/selftests/net/mptcp/config
> > +++ b/tools/testing/selftests/net/mptcp/config
> > @@ -15,3 +15,11 @@ CONFIG_NETFILTER_XTABLES=m
> >  CONFIG_NETFILTER_XT_MATCH_BPF=m
> >  CONFIG_NF_TABLES_IPV4=y
> >  CONFIG_NF_TABLES_IPV6=y
> > +CONFIG_GACT_PROB=y
> > +CONFIG_NET_CLS_FLOWER=m
> > +CONFIG_NET_CLS_ACT=m
> > +CONFIG_NET_SCH_INGRESS=m
> > +CONFIG_NET_SCH_HTB=m
> 
> I guess we no longer need HTB with the TC commands you are using.
> 
> > +CONFIG_NET_ACT_CSUM=m
> > +CONFIG_NET_ACT_GACT=m
> 
> I don't think we need gact anymore if you don't need to filter some
> packets, e.g; one out of 10.
> Same for CONFIG_GACT_PROB.

I'll update this in v2.

> 
> > +CONFIG_NET_ACT_PEDIT=m> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index 7ef639a9d4a6..8bf38db1d54c 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -232,6 +232,20 @@ link_failure()
> >  	done
> >  }
> >  
> > +checksum_failure()
> > +{
> > +	tc -n $ns2 qdisc add dev ns2eth2 clsact
> > +	tc -n $ns2 filter add dev ns2eth2 egress \
> > +		protocol ip prio 1000 \
> > +		flower ip_proto tcp \
> > +		action pedit munge offset 148 u32 invert \
> > +		pipe csum tcp
> > +
> > +	sleep 1
> 
> This looks dangerous to me. We should probably check tc counters to see
> if the pedit action did something, no?

I don't know how to use tc counters, could you please tell me?

> 
> Maybe on slow environments, one second will not be enough (or way too
> much). Maybe iterating every .1 second on the counters to see if
> something happened might help.
> 
> > +
> > +	tc -n $ns2 qdisc del dev ns2eth2 clsact
> > +}
> > +
> >  # $1: IP address
> >  is_v6()
> >  {
> > @@ -419,6 +433,8 @@ do_transfer()
> >  			fi
> >  			sleep 1
> >  			ip netns exec ${connector_ns} ./pm_nl_ctl del 0 $addr
> > +		elif [ $rm_nr_ns2 -eq 10 ]; then
> 
> I'm really not a big fan of giving a number representing something but I
> see we are already doing that before. Also when you use it later:
> 
>   run_tests $ns1 $ns2 10.0.1.1 2 0 -10 fast
> 
> With the IP and "fast" it is clear but not the rest. Using something
> like what we do with the fullmesh (fullmesh_1: 'fullmesh' mode with one
> add_addr) seems clearer to me than giving "-10" or replacing "-10" by a
> variable (e.g. ${CORRUPT_PACKETS}) but OK → we should do more
> modifications around that I guess.

Update in v2.

> > +fail_tests()
> > +{
> > +       # multiple subflows
> > +       reset
> > +       ip netns exec $ns1 ./pm_nl_ctl limits 0 2
> > +       ip netns exec $ns2 ./pm_nl_ctl limits 0 2
> > +       ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 dev ns2eth3 flags subflow
> > +       ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 dev ns2eth2 flags subflow
> > +       run_tests $ns1 $ns2 10.0.1.1 2 0 -10 fast
> > +       chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
> > +}
> 
> It looks like you are using spaces instead of tabs here.

Update in v2.

Thanks,
-Geliang

> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 


Re: [PATCH mptcp-next] selftests: mptcp: add mp_fail testcases

Posted by Matthieu Baerts 1 month, 1 week ago
Hi Geliang,

On 27/10/2021 15:08, Geliang Tang wrote:
> On Wed, Oct 27, 2021 at 01:07:20PM +0200, Matthieu Baerts wrote:
>> On 27/10/2021 12:26, Geliang Tang wrote:
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> index 7ef639a9d4a6..8bf38db1d54c 100755
>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> @@ -232,6 +232,20 @@ link_failure()
>>>  	done
>>>  }
>>>  
>>> +checksum_failure()
>>> +{
>>> +	tc -n $ns2 qdisc add dev ns2eth2 clsact
>>> +	tc -n $ns2 filter add dev ns2eth2 egress \
>>> +		protocol ip prio 1000 \
>>> +		flower ip_proto tcp \
>>> +		action pedit munge offset 148 u32 invert \
>>> +		pipe csum tcp
>>> +
>>> +	sleep 1
>>
>> This looks dangerous to me. We should probably check tc counters to see
>> if the pedit action did something, no?
> 
> I don't know how to use tc counters, could you please tell me?

Davide suggested to use something like:

  tc -n $ns2 -s action show action pedit

But I didn't try.

Thank you for having looked at the other points!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net