1. Patchew
  2. MPTCP
  3. View series

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

Geliang Tang posted 1 patch 2 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/bfa01f99cbb0479029ad284b020dbb250d19b941.1635390811.git.geliang.tang@suse.com
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, "David S. Miller" <davem@davemloft.net>, Mat Martineau <mathew.j.martineau@linux.intel.com>, Shuah Khan <shuah@kernel.org>, Jakub Kicinski <kuba@kernel.org>
There is a newer version of this series
tools/testing/selftests/net/mptcp/config      |  5 ++
.../testing/selftests/net/mptcp/mptcp_join.sh | 60 ++++++++++++++++---
2 files changed, 58 insertions(+), 7 deletions(-)
[PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases
Posted by Geliang Tang 2 years, 6 months 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      |  5 ++
 .../testing/selftests/net/mptcp/mptcp_join.sh | 60 ++++++++++++++++---
 2 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
index 0faaccd21447..f522288b2204 100644
--- a/tools/testing/selftests/net/mptcp/config
+++ b/tools/testing/selftests/net/mptcp/config
@@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
 CONFIG_NETFILTER_XT_MATCH_BPF=m
 CONFIG_NF_TABLES_IPV4=y
 CONFIG_NF_TABLES_IPV6=y
+CONFIG_NET_ACT_CSUM=m
+CONFIG_NET_ACT_PEDIT=m
+CONFIG_NET_CLS_ACT=m
+CONFIG_NET_CLS_FLOWER=m
+CONFIG_NET_SCH_INGRESS=m
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 7ef639a9d4a6..95bac447f857 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -232,6 +232,29 @@ link_failure()
 	done
 }
 
+checksum_failure()
+{
+	i="$1"
+
+	tc -n $ns2 qdisc add dev ns2eth$i clsact
+	tc -n $ns2 filter add dev ns2eth$i egress \
+		protocol ip prio 1000 \
+		flower ip_proto tcp \
+		action pedit munge offset 148 u32 invert \
+		pipe csum tcp \
+		index 100
+
+	while true; do
+		local pkt=`tc -n $ns2 -j -s action show action csum index 100 | \
+			awk -F ',' '{ print $12 }' | \
+			awk -F ':' '{ print $2 }'`
+		if [ $pkt -gt 0 ]; then
+			tc -n $ns2 qdisc del dev ns2eth$i clsact
+			break
+		fi
+	done
+}
+
 # $1: IP address
 is_v6()
 {
@@ -371,6 +394,9 @@ do_transfer()
 	if [[ "${addr_nr_ns2}" = "fullmesh_"* ]]; then
 		flags="${flags},fullmesh"
 		addr_nr_ns2=${addr_nr_ns2:9}
+	elif [[ "${addr_nr_ns2}" = "fail_"* ]]; then
+		checksum_failure ${addr_nr_ns2:5}
+		addr_nr_ns2=0
 	fi
 
 	if [ $addr_nr_ns2 -gt 0 ]; then
@@ -542,6 +568,8 @@ run_tests()
 chk_csum_nr()
 {
 	local msg=${1:-""}
+	local csum_ns1=${2:-0}
+	local csum_ns2=${3:-0}
 	local count
 	local dump_stats
 
@@ -553,8 +581,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_ns1 ]; then
+		echo "[fail] got $count data checksum error[s] expected $csum_ns1"
 		ret=1
 		dump_stats=1
 	else
@@ -563,8 +591,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_ns2 ]; then
+		echo "[fail] got $count data checksum error[s] expected $csum_ns2"
 		ret=1
 		dump_stats=1
 	else
@@ -621,6 +649,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 +692,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 +1828,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 "fail_2" fast
+	chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
+}
+
 all_tests()
 {
 	subflows_tests
@@ -1815,6 +1856,7 @@ all_tests()
 	checksum_tests
 	deny_join_id0_tests
 	fullmesh_tests
+	fail_tests
 }
 
 usage()
@@ -1834,6 +1876,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 +1912,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 +1956,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
 		m)
 			fullmesh_tests
 			;;
+		F)
+			fail_tests
+			;;
 		c)
 			;;
 		C)
