Re: [PATCH v5 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process

[Yonglong Li <liyonglong@chinatelecom.cn>]

On 2021/6/25 8:28, Mat Martineau wrote:
> On Tue, 22 Jun 2021, Yonglong Li wrote:
> 
>> fix issue: ADD_ADDR and RM_ADDR use pm.add_signal to mark event, so
>> in some case pm.add_signal will be flush when ADD_ADDR/RM_ADDR in
>> process.
>>
>> fix issue: if ADD_ADDR and ADD_ADDR-echo process at the same time,
>> only one event can write pm.add_signal. so ADD_ADDR will process
>> after add_timer timeout or ADD_ADDR-echo will not be process.
>>
>> Patch 1 fix ADD_ADDR and RM_ADDR maybe clear addr_signal each other.
>>
>> Patch 2 and 3 deal ADD_ADDR and ADD_ADDR-echo with separately to fix
>> conflicts in using pm.addr_signal porcess.
>>
>> Patch 4 MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT is not necessary.
>>
>> v1->v2:
>> - remove READ_ONCE under the pm spin lock.
>>
>> v2->v3:
>> - Patch 2: rename mptcp_pm_should_add_addr to mptcp_pm_should_add_signal_addr
>> - Patch 3: avoid read-modify-write of msk->pm.addr_signal and change
>>   mptcp_pm_add_addr_signal to return void.
>>
>> v3->v4:
>> - Patch 1: use ~(BIT(MPTCP_ADD_ADDR_SIGNAL) | BIT(MPTCP_ADD_ADDR_ECHO)) instead
>>   of BIT(MPTCP_RM_ADDR_SIGNAL)
>> - Patch 3: simple the code; init flags; fix wrong goto logic code;
>>
>> v4->v5:
>> - Patch 3: simple the code of function 'mptcp_established_options_add_addr'
> 
> Hi Yonglong -
> 
> Thank you for all of your work on this patch series! I think it is close to being ready, in terms of code review I don't have any changes to suggest.
> 
> When I run the ADD_ADDR timeout test cases (mptcp_join.sh -t), I'm seeing test case failures on every run because fewer ADD_ADDRs were received than were expected.
> 
> Here's one run:
> 
> """
> [mjmartin@mjmartin-nucvm01 mptcp]$ sudo ./mptcp_join.sh -t -c
> Created /tmp/tmp.SoSi2pHquT (size 1 KB) containing data sent by client
> Created /tmp/tmp.CGf8pIwMGf (size 1 KB) containing data sent by server
> Capturing traffic for test 1 into mp_join-01-ns1-0-kHO27s.pcap
> tcpdump: data link type LINUX_SLL2
> dropped privs to mjmartin
> tcpdump: listening on any, link-type LINUX_SLL2 (Linux cooked v2), snapshot length 65535 bytes
> 149 packets captured
> 149 packets received by filter
> 0 packets dropped by kernel
> 01 signal address, ADD_ADDR timeout     syn[ ok ] - synack[ ok ] - ack[ ok ]
>                                        add[ ok ] - echo  [ ok ]
> Capturing traffic for test 2 into mp_join-02-ns1-0-MXTUAg.pcap
> tcpdump: data link type LINUX_SLL2
> dropped privs to mjmartin
> tcpdump: listening on any, link-type LINUX_SLL2 (Linux cooked v2), snapshot length 65535 bytes
> 150 packets captured
> 150 packets received by filter
> 0 packets dropped by kernel
> 02 signal address, ADD_ADDR6 timeout    syn[ ok ] - synack[ ok ] - ack[ ok ]
>                                        add[fail] got 2 ADD_ADDR[s] expected 4
> - echo  [ ok ]
> Server ns stats
> MPTcpExtMPCapableSYNRX          1                  0.0
> MPTcpExtMPCapableACKRX          1                  0.0
> MPTcpExtMPJoinSynRx             1                  0.0
> MPTcpExtMPJoinAckRx             1                  0.0
> Client ns stats
> MPTcpExtMPCapableSYNTX          1                  0.0
> MPTcpExtMPCapableSYNACKRX       1                  0.0
> MPTcpExtMPJoinSynAckRx          1                  0.0
> MPTcpExtAddAddr                 2                  0.0
> Capturing traffic for test 3 into mp_join-03-ns1-0-J4cBbN.pcap
> tcpdump: data link type LINUX_SLL2
> dropped privs to mjmartin
> tcpdump: listening on any, link-type LINUX_SLL2 (Linux cooked v2), snapshot length 65535 bytes
> 432 packets captured
> 432 packets received by filter
> 0 packets dropped by kernel
> 03 signal addresses, ADD_ADDR timeout   syn[ ok ] - synack[ ok ] - ack[ ok ]
>                                        add[ ok ] - echo  [ ok ]
> Capturing traffic for test 4 into mp_join-04-ns1-0-0DJYdc.pcap
> tcpdump: data link type LINUX_SLL2
> dropped privs to mjmartin
> tcpdump: listening on any, link-type LINUX_SLL2 (Linux cooked v2), snapshot length 65535 bytes
> 415 packets captured
> 415 packets received by filter
> 0 packets dropped by kernel
> 04 invalid address, ADD_ADDR timeout    syn[ ok ] - synack[ ok ] - ack[ ok ]
>                                        add[fail] got 7 ADD_ADDR[s] expected 8
> - echo  [ ok ]
> Server ns stats
> MPTcpExtMPCapableSYNRX          1                  0.0
> MPTcpExtMPCapableACKRX          1                  0.0
> MPTcpExtMPJoinSynRx             1                  0.0
> MPTcpExtMPJoinAckRx             1                  0.0
> Client ns stats
> MPTcpExtMPCapableSYNTX          1                  0.0
> MPTcpExtMPCapableSYNACKRX       1                  0.0
> MPTcpExtMPJoinSynAckRx          1                  0.0
> MPTcpExtAddAddr                 7                  0.0
> """
> 
> If you run the tests with -c (mptcp_join.sh -t -c) you can review the .pcap files to see if the new behavior is expected.
> 
> Please see if you can replicate the test results above (I've seen different combinations of these tests succeed and fail). If I'm not the only person seeing the failures, either the tests or the code need to be updated.
> 
> 
> Best regards,
> 
> Mat
> 

Thanks for your patience and suggestions.
I will check the test cases.

> 
> 
>>
>> Yonglong Li (4):
>>  mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other
>>  mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate
>>  mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal
>>  mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT
>>
>> include/net/mptcp.h    |   1 +
>> net/mptcp/options.c    | 161 ++++++++++++++++++++++++++++++++-----------------
>> net/mptcp/pm.c         |  53 +++++++---------
>> net/mptcp/pm_netlink.c |  10 ++-
>> net/mptcp/protocol.h   |  31 ++++------
>> 5 files changed, 147 insertions(+), 109 deletions(-)
>>
>> -- 
>> 1.8.3.1
>>
>>
> 
> -- 
> Mat Martineau
> Intel
> 

-- 
Li YongLong