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

[Geliang Tang <geliang.tang@suse.com>]

Hi Matt, Mat,

On Tue, Feb 08, 2022 at 01:26:10PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 07/02/2022 15:08, Geliang Tang wrote:
> > On Mon, Feb 07, 2022 at 11:32:05AM +0100, Matthieu Baerts wrote:
> >> Hi Geliang,
> >>
> >> On 07/02/2022 05:13, Geliang Tang wrote:
> >>> Hi Matt,
> >>>
> >>> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月29日周六 01:04写道：
> >>>>
> >>>> Hi Mat, Geliang,
> >>>>
> >>>> On 27/01/2022 01:07, Mat Martineau wrote:
> >>>>> On Tue, 25 Jan 2022, Geliang Tang wrote:
> >>>>>
> >>>>>> Added the test cases for MP_FAIL, use 'tc' commands to trigger the
> >>>>>> checksum failures.
> >>>>>>
> >>>>>
> >>>>> This patch is working well with the inverted byte detection, thanks!
> >>>>
> >>>> The public CI seems quite happy with the new version, good work :)
> >>>>
> >>>> Here is its report:
> >>>>
> >>>> ====== 8< ======
> >>>> Our CI did some validations and here is its report:
> >>>>
> >>>> - KVM Validation: normal:
> >>>>   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
> >>>>   - Task: https://cirrus-ci.com/task/4993794666921984
> >>>>   - Summary:
> >>>> https://api.cirrus-ci.com/v1/artifact/task/4993794666921984/summary/summary.txt
> >>>>
> >>>> - KVM Validation: debug:
> >>>>   - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
> >>>>   - Task: https://cirrus-ci.com/task/4682233209421824
> >>>>   - Summary:
> >>>> https://api.cirrus-ci.com/v1/artifact/task/4682233209421824/summary/summary.txt
> >>>>
> >>>> Initiator: Patchew Applier
> >>>> Commits:
> >>>> https://github.com/multipath-tcp/mptcp_net-next/commits/c52e51136372
> >>>> ====== 8< ======
> >>>>
> >>>> But if you look at the logs [1], there are probably 3 last things to
> >>>> improve:
> >>>>
> >>>> [1] https://api.cirrus-ci.com/v1/task/4993794666921984/logs/test.log
> >>>>
> >>>> This line is repeated 90+ times:
> >>>>
> >>>>   tc action pedit offset 162 out of bounds
> >>>>
> >>>> (we talked about this one at the last meeting with ideas on how to
> >>>> reduce/get rid of them)
> >>>>
> >>>
> >>> Dose 'tc + iptables' work for our case?
> >>>
> >>> Use iptables to select the packets with enough data. Then use tc to
> >>> edit the selected packets only. (I don't know how to do it yet.)
> >>>
> >>> Here's a link about this:
> >>>
> >>> https://wiki.archlinux.org/title/advanced_traffic_control#Using_tc_+_iptables
> >>> .
> >>> '''
> >>> Using tc + iptables
> >>>
> >>> Iptables has a method called fwmark that can be used to mark packets
> >>> across interfaces.
> >>>
> >>> First, this makes packets marked with 6, to be processed by the 1:30 class
> >>>
> >>> # tc filter add dev eth0 protocol ip parent 1: prio 1 handle 6 fw flowid 1:30
> >>>
> >>> This sets that mark 6, using iptables
> >>>
> >>> # iptables -A PREROUTING -t mangle -i eth0 -j MARK --set-mark 6
> >>>
> >>> You can then use iptables normally to match packets and then mark them
> >>> with fwmark.
> >>> '''
> >>
> >> Good idea, probably simpler!
> >>
> >> I guess we can mark some packets in POSTROUTING and modify the egress TC
> >> rule.
> >>
> >> Do you need a hand for that?
> >>
> > 
> > Thanks, Matt, I really need your help :)
> 
> Sure.
> May you try the attached patch if you don't mind?

Great! Thanks Matt. I just tested your patch, the single subflow test
("002 1 MP_FAIL, single subflow") works well. And very stable. The two
issues that I mentioned in the cover letter of this series didn't happen
when running this new selftest. So the first two squash-to patches in
this series can be dropped.

Mat, it looks like no need to analyse my attached pcaps any more. Thanks
for your help.

> 
> I now have this output:
> 
> 001 0 MP_FAIL, multiple subflows         syn[ ok ] - synack[ ok ] - ack[
> ok ]
>                                          sum[ ok ] - csum  [ ok ]
>                                          ftx[ ok ] - frx   [ ok ]
>                                          itx[ ok ] - irx   [ ok ]

But the multiple subflows test ("001 0 MP_FAIL, multiple subflows") doesn't
work. I'll try to fix it tomorrow. I think it's easy to fix.

Thanks,
-Geliang

> Created /tmp/tmp.SNhvH7ucj9 (size 512 KB) containing data sent by client
> Created /tmp/tmp.RFEqEXi0bl (size 512 KB) containing data sent by server
> file received by server has inverted byte at 69
> file received by server has inverted byte at 70
> file received by server has inverted byte at 71
> file received by server has inverted byte at 72
> 002 1 MP_FAIL, single subflow            syn[ ok ] - synack[ ok ] - ack[
> ok ]
>                                          sum[ ok ] - csum  [ ok ]
>                                          ftx[ ok ] - frx   [ ok ]
>                                          itx[ ok ] - irx   [ ok ]
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> index 26955abe49f0..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
> @@ -22,5 +25,5 @@ CONFIG_IPV6_MULTIPLE_TABLES=y
>  CONFIG_NET_ACT_CSUM=m
>  CONFIG_NET_ACT_PEDIT=m
>  CONFIG_NET_CLS_ACT=y
> -CONFIG_NET_CLS_FLOWER=m
> +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 a51b0915b7b1..52a0f3df3b2e 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -180,15 +180,12 @@ reset_with_allow_join_id0()
>  # has not been updated, we will detect the modification and emit an
>  # MP_FAIL: what we want to validate here.
>  #
> -# Please note that this rule will produce this pr_info() message for
> -# each TCP ACK packets not carrying enough data:
> +# 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
>  #
> -# But this should be limited to a very few numbers of packets as we
> -# restrict this rule to outgoing TCP traffic with only the ACK flag
> -# + except the 3rd ACK, only packets carrying data should be seen in
> -# this direction.
> +# Netfilter is used to mark packets with enough data.
>  reset_with_fail()
>  {
>  	reset
> @@ -199,12 +196,20 @@ reset_with_fail()
>  	check_invert=1
>  	validate_checksum=1
>  	nr_fail=0
> -	i="$1"
> +	local i="$1"
> +
> +	ip netns exec $ns2 iptables \
> +		-t mangle \
> +		-A OUTPUT \
> +		-p tcp \
> +		-m length --length 150:9999 \
> +		-m statistic --mode nth --packet 0 --every 9999 \
> +		-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 \
> -		flower ip_proto tcp tcp_flags 0x10/0xff \
> +		handle 42 fw \
>  		action pedit munge offset 148 u32 invert \
>  		pipe csum tcp \
>  		index 100
> @@ -296,14 +301,12 @@ link_failure()
>  
>  get_nr_fail()
>  {
> -	i="$1"
> +	local 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')
> -	local overlimits=$(echo $action | jq '.[1].actions[0].stats.overlimits')
>  
> -	let pkt=$packets-$overlimits
> -	if [ $pkt -gt 0 ]; then
> +	if [ $packets -gt 0 ]; then
>  		nr_fail=1
>  	fi
>  	tc -n $ns2 qdisc del dev ns2eth$i clsact