-- 
2.26.2
Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases
Posted by Geliang Tang 2 years, 6 months ago 
On Thu, Oct 28, 2021 at 11:16:12AM +0800, Geliang Tang wrote:
> 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      |  5 ++
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 60 ++++++++++++++++---
>  2 files changed, 58 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> index 0faaccd21447..f522288b2204 100644
> --- a/tools/testing/selftests/net/mptcp/config
> +++ b/tools/testing/selftests/net/mptcp/config
> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
>  CONFIG_NETFILTER_XT_MATCH_BPF=m
>  CONFIG_NF_TABLES_IPV4=y
>  CONFIG_NF_TABLES_IPV6=y
> +CONFIG_NET_ACT_CSUM=m
> +CONFIG_NET_ACT_PEDIT=m
> +CONFIG_NET_CLS_ACT=m
> +CONFIG_NET_CLS_FLOWER=m
> +CONFIG_NET_SCH_INGRESS=m
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 7ef639a9d4a6..95bac447f857 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -232,6 +232,29 @@ link_failure()
>  	done
>  }
>  
> +checksum_failure()
> +{
> +	i="$1"
> +
> +	tc -n $ns2 qdisc add dev ns2eth$i clsact
> +	tc -n $ns2 filter add dev ns2eth$i egress \
> +		protocol ip prio 1000 \
> +		flower ip_proto tcp \
> +		action pedit munge offset 148 u32 invert \
> +		pipe csum tcp \
> +		index 100
> +
> +	while true; do
> +		local pkt=`tc -n $ns2 -j -s action show action csum index 100 | \
> +			awk -F ',' '{ print $12 }' | \
> +			awk -F ':' '{ print $2 }'`
> +		if [ $pkt -gt 0 ]; then
> +			tc -n $ns2 qdisc del dev ns2eth$i clsact
> +			break
> +		fi
> +	done
> +}

I got this log in the dmesg output:

[ 2495.182631] tc action pedit offset 162 out of bounds

It looks like something wrong with tc.

How can I fix this?

Thanks,
-Geliang

> +
>  # $1: IP address
>  is_v6()
>  {
> @@ -371,6 +394,9 @@ do_transfer()
>  	if [[ "${addr_nr_ns2}" = "fullmesh_"* ]]; then
>  		flags="${flags},fullmesh"
>  		addr_nr_ns2=${addr_nr_ns2:9}
> +	elif [[ "${addr_nr_ns2}" = "fail_"* ]]; then
> +		checksum_failure ${addr_nr_ns2:5}
> +		addr_nr_ns2=0
>  	fi
>  
>  	if [ $addr_nr_ns2 -gt 0 ]; then
> @@ -542,6 +568,8 @@ run_tests()
>  chk_csum_nr()
>  {
>  	local msg=${1:-""}
> +	local csum_ns1=${2:-0}
> +	local csum_ns2=${3:-0}
>  	local count
>  	local dump_stats
>  
> @@ -553,8 +581,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_ns1 ]; then
> +		echo "[fail] got $count data checksum error[s] expected $csum_ns1"
>  		ret=1
>  		dump_stats=1
>  	else
> @@ -563,8 +591,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_ns2 ]; then
> +		echo "[fail] got $count data checksum error[s] expected $csum_ns2"
>  		ret=1
>  		dump_stats=1
>  	else
> @@ -621,6 +649,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 +692,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 +1828,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 "fail_2" fast
> +	chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
> +}
> +
>  all_tests()
>  {
>  	subflows_tests
> @@ -1815,6 +1856,7 @@ all_tests()
>  	checksum_tests
>  	deny_join_id0_tests
>  	fullmesh_tests
> +	fail_tests
>  }
>  
>  usage()
> @@ -1834,6 +1876,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 +1912,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 +1956,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
>  		m)
>  			fullmesh_tests
>  			;;
> +		F)
> +			fail_tests
> +			;;
>  		c)
>  			;;
>  		C)
> -- 
> 2.26.2
>
Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases
Posted by Davide Caratti 2 years, 6 months ago 
On Thu, Oct 28, 2021 at 11:23 AM Geliang Tang <geliang.tang@suse.com> wrote:
>
> On Thu, Oct 28, 2021 at 11:16:12AM +0800, Geliang Tang wrote:
> > 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      |  5 ++
> >  .../testing/selftests/net/mptcp/mptcp_join.sh | 60 ++++++++++++++++---
> >  2 files changed, 58 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> > index 0faaccd21447..f522288b2204 100644
> > --- a/tools/testing/selftests/net/mptcp/config
> > +++ b/tools/testing/selftests/net/mptcp/config
> > @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
> >  CONFIG_NETFILTER_XT_MATCH_BPF=m
> >  CONFIG_NF_TABLES_IPV4=y
> >  CONFIG_NF_TABLES_IPV6=y
> > +CONFIG_NET_ACT_CSUM=m
> > +CONFIG_NET_ACT_PEDIT=m
> > +CONFIG_NET_CLS_ACT=m
> > +CONFIG_NET_CLS_FLOWER=m
> > +CONFIG_NET_SCH_INGRESS=m
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index 7ef639a9d4a6..95bac447f857 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -232,6 +232,29 @@ link_failure()
> >       done
> >  }
> >
> > +checksum_failure()
> > +{
> > +     i="$1"
> > +
> > +     tc -n $ns2 qdisc add dev ns2eth$i clsact
> > +     tc -n $ns2 filter add dev ns2eth$i egress \
> > +             protocol ip prio 1000 \
> > +             flower ip_proto tcp \
> > +             action pedit munge offset 148 u32 invert \
> > +             pipe csum tcp \
> > +             index 100
> > +
> > +     while true; do
> > +             local pkt=`tc -n $ns2 -j -s action show action csum index 100 | \
> > +                     awk -F ',' '{ print $12 }' | \
> > +                     awk -F ':' '{ print $2 }'`
> > +             if [ $pkt -gt 0 ]; then
> > +                     tc -n $ns2 qdisc del dev ns2eth$i clsact
> > +                     break
> > +             fi
> > +     done
> > +}
>
> I got this log in the dmesg output:
>
> [ 2495.182631] tc action pedit offset 162 out of bounds
>
> It looks like something wrong with tc.

