* [PATCH mptcp-next v2 0/2] add mp_fail testcases @ 2022-02-08 23:59 Geliang Tang 2022-02-08 23:59 ` [PATCH mptcp-next v2 1/2] Squash to "mptcp: infinite mapping receiving" Geliang Tang 2022-02-08 23:59 ` [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases Geliang Tang 0 siblings, 2 replies; 7+ messages in thread From: Geliang Tang @ 2022-02-08 23:59 UTC (permalink / raw) To: mptcp; +Cc: Geliang Tang This is v12 of the mp_fail testcases with Matt's changes. It works well and very stable. Geliang Tang (2): Squash to "mptcp: infinite mapping receiving" selftests: mptcp: add mp_fail testcases net/mptcp/subflow.c | 1 + tools/testing/selftests/net/mptcp/config | 8 + .../testing/selftests/net/mptcp/mptcp_join.sh | 142 ++++++++++++++++-- 3 files changed, 137 insertions(+), 14 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH mptcp-next v2 1/2] Squash to "mptcp: infinite mapping receiving" 2022-02-08 23:59 [PATCH mptcp-next v2 0/2] add mp_fail testcases Geliang Tang @ 2022-02-08 23:59 ` Geliang Tang 2022-02-08 23:59 ` [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases Geliang Tang 1 sibling, 0 replies; 7+ messages in thread From: Geliang Tang @ 2022-02-08 23:59 UTC (permalink / raw) To: mptcp; +Cc: Geliang Tang Print out the infinite map received info. Signed-off-by: Geliang Tang <geliang.tang@suse.com> --- net/mptcp/subflow.c | 1 + 1 file changed, 1 insertion(+) diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 485f00dcaf84..ae4d855f3c2f 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -962,6 +962,7 @@ static enum mapping_status get_mapping_status(struct sock *ssk, data_len = mpext->data_len; if (data_len == 0) { + pr_debug("infinite mapping received"); MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX); subflow->map_data_len = 0; return MAPPING_INVALID; -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases 2022-02-08 23:59 [PATCH mptcp-next v2 0/2] add mp_fail testcases Geliang Tang 2022-02-08 23:59 ` [PATCH mptcp-next v2 1/2] Squash to "mptcp: infinite mapping receiving" Geliang Tang @ 2022-02-08 23:59 ` Geliang Tang 2022-02-09 2:10 ` Mat Martineau 1 sibling, 1 reply; 7+ messages in thread From: Geliang Tang @ 2022-02-08 23:59 UTC (permalink / raw) To: mptcp; +Cc: Geliang Tang, Davide Caratti, Matthieu Baerts Added the test cases for MP_FAIL, the multiple subflows test for MP_RST and the single subflow test for the infinite mapping, use 'tc' commands to trigger the checksum failures. Suggested-by: Davide Caratti <dcaratti@redhat.com> Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net> Signed-off-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 | 142 ++++++++++++++++-- 2 files changed, 136 insertions(+), 14 deletions(-) diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config index d36b7da5082a..38021a0dd527 100644 --- a/tools/testing/selftests/net/mptcp/config +++ b/tools/testing/selftests/net/mptcp/config @@ -12,6 +12,9 @@ CONFIG_NF_TABLES=m CONFIG_NFT_COMPAT=m CONFIG_NETFILTER_XTABLES=m CONFIG_NETFILTER_XT_MATCH_BPF=m +CONFIG_NETFILTER_XT_MATCH_LENGTH=m +CONFIG_NETFILTER_XT_MATCH_STATISTIC=m +CONFIG_NETFILTER_XT_TARGET_MARK=m CONFIG_NF_TABLES_INET=y CONFIG_NFT_TPROXY=m CONFIG_NFT_SOCKET=m @@ -19,3 +22,8 @@ CONFIG_IP_ADVANCED_ROUTER=y CONFIG_IP_MULTIPLE_TABLES=y CONFIG_IP_NF_TARGET_REJECT=m CONFIG_IPV6_MULTIPLE_TABLES=y +CONFIG_NET_ACT_CSUM=m +CONFIG_NET_ACT_PEDIT=m +CONFIG_NET_CLS_ACT=y +CONFIG_NET_CLS_FW=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 90a6adc36490..686c54276ea9 100755 --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh @@ -17,6 +17,8 @@ capture=0 checksum=0 ip_mptcp=0 check_invert=0 +validate_checksum=0 +nr_mp_fail=0 do_all_tests=1 TEST_COUNT=0 @@ -62,6 +64,7 @@ init() done check_invert=0 + validate_checksum=$checksum # ns1 ns2 # ns1eth1 ns2eth1 @@ -167,6 +170,52 @@ reset_with_allow_join_id0() ip netns exec $ns2 sysctl -q net.mptcp.allow_join_initial_addr_port=$ns2_enable } +# Modify TCP payload without corrupting the TCP packet +# +# This rule inverts a 8-bit word at byte offset 148 for the 2nd TCP ACK packets +# carrying enough data. +# Once it is done, the TCP Checksum field is updated so the packet is still +# considered as valid at the TCP level. +# Because the MPTCP checksum, covering the TCP options and data, has not been +# updated, the modification will be detected and an MP_FAIL will be emitted: +# what we want to validate here without corrupting "random" MPTCP options. +# +# To avoid having tc producing this pr_info() message for each TCP ACK packets +# not carrying enough data: +# +# tc action pedit offset 162 out of bounds +# +# Netfilter is used to mark packets with enough data. +reset_with_fail() +{ + reset + + 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 + nr_mp_fail=0 + local i="$1" + + ip netns exec $ns2 iptables \ + -t mangle \ + -A OUTPUT \ + -o ns2eth$i \ + -p tcp \ + -m length --length 150:9999 \ + -m statistic --mode nth --packet 1 --every 99999 \ + -j MARK --set-mark 42 + + tc -n $ns2 qdisc add dev ns2eth$i clsact + 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 +} + ip -Version > /dev/null 2>&1 if [ $? -ne 0 ];then echo "SKIP: Could not run test without ip tool" @@ -185,6 +234,12 @@ if [ $? -ne 0 ];then exit $ksft_skip fi +jq -V > /dev/null 2>&1 +if [ $? -ne 0 ];then + echo "SKIP: Could not run all tests without jq tool" + exit $ksft_skip +fi + print_file_err() { ls -l "$1" 1>&2 @@ -245,6 +300,19 @@ link_failure() done } +get_nr_mp_fail() +{ + i="$1" + + local action=$(tc -n $ns2 -j -s action show action pedit index 100) + local packets=$(echo $action | jq '.[1].actions[0].stats.packets') + + if [ $packets -gt 0 ]; then + nr_mp_fail=1 + fi + tc -n $ns2 qdisc del dev ns2eth$i clsact +} + # $1: IP address is_v6() { @@ -446,7 +514,7 @@ do_transfer() local_addr="0.0.0.0" fi - if [ "$test_link_fail" -eq 2 ];then + if [ "$test_link_fail" -gt 1 ];then timeout ${timeout_test} \ ip netns exec ${listener_ns} \ $mptcp_connect -t ${timeout_poll} -l -p $port -s ${srv_proto} \ @@ -466,13 +534,19 @@ do_transfer() ip netns exec ${connector_ns} \ $mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \ $connect_addr < "$cin" > "$cout" & - else + elif [ "$test_link_fail" -eq 1 ] || [ "$test_link_fail" -eq 2 ];then ( cat "$cinfail" ; sleep 2; link_failure $listener_ns ; cat "$cinfail" ) | \ tee "$cinsent" | \ timeout ${timeout_test} \ ip netns exec ${connector_ns} \ $mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \ $connect_addr > "$cout" & + else + cat "$cinfail" | tee "$cinsent" | \ + timeout ${timeout_test} \ + ip netns exec ${connector_ns} \ + $mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \ + $connect_addr > "$cout" & fi cpid=$! @@ -632,7 +706,7 @@ do_transfer() return 1 fi - if [ "$test_link_fail" -eq 2 ];then + if [ "$test_link_fail" -gt 1 ];then check_transfer $sinfail $cout "file received by client" else check_transfer $sin $cout "file received by client" @@ -681,7 +755,12 @@ run_tests() # create the input file for the failure test when # the first failure test run - if [ "$test_linkfail" -ne 0 -a -z "$cinfail" ]; then + if [ "$test_linkfail" -eq 3 ]; then + if [ -z "$cinfail" ]; then + cinfail=$(mktemp) + fi + make_file "$cinfail" "client" 512 + elif [ "$test_linkfail" -ne 0 -a -z "$cinfail" ]; then # the client file must be considerably larger # of the maximum expected cwin value, or the # link utilization will be not predicable @@ -694,7 +773,12 @@ run_tests() make_file "$cinfail" "client" $size fi - if [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then + if [ "$test_linkfail" -eq 3 ]; then + if [ -z "$sinfail" ]; then + sinfail=$(mktemp) + fi + make_file "$sinfail" "server" 512 + elif [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then size=$((RANDOM%16)) size=$((size+1)) size=$((size*2048)) @@ -719,6 +803,8 @@ dump_stats() chk_csum_nr() { local msg=${1:-""} + local csum_ns1=${2:-0} + local csum_ns2=${3:-0} local count local dump_stats @@ -730,8 +816,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 @@ -740,8 +826,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 @@ -820,6 +906,8 @@ chk_join_nr() local syn_nr=$2 local syn_ack_nr=$3 local ack_nr=$4 + local fail_nr=${5:-0} + local infi_nr=${6:-0} local count local dump_stats @@ -856,10 +944,10 @@ chk_join_nr() echo "[ ok ]" fi [ "${dump_stats}" = 1 ] && dump_stats - if [ $checksum -eq 1 ]; then - chk_csum_nr - chk_fail_nr 0 0 - chk_infi_nr 0 0 + if [ $validate_checksum -eq 1 ]; then + chk_csum_nr "" $fail_nr + chk_fail_nr $fail_nr $fail_nr + chk_infi_nr $infi_nr $infi_nr fi } @@ -2164,6 +2252,27 @@ userspace_tests() chk_rm_nr 0 0 } +fail_tests() +{ + # multiple subflows + reset_with_fail 2 + pm_nl_set_limits $ns1 0 2 + pm_nl_set_limits $ns2 0 2 + pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow + pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow + run_tests $ns1 $ns2 10.0.1.1 3 + get_nr_mp_fail 2 + chk_join_nr "$nr_mp_fail MP_FAIL, multiple subflows, MP_RST" 2 2 2 $nr_mp_fail + + # single subflow + reset_with_fail 1 + pm_nl_set_limits $ns1 0 2 + pm_nl_set_limits $ns2 0 2 + run_tests $ns1 $ns2 10.0.1.1 3 + get_nr_mp_fail 1 + chk_join_nr "$nr_mp_fail MP_FAIL, single subflow, infinite" 0 0 0 $nr_mp_fail $nr_mp_fail +} + all_tests() { subflows_tests @@ -2182,6 +2291,7 @@ all_tests() deny_join_id0_tests fullmesh_tests userspace_tests + fail_tests } usage() @@ -2203,6 +2313,7 @@ usage() echo " -d deny_join_id0_tests" echo " -m fullmesh_tests" echo " -u userspace_tests" + echo " -F fail_tests" echo " -c capture pcap files" echo " -C enable data checksum" echo " -i use ip mptcp" @@ -2242,7 +2353,7 @@ if [ $do_all_tests -eq 1 ]; then exit $ret fi -while getopts 'fesltra64bpkdmuchCSi' opt; do +while getopts 'fesltra64bpkdmuchCSFi' opt; do case $opt in f) subflows_tests @@ -2292,6 +2403,9 @@ while getopts 'fesltra64bpkdmuchCSi' opt; do u) userspace_tests ;; + F) + fail_tests + ;; c) ;; C) -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases 2022-02-08 23:59 ` [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases Geliang Tang @ 2022-02-09 2:10 ` Mat Martineau 2022-02-09 5:47 ` Geliang Tang 2022-02-09 8:56 ` Matthieu Baerts 0 siblings, 2 replies; 7+ messages in thread From: Mat Martineau @ 2022-02-09 2:10 UTC (permalink / raw) To: Geliang Tang; +Cc: mptcp, Davide Caratti, Matthieu Baerts On Wed, 9 Feb 2022, Geliang Tang wrote: > Added the test cases for MP_FAIL, the multiple subflows test for MP_RST > and the single subflow test for the infinite mapping, use 'tc' commands > to trigger the checksum failures. > > Suggested-by: Davide Caratti <dcaratti@redhat.com> > Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net> > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> > Signed-off-by: Geliang Tang <geliang.tang@suse.com> Once I built a correctly configured kernel, this worked great! Thanks Geliang and Matthieu. Before I fixed my configuration, I ran in to a couple of issues, see below. > --- > tools/testing/selftests/net/mptcp/config | 8 + > .../testing/selftests/net/mptcp/mptcp_join.sh | 142 ++++++++++++++++-- > 2 files changed, 136 insertions(+), 14 deletions(-) > > diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config > index d36b7da5082a..38021a0dd527 100644 > --- a/tools/testing/selftests/net/mptcp/config > +++ b/tools/testing/selftests/net/mptcp/config > @@ -12,6 +12,9 @@ CONFIG_NF_TABLES=m > CONFIG_NFT_COMPAT=m > CONFIG_NETFILTER_XTABLES=m > CONFIG_NETFILTER_XT_MATCH_BPF=m > +CONFIG_NETFILTER_XT_MATCH_LENGTH=m > +CONFIG_NETFILTER_XT_MATCH_STATISTIC=m > +CONFIG_NETFILTER_XT_TARGET_MARK=m > CONFIG_NF_TABLES_INET=y > CONFIG_NFT_TPROXY=m > CONFIG_NFT_SOCKET=m > @@ -19,3 +22,8 @@ CONFIG_IP_ADVANCED_ROUTER=y > CONFIG_IP_MULTIPLE_TABLES=y > CONFIG_IP_NF_TARGET_REJECT=m > CONFIG_IPV6_MULTIPLE_TABLES=y > +CONFIG_NET_ACT_CSUM=m > +CONFIG_NET_ACT_PEDIT=m > +CONFIG_NET_CLS_ACT=y > +CONFIG_NET_CLS_FW=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 90a6adc36490..686c54276ea9 100755 > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh > @@ -17,6 +17,8 @@ capture=0 > checksum=0 > ip_mptcp=0 > check_invert=0 > +validate_checksum=0 > +nr_mp_fail=0 > do_all_tests=1 > > TEST_COUNT=0 > @@ -62,6 +64,7 @@ init() > done > > check_invert=0 > + validate_checksum=$checksum > > # ns1 ns2 > # ns1eth1 ns2eth1 > @@ -167,6 +170,52 @@ reset_with_allow_join_id0() > ip netns exec $ns2 sysctl -q net.mptcp.allow_join_initial_addr_port=$ns2_enable > } > > +# Modify TCP payload without corrupting the TCP packet > +# > +# This rule inverts a 8-bit word at byte offset 148 for the 2nd TCP ACK packets > +# carrying enough data. > +# Once it is done, the TCP Checksum field is updated so the packet is still > +# considered as valid at the TCP level. > +# Because the MPTCP checksum, covering the TCP options and data, has not been > +# updated, the modification will be detected and an MP_FAIL will be emitted: > +# what we want to validate here without corrupting "random" MPTCP options. > +# > +# To avoid having tc producing this pr_info() message for each TCP ACK packets > +# not carrying enough data: > +# > +# tc action pedit offset 162 out of bounds > +# > +# Netfilter is used to mark packets with enough data. > +reset_with_fail() > +{ > + reset > + > + 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 > + nr_mp_fail=0 > + local i="$1" > + > + ip netns exec $ns2 iptables \ > + -t mangle \ > + -A OUTPUT \ > + -o ns2eth$i \ > + -p tcp \ > + -m length --length 150:9999 \ > + -m statistic --mode nth --packet 1 --every 99999 \ > + -j MARK --set-mark 42 > + I think it would help to check the exit code of iptables here, my first attempt was with a misconfigured kernel and it would help to fail in a more informative way: $ sudo ./mptcp_join.sh -F Created /tmp/tmp.P4AWbNYGQ5 (size 1 KB) containing data sent by client Created /tmp/tmp.ny1Q9MWo85 (size 1 KB) containing data sent by server iptables v1.8.7 (legacy): Couldn't load match `length':No such file or directory Try `iptables -h' or 'iptables --help' for more information. Error: TC classifier not found. We have an error talking to the kernel > + tc -n $ns2 qdisc add dev ns2eth$i clsact > + 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 > +} > + > ip -Version > /dev/null 2>&1 > if [ $? -ne 0 ];then > echo "SKIP: Could not run test without ip tool" > @@ -185,6 +234,12 @@ if [ $? -ne 0 ];then > exit $ksft_skip > fi > > +jq -V > /dev/null 2>&1 > +if [ $? -ne 0 ];then > + echo "SKIP: Could not run all tests without jq tool" > + exit $ksft_skip > +fi > + > print_file_err() > { > ls -l "$1" 1>&2 > @@ -245,6 +300,19 @@ link_failure() > done > } > > +get_nr_mp_fail() > +{ > + i="$1" > + > + local action=$(tc -n $ns2 -j -s action show action pedit index 100) > + local packets=$(echo $action | jq '.[1].actions[0].stats.packets') > + > + if [ $packets -gt 0 ]; then This triggered a bash parsing error when I did not have the added netfilter kernel config options: $ sudo ./mptcp_join.sh -F Created /tmp/tmp.P4AWbNYGQ5 (size 1 KB) containing data sent by client Created /tmp/tmp.ny1Q9MWo85 (size 1 KB) containing data sent by server iptables v1.8.7 (legacy): Couldn't load match `length':No such file or directory Try `iptables -h' or 'iptables --help' for more information. Error: TC classifier not found. We have an error talking to the kernel Created /tmp/tmp.CvX8qnG1gC (size 512 KB) containing data sent by client Created /tmp/tmp.wenxUAgsj8 (size 512 KB) containing data sent by server ./mptcp_join.sh: line 310: [: null: integer expression expected 001 0 MP_FAIL, multiple subflows, MP_RST syn[ ok ] - synack[ ok ] - ack[ ok ] sum[ ok ] - csum [ ok ] ftx[ ok ] - frx [ ok ] itx[ ok ] - irx [ ok ] > + nr_mp_fail=1 > + fi > + tc -n $ns2 qdisc del dev ns2eth$i clsact > +} > + > # $1: IP address > is_v6() > { > @@ -446,7 +514,7 @@ do_transfer() > local_addr="0.0.0.0" > fi > > - if [ "$test_link_fail" -eq 2 ];then > + if [ "$test_link_fail" -gt 1 ];then > timeout ${timeout_test} \ > ip netns exec ${listener_ns} \ > $mptcp_connect -t ${timeout_poll} -l -p $port -s ${srv_proto} \ > @@ -466,13 +534,19 @@ do_transfer() > ip netns exec ${connector_ns} \ > $mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \ > $connect_addr < "$cin" > "$cout" & > - else > + elif [ "$test_link_fail" -eq 1 ] || [ "$test_link_fail" -eq 2 ];then > ( cat "$cinfail" ; sleep 2; link_failure $listener_ns ; cat "$cinfail" ) | \ > tee "$cinsent" | \ > timeout ${timeout_test} \ > ip netns exec ${connector_ns} \ > $mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \ > $connect_addr > "$cout" & > + else > + cat "$cinfail" | tee "$cinsent" | \ > + timeout ${timeout_test} \ > + ip netns exec ${connector_ns} \ > + $mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \ > + $connect_addr > "$cout" & > fi > cpid=$! > > @@ -632,7 +706,7 @@ do_transfer() > return 1 > fi > > - if [ "$test_link_fail" -eq 2 ];then > + if [ "$test_link_fail" -gt 1 ];then > check_transfer $sinfail $cout "file received by client" > else > check_transfer $sin $cout "file received by client" > @@ -681,7 +755,12 @@ run_tests() > > # create the input file for the failure test when > # the first failure test run > - if [ "$test_linkfail" -ne 0 -a -z "$cinfail" ]; then > + if [ "$test_linkfail" -eq 3 ]; then > + if [ -z "$cinfail" ]; then > + cinfail=$(mktemp) > + fi > + make_file "$cinfail" "client" 512 > + elif [ "$test_linkfail" -ne 0 -a -z "$cinfail" ]; then > # the client file must be considerably larger > # of the maximum expected cwin value, or the > # link utilization will be not predicable > @@ -694,7 +773,12 @@ run_tests() > make_file "$cinfail" "client" $size > fi > > - if [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then > + if [ "$test_linkfail" -eq 3 ]; then > + if [ -z "$sinfail" ]; then > + sinfail=$(mktemp) > + fi > + make_file "$sinfail" "server" 512 > + elif [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then > size=$((RANDOM%16)) > size=$((size+1)) > size=$((size*2048)) > @@ -719,6 +803,8 @@ dump_stats() > chk_csum_nr() > { > local msg=${1:-""} > + local csum_ns1=${2:-0} > + local csum_ns2=${3:-0} > local count > local dump_stats > > @@ -730,8 +816,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 > @@ -740,8 +826,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 > @@ -820,6 +906,8 @@ chk_join_nr() > local syn_nr=$2 > local syn_ack_nr=$3 > local ack_nr=$4 > + local fail_nr=${5:-0} > + local infi_nr=${6:-0} > local count > local dump_stats > > @@ -856,10 +944,10 @@ chk_join_nr() > echo "[ ok ]" > fi > [ "${dump_stats}" = 1 ] && dump_stats > - if [ $checksum -eq 1 ]; then > - chk_csum_nr > - chk_fail_nr 0 0 > - chk_infi_nr 0 0 > + if [ $validate_checksum -eq 1 ]; then > + chk_csum_nr "" $fail_nr > + chk_fail_nr $fail_nr $fail_nr > + chk_infi_nr $infi_nr $infi_nr > fi > } > > @@ -2164,6 +2252,27 @@ userspace_tests() > chk_rm_nr 0 0 > } > > +fail_tests() > +{ > + # multiple subflows > + reset_with_fail 2 > + pm_nl_set_limits $ns1 0 2 > + pm_nl_set_limits $ns2 0 2 > + pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow > + pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow > + run_tests $ns1 $ns2 10.0.1.1 3 > + get_nr_mp_fail 2 > + chk_join_nr "$nr_mp_fail MP_FAIL, multiple subflows, MP_RST" 2 2 2 $nr_mp_fail Is the first $nr_mp_fail supposed to be in the test description? It's not clear what it means in the test output. > + > + # single subflow > + reset_with_fail 1 > + pm_nl_set_limits $ns1 0 2 > + pm_nl_set_limits $ns2 0 2 > + run_tests $ns1 $ns2 10.0.1.1 3 > + get_nr_mp_fail 1 > + chk_join_nr "$nr_mp_fail MP_FAIL, single subflow, infinite" 0 0 0 $nr_mp_fail $nr_mp_fail ^^^^^^^^^^^ same question > +} > + > all_tests() > { > subflows_tests > @@ -2182,6 +2291,7 @@ all_tests() > deny_join_id0_tests > fullmesh_tests > userspace_tests > + fail_tests > } > > usage() > @@ -2203,6 +2313,7 @@ usage() > echo " -d deny_join_id0_tests" > echo " -m fullmesh_tests" > echo " -u userspace_tests" > + echo " -F fail_tests" > echo " -c capture pcap files" > echo " -C enable data checksum" > echo " -i use ip mptcp" > @@ -2242,7 +2353,7 @@ if [ $do_all_tests -eq 1 ]; then > exit $ret > fi > > -while getopts 'fesltra64bpkdmuchCSi' opt; do > +while getopts 'fesltra64bpkdmuchCSFi' opt; do > case $opt in > f) > subflows_tests > @@ -2292,6 +2403,9 @@ while getopts 'fesltra64bpkdmuchCSi' opt; do > u) > userspace_tests > ;; > + F) > + fail_tests > + ;; > c) > ;; > C) > -- > 2.34.1 > > > -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases 2022-02-09 2:10 ` Mat Martineau @ 2022-02-09 5:47 ` Geliang Tang 2022-02-09 8:59 ` Matthieu Baerts 2022-02-09 8:56 ` Matthieu Baerts 1 sibling, 1 reply; 7+ messages in thread From: Geliang Tang @ 2022-02-09 5:47 UTC (permalink / raw) To: Mat Martineau; +Cc: mptcp Hi Mat, Thanks for your review. On Tue, Feb 08, 2022 at 06:10:20PM -0800, Mat Martineau wrote: > On Wed, 9 Feb 2022, Geliang Tang wrote: > > > Added the test cases for MP_FAIL, the multiple subflows test for MP_RST > > and the single subflow test for the infinite mapping, use 'tc' commands > > to trigger the checksum failures. > > > > Suggested-by: Davide Caratti <dcaratti@redhat.com> > > Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net> > > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> > > Signed-off-by: Geliang Tang <geliang.tang@suse.com> > > Once I built a correctly configured kernel, this worked great! Thanks > Geliang and Matthieu. > > Before I fixed my configuration, I ran in to a couple of issues, see below. > > > --- > > tools/testing/selftests/net/mptcp/config | 8 + > > .../testing/selftests/net/mptcp/mptcp_join.sh | 142 ++++++++++++++++-- > > 2 files changed, 136 insertions(+), 14 deletions(-) > > > > diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config > > index d36b7da5082a..38021a0dd527 100644 > > --- a/tools/testing/selftests/net/mptcp/config > > +++ b/tools/testing/selftests/net/mptcp/config > > @@ -12,6 +12,9 @@ CONFIG_NF_TABLES=m > > CONFIG_NFT_COMPAT=m > > CONFIG_NETFILTER_XTABLES=m > > CONFIG_NETFILTER_XT_MATCH_BPF=m > > +CONFIG_NETFILTER_XT_MATCH_LENGTH=m > > +CONFIG_NETFILTER_XT_MATCH_STATISTIC=m > > +CONFIG_NETFILTER_XT_TARGET_MARK=m > > CONFIG_NF_TABLES_INET=y > > CONFIG_NFT_TPROXY=m > > CONFIG_NFT_SOCKET=m > > @@ -19,3 +22,8 @@ CONFIG_IP_ADVANCED_ROUTER=y > > CONFIG_IP_MULTIPLE_TABLES=y > > CONFIG_IP_NF_TARGET_REJECT=m > > CONFIG_IPV6_MULTIPLE_TABLES=y > > +CONFIG_NET_ACT_CSUM=m > > +CONFIG_NET_ACT_PEDIT=m > > +CONFIG_NET_CLS_ACT=y > > +CONFIG_NET_CLS_FW=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 90a6adc36490..686c54276ea9 100755 > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh > > @@ -17,6 +17,8 @@ capture=0 > > checksum=0 > > ip_mptcp=0 > > check_invert=0 > > +validate_checksum=0 > > +nr_mp_fail=0 > > do_all_tests=1 > > > > TEST_COUNT=0 > > @@ -62,6 +64,7 @@ init() > > done > > > > check_invert=0 > > + validate_checksum=$checksum > > > > # ns1 ns2 > > # ns1eth1 ns2eth1 > > @@ -167,6 +170,52 @@ reset_with_allow_join_id0() > > ip netns exec $ns2 sysctl -q net.mptcp.allow_join_initial_addr_port=$ns2_enable > > } > > > > +# Modify TCP payload without corrupting the TCP packet > > +# > > +# This rule inverts a 8-bit word at byte offset 148 for the 2nd TCP ACK packets > > +# carrying enough data. > > +# Once it is done, the TCP Checksum field is updated so the packet is still > > +# considered as valid at the TCP level. > > +# Because the MPTCP checksum, covering the TCP options and data, has not been > > +# updated, the modification will be detected and an MP_FAIL will be emitted: > > +# what we want to validate here without corrupting "random" MPTCP options. > > +# > > +# To avoid having tc producing this pr_info() message for each TCP ACK packets > > +# not carrying enough data: > > +# > > +# tc action pedit offset 162 out of bounds > > +# > > +# Netfilter is used to mark packets with enough data. > > +reset_with_fail() > > +{ > > + reset > > + > > + 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 > > + nr_mp_fail=0 > > + local i="$1" > > + > > + ip netns exec $ns2 iptables \ > > + -t mangle \ > > + -A OUTPUT \ > > + -o ns2eth$i \ > > + -p tcp \ > > + -m length --length 150:9999 \ > > + -m statistic --mode nth --packet 1 --every 99999 \ > > + -j MARK --set-mark 42 > > + > > I think it would help to check the exit code of iptables here, my first > attempt was with a misconfigured kernel and it would help to fail in a more > informative way: > > $ sudo ./mptcp_join.sh -F > Created /tmp/tmp.P4AWbNYGQ5 (size 1 KB) containing data sent by client > Created /tmp/tmp.ny1Q9MWo85 (size 1 KB) containing data sent by server > iptables v1.8.7 (legacy): Couldn't load match `length':No such file or directory > Try `iptables -h' or 'iptables --help' for more information. > Error: TC classifier not found. > We have an error talking to the kernel > Done in v3. > > > + tc -n $ns2 qdisc add dev ns2eth$i clsact > > + 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 > > +} > > + > > ip -Version > /dev/null 2>&1 > > if [ $? -ne 0 ];then > > echo "SKIP: Could not run test without ip tool" > > @@ -185,6 +234,12 @@ if [ $? -ne 0 ];then > > exit $ksft_skip > > fi > > > > +jq -V > /dev/null 2>&1 > > +if [ $? -ne 0 ];then > > + echo "SKIP: Could not run all tests without jq tool" > > + exit $ksft_skip > > +fi > > + > > print_file_err() > > { > > ls -l "$1" 1>&2 > > @@ -245,6 +300,19 @@ link_failure() > > done > > } > > > > +get_nr_mp_fail() > > +{ > > + i="$1" > > + > > + local action=$(tc -n $ns2 -j -s action show action pedit index 100) > > + local packets=$(echo $action | jq '.[1].actions[0].stats.packets') > > + > > + if [ $packets -gt 0 ]; then > > This triggered a bash parsing error when I did not have the added netfilter > kernel config options: > > $ sudo ./mptcp_join.sh -F > Created /tmp/tmp.P4AWbNYGQ5 (size 1 KB) containing data sent by client > Created /tmp/tmp.ny1Q9MWo85 (size 1 KB) containing data sent by server > iptables v1.8.7 (legacy): Couldn't load match `length':No such file or directory > Try `iptables -h' or 'iptables --help' for more information. > Error: TC classifier not found. > We have an error talking to the kernel > Created /tmp/tmp.CvX8qnG1gC (size 512 KB) containing data sent by client > Created /tmp/tmp.wenxUAgsj8 (size 512 KB) containing data sent by server > ./mptcp_join.sh: line 310: [: null: integer expression expected > 001 0 MP_FAIL, multiple subflows, MP_RST syn[ ok ] - synack[ ok ] - ack[ ok ] > sum[ ok ] - csum [ ok ] > ftx[ ok ] - frx [ ok ] > itx[ ok ] - irx [ ok ] > Done in v3. > > > > + nr_mp_fail=1 > > + fi > > + tc -n $ns2 qdisc del dev ns2eth$i clsact > > +} > > + > > # $1: IP address > > is_v6() > > { > > @@ -446,7 +514,7 @@ do_transfer() > > local_addr="0.0.0.0" > > fi > > > > - if [ "$test_link_fail" -eq 2 ];then > > + if [ "$test_link_fail" -gt 1 ];then > > timeout ${timeout_test} \ > > ip netns exec ${listener_ns} \ > > $mptcp_connect -t ${timeout_poll} -l -p $port -s ${srv_proto} \ > > @@ -466,13 +534,19 @@ do_transfer() > > ip netns exec ${connector_ns} \ > > $mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \ > > $connect_addr < "$cin" > "$cout" & > > - else > > + elif [ "$test_link_fail" -eq 1 ] || [ "$test_link_fail" -eq 2 ];then > > ( cat "$cinfail" ; sleep 2; link_failure $listener_ns ; cat "$cinfail" ) | \ > > tee "$cinsent" | \ > > timeout ${timeout_test} \ > > ip netns exec ${connector_ns} \ > > $mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \ > > $connect_addr > "$cout" & > > + else > > + cat "$cinfail" | tee "$cinsent" | \ > > + timeout ${timeout_test} \ > > + ip netns exec ${connector_ns} \ > > + $mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \ > > + $connect_addr > "$cout" & > > fi > > cpid=$! > > > > @@ -632,7 +706,7 @@ do_transfer() > > return 1 > > fi > > > > - if [ "$test_link_fail" -eq 2 ];then > > + if [ "$test_link_fail" -gt 1 ];then > > check_transfer $sinfail $cout "file received by client" > > else > > check_transfer $sin $cout "file received by client" > > @@ -681,7 +755,12 @@ run_tests() > > > > # create the input file for the failure test when > > # the first failure test run > > - if [ "$test_linkfail" -ne 0 -a -z "$cinfail" ]; then > > + if [ "$test_linkfail" -eq 3 ]; then > > + if [ -z "$cinfail" ]; then > > + cinfail=$(mktemp) > > + fi > > + make_file "$cinfail" "client" 512 > > + elif [ "$test_linkfail" -ne 0 -a -z "$cinfail" ]; then > > # the client file must be considerably larger > > # of the maximum expected cwin value, or the > > # link utilization will be not predicable > > @@ -694,7 +773,12 @@ run_tests() > > make_file "$cinfail" "client" $size > > fi > > > > - if [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then > > + if [ "$test_linkfail" -eq 3 ]; then > > + if [ -z "$sinfail" ]; then > > + sinfail=$(mktemp) > > + fi > > + make_file "$sinfail" "server" 512 > > + elif [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then > > size=$((RANDOM%16)) > > size=$((size+1)) > > size=$((size*2048)) > > @@ -719,6 +803,8 @@ dump_stats() > > chk_csum_nr() > > { > > local msg=${1:-""} > > + local csum_ns1=${2:-0} > > + local csum_ns2=${3:-0} > > local count > > local dump_stats > > > > @@ -730,8 +816,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 > > @@ -740,8 +826,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 > > @@ -820,6 +906,8 @@ chk_join_nr() > > local syn_nr=$2 > > local syn_ack_nr=$3 > > local ack_nr=$4 > > + local fail_nr=${5:-0} > > + local infi_nr=${6:-0} > > local count > > local dump_stats > > > > @@ -856,10 +944,10 @@ chk_join_nr() > > echo "[ ok ]" > > fi > > [ "${dump_stats}" = 1 ] && dump_stats > > - if [ $checksum -eq 1 ]; then > > - chk_csum_nr > > - chk_fail_nr 0 0 > > - chk_infi_nr 0 0 > > + if [ $validate_checksum -eq 1 ]; then > > + chk_csum_nr "" $fail_nr > > + chk_fail_nr $fail_nr $fail_nr > > + chk_infi_nr $infi_nr $infi_nr > > fi > > } > > > > @@ -2164,6 +2252,27 @@ userspace_tests() > > chk_rm_nr 0 0 > > } > > > > +fail_tests() > > +{ > > + # multiple subflows > > + reset_with_fail 2 > > + pm_nl_set_limits $ns1 0 2 > > + pm_nl_set_limits $ns2 0 2 > > + pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow > > + pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow > > + run_tests $ns1 $ns2 10.0.1.1 3 > > + get_nr_mp_fail 2 > > + chk_join_nr "$nr_mp_fail MP_FAIL, multiple subflows, MP_RST" 2 2 2 $nr_mp_fail > > Is the first $nr_mp_fail supposed to be in the test description? It's not > clear what it means in the test output. I renamed nr_mp_fail to pedit_action and get_nr_mp_fail to pedit_action_happened in v3. And updated the test descriptions like this: 001 MP_FAIL MP_RST: 1 pedit action syn[ ok ] - synack[ ok ] - ack[ ok ] 002 MP_FAIL infinite map: 1 pedit action syn[ ok ] - synack[ ok ] - ack[ ok ] I put the variable pedit_action in the test description to distinguish between the following two cases: 1 Created /tmp/tmp.vizRgvd6NM (size 512 KB) containing data sent by client Created /tmp/tmp.UZqbnNdWAK (size 512 KB) containing data sent by server 001 MP_FAIL MP_RST: 0 pedit action syn[ ok ] - synack[ ok ] - ack[ ok ] sum[ ok ] - csum [ ok ] ftx[ ok ] - frx [ ok ] itx[ ok ] - irx [ ok ] All checks passed, and no inverted byte received. In this case, no checksum failure happened. 2 Created /tmp/tmp.vizRgvd6NM (size 512 KB) containing data sent by client Created /tmp/tmp.UZqbnNdWAK (size 512 KB) containing data sent by server 001 MP_FAIL MP_RST: 1 pedit action syn[ ok ] - synack[ ok ] - ack[ ok ] sum[ ok ] - csum [ ok ] ftx[ ok ] - frx [ ok ] itx[ ok ] - irx [ ok ] All checks passed, and no inverted byte received. In this case, the bad data is dropped correctly. If no pedit_action showed in the test description, the outputs of these two cases will be the same. Thanks, -Geliang > > > + > > + # single subflow > > + reset_with_fail 1 > > + pm_nl_set_limits $ns1 0 2 > > + pm_nl_set_limits $ns2 0 2 > > + run_tests $ns1 $ns2 10.0.1.1 3 > > + get_nr_mp_fail 1 > > + chk_join_nr "$nr_mp_fail MP_FAIL, single subflow, infinite" 0 0 0 $nr_mp_fail $nr_mp_fail > > ^^^^^^^^^^^ same question > > > +} > > + > > all_tests() > > { > > subflows_tests > > @@ -2182,6 +2291,7 @@ all_tests() > > deny_join_id0_tests > > fullmesh_tests > > userspace_tests > > + fail_tests > > } > > > > usage() > > @@ -2203,6 +2313,7 @@ usage() > > echo " -d deny_join_id0_tests" > > echo " -m fullmesh_tests" > > echo " -u userspace_tests" > > + echo " -F fail_tests" > > echo " -c capture pcap files" > > echo " -C enable data checksum" > > echo " -i use ip mptcp" > > @@ -2242,7 +2353,7 @@ if [ $do_all_tests -eq 1 ]; then > > exit $ret > > fi > > > > -while getopts 'fesltra64bpkdmuchCSi' opt; do > > +while getopts 'fesltra64bpkdmuchCSFi' opt; do > > case $opt in > > f) > > subflows_tests > > @@ -2292,6 +2403,9 @@ while getopts 'fesltra64bpkdmuchCSi' opt; do > > u) > > userspace_tests > > ;; > > + F) > > + fail_tests > > + ;; > > c) > > ;; > > C) > > -- > > 2.34.1 > > > > > > > > -- > Mat Martineau > Intel > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases 2022-02-09 5:47 ` Geliang Tang @ 2022-02-09 8:59 ` Matthieu Baerts 0 siblings, 0 replies; 7+ messages in thread From: Matthieu Baerts @ 2022-02-09 8:59 UTC (permalink / raw) To: Geliang Tang, Mat Martineau; +Cc: mptcp Hi Geliang, On 09/02/2022 06:47, Geliang Tang wrote: > On Tue, Feb 08, 2022 at 06:10:20PM -0800, Mat Martineau wrote: >> On Wed, 9 Feb 2022, Geliang Tang wrote: >>> +fail_tests() >>> +{ >>> + # multiple subflows >>> + reset_with_fail 2 >>> + pm_nl_set_limits $ns1 0 2 >>> + pm_nl_set_limits $ns2 0 2 >>> + pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow >>> + pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow >>> + run_tests $ns1 $ns2 10.0.1.1 3 >>> + get_nr_mp_fail 2 >>> + chk_join_nr "$nr_mp_fail MP_FAIL, multiple subflows, MP_RST" 2 2 2 $nr_mp_fail >> >> Is the first $nr_mp_fail supposed to be in the test description? It's not >> clear what it means in the test output. > > I renamed nr_mp_fail to pedit_action and get_nr_mp_fail to > pedit_action_happened in v3. And updated the test descriptions like > this: > 001 MP_FAIL MP_RST: 1 pedit action syn[ ok ] - synack[ ok ] - ack[ ok ] > 002 MP_FAIL infinite map: 1 pedit action syn[ ok ] - synack[ ok ] - ack[ ok ] > > I put the variable pedit_action in the test description to distinguish > between the following two cases: > > 1 > Created /tmp/tmp.vizRgvd6NM (size 512 KB) containing data sent by client > Created /tmp/tmp.UZqbnNdWAK (size 512 KB) containing data sent by server > 001 MP_FAIL MP_RST: 0 pedit action syn[ ok ] - synack[ ok ] - ack[ ok ] > sum[ ok ] - csum [ ok ] > ftx[ ok ] - frx [ ok ] > itx[ ok ] - irx [ ok ] > > All checks passed, and no inverted byte received. In this case, no > checksum failure happened. I don't think we can say the test was a success in this case, no? We expect one MP_FAIL. If we didn't have any, that's an issue. Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases 2022-02-09 2:10 ` Mat Martineau 2022-02-09 5:47 ` Geliang Tang @ 2022-02-09 8:56 ` Matthieu Baerts 1 sibling, 0 replies; 7+ messages in thread From: Matthieu Baerts @ 2022-02-09 8:56 UTC (permalink / raw) To: Mat Martineau, Geliang Tang; +Cc: mptcp, Davide Caratti Hi Mat, On 09/02/2022 03:10, Mat Martineau wrote: > On Wed, 9 Feb 2022, Geliang Tang wrote: (...) >> + ip netns exec $ns2 iptables \ >> + -t mangle \ >> + -A OUTPUT \ >> + -o ns2eth$i \ >> + -p tcp \ >> + -m length --length 150:9999 \ >> + -m statistic --mode nth --packet 1 --every 99999 \ >> + -j MARK --set-mark 42 >> + > > I think it would help to check the exit code of iptables here, my first > attempt was with a misconfigured kernel and it would help to fail in a > more informative way: > > $ sudo ./mptcp_join.sh -F > Created /tmp/tmp.P4AWbNYGQ5 (size 1 KB) containing data sent by client > Created /tmp/tmp.ny1Q9MWo85 (size 1 KB) containing data sent by server > iptables v1.8.7 (legacy): Couldn't load match `length':No such file or > directory > Try `iptables -h' or 'iptables --help' for more information. > Error: TC classifier not found. > We have an error talking to the kernel I understand it is not good to ignore the error but just to be sure, here you suggest to exit with an error, right? In Geliang's v3, the whole test is skip but I don't think that's a good idea. I mean: we manage a kernel config file so we expect the right kernel config is applied. If the kconfig is not the correct one, that's an error. For the tools, that's different and that's why we check if iptables tool is available. (a plugin might be missing but no need to check everything I think). In other words, I think we should add a " || exit 1" after the IPTables and TC commands in this function. @Geliang: WDYT? (...) >> +get_nr_mp_fail() >> +{ >> + i="$1" >> + >> + local action=$(tc -n $ns2 -j -s action show action pedit index 100) >> + local packets=$(echo $action | jq '.[1].actions[0].stats.packets') >> + >> + if [ $packets -gt 0 ]; then > > This triggered a bash parsing error when I did not have the added > netfilter kernel config options: > > $ sudo ./mptcp_join.sh -F > Created /tmp/tmp.P4AWbNYGQ5 (size 1 KB) containing data sent by client > Created /tmp/tmp.ny1Q9MWo85 (size 1 KB) containing data sent by server > iptables v1.8.7 (legacy): Couldn't load match `length':No such file or > directory > Try `iptables -h' or 'iptables --help' for more information. > Error: TC classifier not found. > We have an error talking to the kernel > Created /tmp/tmp.CvX8qnG1gC (size 512 KB) containing data sent by client > Created /tmp/tmp.wenxUAgsj8 (size 512 KB) containing data sent by server > ./mptcp_join.sh: line 310: [: null: integer expression expected I think we don't have to deal with this error if we already check if the previous TC commands were OK, no? (...) >> +fail_tests() >> +{ >> + # multiple subflows >> + reset_with_fail 2 >> + pm_nl_set_limits $ns1 0 2 >> + pm_nl_set_limits $ns2 0 2 >> + pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow >> + pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow >> + run_tests $ns1 $ns2 10.0.1.1 3 >> + get_nr_mp_fail 2 >> + chk_join_nr "$nr_mp_fail MP_FAIL, multiple subflows, MP_RST" 2 2 >> 2 $nr_mp_fail > > Is the first $nr_mp_fail supposed to be in the test description? It's > not clear what it means in the test output. In the v3, this is written after, it can help if this test fails. But I don't think we should use it in the MP FAIL we expect. Otherwise we will have the same situation Mat had: IPTables and TC commands fail but the test showed no error :) So we should have: chk_join_nr "MP_FAIL (...) $nr_mp_fail" 2 2 2 1 No? Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-02-09 8:59 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-02-08 23:59 [PATCH mptcp-next v2 0/2] add mp_fail testcases Geliang Tang 2022-02-08 23:59 ` [PATCH mptcp-next v2 1/2] Squash to "mptcp: infinite mapping receiving" Geliang Tang 2022-02-08 23:59 ` [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases Geliang Tang 2022-02-09 2:10 ` Mat Martineau 2022-02-09 5:47 ` Geliang Tang 2022-02-09 8:59 ` Matthieu Baerts 2022-02-09 8:56 ` Matthieu Baerts
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.