| Message ID | 7cc29abe8da61eeb3e6c7bd5d2d8fe1d51dcdd7e.1606924518.git.pabeni@redhat.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [mptcp-next] mptcp: address potential infinite loop on receive | expand |
On Wed, 2 Dec 2020, Paolo Abeni wrote: > Eric noted the MPTCP recvmsg() loop can iterate multiple > times when the argument lenght is 0 and some subflow is > feeding new data. > > Be sure to quite the loop soon moving the exit condition > earlier. Additionally move a related comment into the > right place. > > Reported-by: Eric Dumazet <eric.dumazet@gmail.com> > Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > Note: looking again at the current code, I think that at worst we > can do a single additional, unneeded iteration - after the first > 'continue' statement __mptcp_move_skbs() will have moved some data > into the receive_queue, and we are not going to receive them. On > next iteration the receive_queue will be not empty. > > Unless the peer is crafting some malicious DSS/packets combo I'm > unable to foreseen. > --- > net/mptcp/protocol.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > To close the loop here on the mailing list (based on IRC, patchwork, and netdev), Paolo has tested Eric's fix for the issue and that will be merged instead: https://patchwork.kernel.org/project/netdevbpf/patch/20201202171657.1185108-1-eric.dumazet@gmail.com/ -- Mat Martineau Intel
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 221f7cdd416b..5a3f4bc38ee3 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1933,21 +1933,21 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, copied += bytes_read; - if (skb_queue_empty(&msk->receive_queue) && - __mptcp_move_skbs(msk, len - copied)) - continue; - /* be sure to advertise window change */ old_space = READ_ONCE(msk->old_wspace); if ((tcp_space(sk) - old_space) >= old_space) mptcp_cleanup_rbuf(msk); - /* only the master socket status is relevant here. The exit - * conditions mirror closely tcp_recvmsg() - */ if (copied >= target) break; + if (skb_queue_empty(&msk->receive_queue) && + __mptcp_move_skbs(msk, len - copied)) + continue; + + /* only the master socket status is relevant here. The exit + * conditions mirror closely tcp_recvmsg() + */ if (copied) { if (sk->sk_err || sk->sk_state == TCP_CLOSE ||
Eric noted the MPTCP recvmsg() loop can iterate multiple times when the argument lenght is 0 and some subflow is feeding new data. Be sure to quite the loop soon moving the exit condition earlier. Additionally move a related comment into the right place. Reported-by: Eric Dumazet <eric.dumazet@gmail.com> Fixes: ea4ca586b16f ("mptcp: refine MPTCP-level ack scheduling") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- Note: looking again at the current code, I think that at worst we can do a single additional, unneeded iteration - after the first 'continue' statement __mptcp_move_skbs() will have moved some data into the receive_queue, and we are not going to receive them. On next iteration the receive_queue will be not empty. Unless the peer is crafting some malicious DSS/packets combo I'm unable to foreseen. --- net/mptcp/protocol.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)