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

[Matthieu Baerts <matthieu.baerts@tessares.net>]

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