[v3,mptcp-net,1/3] mptcp: always parse mptcp options for MPC reqsk

Commit Message

Paolo Abeni May 20, 2021, 2:46 p.m. UTC

In subflow_syn_recv_sock() we currently skip options parsing
for OoO packet, given that such packets may not carry the relevant
MPC option.

If the peer generates an MPC+data TSO packet and some of the early
segments are lost or get reorder, we server will ignore the peer key,
causing transient, unexpected fallback to TCP.

The solution is always parsing the incoming MPTCP options, and
do the fallback only for in-order packets. This actually cleans
the existing code a bit.

Reported-by:  Matthieu Baerts <[e-mail]>
Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option")
Signed-off-by: Paolo Abeni <[e-mail]>
---
a note on data ack len: with this patch the server will use
ack32 for OoO MPC+data pkts, and will move to ack64 ASA will get
the first in order MPC+data pkt.

We can clean-up/make more consistent the behavior with some additional
check in mptcp_sk_clone and/or subflow_syn_recv_sock(), but I prefer
to not introduce only partially related changes here
---
 net/mptcp/subflow.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Comments

Matthieu Baerts May 25, 2021, 10:57 a.m. UTC | #1

Hi Paolo, Mat,

On 20/05/2021 16:46, Paolo Abeni wrote:
> In subflow_syn_recv_sock() we currently skip options parsing
> for OoO packet, given that such packets may not carry the relevant
> MPC option.
> 
> If the peer generates an MPC+data TSO packet and some of the early
> segments are lost or get reorder, we server will ignore the peer key,
> causing transient, unexpected fallback to TCP.
> 
> The solution is always parsing the incoming MPTCP options, and
> do the fallback only for in-order packets. This actually cleans
> the existing code a bit.
> 
> Reported-by:  Matthieu Baerts <[e-mail]>
> Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option")
> Signed-off-by: Paolo Abeni <[e-mail]>

Thank you for the patches and the reviews!

- f407a7aa834a: mptcp: always parse mptcp options for MPC reqsk

- 68379f4d6133: mptcp: do not reset MP_CAPABLE subflow on mapping errors
  - In this commit has been squashed "mptcp: cleanup error path in
subflow_check_data_avail()" commit

- cedf8c99c004: mptcp: update selftest for fallback due to OoO

- f76f266a991f: conflict in t/mptcp-add-sk-parameter-for-mptcp_parse_option

- b10d71891518: conflict in
t/mptcp-cleanup-error-path-in-subflow_check_data_avail

- Results: 9d8cf0870447..3ebd090a8fbc

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210525T105728
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210525T105728

Cheers,
Matt

Patch

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 554e7ccee02a..278986585088 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -633,21 +633,20 @@  static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 
 	/* if the sk is MP_CAPABLE, we try to fetch the client key */
 	if (subflow_req->mp_capable) {
-		if (TCP_SKB_CB(skb)->seq != subflow_req->ssn_offset + 1) {
-			/* here we can receive and accept an in-window,
-			 * out-of-order pkt, which will not carry the MP_CAPABLE
-			 * opt even on mptcp enabled paths
-			 */
-			goto create_msk;
-		}
-
+		/* we can receive and accept an in-window, out-of-order pkt,
+		 * which may not carry the MP_CAPABLE opt even on mptcp enabled
+		 * paths: always try to extract the peer key, and fallback
+		 * for packets missing it.
+		 * Even OoO DSS packets coming legitly after dropped or
+		 * reordered MPC will cause fallback, but we don't have other
+		 * options.
+		 */
 		mptcp_get_options(sk, skb, &mp_opt);
 		if (!mp_opt.mp_capable) {
 			fallback = true;
 			goto create_child;
 		}
 
-create_msk:
 		new_msk = mptcp_sk_clone(listener->conn, &mp_opt, req);
 		if (!new_msk)
 			fallback = true;