this is a packet too small to be edited by pedit:

https://elixir.bootlin.com/linux/latest/source/net/sched/act_pedit.c#L368

when this happens, pedit gives up mangling and passes the packet to
the next action (csum in your case, but it's useless)

> How can I fix this?
is that a problem? maybe we can just ignore it.

Otherwise, a possibility is to add a more narrow filter (e.g. flower),
 to only mangle packets that have the ack flag but not the syn or
push. We might also try corrupting the TCP payload to a smaller
offset, so that more packets are impacted.

--
davide
Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases
Posted by Matthieu Baerts 2 years, 6 months ago 
Hi Davide, Geliang,

On 28/10/2021 13:01, Davide Caratti wrote:
> On Thu, Oct 28, 2021 at 11:23 AM Geliang Tang <geliang.tang@suse.com> wrote:
>>
>> On Thu, Oct 28, 2021 at 11:16:12AM +0800, Geliang Tang wrote:
>>> 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>

Thank you for the new version!

>>> ---
>>>  tools/testing/selftests/net/mptcp/config      |  5 ++
>>>  .../testing/selftests/net/mptcp/mptcp_join.sh | 60 ++++++++++++++++---
>>>  2 files changed, 58 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
>>> index 0faaccd21447..f522288b2204 100644
>>> --- a/tools/testing/selftests/net/mptcp/config
>>> +++ b/tools/testing/selftests/net/mptcp/config
>>> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
>>>  CONFIG_NETFILTER_XT_MATCH_BPF=m
>>>  CONFIG_NF_TABLES_IPV4=y
>>>  CONFIG_NF_TABLES_IPV6=y
>>> +CONFIG_NET_ACT_CSUM=m
>>> +CONFIG_NET_ACT_PEDIT=m
>>> +CONFIG_NET_CLS_ACT=m
>>> +CONFIG_NET_CLS_FLOWER=m
>>> +CONFIG_NET_SCH_INGRESS=m
>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> index 7ef639a9d4a6..95bac447f857 100755
>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> @@ -232,6 +232,29 @@ link_failure()
>>>       done
>>>  }
>>>
>>> +checksum_failure()
>>> +{
>>> +     i="$1"
>>> +
>>> +     tc -n $ns2 qdisc add dev ns2eth$i clsact
>>> +     tc -n $ns2 filter add dev ns2eth$i egress \
>>> +             protocol ip prio 1000 \
>>> +             flower ip_proto tcp \
>>> +             action pedit munge offset 148 u32 invert \
>>> +             pipe csum tcp \
>>> +             index 100
>>> +
>>> +     while true; do
>>> +             local pkt=`tc -n $ns2 -j -s action show action csum index 100 | \
>>> +                     awk -F ',' '{ print $12 }' | \
>>> +                     awk -F ':' '{ print $2 }'`

(small detail if you have to send a new version: it is often recommended
to use "$(cmd)" form instead of `cmd`: more visible, allow nested
commands, quotes, etc.)

Also if a v3 is needed, do not hesitate to add in a comment an example
of the output just in case it changes later and also to better
understand what you are trying to extract :)

