Re: [MPTCP][PATCH mptcp-next] mptcp: don't fallback when data_fin enabled

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

On 07/05/2021 14:19, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 07/05/2021 11:08, Geliang Tang wrote:
>> Hi Matt,
>>
>> Thanks for your review.
>>
>> Matthieu Baerts <matthieu.baerts@tessares.net> 于2021年5月7日周五 下午4:51写道：
>>>
>>> Hi Geliang,
>>>
>>> On 07/05/2021 09:58, Geliang Tang wrote:
>>>> This patch fixed this fallback error:
>>>
>>> Thank you for the patch!
>>>
>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>> index f3e379a0f08d..940b32511443 100644
>>>> --- a/net/mptcp/options.c
>>>> +++ b/net/mptcp/options.c
>>>> @@ -901,7 +901,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>>>>        * MP_JOIN subflows.
>>>>        */
>>>>       if (!mp_opt->mp_capable) {
>>>> -             if (subflow->mp_join)
>>>> +             if (subflow->mp_join || mptcp_data_fin_enabled(msk))>                   goto reset;
>>>
>>> I don't think we should send a reset here. For me this behaviour is normal.
>>
>> Yes, we don't need to send a reset here. What about just return here?
>> Something like this:
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 3428c163299b..bd62dadfefdb 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -905,6 +905,8 @@ static bool check_fully_established(struct
>> mptcp_sock *msk, struct sock *ssk,
>>         if (!mp_opt->mp_capable) {
>>                 if (subflow->mp_join)
>>                         goto reset;
>> +               if (mptcp_data_fin_enabled(msk))
>> +                       return false;
>>                 subflow->mp_capable = 0;
>>                 pr_fallback(msk);
>>                 __mptcp_do_fallback(msk);
> 
> I think we still need to do a fallback here not to expect a DATA_ACK.
> For example in __mptcp_check_send_data_fin(), we change the state
> because we will not get a DATA_ACK.
> 
> So I think everything is OK there.
> 
> Just one thing: should we send a DATA_FIN if we were not "fully
> established" when closing the socket?

Paolo shared this answer on IRC:

  I would say yes. The server side could have already allocated the msk
socket (and the 4th ack could have been lost)

Which makes sense.

So I think we should not do anything here. Except maybe a comment here
in the code? e.g.


diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 3428c163299b..43d55039743a 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -905,6 +905,14 @@ static bool check_fully_established(struct
mptcp_sock *msk, struct sock *ssk,
        if (!mp_opt->mp_capable) {
                if (subflow->mp_join)
                        goto reset;
+
+               /* We also want to fallback if the connection is closed
while we
+                * were not in fully established mode.
+                * But we still want to send the DATA_FIN if the
userspace has
+                * closed the connection at this stage, just in case the
server
+                * side has already allocated the msk socket or if the
4th ACK
+                * has been lost.
+                */
                subflow->mp_capable = 0;
                pr_fallback(msk);
                __mptcp_do_fallback(msk);


Cheers,
Matt

-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net