Re: [PATCH net-next v2] mptcp: be careful on MPTCP-level ack.

[Eric Dumazet <eric.dumazet@gmail.com>]

On 12/2/20 5:32 PM, Eric Dumazet wrote:
> 
> 
> On 12/2/20 5:30 PM, Eric Dumazet wrote:
>>
>>
>> On 12/2/20 5:10 PM, Eric Dumazet wrote:
>>>
>>>
>>> On 12/2/20 4:37 PM, Paolo Abeni wrote:
>>>> On Wed, 2020-12-02 at 14:18 +0100, Eric Dumazet wrote:
>>>>>
>>>>> On 11/24/20 10:51 PM, Paolo Abeni wrote:
>>>>>> We can enter the main mptcp_recvmsg() loop even when
>>>>>> no subflows are connected. As note by Eric, that would
>>>>>> result in a divide by zero oops on ack generation.
>>>>>>
>>>>>> Address the issue by checking the subflow status before
>>>>>> sending the ack.
>>>>>>
>>>>>> Additionally protect mptcp_recvmsg() against invocation
>>>>>> with weird socket states.
>>>>>>
>>>>>> v1 -> v2:
>>>>>>  - removed unneeded inline keyword - Jakub
>>>>>>
>>>>>> Reported-and-suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
>>>>>> Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
>>>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>>>>> ---
>>>>>>  net/mptcp/protocol.c | 67 ++++++++++++++++++++++++++++++++------------
>>>>>>  1 file changed, 49 insertions(+), 18 deletions(-)
>>>>>>
>>>>>
>>>>> Looking at mptcp recvmsg(), it seems that a read(fd, ..., 0) will
>>>>> trigger an infinite loop if there is available data in receive queue ?
>>>>
>>>> Thank you for looking into this!
>>>>
>>>> I can't reproduce the issue with the following packetdrill ?!?
>>>>
>>>> +0.0  connect(3, ..., ...) = -1 EINPROGRESS (Operation now in progress)
>>>> +0.1   > S 0:0(0) <mss 1460,sackOK,TS val 100 ecr 0,nop,wscale 8,mpcapable v1 fflags[flag_h] nokey>
>>>> +0.1   < S. 0:0(0) ack 1 win 65535 <mss 1460,sackOK,TS val 700 ecr 100,nop,wscaale 8,mpcapable v1 flags[flag_h] key[skey=2] >
>>>> +0.1  > . 1:1(0) ack 1 <nop, nop, TS val 100 ecr 700,mpcapable v1 flags[flag_h]] key[ckey,skey]>
>>>> +0.1 fcntl(3, F_SETFL, O_RDWR) = 0
>>>> +0.1   < .  1:201(200) ack 1 win 225 <dss dack8=1 dsn8=1 ssn=1 dll=200 nocs,  nop, nop>
>>>> +0.1   > .  1:1(0) ack 201 <nop, nop, TS val 100 ecr 700, dss dack8=201 dll=00 nocs>
>>>> +0.1 read(3, ..., 0) = 0
>>>>
>>>> The main recvmsg() loop is interrupted by the following check:
>>>>
>>>>                 if (copied >= target)
>>>>                         break;
>>>
>>> @copied should be 0, and @target should be 1
>>>
>>> Are you sure the above condition is triggering ?
>>>
>>> Maybe read(fd, ..., 0) does not reach recvmsg() at all.
>>
>> Yes, sock_read_iter() has a shortcut :
>>
>> if (!iov_iter_count(to))    /* Match SYS5 behaviour */
>>      res = sock_recvmsg(sock, &msg, msg.msg_flags);
> 
> No idea what went wrong with my copy/paste.
> 
> The real code is more like :
> 
> if (!iov_iter_count(to))    /* Match SYS5 behaviour */
>     return 0;
>

Packetdrill recvmsg syntax would be something like

   +0	recvmsg(3, {msg_name(...)=...,
		    msg_iov(1)=[{..., 0}],
		    msg_flags=0
		    }, 0) = 0