When an accepted remote address is used by a subflow that is
closed before the corresponding RM_ADDR is received, the
add_addr_accepted counter is not decremented when the subflow
is removed.

As a result, the accepted-address budget can remain consumed
for the lifetime of the MPTCP connection. A subsequent ADD_ADDR
is then not acted upon because the peer is considered to have
already reached the add_addr_accepted limit.

Release the accepted-address slot when the subflow using the
remote address is closed, so that a later RM_ADDR and subsequent
ADD_ADDR can correctly reuse the available budget.

This allows a new MP_JOIN to be created after a previously
accepted subflow has been closed and its address has been
removed.

Fixes: 1c1f72137598 ("mptcp: pm: only decrement add_addr_accepted for MPJ req")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/498
Signed-off-by: Akshit Patadiya <akshit@mpiricsoftware.com>
---
 net/mptcp/pm.c        |  8 +++---
 net/mptcp/pm_kernel.c | 63 ++++++++++++++++++++++++++++++++++++-------
 net/mptcp/protocol.h  |  3 +++
 3 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index ba7c6f80a183..b6edb9df3216 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -681,8 +681,10 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
 		return;
 
 	spin_lock_bh(&pm->lock);
-	if (update_subflows)
+	if (update_subflows) {
 		__mptcp_pm_close_subflow(msk);
+		mptcp_pm_nl_close_subflow(msk, subflow);
+	}
 
 	/* Even if this subflow is not really established, tell the PM to try
 	 * to pick the next ones, if possible.
@@ -786,7 +788,6 @@ static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
 
 	for (i = 0; i < rm_list->nr; i++) {
 		u8 rm_id = rm_list->ids[i];
-		bool removed = false;
 
 		mptcp_for_each_subflow_safe(msk, subflow, tmp) {
 			struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
@@ -807,7 +808,6 @@ static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
 				 i, rm_id, id, remote_id, msk->mpc_endpoint_id);
 			spin_unlock_bh(&msk->pm.lock);
 			mptcp_subflow_shutdown(sk, ssk, how);
-			removed |= subflow->request_join;
 
 			/* the following takes care of updating the subflows counter */
 			mptcp_close_ssk(sk, ssk, subflow);
