[PATCH net-next] mptcp: fix infinite loop on recvmsg()/worker() race.

[PATCH net-next] mptcp: fix infinite loop on recvmsg()/worker() race.

[Paolo Abeni]

If recvmsg() and the workqueue race to dequeue the data
pending on some subflow, the current mapping for such
subflow covers several skbs and some of them have not
reached yet the received, either the worker or recvmsg()
can find a subflow with the data_avail flag set - since
the current mapping is valid and in sequence - but no
skbs in the receive queue - since the other entity just
processed them.

The above will lead to an unbounded loop in __mptcp_move_skbs()
and a subsequent hang of any task trying to acquiring the msk
socket lock.

This change addresses the issue stopping the __mptcp_move_skbs()
loop as soon as we detect the above race (empty receive queue
with data_avail set).

Reported-and-tested-by: syzbot+fcf8ca5817d6e92c6567@syzkaller.appspotmail.com
Fixes: ab174ad8ef76 ("mptcp: move ooo skbs into msk out of order queue.")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f483eab0081a..42928db28351 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -471,8 +471,15 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
 				mptcp_subflow_get_map_offset(subflow);
 
 		skb = skb_peek(&ssk->sk_receive_queue);
-		if (!skb)
+		if (!skb) {
+			/* if no data is found, a racing workqueue/recvmsg
+			 * already processed the new data, stop here or we
+			 * can enter an infinite loop
+			 */
+			if (!moved)
+				done = true;
 			break;
+		}
 
 		if (__mptcp_check_fallback(msk)) {
 			/* if we are running under the workqueue, TCP could have
-- 
2.26.2

Re: [PATCH net-next] mptcp: fix infinite loop on recvmsg()/worker() race.

[Mat Martineau]

On Tue, 6 Oct 2020, Paolo Abeni wrote:

> If recvmsg() and the workqueue race to dequeue the data
> pending on some subflow, the current mapping for such
> subflow covers several skbs and some of them have not
> reached yet the received, either the worker or recvmsg()
> can find a subflow with the data_avail flag set - since
> the current mapping is valid and in sequence - but no
> skbs in the receive queue - since the other entity just
> processed them.
>
> The above will lead to an unbounded loop in __mptcp_move_skbs()
> and a subsequent hang of any task trying to acquiring the msk
> socket lock.
>
> This change addresses the issue stopping the __mptcp_move_skbs()
> loop as soon as we detect the above race (empty receive queue
> with data_avail set).
>
> Reported-and-tested-by: syzbot+fcf8ca5817d6e92c6567@syzkaller.appspotmail.com
> Fixes: ab174ad8ef76 ("mptcp: move ooo skbs into msk out of order queue.")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

--
Mat Martineau
Intel

Re: [PATCH net-next] mptcp: fix infinite loop on recvmsg()/worker() race.

[Jakub Kicinski]

On Tue,  6 Oct 2020 08:27:34 +0200 Paolo Abeni wrote:
> If recvmsg() and the workqueue race to dequeue the data
> pending on some subflow, the current mapping for such
> subflow covers several skbs and some of them have not
> reached yet the received, either the worker or recvmsg()
> can find a subflow with the data_avail flag set - since
> the current mapping is valid and in sequence - but no
> skbs in the receive queue - since the other entity just
> processed them.
> 
> The above will lead to an unbounded loop in __mptcp_move_skbs()
> and a subsequent hang of any task trying to acquiring the msk
> socket lock.
> 
> This change addresses the issue stopping the __mptcp_move_skbs()
> loop as soon as we detect the above race (empty receive queue
> with data_avail set).

Applied, thanks!