* [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases @ 2021-10-28 3:16 Geliang Tang 2021-10-28 9:23 ` Geliang Tang 0 siblings, 1 reply; 9+ messages in thread From: Geliang Tang @ 2021-10-28 3:16 UTC (permalink / raw) To: mptcp; +Cc: Geliang Tang, Davide Caratti, Matthieu Baerts 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases 2021-10-28 3:16 [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases Geliang Tang @ 2021-10-28 9:23 ` Geliang Tang 2021-10-28 11:01 ` Davide Caratti 0 siblings, 1 reply; 9+ messages in thread From: Geliang Tang @ 2021-10-28 9:23 UTC (permalink / raw) To: mptcp; +Cc: Davide Caratti, Matthieu Baerts 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases 2021-10-28 9:23 ` Geliang Tang @ 2021-10-28 11:01 ` Davide Caratti 2021-10-28 11:21 ` Matthieu Baerts 0 siblings, 1 reply; 9+ messages in thread From: Davide Caratti @ 2021-10-28 11:01 UTC (permalink / raw) To: Geliang Tang; +Cc: MPTCP Upstream, Matthieu Baerts 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases 2021-10-28 11:01 ` Davide Caratti @ 2021-10-28 11:21 ` Matthieu Baerts 2021-10-28 12:24 ` Davide Caratti 0 siblings, 1 reply; 9+ messages in thread From: Matthieu Baerts @ 2021-10-28 11:21 UTC (permalink / raw) To: Davide Caratti, Geliang Tang; +Cc: MPTCP Upstream 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases 2021-10-28 11:21 ` Matthieu Baerts @ 2021-10-28 12:24 ` Davide Caratti 2021-10-28 12:27 ` Davide Caratti 2021-10-28 13:21 ` Matthieu Baerts 0 siblings, 2 replies; 9+ messages in thread From: Davide Caratti @ 2021-10-28 12:24 UTC (permalink / raw) To: Matthieu Baerts; +Cc: Geliang Tang, MPTCP Upstream 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases 2021-10-28 12:24 ` Davide Caratti @ 2021-10-28 12:27 ` Davide Caratti 2021-10-28 13:21 ` Matthieu Baerts 1 sibling, 0 replies; 9+ messages in thread From: Davide Caratti @ 2021-10-28 12:27 UTC (permalink / raw) To: Matthieu Baerts; +Cc: Geliang Tang, MPTCP Upstream 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases 2021-10-28 12:24 ` Davide Caratti 2021-10-28 12:27 ` Davide Caratti @ 2021-10-28 13:21 ` Matthieu Baerts 2021-10-28 16:46 ` Matthieu Baerts 1 sibling, 1 reply; 9+ messages in thread From: Matthieu Baerts @ 2021-10-28 13:21 UTC (permalink / raw) To: Davide Caratti; +Cc: Geliang Tang, MPTCP Upstream 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases 2021-10-28 13:21 ` Matthieu Baerts @ 2021-10-28 16:46 ` Matthieu Baerts 2021-10-29 4:53 ` Geliang Tang 0 siblings, 1 reply; 9+ messages in thread From: Matthieu Baerts @ 2021-10-28 16:46 UTC (permalink / raw) To: Geliang Tang; +Cc: MPTCP Upstream, Davide Caratti 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases 2021-10-28 16:46 ` Matthieu Baerts @ 2021-10-29 4:53 ` Geliang Tang 0 siblings, 0 replies; 9+ messages in thread From: Geliang Tang @ 2021-10-29 4:53 UTC (permalink / raw) To: Matthieu Baerts; +Cc: Davide Caratti, mptcp 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-10-29 4:53 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-28 3:16 [PATCH mptcp-next v2] selftests: mptcp: add mp_fail testcases Geliang Tang 2021-10-28 9:23 ` Geliang Tang 2021-10-28 11:01 ` Davide Caratti 2021-10-28 11:21 ` Matthieu Baerts 2021-10-28 12:24 ` Davide Caratti 2021-10-28 12:27 ` Davide Caratti 2021-10-28 13:21 ` Matthieu Baerts 2021-10-28 16:46 ` Matthieu Baerts 2021-10-29 4:53 ` Geliang Tang
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.