@@ -819,7 +819,7 @@ static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
 
 		if (rm_type == MPTCP_MIB_RMADDR) {
 			__MPTCP_INC_STATS(sock_net(sk), rm_type);
-			if (removed && mptcp_pm_is_kernel(msk))
+			if (mptcp_pm_is_kernel(msk))
 				mptcp_pm_nl_rm_addr(msk, rm_id);
 		}
 	}
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index d3014bf57bf3..8b087677a4e7 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -694,8 +694,13 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 	spin_lock_bh(&msk->pm.lock);
 
 	if (sf_created) {
-		/* add_addr_accepted is not decr for ID 0 */
-		if (remote.id)
+		/* ID 0 is not accounted: the remote address of the initial
+		 * subflow is known from the beginning. Remember the other
+		 * accepted IDs, so the counter can be balanced later on even
+		 * if the linked subflows are gone by then.
+		 */
+		if (remote.id &&
+		    !__test_and_set_bit(remote.id, msk->pm.id_accepted_bitmap))
 			msk->pm.add_addr_accepted++;
 		if (msk->pm.add_addr_accepted >= limit_add_addr_accepted ||
 		    msk->pm.extra_subflows >= limit_extra_subflows)
@@ -705,16 +710,53 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 
 void mptcp_pm_nl_rm_addr(struct mptcp_sock *msk, u8 rm_id)
 {
-	if (rm_id && !WARN_ON_ONCE(msk->pm.add_addr_accepted == 0)) {
-		u8 limit_add_addr_accepted =
-			mptcp_pm_get_limit_add_addr_accepted(msk);
+	u8 limit_add_addr_accepted;
 
-		/* Note: if the subflow has been closed before, this
-		 * add_addr_accepted counter will not be decremented.
-		 */
-		if (--msk->pm.add_addr_accepted < limit_add_addr_accepted)
-			WRITE_ONCE(msk->pm.accept_addr, true);
+	/* Only remote addresses that have been accepted by this host are
+	 * accounted: not ID 0, and not MP_JOIN requests initiated by the peer.
+	 * The bit, not the presence of a subflow, is what tells them apart, so
+	 * this works even when the subflows are already closed, and a
+	 * duplicated RM_ADDR is a no-op.
+	 */
+	if (!rm_id || !__test_and_clear_bit(rm_id, msk->pm.id_accepted_bitmap))
+		return;
+
+	if (WARN_ON_ONCE(msk->pm.add_addr_accepted == 0))
+		return;
+
+	limit_add_addr_accepted = mptcp_pm_get_limit_add_addr_accepted(msk);
+	if (--msk->pm.add_addr_accepted < limit_add_addr_accepted)
+		WRITE_ONCE(msk->pm.accept_addr, true);
+}
+
+/* Called with the PM lock held, from the subflow close path, before the
+ * subflow is removed from conn_list.
+ */
+void mptcp_pm_nl_close_subflow(struct mptcp_sock *msk,
+			       const struct mptcp_subflow_context *subflow)
+{
+	u8 remote_id = READ_ONCE(subflow->remote_id);
+	struct mptcp_subflow_context *iter;
+
+	/* Only the subflows this host has created upon an ADD_ADDR reception
+	 * are accounted, and never the initial one.
+	 */
+	if (!subflow->request_join || !remote_id ||
+	    !test_bit(remote_id, msk->pm.id_accepted_bitmap))
+		return;
+
+	/* The remote address can still be used by another subflow, e.g. with
+	 * fullmesh endpoints.
+	 */
+	mptcp_for_each_subflow(msk, iter) {
+		if (iter == subflow || iter->close_event_done)
+			continue;
+		if (iter->request_join &&
+		    READ_ONCE(iter->remote_id) == remote_id)
+			return;
 	}
+
+	mptcp_pm_nl_rm_addr(msk, remote_id);
 }
 
 static bool address_use_port(struct mptcp_pm_addr_entry *entry)
@@ -1668,6 +1710,7 @@ static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
 	WRITE_ONCE(pm->accept_subflow, subflows_allowed);
 
 	bitmap_fill(pm->id_avail_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
+	bitmap_zero(pm->id_accepted_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
 }
 
 struct mptcp_pm_ops mptcp_pm_kernel = {
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7e168e450fb0..54663d7aca34 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -243,6 +243,7 @@ struct mptcp_pm_data {
 	);
 
 	DECLARE_BITMAP(id_avail_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
+	DECLARE_BITMAP(id_accepted_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
 	struct mptcp_rm_list rm_list_tx;
 	struct mptcp_rm_list rm_list_rx;
 };
@@ -1127,6 +1128,8 @@ void mptcp_pm_send_ack(struct mptcp_sock *msk,
 		       bool prio, bool backup);
 void mptcp_pm_addr_send_ack(struct mptcp_sock *msk);
 void mptcp_pm_nl_rm_addr(struct mptcp_sock *msk, u8 rm_id);
+void mptcp_pm_nl_close_subflow(struct mptcp_sock *msk,
+			       const struct mptcp_subflow_context *subflow);
 void mptcp_pm_rm_subflow(struct mptcp_sock *msk,
 			 const struct mptcp_rm_list *rm_list);
 void mptcp_pm_rm_addr_received(struct mptcp_sock *msk,
-- 
2.43.0

Hi Akshit,

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): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/31697873897

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


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)

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent updates to msk->pm.rm_list_rx can cause RM_ADDR IDs to be silently skipped, resulting in leaked subflows.
--

--- Patch [1]: [PATCH mptcp-next] mptcp: fix add_addr_accepted accounting on subflow close ---
commit 09eae8ab35dfee45655629d4bbc00b4a93d797f3
Author: Akshit Patadiya <akshit@mpiricsoftware.com>

mptcp: fix add_addr_accepted accounting on subflow close

When an accepted remote address is used by a subflow that is closed before
the corresponding RM_ADDR is received, the add_addr_accepted counter is not
decremented. This commit releases the accepted-address slot when the subflow
closes, allowing subsequent ADD_ADDR requests to reuse the budget.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/498

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
[ ... ]
> @@ -786,7 +788,6 @@ static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
>  
>  	for (i = 0; i < rm_list->nr; i++) {
>  		u8 rm_id = rm_list->ids[i];
> -		bool removed = false;
>  
>  		mptcp_for_each_subflow_safe(msk, subflow, tmp) {
>  			struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> @@ -807,7 +808,6 @@ static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
>  				 i, rm_id, id, remote_id, msk->mpc_endpoint_id);
>  			spin_unlock_bh(&msk->pm.lock);
>  			mptcp_subflow_shutdown(sk, ssk, how);

[Severity: High]
This isn't a bug introduced by this patch, but could this loop miss RM_ADDR 
IDs and leak subflows due to concurrent list updates?

In mptcp_pm_rm_addr_or_subflow(), this loop iterates over the rm_list (which 
is passed as &msk->pm.rm_list_rx) and temporarily drops pm->lock around 
mptcp_subflow_shutdown().

If a remote peer sends multiple RM_ADDR options in rapid succession, softirq 
context can call mptcp_pm_rm_addr_received(), which unconditionally overwrites 
pm->rm_list_rx while scheduling work:

net/mptcp/pm.c:mptcp_pm_rm_addr_received() {
    ...
    if (mptcp_pm_schedule_work(msk, MPTCP_PM_RM_ADDR_RECEIVED))
        pm->rm_list_rx = *rm_list;
    ...
}

If pm->rm_list_rx is overwritten mid-iteration while the worker thread dropped 
pm->lock, wouldn't the loop in mptcp_pm_rm_addr_or_subflow() either terminate 
early or read incorrect indices from the new list, silently skipping unhandled 
IDs from the original list and leaking the associated resources?

> -			removed |= subflow->request_join;
>  
>  			/* the following takes care of updating the subflows counter */
>  			mptcp_close_ssk(sk, ssk, subflow);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813113208.3809139-1-akshit@mpiricsoftware.com?part=1