[mptcp-net] mptcp: always parse mptcp options for MPC reqsk

Commit Message

Paolo Abeni May 17, 2021, 10:44 a.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")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/192
iSigned-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, 7 insertions(+), 10 deletions(-)

Comments

Paolo Abeni May 17, 2021, 11:32 a.m. UTC | #1

On Mon, 2021-05-17 at 12:44 +0200, 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")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/192
> iSigned-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

I just note the above is not a complete fix for issues/192.

Specifically we can have:

syn +  MPC ->
          <- syn + ack + MPC ACK
ack + MPC -> [not received by the server due to packet loss]
ack + MPC + data -> [not received by the server due to packet loss]
		    [this cover data _after_ the MPC + data above]
ack + DSS(DSS > 1) -> 

Reading the RFC, I think the client is allowed to sending the DSS even
if it did not received anything back from the server yet.

When the server receives the DSS packet, it does not have yet the peer
key, so it can't send any kind of MPTCP level ack, unless it guesses
the IDSN from the incoming DSS, which looks unsafe.

Am I missing something?

For net-next, I think we can "fix" (or at least hide) the issue,
forcing the client to avoid sending any data after the MPC+data block
some if such block is at least partially acked (at the MPTCP level).

But I think a different client not respect the above, and still trigger
the issue.

Another option would be ignoring (for fallback's sake) initial incoming
sack packet not covering the MPC-data mapped data and lacking MPTCP
level ack.

I'm unsure if that will be "safe enough" and/or will cover correctly
the scenario where the server is sending data (DSS) before correctly
receving the peer's key. In such case, could the dack block be omitted
due to lack of TCP option space ??!

Any comments more than welcome.

Thanks,

Paolo

Paolo Abeni May 17, 2021, 4:43 p.m. UTC | #2

On Mon, 2021-05-17 at 13:32 +0200, Paolo Abeni wrote:
> On Mon, 2021-05-17 at 12:44 +0200, 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")
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/192
> > iSigned-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
> 
> I just note the above is not a complete fix for issues/192.
> 
> Specifically we can have:
> 
> syn +  MPC ->
>           <- syn + ack + MPC ACK
> ack + MPC -> [not received by the server due to packet loss]
> ack + MPC + data -> [not received by the server due to packet loss]
> 		    [this cover data _after_ the MPC + data above]
> ack + DSS(DSS > 1) -> 

At this point the server can only reply with a TCP sack with no MPTCP
ack. Davide noted that the server should as well fallback to TCP.

I'll send a v2 doing the above after some more testing.

Side note: the self-tests end-up failing with reset and timeout because
we currently mis-handle a fallback scenario with a rst.

Another patch needed :(

/P

Patch

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 554e7ccee02a..847e75f1ea77 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -633,21 +633,18 @@  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
+		 * only for in-sequence packet missing it.
+		 */
 		mptcp_get_options(sk, skb, &mp_opt);
-		if (!mp_opt.mp_capable) {
+		if (!mp_opt.mp_capable &&
+		    TCP_SKB_CB(skb)->seq == subflow_req->ssn_offset + 1) {
 			fallback = true;
 			goto create_child;
 		}
 
-create_msk:
 		new_msk = mptcp_sk_clone(listener->conn, &mp_opt, req);
 		if (!new_msk)
 			fallback = true;