| Message ID | 024b61f2b46bb7f8d02db1dbdd1c822859b78674.1621248230.git.pabeni@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [mptcp-net] mptcp: always parse mptcp options for MPC reqsk | expand |
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 <matthieu.baerts@tessares.net> > Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option") > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/192 > iSigned-off-by: Paolo Abeni <pabeni@redhat.com> > --- > 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
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 <matthieu.baerts@tessares.net> > > Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option") > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/192 > > iSigned-off-by: Paolo Abeni <pabeni@redhat.com> > > --- > > 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
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;
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 <matthieu.baerts@tessares.net> Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option") Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/192 iSigned-off-by: Paolo Abeni <pabeni@redhat.com> --- 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(-)