[PATCH mptcp-net v2 0/5] selftests: mptcp: avoid spurious errors on TCP disconnect

[PATCH mptcp-net v2 0/5] selftests: mptcp: avoid spurious errors on TCP disconnect

[Matthieu Baerts (NGI0)]

This should fix the recent instabilities seen by MPTCP and NIPA CIs
where the 'mptcp_connect.sh' tests fail regularly when running the
'disconnect' subtests with "plain" TCP sockets, e.g.

  # INFO: disconnect
  # 63 ns1 MPTCP -> ns1 (10.0.1.1:20001      ) MPTCP     (duration   996ms) [ OK ]
  # 64 ns1 MPTCP -> ns1 (10.0.1.1:20002      ) TCP       (duration   851ms) [ OK ]
  # 65 ns1 TCP   -> ns1 (10.0.1.1:20003      ) MPTCP     Unexpected revents: POLLERR/POLLNVAL(19)
  # (duration   896ms) [FAIL] file received by server does not match (in, out):
  # -rw-r--r-- 1 root root 11112852 Aug 19 09:16 /tmp/tmp.hlJe5DoMoq.disconnect
  # Trailing bytes are:
  # /{ga 6@=#.8:-rw------- 1 root root 10085368 Aug 19 09:16 /tmp/tmp.blClunilxx
  # Trailing bytes are:
  # /{ga 6@=#.8:66 ns1 MPTCP -> ns1 (dead:beef:1::1:20004) MPTCP     (duration   987ms) [ OK ]
  # 67 ns1 MPTCP -> ns1 (dead:beef:1::1:20005) TCP       (duration   911ms) [ OK ]
  # 68 ns1 TCP   -> ns1 (dead:beef:1::1:20006) MPTCP     (duration   980ms) [ OK ]
  # [FAIL] Tests of the full disconnection have failed

Patch 3 fixes this issue, but a fix in MPTCP in patch 1 is also needed
to fix MPTCP behaviour, and simplify patch 3. Patches 2 and 4 improve
some errors reported by the selftests, and patch 5 helps with the
debugging. I guess all these patches can be sent to 'net'

Note: Patch 1 will cause errors in Packetdrill, that's normal, see [1].

Link: https://github.com/multipath-tcp/packetdrill/pull/171 [1]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v2:
- Patch 1: new
- Patch 3: no more state filtering needed thanks to patch 1
- Patch 4: switch from hexdump to od (Mat)
- Patch 5: new
- Link to v1: https://lore.kernel.org/r/20250819-sft-mptcp-disc-err-v1-0-9d0cf296bc13@kernel.org

---
Matthieu Baerts (NGI0) (5):
      mptcp: propagate shutdown to subflows when possible
      selftests: mptcp: connect: catch IO errors on listen side
      selftests: mptcp: avoid spurious errors on TCP disconnect
      selftests: mptcp: print trailing bytes with od
      selftests: mptcp: connect: print pcap suffix

 net/mptcp/protocol.c                               | 16 ++++++++++++++++
 tools/testing/selftests/net/mptcp/mptcp_connect.c  | 10 +++++++---
 tools/testing/selftests/net/mptcp/mptcp_connect.sh |  6 +++++-
 tools/testing/selftests/net/mptcp/mptcp_lib.sh     |  2 +-
 4 files changed, 29 insertions(+), 5 deletions(-)
---
base-commit: 9335a0684b0967cc704105e953f2a9d893fbcf57
change-id: 20250806-sft-mptcp-disc-err-3357b769bcdb

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-net v2 1/5] mptcp: propagate shutdown to subflows when possible

[Matthieu Baerts (NGI0)]

When the MPTCP DATA FIN have been ACKed, there is no more MPTCP related
metadata to exchange, and all subflows can be safely shutdown.

Before this patch, the subflows were actually terminated at 'close()'
time. That's certainly fine most of the time, but not when the userspace
'shutdown()' a connection, without close()ing it. When doing so, the
subflows were staying in LAST_ACK state on one side -- and consequently
in FIN_WAIT2 on the other side -- until the 'close()' of the MPTCP
socket.

Now, when the DATA FIN have been ACKed, all subflows are shutdown. A
consequence of this is that the TCP 'FIN' flag can be set earlier now,
but the end result is the same. This affects the packetdrill tests
looking at the end of the MPTCP connections, but for a good reason.

Fixes: 3721b9b64676 ("mptcp: Track received DATA_FIN sequence number and add related helpers")
Fixes: 16a9a9da1723 ("mptcp: Add helper to process acks of DATA_FIN")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 1c26acf6c4145896ecbb5c7f7004121c66a20649..9feb9f437c2bdfff392249e7ad00edd09ba67ac3 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -372,6 +372,20 @@ static void mptcp_close_wake_up(struct sock *sk)
 		sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
 }
 
+static void mptcp_shutdown_subflows(struct mptcp_sock *msk)
+{
+	struct mptcp_subflow_context *subflow;
+
+	mptcp_for_each_subflow(msk, subflow) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+		bool slow;
+
+		slow = lock_sock_fast(ssk);
+		tcp_shutdown(ssk, SEND_SHUTDOWN);
+		unlock_sock_fast(ssk, slow);
+	}
+}
+
 /* called under the msk socket lock */
 static bool mptcp_pending_data_fin_ack(struct sock *sk)
 {
@@ -396,6 +410,7 @@ static void mptcp_check_data_fin_ack(struct sock *sk)
 			break;
 		case TCP_CLOSING:
 		case TCP_LAST_ACK:
+			mptcp_shutdown_subflows(msk);
 			mptcp_set_state(sk, TCP_CLOSE);
 			break;
 		}
@@ -564,6 +579,7 @@ static bool mptcp_check_data_fin(struct sock *sk)
 			mptcp_set_state(sk, TCP_CLOSING);
 			break;
 		case TCP_FIN_WAIT2:
+			mptcp_shutdown_subflows(msk);
 			mptcp_set_state(sk, TCP_CLOSE);
 			break;
 		default:

-- 
2.51.0

[PATCH mptcp-net v2 2/5] selftests: mptcp: connect: catch IO errors on listen side

[Matthieu Baerts (NGI0)]

IO errors were correctly printed to stderr, and propagated up to the
main loop for the server side, but the returned value was ignored. As a
consequence, the program for the listener side was no longer exiting
with an error code in case of IO issues.

Because of that, some issues might not have been seen. But very likely,
most issues either had an effect on the client side, or the file
transfer was not the expected one, e.g. the connection got reset before
the end. Still, it is better to fix this.

The main consequence of this issue is the error that was reported by the
selftests: the received and sent files were different, and the MIB
counters were not printed. Also, when such errors happened during the
'disconnect' tests, the program tried to continue until the timeout.

Now when an IO error is detected, the program exits directly with an
error.

