[PATCH mptcp-next 2/2] mptcp: never re-inject data on a single subflow

Paolo Abeni posted 2 patches 3 years, 3 months ago
Maintainers: Mat Martineau <mathew.j.martineau@linux.intel.com>, Matthieu Baerts <matthieu.baerts@tessares.net>, Paolo Abeni <pabeni@redhat.com>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>
[PATCH mptcp-next 2/2] mptcp: never re-inject data on a single subflow
Posted by Paolo Abeni 3 years, 3 months ago
If the MPTCP-level data ack is delayed WRT to the TCP-level ack,
there is a single subflow and the user-space stops pushing data,
the MPTCP-level retransmit may trigger in, fooling (disallowing)
infinite mapping fallback.

All the above is triggered quite reliably by the self-tests, once
we move the MPTCP receive path under the msk socket lock - delaying
the MPTCP-level acks.

Plain TCP is good enough to handle retransmissions as needed.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
Note that there are a couple of sad untold stories here. The first one
is as follow: with the delayed mptcp-level ack, at MPC handshake time we have:

MPC + SYN ->
	<- MPC + SYN_ACK(1)
MPC + ACK(1) ->
	<- DSS(n bytes) + ACK(1)
DSS_ACK(1) + ACK(n+1)

and no more acks from the client side, even after reading the
incoming n bytes data. So a possible alternative solution would be
to tweek mptcp_cleanup_rbuf() to properly handle this scenario.

Additionally, possibly, still due to the mptcp-level delyated ack,
we could need to tweek mptcp_cleanup_rbuf() more - e.g. do we need
to force mptcp-level ack when there is a large enough amount of newly
"ackable" data due to __mptcp_move_skbs() action? I don't know.

I guess/hope that the condition on mptcp-level window update already
implemented in mptcp_cleanup_rbuf() should also cover for the above,
but I'm not sure.

The other point is that I can't recall the previous discussion about
avoiding re-injection with a single subflow. I now think that is bad,
at least with delayed mptcp-level ack, and I don't see a need for that,
but possibly/likely I'm forgetting something?!?
---
 net/mptcp/sched.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index 044c5ec8bbfb..409e832085c2 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -162,6 +162,10 @@ struct sock *mptcp_sched_get_retrans(struct mptcp_sock *msk)
 	if (__mptcp_check_fallback(msk))
 		return NULL;
 
+	/* never retransmit when there is a single subflow */
+	if (list_is_singular(&msk->conn_list) && list_empty(&msk->join_list))
+		return NULL;
+
 	if (!msk->sched)
 		return mptcp_subflow_get_retrans(msk);
 
-- 
2.37.1