>>> +             if [ $pkt -gt 0 ]; then
>>> +                     tc -n $ns2 qdisc del dev ns2eth$i clsact
>>> +                     break
>>> +             fi
>>> +     done
>>> +}
>>
>> I got this log in the dmesg output:
>>
>> [ 2495.182631] tc action pedit offset 162 out of bounds
>>
>> It looks like something wrong with tc.
> 
> this is a packet too small to be edited by pedit:
> 
> https://elixir.bootlin.com/linux/latest/source/net/sched/act_pedit.c#L368
> 
> when this happens, pedit gives up mangling and passes the packet to
> the next action (csum in your case, but it's useless)
> 
>> How can I fix this?
> is that a problem? maybe we can just ignore it.

Do you have this message once or one for each packet?
Does it increment the counter you are monitoring when the warning is
printed?

If I understand the code, it might print that once per packet and once
but also increment the counter which is not good for us. But another
counter -- "overlimits" -- seems to be incremented too, maybe we can use
the two counters.

> Otherwise, a possibility is to add a more narrow filter (e.g. flower),
>  to only mangle packets that have the ack flag but not the syn or
> push. We might also try corrupting the TCP payload to a smaller
> offset, so that more packets are impacted.

Yes, maybe good to narrow a bit the filter by using:

  flower ip_proto tcp tcp_flags 0x10/0xff

Or is there another filter to specify a minimum size?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases
Posted by Davide Caratti 2 years, 5 months ago 
On Thu, Oct 28, 2021 at 1:21 PM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Davide, Geliang,
>
> On 28/10/2021 13:01, Davide Caratti wrote:
> > On Thu, Oct 28, 2021 at 11:23 AM Geliang Tang <geliang.tang@suse.com> wrote:
> >>
> >> On Thu, Oct 28, 2021 at 11:16:12AM +0800, Geliang Tang wrote:
> >>> 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>
>
> Thank you for the new version!
>
> >>> ---
> >>>  tools/testing/selftests/net/mptcp/config      |  5 ++
> >>>  .../testing/selftests/net/mptcp/mptcp_join.sh | 60 ++++++++++++++++---
> >>>  2 files changed, 58 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> >>> index 0faaccd21447..f522288b2204 100644
> >>> --- a/tools/testing/selftests/net/mptcp/config
> >>> +++ b/tools/testing/selftests/net/mptcp/config
> >>> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
> >>>  CONFIG_NETFILTER_XT_MATCH_BPF=m
> >>>  CONFIG_NF_TABLES_IPV4=y
> >>>  CONFIG_NF_TABLES_IPV6=y
> >>> +CONFIG_NET_ACT_CSUM=m
> >>> +CONFIG_NET_ACT_PEDIT=m
> >>> +CONFIG_NET_CLS_ACT=m
> >>> +CONFIG_NET_CLS_FLOWER=m
> >>> +CONFIG_NET_SCH_INGRESS=m
> >>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >>> index 7ef639a9d4a6..95bac447f857 100755
> >>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >>> @@ -232,6 +232,29 @@ link_failure()
> >>>       done
> >>>  }
> >>>
> >>> +checksum_failure()
> >>> +{
> >>> +     i="$1"
> >>> +
> >>> +     tc -n $ns2 qdisc add dev ns2eth$i clsact
> >>> +     tc -n $ns2 filter add dev ns2eth$i egress \
> >>> +             protocol ip prio 1000 \
> >>> +             flower ip_proto tcp \
> >>> +             action pedit munge offset 148 u32 invert \
> >>> +             pipe csum tcp \
> >>> +             index 100
> >>> +
> >>> +     while true; do
> >>> +             local pkt=`tc -n $ns2 -j -s action show action csum index 100 | \
> >>> +                     awk -F ',' '{ print $12 }' | \
> >>> +                     awk -F ':' '{ print $2 }'`
>
> (small detail if you have to send a new version: it is often recommended
> to use "$(cmd)" form instead of `cmd`: more visible, allow nested
> commands, quotes, etc.)

also, please consider using jq rather than awk. Specially if you are
planning to use more than one counter. For reference, see [1]
>

[...]
> >
> >> How can I fix this?
> > is that a problem? maybe we can just ignore it.
>
> Do you have this message once or one for each packet?
> Does it increment the counter you are monitoring when the warning is
> printed?
>
> If I understand the code, it might print that once per packet and once
> but also increment the counter which is not good for us. But another
> counter -- "overlimits" -- seems to be incremented too, maybe we can use
> the two counters.
>
> > Otherwise, a possibility is to add a more narrow filter (e.g. flower),
> >  to only mangle packets that have the ack flag but not the syn or
> > push. We might also try corrupting the TCP payload to a smaller
> > offset, so that more packets are impacted.
>
> Yes, maybe good to narrow a bit the filter by using:
>
>   flower ip_proto tcp tcp_flags 0x10/0xff
>
> Or is there another filter to specify a minimum size?

unfortunately we can't do that with TC filters (unless using the eBPF
classifier for this (and I'm still not 100% the verifier would be
happy _ but that's still in my long-term todo list :) ). I know there
are ongoing efforts to introduce a "check_pkt_larger" (because this
check is actually a possible openflow action) , but that won't happen
in the short period.
-- 
davide
Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases
Posted by Davide Caratti 2 years, 5 months ago 
On Thu, Oct 28, 2021 at 2:24 PM Davide Caratti <dcaratti@redhat.com> wrote:
>

> also, please consider using jq rather than awk. Specially if you are
> planning to use more than one counter. For reference, see [1]

ok, but where's [1]  ;) ? it's below:

[1] https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/net/forwarding/lib.sh#L701
Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases
Posted by Matthieu Baerts 2 years, 5 months ago 
Hi Davide,

On 28/10/2021 14:24, Davide Caratti wrote:
> On Thu, Oct 28, 2021 at 1:21 PM Matthieu Baerts
> <matthieu.baerts@tessares.net> wrote:
>>
>> Hi Davide, Geliang,
>>
>> On 28/10/2021 13:01, Davide Caratti wrote:
>>> On Thu, Oct 28, 2021 at 11:23 AM Geliang Tang <geliang.tang@suse.com> wrote:
>>>>
>>>> On Thu, Oct 28, 2021 at 11:16:12AM +0800, Geliang Tang wrote:
>>>>> 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>
>>
>> Thank you for the new version!
>>
>>>>> ---
>>>>>  tools/testing/selftests/net/mptcp/config      |  5 ++
>>>>>  .../testing/selftests/net/mptcp/mptcp_join.sh | 60 ++++++++++++++++---
>>>>>  2 files changed, 58 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
>>>>> index 0faaccd21447..f522288b2204 100644
>>>>> --- a/tools/testing/selftests/net/mptcp/config
>>>>> +++ b/tools/testing/selftests/net/mptcp/config
>>>>> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
>>>>>  CONFIG_NETFILTER_XT_MATCH_BPF=m
>>>>>  CONFIG_NF_TABLES_IPV4=y
>>>>>  CONFIG_NF_TABLES_IPV6=y
>>>>> +CONFIG_NET_ACT_CSUM=m
>>>>> +CONFIG_NET_ACT_PEDIT=m
>>>>> +CONFIG_NET_CLS_ACT=m
>>>>> +CONFIG_NET_CLS_FLOWER=m
>>>>> +CONFIG_NET_SCH_INGRESS=m
>>>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>>> index 7ef639a9d4a6..95bac447f857 100755
>>>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>>> @@ -232,6 +232,29 @@ link_failure()
>>>>>       done
>>>>>  }
>>>>>
>>>>> +checksum_failure()
>>>>> +{
>>>>> +     i="$1"
>>>>> +
>>>>> +     tc -n $ns2 qdisc add dev ns2eth$i clsact
>>>>> +     tc -n $ns2 filter add dev ns2eth$i egress \
>>>>> +             protocol ip prio 1000 \
>>>>> +             flower ip_proto tcp \
>>>>> +             action pedit munge offset 148 u32 invert \
>>>>> +             pipe csum tcp \
>>>>> +             index 100
>>>>> +
>>>>> +     while true; do
>>>>> +             local pkt=`tc -n $ns2 -j -s action show action csum index 100 | \
>>>>> +                     awk -F ',' '{ print $12 }' | \
>>>>> +                     awk -F ':' '{ print $2 }'`
>>
>> (small detail if you have to send a new version: it is often recommended
>> to use "$(cmd)" form instead of `cmd`: more visible, allow nested
>> commands, quotes, etc.)
> 
> also, please consider using jq rather than awk. Specially if you are
> planning to use more than one counter. For reference, see [1]

Good idea, I didn't know jq was used in other selftests. But then we
would need to check if 'jq' is installed. *Maybe* if we only need once
to retrieve 2 counters, it would be "easier" to use awk?

> [...]
>>>
>>>> How can I fix this?
>>> is that a problem? maybe we can just ignore it.
>>
>> Do you have this message once or one for each packet?
>> Does it increment the counter you are monitoring when the warning is
>> printed?
>>
>> If I understand the code, it might print that once per packet and once
>> but also increment the counter which is not good for us. But another
>> counter -- "overlimits" -- seems to be incremented too, maybe we can use
>> the two counters.
>>
>>> Otherwise, a possibility is to add a more narrow filter (e.g. flower),
>>>  to only mangle packets that have the ack flag but not the syn or
>>> push. We might also try corrupting the TCP payload to a smaller
>>> offset, so that more packets are impacted.
>>
>> Yes, maybe good to narrow a bit the filter by using:
>>
>>   flower ip_proto tcp tcp_flags 0x10/0xff
>>
>> Or is there another filter to specify a minimum size?
> 
> unfortunately we can't do that with TC filters (unless using the eBPF
> classifier for this (and I'm still not 100% the verifier would be
> happy _ but that's still in my long-term todo list :) ). I know there
> are ongoing efforts to introduce a "check_pkt_larger" (because this
> check is actually a possible openflow action) , but that won't happen
> in the short period.

That's a shame but then better to restrict to ACK flag to omit a few
packets.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases
Posted by Matthieu Baerts 2 years, 5 months ago 
Hi Geliang,

On 28/10/2021 15:21, Matthieu Baerts wrote:
> Hi Davide,
> 
> On 28/10/2021 14:24, Davide Caratti wrote:
>> On Thu, Oct 28, 2021 at 1:21 PM Matthieu Baerts
>> <matthieu.baerts@tessares.net> wrote:
>>>
>>> Hi Davide, Geliang,
>>>
>>> On 28/10/2021 13:01, Davide Caratti wrote:
>>>> On Thu, Oct 28, 2021 at 11:23 AM Geliang Tang <geliang.tang@suse.com> wrote:
>>>>>
>>>>> On Thu, Oct 28, 2021 at 11:16:12AM +0800, Geliang Tang wrote:
>>>>>> 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>
>>>
>>> Thank you for the new version!
>>>
>>>>>> ---
>>>>>>  tools/testing/selftests/net/mptcp/config      |  5 ++
>>>>>>  .../testing/selftests/net/mptcp/mptcp_join.sh | 60 ++++++++++++++++---
>>>>>>  2 files changed, 58 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
>>>>>> index 0faaccd21447..f522288b2204 100644
>>>>>> --- a/tools/testing/selftests/net/mptcp/config
>>>>>> +++ b/tools/testing/selftests/net/mptcp/config
>>>>>> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
>>>>>>  CONFIG_NETFILTER_XT_MATCH_BPF=m
>>>>>>  CONFIG_NF_TABLES_IPV4=y
>>>>>>  CONFIG_NF_TABLES_IPV6=y
>>>>>> +CONFIG_NET_ACT_CSUM=m
>>>>>> +CONFIG_NET_ACT_PEDIT=m
>>>>>> +CONFIG_NET_CLS_ACT=m
>>>>>> +CONFIG_NET_CLS_FLOWER=m
>>>>>> +CONFIG_NET_SCH_INGRESS=m
>>>>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>>>> index 7ef639a9d4a6..95bac447f857 100755
>>>>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>>>> @@ -232,6 +232,29 @@ link_failure()
>>>>>>       done
>>>>>>  }
>>>>>>
>>>>>> +checksum_failure()
>>>>>> +{
>>>>>> +     i="$1"
>>>>>> +
>>>>>> +     tc -n $ns2 qdisc add dev ns2eth$i clsact
>>>>>> +     tc -n $ns2 filter add dev ns2eth$i egress \
>>>>>> +             protocol ip prio 1000 \
>>>>>> +             flower ip_proto tcp \
>>>>>> +             action pedit munge offset 148 u32 invert \
>>>>>> +             pipe csum tcp \
>>>>>> +             index 100
>>>>>> +
>>>>>> +     while true; do
>>>>>> +             local pkt=`tc -n $ns2 -j -s action show action csum index 100 | \
>>>>>> +                     awk -F ',' '{ print $12 }' | \
>>>>>> +                     awk -F ':' '{ print $2 }'`
>>>
>>> (small detail if you have to send a new version: it is often recommended
>>> to use "$(cmd)" form instead of `cmd`: more visible, allow nested
>>> commands, quotes, etc.)
>>
>> also, please consider using jq rather than awk. Specially if you are
>> planning to use more than one counter. For reference, see [1]
> 
> Good idea, I didn't know jq was used in other selftests. But then we
> would need to check if 'jq' is installed. *Maybe* if we only need once
> to retrieve 2 counters, it would be "easier" to use awk?
> 
>> [...]
>>>>
>>>>> How can I fix this?
>>>> is that a problem? maybe we can just ignore it.
>>>
>>> Do you have this message once or one for each packet?
>>> Does it increment the counter you are monitoring when the warning is
>>> printed?
>>>
>>> If I understand the code, it might print that once per packet and once
>>> but also increment the counter which is not good for us. But another
>>> counter -- "overlimits" -- seems to be incremented too, maybe we can use
>>> the two counters.
>>>
>>>> Otherwise, a possibility is to add a more narrow filter (e.g. flower),
>>>>  to only mangle packets that have the ack flag but not the syn or
>>>> push. We might also try corrupting the TCP payload to a smaller
>>>> offset, so that more packets are impacted.
>>>
>>> Yes, maybe good to narrow a bit the filter by using:
>>>
>>>   flower ip_proto tcp tcp_flags 0x10/0xff
>>>
>>> Or is there another filter to specify a minimum size?
>>
>> unfortunately we can't do that with TC filters (unless using the eBPF
>> classifier for this (and I'm still not 100% the verifier would be
>> happy _ but that's still in my long-term todo list :) ). I know there
>> are ongoing efforts to introduce a "check_pkt_larger" (because this
>> check is actually a possible openflow action) , but that won't happen
>> in the short period.
> 
> That's a shame but then better to restrict to ACK flag to omit a few
> packets.

As discussed at the last meeting, we should reduce the number of kernel
message (pr_info for each packet not matched by the pedit action). For that:
- best to restrict to TCP packets with ACK flag only:

    flower ip_proto tcp tcp_flags 0x10/0xff

- and send only in one direction to reduce the chance to have "pure ACKs".

Having a few pr_info message is OK but plenty messages in dmesg/serial
will likely cause other issues (flooding logs, slowing down stuff, etc.)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases
Posted by Geliang Tang 2 years, 5 months ago 
Hi Matt, Davide,

V3 of this patch was put into the infinite map series v8.

On Thu, Oct 28, 2021 at 06:46:52PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 28/10/2021 15:21, Matthieu Baerts wrote:
> > Hi Davide,
> > 
> > On 28/10/2021 14:24, Davide Caratti wrote:
> >> On Thu, Oct 28, 2021 at 1:21 PM Matthieu Baerts
> >> <matthieu.baerts@tessares.net> wrote:
> >>>
> >>> Hi Davide, Geliang,
> >>>
> >>> On 28/10/2021 13:01, Davide Caratti wrote:
> >>>> On Thu, Oct 28, 2021 at 11:23 AM Geliang Tang <geliang.tang@suse.com> wrote:
> >>>>>
> >>>>> On Thu, Oct 28, 2021 at 11:16:12AM +0800, Geliang Tang wrote:
> >>>>>> 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>
> >>>
> >>> Thank you for the new version!
> >>>
> >>>>>> ---
> >>>>>>  tools/testing/selftests/net/mptcp/config      |  5 ++
> >>>>>>  .../testing/selftests/net/mptcp/mptcp_join.sh | 60 ++++++++++++++++---
> >>>>>>  2 files changed, 58 insertions(+), 7 deletions(-)
> >>>>>>
> >>>>>> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> >>>>>> index 0faaccd21447..f522288b2204 100644
> >>>>>> --- a/tools/testing/selftests/net/mptcp/config
> >>>>>> +++ b/tools/testing/selftests/net/mptcp/config
> >>>>>> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
> >>>>>>  CONFIG_NETFILTER_XT_MATCH_BPF=m
> >>>>>>  CONFIG_NF_TABLES_IPV4=y
> >>>>>>  CONFIG_NF_TABLES_IPV6=y
> >>>>>> +CONFIG_NET_ACT_CSUM=m
> >>>>>> +CONFIG_NET_ACT_PEDIT=m
> >>>>>> +CONFIG_NET_CLS_ACT=m
> >>>>>> +CONFIG_NET_CLS_FLOWER=m
> >>>>>> +CONFIG_NET_SCH_INGRESS=m
> >>>>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >>>>>> index 7ef639a9d4a6..95bac447f857 100755
> >>>>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >>>>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >>>>>> @@ -232,6 +232,29 @@ link_failure()
> >>>>>>       done
> >>>>>>  }
> >>>>>>
> >>>>>> +checksum_failure()
> >>>>>> +{
> >>>>>> +     i="$1"
> >>>>>> +
> >>>>>> +     tc -n $ns2 qdisc add dev ns2eth$i clsact
> >>>>>> +     tc -n $ns2 filter add dev ns2eth$i egress \
> >>>>>> +             protocol ip prio 1000 \
> >>>>>> +             flower ip_proto tcp \
> >>>>>> +             action pedit munge offset 148 u32 invert \
> >>>>>> +             pipe csum tcp \
> >>>>>> +             index 100
> >>>>>> +
> >>>>>> +     while true; do
> >>>>>> +             local pkt=`tc -n $ns2 -j -s action show action csum index 100 | \
> >>>>>> +                     awk -F ',' '{ print $12 }' | \
> >>>>>> +                     awk -F ':' '{ print $2 }'`
> >>>
> >>> (small detail if you have to send a new version: it is often recommended
> >>> to use "$(cmd)" form instead of `cmd`: more visible, allow nested
> >>> commands, quotes, etc.)

Updated.

> >>
> >> also, please consider using jq rather than awk. Specially if you are
> >> planning to use more than one counter. For reference, see [1]
> > 
> > Good idea, I didn't know jq was used in other selftests. But then we
> > would need to check if 'jq' is installed. *Maybe* if we only need once
> > to retrieve 2 counters, it would be "easier" to use awk?

I used jq in the new version, since awk will get wrong values
sometimes.

> > 
> >> [...]
> >>>>
> >>>>> How can I fix this?
> >>>> is that a problem? maybe we can just ignore it.
> >>>
> >>> Do you have this message once or one for each packet?
> >>> Does it increment the counter you are monitoring when the warning is
> >>> printed?
> >>>
> >>> If I understand the code, it might print that once per packet and once
> >>> but also increment the counter which is not good for us. But another
> >>> counter -- "overlimits" -- seems to be incremented too, maybe we can use
> >>> the two counters.
> >>>
> >>>> Otherwise, a possibility is to add a more narrow filter (e.g. flower),
> >>>>  to only mangle packets that have the ack flag but not the syn or
> >>>> push. We might also try corrupting the TCP payload to a smaller
> >>>> offset, so that more packets are impacted.
> >>>
> >>> Yes, maybe good to narrow a bit the filter by using:
> >>>
> >>>   flower ip_proto tcp tcp_flags 0x10/0xff

This dosen't work in my test, so I didn't include it in the new
version.

Thanks,
-Geliang

> >>>
> >>> Or is there another filter to specify a minimum size?
> >>
> >> unfortunately we can't do that with TC filters (unless using the eBPF
> >> classifier for this (and I'm still not 100% the verifier would be
> >> happy _ but that's still in my long-term todo list :) ). I know there
> >> are ongoing efforts to introduce a "check_pkt_larger" (because this
> >> check is actually a possible openflow action) , but that won't happen
> >> in the short period.
> > 
> > That's a shame but then better to restrict to ACK flag to omit a few
> > packets.
> 
> As discussed at the last meeting, we should reduce the number of kernel
> message (pr_info for each packet not matched by the pedit action). For that:
> - best to restrict to TCP packets with ACK flag only:
> 
>     flower ip_proto tcp tcp_flags 0x10/0xff
> 
> - and send only in one direction to reduce the chance to have "pure ACKs".
> 
> Having a few pr_info message is OK but plenty messages in dmesg/serial
> will likely cause other issues (flooding logs, slowing down stuff, etc.)
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.