tools/testing/selftests/net/mptcp/config | 8 +++ .../testing/selftests/net/mptcp/mptcp_join.sh | 50 ++++++++++++++++--- 2 files changed, 51 insertions(+), 7 deletions(-)
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
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
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 >
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
© 2016 - 2024 Red Hat, Inc.