[PATCH mptcp-net 00/34] mptcp: misc fixes

[PATCH mptcp-net 00/34] mptcp: misc fixes

[Matthieu Baerts (NGI0)]

Here are various fixes, mostly from Chris Mason and Josef Bacik:

- Patches 1-2: drain the queues of a TCP or MPTCP socket when listen()
  fails, not to leak sockets and their netns. Fixes for v3.7 and v5.11.

- Patch 3: clear inet_opt in the child of an IPv6 listener, to avoid a
  double free with SELinux and NetLabel. A fix for v5.6.

- Patches 4-6: do not lose an RM_ADDR, when received in a row, or sent
  with a pending ADD_ADDR echo. Fixes for v5.13, v5.10 and v5.15.

- Patches 7-8: do not leave data unsent after a memory shortage. Fixes
  for another fix introduced in v6.19 and targeting v5.15, and for
  v5.11.

- Patches 9-10: do not leak an orphan moved to the CLOSE state outside
  the worker. Fixes for v6.15, and for another fix introduced in v6.6
  and targeting v5.11.

- Patch 11: do not send data beyond the window announced by the peer. A
  fix for v5.11.

- Patch 12: drop the ACKs with a SYN cookie and no MP_JOIN on PM
  listeners, which could stop accepting new subflows. A fix for v5.12.

- Patches 13-14: do not let PM listeners pin their netns, leaked
  otherwise. A fix for v5.12.

- Patch 15: close a subflow reset because of a bad mapping, not to keep
  the MPTCP socket established without subflows. A fix for v5.12.

- Patch 16: initialize drop_req, not to leak sockets with SO_REUSEPORT
  migration. A fix for v5.14.

- Patches 17-18: count MPTCP listeners correctly in /proc/net/protocols,
  and send their creation event once. Fixes for another fix introduced
  in v6.4 and targeting v5.17, and for v6.2.

- Patches 19-20: avoid data delivered twice around a fallback to TCP,
  with old data in flight, or resent to a peer that fell back. Fixes for
  v5.19, and for another fix introduced in v6.12 and targeting v5.13.

- Patches 21-22: do not count a signal+subflow ID 0 endpoint as used at
  each re-creation, and validate it. A fix for other fixes introduced in
  v6.11 and v7.0, and targeting v6.0 and v6.11.

- Patches 23-26: with TFO, send the data written by a server before the
  third ACK as soon as this ACK arrives, also in case of fallback to
  TCP, even if the connection was shut down. Fixes for v6.2 and v5.19.

- Patch 27: do not pass a BPF request sock to an MPTCP listener, to
  avoid a slab-out-of-bounds write. A fix for v6.9.

- Patches 28-31: reclaim the forward memory after coalescing skbs, and
  do not overcharge the memcg at accept(). Fixes for v6.19.

- Patches 32-33: after a fallback, fix the dummy mapping of overlapping
  segments, and continue the last mapping. Fixes for v6.19.

- Patch 34: RFC for mptcp-next: reset the subflow when the DSS
  disappears after a DATA_ACK, instead of a fallback to TCP, as
  suggested by RFC 8684.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Chris Mason (16):
      tcp: drain the queues of a listen() that fails
      mptcp: drain the queues of a listen() that fails
      mptcp: clear inet_opt in the child of an IPv6 listener
      mptcp: arm the rtx timer if a push leaves data behind
      mptcp: force the charge for the first skb of a subflow
      mptcp: leave an orphan's DATA_FIN to the worker
      mptcp: drop non-MP_JOIN cookie ACKs on PM listeners
      mptcp: close a subflow reset for a bad mapping
      mptcp: initialize drop_req in subflow_init_req()
      mptcp: do not fallback while old data is in flight
      mptcp: stop resending data to a peer that fell back
      tcp: do not pass a BPF reqsk to an MPTCP listener
      mptcp: reclaim forward memory after coalescing skbs
      mptcp: don't charge a shared page twice at accept()
      mptcp: don't charge the backlog twice at accept()
      mptcp: continue the last mapping after a fallback

Geliang Tang (1):
      mptcp: fix dummy mapping of overlapping segments

Josef Bacik (5):
      mptcp: pm: work on a copy of the received RM_ADDR list
      mptcp: pm: don't drop an RM_ADDR behind a pending one
      mptcp: pm: don't drop an RM_ADDR because of an ADD_ADDR
      mptcp: pm: don't let listeners pin their netns
      mptcp: pm: kernel: don't count signal+subflow ID 0 endp

Matthieu Baerts (NGI0) (12):
      mptcp: don't leak an orphan when the PM closes its last subflow
      mptcp: do not use a raised snd_wnd for the send window
      net: add __sk_net_refcnt_upgrade() with a GFP mask
      mptcp: pm: kernel: count endpoint listeners as sockets in use
      mptcp: count a listener once when listen() is called again
      selftests: mptcp: join: check re-adding signal+subflow ID 0 endp
      mptcp: use sk instead of casting msk in check_fully_established()
      mptcp: push queued data on passive TFO subflows becoming established
      mptcp: push queued data on passive TFO subflows falling back to TCP
      mptcp: don't send a FIN before the pending data on fallback
      mptcp: don't charge a subflow twice when it joins during accept()
      [RFC mptcp-next] mptcp: reset when the DSS disappears after a DATA_ACK

 include/net/sock.h                              |   1 +
 net/core/sock.c                                 |   9 +-
 net/ipv4/inet_connection_sock.c                 |   8 +-
 net/ipv4/syncookies.c                           |   3 +-
 net/mptcp/options.c                             |  39 +++++-
 net/mptcp/pm.c                                  |  24 +++-
 net/mptcp/pm_kernel.c                           |   7 +-
 net/mptcp/protocol.c                            | 175 ++++++++++++++++--------
 net/mptcp/protocol.h                            |  23 +++-
 net/mptcp/subflow.c                             |  81 +++++++++--
 tools/testing/selftests/net/mptcp/mptcp_join.sh |  10 +-
 11 files changed, 290 insertions(+), 90 deletions(-)
---
base-commit: 168bc9ace0731cc00cfabb8198231e577df8efaf
base-tree: https://github.com/multipath-tcp/mptcp_net-next.git t/upstream
change-id: 20261110-mptcp-cmason-fixes-eec0054c880a

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

[PATCH mptcp-net 01/34] tcp: drain the queues of a listen() that fails

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

If listen() fails on a TCP socket that has listened before, the sockets
of connections from the earlier listen() can leak, together with their
network namespace.

inet_csk_listen_start() sets TCP_LISTEN before it calls get_port() and
hash(), and its error path only sets TCP_CLOSE again, which relies on
nothing reaching the socket before it is in the listener hash. But
pending request socks outlive an earlier listen() and still point to the
socket, so in TCP_LISTEN, inet_csk_reqsk_queue_add() adds them to the
accept queue and reqsk_fastopen_remove() adds them to the RST queue.
These entries, and the sockets that they hold, are never freed, since
close() drains the two queues only in TCP_LISTEN and the next listen()
stores NULL in both heads, rskq_accept_head and fastopenq.rskq_rst_head.

The fix used here is to call inet_csk_listen_stop() after the error path
sets TCP_CLOSE, except for an MPTCP subflow, which needs
mptcp_subflow_queue_clean() first: mptcp_listen() has to drain it, which
is done in the next patch, "mptcp: drain the queues of a listen() that
fails".

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch,
run as an unprivileged user: listen() failing with EADDRINUSE in a loop,
while the connections of an earlier listen() complete. Without this
patch, sockets were leaked, and network namespaces too when each round
was in its own one, without any message from the kernel:

  result: request socks: 10000 made, 3025 left. Sockets: 33205 made, 6050 left
  [...]
  result: namespaces: 1000 made, 735 taken down, 265 left

Fixes: 8336886f786f ("tcp: TCP Fast Open Server - support TFO listeners")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Shorter comments. Commit message: how the issue was found, and its
  symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/ipv4/inet_connection_sock.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 5807067083c9..3d9cc189c807 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -1355,8 +1355,8 @@ int inet_csk_listen_start(struct sock *sk)
 
 	/* There is race window here: we announce ourselves listening,
 	 * but this transition is still not validated by get_port().
-	 * It is OK, because this socket enters to hash table only
-	 * after validation is complete.
+	 * This socket enters to hash table only after validation is
+	 * complete, but reqs of an earlier listen() may queue children.
 	 */
 	inet_sk_state_store(sk, TCP_LISTEN);
 	err = sk->sk_prot->get_port(sk, inet->inet_num);
@@ -1371,6 +1371,10 @@ int inet_csk_listen_start(struct sock *sk)
 	}
 
 	inet_sk_set_state(sk, TCP_CLOSE);
+
+	/* MPTCP subflows are drained by mptcp_listen() */
+	if (!sk_is_mptcp(sk))
+		inet_csk_listen_stop(sk);
 	return err;
 }
 

-- 
2.55.0

[PATCH mptcp-net 02/34] mptcp: drain the queues of a listen() that fails

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

Sockets and their network namespace can leak, if a handshake completes
while listen() fails on an MPTCP socket that has listened before.

mptcp_listen() passes the first subflow through __inet_listen_sk() to
inet_csk_listen_start(), which sets TCP_LISTEN, then calls get_port()
and hash(), and sets TCP_CLOSE again if either fails. Until then the
subflow is in TCP_LISTEN, so a request sock of a previous listen() that
still points to it can be added to its Fast Open RST queue, or can get a
new child and be added to the subflow's accept queue. But neither
close() nor the next listen() frees what is queued, and the earlier
commit "tcp: drain the queues of a listen() that fails" skips an MPTCP
subflow, which needs mptcp_subflow_queue_clean() before a drain.

The fix used here is to call mptcp_subflow_queue_clean() and then
inet_csk_listen_stop() on the subflow in mptcp_listen(), if
__inet_listen_sk() returns an error with the subflow in TCP_CLOSE.

This issue has been found with the help of an LLM. It has not been
reproduced: the reproducer of the previous commit only uses TCP sockets.
This fix is based on code inspection only, which is acceptable here:
the issue is similar to the one fixed by the previous commit, and the
fix is simple.

Fixes: 76e2a55d1625 ("mptcp: better msk-level shutdown.")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Shorter comment. Add a Fixes tag. Commit message: how the issue was
  found, and why it is fine without a reproducer. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 50de5cedfd0f..dd9fb803cad5 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -4381,6 +4381,11 @@ static int mptcp_listen(struct socket *sock, int backlog)
 
 	lock_sock(ssk);
 	err = __inet_listen_sk(ssk, backlog);
+	if (err && ssk->sk_state == TCP_CLOSE) {
+		/* inet_csk_listen_start() leaves this drain to MPTCP */
+		mptcp_subflow_queue_clean(sk, ssk);
+		inet_csk_listen_stop(ssk);
+	}
 	release_sock(ssk);
 	mptcp_set_state(sk, inet_sk_state_load(ssk));
 

-- 
2.55.0

Re: [PATCH mptcp-net 02/34] mptcp: drain the queues of a listen() that fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] mptcp: missing inuse add and state reset in
  mptcp_pm_nl_create_listen_socket() [1]
--

commit 2094b28eb838bd9f43ac4fd932bbfb028ce12096
Author: Chris Mason <mason@kernel.org>

mptcp: drain the queues of a listen() that fails

This commit cleans up the subflow accept queue and Fast Open RST queue in
mptcp_listen() when __inet_listen_sk() fails and leaves the subflow in
TCP_CLOSE.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 50de5cedfd0fb..dd9fb803cad5f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -4381,6 +4381,11 @@ static int mptcp_listen(struct socket *sock, int backlog)
>  
>  	lock_sock(ssk);
>  	err = __inet_listen_sk(ssk, backlog);
> +	if (err && ssk->sk_state == TCP_CLOSE) {
> +		/* inet_csk_listen_start() leaves this drain to MPTCP */
> +		mptcp_subflow_queue_clean(sk, ssk);
> +		inet_csk_listen_stop(ssk);
> +	}
>  	release_sock(ssk);
>  	mptcp_set_state(sk, inet_sk_state_load(ssk));
>  

[Severity: High]
This problem wasn't introduced by this patch, but does the other
__inet_listen_sk() caller in mptcp_pm_nl_create_listen_socket() [1]
miss updating newsk's state and sock_prot_inuse_add()?

net/mptcp/pm_kernel.c:mptcp_pm_nl_create_listen_socket() {
	...
	inet_sk_state_store(newsk, TCP_LISTEN);
	sock_set_flag(newsk, SOCK_RCU_FREE);
	lock_sock(ssk);
	WRITE_ONCE(mptcp_subflow_ctx(ssk)->pm_listener, true);
	err = __inet_listen_sk(ssk, backlog);
	if (!err)
		mptcp_event_pm_listener(ssk, MPTCP_EVENT_LISTENER_CREATED);
	release_sock(ssk);
	return err;
}

Unlike mptcp_listen(), mptcp_pm_nl_create_listen_socket() does not call
sock_prot_inuse_add() on success or reset newsk from ssk's state when
__inet_listen_sk() fails.  When the PM listener socket is later closed via
__mptcp_pm_release_addr_entry() -> __mptcp_close() ->
mptcp_check_listen_stop(), seeing newsk in TCP_LISTEN decrements the
per-netns protocol in-use counter without a matching increment and triggers
WARN_ON_ONCE(inet_sk_state_load(ssk) != TCP_LISTEN) if __inet_listen_sk()
failed and left ssk in TCP_CLOSE.

[1] https://sashiko.dev/#/bug/linux-2c383603-dae6-4f8b-a0ea-d210d6d43b44

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-mptcp-cmason-fixes-v1-0-9205b77e9a2f@kernel.org?part=2

[PATCH mptcp-net 03/34] mptcp: clear inet_opt in the child of an IPv6 listener

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

With SELinux, an IPv6 MPTCP listener and its children can cause a double
free of one CIPSO option, and then a use-after-free read of it, if
NetLabel uses CIPSO for IPv4 and selects the IPv6 labels by address.

mptcp_sk_clone_init() creates the child of a listener with
sk_clone_lock(), as a byte copy of the listener, so each pointer in the
copy that the socket destructor frees has to be replaced or cleared.
mptcp_copy_ip_options() duplicates inet_opt, the IPv4 options, for
AF_INET, and mptcp_copy_ip6_options() duplicates the IPv6 options for
AF_INET6. But an AF_INET6 socket has an inet_opt pointer too, and its
child keeps the listener's pointer.

MPTCP does not support setsockopt(IP_OPTIONS), so that pointer is
usually NULL. But if connect() gets a struct sockaddr_in,
netlbl_conn_setattr() does not look at the family of the socket, so the
connect() hook of SELinux can store a CIPSO option in inet_opt of an
AF_INET6 MPTCP listener. inet_sock_destruct() then passes that option to
kfree_rcu() for each child and again for the listener, and after a child
has freed it, another such connect() on the listener reads it in
cipso_v4_sock_setattr().

The fix used here is to clear inet_opt of the child in
mptcp_copy_ip6_options().

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch,
with SELinux, NetLabel -- CIPSO for some IPv4 addresses, and the
address-based selection used for IPv6 too -- and KASAN: an unprivileged
user calls connect() with an IPv4 address on an IPv6 MPTCP listener.
Without this patch, an object is freed twice:

  BUG: KASAN: double-free in rcu_free_sheaf+0x23/0x100
  Free of addr ffff888012c27a80 by task kworker/1:1/50
  CPU: 1 UID: 0 PID: 50 Comm: kworker/1:1 Not tainted 7.4.0-rc1+ #1 PREEMPT(full)
  Workqueue: events mptcp_worker
  Call Trace:
   <IRQ>
   kasan_report_invalid_free+0xd4/0x1b0
   check_slab_allocation+0xc2/0xf0
   __rcu_free_sheaf_prepare+0x102/0x2a0
   rcu_free_sheaf+0x23/0x100
   rcu_do_batch+0x29a/0xf80
   rcu_core+0x33c/0x830
   handle_softirqs+0x212/0x980
   do_softirq+0xb4/0xf0
   </IRQ>
   <TASK>
   __local_bh_enable_ip+0x11c/0x150
   mptcp_token_destroy+0x1c9/0x360
   mptcp_destroy_common+0x2d9/0x430
   mptcp_destroy+0x86/0x110
   __mptcp_destroy_sock+0x176/0x350
   mptcp_worker+0xbb5/0xf80
  [...]
  Allocated by task 176:
  [...]
   cipso_v4_sock_setattr+0x9b/0x3d0
   netlbl_conn_setattr+0x3ac/0x500
   selinux_netlbl_socket_connect_locked+0x102/0x210
   selinux_netlbl_socket_connect+0x23/0x40
   security_socket_connect+0x2e/0x60
   __sys_connect_file+0xcb/0x200
  [...]

In another case, the CIPSO option is used after having been freed:

  BUG: KASAN: slab-use-after-free in cipso_v4_sock_setattr+0x3c7/0x3d0
  Read of size 1 at addr ffff8880161c2f18 by task python3/177
  CPU: 1 UID: 65534 PID: 177 Comm: python3 Not tainted 7.4.0-rc1+ #1 PREEMPT(full)
  Call Trace:
   <TASK>
   kasan_report+0x101/0x140
   cipso_v4_sock_setattr+0x3c7/0x3d0
   netlbl_conn_setattr+0x3ac/0x500
   selinux_netlbl_socket_connect_locked+0x102/0x210
   selinux_netlbl_socket_connect+0x23/0x40
   security_socket_connect+0x2e/0x60
   __sys_connect_file+0xcb/0x200
   __sys_connect+0xfd/0x130
   __x64_sys_connect+0x72/0xd0
  [...]

Fixes: cf7da0d66cc1 ("mptcp: Create SUBFLOW socket for incoming connections")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Commit message: how the issue was found, and its symptoms ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index dd9fb803cad5..e484f7edad49 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3718,6 +3718,9 @@ static void mptcp_copy_ip6_options(struct sock *newsk, const struct sock *sk)
 
 	newnp = inet6_sk(newsk);
 
