[mptcp-net] mptcp: relax check on MPC passive fallback

Commit Message

Paolo Abeni Jan. 15, 2024, 2:56 p.m. UTC

While testing the blamed commit below, I was able to miss noticing(!)
packetdrill failures in the fastopen test-cases.

On passive fastopen the child socket is created by incoming TCP MPC syn,
allow for both MPC_SYN and MPC_ACK header.

While at it, additionally allow for real OoO packets, as the RFC
mandates.

Fixes: 724b00c12957 ("mptcp: refine opt_mp_capable determination")
Signed-off-by: Paolo Abeni <[e-mail]>
---
 net/mptcp/subflow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Matthieu Baerts Jan. 15, 2024, 3:52 p.m. UTC | #1

Hi Paolo,

On 15/01/2024 15:56, Paolo Abeni wrote:
> While testing the blamed commit below, I was able to miss noticing(!)
> packetdrill failures in the fastopen test-cases.
> 
> On passive fastopen the child socket is created by incoming TCP MPC syn,
> allow for both MPC_SYN and MPC_ACK header.

Thank you for the fix!

> While at it, additionally allow for real OoO packets, as the RFC
> mandates.

Should this be split into another patch? (it is not a fix for the commit
mentioned below)

Also, the comment just above the code you modified seems to suggest we
should do a fallback if some DSS are detected, not the opposite, no?

  /* (...)
   * Even OoO DSS packets coming legitly after dropped or
   * reordered MPC will cause fallback, but we don't have other
   * options.
   */

Cheers,
Matt

MPTCP CI Jan. 15, 2024, 4:08 p.m. UTC | #2

Hi Paolo,

Thank you for your modifications, that's great!

Our CI (GitHub Action) did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7530884832

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/c8255ee720ba


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

MPTCP CI Jan. 15, 2024, 5:05 p.m. UTC | #3

Hi Paolo,

Thank you for your modifications, that's great!

Our CI (Cirrus) did some validations with a debug kernel and here is its report:

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): packetdrill_sockopts 

Paolo Abeni Jan. 15, 2024, 6:13 p.m. UTC | #4

On Mon, 2024-01-15 at 16:52 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 15/01/2024 15:56, Paolo Abeni wrote:
> > While testing the blamed commit below, I was able to miss noticing(!)
> > packetdrill failures in the fastopen test-cases.
> > 
> > On passive fastopen the child socket is created by incoming TCP MPC syn,
> > allow for both MPC_SYN and MPC_ACK header.
> 
> Thank you for the fix!
> 
> > While at it, additionally allow for real OoO packets, as the RFC
> > mandates.
> 
> Should this be split into another patch? (it is not a fix for the commit
> mentioned below)
> 
> Also, the comment just above the code you modified seems to suggest we
> should do a fallback if some DSS are detected, not the opposite, no?
> 
>   /* (...)
>    * Even OoO DSS packets coming legitly after dropped or
>    * reordered MPC will cause fallback, but we don't have other
>    * options.
>    */

Oops, I forgot to update the comment. 

The point is that we should be able to accept OoO DSS packets, and
switch to fully established when receiving the MPC_ACK, but I agree you
are right, that much of change is dangerous, let's just accept SYN &&
ACK and ev. handle better the thing later

I'll send a v2.

Cheers,

Paolo

Patch

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 1117d1e84274..90984b053adc 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -783,7 +783,8 @@  static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 		 * options.
 		 */
 		mptcp_get_options(skb, &mp_opt);
-		if (!(mp_opt.suboptions & OPTION_MPTCP_MPC_ACK))
+		if (!(mp_opt.suboptions &
+		      (OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_ACK | OPTION_MPTCP_DSS)))
 			fallback = true;
 
 	} else if (subflow_req->mp_join) {