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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ca50540b-f305-7519-6039-f3beced5e5d8@gmail.com>
Date:   Wed, 2 Dec 2020 17:32:04 +0100
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:     Jakub Kicinski <kuba@...nel.org>, mptcp@...ts.01.org
Subject: Re: [PATCH net-next v2] mptcp: be careful on MPTCP-level ack.



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@...il.com>
>>>>> Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling")
>>>>> Signed-off-by: Paolo Abeni <pabeni@...hat.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;


> 
> but recvmsg() does not have such check, or maybe I have not looked at the right place.
> 
>>
>> You could try recvmsg() or recvmmsg(), 
>>
>>>
>>> I guess we could loop while the msk has available rcv space and some
>>> subflow is feeding new data. If so, I think moving:
>>>
>>> 	if (skb_queue_empty(&msk->receive_queue) &&
>>>                     __mptcp_move_skbs(msk, len - copied))
>>>                         continue;
>>>
>>> after the above check should address the issue, and will make the
>>> common case faster. Let me test the above - unless I underlooked
>>> something relevant!
>>>
>>> Thanks,
>>>
>>> Paolo
>>>