[PATCH mptcp-next 0/2] mptcp: subflow removal follow-ups

[PATCH mptcp-next 0/2] mptcp: subflow removal follow-ups

[Paolo Abeni]

A couple of follow-up to the recent refactor.

Patch 1 address the serious issue reported by Christoph/syzkaller.
Patch 2 is just a cleanup.

Anyhow everything is useless untill we fix/revert the tcp induced
mptcp breakage ;)

Paolo Abeni (2):
  Squash-to: "mptcp: avoid unneeded mptcp_token_destroy() call"
  Squash-to: "mptcp: avoid additional __inet_stream_connect() call"

 net/mptcp/protocol.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

-- 
2.41.0

[PATCH mptcp-next 1/2] Squash-to: "mptcp: avoid unneeded mptcp_token_destroy() call"

[Paolo Abeni]

Replace commit message with:

"""
the mptcp protocol currently clears the msk token both at
connect() and listen() time. That is needed to deal with failing
connect() calls that can create a new token while leaving the sk
in TCP_CLOSE,SS_UNCONNECTED status and thus allowing later connect()
and/or listen() calls.

Let's deal with such failures explcitly, cleaning the token in
a timely manner and avoid the confusing early mptcp_token_destroy()
"""

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index c6cd817ee103..9e48dfbd102f 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3656,6 +3656,8 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	 * subflow_finish_connect()
 	 */
 	if (unlikely(err && err != -EINPROGRESS)) {
+		/* avoid leaving a dangling token in an unconnected socket */
+		mptcp_token_destroy(msk);
 		inet_sk_state_store(sk, inet_sk_state_load(ssk));
 		return err;
 	}
-- 
2.41.0

[PATCH mptcp-next 2/2] Squash-to: "mptcp: avoid additional __inet_stream_connect() call"

[Paolo Abeni]

In the mentioned commit I unintenionally left over a couple of
possible cleanup.

Add the following to the commit message, befere "No functional change
intended.":

"""
The sk-level connect never return -EINPROGRESS, cleanup the error
path accordingly. Additionally, the ssk status on error is always
TCP_CLOSE. Avoid unneeded access to the subflow sk state.
"""

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 9e48dfbd102f..067e90168a49 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3655,18 +3655,14 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	/* on successful connect, the msk state will be moved to established by
 	 * subflow_finish_connect()
 	 */
-	if (unlikely(err && err != -EINPROGRESS)) {
+	if (unlikely(err)) {
 		/* avoid leaving a dangling token in an unconnected socket */
 		mptcp_token_destroy(msk);
-		inet_sk_state_store(sk, inet_sk_state_load(ssk));
+		inet_sk_state_store(sk, TCP_CLOSE);
 		return err;
 	}
 
 	mptcp_copy_inaddrs(sk, ssk);
-
-	/* silence EINPROGRESS and let the caller inet_stream_connect
-	 * handle the connection in progress
-	 */
 	return 0;
 }
 
-- 
2.41.0

Re: Squash-to: "mptcp: avoid additional __inet_stream_connect() call": Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_connect - Critical: Global Timeout ❌:
  - Task: https://cirrus-ci.com/task/5012928097681408
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5012928097681408/summary/summary.txt

- {"code":404,"message":
  - "Can't find artifacts containing file conclusion.txt"}:
  - Task: https://cirrus-ci.com/task/6138828004524032
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6138828004524032/summary/summary.txt

- {"code":404,"message":
  - "Can't find artifacts containing file conclusion.txt"}:
  - Task: https://cirrus-ci.com/task/6701777957945344
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6701777957945344/summary/summary.txt

- {"code":404,"message":
  - "Can't find artifacts containing file conclusion.txt"}:
  - Task: https://cirrus-ci.com/task/5575878051102720
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5575878051102720/summary/summary.txt

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


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-debug

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 (Tessares)

Re: Squash-to: "mptcp: avoid additional __inet_stream_connect() call": Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6269486597144576
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6269486597144576/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5990205023322112
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5990205023322112/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): packetdrill_sockopts 🔴:
  - Task: https://cirrus-ci.com/task/5427255069900800
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5427255069900800/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6553154976743424
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6553154976743424/summary/summary.txt

Initiator: Matthieu Baerts
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/421d0a90e2b8


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-debug

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 (Tessares)

Re: [PATCH mptcp-next 0/2] mptcp: subflow removal follow-ups

[Matthieu Baerts]

Hi Paolo, Christoph,

On 20/07/2023 08:34, Paolo Abeni wrote:
> A couple of follow-up to the recent refactor.
> 
> Patch 1 address the serious issue reported by Christoph/syzkaller.
> Patch 2 is just a cleanup.

@Paolo: Thank you for the fixes!

@Christoph: happy testing :-) (thank you!)
Do we already close some of the new tickets that have been opened two
days ago? (421+)

> Anyhow everything is useless untill we fix/revert the tcp induced
> mptcp breakage ;)

This has been fixed so I can apply these two patches :)

Now in our tree (feat. for net-next):

- 9fea9fa43aca: "squashed" (with conflicts) patch 1/2 in "mptcp: avoid
unneeded mptcp_token_destroy() calls"
- 1a98012d684c: tg:msg: replace commit msg as requested by Paolo
- cf229e670d26: conflict in
t/mptcp-avoid-additional-__inet_stream_connect-call

- 8c64a5490e5e: "squashed" patch 2/2 in "mptcp: avoid additional
__inet_stream_connect() call"
- 04f57dcde15e: tg:msg: adapt commit msg as requested by Paolo
- Results: 6af60b285fa0..528b797b3425 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230720T190202

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net