Fixes: 05be5e273c84 ("selftests: mptcp: add disconnect tests")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 4f07ac9fa207cb08a934582b98d688d0b9512f97..c1586a7286123a509495ac319fd624b98f0bfd18 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -1112,6 +1112,8 @@ int main_loop_s(int listensock)
 	salen = sizeof(ss);
 	remotesock = accept(listensock, (struct sockaddr *)&ss, &salen);
 	if (remotesock >= 0) {
+		int err;
+
 		maybe_close(listensock);
 		check_sockaddr(pf, &ss, salen);
 		check_getpeername(remotesock, &ss, salen);
@@ -1125,7 +1127,9 @@ int main_loop_s(int listensock)
 		SOCK_TEST_TCPULP(remotesock, 0);
 
 		memset(&winfo, 0, sizeof(winfo));
-		copyfd_io(fd, remotesock, 1, true, &winfo);
+		err = copyfd_io(fd, remotesock, 1, true, &winfo);
+		if (err)
+			return err;
 	} else {
 		perror("accept");
 		return 1;

-- 
2.51.0

[PATCH mptcp-net v2 3/5] selftests: mptcp: avoid spurious errors on TCP disconnect

[Matthieu Baerts (NGI0)]

The disconnect test-case, with 'plain' TCP sockets generates spurious
errors, e.g.

  07 ns1 TCP   -> ns1 (dead:beef:1::1:10006) MPTCP
  read: Connection reset by peer
  read: Connection reset by peer
  (duration   155ms) [FAIL] client exit code 3, server 3

  netns ns1-FloSdv (listener) socket stat for 10006:
  TcpActiveOpens                  2                  0.0
  TcpPassiveOpens                 2                  0.0
  TcpEstabResets                  2                  0.0
  TcpInSegs                       274                0.0
  TcpOutSegs                      276                0.0
  TcpOutRsts                      3                  0.0
  TcpExtPruneCalled               2                  0.0
  TcpExtRcvPruned                 1                  0.0
  TcpExtTCPPureAcks               104                0.0
  TcpExtTCPRcvCollapsed           2                  0.0
  TcpExtTCPBacklogCoalesce        42                 0.0
  TcpExtTCPRcvCoalesce            43                 0.0
  TcpExtTCPChallengeACK           1                  0.0
  TcpExtTCPFromZeroWindowAdv      42                 0.0
  TcpExtTCPToZeroWindowAdv        41                 0.0
  TcpExtTCPWantZeroWindowAdv      13                 0.0
  TcpExtTCPOrigDataSent           164                0.0
  TcpExtTCPDelivered              165                0.0
  TcpExtTCPRcvQDrop               1                  0.0

In the failing scenarios (TCP -> MPTCP), the involved sockets are
actually plain TCP ones, as fallbacks for passive sockets at 2WHS time
cause the MPTCP listeners to actually create 'plain' TCP sockets.

Similar to commit 218cc166321f ("selftests: mptcp: avoid spurious errors
on disconnect"), the root cause is in the user-space bits: the test
program tries to disconnect as soon as all the pending data has been
spooled, generating an RST. If such option reaches the peer before the
connection has reached the closed status, the TCP socket will report an
error to the user-space, as per protocol specification, causing the
above failure. Note that it looks like this issue got more visible since
the "tcp: receiver changes" series from commit 06baf9bfa6ca ("Merge
branch 'tcp-receiver-changes'").

Address the issue by explicitly waiting for the TCP sockets (-t) to
reach a closed status before performing the disconnect. More precisely,
the test program now waits for plain TCP sockets or TCP subflows in
addition to the MPTCP sockets that were already monitored.

While at it, use 'ss' with '-n' to avoid resolving service names, which
is not needed here.

Fixes: 218cc166321f ("selftests: mptcp: avoid spurious errors on disconnect")
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
- v2: no more state filtering needed, thanks to the fix in the kernel
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index c1586a7286123a509495ac319fd624b98f0bfd18..130f6348ce042514a156bf63c6f8cd0fe1d7be4a 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -1251,7 +1251,7 @@ void xdisconnect(int fd)
 	else
 		xerror("bad family");
 
-	strcpy(cmd, "ss -M | grep -q ");
+	strcpy(cmd, "ss -Mnt | grep -q ");
 	cmdlen = strlen(cmd);
 	if (!inet_ntop(addr.ss_family, raw_addr, &cmd[cmdlen],
 		       sizeof(cmd) - cmdlen))
@@ -1261,7 +1261,7 @@ void xdisconnect(int fd)
 
 	/*
 	 * wait until the pending data is completely flushed and all
-	 * the MPTCP sockets reached the closed status.
+	 * the sockets reached the closed status.
 	 * disconnect will bypass/ignore/drop any pending data.
 	 */
 	for (i = 0; ; i += msec_sleep) {

-- 
2.51.0

[PATCH mptcp-net v2 4/5] selftests: mptcp: print trailing bytes with od

[Matthieu Baerts (NGI0)]

This is better than printing random bytes in the terminal.

Note Jakub suggested 'hexdump', but Mat found out that this tool is not
often installed by default. 'od' can do a similar job, and it is in the
POSIX specs and available in coreutils, so it should be on more systems.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Suggested-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
v2: switch to 'od', and no more check for its presence (Mat)
---
 tools/testing/selftests/net/mptcp/mptcp_lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 09cd24b2ae466205dacbdf8289eb86c08534c475..d62e653d48b0f2ef7a01e289fa0be8907825667d 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -384,7 +384,7 @@ mptcp_lib_make_file() {
 mptcp_lib_print_file_err() {
 	ls -l "${1}" 1>&2
 	echo "Trailing bytes are: "
-	tail -c 27 "${1}"
+	tail -c 32 "${1}" | od -x | head -n2
 }
 
 # $1: input file ; $2: output file ; $3: what kind of file

-- 
2.51.0

[PATCH mptcp-net v2 5/5] selftests: mptcp: connect: print pcap suffix

[Matthieu Baerts (NGI0)]

To be able to find which capture files have been produced after several
runs.

This suffix was not printed anywhere before.

While at it, always use the same prefix by taking info from ns1, instead
of "$connector_ns", which is sometimes ns1, sometimes ns2 in the
subtests.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 5e3c56253274a1f938d2ed9986c4290fcea8b96b..19e2bd3000ec8be470a34d503ec034404e798fec 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -211,6 +211,11 @@ if $checksum; then
 	done
 fi
 
+if $capture; then
+	rndh="${ns1:4}"
+	mptcp_lib_pr_info "Packet capture files will have this prefix: ${rndh}-"
+fi
+
 set_ethtool_flags() {
 	local ns="$1"
 	local dev="$2"
@@ -361,7 +366,6 @@ do_transfer()
 
 	if $capture; then
 		local capuser
-		local rndh="${connector_ns:4}"
 		if [ -z $SUDO_USER ] ; then
 			capuser=""
 		else

-- 
2.51.0

Re: [PATCH mptcp-net v2 0/5] selftests: mptcp: avoid spurious errors on TCP disconnect

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Unstable: 3 failed test(s): packetdrill_dss packetdrill_fastopen packetdrill_syscalls 🔴
- KVM Validation: debug: Unstable: 3 failed test(s): packetdrill_dss packetdrill_fastopen packetdrill_syscalls 🔴
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/17501634949

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/8e609acad818
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=999479


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)

Re: [PATCH mptcp-net v2 5/5] selftests: mptcp: connect: print pcap suffix

[Geliang Tang]

Hi Matt,

Thanks for this v2.

On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
> To be able to find which capture files have been produced after
> several
> runs.
> 
> This suffix was not printed anywhere before.
> 
> While at it, always use the same prefix by taking info from ns1,
> instead
> of "$connector_ns", which is sometimes ns1, sometimes ns2 in the
> subtests.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 6 +++++-

Just wondering, is it necessary to synchronize this modification to
mptcp_join.sh to maintain consistency?

-Geliang

>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index
> 5e3c56253274a1f938d2ed9986c4290fcea8b96b..19e2bd3000ec8be470a34d503ec
> 034404e798fec 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -211,6 +211,11 @@ if $checksum; then
>  	done
>  fi
>  
> +if $capture; then
> +	rndh="${ns1:4}"
> +	mptcp_lib_pr_info "Packet capture files will have this
> prefix: ${rndh}-"
> +fi
> +
>  set_ethtool_flags() {
>  	local ns="$1"
>  	local dev="$2"
> @@ -361,7 +366,6 @@ do_transfer()
>  
>  	if $capture; then
>  		local capuser
> -		local rndh="${connector_ns:4}"
>  		if [ -z $SUDO_USER ] ; then
>  			capuser=""
>  		else

Re: [PATCH mptcp-net v2 4/5] selftests: mptcp: print trailing bytes with od

[Geliang Tang]

On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
> This is better than printing random bytes in the terminal.
> 
> Note Jakub suggested 'hexdump', but Mat found out that this tool is
> not
> often installed by default. 'od' can do a similar job, and it is in
> the
> POSIX specs and available in coreutils, so it should be on more
> systems.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Suggested-by: Mat Martineau <martineau@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> v2: switch to 'od', and no more check for its presence (Mat)
> ---
>  tools/testing/selftests/net/mptcp/mptcp_lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index
> 09cd24b2ae466205dacbdf8289eb86c08534c475..d62e653d48b0f2ef7a01e289fa0
> be8907825667d 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -384,7 +384,7 @@ mptcp_lib_make_file() {
>  mptcp_lib_print_file_err() {
>  	ls -l "${1}" 1>&2
>  	echo "Trailing bytes are: "
> -	tail -c 27 "${1}"
> +	tail -c 32 "${1}" | od -x | head -n2

nit:

It shows more bytes (32) here, but it wasn't mentioned in the commit
log.

Thanks,
-Geliang

>  }
>  
>  # $1: input file ; $2: output file ; $3: what kind of file

Re: [PATCH mptcp-net v2 2/5] selftests: mptcp: connect: catch IO errors on listen side

[Geliang Tang]

On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
> IO errors were correctly printed to stderr, and propagated up to the
> main loop for the server side, but the returned value was ignored. As
> a
> consequence, the program for the listener side was no longer exiting
> with an error code in case of IO issues.
> 
> Because of that, some issues might not have been seen. But very
> likely,
> most issues either had an effect on the client side, or the file
> transfer was not the expected one, e.g. the connection got reset
> before
> the end. Still, it is better to fix this.
> 
> The main consequence of this issue is the error that was reported by
> the
> selftests: the received and sent files were different, and the MIB
> counters were not printed. Also, when such errors happened during the
> 'disconnect' tests, the program tried to continue until the timeout.
> 
> Now when an IO error is detected, the program exits directly with an
> error.
> 
> Fixes: 05be5e273c84 ("selftests: mptcp: add disconnect tests")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index
> 4f07ac9fa207cb08a934582b98d688d0b9512f97..c1586a7286123a509495ac319fd
> 624b98f0bfd18 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -1112,6 +1112,8 @@ int main_loop_s(int listensock)
>  	salen = sizeof(ss);
>  	remotesock = accept(listensock, (struct sockaddr *)&ss,
> &salen);
>  	if (remotesock >= 0) {
> +		int err;
> +
>  		maybe_close(listensock);
>  		check_sockaddr(pf, &ss, salen);
>  		check_getpeername(remotesock, &ss, salen);
> @@ -1125,7 +1127,9 @@ int main_loop_s(int listensock)
>  		SOCK_TEST_TCPULP(remotesock, 0);
>  
>  		memset(&winfo, 0, sizeof(winfo));
> -		copyfd_io(fd, remotesock, 1, true, &winfo);
> +		err = copyfd_io(fd, remotesock, 1, true, &winfo);
> +		if (err)
> +			return err;

The file descriptor (fd) should be closed before returning.

Thanks,
-Geliang

>  	} else {
>  		perror("accept");
>  		return 1;

Re: [PATCH mptcp-net v2 1/5] mptcp: propagate shutdown to subflows when possible

[Geliang Tang]

On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
> When the MPTCP DATA FIN have been ACKed, there is no more MPTCP
> related
> metadata to exchange, and all subflows can be safely shutdown.
> 
> Before this patch, the subflows were actually terminated at 'close()'
> time. That's certainly fine most of the time, but not when the
> userspace
> 'shutdown()' a connection, without close()ing it. When doing so, the
> subflows were staying in LAST_ACK state on one side -- and
> consequently
> in FIN_WAIT2 on the other side -- until the 'close()' of the MPTCP
> socket.
> 
> Now, when the DATA FIN have been ACKed, all subflows are shutdown. A
> consequence of this is that the TCP 'FIN' flag can be set earlier
> now,
> but the end result is the same. This affects the packetdrill tests
> looking at the end of the MPTCP connections, but for a good reason.
> 
> Fixes: 3721b9b64676 ("mptcp: Track received DATA_FIN sequence number
> and add related helpers")
> Fixes: 16a9a9da1723 ("mptcp: Add helper to process acks of DATA_FIN")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/protocol.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index
> 1c26acf6c4145896ecbb5c7f7004121c66a20649..9feb9f437c2bdfff392249e7ad0
> 0edd09ba67ac3 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -372,6 +372,20 @@ static void mptcp_close_wake_up(struct sock *sk)
>  		sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
>  }
>  
> +static void mptcp_shutdown_subflows(struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow;
> +
> +	mptcp_for_each_subflow(msk, subflow) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +		bool slow;
> +
> +		slow = lock_sock_fast(ssk);
> +		tcp_shutdown(ssk, SEND_SHUTDOWN);
> +		unlock_sock_fast(ssk, slow);
> +	}
> +}
> +
>  /* called under the msk socket lock */
>  static bool mptcp_pending_data_fin_ack(struct sock *sk)
>  {
> @@ -396,6 +410,7 @@ static void mptcp_check_data_fin_ack(struct sock
> *sk)
>  			break;
>  		case TCP_CLOSING:
>  		case TCP_LAST_ACK:
> +			mptcp_shutdown_subflows(msk);
>  			mptcp_set_state(sk, TCP_CLOSE);
>  			break;
>  		}
> @@ -564,6 +579,7 @@ static bool mptcp_check_data_fin(struct sock *sk)
>  			mptcp_set_state(sk, TCP_CLOSING);
>  			break;
>  		case TCP_FIN_WAIT2:
> +			mptcp_shutdown_subflows(msk);

I think we should not directly call mptcp_shutdown_subflows() within
mptcp_check_data_fin() and mptcp_check_data_fin_ack(), but should
instead call it after these functions return and the sk state is
TCP_CLOSE. This ensures that the original purpose of these two
functions remains unchanged.

WDYT?

Thanks,
-Geliang

>  			mptcp_set_state(sk, TCP_CLOSE);
>  			break;
>  		default:

Re: [PATCH mptcp-net v2 1/5] mptcp: propagate shutdown to subflows when possible

[Geliang Tang]

On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
> When the MPTCP DATA FIN have been ACKed, there is no more MPTCP
> related
> metadata to exchange, and all subflows can be safely shutdown.
> 
> Before this patch, the subflows were actually terminated at 'close()'
> time. That's certainly fine most of the time, but not when the
> userspace
> 'shutdown()' a connection, without close()ing it. When doing so, the
> subflows were staying in LAST_ACK state on one side -- and
> consequently
> in FIN_WAIT2 on the other side -- until the 'close()' of the MPTCP
> socket.
> 
> Now, when the DATA FIN have been ACKed, all subflows are shutdown. A
> consequence of this is that the TCP 'FIN' flag can be set earlier
> now,
> but the end result is the same. This affects the packetdrill tests
> looking at the end of the MPTCP connections, but for a good reason.
> 
> Fixes: 3721b9b64676 ("mptcp: Track received DATA_FIN sequence number
> and add related helpers")
> Fixes: 16a9a9da1723 ("mptcp: Add helper to process acks of DATA_FIN")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/protocol.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index
> 1c26acf6c4145896ecbb5c7f7004121c66a20649..9feb9f437c2bdfff392249e7ad0
> 0edd09ba67ac3 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -372,6 +372,20 @@ static void mptcp_close_wake_up(struct sock *sk)
>  		sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
>  }
>  
> +static void mptcp_shutdown_subflows(struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow;
> +
> +	mptcp_for_each_subflow(msk, subflow) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +		bool slow;
> +
> +		slow = lock_sock_fast(ssk);
> +		tcp_shutdown(ssk, SEND_SHUTDOWN);
> +		unlock_sock_fast(ssk, slow);
> +	}
> +}
> +
>  /* called under the msk socket lock */
>  static bool mptcp_pending_data_fin_ack(struct sock *sk)
>  {
> @@ -396,6 +410,7 @@ static void mptcp_check_data_fin_ack(struct sock
> *sk)
>  			break;
>  		case TCP_CLOSING:
>  		case TCP_LAST_ACK:
> +			mptcp_shutdown_subflows(msk);
>  			mptcp_set_state(sk, TCP_CLOSE);
>  			break;
>  		}
> @@ -564,6 +579,7 @@ static bool mptcp_check_data_fin(struct sock *sk)
>  			mptcp_set_state(sk, TCP_CLOSING);
>  			break;
>  		case TCP_FIN_WAIT2:
> +			mptcp_shutdown_subflows(msk);

I think we should not directly call mptcp_shutdown_subflows() within
mptcp_check_data_fin() and mptcp_check_data_fin_ack(), but should
instead call it after these functions return and the sk state is
TCP_CLOSE. This ensures that the original purpose of these two
functions remains unchanged.

WDYT?

Thanks,
-Geliang

>  			mptcp_set_state(sk, TCP_CLOSE);
>  			break;
>  		default:

Re: [PATCH mptcp-net v2 1/5] mptcp: propagate shutdown to subflows when possible

[Matthieu Baerts]

Hi Geliang,

On 06/09/2025 02:45, Geliang Tang wrote:
> On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
>> When the MPTCP DATA FIN have been ACKed, there is no more MPTCP
>> related
>> metadata to exchange, and all subflows can be safely shutdown.
>>
>> Before this patch, the subflows were actually terminated at 'close()'
>> time. That's certainly fine most of the time, but not when the
>> userspace
>> 'shutdown()' a connection, without close()ing it. When doing so, the
>> subflows were staying in LAST_ACK state on one side -- and
>> consequently
>> in FIN_WAIT2 on the other side -- until the 'close()' of the MPTCP
>> socket.
>>
>> Now, when the DATA FIN have been ACKed, all subflows are shutdown. A
>> consequence of this is that the TCP 'FIN' flag can be set earlier
>> now,
>> but the end result is the same. This affects the packetdrill tests
>> looking at the end of the MPTCP connections, but for a good reason.
>>
>> Fixes: 3721b9b64676 ("mptcp: Track received DATA_FIN sequence number
>> and add related helpers")
>> Fixes: 16a9a9da1723 ("mptcp: Add helper to process acks of DATA_FIN")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  net/mptcp/protocol.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index
>> 1c26acf6c4145896ecbb5c7f7004121c66a20649..9feb9f437c2bdfff392249e7ad0
>> 0edd09ba67ac3 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -372,6 +372,20 @@ static void mptcp_close_wake_up(struct sock *sk)
>>  		sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
>>  }
>>  
>> +static void mptcp_shutdown_subflows(struct mptcp_sock *msk)
>> +{
>> +	struct mptcp_subflow_context *subflow;
>> +
>> +	mptcp_for_each_subflow(msk, subflow) {
>> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>> +		bool slow;
>> +
>> +		slow = lock_sock_fast(ssk);
>> +		tcp_shutdown(ssk, SEND_SHUTDOWN);
>> +		unlock_sock_fast(ssk, slow);
>> +	}
>> +}
>> +
>>  /* called under the msk socket lock */
>>  static bool mptcp_pending_data_fin_ack(struct sock *sk)
>>  {
>> @@ -396,6 +410,7 @@ static void mptcp_check_data_fin_ack(struct sock
>> *sk)
>>  			break;
>>  		case TCP_CLOSING:
>>  		case TCP_LAST_ACK:
>> +			mptcp_shutdown_subflows(msk);
>>  			mptcp_set_state(sk, TCP_CLOSE);
>>  			break;
>>  		}
>> @@ -564,6 +579,7 @@ static bool mptcp_check_data_fin(struct sock *sk)
>>  			mptcp_set_state(sk, TCP_CLOSING);
>>  			break;
>>  		case TCP_FIN_WAIT2:
>> +			mptcp_shutdown_subflows(msk);
> 
> I think we should not directly call mptcp_shutdown_subflows() within
> mptcp_check_data_fin() and mptcp_check_data_fin_ack(), but should
> instead call it after these functions return and the sk state is
> TCP_CLOSE. This ensures that the original purpose of these two
> functions remains unchanged.

Thank you for the review. I'm not sure to understand what you mean about
the "original purpose": they both set the appropriated actions when
DATA_FIN have been received. Here, I do want to shut the subflows down
when the DATA_FIN ACK are received, so that's the good spot, no?

Note that commit 3721b9b64676 ("mptcp: Track received DATA_FIN sequence
number and add related helpers") already had a hint of what should have
been done: see the "Close subflows now?" comment, later removed in
commit e16163b6e2b7 ("mptcp: refactor shutdown and close").

Or maybe do you mean their original name "check_data_fin(_ack)" is
confusing? The "check" here is not just to check if the DATA_FIN (ACK)
was set, but to check actions related to them.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net v2 2/5] selftests: mptcp: connect: catch IO errors on listen side

[Matthieu Baerts]

Hi Geliang,

On 06/09/2025 01:49, Geliang Tang wrote:
> On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
>> IO errors were correctly printed to stderr, and propagated up to the
>> main loop for the server side, but the returned value was ignored. As
>> a
>> consequence, the program for the listener side was no longer exiting
>> with an error code in case of IO issues.
>>
>> Because of that, some issues might not have been seen. But very
>> likely,
>> most issues either had an effect on the client side, or the file
>> transfer was not the expected one, e.g. the connection got reset
>> before
>> the end. Still, it is better to fix this.
>>
>> The main consequence of this issue is the error that was reported by
>> the
>> selftests: the received and sent files were different, and the MIB
>> counters were not printed. Also, when such errors happened during the
>> 'disconnect' tests, the program tried to continue until the timeout.
>>
>> Now when an IO error is detected, the program exits directly with an
>> error.
>>
>> Fixes: 05be5e273c84 ("selftests: mptcp: add disconnect tests")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_connect.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> b/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> index
>> 4f07ac9fa207cb08a934582b98d688d0b9512f97..c1586a7286123a509495ac319fd
>> 624b98f0bfd18 100644
>> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> @@ -1112,6 +1112,8 @@ int main_loop_s(int listensock)
>>  	salen = sizeof(ss);
>>  	remotesock = accept(listensock, (struct sockaddr *)&ss,
>> &salen);
>>  	if (remotesock >= 0) {
>> +		int err;
>> +
>>  		maybe_close(listensock);
>>  		check_sockaddr(pf, &ss, salen);
>>  		check_getpeername(remotesock, &ss, salen);
>> @@ -1125,7 +1127,9 @@ int main_loop_s(int listensock)
>>  		SOCK_TEST_TCPULP(remotesock, 0);
>>  
>>  		memset(&winfo, 0, sizeof(winfo));
>> -		copyfd_io(fd, remotesock, 1, true, &winfo);
>> +		err = copyfd_io(fd, remotesock, 1, true, &winfo);
>> +		if (err)
>> +			return err;
> 
> The file descriptor (fd) should be closed before returning.

I do agree it would be better to do so, but when you look in this file,
most errors paths don't close the FD, because that's the exit path.

'fd' will need to be closed, 'remotesock' as well, but same when it was
not possible to create 'fd'. Also, I guess 'listensock' is never closed.
Honestly, I don't think we need to increase the complexity, and close
them, just for the tests. It would be different if the FD were reused
later on, but I don't think we need to spend time on these details when
this test program is about to close.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net v2 4/5] selftests: mptcp: print trailing bytes with od

[Matthieu Baerts]

Hi Geliang,

On 06/09/2025 01:42, Geliang Tang wrote:
> On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
>> This is better than printing random bytes in the terminal.
>>
>> Note Jakub suggested 'hexdump', but Mat found out that this tool is
>> not
>> often installed by default. 'od' can do a similar job, and it is in
>> the
>> POSIX specs and available in coreutils, so it should be on more
>> systems.
>>
>> Suggested-by: Jakub Kicinski <kuba@kernel.org>
>> Suggested-by: Mat Martineau <martineau@kernel.org>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> v2: switch to 'od', and no more check for its presence (Mat)
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_lib.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>> index
>> 09cd24b2ae466205dacbdf8289eb86c08534c475..d62e653d48b0f2ef7a01e289fa0
>> be8907825667d 100644
>> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>> @@ -384,7 +384,7 @@ mptcp_lib_make_file() {
>>  mptcp_lib_print_file_err() {
>>  	ls -l "${1}" 1>&2
>>  	echo "Trailing bytes are: "
>> -	tail -c 27 "${1}"
>> +	tail -c 32 "${1}" | od -x | head -n2
> 
> nit:
> 
> It shows more bytes (32) here, but it wasn't mentioned in the commit
> log.

Good point, I can add this when applying the patches:

  Show a few more bytes, just to fill in the two lines.

Would that be OK?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net v2 5/5] selftests: mptcp: connect: print pcap suffix

[Matthieu Baerts]

Hi Geliang,

On 06/09/2025 01:39, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for this v2.
> 
> On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
>> To be able to find which capture files have been produced after
>> several
>> runs.
>>
>> This suffix was not printed anywhere before.
>>
>> While at it, always use the same prefix by taking info from ns1,
>> instead
>> of "$connector_ns", which is sometimes ns1, sometimes ns2 in the
>> subtests.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 6 +++++-
> 
> Just wondering, is it necessary to synchronize this modification to
> mptcp_join.sh to maintain consistency?

I don't think that's needed: there is already a similar message in
mptcp_join.sh:

  Capturing traffic for test $MPTCP_LIB_TEST_COUNTER into $capfile

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net v2 0/5] selftests: mptcp: avoid spurious errors on TCP disconnect

[Matthieu Baerts]

Hello,

On 05/09/2025 22:46, MPTCP CI wrote:
> Hi Matthieu,
> 
> Thank you for your modifications, that's great!
> 
> Our CI did some validations and here is its report:
> 
> - KVM Validation: normal: Unstable: 3 failed test(s): packetdrill_dss packetdrill_fastopen packetdrill_syscalls 🔴
> - KVM Validation: debug: Unstable: 3 failed test(s): packetdrill_dss packetdrill_fastopen packetdrill_syscalls 🔴

As mentioned in the cover letter, these failures were expected:

Note: Patch 1 will cause errors in Packetdrill, that's normal, see:

  https://github.com/multipath-tcp/packetdrill/pull/171

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net v2 2/5] selftests: mptcp: connect: catch IO errors on listen side

[Geliang Tang]

Hi Matt,

On Sat, 2025-09-06 at 15:54 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 06/09/2025 01:49, Geliang Tang wrote:
> > On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
> > > IO errors were correctly printed to stderr, and propagated up to
> > > the
> > > main loop for the server side, but the returned value was
> > > ignored. As
> > > a
> > > consequence, the program for the listener side was no longer
> > > exiting
> > > with an error code in case of IO issues.
> > > 
> > > Because of that, some issues might not have been seen. But very
> > > likely,
> > > most issues either had an effect on the client side, or the file
> > > transfer was not the expected one, e.g. the connection got reset
> > > before
> > > the end. Still, it is better to fix this.
> > > 
> > > The main consequence of this issue is the error that was reported
> > > by
> > > the
> > > selftests: the received and sent files were different, and the
> > > MIB
> > > counters were not printed. Also, when such errors happened during
> > > the
> > > 'disconnect' tests, the program tried to continue until the
> > > timeout.
> > > 
> > > Now when an IO error is detected, the program exits directly with
> > > an
> > > error.
> > > 
> > > Fixes: 05be5e273c84 ("selftests: mptcp: add disconnect tests")
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_connect.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > index
> > > 4f07ac9fa207cb08a934582b98d688d0b9512f97..c1586a7286123a509495ac3
> > > 19fd
> > > 624b98f0bfd18 100644
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > @@ -1112,6 +1112,8 @@ int main_loop_s(int listensock)
> > >  	salen = sizeof(ss);
> > >  	remotesock = accept(listensock, (struct sockaddr *)&ss,
> > > &salen);
> > >  	if (remotesock >= 0) {
> > > +		int err;
> > > +
> > >  		maybe_close(listensock);
> > >  		check_sockaddr(pf, &ss, salen);
> > >  		check_getpeername(remotesock, &ss, salen);
> > > @@ -1125,7 +1127,9 @@ int main_loop_s(int listensock)
> > >  		SOCK_TEST_TCPULP(remotesock, 0);
> > >  
> > >  		memset(&winfo, 0, sizeof(winfo));
> > > -		copyfd_io(fd, remotesock, 1, true, &winfo);
> > > +		err = copyfd_io(fd, remotesock, 1, true,
> > > &winfo);
> > > +		if (err)
> > > +			return err;
> > 
> > The file descriptor (fd) should be closed before returning.
> 
> I do agree it would be better to do so, but when you look in this
> file,
> most errors paths don't close the FD, because that's the exit path.
> 
> 'fd' will need to be closed, 'remotesock' as well, but same when it
> was
> not possible to create 'fd'. Also, I guess 'listensock' is never
> closed.
> Honestly, I don't think we need to increase the complexity, and close
> them, just for the tests. It would be different if the FD were reused
> later on, but I don't think we need to spend time on these details
> when
> this test program is about to close.

What I mean here is not to return directly, something like:

'''
int err = 0;

if (remotesock >= 0) {
    ... ...
    err = copyfd_io(fd, remotesock, 1, true, &winfo);
}

if (cfg_input)
    close(fd);

if (!err && --cfg_repeat > 0)
    goto again;

return err;
'''

WDYT?

I will send a patch to close remotesock and listensock later.

Thanks,
-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-net v2 1/5] mptcp: propagate shutdown to subflows when possible

[Geliang Tang]

Hi Matt,

On Sat, 2025-09-06 at 15:42 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 06/09/2025 02:45, Geliang Tang wrote:
> > On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
> > > When the MPTCP DATA FIN have been ACKed, there is no more MPTCP
> > > related
> > > metadata to exchange, and all subflows can be safely shutdown.
> > > 
> > > Before this patch, the subflows were actually terminated at
> > > 'close()'
> > > time. That's certainly fine most of the time, but not when the
> > > userspace
> > > 'shutdown()' a connection, without close()ing it. When doing so,
> > > the
> > > subflows were staying in LAST_ACK state on one side -- and
> > > consequently
> > > in FIN_WAIT2 on the other side -- until the 'close()' of the
> > > MPTCP
> > > socket.
> > > 
> > > Now, when the DATA FIN have been ACKed, all subflows are
> > > shutdown. A
> > > consequence of this is that the TCP 'FIN' flag can be set earlier
> > > now,
> > > but the end result is the same. This affects the packetdrill
> > > tests
> > > looking at the end of the MPTCP connections, but for a good
> > > reason.
> > > 
> > > Fixes: 3721b9b64676 ("mptcp: Track received DATA_FIN sequence
> > > number
> > > and add related helpers")
> > > Fixes: 16a9a9da1723 ("mptcp: Add helper to process acks of
> > > DATA_FIN")
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  net/mptcp/protocol.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > index
> > > 1c26acf6c4145896ecbb5c7f7004121c66a20649..9feb9f437c2bdfff392249e
> > > 7ad0
> > > 0edd09ba67ac3 100644
> > > --- a/net/mptcp/protocol.c
> > > +++ b/net/mptcp/protocol.c
> > > @@ -372,6 +372,20 @@ static void mptcp_close_wake_up(struct sock
> > > *sk)
> > >  		sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
> > >  }
> > >  
> > > +static void mptcp_shutdown_subflows(struct mptcp_sock *msk)
> > > +{
> > > +	struct mptcp_subflow_context *subflow;
> > > +
> > > +	mptcp_for_each_subflow(msk, subflow) {
> > > +		struct sock *ssk =
> > > mptcp_subflow_tcp_sock(subflow);
> > > +		bool slow;
> > > +
> > > +		slow = lock_sock_fast(ssk);
> > > +		tcp_shutdown(ssk, SEND_SHUTDOWN);
> > > +		unlock_sock_fast(ssk, slow);
> > > +	}
> > > +}
> > > +
> > >  /* called under the msk socket lock */
> > >  static bool mptcp_pending_data_fin_ack(struct sock *sk)
> > >  {
> > > @@ -396,6 +410,7 @@ static void mptcp_check_data_fin_ack(struct
> > > sock
> > > *sk)
> > >  			break;
> > >  		case TCP_CLOSING:
> > >  		case TCP_LAST_ACK:
> > > +			mptcp_shutdown_subflows(msk);
> > >  			mptcp_set_state(sk, TCP_CLOSE);
> > >  			break;
> > >  		}
> > > @@ -564,6 +579,7 @@ static bool mptcp_check_data_fin(struct sock
> > > *sk)
> > >  			mptcp_set_state(sk, TCP_CLOSING);
> > >  			break;
> > >  		case TCP_FIN_WAIT2:
> > > +			mptcp_shutdown_subflows(msk);
> > 
> > I think we should not directly call mptcp_shutdown_subflows()
> > within
> > mptcp_check_data_fin() and mptcp_check_data_fin_ack(), but should
> > instead call it after these functions return and the sk state is
> > TCP_CLOSE. This ensures that the original purpose of these two
> > functions remains unchanged.
> 
> Thank you for the review. I'm not sure to understand what you mean
> about
> the "original purpose": they both set the appropriated actions when
> DATA_FIN have been received. Here, I do want to shut the subflows
> down
> when the DATA_FIN ACK are received, so that's the good spot, no?
> 
> Note that commit 3721b9b64676 ("mptcp: Track received DATA_FIN
> sequence
> number and add related helpers") already had a hint of what should
> have
> been done: see the "Close subflows now?" comment, later removed in
> commit e16163b6e2b7 ("mptcp: refactor shutdown and close").
> 
> Or maybe do you mean their original name "check_data_fin(_ack)" is
> confusing? The "check" here is not just to check if the DATA_FIN
> (ACK)
> was set, but to check actions related to them.

I mean not to modify mptcp_check_data_fin and mptcp_check_data_fin_ack,
but to check TCP_CLOSE in mptcp_worker(), something like:

@@ -2749,6 +2749,9 @@ static void mptcp_worker(struct work_struct
*work)
        mptcp_check_data_fin_ack(sk);
        mptcp_check_data_fin(sk);
 
+       if (sk->sk_state == TCP_CLOSE)
+               mptcp_shutdown_subflows(msk);
+
        if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
                __mptcp_close_subflow(sk);

Another option is to call mptcp_shutdown_subflows in
mptcp_close_wake_up, which one do you think is better?

Thanks,
-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-net v2 4/5] selftests: mptcp: print trailing bytes with od

[Geliang Tang]

Hi Matt,

On Sat, 2025-09-06 at 15:56 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 06/09/2025 01:42, Geliang Tang wrote:
> > On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
> > > This is better than printing random bytes in the terminal.
> > > 
> > > Note Jakub suggested 'hexdump', but Mat found out that this tool
> > > is
> > > not
> > > often installed by default. 'od' can do a similar job, and it is
> > > in
> > > the
> > > POSIX specs and available in coreutils, so it should be on more
> > > systems.
> > > 
> > > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > > Suggested-by: Mat Martineau <martineau@kernel.org>
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > > v2: switch to 'od', and no more check for its presence (Mat)
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_lib.sh | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > > b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > > index
> > > 09cd24b2ae466205dacbdf8289eb86c08534c475..d62e653d48b0f2ef7a01e28
> > > 9fa0
> > > be8907825667d 100644
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > > @@ -384,7 +384,7 @@ mptcp_lib_make_file() {
> > >  mptcp_lib_print_file_err() {
> > >  	ls -l "${1}" 1>&2
> > >  	echo "Trailing bytes are: "
> > > -	tail -c 27 "${1}"
> > > +	tail -c 32 "${1}" | od -x | head -n2
> > 
> > nit:
> > 
> > It shows more bytes (32) here, but it wasn't mentioned in the
> > commit
> > log.
> 
> Good point, I can add this when applying the patches:
> 
>   Show a few more bytes, just to fill in the two lines.
> 
> Would that be OK?

That would be great.

Thanks,
-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-net v2 5/5] selftests: mptcp: connect: print pcap suffix

[Geliang Tang]

Hi Matt,

On Sat, 2025-09-06 at 15:58 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 06/09/2025 01:39, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Thanks for this v2.
> > 
> > On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
> > > To be able to find which capture files have been produced after
> > > several
> > > runs.
> > > 
> > > This suffix was not printed anywhere before.
> > > 
> > > While at it, always use the same prefix by taking info from ns1,
> > > instead
> > > of "$connector_ns", which is sometimes ns1, sometimes ns2 in the
> > > subtests.
> > > 
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 6 +++++-
> > 
> > Just wondering, is it necessary to synchronize this modification to
> > mptcp_join.sh to maintain consistency?
> 
> I don't think that's needed: there is already a similar message in
> mptcp_join.sh:
> 
>   Capturing traffic for test $MPTCP_LIB_TEST_COUNTER into $capfile

Thank you for your explanation.

-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-net v2 2/5] selftests: mptcp: connect: catch IO errors on listen side

[Matthieu Baerts]

Hi Geliang,

On 10/09/2025 04:44, Geliang Tang wrote:
> Hi Matt,
> 
> On Sat, 2025-09-06 at 15:54 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 06/09/2025 01:49, Geliang Tang wrote:
>>> On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
>>>> IO errors were correctly printed to stderr, and propagated up to
>>>> the
>>>> main loop for the server side, but the returned value was
>>>> ignored. As
>>>> a
>>>> consequence, the program for the listener side was no longer
>>>> exiting
>>>> with an error code in case of IO issues.
>>>>
>>>> Because of that, some issues might not have been seen. But very
>>>> likely,
>>>> most issues either had an effect on the client side, or the file
>>>> transfer was not the expected one, e.g. the connection got reset
>>>> before
>>>> the end. Still, it is better to fix this.
>>>>
>>>> The main consequence of this issue is the error that was reported
>>>> by
>>>> the
>>>> selftests: the received and sent files were different, and the
>>>> MIB
>>>> counters were not printed. Also, when such errors happened during
>>>> the
>>>> 'disconnect' tests, the program tried to continue until the
>>>> timeout.
>>>>
>>>> Now when an IO error is detected, the program exits directly with
>>>> an
>>>> error.
>>>>
>>>> Fixes: 05be5e273c84 ("selftests: mptcp: add disconnect tests")
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>> ---
>>>>  tools/testing/selftests/net/mptcp/mptcp_connect.c | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
>>>> b/tools/testing/selftests/net/mptcp/mptcp_connect.c
>>>> index
>>>> 4f07ac9fa207cb08a934582b98d688d0b9512f97..c1586a7286123a509495ac3
>>>> 19fd
>>>> 624b98f0bfd18 100644
>>>> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
>>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
>>>> @@ -1112,6 +1112,8 @@ int main_loop_s(int listensock)
>>>>  	salen = sizeof(ss);
>>>>  	remotesock = accept(listensock, (struct sockaddr *)&ss,
>>>> &salen);
>>>>  	if (remotesock >= 0) {
>>>> +		int err;
>>>> +
>>>>  		maybe_close(listensock);
>>>>  		check_sockaddr(pf, &ss, salen);
>>>>  		check_getpeername(remotesock, &ss, salen);
>>>> @@ -1125,7 +1127,9 @@ int main_loop_s(int listensock)
>>>>  		SOCK_TEST_TCPULP(remotesock, 0);
>>>>  
>>>>  		memset(&winfo, 0, sizeof(winfo));
>>>> -		copyfd_io(fd, remotesock, 1, true, &winfo);
>>>> +		err = copyfd_io(fd, remotesock, 1, true,
>>>> &winfo);
>>>> +		if (err)
>>>> +			return err;
>>>
>>> The file descriptor (fd) should be closed before returning.
>>
>> I do agree it would be better to do so, but when you look in this
>> file,
>> most errors paths don't close the FD, because that's the exit path.
>>
>> 'fd' will need to be closed, 'remotesock' as well, but same when it
>> was
>> not possible to create 'fd'. Also, I guess 'listensock' is never
>> closed.
>> Honestly, I don't think we need to increase the complexity, and close
>> them, just for the tests. It would be different if the FD were reused
>> later on, but I don't think we need to spend time on these details
>> when
>> this test program is about to close.
> 
> What I mean here is not to return directly, something like:
> 
> '''
> int err = 0;
> 
> if (remotesock >= 0) {
>     ... ...
>     err = copyfd_io(fd, remotesock, 1, true, &winfo);
> }
> 
> if (cfg_input)
>     close(fd);
> 
> if (!err && --cfg_repeat > 0)
>     goto again;
> 
> return err;
> '''
> 
> WDYT?

I do agree it is better. But again, I think there is no need to do that
in this patch, because most error exit paths in this file don't close
the fd's. That makes the patch longer and slightly more complex -- which
can cause more troubles during the backports -- just for the exit error
path. But because I keep being reminded by net maintainers that this
series is needed to fix CI noises (and real issues), I want to move on,
and I'm going to send a v3 with this modification.

> I will send a patch to close remotesock and listensock later.
> 
> Thanks,
> -Geliang
> 
>>
>> Cheers,
>> Matt

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net v2 1/5] mptcp: propagate shutdown to subflows when possible

[Matthieu Baerts]

Hi Geliang,

On 10/09/2025 05:24, Geliang Tang wrote:
> Hi Matt,
> 
> On Sat, 2025-09-06 at 15:42 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 06/09/2025 02:45, Geliang Tang wrote:
>>> On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
>>>> When the MPTCP DATA FIN have been ACKed, there is no more MPTCP
>>>> related
>>>> metadata to exchange, and all subflows can be safely shutdown.
>>>>
>>>> Before this patch, the subflows were actually terminated at
>>>> 'close()'
>>>> time. That's certainly fine most of the time, but not when the
>>>> userspace
>>>> 'shutdown()' a connection, without close()ing it. When doing so,
>>>> the
>>>> subflows were staying in LAST_ACK state on one side -- and
>>>> consequently
>>>> in FIN_WAIT2 on the other side -- until the 'close()' of the
>>>> MPTCP
>>>> socket.
>>>>
>>>> Now, when the DATA FIN have been ACKed, all subflows are
>>>> shutdown. A
>>>> consequence of this is that the TCP 'FIN' flag can be set earlier
>>>> now,
>>>> but the end result is the same. This affects the packetdrill
>>>> tests
>>>> looking at the end of the MPTCP connections, but for a good
>>>> reason.
>>>>
>>>> Fixes: 3721b9b64676 ("mptcp: Track received DATA_FIN sequence
>>>> number
>>>> and add related helpers")
>>>> Fixes: 16a9a9da1723 ("mptcp: Add helper to process acks of
>>>> DATA_FIN")
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>> ---
>>>>  net/mptcp/protocol.c | 16 ++++++++++++++++
>>>>  1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>>> index
>>>> 1c26acf6c4145896ecbb5c7f7004121c66a20649..9feb9f437c2bdfff392249e
>>>> 7ad0
>>>> 0edd09ba67ac3 100644
>>>> --- a/net/mptcp/protocol.c
>>>> +++ b/net/mptcp/protocol.c
>>>> @@ -372,6 +372,20 @@ static void mptcp_close_wake_up(struct sock
>>>> *sk)
>>>>  		sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
>>>>  }
>>>>  
>>>> +static void mptcp_shutdown_subflows(struct mptcp_sock *msk)
>>>> +{
>>>> +	struct mptcp_subflow_context *subflow;
>>>> +
>>>> +	mptcp_for_each_subflow(msk, subflow) {
>>>> +		struct sock *ssk =
>>>> mptcp_subflow_tcp_sock(subflow);
>>>> +		bool slow;
>>>> +
>>>> +		slow = lock_sock_fast(ssk);
>>>> +		tcp_shutdown(ssk, SEND_SHUTDOWN);
>>>> +		unlock_sock_fast(ssk, slow);
>>>> +	}
>>>> +}
>>>> +
>>>>  /* called under the msk socket lock */
>>>>  static bool mptcp_pending_data_fin_ack(struct sock *sk)
>>>>  {
>>>> @@ -396,6 +410,7 @@ static void mptcp_check_data_fin_ack(struct
>>>> sock
>>>> *sk)
>>>>  			break;
>>>>  		case TCP_CLOSING:
>>>>  		case TCP_LAST_ACK:
>>>> +			mptcp_shutdown_subflows(msk);
>>>>  			mptcp_set_state(sk, TCP_CLOSE);
>>>>  			break;
>>>>  		}
>>>> @@ -564,6 +579,7 @@ static bool mptcp_check_data_fin(struct sock
>>>> *sk)
>>>>  			mptcp_set_state(sk, TCP_CLOSING);
>>>>  			break;
>>>>  		case TCP_FIN_WAIT2:
>>>> +			mptcp_shutdown_subflows(msk);
>>>
>>> I think we should not directly call mptcp_shutdown_subflows()
>>> within
>>> mptcp_check_data_fin() and mptcp_check_data_fin_ack(), but should
>>> instead call it after these functions return and the sk state is
>>> TCP_CLOSE. This ensures that the original purpose of these two
>>> functions remains unchanged.
>>
>> Thank you for the review. I'm not sure to understand what you mean
>> about
>> the "original purpose": they both set the appropriated actions when
>> DATA_FIN have been received. Here, I do want to shut the subflows
>> down
>> when the DATA_FIN ACK are received, so that's the good spot, no?
>>
>> Note that commit 3721b9b64676 ("mptcp: Track received DATA_FIN
>> sequence
>> number and add related helpers") already had a hint of what should
>> have
>> been done: see the "Close subflows now?" comment, later removed in
>> commit e16163b6e2b7 ("mptcp: refactor shutdown and close").
>>
>> Or maybe do you mean their original name "check_data_fin(_ack)" is
>> confusing? The "check" here is not just to check if the DATA_FIN
>> (ACK)
>> was set, but to check actions related to them.
> 
> I mean not to modify mptcp_check_data_fin and mptcp_check_data_fin_ack,
> but to check TCP_CLOSE in mptcp_worker(), something like:
> 
> @@ -2749,6 +2749,9 @@ static void mptcp_worker(struct work_struct
> *work)
>         mptcp_check_data_fin_ack(sk);
>         mptcp_check_data_fin(sk);
>  
> +       if (sk->sk_state == TCP_CLOSE)
> +               mptcp_shutdown_subflows(msk);
> +
>         if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
>                 __mptcp_close_subflow(sk);

No, we should only do that when receiving the DATA_FIN ACK, when
transitioning from FIN_WAIT_2/LAST_ACK to CLOSE.

I do think these data_fin functions are the best place, in the
operations linked to the state machine, not here in the worker.

> Another option is to call mptcp_shutdown_subflows in
> mptcp_close_wake_up, which one do you think is better?

No, that's not linked to 'waking up the userspace'.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.