+	/* The IPv4 options belong to the listener, which frees them */
+	RCU_INIT_POINTER(inet_sk(newsk)->inet_opt, NULL);
+
 	rcu_read_lock();
 	opt = rcu_dereference(np->opt);
 	if (opt) {

-- 
2.55.0

[PATCH mptcp-net 04/34] mptcp: pm: work on a copy of the received RM_ADDR list

[Matthieu Baerts (NGI0)]

From: Josef Bacik <josef@toxicpanda.com>

The worker clears MPTCP_PM_RM_ADDR_RECEIVED and walks pm.rm_list_rx in
place.  For every subflow it closes, mptcp_pm_rm_addr_or_subflow() drops
pm.lock.  If the peer's next RM_ADDR arrives then, the flag is clear and
mptcp_pm_rm_addr_received() overwrites the list.  The walk goes on at
its index in the new list: the ids of the first list that were still to
come are never handled, and their subflows stay.

Hand a copy of the list to the walk.

This issue has been found with the help of an LLM. It has not been
reproduced: the next RM_ADDR has to be processed while the worker has
dropped pm.lock to close a subflow, a window that a Packetdrill script
cannot target. This fix is based on code inspection only, which is
acceptable here: it is simple, and the issue is clear from the code.

Fixes: d0b698ca9a27 ("mptcp: remove multi addresses in PM")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
[ Commit message: how the issue was found, and why it is fine without
  a reproducer. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d7c5b50b34cc..cdc8bab0eaf8 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -827,7 +827,10 @@ static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
 
 static void mptcp_pm_rm_addr_recv(struct mptcp_sock *msk)
 {
-	mptcp_pm_rm_addr_or_subflow(msk, &msk->pm.rm_list_rx, MPTCP_MIB_RMADDR);
+	/* pm.lock is dropped on the way: the next RM_ADDR may come in */
+	struct mptcp_rm_list rm_list = msk->pm.rm_list_rx;
+
+	mptcp_pm_rm_addr_or_subflow(msk, &rm_list, MPTCP_MIB_RMADDR);
 }
 
 void mptcp_pm_rm_subflow(struct mptcp_sock *msk,

-- 
2.55.0

[PATCH mptcp-net 05/34] mptcp: pm: don't drop an RM_ADDR behind a pending one

[Matthieu Baerts (NGI0)]

From: Josef Bacik <josef@toxicpanda.com>

There is room for one received list of address ids.  An RM_ADDR that
arrives before the worker has taken the previous one is counted as
RmAddrDrop and forgotten, although the netlink events for its ids have
been sent already.  Two options follow each other at once if, for
instance, a path manager in user space removes two addresses of the
peer.  The subflows to the second address stay.

Add the new ids to the pending list.  Only the ids that do not fit in
the list of MPTCP_RM_IDS_MAX ids are dropped, and the RM_ADDR is then
counted as RmAddrDrop.

This issue has been found with the help of an LLM. It has not been
reproduced: the second RM_ADDR has to be processed before the PM worker
gets to the first one, a window that a Packetdrill script cannot
target. This fix is based on code inspection only, which is acceptable
here: the issue is clear from the code, and the fix is simple.

Fixes: d0876b2284cf ("mptcp: add the incoming RM_ADDR support")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
[ Commit message: only the ids that do not fit are dropped + how the
  issue was found, and why it is fine without a reproducer. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index cdc8bab0eaf8..32885e80486e 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -843,6 +843,7 @@ void mptcp_pm_rm_addr_received(struct mptcp_sock *msk,
 			       const struct mptcp_rm_list *rm_list)
 {
 	struct mptcp_pm_data *pm = &msk->pm;
+	bool drop = false;
 	u8 i;
 
 	pr_debug("msk=%p remote_ids_nr=%d\n", msk, rm_list->nr);
@@ -852,8 +853,20 @@ void mptcp_pm_rm_addr_received(struct mptcp_sock *msk,
 
 	spin_lock_bh(&pm->lock);
 	if (mptcp_pm_schedule_work(msk, MPTCP_PM_RM_ADDR_RECEIVED))
-		pm->rm_list_rx = *rm_list;
-	else
+		pm->rm_list_rx.nr = 0;
+
+	/* the worker may not have got to the previous RM_ADDR yet */
+	for (i = 0; i < rm_list->nr; i++) {
+		if (memchr(pm->rm_list_rx.ids, rm_list->ids[i],
+			   pm->rm_list_rx.nr))
+			continue;
+		if (pm->rm_list_rx.nr == MPTCP_RM_IDS_MAX) {
+			drop = true;
+			break;
+		}
+		pm->rm_list_rx.ids[pm->rm_list_rx.nr++] = rm_list->ids[i];
+	}
+	if (drop)
 		__MPTCP_INC_STATS(sock_net((struct sock *)msk), MPTCP_MIB_RMADDRDROP);
 	spin_unlock_bh(&pm->lock);
 }

-- 
2.55.0

[PATCH mptcp-net 06/34] mptcp: pm: don't drop an RM_ADDR because of an ADD_ADDR

[Matthieu Baerts (NGI0)]

From: Josef Bacik <josef@toxicpanda.com>

mptcp_pm_remove_addr() gives up, and counts RmAddrTxDrop, if any bit of
pm.addr_signal is set. That dates from the time when the byte was
cleared as a whole after a signal had been sent. Since
commit 119c022096f5 ("mptcp: fix ADD_ADDR and RM_ADDR maybe flush
addr_signal each other") an ADD_ADDR, an echo and an RM_ADDR each clear
their own bit, and they have storage of their own: pm.local, pm.remote
and pm.rm_list_tx. mptcp_pm_announce_addr() looks at its own bit only.

The callers have updated their state by then and ignore the result: the
endpoint is gone, the announcement is forgotten, and the peer never
learns that the address is not valid any more.

When the bit of an echo is set at the arrival of the peer's ADD_ADDR,
it stays set until the PM worker has sent the ACK. An endpoint that is
removed in between loses its RM_ADDR.

Only a pending RM_ADDR stands in the way, as rm_list_tx would be
overwritten. The ACK that is sent at once carries the ADD_ADDR or the
echo then, and the RM_ADDR goes with the next packet, e.g. the ACK that
the PM worker sends for the pending echo.

This issue has been found with the help of an LLM. It has not been
reproduced: the endpoint has to be removed before the PM worker sends
the pending ADD_ADDR or echo, a window that a Packetdrill script cannot
target. This fix is based on code inspection only, which is acceptable
here: the issue is clear from the code, and the fix is a one-liner.

Fixes: 119c022096f5 ("mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
[ Use mptcp_pm_should_rm_signal() instead of a comment. Fixes tag:
  119c022096f5 instead of 42842a425ad6. Commit message: remove the
  number of attempts, say which packet carries the RM_ADDR then, and
  when the echo bit is set + how the issue was found, and why it is fine
  without a reproducer. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 32885e80486e..5f844b9a32d8 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -529,7 +529,7 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
 
 	pr_debug("msk=%p, rm_list_nr=%d\n", msk, rm_list->nr);
 
-	if (rm_addr) {
+	if (mptcp_pm_should_rm_signal(msk)) {
 		MPTCP_ADD_STATS(sock_net((struct sock *)msk),
 				MPTCP_MIB_RMADDRTXDROP, rm_list->nr);
 		return -EINVAL;

-- 
2.55.0

[PATCH mptcp-net 07/34] mptcp: arm the rtx timer if a push leaves data behind

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

In a memory shortage, send() on an idle MPTCP socket can succeed but
leave its data unsent, and the connection stays stalled after the
shortage has ended.

send() puts its data in the write queue of the MPTCP socket, and the
push to a subflow leaves the data there if the memory charge to the
subflow is refused or no skb can be allocated. A push that leaves data
in the write queue has to be tried again, on the acknowledgment of data
in flight or from the handler of the MPTCP retransmission (rtx) timer.
But __mptcp_push_pending() and __mptcp_subflow_push_pending() arm that
timer only if they have pushed data, so with no data in flight and no
timer pending, the data waits for another event, such as the next
send().

The fix used here is to also arm the timer, in both functions, if unsent
data is left in the write queue.

This does not help a socket that has fallen back to TCP, as its rtx
timer is disabled since commit e2ab913f68c7 ("mptcp: do not reschedule
the RTX timer for fallback sockets"). "mptcp: force the charge for the
first skb of a subflow" avoids a refused charge in this case, but not a
failed skb allocation.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch,
with fault injection: one skb allocation made to fail during a send().
Without this patch, the data accepted by this send() was only sent with
the next one, 60 seconds later here:

  3.007 s: CLIENT send() of 1,000 bytes returned 1000. Allocations that were made to fail in it: 1
  [...]
  63.008 s: CLIENT the second send(), of 100 bytes, returned 100
  63.008 s: SERVER has 1200 bytes
  RESULT-FAILSLAB [...] the_1000_bytes_at_the_server_at=never [...]
  [...] arrived_before_the_second_send=no [...]

Fixes: 2ea6190f42d0 ("mptcp: schedule rtx timer only after pushing data")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Shorter comment. Commit message: sockets that fell back to TCP are not
  covered + how the issue was found, and its symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index e484f7edad49..dc8cdda31a98 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1850,14 +1850,11 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
 	if (ssk)
 		mptcp_push_release(ssk, &info);
 
-	/* Avoid scheduling the rtx timer if no data has been pushed; the timer
-	 * will be updated on positive acks by __mptcp_cleanup_una().
-	 */
-	if (copied) {
-		if (!mptcp_rtx_timer_pending(sk))
-			mptcp_reset_rtx_timer(sk);
+	/* also retry later if unsent data is left: no ack may trigger it */
+	if ((copied || mptcp_send_head(sk)) && !mptcp_rtx_timer_pending(sk))
+		mptcp_reset_rtx_timer(sk);
+	if (copied)
 		mptcp_check_send_data_fin(sk);
-	}
 }
 
 static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool first)
@@ -1919,13 +1916,15 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool
 	if (copied) {
 		tcp_push(ssk, 0, info.mss_now, tcp_sk(ssk)->nonagle,
 			 info.size_goal);
-		if (!mptcp_rtx_timer_pending(sk))
-			mptcp_reset_rtx_timer(sk);
 
 		if (msk->snd_data_fin_enable &&
 		    msk->snd_nxt + 1 == msk->write_seq)
 			mptcp_schedule_work(sk);
 	}
+
+	/* see __mptcp_push_pending() */
+	if ((copied || mptcp_send_head(sk)) && !mptcp_rtx_timer_pending(sk))
+		mptcp_reset_rtx_timer(sk);
 }
 
 static int mptcp_disconnect(struct sock *sk, int flags);

-- 
2.55.0

[PATCH mptcp-net 08/34] mptcp: force the charge for the first skb of a subflow

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

After a fallback to TCP, an MPTCP connection can stay stalled once TCP
memory use is back below the hard limit of net.ipv4.tcp_mem, and above
that limit send() on an idle MPTCP socket can leave its data unsent.

A push moves data from the write queue of the MPTCP socket to a subflow,
and charges the subflow twice: for a new skb in __mptcp_alloc_tx_skb()
and for the payload in mptcp_sendmsg_frag(). On an idle socket, with
nothing queued, TCP forces both charges past the limit, in
tcp_stream_alloc_skb() and tcp_wmem_schedule(), since no ACK would ever
restart such a socket. But a push forces neither, so above the hard
limit both can be refused, and the push then moves no data.

The earlier commit "mptcp: arm the rtx timer if a push leaves data
behind" repeats such a push from the MPTCP retransmission (rtx) timer.
But each attempt fails while memory use stays above the limit. After a
fallback that timer is disabled, so the data stays unsent until another
event, such as the next send(), repeats the push.

The fix used here is to:
- force the charge for the skb if the subflow has nothing queued
- charge the payload with tcp_wmem_schedule()
- send only the bytes that tcp_wmem_schedule() grants

The first skb of an MPTCP-level reinjection is then forced too, as it is
sent on a subflow with nothing queued. This is bounded the same way.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch:
a TCP memory shortage, for 30 seconds, during which each of ten idle
MPTCP clients calls send() once. Without this patch, only 5 of them had
their answer during the shortage, versus 8 with it: the 2 others are
blocked in send() during the shortage, which is not fixed here. Some
answers only came after the next send(), 300 seconds later, because of
the issue fixed by the previous commit:

  IDLE label=behind-the-second-send t=338.9 conn=0 [...] answer_at=336.4
  IDLE label=behind-the-second-send t=338.9 conn=1 [...] answer_at=4.2

Fixes: 724cfd2ee8aa ("mptcp: allocate TX skbs in msk context")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Shorter comments. Commit message: the first skb of a reinjection is
  forced too + how the issue was found, and its symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index dc8cdda31a98..8d6ed293f6e5 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1387,13 +1387,17 @@ static struct sk_buff *__mptcp_alloc_tx_skb(struct sock *sk, struct sock *ssk, g
 	if (!skb)
 		return NULL;
 
-	if (likely(sk_wmem_schedule(ssk, skb->truesize))) {
-		tcp_skb_entail(ssk, skb);
-		return skb;
+	/* like TCP, force the charge if nothing is queued */
+	if (tcp_rtx_and_write_queues_empty(ssk)) {
+		sk_forced_mem_schedule(ssk, skb->truesize);
+	} else if (unlikely(!sk_wmem_schedule(ssk, skb->truesize))) {
+		tcp_skb_tsorted_anchor_cleanup(skb);
+		kfree_skb(skb);
+		return NULL;
 	}
-	tcp_skb_tsorted_anchor_cleanup(skb);
-	kfree_skb(skb);
-	return NULL;
+
+	tcp_skb_entail(ssk, skb);
+	return skb;
 }
 
 static struct sk_buff *mptcp_alloc_tx_skb(struct sock *sk, struct sock *ssk, bool data_lock_held)
@@ -1449,6 +1453,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	bool reuse_skb = true;
 	struct sk_buff *skb;
 	size_t copy;
+	int avail;
 	int i;
 
 	pr_debug("msk=%p ssk=%p sending dfrag at seq=%llu len=%u already sent=%u\n",
@@ -1522,10 +1527,14 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	}
 
 	copy = min_t(size_t, copy, info->limit - info->sent);
-	if (!sk_wmem_schedule(ssk, copy)) {
+
+	/* like TCP, allow some progress up to tcp_wmem[0] */
+	avail = tcp_wmem_schedule(ssk, copy);
+	if (avail <= 0) {
 		tcp_remove_empty_skb(ssk);
 		return -ENOMEM;
 	}
+	copy = avail;
 
 	if (can_coalesce) {
 		skb_frag_size_add(&skb_shinfo(skb)->frags[i - 1], copy);

-- 
2.55.0

Re: [PATCH mptcp-net 08/34] mptcp: force the charge for the first skb of a subflow

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] mptcp: cross-subflow state leak in __mptcp_push_pending()
  [1]
--

commit 045933b0c1ae73b56c16499f1fb80d8244a44751
Author: Chris Mason <mason@kernel.org>

mptcp: force the charge for the first skb of a subflow

This commit forces the memory charge when allocating the first skb on an
empty MPTCP subflow and uses tcp_wmem_schedule() to bound the payload copy
when charging subflow write memory in mptcp_sendmsg_frag().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index dc8cdda31a984..8d6ed293f6e58 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -1449,6 +1453,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
>  	bool reuse_skb = true;
>  	struct sk_buff *skb;
>  	size_t copy;
> +	int avail;
>  	int i;
>  
>  	pr_debug("msk=%p ssk=%p sending dfrag at seq=%llu len=%u already sent=%u\n",

[Severity: Critical]
This problem wasn't introduced by this patch, but can __mptcp_push_pending()
leak stale info.mss_now and info.size_goal across subflows [1] when
mptcp_sendmsg_frag() returns early?

__mptcp_push_pending() shares a single struct mptcp_sendmsg_info across its
subflow loop without resetting it when switching subflows:

net/mptcp/protocol.c:__mptcp_push_pending() {
	struct mptcp_sendmsg_info info = {
				.flags = flags,
	};
    ...
		mptcp_for_each_subflow(msk, subflow) {
			if (READ_ONCE(subflow->scheduled)) {
    ...
				if (ssk != prev_ssk) {
					if (prev_ssk)
						mptcp_push_release(prev_ssk, &info);
					lock_sock(ssk);
				}
    ...
				ret = __subflow_push_pending(sk, ssk, &info);
				if (ret <= 0) {
    ...
					continue;
				}
    ...
	if (ssk)
		mptcp_push_release(ssk, &info);
}

After mptcp_sendmsg_frag() populates info.mss_now and info.size_goal for one
subflow, a subsequent subflow can fail in mptcp_sendmsg_frag() at
!__tcp_can_send(ssk) before tcp_send_mss() overwrites those fields.  When
mptcp_push_release() runs for that second subflow, it sees the first
subflow's non-zero info.mss_now and calls tcp_push() with the wrong
mss_now and size_goal, causing tcp_write_xmit() to segment queued frames
with another subflow's segment size and exceed the path mtu.

[1] https://sashiko.dev/#/bug/linux-3f08f285-058d-4038-8cd3-6d287fea7814

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-mptcp-cmason-fixes-v1-0-9205b77e9a2f@kernel.org?part=8

[PATCH mptcp-net 09/34] mptcp: leave an orphan's DATA_FIN to the worker

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

Closed MPTCP sockets can leak until reboot, with the data in their
receive queues, which can exhaust tcp_mem and stall other TCP
connections.

Once close() has returned, only mptcp_worker() destroys an orphaned msk,
and only in the run that moves it to TCP_CLOSE: a later run returns at
once, and mptcp_schedule_work() queues no work for an msk in that state.
The receive path, in softirq, puts data on the backlog while the msk is
owned, and release_sock() calls mptcp_release_cb(), which passes the
backlog to __mptcp_move_skbs(). But __mptcp_move_skbs() calls
mptcp_check_data_fin(), which can move the msk to TCP_CLOSE, and by then
the lock owner, here the worker, no longer looks at the state:

    receive path, softirq             mptcp_worker()
    =====================             ==============
                                      not in TCP_CLOSE: no destroy
    last data, with the DATA_FIN
      the msk is owned: backlog
                                      release_sock()
                                        mptcp_release_cb()
                                          __mptcp_move_skbs()
                                            mptcp_check_data_fin()
                                              TCP_CLOSE, never destroyed

The fix used here is to queue the worker in __mptcp_move_skbs() if an
orphaned msk has a DATA_FIN to handle, instead of calling
mptcp_check_data_fin().

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch:
200,000 connections in which the server accept()s and close()s, while
the client reads the end of file, sends one byte, and closes. Without
this patch, some MPTCP sockets of the server stayed in the CLOSE state
for good, with their network namespace:

  === connections so far : MPTCP sockets of the server in the state CLOSE: 200000:722,
  [...]
  RESULT [...] | left 300 s behind the end: slab_MPTCP=722 sockets_MPTCP=722 [...]
  [...] | veth_end_left=1 slab_net_namespace_left=2 | [...]

Fixes: bc68b0efa1bf ("mptcp: move the whole rx path under msk socket lock protection")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Shorter comment. Commit message: how the issue was found, and its
  symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 8d6ed293f6e5..16824bed85b4 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2363,9 +2363,15 @@ static bool __mptcp_move_skbs(struct sock *sk, struct list_head *skbs, u32 *delt
 	}
 
 	__mptcp_ofo_queue(msk);
-	if (moved)
-		mptcp_check_data_fin((struct sock *)msk);
-	return moved;
+	if (!moved)
+		return false;
+
+	/* Only the worker can close an orphan, as it must then destroy it */
+	if (!sock_flag(sk, SOCK_DEAD))
+		mptcp_check_data_fin(sk);
+	else if (mptcp_pending_data_fin(sk, NULL))
+		mptcp_schedule_work(sk);
+	return true;
 }
 
 static bool mptcp_can_spool_backlog(struct sock *sk, struct list_head *skbs)

-- 
2.55.0

[PATCH mptcp-net 10/34] mptcp: don't leak an orphan when the PM closes its last subflow

[Matthieu Baerts (NGI0)]

Once close() has returned, an orphaned MPTCP socket is only destroyed by
mptcp_worker(), in the run that sees it in TCP_CLOSE: later on, the
worker returns at once for an msk in this state, and
mptcp_schedule_work() no longer queues it.

__mptcp_close_ssk() moves an orphan to TCP_CLOSE when, after the closure
of a subflow, only the initial one remains, and it is closed. This
function is not only called by the worker: path manager commands also
close subflows of any msk of the netns, orphans included, under the msk
lock: e.g. 'ip mptcp endpoint delete' or 'flush' with the in-kernel PM,
or MPTCP_PM_CMD_SUBFLOW_DESTROY with the userspace one. In this case,
nothing destroys the orphan: it stays in TCP_CLOSE until the reboot,
with its token, its initial subflow, and its reference to the netns,
which is then never freed.

This can happen when the initial subflow has been reset or has timed
out, and the connection is closed -- the orphan then waits for the end
of the DATA_FIN exchange, up to TCP_TIMEWAIT_LEN -- before the endpoint
used by another subflow is removed, e.g. when an interface goes down.

Only the worker can close an orphan, as it must then destroy it. So
here, mark its close timeout as expired, and queue the worker, like it
is done for an msk in the accept queue. The worker then closes the
orphan with mptcp_do_fastclose() -- nothing is sent: the only subflow
left is closed -- and destroys it. If __mptcp_close_ssk() was called by
the worker, this happens later in the same run.

This issue has been found by an AI code review agent, while reviewing
the code of the commit "mptcp: leave an orphan's DATA_FIN to the
worker", and confirmed with a new Packetdrill test: the initial subflow
of an orphan waiting for the end of the DATA_FIN exchange is reset, then
the endpoint of the other subflow is removed. Without this patch, the
orphan stayed:

  UNCONN 0      0      192.168.0.1:8080 192.0.2.1:39933
  kernel_orphan_last_subflow.pkt:39: error executing `! ss -HMan | grep -v LISTEN` command: non-zero status 1

Fixes: 27e5ccc2d5a5 ("mptcp: fix dangling connection hang-up")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 16824bed85b4..6dc55a3f027b 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2794,8 +2794,13 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 	 */
 	if (list_is_singular(&msk->conn_list) && msk->first &&
 	    inet_sk_state_load(msk->first) == TCP_CLOSE) {
-		if (sk->sk_state != TCP_ESTABLISHED ||
-		    msk->in_accept_queue || sock_flag(sk, SOCK_DEAD)) {
+		if (sock_flag(sk, SOCK_DEAD)) {
+			/* let the worker close and destroy the orphan */
+			mptcp_set_close_tout(sk, tcp_jiffies32 -
+					     (mptcp_close_timeout(sk) + 1));
+			mptcp_schedule_work(sk);
+		} else if (sk->sk_state != TCP_ESTABLISHED ||
+			   msk->in_accept_queue) {
 			mptcp_set_state(sk, TCP_CLOSE);
 			mptcp_close_wake_up(sk);
 		} else {

-- 
2.55.0

Re: [PATCH mptcp-net 10/34] mptcp: don't leak an orphan when the PM closes its last subflow

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] mptcp: skipped TCP_CLOSE cleanup for msk->first in
  mptcp_close_ssk() [1]
--

commit 9c49fccbfab6fbc60fdc19e95bb13b504edf3ebc
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

mptcp: don't leak an orphan when the PM closes its last subflow

This commit expires the close timeout and schedules mptcp_worker() when
__mptcp_close_ssk() leaves an orphaned MPTCP socket with only a closed
initial subflow, instead of moving it directly to TCP_CLOSE.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 16824bed85b42..6dc55a3f027bd 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2794,8 +2794,13 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  	 */
>  	if (list_is_singular(&msk->conn_list) && msk->first &&
>  	    inet_sk_state_load(msk->first) == TCP_CLOSE) {
> -		if (sk->sk_state != TCP_ESTABLISHED ||
> -		    msk->in_accept_queue || sock_flag(sk, SOCK_DEAD)) {
> +		if (sock_flag(sk, SOCK_DEAD)) {
> +			/* let the worker close and destroy the orphan */
> +			mptcp_set_close_tout(sk, tcp_jiffies32 -
> +					     (mptcp_close_timeout(sk) + 1));
> +			mptcp_schedule_work(sk);
> +		} else if (sk->sk_state != TCP_ESTABLISHED ||
> +			   msk->in_accept_queue) {
>  			mptcp_set_state(sk, TCP_CLOSE);
>  			mptcp_close_wake_up(sk);
>  		} else {

[Severity: High]
This problem wasn't introduced by this patch, but can mptcp_close_ssk() skip
this TCP_CLOSE cleanup for msk->first when it is half-closed first [1]?

When msk->first is gracefully closed before reaching TCP_CLOSE (for example,
on a subflow-level FIN without DATA_FIN or via mptcp_pm_rm_addr_or_subflow())
while msk->free_first and msk->fastclosing are false, mptcp_close_ssk() sets
subflow->close_event_done = true and __mptcp_close_ssk() calls
__mptcp_subflow_disconnect(), which only runs tcp_shutdown(ssk, SEND_SHUTDOWN)
before jumping to out: while msk->first is still not in TCP_CLOSE:

net/mptcp/protocol.c:__mptcp_subflow_disconnect() {
	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
	    fastclosing) {
    ...
		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
		mptcp_subflow_ctx_reset(subflow);
	} else {
		tcp_shutdown(ssk, SEND_SHUTDOWN);
	}
}

net/mptcp/protocol.c:mptcp_close_ssk() {
	if (subflow->close_event_done || READ_ONCE(subflow->local_id) < 0)
		return;

	subflow->close_event_done = true;
    ...
	__mptcp_close_ssk(sk, ssk, subflow, MPTCP_CF_PUSH);
}

When the FIN handshake later completes and msk->first reaches TCP_CLOSE,
__mptcp_close_subflow() calls mptcp_close_ssk() again, which returns early
because subflow->close_event_done is already set.  As a result, msk->first is
never disconnected or reset, and if it is the last subflow in msk->conn_list,
the parent msk never starts its close timeout timer or transitions to
TCP_CLOSE.

[1] https://sashiko.dev/#/bug/linux-47cd2e29-62e6-4ad6-9246-4044e6d4b0e9

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-mptcp-cmason-fixes-v1-0-9205b77e9a2f@kernel.org?part=10

[PATCH mptcp-net 11/34] mptcp: do not use a raised snd_wnd for the send window

[Matthieu Baerts (NGI0)]

ack_update_msk() sets the right edge of the MPTCP send window from the
DATA_ACK and tcp_sk(ssk)->snd_wnd, but this is not always what the peer
announced: mptcp_check_allowed_size() raises it to
window_end - data_seq, so that TCP lets through what the MPTCP-level
window allows.

That is fine for new data. For data sent again, data_seq can be below
snd_una, and snd_wnd ends up too large by the difference. TCP only
overwrites it when an ACK moves things forward on this subflow, see
tcp_may_update_window(). If a DATA_ACK moved by another subflow comes in
on this subflow before that, wnd_end goes past what the peer offered. It
only moves forward, so it stays there.

The sender then transmits data beyond the peer's window: the peer drops
it (MPTCP_MIB_NODSSWINDOW) after TCP acked it, and only an MPTCP-level
retransmission can recover it, which raises snd_wnd again. The
connection can then stay stuck at a low rate for several seconds, with
nothing in flight on any subflow.

This raise has been added by commit b713d0067574 ("mptcp: really share
subflow snd_wnd"), but snd_wnd could already be too large before: when
nothing moves at the TCP level, tcp_may_update_window() ignores a lower
non-zero window, while the peer can announce one with a DATA_ACK that
moved, keeping the same right edge.

Use the window announced in the packet carrying the DATA_ACK instead.

In my case with BBR, a small TCP rmem, and a second path with a much
higher latency, a lower throughput and some losses, this small change
can double the throughput in the worst cases.

The issue has been reproduced with a new Packetdrill test, on a single
subflow: the data is acked by TCP, then by MPTCP with a lower window,
keeping the same right edge. Without this patch, the 2000 bytes written
next were sent, instead of the 1280 bytes up to this right edge:

  dss_dack_window_server.pkt:23: error handling packet: live packet payload: expected 1280 bytes vs actual 2000 bytes

Fixes: 6f8a612a33e4 ("mptcp: keep track of advertised windows right edge")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/options.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 71579238a951..b4b3292fb4dc 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1121,9 +1121,11 @@ static void rwin_update(struct mptcp_sock *msk, struct sock *ssk,
 
 static void ack_update_msk(struct mptcp_sock *msk,
 			   struct sock *ssk,
+			   const struct sk_buff *skb,
 			   struct mptcp_options_received *mp_opt)
 {
 	u64 new_wnd_end, new_snd_una, old_snd_una;
+	u32 win = ntohs(tcp_hdr(skb)->window);
 	struct sock *sk = (struct sock *)msk;
 	u64 snd_nxt;
 
@@ -1144,7 +1146,12 @@ static void ack_update_msk(struct mptcp_sock *msk,
 		     after64(new_snd_una, snd_nxt)))
 		new_snd_una = old_snd_una;
 
-	new_wnd_end = new_snd_una + tcp_sk(ssk)->snd_wnd;
+	/* What the peer announced: tcp_sk(ssk)->snd_wnd can be larger, see
+	 * mptcp_check_allowed_size(), and is not always updated.
+	 */
+	if (likely(!tcp_hdr(skb)->syn))
+		win <<= tcp_sk(ssk)->rx_opt.snd_wscale;
+	new_wnd_end = new_snd_una + win;
 
 	if (after64(new_wnd_end, msk->wnd_end))
 		WRITE_ONCE(msk->wnd_end, new_wnd_end);
@@ -1314,7 +1321,7 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 	 * monodirectional flows will stuck
 	 */
 	if (mp_opt.use_ack)
-		ack_update_msk(msk, sk, &mp_opt);
+		ack_update_msk(msk, sk, skb, &mp_opt);
 	rwin_update(msk, sk, skb);
 
 	/* Zero-data-length packets are dropped by the caller and not

-- 
2.55.0

[PATCH mptcp-net 12/34] mptcp: drop non-MP_JOIN cookie ACKs on PM listeners

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

With SYN cookies enabled, a port-based MPTCP signal endpoint can stop
accepting new subflows until it is deleted.

Nobody calls accept() on the listener that the in-kernel path manager
opens for such an endpoint, so it must create only MP_JOIN subflows,
which never enter the accept queue. The listener is marked pm_listener,
and since commit 3d041393ea8c ("mptcp: prevent MPC handshake on
port-based signal endpoints") subflow_check_req() refuses a SYN with no
MP_JOIN option on it. But a SYN cookie does not record the MPTCP option
of the SYN, so mptcp_subflow_init_cookie_req() picks the kind of child
from the options of the ACK, without looking at pm_listener:

    ACK with a valid cookie and no MP_JOIN option
      creates a plain TCP child, or an MPTCP one if it has MP_CAPABLE
        which stays on the accept queue until the endpoint is deleted
          once the queue is full, the listener drops MP_JOIN SYNs too

The fix used here is to refuse an ACK with no MP_JOIN option on such a
listener in mptcp_subflow_init_cookie_req(), so that the request sock is
freed and the ACK is dropped.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch:
a client sent 1025 ACKs with valid SYN cookies and no MP_JOIN to the
listener of such an endpoint. Without this patch, its accept queue
stayed full, and another client could not create its additional subflow,
even 130 seconds later:

  TCP: request_sock_subflow_v4: Possible SYN flooding on port 10.9.0.2:6666. Sending cookies.
  [...]
  C2         4.007  [130 s after C1 has gone] RESULT: subflows established 1, in SYN_SENT 1, the service works
  --- V, after that try of C2
      listener 10.9.0.2:6666 Recv-Q 1025 Send-Q 1024

Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Shorter comment, the details are in the commit message. Commit
  message: how the issue was found, and its symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/subflow.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 00677fb3efcd..14e4557c3a9c 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -312,6 +312,11 @@ int mptcp_subflow_init_cookie_req(struct request_sock *req,
 
 	opt_mp_capable = !!(mp_opt.suboptions & OPTION_MPTCP_MPC_ACK);
 	opt_mp_join = !!(mp_opt.suboptions & OPTION_MPTCP_MPJ_ACK);
+
+	/* PM listeners only take MP_JOIN, like in subflow_check_req() */
+	if (unlikely(listener->pm_listener) && !opt_mp_join)
+		return subflow_reset_req_endp(req, skb);
+
 	if (opt_mp_capable && listener->request_mptcp) {
 		if (mp_opt.sndr_key == 0)
 			return -EINVAL;

-- 
2.55.0

[PATCH mptcp-net 13/34] net: add __sk_net_refcnt_upgrade() with a GFP mask

[Matthieu Baerts (NGI0)]

sk_net_refcnt_upgrade() gives a kernel socket a reference on its network
namespace, with a netns tracker allocated with GFP_KERNEL. The next
commit needs to do that for a socket created in atomic context: the
child of a kernel MPTCP listener, created when an MP_JOIN request is
accepted.

Add __sk_net_refcnt_upgrade(), taking the GFP mask to use, and make
sk_net_refcnt_upgrade() call it with GFP_KERNEL: no functional change
for the existing callers.

The 'Fixes' tag here below is the same as the one from the next commit:
this patch is needed to fix the issue introduced by this commit ID.

Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 include/net/sock.h | 1 +
 net/core/sock.c    | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index d58d38db732d..1f64f08a8131 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1847,6 +1847,7 @@ static inline bool sock_allow_reclassification(const struct sock *csk)
 struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
 		      struct proto *prot, int kern);
 void sk_free(struct sock *sk);
+void __sk_net_refcnt_upgrade(struct sock *sk, gfp_t gfp);
 void sk_net_refcnt_upgrade(struct sock *sk);
 void sk_destruct(struct sock *sk);
 struct sock *sk_clone(const struct sock *sk, const gfp_t priority, bool lock);
diff --git a/net/core/sock.c b/net/core/sock.c
index e529ffeaedb9..c9fa717d953e 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2434,7 +2434,7 @@ static void __sk_destruct(struct rcu_head *head)
 	sk_prot_free(sk->sk_prot_creator, sk);
 }
 
-void sk_net_refcnt_upgrade(struct sock *sk)
+void __sk_net_refcnt_upgrade(struct sock *sk, gfp_t gfp)
 {
 	struct net *net = sock_net(sk);
 
@@ -2442,9 +2442,14 @@ void sk_net_refcnt_upgrade(struct sock *sk)
 	__netns_tracker_free(net, &sk->ns_tracker, false);
 	net_passive_dec(net);
 	sk->sk_net_refcnt = 1;
-	get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
+	get_net_track(net, &sk->ns_tracker, gfp);
 	sock_inuse_add(net, 1);
 }
+
+void sk_net_refcnt_upgrade(struct sock *sk)
+{
+	__sk_net_refcnt_upgrade(sk, GFP_KERNEL);
+}
 EXPORT_SYMBOL_GPL(sk_net_refcnt_upgrade);
 
 void sk_destruct(struct sock *sk)

-- 
2.55.0

[PATCH mptcp-net 14/34] mptcp: pm: don't let listeners pin their netns

[Matthieu Baerts (NGI0)]

From: Josef Bacik <josef@toxicpanda.com>

For a "signal" endpoint with a port, the in-kernel path manager creates a
listening MPTCP socket with sock_create_kern(). This socket is released
when the endpoint is deleted or, failing that, by pm_nl_exit_net() when the
network namespace is dismantled.

The MPTCP socket itself holds no reference on the namespace. But its first
subflow is created by mptcp_subflow_create_socket(), which always calls
sk_net_refcnt_upgrade(). So the namespace cannot be dismantled before
the listener is released, and the listener is not released before the
namespace is dismantled: unless somebody deletes the endpoint, both stay
around for ever once the last process of the namespace is gone.

  unshare -Urn sh -c 'ip link set lo up;
                      ip mptcp endpoint add 127.0.0.1 port 4321 signal'

leaks a namespace, with everything it contains, each time it is run. It
remains charged to the user's max_net_namespaces limit.

Only take the reference if the MPTCP socket has one. A listening TCP
socket has no timer of its own, and request sockets of a dead namespace are
taken care of by inet_twsk_purge().

The sockets cloned from that listener for incoming MP_JOIN requests do
need the reference, for their timers, but inherit sk_net_refcnt == 0. Take
it when they are created, with __sk_net_refcnt_upgrade(), from the
previous commit, as this is done in atomic context, and drop the
connection request if the namespace is already going away.

The TCP listener of such an endpoint is now a plain kernel socket, like
the MPTCP socket it belongs to: it is not counted in "sockets: used" in
/proc/net/sockstat any more, and its destruction is not broadcast to the
listeners of the SKNLGRP_INET*_TCP_DESTROY sock_diag groups.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch:
the command above, run 1000 times by an unprivileged user. Without this
patch, all the network namespaces were leaked:

  result: 1000 network namespaces made, 0 taken down, 1000 are left

Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
[ Use __sk_net_refcnt_upgrade(), from the previous commit, instead of an
  open-coded copy of sk_net_refcnt_upgrade(), with maybe_get_net() like
  rds_tcp_tune(). Shorter comments, and describe the condition: any
  kernel msk. Commit message: how the issue was found, and its
  symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/subflow.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 14e4557c3a9c..d23da4ac1968 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -849,6 +849,22 @@ void __mptcp_subflow_fully_established(struct mptcp_sock *msk,
 	WRITE_ONCE(msk->fully_established, true);
 }
 
+/* Children of a kernel listener need a netns ref for their timers */
+static bool subflow_child_get_net(struct sock *child)
+{
+	struct net *net = sock_net(child);
+
+	if (likely(child->sk_net_refcnt))
+		return true;
+
+	if (!maybe_get_net(net))
+		return false;
+
+	__sk_net_refcnt_upgrade(child, GFP_ATOMIC);
+	put_net(net);
+	return true;
+}
+
 static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 					  struct sk_buff *skb,
 					  struct request_sock *req,
@@ -917,6 +933,11 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 
 		tcp_rsk(req)->drop_req = false;
 
+		if (!subflow_child_get_net(child)) {
+			subflow_add_reset_reason(skb, MPTCP_RST_EUNSPEC);
+			goto dispose_child;
+		}
+
 		/* we need to fallback on ctx allocation failure and on pre-reqs
 		 * checking above. In the latter scenario we additionally need
 		 * to reset the context to non MPTCP status.
@@ -1865,8 +1886,10 @@ int mptcp_subflow_create_socket(struct sock *sk, unsigned short family,
 	/* kernel sockets do not by default acquire net ref, but TCP timer
 	 * needs it.
 	 * Update ns_tracker to current stack trace and refcounted tracker.
+	 * Not for a kernel msk, e.g. a PM listener: it would pin the netns.
 	 */
-	sk_net_refcnt_upgrade(sf->sk);
+	if (sk->sk_net_refcnt)
+		sk_net_refcnt_upgrade(sf->sk);
 	err = tcp_set_ulp(sf->sk, "mptcp");
 	if (err)
 		goto err_free;

-- 
2.55.0

[PATCH mptcp-net 15/34] mptcp: close a subflow reset for a bad mapping

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

An MPTCP socket can stay in the ESTABLISHED state after MPTCP has reset
its only subflow because of data with no valid mapping, and recv() and
send() then block with no error.

If data arrives with no valid mapping and a fallback to TCP is not
possible, subflow_check_data_avail() resets the subflow: it moves the
TCP socket to TCP_CLOSE with tcp_set_state() and sends a RST. The MPTCP
worker then has to close the subflow in __mptcp_close_subflow(), and
closing the last subflow starts a timer that moves the MPTCP socket to
TCP_CLOSE. But the worker only calls that function if the bit
MPTCP_WORK_CLOSE_SUBFLOW is set, and this reset neither sets the bit nor
schedules the worker.

The fix used here is to call subflow_sched_work_if_closed() after the
reset, which sets the bit and schedules the worker.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch,
and with a new Packetdrill test: data without DSS while the fallback is
not possible, here because of out-of-order data at the MPTCP level.
Without this patch, the subflow was reset, but the MPTCP socket stayed
established, with no subflow: read() returned EAGAIN and send() timed
out, for good:

  dss_drop_ooo_reset_server.pkt:30: runtime error in read call: Expected errno 107 (Transport endpoint is not connected) but got 11 (Resource temporarily unavailable)

Fixes: 40947e13997a ("mptcp: schedule worker when subflow is closed")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Add a Fixes tag. Moved before "mptcp: continue the last mapping after
  a fallback", which makes this reset path reachable in more cases.
  Commit message: how the issue was found, and its symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/subflow.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index d23da4ac1968..7b8183b6a13e 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1499,8 +1499,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
 		}
 
 		if (!mptcp_try_fallback(ssk, MPTCP_MIB_DSSFALLBACK)) {
-			/* fatal protocol error, close the socket.
-			 * subflow_error_report() will introduce the appropriate barriers
+			/* fatal protocol error, reset the subflow and let the
+			 * worker close it
 			 */
 			subflow->reset_transient = 0;
 			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DSSRESET);
@@ -1515,6 +1515,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 				sk_eat_skb(ssk, skb);
 			mptcp_send_active_reset_reason(ssk);
 			WRITE_ONCE(subflow->data_avail, false);
+			subflow_sched_work_if_closed(msk, ssk);
 			return false;
 		}
 	}

-- 
2.55.0

Re: [PATCH mptcp-net 15/34] mptcp: close a subflow reset for a bad mapping

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] mptcp: first stream byte lost on simultaneous connect fallback
  in subflow_finish_connect() [1]
--

commit 68bf5d98c7b71382964b2e0b6553373923662fac
Author: Chris Mason <mason@kernel.org>

mptcp: close a subflow reset for a bad mapping

This commit calls subflow_sched_work_if_closed() after resetting a subflow
due to a bad mapping when TCP fallback is not possible, so the MPTCP worker
closes the subflow and updates the MPTCP socket state.

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index d23da4ac19684..7b8183b6a13e2 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1499,8 +1499,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  		}
>  
>  		if (!mptcp_try_fallback(ssk, MPTCP_MIB_DSSFALLBACK)) {
> -			/* fatal protocol error, close the socket.
> -			 * subflow_error_report() will introduce the appropriate barriers
> +			/* fatal protocol error, reset the subflow and let the
> +			 * worker close it
>  			 */
>  			subflow->reset_transient = 0;
>  			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DSSRESET);
> @@ -1515,6 +1515,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  				sk_eat_skb(ssk, skb);
>  			mptcp_send_active_reset_reason(ssk);
>  			WRITE_ONCE(subflow->data_avail, false);
> +			subflow_sched_work_if_closed(msk, ssk);
>  			return false;
>  		}
>  	}

[Severity: High]
This problem wasn't introduced by this patch, but can the fallback dummy
mapping right below this hunk in subflow_check_data_avail() drop the first
byte of incoming data after a TCP simultaneous connect [1]?

During simultaneous connect in TCP_SYN_SENT, mptcp_syn_options() detects
subflow_simultaneous_connect(), calls mptcp_try_fallback(), and clears
subflow->request_mptcp without setting subflow->conn_finished or
subflow->ssn_offset.  Because tcp_rcv_state_process() moves the socket from
TCP_SYN_RECV to TCP_ESTABLISHED without calling sk_rx_dst_set(),
subflow_finish_connect() is only invoked when the first packet in
TCP_ESTABLISHED reaches tcp_rcv_established().  At that point
TCP_SKB_CB(skb)->seq is peer_isn + 1 instead of peer_isn:

net/mptcp/subflow.c:subflow_finish_connect() {
	...
	subflow->rel_write_seq = 1;
	subflow->conn_finished = 1;
	subflow->ssn_offset = TCP_SKB_CB(skb)->seq;
	...
}

net/mptcp/subflow.c:subflow_check_data_avail() {
	...
fallback:
	...
	skb = skb_peek(&ssk->sk_receive_queue);
	subflow->map_valid = 1;
	subflow->map_data_len = skb->len;
	subflow->map_subflow_seq = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;
	subflow->map_seq = __mptcp_expand_seq(subflow->map_seq,
					      subflow->iasn +
					      TCP_SKB_CB(skb)->seq -
					      subflow->ssn_offset - 1);
}

When the first data segment (with seq == peer_isn + 1) reaches the fallback
path in subflow_check_data_avail(), subflow->iasn + TCP_SKB_CB(skb)->seq -
subflow->ssn_offset - 1 evaluates to -1, setting subflow->map_seq to -1ULL
while msk->ack_seq is 0.  In __mptcp_move_skb(), a 1-byte initial segment
is then dropped as duplicate data, or a multi-byte segment has its offset
advanced by 1 byte, corrupting the stream.

[1] https://sashiko.dev/#/bug/linux-17755322-ec78-468c-aabb-cd93fe7c5a2f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-mptcp-cmason-fixes-v1-0-9205b77e9a2f@kernel.org?part=15

[PATCH mptcp-net 16/34] mptcp: initialize drop_req in subflow_init_req()

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

With CONFIG_MPTCP and SO_REUSEPORT migration, which
net.ipv4.tcp_migrate_req=1 or a BPF program enables, TCP sockets can
leak until reboot, and each keeps its network namespace alive.

tcp_check_req() keeps a child off the accept queue if drop_req is true
in a request socket that an MPTCP listener created. drop_req has one
writer, subflow_syn_recv_sock(), which tcp_check_req() calls before that
test only for an MPTCP listener. But if SO_REUSEPORT migration moves the
request socket to a plain TCP listener, the test reads what the
allocation left in drop_req, and nothing drops the last reference of a
child that stays off the accept queue.

The fix used here is to set drop_req to false in subflow_init_req(),
which runs when the request socket of an MPTCP listener is set up.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch,
run as an unprivileged user in a user namespace, with
net.ipv4.tcp_migrate_req=1: an MPTCP listener and a TCP one sharing a
port with SO_REUSEPORT, the MPTCP one being closed while handshakes
complete. Without this patch, the children were leaked, with their
network namespace, without any message from the kernel:

  repro: step 3 to 6: 32 SYNs, 16 went to A. A closed, 16 third ACKs. accept() on B returned 0
  [...]
  repro: RESULT: 16 children never reached an accept queue
  repro: one RST for each connection. TCP: inuse 0 orphan 0 tw 0 alloc 16 mem 0
  === exit code 0. The process and its namespaces are gone
  === after:  TCP sockets allocated, all namespaces: 16 [...]

With UBSAN, the original version of this patch also reported:

  UBSAN: invalid-load in [...]/include/net/mptcp.h:144:47
  load of value 14 is not a valid value for type '_Bool'
   tcp_check_req.cold+0xb6/0x189
   tcp_v4_rcv+0x11f4/0x4230

Fixes: c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Shorter comment. Commit message: how the issue was found, and its
  symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/subflow.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 7b8183b6a13e..543368cfc890 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -151,6 +151,9 @@ static void subflow_init_req(struct request_sock *req, const struct sock *sk_lis
 	subflow_req->allow_join_id0 = mptcp_allow_join_id0(sock_net(sk_listener));
 	subflow_req->msk = NULL;
 	mptcp_token_init_request(req);
+
+	/* read by tcp_check_req() even after migration to a TCP listener */
+	tcp_rsk(req)->drop_req = false;
 }
 
 static bool subflow_use_different_sport(struct mptcp_sock *msk, const struct sock *sk)

-- 
2.55.0

[PATCH mptcp-net 17/34] mptcp: pm: kernel: count endpoint listeners as sockets in use

[Matthieu Baerts (NGI0)]

The listener that the in-kernel path manager creates for a "signal"
endpoint with a port is not counted in the "sockets" column of the MPTCP
or MPTCPv6 row of /proc/net/protocols, but it is uncounted when it is
closed. Each removal of such an endpoint then hides one MPTCP socket of
the same family from this column, for the lifetime of the netns.

mptcp_check_listen_stop() decrements this counter for any MPTCP socket
closed in TCP_LISTEN state, and mptcp_listen() increments it when
listen() succeeds. But mptcp_pm_nl_create_listen_socket() sets its MPTCP
socket in TCP_LISTEN state without incrementing it. This socket is
closed when the endpoint is deleted or flushed, when adding the endpoint
fails after the creation of the listener, e.g. because its ID is already
used, and when the netns is dismantled.

Nothing depends on this counter, it is only displayed. But it is wrong
once such an endpoint has been added: its listener is not counted while
"ss -Ml" lists it, and after its removal, the total is one below the
number of MPTCP sockets of the same family in use, a negative total
being displayed as 0 by sock_prot_inuse_get().

The counter was introduced by commit c558246ee73e ("mptcp: add
statistics for mptcp socket in use"), when only the subflow of this
socket was in TCP_LISTEN state. Commit 57fc0f1ceaa4 ("mptcp: ensure
listener is unhashed before updating the sk status") then set this
socket in TCP_LISTEN state too, without counting it.

The fix used here is to increment the counter when this socket is set
in TCP_LISTEN state, even if __inet_listen_sk() fails then: unlike in
mptcp_listen(), the socket stays in this state, so the counter is
decremented when it is released.

This issue has been found by an AI code review agent, while reviewing
the code of the commit "mptcp: pm: don't let listeners pin their netns",
and confirmed with a new Packetdrill test, and with a shell script
adding and flushing such an endpoint 3 times in a new netns, then
creating 3 MPTCP listeners. Without this patch:

  kernel_listener_inuse.pkt:15: error executing `test $(awk '/^MPTCP/ { n += $3 } END { print n }' /proc/net/protocols) -eq 2` command: non-zero status 1

  inuse: endpoint 1 present: 0 (expected 1)
  [...]
  inuse: 3 user listeners: 0 (expected 3)

Fixes: 57fc0f1ceaa4 ("mptcp: ensure listener is unhashed before updating the sk status")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_kernel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index cd882eb6af54..4955c928ea32 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -906,6 +906,7 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	 * old status is known (TCP_CLOSE).
 	 */
 	inet_sk_state_store(newsk, TCP_LISTEN);
+	sock_prot_inuse_add(sock_net(newsk), newsk->sk_prot, 1);
 	sock_set_flag(newsk, SOCK_RCU_FREE);
 	lock_sock(ssk);
 	WRITE_ONCE(mptcp_subflow_ctx(ssk)->pm_listener, true);

-- 
2.55.0

[PATCH mptcp-net 18/34] mptcp: count a listener once when listen() is called again

[Matthieu Baerts (NGI0)]

listen() on a listening MPTCP socket only updates its backlog, as with
TCP. But mptcp_listen() then counts this socket once more in the
"sockets" column of the MPTCP or MPTCPv6 row of /proc/net/protocols, and
sends a new MPTCP_EVENT_LISTENER_CREATED event, while only one decrement
and one MPTCP_EVENT_LISTENER_CLOSED event are done when it is closed.
Each extra listen() call then leaves the counter one above the number of
MPTCP sockets in use, for the lifetime of the netns, and a userspace
path manager can see two listeners created for one closed.

With TCP, the socket is only counted when it is hashed, which is not
done again for a listening socket.

The fix used here is to only count the socket, and send the event, if it
was not already listening.

This issue has been found by an AI code review agent, while reviewing
the code of the commit "mptcp: pm: kernel: count endpoint listeners as
sockets in use", and confirmed with a new Packetdrill test, and with a
script calling listen() twice. Without this patch:

  listen_listen.pkt:12: error executing `test $(awk '/^MPTCP/ { n += $3 } END { print n }' /proc/net/protocols) -eq 1` command: non-zero status 1

  inuse: relisten: listening: 2 (expected 1)
  inuse: relisten: closed: 1 (expected 0)

Fixes: f8c9dfbd875b ("mptcp: add pm listener events")
Fixes: c558246ee73e ("mptcp: add statistics for mptcp socket in use")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 6dc55a3f027b..8f954a752797 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -4381,6 +4381,7 @@ static int mptcp_listen(struct socket *sock, int backlog)
 	struct mptcp_sock *msk = mptcp_sk(sock->sk);
 	struct sock *sk = sock->sk;
 	struct sock *ssk;
+	bool listening;
 	int err;
 
 	pr_debug("msk=%p\n", msk);
@@ -4397,6 +4398,7 @@ static int mptcp_listen(struct socket *sock, int backlog)
 		goto unlock;
 	}
 
+	listening = sk->sk_state == TCP_LISTEN;
 	set_bit(MPTCP_RTX_ENABLED, &msk->flags);
 	mptcp_set_state(sk, TCP_LISTEN);
 	sock_set_flag(sk, SOCK_RCU_FREE);
@@ -4411,7 +4413,8 @@ static int mptcp_listen(struct socket *sock, int backlog)
 	release_sock(ssk);
 	mptcp_set_state(sk, inet_sk_state_load(ssk));
 
-	if (!err) {
+	/* only the backlog changes when listening already */
+	if (!err && !listening) {
 		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
 		mptcp_copy_inaddrs(sk, ssk);
 		mptcp_event_pm_listener(ssk, MPTCP_EVENT_LISTENER_CREATED);

-- 
2.55.0

[PATCH mptcp-net 19/34] mptcp: do not fallback while old data is in flight

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

If an established MPTCP connection falls back to TCP while a zero window
probe or reinjected data is queued or in flight, recv() can return data
twice with no error, so the stream is silently corrupted.

After a fallback mptcp_established_options() writes a DSS option only
for an infinite mapping, so an skb that mptcp_sendmsg_frag() queued with
a mapping to data sequence numbers (DSN) is sent without that mapping.
The receiver treats the bytes of the subflow, in sequence order, as the
rest of the stream, so a fallback is only correct if the subflow carries
each byte of the stream once. But allow_infinite_fallback, the flag that
permits a fallback, stays set in two cases in which old data is queued
again on the only subflow.

First, a zero window probe is a copy of the next unsent byte, mapped to
the last acknowledged DSN so that the peer drops it as a duplicate.
Second, when a subflow whose MP_JOIN handshake has failed is closed,
__mptcp_retransmit_pending_data() marks all data that has no DATA_ACK
yet as unsent, although no second subflow has joined. If the sender
falls back while such bytes are queued, they are sent without their
mapping and the receiver delivers them as new data, as here for a probe:

    sender                            receiver
    ======                            ========
    queue a probe, a copy of byte N
    get data with no DSS, fall back
                                      read, open the window
    send the probe with no DSS
                                      no DSS: fall back
                                      deliver byte N
    send byte N as data
                                      deliver byte N again /* corrupt */

The fix used here is to refuse the fallback while old data is in flight:
- when mptcp_sendmsg_frag() maps bytes to a DSN below snd_nxt of the
  MPTCP socket, set the new bit old_data_pending of the subflow, and
  store the TCP sequence number after these bytes in its new member
  old_data_end
- clear the bit in mptcp_incoming_options() when a segment with a DSS,
  other than an infinite mapping, acknowledges that sequence number
- make mptcp_try_fallback() fail while the bit is set, so that its
  callers reset the subflow
- delay the infinite mapping that answers an MP_FAIL, and the fallback
  that comes with it, to the first data that is queued with the bit
  clear, and send plain mappings until then
- after a fallback, skip data below snd_nxt in mptcp_sendmsg_frag(): a
  reinjection that has not reached the subflow yet does not set the bit

A consequence: if the old data reached the peer with its mapping, but
the DSS acknowledging it is still on its way back, or lost its options,
a fallback attempted meanwhile resets the subflow, although it would
have been safe. This window lasts about one round trip, or as long as
the acknowledgments lose their options.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch:
between two unmodified Linux hosts, nftables removing the MPTCP options,
e.g. while a zero window probe is pending, or after a failed MP_JOIN.
Without this patch, the reader got some data twice, with a regular end
of file: the stream was silently corrupted:

  receiver   4.692: READ-TOTAL outcome iii-repeat-or-gap bytes 8388609 of 8388608 missing 0 twice 1 [...] end: eof
  [...]
  receiver   8.611: READ-TOTAL outcome iii-repeat-or-gap bytes 26000 of 14000 missing 0 twice 12000 [...] end: eof

Note for the backports: this patch depends on commit 119d51e225fe
("mptcp: fallback when MPTCP opts are dropped after 1st data") and
commit f8a1d9b18c5e ("mptcp: make fallback action and fallback decision
atomic"), two fixes which may have already been backported: they allowed
the fallback in the reproduced cases, once data has been acknowledged
without checksums, and after a failed MP_JOIN.

Fixes: 1e39e5a32ad7 ("mptcp: infinite mapping sending")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Shorter comments. Move old_data_end to an existing hole, after
  subflow_id. Commit message: mention the reset that can now replace a
  safe fallback + how the issue was found, and its symptoms + note for
  the backports. Moved before "mptcp: continue the last mapping after a
  fallback". ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/options.c  |  6 ++++++
 net/mptcp/protocol.c | 17 ++++++++++++++++-
 net/mptcp/protocol.h |  9 ++++++++-
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index b4b3292fb4dc..682a7b93d962 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1317,6 +1317,12 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 			return true;
 	}
 
+	/* DSS acking the old data, not an infinite map: no peer fallback */
+	if (unlikely(subflow->old_data_pending) &&
+	    !(mp_opt.use_map && !mp_opt.data_len) &&
+	    !before(TCP_SKB_CB(skb)->ack_seq, subflow->old_data_end))
+		subflow->old_data_pending = 0;
+
 	/* we can't wait for recvmsg() to update the ack_seq, otherwise
 	 * monodirectional flows will stuck
 	 */
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 8f954a752797..7329deb43c62 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1472,6 +1472,12 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	info->mss_now = tcp_send_mss(ssk, &info->size_goal, info->flags);
 	copy = info->size_goal;
 
+	/* after a fallback, the subflow got all data below snd_nxt already */
+	if (unlikely(__mptcp_check_fallback(msk) &&
+		     before64(data_seq, msk->snd_nxt)))
+		return min_t(u64, msk->snd_nxt - data_seq,
+			     info->limit - info->sent);
+
 	skb = tcp_write_queue_tail(ssk);
 	if (skb && copy > skb->len) {
 		/* Limit the write to the size available in the
@@ -1552,6 +1558,12 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	TCP_SKB_CB(skb)->end_seq += copy;
 	tcp_skb_pcount_set(skb, 0);
 
+	/* old data mapped again: no fallback until acked with a DSS */
+	if (unlikely(before64(data_seq, msk->snd_nxt))) {
+		mptcp_subflow_ctx(ssk)->old_data_end = tcp_sk(ssk)->write_seq;
+		mptcp_subflow_ctx(ssk)->old_data_pending = 1;
+	}
+
 	/* on skb reuse we just need to update the DSS len */
 	if (reuse_skb) {
 		TCP_SKB_CB(skb)->tcp_flags &= ~TCPHDR_PSH;
@@ -1582,7 +1594,10 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 out:
 	if (READ_ONCE(msk->csum_enabled))
 		mptcp_update_data_checksum(skb, copy);
-	if (mptcp_subflow_ctx(ssk)->send_infinite_map)
+
+	/* the infinite map implies a fallback: wait for old data acks */
+	if (mptcp_subflow_ctx(ssk)->send_infinite_map &&
+	    !mptcp_subflow_ctx(ssk)->old_data_pending)
 		mptcp_update_infinite_map(msk, ssk, mpext);
 	trace_mptcp_sendmsg_frag(mpext);
 	mptcp_subflow_ctx(ssk)->rel_write_seq += copy;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3be60da12924..75d4c2f82501 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -602,7 +602,8 @@ struct mptcp_subflow_context {
 		is_mptfo : 1,	    /* subflow is doing TFO */
 		mpc_drop : 1,	    /* the MPC option has been dropped in a rtx */
 		resetting : 1,	    /* subflow is resetting */
-		__unused : 9;
+		old_data_pending : 1, /* old_data_end not acked with a DSS */
+		__unused : 8;
 	bool	data_avail;
 	bool	scheduled;
 	bool	pm_listener;	    /* a listener managed by the kernel PM? */
@@ -630,6 +631,7 @@ struct mptcp_subflow_context {
 				     */
 
 	u32	subflow_id;
+	u32	old_data_end;	    /* TCP write_seq after the last old data */
 
 	long	delegated_status;
 	unsigned long	fail_tout;
@@ -1330,6 +1332,11 @@ static inline bool mptcp_try_fallback(struct sock *ssk, int fb_mib)
 	struct mptcp_sock *msk;
 
 	msk = mptcp_sk(sk);
+
+	/* queued old data would be sent without mapping after a fallback */
+	if (subflow->old_data_pending && !__mptcp_check_fallback(msk))
+		return false;
+
 	if (!__mptcp_try_fallback(msk, fb_mib))
 		return false;
 	if (READ_ONCE(msk->snd_data_fin_enable) && !(ssk->sk_shutdown & SEND_SHUTDOWN)) {

-- 
2.55.0

Re: [PATCH mptcp-net 19/34] mptcp: do not fallback while old data is in flight

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] mptcp: infinite map emission stall in
  mptcp_pm_mp_fail_received() [1]
--

commit 4c9a52a9f4dc2474ec08fa066289f109db0a1859
Author: Chris Mason <mason@kernel.org>

mptcp: do not fallback while old data is in flight

This commit prevents an MPTCP connection from falling back to plain TCP
while a zero window probe or reinjected data is still in flight on the
subflow, avoiding duplicate data delivery on the receiver.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 8f954a752797d..7329deb43c622 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -1582,7 +1594,10 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
>  out:
>  	if (READ_ONCE(msk->csum_enabled))
>  		mptcp_update_data_checksum(skb, copy);
> -	if (mptcp_subflow_ctx(ssk)->send_infinite_map)
> +
> +	/* the infinite map implies a fallback: wait for old data acks */
> +	if (mptcp_subflow_ctx(ssk)->send_infinite_map &&
> +	    !mptcp_subflow_ctx(ssk)->old_data_pending)
>  		mptcp_update_infinite_map(msk, ssk, mpext);

[Severity: High]
This problem wasn't introduced by this patch, but can subflow->send_infinite_map
remain pending forever [1] when an MP_FAIL arrives while the connection
has unacknowledged data and no unsent data left on the send queue?

mptcp_pm_mp_fail_received() sets subflow->send_infinite_map = 1 and
msk->allow_subflows = false:

net/mptcp/pm.c:mptcp_pm_mp_fail_received() {
	...
	msk->allow_subflows = false;
	spin_unlock_bh(&msk->fallback_lock);

	if (!subflow->fail_tout) {
		pr_debug("send MP_FAIL response and infinite map\n");

		subflow->send_mp_fail = 1;
		subflow->send_infinite_map = 1;
		tcp_send_ack(sk);
	}
	...
}

Attaching the infinite map in mptcp_sendmsg_frag() requires sending a data
skb.  When there is no unsent data, mptcp_send_head(sk) is NULL so
__mptcp_subflow_push_pending() does not call mptcp_sendmsg_frag().  Meanwhile,
the MPTCP retransmission timer cannot retransmit the unacknowledged data to
carry the infinite map because __mptcp_push_retrans() aborts when
!msk->allow_subflows:

net/mptcp/protocol.c:__mptcp_push_retrans() {
	...
			spin_lock_bh(&msk->fallback_lock);
			if (__mptcp_check_fallback(msk) ||
			    !msk->allow_subflows) {
				spin_unlock_bh(&msk->fallback_lock);
				release_sock(ssk);
				return -1;
			}
	...
}

As a result, the infinite map is never transmitted and the connection stalls
indefinitely.

[1] https://sashiko.dev/#/bug/linux-906f0960-8875-45cb-8066-cc9808d83b26

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-mptcp-cmason-fixes-v1-0-9205b77e9a2f@kernel.org?part=19

[PATCH mptcp-net 20/34] mptcp: stop resending data to a peer that fell back

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

recv() on an established MPTCP connection can silently return data that
it has returned before, again and again, if the receiver falls back to
TCP and the sender does not.

While no DATA_ACK arrives, the MPTCP retransmit timer makes
mptcp_sendmsg_frag() queue old data on the subflow again: either
fragments from __mptcp_retrans(), or a zero window probe. That is only
correct while the receiver reads the DSS mapping of these bytes and
drops them as duplicates: a receiver that has fallen back to TCP
delivers them as new data. But mptcp_incoming_options() ignores a bare
ACK, one with no MPTCP option, on a fully established subflow, so a
sender that gets only bare ACKs from that receiver stays in MPTCP mode:

    sender                            receiver
    ======                            ========
    send bytes N to M
    (a middlebox removes the DSS)
                                      no DSS: fall back
                                      deliver N to M, send a bare ACK
    no DATA_ACK, the timer expires
    send bytes N to M again
                                      deliver N to M again  <-- wrong
                                      send a bare ACK

The fix used here is to:
- reset the subflow, with the reason "middlebox interference", when
  mptcp_sendmsg_frag() has more old data to queue and only bare ACKs
  have acknowledged the old data queued so far. The test reads
  old_data_pending, which the earlier commit "mptcp: do not fallback
  while old data is in flight" sets when old data is queued. That commit
  clears it when a segment with a DSS, other than an infinite mapping,
  acknowledges old_data_end, the TCP sequence number after that data.
  The reset is counted in FallbackFailed: the peer has likely fallen
  back, but this side cannot
- clear old_data_pending for a segment with any other MPTCP option too,
  in mptcp_incoming_options(). An infinite mapping stays the exception
- skip a zero window probe while the TCP retransmit queue of the subflow
  holds data: the MPTCP retransmit timer, armed when a push leaves data
  behind, tries again later

This limits the repeats, but does not prevent the first one: data that
was new when it was first sent does not set old_data_pending, so its
first MPTCP-level retransmission is still delivered twice by a peer that
has fallen back. After a failed MP_JOIN, more of the reinjected data can
be.

A consequence: a peer still in MPTCP mode, whose options are removed on
the way back for a while only, recovered once the options came back.
Its subflow is now reset as soon as more old data is due.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch:
between two unmodified Linux hosts, nftables removing the MPTCP options
in the middle of the transfer, so that only the receiver falls back.
Without this patch, the reader got the same data again and again, here
65 MB more than once in 120 seconds:

  receiver 127.514: READ-TOTAL outcome iii-repeat-or-gap bytes 68255744 of 8388608 missing 0 twice 65650688 [...]
  [...] took 120.0 s end: limit: still data after 120 s

Fixes: 119d51e225fe ("mptcp: fallback when MPTCP opts are dropped after 1st data")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Shorter comments, also in struct mptcp_subflow_context. No timer
  arming when a probe is skipped: "mptcp: arm the rtx timer if a push
  leaves data behind" already does it. Commit message: the first repeat
  is still delivered, and a peer that loses its options for a while only
  is now reset + how the issue was found, and its symptoms. Count the
  reset in MPTCP_MIB_FALLBACKFAILED. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/options.c  | 12 ++++++------
 net/mptcp/protocol.c | 19 +++++++++++++++++++
 net/mptcp/protocol.h |  2 +-
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 682a7b93d962..6790c9e15521 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1271,6 +1271,12 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
 		return sk->sk_state != TCP_CLOSE;
 
+	/* MPTCP opt acking the old data, not an infinite map: no fallback */
+	if (unlikely(subflow->old_data_pending) && mp_opt.suboptions &&
+	    !(mp_opt.use_map && !mp_opt.data_len) &&
+	    !before(TCP_SKB_CB(skb)->ack_seq, subflow->old_data_end))
+		subflow->old_data_pending = 0;
+
 	if (unlikely(mp_opt.suboptions != OPTION_MPTCP_DSS)) {
 		if ((mp_opt.suboptions & OPTION_MPTCP_FASTCLOSE) &&
 		    READ_ONCE(msk->local_key) == mp_opt.fc_recv_key) {
@@ -1317,12 +1323,6 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 			return true;
 	}
 
-	/* DSS acking the old data, not an infinite map: no peer fallback */
-	if (unlikely(subflow->old_data_pending) &&
-	    !(mp_opt.use_map && !mp_opt.data_len) &&
-	    !before(TCP_SKB_CB(skb)->ack_seq, subflow->old_data_end))
-		subflow->old_data_pending = 0;
-
 	/* we can't wait for recvmsg() to update the ack_seq, otherwise
 	 * monodirectional flows will stuck
 	 */
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 7329deb43c62..388e77da8f16 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1444,6 +1444,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 			      struct mptcp_data_frag *dfrag,
 			      struct mptcp_sendmsg_info *info)
 {
+	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	u64 data_seq = dfrag->data_seq + info->sent;
 	int offset = dfrag->offset + info->sent;
 	struct mptcp_sock *msk = mptcp_sk(sk);
@@ -1527,11 +1528,29 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 			return 0;
 		}
 
+		/* a new probe would move old_data_end: one at a time */
+		if (!tcp_rtx_queue_empty(ssk)) {
+			tcp_remove_empty_skb(ssk);
+			return 0;
+		}
+
 		zero_window_probe = true;
 		data_seq = snd_una - 1;
 		copy = 1;
 	}
 
+	/* old data acked without MPTCP opts: the peer may have fallen back */
+	if (unlikely(before64(data_seq, msk->snd_nxt) &&
+		     subflow->old_data_pending &&
+		     !before(tcp_sk(ssk)->snd_una, subflow->old_data_end))) {
+		tcp_remove_empty_skb(ssk);
+		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_FALLBACKFAILED);
+		subflow->reset_transient = 0;
+		subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
+		mptcp_subflow_reset(ssk);
+		return -EAGAIN;
+	}
+
 	copy = min_t(size_t, copy, info->limit - info->sent);
 
 	/* like TCP, allow some progress up to tcp_wmem[0] */
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 75d4c2f82501..e61b6a3a955d 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -602,7 +602,7 @@ struct mptcp_subflow_context {
 		is_mptfo : 1,	    /* subflow is doing TFO */
 		mpc_drop : 1,	    /* the MPC option has been dropped in a rtx */
 		resetting : 1,	    /* subflow is resetting */
-		old_data_pending : 1, /* old_data_end not acked with a DSS */
+		old_data_pending : 1, /* old data not acked with MPTCP opts */
 		__unused : 8;
 	bool	data_avail;
 	bool	scheduled;

-- 
2.55.0

Re: [PATCH mptcp-net 20/34] mptcp: stop resending data to a peer that fell back

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mptcp: drop skb in mptcp_incoming_options() if
  ack_update_msk() resets subflow
--

commit 7d0f1ec53421d182e690548304d3c287dc956345
Author: Chris Mason <mason@kernel.org>

mptcp: stop resending data to a peer that fell back

This commit resets the subflow when mptcp_sendmsg_frag() is about to queue
more old data after previous old data was acknowledged only by bare ACKs,
clears old_data_pending on any non-infinite MPTCP option, and skips zero
window probes while the subflow retransmit queue is non-empty.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7329deb43c622..388e77da8f16c 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -1527,11 +1528,29 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
>  			return 0;
>  		}
>  
> +		/* a new probe would move old_data_end: one at a time */
> +		if (!tcp_rtx_queue_empty(ssk)) {
> +			tcp_remove_empty_skb(ssk);
> +			return 0;
> +		}
> +
>  		zero_window_probe = true;
>  		data_seq = snd_una - 1;
>  		copy = 1;
>  	}
>  
> +	/* old data acked without MPTCP opts: the peer may have fallen back */
> +	if (unlikely(before64(data_seq, msk->snd_nxt) &&
> +		     subflow->old_data_pending &&
> +		     !before(tcp_sk(ssk)->snd_una, subflow->old_data_end))) {
> +		tcp_remove_empty_skb(ssk);
> +		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_FALLBACKFAILED);
> +		subflow->reset_transient = 0;
> +		subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
> +		mptcp_subflow_reset(ssk);
> +		return -EAGAIN;
> +	}

[Severity: Medium]
Can mptcp_incoming_options() still return true to tcp_data_queue() after
mptcp_sendmsg_frag() resets the subflow from ack_update_msk()?

mptcp_incoming_options() only checks for sk->sk_state != TCP_CLOSE right
after check_fully_established(), assuming nothing later in the function
can reset sk.  However, ack_update_msk() calls __mptcp_check_push() ->
__mptcp_subflow_push_pending() -> __subflow_push_pending() ->
mptcp_sendmsg_frag() when !sock_owned_by_user(sk):

net/mptcp/options.c:mptcp_incoming_options() {
	...
	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
		return sk->sk_state != TCP_CLOSE;
	...
	if (mp_opt.use_ack)
		ack_update_msk(msk, sk, skb, &mp_opt);
	rwin_update(msk, sk, skb);
	...
	return true;
}

If the subflow has old_data_pending set and snd_una has reached
old_data_end (for example via a prior bare ACK or an incoming DSS carrying
both DATA_ACK and an infinite mapping) while msk has unsent reinjected
data, mptcp_sendmsg_frag() calls mptcp_subflow_reset(ssk), which purges
sk_receive_queue, sends a RST, and moves ssk to TCP_CLOSE via tcp_done().
Because mptcp_incoming_options() still returns true, tcp_data_queue()
continues processing the incoming skb on the TCP_CLOSE subflow socket,
queueing payload onto sk_receive_queue, re-arming ACK timers, or hitting
tcp_fin() on a closed socket.

Checking sk->sk_state == TCP_CLOSE after ack_update_msk() and returning
false would let tcp_data_queue() drop the packet.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-mptcp-cmason-fixes-v1-0-9205b77e9a2f@kernel.org?part=20

[PATCH mptcp-net 21/34] mptcp: pm: kernel: don't count signal+subflow ID 0 endp

[Matthieu Baerts (NGI0)]

From: Josef Bacik <josef@toxicpanda.com>

local_addr_used must not be incremented for the endpoint of the initial
subflow, because __mark_subflow_endp_available() does not decrement it
for ID 0. mptcp_pm_create_subflow_or_signal_addr() tells that endpoint
by local.addr.id == msk->mpc_endpoint_id, in the loop that creates the
subflows and after it.

For an endpoint with both the signal and the subflow flag that
comparison comes too late: the part that announces the address has
replaced local.addr.id by 0 already. So the counter goes up each time
such an endpoint is removed and added again while a connection that
started from its address is alive, and never comes down. When it has
reached the number of subflow endpoints, the connection does not use any
new endpoint: after four rounds local_addr_used was 5 instead of 1, and
an endpoint added then got no subflow.

ID 0 can't be the ID of any other endpoint: look at that, after the
replacement.

This issue has been found with the help of an LLM. It has been
reproduced with the modified "delete and re-add" subtest of
mptcp_join.sh, from the next commit: without this patch, the subflow of
the ID 0 endpoint was not created again at the third round,
local_addr_used being above local_addr_max:

  # 002 delete and re-add
  [...]
  #       after re-add id 0 (3)               [FAIL] got 2:2 subflows expected 3
  [...]
  [...] subflows:2 [...] local_addr_used:4 local_addr_max:3 [...]

Fixes: 9366922adc6a ("mptcp: pm: fix ID 0 endp usage after multiple re-creations")
Fixes: 579a752464a6 ("mptcp: pm: in-kernel: always mark signal+subflow endp as used")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
[ Keep the original one-line comment. Commit message: 'behind it' ->
  'after it' + how the issue was found, and its symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_kernel.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 4955c928ea32..976d72565bda 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -411,7 +411,9 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 		/* Special case for ID0: set the correct ID */
 		if (local.addr.id == msk->mpc_endpoint_id)
 			local.addr.id = 0;
-		else /* local_addr_used is not decr for ID 0 */
+
+		/* local_addr_used is not decr for ID 0 */
+		if (local.addr.id)
 			msk->pm.local_addr_used++;
 
 		nr = fill_remote_addresses_vec(msk, &local.addr, fullmesh,
@@ -432,7 +434,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 	 * case. This avoids issues later when removing the endpoint and calling
 	 * __mark_subflow_endp_available(), which expects the increment here.
 	 */
-	if (signal_and_subflow && local.addr.id != msk->mpc_endpoint_id)
+	if (signal_and_subflow && local.addr.id)
 		msk->pm.local_addr_used++;
 
 	mptcp_pm_nl_check_work_pending(msk);

-- 
2.55.0

[PATCH mptcp-net 22/34] selftests: mptcp: join: check re-adding signal+subflow ID 0 endp

[Matthieu Baerts (NGI0)]

This test extends "delete and re-add" to validate the previous commit:
when the endpoint linked to the initial subflow (ID 0) had both the
signal and subflow flags, it was counted as used each time it was
re-added, while this counter is not decremented for this special
endpoint. Once the counter reached the number of subflow endpoints, no
more subflows were created.

From the second round, this endpoint is now re-added with the signal
flag too: without the previous commit, the subflow is not re-created at
the third round.

The ADD_ADDR sent at these two rounds are dropped, and not
retransmitted: if the server received them before the new subflow, it
would try to connect back to the address and port of the client, and
fail, which would change the counters.

The 'Fixes' tags here below are the same as the ones from the previous
commit: this patch here is not fixing anything wrong in the selftests,
but it validates the previous fix for an issue introduced by these
commit IDs.

Fixes: 9366922adc6a ("mptcp: pm: fix ID 0 endp usage after multiple re-creations")
Fixes: 579a752464a6 ("mptcp: pm: in-kernel: always mark signal+subflow endp as used")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index c3ccce96b45b..6cd187577479 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -4387,17 +4387,23 @@ endpoint_tests()
 		ip netns exec "${ns2}" nft insert rule ip filter OUTPUT \
 			ip saddr 10.0.1.2 meta l4proto tcp \
 			"${RM_ADDR_OFFSETS}" drop || nft=0
-		local i
+		# No ADD_ADDR to ns1: it could try to connect back to ns2's port
+		ip netns exec "${ns2}" sysctl -q net.mptcp.add_addr_timeout=0
+		ip netns exec "${ns2}" nft insert rule ip filter OUTPUT \
+			meta l4proto tcp \
+			tcp option mptcp subtype add-addr drop || nft=0
+		local i flags="subflow"
 		for i in $(seq 3); do
 			pm_nl_del_endpoint $ns2 1 10.0.1.2
 			sleep 0.5
 			chk_subflow_nr "after delete id 0 ($i)" 2
 			chk_mptcp_info subflows 2 subflows 2 # only decr for additional sf
 
-			pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow
+			pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags "${flags}"
 			wait_mpj $((3 + i))
 			chk_subflow_nr "after re-add id 0 ($i)" 3
 			chk_mptcp_info subflows 3 subflows 3
+			flags="signal,subflow" # ID 0 not counted as used either
 		done
 		[ "${nft}" = 1 ] && ip netns exec "${ns2}" nft flush chain ip filter OUTPUT
 

-- 
2.55.0

[PATCH mptcp-net 23/34] mptcp: use sk instead of casting msk in check_fully_established()

[Matthieu Baerts (NGI0)]

The MPTCP socket is cast to a struct sock in check_fully_established(),
to take its data lock. The next commits need it more, to push the
pending data.

Add a local 'sk' variable instead of casting msk each time: no
functional change.

The 'Fixes' tag here below is the same as the one from the next
commits: this patch is needed to fix the issues introduced by this
commit ID.

Fixes: 36b122baf6a8 ("mptcp: add subflow_v(4,6)_send_synack()")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/options.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 6790c9e15521..a74520f6b069 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -982,6 +982,8 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 				    struct sk_buff *skb,
 				    struct mptcp_options_received *mp_opt)
 {
+	struct sock *sk = (struct sock *)msk;
+
 	/* here we can process OoO, in-window pkts, only in-sequence 4th ack
 	 * will make the subflow fully established
 	 */
@@ -1044,9 +1046,9 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 	if (mp_opt->deny_join_id0)
 		WRITE_ONCE(msk->pm.remote_deny_join_id0, true);
 
-	mptcp_data_lock((struct sock *)msk);
+	mptcp_data_lock(sk);
 	__mptcp_subflow_fully_established(msk, subflow, mp_opt);
-	mptcp_data_unlock((struct sock *)msk);
+	mptcp_data_unlock(sk);
 
 check_notify:
 	/* if the subflow is not already linked into the conn_list, we can't

-- 
2.55.0

[PATCH mptcp-net 24/34] mptcp: push queued data on passive TFO subflows becoming established

[Matthieu Baerts (NGI0)]

With TCP Fast Open (TFO), the data written by an MPTCP server before the
third ACK is not sent when this ACK arrives, but only when another event
triggers a push, e.g. new data from the peer. With a request/response
protocol, the client waits for the reply, which can then stay stuck.

A SYN with a valid TFO cookie creates a child socket right away, which
can be accepted while its subflow is still in the SYN_RECV state. The
MPTCP socket is already in the ESTABLISHED state: send() puts the data
in its write queue, but this data cannot be pushed to the subflow before
it is established, see __mptcp_subflow_active(). When the third ACK,
with the MP_CAPABLE option, arrives, check_fully_established() marks
the subflow as fully established, but nothing pushes the pending data:
this ACK has no DSS option, so ack_update_msk() is not called.

The fix used here is to push the pending data when a passive TFO subflow
becomes fully established. Checking subflow->is_mptfo is not enough:
since commit e00b63056fb4 ("mptcp: fastopen: only mark MPTFO subflows
with SYN data"), it is not set when the SYN has no data, while a child
is also created in this case. syn_fastopen_child is then used instead.

This issue has been reported by a user of proxies over MPTCP: the
clients put their requests in the SYN, and the replies from the targets
close to the server were stuck. It has been reproduced with new
Packetdrill tests, where the server writes data before the third ACK,
after a SYN with and without data. Without this patch, and before the
earlier commit "mptcp: arm the rtx timer if a push leaves data behind",
this data was not sent:

  server-tfo-write-before-3rd-ack.pkt:24: error handling packet: Timed out waiting for packet

Since this earlier commit, this data is sent, but only when the MPTCP
retransmission timer fires, here after one second, as the tests increase
its interval:

  server-tfo-write-before-3rd-ack.pkt:24: error handling packet: timing error: expected outbound packet at 0.237174 sec but happened at 1.181857 sec; tolerance 0.100000 sec

Fixes: 36b122baf6a8 ("mptcp: add subflow_v(4,6)_send_synack()")
Suggested-by: Petar Sakic <petar.sakic@ink.fish>
Suggested-by: T S Rameshkumar <rameshkumar.t@phytecembedded.in>
Reported-by: Petar Sakic <petar.sakic@ink.fish>
Closes: https://lore.kernel.org/mptcp/CAFPPu1gU2Y-D+d4i3F0MoNkYK+e1U+=X3qf6QycjfKBw+8snPg@mail.gmail.com/
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Note: T S Rameshkumar sent earlier versions of this patch [1], but
stopped. In the end, we are back at the version suggested by Petar, with
syn_fastopen_child instead of is_mptfo. This issue is affecting users,
we can no longer wait: that's why I'm taking over, after having sent a
few reminders.

[1] https://lore.kernel.org/20261012111001.542847-1-rameshkumar.t@phytecembedded.in
---
 net/mptcp/options.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index a74520f6b069..3477a7ec5a05 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1048,6 +1048,8 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 
 	mptcp_data_lock(sk);
 	__mptcp_subflow_fully_established(msk, subflow, mp_opt);
+	if (tcp_sk(ssk)->syn_fastopen_child)
+		__mptcp_check_push(sk, ssk);
 	mptcp_data_unlock(sk);
 
 check_notify:

-- 
2.55.0

[PATCH mptcp-net 25/34] mptcp: push queued data on passive TFO subflows falling back to TCP

[Matthieu Baerts (NGI0)]

With TCP Fast Open (TFO), the data written by an MPTCP server before the
third ACK is not sent when this ACK arrives without MPTCP options, but
only when another event triggers a push, e.g. new data from the peer.

The previous commit pushes this data when the third ACK, with the
MP_CAPABLE option, makes a passive TFO subflow fully established. If
this ACK has no MPTCP options, e.g. because a middlebox removed them,
check_fully_established() falls back to TCP, and returns before this
push. The fallback case of mptcp_incoming_options(), which pushes the
pending data, is only reached with the next packets.

The fix used here is to push the pending data after this fallback too,
for the passive TFO subflows only, as in the previous commit.

This issue has been found by an AI code review agent (Clashiko), while
reviewing an earlier version of the previous commit. It has been
reproduced with a new Packetdrill test, where the MPTCP options are
removed from the third ACK. Without this patch, and before the earlier
commit "mptcp: arm the rtx timer if a push leaves data behind", this
data was not sent:

  server-tfo-fallback-write-before-3rd-ack.pkt:26: error handling packet: Timed out waiting for packet

Since this earlier commit, this data is sent, but only when the MPTCP
retransmission timer fires, here after one second, as the test increases
its interval:

  server-tfo-fallback-write-before-3rd-ack.pkt:26: error handling packet: timing error: expected outbound packet at 0.235869 sec but happened at 1.144189 sec; tolerance 0.100000 sec

Fixes: 36b122baf6a8 ("mptcp: add subflow_v(4,6)_send_synack()")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/options.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 3477a7ec5a05..c56ca5e4df14 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1035,6 +1035,11 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_FALLBACKFAILED);
 			goto reset;
 		}
+		if (tcp_sk(ssk)->syn_fastopen_child) {
+			mptcp_data_lock(sk);
+			__mptcp_check_push(sk, ssk);
+			mptcp_data_unlock(sk);
+		}
 		return false;
 	}
 

-- 
2.55.0

Re: [PATCH mptcp-net 25/34] mptcp: push queued data on passive TFO subflows falling back to TCP

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mptcp: incomplete fallback handling in
  check_fully_established()
--

commit 66148bc476eb2e96002647feaf23c977ed60ceb1
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

mptcp: push queued data on passive TFO subflows falling back to TCP

This commit pushes pending data in check_fully_established() when a
passive TFO subflow falls back to plain TCP on the third ACK.

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 3477a7ec5a051..c56ca5e4df14b 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1035,6 +1035,11 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>  			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_FALLBACKFAILED);
>  			goto reset;
>  		}
> +		if (tcp_sk(ssk)->syn_fastopen_child) {
> +			mptcp_data_lock(sk);
> +			__mptcp_check_push(sk, ssk);
> +			mptcp_data_unlock(sk);
> +		}
>  		return false;
>  	}

[Severity: Medium]
Should this fallback path also run for active-open subflows and clean up
msk->rtx_queue the same way mptcp_incoming_options() does?

net/mptcp/options.c:mptcp_incoming_options() {
	...
	if (__mptcp_check_fallback(msk)) {
		...
		mptcp_data_lock(subflow->conn);
		if (sk_stream_memory_free(sk))
			__mptcp_check_push(subflow->conn, sk);
		...
		__mptcp_snd_una_update(msk, READ_ONCE(msk->snd_nxt));

		__mptcp_data_acked(subflow->conn);
		mptcp_data_unlock(subflow->conn);
		return !mptcp_over_limit(subflow->conn, sk, skb);
	}
	...
}

Because check_fully_established() returns false, mptcp_incoming_options()
exits without running its fallback block on the packet that triggers the
fallback.

On a passive TFO subflow, __mptcp_check_push() spools pending dfrags to
ssk and advances msk->snd_nxt, but omitting __mptcp_snd_una_update() and
__mptcp_data_acked() leaves those dfrags in msk->rtx_queue until another
packet arrives, keeping the bytes charged to both sk->sk_wmem_queued and
ssk->sk_wmem_queued and double-counting SIOCOUTQ.

On an active-open subflow, msk->wnd_end is initialized from the unscaled
SYN-ACK window, so mptcp_sendmsg() can leave unsent data in msk->rtx_queue
beyond the initial flight.  If a middlebox strips MPTCP options after the
SYN-ACK and the peer sends a bare TCP ACK opening the scaled window,
gating __mptcp_check_push() on syn_fastopen_child skips pushing the
remaining data, and __mptcp_retrans() -> __mptcp_push_retrans() aborts on
__mptcp_check_fallback(msk) without setting MPTCP_PUSH_PENDING, stalling
the remaining data in msk->rtx_queue if the peer is waiting for it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-mptcp-cmason-fixes-v1-0-9205b77e9a2f@kernel.org?part=25

[PATCH mptcp-net 26/34] mptcp: don't send a FIN before the pending data on fallback

[Matthieu Baerts (NGI0)]

When an MPTCP connection falls back to TCP after having been shut down,
the data that is still in the write queue of the MPTCP socket is lost:
a FIN is sent at fallback time, and this data cannot be sent after it.
This can happen with TCP Fast Open (TFO), when a server writes data and
shuts the connection down before the third ACK, and this ACK has no
MPTCP options.

When the MPTCP socket has been shut down, mptcp_try_fallback() also
shuts the subflow down: if the DATA_FIN had already been sent, nothing
would send a FIN on this subflow otherwise. The commit introducing this
only considered the case where all the data had already been pushed to
the subflow.

The fix used here is to only shut the subflow down at fallback time if
no data is pending. Otherwise, mptcp_check_send_data_fin() will do it
once this data has been pushed, e.g. by the previous commit.

This issue has been found by an AI code review agent, while reviewing
the code of the previous commit, and confirmed with a new Packetdrill
test, where the server writes data and shuts the connection down before
the third ACK, from which the MPTCP options are removed. Without this
patch, a FIN was sent right after this ACK, and the data was never
sent:

  inbound injected packet:  0.240127 . 501:501(0) ack 4103187975 win 450
  outbound sniffed packet:  0.240207 F. 4103187975:4103187975(0) ack 501 win 1050
  outbound sniffed packet:  1.347800 F. 4103187975:4103187975(0) ack 501 win 1050
  outbound sniffed packet:  2.499829 F. 4103187975:4103187975(0) ack 501 win 1050
  outbound sniffed packet:  4.739843 F. 4103187975:4103187975(0) ack 501 win 1050
  server-tfo-fallback-shutdown-before-3rd-ack.pkt:27: error handling packet: Timed out waiting for packet

Fixes: d51991e2e314 ("mptcp: fix shutdown vs fallback race")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index e61b6a3a955d..d76eaed4f01e 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1339,7 +1339,10 @@ static inline bool mptcp_try_fallback(struct sock *ssk, int fb_mib)
 
 	if (!__mptcp_try_fallback(msk, fb_mib))
 		return false;
-	if (READ_ONCE(msk->snd_data_fin_enable) && !(ssk->sk_shutdown & SEND_SHUTDOWN)) {
+
+	/* no FIN before pending data: see mptcp_check_send_data_fin() */
+	if (READ_ONCE(msk->snd_data_fin_enable) && !mptcp_send_head(sk) &&
+	    !(ssk->sk_shutdown & SEND_SHUTDOWN)) {
 		gfp_t saved_allocation = ssk->sk_allocation;
 
 		/* we are in a atomic (BH) scope, override ssk default for data

-- 
2.55.0

[PATCH mptcp-net 27/34] tcp: do not pass a BPF reqsk to an MPTCP listener

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

A slab-out-of-bounds write and a panic can occur if a tc BPF program
that validates SYN cookies assigns an ACK to a TCP listener, and the ACK
then goes to an MPTCP listener of the same SO_REUSEPORT group.

bpf_sk_assign_tcp_reqsk() attaches a plain TCP request sock to the skb,
and returns -EINVAL if the program passes in an MPTCP listener. But
inet_steal_sock() and inet6_steal_sock() select a listener again from
the SO_REUSEPORT group of the listener that was passed in, and the
result can be an MPTCP listener. cookie_bpf_check() then passes the
request sock to mptcp_subflow_init_cookie_req(), which writes to members
of the larger struct mptcp_subflow_request_sock, so the writes can land
in the next object of the slab.

The fix used here is to drop the segment in cookie_bpf_check() if the
listener is an MPTCP listener.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch:
a tc BPF program validating SYN cookies -- the one from the BPF
selftests -- and an SO_REUSEPORT group with a TCP listener and an MPTCP
one, the latter being selected. With KASAN, the original version of this
patch reported:

  BUG: KASAN: slab-out-of-bounds in mptcp_subflow_init_cookie_req
  Write of size 1 at addr ... by task python3
   mptcp_subflow_init_cookie_req
   cookie_bpf_check
   cookie_v4_check
   tcp_v4_do_rcv
   tcp_v4_rcv

Without debug options, and with request sockets of half-open connections
next to the corrupted one, the kernel panicked a second later, when the
timer of a request socket fired:

  BUG: kernel NULL pointer dereference, address: 000000000000058f
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  Oops: Oops: 0000 [#1] SMP NOPTI
  CPU: 3 UID: 0 PID: 0 Comm: swapper/3 Not tainted 7.4.0-rc1+ #1 PREEMPT(lazy)
  RIP: 0010:tcp_syn_ack_timeout+0xd/0x30
  Call Trace:
   <IRQ>
   reqsk_timer_handler+0x22e/0x500
   call_timer_fn+0x2a/0x140
   __run_timers+0x1f7/0x280
   run_timer_softirq+0x49/0xf0
   handle_softirqs+0xfa/0x300
   __irq_exit_rcu+0xb5/0xf0
   sysvec_apic_timer_interrupt+0x73/0x80
   </IRQ>
  Kernel panic - not syncing: Fatal exception in interrupt

Fixes: e472f88891ab ("bpf: tcp: Support arbitrary SYN Cookie.")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Shorter comment. Commit message: how the issue was found, and its
  symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/ipv4/syncookies.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 4e0ff3b82062..8433c856a7d8 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -308,7 +308,8 @@ struct request_sock *cookie_bpf_check(struct sock *sk, struct sk_buff *skb)
 	skb->sk = NULL;
 	skb->destructor = NULL;
 
-	if (cookie_tcp_reqsk_init(sk, skb, req)) {
+	/* reuseport can pick an MPTCP listener: req is too small for it */
+	if (sk_is_mptcp(sk) || cookie_tcp_reqsk_init(sk, skb, req)) {
 		reqsk_free(req);
 		req = NULL;
 	}

-- 
2.55.0

[PATCH mptcp-net 28/34] mptcp: reclaim forward memory after coalescing skbs

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

If MPTCP sockets receive many small segments whose data stays queued,
TCP can stall on the whole host, and an MPTCP server can be OOM killed
in its memory cgroup, although almost no memory is in use.

When the MPTCP socket (msk) takes over an skb from a subflow,
mptcp_borrow_fwdmem() adds skb->truesize to the sk_forward_alloc of the
msk, and the subflow reserves new memory for the next skb. Both
sk_forward_alloc and sk_rmem_alloc count against tcp_mem and the memory
cgroup, but only sk_rmem_alloc has a limit, sk_rcvbuf, so what queueing
the skb does not move to sk_rmem_alloc has to be released. But
mptcp_try_coalesce() moves only the increase in truesize of the skb that
receives the data, and skb_condense() in mptcp_data_queue_ofo() can
lower skb->truesize, so the rest piles up in sk_forward_alloc.

The fix used here is to return every whole page of sk_forward_alloc with
sk_mem_reclaim(), in mptcp_try_coalesce() once the merged skb is freed,
and in mptcp_data_queue_ofo() once the skb is queued.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch:
a remote peer sends 33000 segments of one byte after a hole in the data
sequence, which are merged in the out-of-order queue. Without this
patch, the memory forward-allocated by the MPTCP socket -- msk_f -- grew
with each merged segment, above 29 MB, versus 3200 bytes with it:

  SNAP label=stage1-1-connections tcp_mem_pages=7168 [...] msk_f=29445248 [...]
  [...] OFOQueue=33000 OFOMerge=32862 [...]

With more connections, the original version of this patch reported that
the TCP memory went above tcp_mem[2], stalling the other TCP
connections, or that the memory cgroup went above its limit.

Fixes: 9db5b3cec4ec ("mptcp: borrow forward memory from subflow")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Shorter comment. Commit message: the rest of the borrowed amount has
  to be released + how the issue was found, and its symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 388e77da8f16..0758b048316e 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -197,6 +197,9 @@ static bool mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
 	sk_mem_charge(sk, delta);
 	kfree_skb_partial(from, fragstolen);
 
+	/* release what is left of the borrowed 'from' truesize */
+	sk_mem_reclaim(sk);
+
 	return true;
 }
 
@@ -422,6 +425,9 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
 end:
 	skb_condense(skb);
 	skb_set_owner_r(skb, sk);
+
+	/* skb_condense() can shrink the truesize borrowed from the subflow */
+	sk_mem_reclaim(sk);
 }
 
 static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,

-- 
2.55.0

[PATCH mptcp-net 29/34] mptcp: don't charge a shared page twice at accept()

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

accept() of an MPTCP connection that has already received data can leak
one page of memory cgroup charge, and the leaked charges can get the
server OOM killed.

At accept(), __inet_accept() charges the MPTCP socket (msk) for the
memory that it holds, rounded up to whole pages, and
mptcp_graft_subflows() does the same for each subflow. A page is
uncharged only once, when the socket that holds it releases it, so
rounding up each socket on its own is only correct while each page
belongs to one socket. But for each skb that the msk takes over from a
subflow, it borrows skb->truesize bytes from the pages that the subflow
reserved, so the two can share a page:

    reserved by the subflow      4096 bytes  1 page
      now held by the msk         896 bytes  charged for 1 page
      still held by the subflow  3200 bytes  charged for 1 page

The fix used here is to keep a running sum of what the msk and its
subflows hold, and to let the new mptcp_charge_subflow() charge each
subflow only for the pages that it adds to the rounded-up sum.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch:
an MPTCP server in a memory cgroup, the first byte of the client
arriving before accept(). Without this patch, 4096 bytes stayed charged
to the memory cgroup for each connection, after its end:

  SNAP label=after-1000 rounds=1000 sockets_left_in_srv=0 [...] c_sock=4096000 [...]
  SNAP label=after-5000 rounds=5000 sockets_left_in_srv=0 [...] c_sock=20480000 [...]

With a memory limit, the original version of this patch reported that
the server was killed by the OOM killer after about 14,400 connections.

Fixes: 9db5b3cec4ec ("mptcp: borrow forward memory from subflow")
Signed-off-by: Chris Mason <mason@kernel.org>
[ Shorter comment. Commit message: how the issue was found, and its
  symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0758b048316e..dd441e5aa435 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -4465,10 +4465,26 @@ static int mptcp_listen(struct socket *sock, int backlog)
 	return err;
 }
 
+/* The msk and its subflows share pages: charge their rounded-up sum */
+static int mptcp_charge_subflow(struct sock *sk, struct sock *ssk, int mem)
+{
+	int new_mem = mem + ssk->sk_forward_alloc +
+		      atomic_read(&ssk->sk_rmem_alloc);
+	int amt = sk_mem_pages(new_mem) - sk_mem_pages(mem);
+	gfp_t gfp = GFP_KERNEL | __GFP_NOFAIL;
+
+	mem_cgroup_sk_inherit(sk, ssk);
+	if (amt > 0)
+		mem_cgroup_sk_charge(ssk, amt, gfp);
+	kmem_cache_charge(ssk, gfp);
+	return new_mem;
+}
+
 static void mptcp_graft_subflows(struct sock *sk)
 {
 	struct mptcp_subflow_context *subflow;
 	struct mptcp_sock *msk = mptcp_sk(sk);
+	int mem;
 
 	if (mem_cgroup_sockets_enabled) {
 		LIST_HEAD(join_list);
@@ -4486,6 +4502,9 @@ static void mptcp_graft_subflows(struct sock *sk)
 		__mptcp_flush_join_list(sk, &join_list);
 	}
 
+	/* What __inet_accept() just charged for the msk, before rounding up. */
+	mem = sk->sk_forward_alloc + atomic_read(&sk->sk_rmem_alloc);
+
 	mptcp_for_each_subflow(msk, subflow) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 
@@ -4501,7 +4520,7 @@ static void mptcp_graft_subflows(struct sock *sk)
 			goto unlock;
 
 		__mptcp_inherit_cgrp_data(sk, ssk);
-		__mptcp_inherit_memcg(sk, ssk, GFP_KERNEL);
+		mem = mptcp_charge_subflow(sk, ssk, mem);
 
 unlock:
 		release_sock(ssk);

-- 
2.55.0

[PATCH mptcp-net 30/34] mptcp: don't charge the backlog twice at accept()

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

accept() can permanently overcharge the memory cgroup of an MPTCP
server, so the server can be OOM killed, if the new connection received
data while its MPTCP socket was locked.

The sockets of an MPTCP connection have no memory cgroup before
accept(), which then charges the sk_forward_alloc and the sk_rmem_alloc
of the MPTCP socket (msk) and of each subflow. mptcp_graft_subflows()
also charges msk->backlog_unaccounted: the truesize of the skbs that
__mptcp_add_backlog() queued while the socket lock of the msk was held
and their subflow had no memory cgroup. But the first charge already
covers each of these skbs that has not been freed, in the sk_rmem_alloc
of its subflow or of the msk, so no later uncharge matches the extra
charge.

The fix used here is to remove msk->backlog_unaccounted, its charge in
mptcp_graft_subflows() and the warning that tests it in
mptcp_can_spool_backlog(). An RFC patch [1] removes the same lines, as
part of another approach.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch,
on a kernel with the previous patch: 100,000 connections to an MPTCP
server in a memory cgroup. Without this patch, about 63 MB stayed
charged to this memory cgroup once all the connections were closed, and
to its parent after its removal:

  SNAP label=after-100000 rounds=100000 sockets_left_in_srv=0 tcp_mem_pages=0 [...] c_sock=63295488 [...]
  [...]
  SNAP label=c-removed rounds=100000 sockets_left_in_srv=0 tcp_mem_pages=0 c=removed [...] p_sock=63295488 [...]

And about 12 MB with only 50 connections, each getting 20,000 duplicates
of a byte before accept(), beside bare ACKs with RM_ADDR, which make the
MPTCP worker take the socket lock:

  SNAP label=after-50 rounds=50 sockets_left_in_srv=0 tcp_mem_pages=256 [...] c_sock=12881920 [...]

Fixes: 6228efe0cc01 ("mptcp: leverage the backlog for RX packet processing")
Link: https://lore.kernel.org/all/20260924080219.1036588-4-caixinchen1@huawei.com/ [1]
Signed-off-by: Chris Mason <mason@kernel.org>
[ No comment change: the commit message explains why the backlog is
  covered. Commit message: refer to the RFC [1] removing the same lines
  + how the issue was found, and its symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 35 +----------------------------------
 net/mptcp/protocol.h |  1 -
 2 files changed, 1 insertion(+), 35 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index dd441e5aa435..2a47107001fc 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -798,12 +798,6 @@ static void __mptcp_add_backlog(struct sock *sk,
 
 account:
 	WRITE_ONCE(msk->backlog_len, msk->backlog_len + delta);
-
-	/* Possibly not accept()ed yet, keep track of memory not CG
-	 * accounted, mptcp_graft_subflows() will handle it.
-	 */
-	if (!mem_cgroup_from_sk(ssk))
-		msk->backlog_unaccounted += delta;
 }
 
 static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
@@ -2418,12 +2412,6 @@ static bool mptcp_can_spool_backlog(struct sock *sk, struct list_head *skbs)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
 
-	/* After CG initialization, subflows should never add skb before
-	 * gaining the CG themself.
-	 */
-	DEBUG_NET_WARN_ON_ONCE(msk->backlog_unaccounted && sk->sk_socket &&
-			       mem_cgroup_from_sk(sk));
-
 	if (list_empty(&msk->backlog_list))
 		return false;
 
@@ -4492,8 +4480,7 @@ static void mptcp_graft_subflows(struct sock *sk)
 		/* Subflows joining after __inet_accept() will get the
 		 * mem CG properly initialized at mptcp_finish_join() time,
 		 * but subflows pending in join_list need explicit
-		 * initialization before flushing `backlog_unaccounted`
-		 * or MPTCP can later unexpectedly observe unaccounted memory.
+		 * initialization by the loop below.
 		 */
 		mptcp_data_lock(sk);
 		list_splice_init(&msk->join_list, &join_list);
@@ -4525,26 +4512,6 @@ static void mptcp_graft_subflows(struct sock *sk)
 unlock:
 		release_sock(ssk);
 	}
-
-	if (mem_cgroup_sk_enabled(sk)) {
-		gfp_t gfp = GFP_KERNEL | __GFP_NOFAIL;
-		int amt;
-
-		/* Account the backlog memory; prior accept() is aware of
-		 * fwd and rmem only.
-		 */
-		mptcp_data_lock(sk);
-		amt = sk_mem_pages(sk->sk_forward_alloc +
-				   msk->backlog_unaccounted +
-				   atomic_read(&sk->sk_rmem_alloc)) -
-		      sk_mem_pages(sk->sk_forward_alloc +
-				   atomic_read(&sk->sk_rmem_alloc));
-		msk->backlog_unaccounted = 0;
-		mptcp_data_unlock(sk);
-
-		if (amt)
-			mem_cgroup_sk_charge(sk, amt, gfp);
-	}
 }
 
 static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index d76eaed4f01e..18222dd78fed 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -392,7 +392,6 @@ struct mptcp_sock {
 
 	struct list_head backlog_list;	/* protected by the data lock */
 	u32		backlog_len;
-	u32		backlog_unaccounted;
 };
 
 #define mptcp_data_lock(sk) spin_lock_bh(&(sk)->sk_lock.slock)

-- 
2.55.0

[PATCH mptcp-net 31/34] mptcp: don't charge a subflow twice when it joins during accept()

[Matthieu Baerts (NGI0)]

A subflow that joins an MPTCP connection while accept() runs can be
charged twice to the memory cgroup of the server, for the memory that
it already holds when accept() charges it. Nothing removes this extra
charge.

At accept(), __inet_accept() gives the MPTCP socket (msk) the memory
cgroup of the caller, then sets its sk_socket. From then on,
mptcp_finish_join() gives this memory cgroup to a joining subflow, via
mptcp_sock_check_graft(), and each page that the subflow reserves is
charged. Because accept() owns the msk, such a subflow is added to the
join list, which mptcp_graft_subflows() moves to the subflow list
before charging each subflow for the memory that it holds. A subflow
that joined in between is then charged a second time for this memory,
which will be uncharged only once.

The window is short: from the end of __inet_accept() to the start of
mptcp_graft_subflows(). And the extra charge is not zero only if data
was received on the new subflow before mptcp_graft_subflows() gets to
it, while a peer must not send data on it before the fourth ACK
(RFC 8684, section 3.2).

The fix used here is to skip, in mptcp_graft_subflows(), the subflows
that already have a memory cgroup: they got it when they joined, and
their memory has been charged since then.

This issue has been found by an AI code review agent, while reviewing
the code of the commit "mptcp: don't charge a shared page twice at
accept()". It cannot be triggered on demand: the window is too narrow.
It has been reproduced with a 500 ms sleep added after __inet_accept(),
to widen it, and a Packetdrill script sending 1000 bytes on a subflow
joining during accept(). Without this patch, a page stayed charged to
the memory cgroup once all the sockets were freed, and none with it:

  memcg sock: 4096 bytes

Fixes: 6228efe0cc01 ("mptcp: leverage the backlog for RX packet processing")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 2a47107001fc..afc122e476cd 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -4503,7 +4503,8 @@ static void mptcp_graft_subflows(struct sock *sk)
 		if (!ssk->sk_socket)
 			mptcp_sock_graft(ssk, sk->sk_socket);
 
-		if (!mem_cgroup_sk_enabled(sk))
+		/* Got the memcg when joining: its memory is already charged */
+		if (!mem_cgroup_sk_enabled(sk) || mem_cgroup_from_sk(ssk))
 			goto unlock;
 
 		__mptcp_inherit_cgrp_data(sk, ssk);

-- 
2.55.0

[PATCH mptcp-net 32/34] mptcp: fix dummy mapping of overlapping segments

[Matthieu Baerts (NGI0)]

From: Geliang Tang <tanggeliang@kylinos.cn>

An MPTCP socket that has fallen back to TCP can silently lose received
data if a segment partly overlaps data received earlier, and recv() then
blocks, returning neither later data nor end of file.

After a fallback, subflow_check_data_avail() makes up a dummy mapping
for the skb at the head of the subflow's receive queue, in which map_seq
and map_subflow_seq must describe the same byte. But since commit
2834f8edd74d ("mptcp: cleanup fallback dummy mapping generation"), it
computes map_seq from the first byte of the skb, while map_subflow_seq
is still computed from copied_seq, the next unread byte.
mptcp_subflow_get_mapped_dsn() then returns the data sequence number
(DSN) of the first byte of the skb, instead of the one of copied_seq.

These two bytes differ for a segment that starts before rcv_nxt and ends
after it, since TCP does not trim it. mptcp_init_skb() then gives the
new bytes of this skb a DSN that is too low, and __mptcp_move_skb()
skips some of them as old data, leaving a hole in the stream:

    bytes 0 to 999 are received, the skb holds bytes 900 to 1499
      byte 1000 gets the DSN of byte 900: too low by the 100 old bytes
        that is 100 below ack_seq, the next DSN that MPTCP expects
          __mptcp_move_skb() skips bytes 1000 to 1099 as old data
            ack_seq is the DSN of byte 1400, copied_seq is byte 1500
              later data starts at the DSN of byte 1500: out of order
                the TCP FIN becomes a dummy DATA_FIN after the last byte
                  ack_seq does not reach it, so there is no end of file

Fix this by computing map_subflow_seq from the first byte of the skb as
well: mptcp_subflow_get_mapped_dsn() then returns the DSN of copied_seq.

Chris Mason sent the same fix in private, and found this issue with the
help of an LLM. It has been reproduced in a VM, with the reproducer he
shared, and with a new Packetdrill test: after a fallback to TCP, a
segment overlapping data already received is injected. Without this
patch, the new bytes at the beginning of this segment were lost, and the
connection stalled, with no end of file:

  reader   30.64: conn 1 BREAK: at my offset 1000 the bytes claim offset 1100: A GAP OF 100 BYTES
  reader   30.64: READ-TOTAL conn 1 outcome iii-gap-or-repeat bytes 1400 of 2500 [...]
  [...] end: hang: no data and no end of file for 30 s

And with the Packetdrill test:

  dss_fallback_overlap_client.pkt:24: runtime error in read call: Expected result 2500 but got 1400 with errno 115 (Operation now in progress)

Fixes: 2834f8edd74d ("mptcp: cleanup fallback dummy mapping generation")
Link: https://lore.kernel.org/4c48d016490d43b6892c622cbae858b48f176cff.1794304840.git.tanggeliang@kylinos.cn [1]
Co-developed-by: Chris Mason <mason@kernel.org>
Signed-off-by: Chris Mason <mason@kernel.org>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
[ Combine the commit messages of Geliang's [1] and Chris's versions.
  Commit message: how the issue was found, and its symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/subflow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 543368cfc890..64e8f5c36415 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1526,7 +1526,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 	skb = skb_peek(&ssk->sk_receive_queue);
 	subflow->map_valid = 1;
 	subflow->map_data_len = skb->len;
-	subflow->map_subflow_seq = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;
+	subflow->map_subflow_seq = TCP_SKB_CB(skb)->seq - subflow->ssn_offset;
 	subflow->map_seq = __mptcp_expand_seq(subflow->map_seq,
 					      subflow->iasn +
 					      TCP_SKB_CB(skb)->seq -

-- 
2.55.0

[PATCH mptcp-net 33/34] mptcp: continue the last mapping after a fallback

[Matthieu Baerts (NGI0)]

From: Chris Mason <mason@kernel.org>

An established MPTCP connection that falls back to TCP after a zero
window probe or a reinjection can stall in one direction, so that recv()
returns neither the remaining data nor end of file.

After a fallback, subflow_check_data_avail() gives each skb a dummy
mapping, at the data sequence number (DSN) iasn + SSN - 1, where iasn is
the DSN of the peer's first byte and SSN is the subflow sequence number.
This is right only as long as DSNs and SSNs advance together, but a zero
window probe or a reinjection maps old data a second time, which takes
new SSNs and no new DSN. Each later dummy mapping is then too high, so
__mptcp_move_skb() treats all further data as out of order:

    data    SSN 1 to 1000    DSN iasn to iasn + 999
    probe   SSN 1001         DSN iasn + 999, dropped
    ack_seq                  iasn + 1000, the next DSN expected
    dummy   SSN 1002         DSN iasn + 1001

The fix used here is to:
- keep in the new member dsn_base of the subflow context the first DSN
  of the peer's last mapping, minus its first SSN, plus 1
- compute the DSN of a dummy mapping as dsn_base + SSN - 1
- keep in the new member dsn_end the highest end of a mapping of the
  peer so far
- make mptcp_try_fallback() fail while the peer's last mapping ends
  below dsn_end, so that the caller resets the subflow

32 bits are enough for both: the dummy mapping only takes the lower 32
bits of its DSN, see __mptcp_expand_seq(), and the end of a mapping
stays within a window of dsn_end.

One case is left: if only the receiver falls back after a zero window
probe or a reinjection, it now delivers the first MPTCP-level
retransmission of data that was new when first sent, as new data, before
the sender resets the subflow, see "mptcp: stop resending data to a peer
that fell back". Without this patch, the receiver stalled instead.

This issue has been found with the help of an LLM, and reproduced in a
VM, with the reproducer shared with the original version of this patch,
and with a new Packetdrill test: between two unmodified Linux hosts,
nftables removing the MPTCP options after a zero window probe, a DSS
checksum failure, or a failed MP_JOIN. Without this patch, the reader
stalled for good, the data staying in the out-of-order queue:

  receiver  22.764: READ-TOTAL outcome ii-right-so-far-then-nothing bytes 2670592 of 8388608 [...]
  [...] end: hang: no data and no end of file for 20 s
  RESULT [...] R: DssFallback=1 [...] OFOQueue=204 NoDSSInWindow=145 [...]

And with the Packetdrill test:

  dss_fallback_after_probe_server.pkt:25: runtime error in read call: Expected result 2000 but got 1000 with errno 22 (Invalid argument)

Fixes: 2834f8edd74d ("mptcp: cleanup fallback dummy mapping generation")
Signed-off-by: Chris Mason <mason@kernel.org>
[ dsn_base and dsn_end on 32 bits: struct mptcp_subflow_context grows by
  8 bytes instead of 16, and lent_mem_frag stays in the same cacheline.
  Shorter comments. Moved after "mptcp: stop resending data to a peer
  that fell back": alone, this patch turns a stall into data delivered
  twice, when only the receiver falls back after a probe or a
  reinjection. Commit message: mention the remaining case + how the
  issue was found, and its symptoms. ]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.h |  9 ++++++++-
 net/mptcp/subflow.c  | 21 ++++++++++++++++++---
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 18222dd78fed..f0b93c31b16e 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -578,6 +578,8 @@ struct mptcp_subflow_context {
 	__wsum	map_data_csum;
 	u32	map_csum_len;
 	u32	prev_rtt_seq;
+	u32	dsn_base;	    /* MPC: DSN - SSN + 1 of peer's last map */
+	u32	dsn_end;	    /* MPC: highest end of the peer's maps */
 	u32	request_mptcp : 1,  /* send MP_CAPABLE */
 		request_join : 1,   /* send MP_JOIN */
 		request_bkup : 1,
@@ -602,7 +604,8 @@ struct mptcp_subflow_context {
 		mpc_drop : 1,	    /* the MPC option has been dropped in a rtx */
 		resetting : 1,	    /* subflow is resetting */
 		old_data_pending : 1, /* old data not acked with MPTCP opts */
-		__unused : 8;
+		map_old : 1,	    /* MPC: peer's last map is below dsn_end */
+		__unused : 7;
 	bool	data_avail;
 	bool	scheduled;
 	bool	pm_listener;	    /* a listener managed by the kernel PM? */
@@ -1332,6 +1335,10 @@ static inline bool mptcp_try_fallback(struct sock *ssk, int fb_mib)
 
 	msk = mptcp_sk(sk);
 
+	/* the peer might still be resending old data */
+	if (subflow->map_old && !__mptcp_check_fallback(msk))
+		return false;
+
 	/* queued old data would be sent without mapping after a fallback */
 	if (subflow->old_data_pending && !__mptcp_check_fallback(msk))
 		return false;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 64e8f5c36415..728b27be1ad0 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -540,6 +540,8 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
 
 	/* for fallback's sake */
 	subflow->map_seq = subflow->iasn;
+	subflow->dsn_base = subflow->iasn;
+	subflow->dsn_end = subflow->iasn;
 
 	WRITE_ONCE(msk->remote_key, subflow->remote_key);
 	WRITE_ONCE(msk->ack_seq, subflow->iasn);
@@ -1434,6 +1436,17 @@ static bool mptcp_subflow_fail(struct mptcp_sock *msk, struct sock *ssk)
 	return true;
 }
 
+/* Fallback: dummy maps go on from the last valid one, unless it is old */
+static void subflow_update_dsn_base(struct mptcp_subflow_context *subflow)
+{
+	u32 end = subflow->map_seq + subflow->map_data_len;
+
+	subflow->dsn_base = subflow->map_seq - subflow->map_subflow_seq + 1;
+	subflow->map_old = before(end, subflow->dsn_end);
+	if (!subflow->map_old)
+		subflow->dsn_end = end;
+}
+
 static bool subflow_check_data_avail(struct sock *ssk)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
@@ -1467,6 +1480,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
 		if (unlikely(!READ_ONCE(msk->can_ack)))
 			goto fallback;
 
+		subflow_update_dsn_base(subflow);
+
 		old_ack = READ_ONCE(msk->ack_seq);
 		ack_seq = mptcp_subflow_get_mapped_dsn(subflow);
 		pr_debug("msk ack_seq=%llx subflow ack_seq=%llx\n", old_ack,
@@ -1497,6 +1512,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 				subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
 				goto reset;
 			}
+			subflow_update_dsn_base(subflow);
 			WRITE_ONCE(subflow->data_avail, true);
 			return true;
 		}
@@ -1528,9 +1544,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
 	subflow->map_data_len = skb->len;
 	subflow->map_subflow_seq = TCP_SKB_CB(skb)->seq - subflow->ssn_offset;
 	subflow->map_seq = __mptcp_expand_seq(subflow->map_seq,
-					      subflow->iasn +
-					      TCP_SKB_CB(skb)->seq -
-					      subflow->ssn_offset - 1);
+					      subflow->dsn_base +
+					      subflow->map_subflow_seq - 1);
 	WRITE_ONCE(subflow->data_avail, true);
 	return true;
 }

-- 
2.55.0

[PATCH RFC mptcp-next mptcp-net 34/34] mptcp: reset when the DSS disappears after a DATA_ACK

[Matthieu Baerts (NGI0)]

RFC 8684, section 3.7, describes a fallback to TCP for the case where
the MPTCP options get through on the SYN, but not on the next packets.
A sender includes a DSS option in each segment "until one of the sent
segments has been acknowledged with a DSS option containing a Data
ACK": it then has "the confirmation that the DSS option passes in both
directions". After that, a subflow that loses its MPTCP options "SHOULD
be treated as broken and closed with a RST", with the "Middlebox
interference" reason. It's a *SHOULD*.

Commit 119d51e225fe ("mptcp: fallback when MPTCP opts are dropped after
1st data") did this fallback as long as allow_infinite_fallback is set,
i.e. also after this confirmation, and left the check of the DATA_ACK
for later [1]. Such a late fallback is fragile: the peer might still
resend data that was mapped before, or a zero window probe might have
mapped old data again. The previous commits "mptcp: do not fallback
while old data is in flight", "mptcp: stop resending data to a peer
that fell back" and "mptcp: continue the last mapping after a fallback"
handle these cases, except one: if only the receiver falls back, it can
deliver the first MPTCP-level retransmission of the sender as new data,
before the sender resets the subflow. Yeah, a very specific corner case.

Now, when data without DSS is found on a subflow, the subflow is reset
if a DATA_ACK received on it acknowledged data that had been sent: the
MPTCP options then passed in both directions. Except after an MP_FAIL
exchange: a fallback to TCP is then in progress, so data without DSS is
expected, even if the infinite mapping has not been seen. This also
closes the case above, but only for a receiver whose data has been
acknowledged.

Receiving a DATA_ACK is not enough: when the MPTCP options are only
dropped in one direction, the peer can fall back while the DATA_ACKs it
sent before acknowledged no data. A client that is not fully
established falls back when a packet from the server has no MPTCP
options, e.g. an ACK of its data, and the other hosts when they receive
data without DSS. Except after an MP_FAIL, Linux falls back without
sending an infinite mapping, then sends data without DSS: the other
side can only fall back too, as before. For the same reason, the amount
of received data -- e.g. more than an initial window -- is not used.

When a middlebox starts dropping the MPTCP options in the middle of a
connection, after a DATA_ACK acknowledged data, the subflow is then
reset, instead of falling back to TCP when it was safe. If it was the
only subflow, the MPTCP connection stays without subflow until the
close timeout -- 60 seconds by default, see net.mptcp.close_timeout --
unless a new one is created in between.

A new Packetdrill test checks this reset.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/519 [1]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Note: this commit changes the behaviour, it is for next. It fixes a very
specific corner case (but it follows what the RFC 8684 recommends to do)
but for that it needs an extra flag and more checks. Is it worth it? (I
think so, but that's why it is flagged as RFC.)
---
 net/mptcp/options.c  | 11 +++++++----
 net/mptcp/protocol.h |  3 ++-
 net/mptcp/subflow.c  | 20 +++++++++++++++++++-
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index c56ca5e4df14..ccd30e110934 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1128,7 +1128,7 @@ static void rwin_update(struct mptcp_sock *msk, struct sock *ssk,
 	subflow->rcv_wnd_sent = mptcp_rcv_wnd;
 }
 
-static void ack_update_msk(struct mptcp_sock *msk,
+static bool ack_update_msk(struct mptcp_sock *msk,
 			   struct sock *ssk,
 			   const struct sk_buff *skb,
 			   struct mptcp_options_received *mp_opt)
@@ -1137,6 +1137,7 @@ static void ack_update_msk(struct mptcp_sock *msk,
 	u32 win = ntohs(tcp_hdr(skb)->window);
 	struct sock *sk = (struct sock *)msk;
 	u64 snd_nxt;
+	bool acked;
 
 	mptcp_data_lock(sk);
 
@@ -1169,7 +1170,8 @@ static void ack_update_msk(struct mptcp_sock *msk,
 	if (after64(msk->wnd_end, snd_nxt))
 		__mptcp_check_push(sk, ssk);
 
-	if (after64(new_snd_una, old_snd_una)) {
+	acked = after64(new_snd_una, old_snd_una);
+	if (acked) {
 		__mptcp_snd_una_update(msk, new_snd_una);
 		__mptcp_data_acked(sk);
 	}
@@ -1179,6 +1181,7 @@ static void ack_update_msk(struct mptcp_sock *msk,
 	trace_ack_update_msk(mp_opt->data_ack,
 			     old_snd_una, new_snd_una,
 			     new_wnd_end, READ_ONCE(msk->wnd_end));
+	return acked;
 }
 
 bool mptcp_update_rcv_data_fin(struct mptcp_sock *msk, u64 data_fin_seq, bool use_64bit)
@@ -1335,8 +1338,8 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 	/* we can't wait for recvmsg() to update the ack_seq, otherwise
 	 * monodirectional flows will stuck
 	 */
-	if (mp_opt.use_ack)
-		ack_update_msk(msk, sk, skb, &mp_opt);
+	if (mp_opt.use_ack && ack_update_msk(msk, sk, skb, &mp_opt))
+		subflow->dss_confirmed = 1; /* MPTCP opts pass both ways */
 	rwin_update(msk, sk, skb);
 
 	/* Zero-data-length packets are dropped by the caller and not
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index f0b93c31b16e..36121de59ab9 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -605,7 +605,8 @@ struct mptcp_subflow_context {
 		resetting : 1,	    /* subflow is resetting */
 		old_data_pending : 1, /* old data not acked with MPTCP opts */
 		map_old : 1,	    /* MPC: peer's last map is below dsn_end */
-		__unused : 7;
+		dss_confirmed : 1,  /* a DATA_ACK acked our data */
+		__unused : 6;
 	bool	data_avail;
 	bool	scheduled;
 	bool	pm_listener;	    /* a listener managed by the kernel PM? */
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 728b27be1ad0..68a9fe29a5e8 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1447,6 +1447,22 @@ static void subflow_update_dsn_base(struct mptcp_subflow_context *subflow)
 		subflow->dsn_end = end;
 }
 
+/* RFC 8684, 3.7: no fallback once a DATA_ACK acked our data, except MP_FAIL */
+static bool subflow_nodss_is_fatal(struct mptcp_sock *msk,
+				   const struct mptcp_subflow_context *subflow)
+{
+	bool fatal;
+
+	if (!subflow->dss_confirmed)
+		return false;
+
+	/* before a fallback, only an MP_FAIL clears allow_subflows */
+	spin_lock_bh(&msk->fallback_lock);
+	fatal = msk->allow_subflows;
+	spin_unlock_bh(&msk->fallback_lock);
+	return fatal;
+}
+
 static bool subflow_check_data_avail(struct sock *ssk)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
@@ -1517,7 +1533,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
 			return true;
 		}
 
-		if (!mptcp_try_fallback(ssk, MPTCP_MIB_DSSFALLBACK)) {
+		if ((status == MAPPING_NODSS &&
+		     subflow_nodss_is_fatal(msk, subflow)) ||
+		    !mptcp_try_fallback(ssk, MPTCP_MIB_DSSFALLBACK)) {
 			/* fatal protocol error, reset the subflow and let the
 			 * worker close it
 			 */

-- 
2.55.0

Re: [PATCH mptcp-net 00/34] mptcp: misc fixes

[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 (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Unstable: 1 failed test(s): selftest_diag ⚠️ 
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Unstable: 1 failed test(s): validation:sched-weak-secondary ⚠️ 
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/43501866652

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


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)