[PATCH mptcp-net v4 0/8] mptcp: pm: userspace: properly deal with the ID0 case

[PATCH mptcp-net v4 0/8] mptcp: pm: userspace: properly deal with the ID0 case

[Matthieu Baerts (NGI0)]

In MPTCP, the local address and port used by the initial subflow has the
ID "0". It means that when this address and port are used for some
operations -- e.g. creating a new subflow -- they should be linked to
the ID0, and no other addresses and ports can get this special ID while
the initial IP address and port is used.

Instead of dealing with the ID0 case as an exception, add it to the
local addr list, and deal with it like the others, with minor
exceptions. That way, it seems easier to maintain instead of adding new
exceptions at a few places, at the cost of a few more bytes, which seems
OK in this mode.

The first patch modifies add the initial address to the list, and the
others remove exceptions, and validate that in the selftests.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v4:
- patch 2: avoid two iterations (Geliang)
- patch 3: move the declaration of the new helper to one line (Geliang)
- Link to v3: https://patch.msgid.link/20260807-mptcp-pm-userspace-id0-case-v3-0-de9088549924@kernel.org

Changes in v3:
- patch 1: split exceptions + set msk port to 0 like before this series
- patches 2-3: new: from patch 1 + fix
- patch 6: use new is_init_id0 helper
- patch 8: avoid conflicts with WIP BPF MPTCP PM patches
- Link to v2: https://patch.msgid.link/20260806-mptcp-pm-userspace-id0-case-v2-0-fea1808e1c57@kernel.org

Changes in v2:
- patch 1: commit msg + local_addr_used and needs_id exceptions
- patch 2: commit msg
- Link to v1: https://patch.msgid.link/20260727-mptcp-pm-userspace-id0-case-v1-0-9877f02a9bae@kernel.org

---
Matthieu Baerts (NGI0) (8):
      mptcp: pm: userspace: properly handle the ID0 case
      mptcp: pm: userspace: lookup: match port in priority
      mptcp: pm: userspace: ID0 is not part of local_addr_used
      mptcp: pm: userspace: allow announcing ID0 addr
      mptcp: pm: userspace: no ID0 exception for RM_ADDR
      mptcp: pm: userspace: don't dump initial ID0
      selftests: mptcp: join: new ID0 subflow from the right IP
      mptcp: pm: restrict in-kernel worker actions to this PM

 net/mptcp/pm.c                                  |  8 ++-
 net/mptcp/pm_userspace.c                        | 92 ++++++++++++-------------
 net/mptcp/protocol.h                            |  1 +
 tools/testing/selftests/net/mptcp/mptcp_join.sh |  7 +-
 4 files changed, 55 insertions(+), 53 deletions(-)
---
base-commit: bcc5638fabf418e763621a432840cf55287e604d
change-id: 20260724-mptcp-pm-userspace-id0-case-f074f64466c8

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

[PATCH mptcp-net v4 1/8] mptcp: pm: userspace: properly handle the ID0 case

[Matthieu Baerts (NGI0)]

In MPTCP, the local address and port used by the initial subflow has the
ID "0". It means that when this address and port are used for some
operations -- e.g. creating a new subflow -- they should be linked to
the ID0, and no other addresses and ports can get this special ID while
the initial IP address and port is used.

So far, the ID0 case was handled as an exception: each operation dealing
with the ID0 had to be handled differently. Except that this was done in
some places like removing the ID0, but not everywhere the list of local
addresses was iterated. This way of handling the ID0 is prone to bugs
and harder to maintain. Instead, the initial local address corresponding
to ID0 can be added to the list when a connection is created, and the
number of exceptions can be dramatically reduced, handling this case
like any others with existing local address. The existing exceptions are
going to be removed in the following patches.

The main downside of this is that each connection will now have one
allocated entry added the list, possibly one more than before. But that
seems OK to do that with the userspace PM where the path management is
done per connection, with many Netlink messages sent back and forth.
Adding a few more bytes per connections on such setup seems acceptable.

Fixes: 4638de5aefe5 ("mptcp: handle local addrs announced by userspace PMs")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
sashiko: check the next patches before pointing at existing issues.
v2:
 - List new exceptions in the commit message.
 - Handle local_addr_used with the ID0 case properly. (Sashiko)
 - Handle needs_id where the addrs don't match, but the IDs (0) does. (S)
v3:
 - Split local_addr_used case: that's fixing another commit.
 - Set port to 0 (msk) instead of adding exceptions in lookups.
---
 net/mptcp/pm.c           |  4 ++++
 net/mptcp/pm_userspace.c | 20 ++++++++++++++++++++
 net/mptcp/protocol.h     |  1 +
 3 files changed, 25 insertions(+)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d7c5b50b34cc..e9a538296200 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -551,6 +551,10 @@ void mptcp_pm_new_connection(struct mptcp_sock *msk, const struct sock *ssk, int
 	pr_debug("msk=%p, token=%u side=%d\n", msk, READ_ONCE(msk->token), server_side);
 
 	WRITE_ONCE(pm->server_side, server_side);
+
+	if (mptcp_pm_is_userspace(msk))
+		mptcp_pm_userspace_created(msk, ssk);
+
 	mptcp_event(MPTCP_EVENT_CREATED, msk, ssk, GFP_ATOMIC);
 }
 
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index f723a134356f..45faee894ac3 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -707,6 +707,26 @@ int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
 	return ret;
 }
 
+/* Add the initial local address (ID0) to the local list: easier that way */
+void mptcp_pm_userspace_created(struct mptcp_sock *msk, const struct sock *ssk)
+{
+	struct mptcp_pm_addr_entry *entry;
+
+	entry = sock_kmalloc((struct sock *)msk, sizeof(*entry), GFP_ATOMIC);
+	/* Fine not to handle the ID0 case in memory pressure */
+	if (!entry)
+		return;
+
+	memset(entry, 0, sizeof(*entry));
+	mptcp_local_address((struct sock_common *)ssk, &entry->addr);
+	entry->addr.port = 0; /* msk port */
+
+	spin_lock_bh(&msk->pm.lock);
+	list_add_tail_rcu(&entry->list, &msk->pm.userspace_pm_local_addr_list);
+	/* The initial address ID doesn't increment local_addr_used */
+	spin_unlock_bh(&msk->pm.lock);
+}
+
 static struct mptcp_pm_ops mptcp_pm_userspace = {
 	.get_local_id		= mptcp_pm_userspace_get_local_id,
 	.get_priority		= mptcp_pm_userspace_get_priority,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3be60da12924..8c219fabfc9f 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1260,6 +1260,7 @@ void __init mptcp_pm_userspace_register(void);
 void __init mptcp_pm_nl_init(void);
 void mptcp_pm_worker(struct mptcp_sock *msk);
 void __mptcp_pm_kernel_worker(struct mptcp_sock *msk);
+void mptcp_pm_userspace_created(struct mptcp_sock *msk, const struct sock *ssk);
 u8 mptcp_pm_get_endp_signal_max(const struct mptcp_sock *msk);
 u8 mptcp_pm_get_endp_subflow_max(const struct mptcp_sock *msk);
 u8 mptcp_pm_get_endp_laminar_max(const struct mptcp_sock *msk);

-- 
2.55.0

[PATCH mptcp-net v4 2/8] mptcp: pm: userspace: lookup: match port in priority

[Matthieu Baerts (NGI0)]

In the local address list, there can be entries with the port set to 0
-- corresponding to the source port used by the initial subflow -- and
others with a specific port.

When performing a lookup, it is important to compare the ports to pick
the right entry: when a specific port is given, then try to match it
first. If no match is found, try to find entries with the port set to 0.

We need to handle the case where a subflow got created without forcing
the source port: we used the same ID as another entry, and we still need
to match it, even if here the source port is likely not the same as the
msk one (or a subflow attached to the same ID), but that's OK.

Fixes: 24430f8bf516 ("mptcp: add address into userspace pm list")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
sashiko: check the next patches before pointing at existing issues.
- v3: new (Sashiko)
- v4: avoid 2 loops (Geliang)
---
 net/mptcp/pm_userspace.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 45faee894ac3..68a02b15a4c5 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -31,13 +31,19 @@ static struct mptcp_pm_addr_entry *
 mptcp_userspace_pm_lookup_addr(struct mptcp_sock *msk,
 			       const struct mptcp_addr_info *addr)
 {
-	struct mptcp_pm_addr_entry *entry;
+	struct mptcp_pm_addr_entry *entry, *wildcard = NULL;
 
 	mptcp_for_each_userspace_pm_addr(msk, entry) {
-		if (mptcp_addresses_equal(&entry->addr, addr, false))
-			return entry;
+		if (mptcp_addresses_equal(&entry->addr, addr, false)) {
+			if (addr->port == entry->addr.port)
+				return entry;
+
+			if (!wildcard && (!addr->port || !entry->addr.port))
+				wildcard = entry;
+		}
 	}
-	return NULL;
+
+	return wildcard;
 }
 
 static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,

-- 
2.55.0

[PATCH mptcp-net v4 3/8] mptcp: pm: userspace: ID0 is not part of local_addr_used

[Matthieu Baerts (NGI0)]

The PM's local_addr_used counter doesn't take into account the ID0,
similar to what is done with the in-kernel PM.

When deleting a local address, only decrement the counter if it is not
linked to the (initial) ID0. Similarly, do not increment it when
re-adding the exact same entry.

Fixes: 77e4b94a3de6 ("mptcp: update userspace pm infos")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
sashiko: check the next patches before pointing at existing issues.
v3:
 - split from patch 1.
 - do the same check when incrementing it to avoid a "leak". (Sashiko)
 - use a new dedicated helper, clearer.
v4: move to one line when declaring is_init_id0. (Geliang)
---
 net/mptcp/pm_userspace.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 68a02b15a4c5..74969b86fb81 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -27,6 +27,11 @@ void mptcp_userspace_pm_free_local_addr_list(struct mptcp_sock *msk)
 	}
 }
 
+static bool is_init_id0(struct mptcp_pm_addr_entry *entry)
+{
+	return entry->addr.id == 0 && entry->addr.port == 0;
+}
+
 static struct mptcp_pm_addr_entry *
 mptcp_userspace_pm_lookup_addr(struct mptcp_sock *msk,
 			       const struct mptcp_addr_info *addr)
@@ -99,7 +104,9 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 
 		e->addr.id = id;
 		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
-		msk->pm.local_addr_used++;
+		/* Just to avoid this counter not to decrease when deleted */
+		if (!is_init_id0(e))
+			msk->pm.local_addr_used++;
 		ret = e->addr.id;
 	} else if (addr_match && id_match) {
 		ret = entry->addr.id;
@@ -125,12 +132,15 @@ static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
 	if (!entry)
 		return -EINVAL;
 
+	/* The initial address ID doesn't increment local_addr_used */
+	if (!is_init_id0(entry))
+		msk->pm.local_addr_used--;
+
 	/* TODO: a refcount is needed because the entry can
 	 * be used multiple times (e.g. fullmesh mode).
 	 */
 	list_del_rcu(&entry->list);
 	sock_kfree_s(sk, entry, sizeof(*entry));
-	msk->pm.local_addr_used--;
 	return 0;
 }
 

-- 
2.55.0

[PATCH mptcp-net v4 4/8] mptcp: pm: userspace: allow announcing ID0 addr

[Matthieu Baerts (NGI0)]

It is valid to announce an address with the ID0, the in-kernel PM allows
to do that when re-announcing the initial IP address after having been
deleted.

So no need to have such exception. If the ID is set to 0, but the
address doesn't match with the existing one, an error will be returned
by mptcp_userspace_pm_append_new_local_addr().

Note that once removed, the ID0 entry could be replaced by another IP
address. The RFC8684 doesn't specify this specific case with ID0, but it
says [1]: "A host wishing to replace an existing Address ID MUST first
remove the existing one". In these unclear conditions, better to let the
responsibility to the userspace daemon to pick the same IP or another
one, similar to what was in place before.

Fixes: 9ab4807c84a4 ("mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE")
Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.4.1-13 [1]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
sashiko: check the next patches before pointing at existing issues.
v2: Add note about re-using the ID0 with another address (Sashiko)
---
 net/mptcp/pm_userspace.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 74969b86fb81..66dbf67fd6d8 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -239,12 +239,6 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 	if (err < 0)
 		goto announce_err;
 
-	if (addr_val.addr.id == 0) {
-		NL_SET_ERR_MSG_ATTR(info->extack, addr, "invalid addr id");
-		err = -EINVAL;
-		goto announce_err;
-	}
-
 	if (!(addr_val.flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
 		NL_SET_ERR_MSG_ATTR(info->extack, addr, "invalid addr flags");
 		err = -EINVAL;

-- 
2.55.0

[PATCH mptcp-net v4 5/8] mptcp: pm: userspace: no ID0 exception for RM_ADDR

[Matthieu Baerts (NGI0)]

After the first patch of this series, the initial ID0 address is present
in the local addr list when the connection has been created.

Then, no need to have an exception to delete ID0, this can be done like
with other IDs.

Fixes: 84c531f54ad9 ("mptcp: userspace pm send RM_ADDR for ID 0")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
sashiko: check the next patches before pointing at existing issues.
---
 net/mptcp/pm_userspace.c | 36 ------------------------------------
 1 file changed, 36 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 66dbf67fd6d8..0f6400bc3bf8 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -270,37 +270,6 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
-static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk)
-{
-	struct mptcp_rm_list list = { .nr = 0 };
-	struct mptcp_subflow_context *subflow;
-	struct sock *sk = (struct sock *)msk;
-	bool has_id_0 = false;
-	int err = -EINVAL;
-
-	lock_sock(sk);
-	mptcp_for_each_subflow(msk, subflow) {
-		if (READ_ONCE(subflow->local_id) == 0) {
-			has_id_0 = true;
-			break;
-		}
-	}
-	if (!has_id_0)
-		goto remove_err;
-
-	list.ids[list.nr++] = 0;
-
-	spin_lock_bh(&msk->pm.lock);
-	mptcp_pm_remove_addr(msk, &list);
-	spin_unlock_bh(&msk->pm.lock);
-
-	err = 0;
-
-remove_err:
-	release_sock(sk);
-	return err;
-}
-
 static void
 mptcp_userspace_pm_remove_addr_entry(struct mptcp_sock *msk,
 				     struct mptcp_pm_addr_entry *entry)
@@ -343,11 +312,6 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 
 	sk = (struct sock *)msk;
 
-	if (id_val == 0) {
-		err = mptcp_userspace_pm_remove_id_zero_address(msk);
-		goto out;
-	}
-
 	lock_sock(sk);
 
 	spin_lock_bh(&msk->pm.lock);

-- 
2.55.0

[PATCH mptcp-net v4 6/8] mptcp: pm: userspace: don't dump initial ID0

[Matthieu Baerts (NGI0)]

After the first patch of this series, the initial ID0 address is present
in the local addr list when the connection has been created. Not to
change the previous behaviour, but also to have a similar behaviour than
what is done with the in-kernel PM, the initial ID0 address is not
dumped with the rest.

If the address is removed, then re-added later with a new subflow, it
can be dumped.

Fixes: 34e74a5cf3b7 ("mptcp: implement mptcp_userspace_pm_dump_addr")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
v3:
 - use is_init_id0() helper, to check the port and not the flags
   (prev workaround, but doesn't work if modified later)
---
 net/mptcp/pm_userspace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 0f6400bc3bf8..282d7f57c32a 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -637,7 +637,7 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 	lock_sock(sk);
 	spin_lock_bh(&msk->pm.lock);
 	mptcp_for_each_userspace_pm_addr(msk, entry) {
-		if (test_bit(entry->addr.id, bitmap->map))
+		if (is_init_id0(entry) || test_bit(entry->addr.id, bitmap->map))
 			continue;
 
 		if (mptcp_pm_genl_fill_addr(msg, cb, entry) < 0)

-- 
2.55.0

[PATCH mptcp-net v4 7/8] selftests: mptcp: join: new ID0 subflow from the right IP

[Matthieu Baerts (NGI0)]

In MPTCP, the local address and port used by the initial subflow has the
ID "0". It means that when this address and port are used for some
operations -- e.g. creating a new subflow -- they should be linked to
the ID0, and no other addresses and ports can get this special ID while
the initial IP address and port is used.

Hence, this is not correct here to try to create a subflow with the ID
"0", but from a different IP address. With the first patch of this
series, this action now returns an error.

Instead of simply using the right IP address, continue to also use the
wrong one to check that the kernel is correctly not allowing the
creation of an ID0 subflow with the wrong IP address.

While at it, fix the dumped address: the initial address is not supposed
to be dumped, similar to what is being done with the in-kernel PM. So no
"extra" addresses should be printed there.

Fixes: b2e2248f365a ("selftests: mptcp: userspace pm create id 0 subflow")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index c3ccce96b45b..190ab48acbc6 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -4214,10 +4214,11 @@ userspace_tests()
 		wait_event ns2 MPTCP_LIB_EVENT_ESTABLISHED 1
 		chk_mptcp_info subflows 0 subflows 0
 		chk_subflows_total 1 1
-		userspace_pm_add_sf $ns2 10.0.3.2 0
+		# from an IP not linked to ID0: failure expected, no new MPJ
+		userspace_pm_add_sf $ns2 10.0.3.2 0 2>/dev/null
+		userspace_pm_add_sf $ns2 10.0.1.2 0
 		wait_event ns2 MPTCP_LIB_EVENT_SUB_ESTABLISHED 1
-		userspace_pm_chk_dump_addr "${ns2}" \
-			"id 0 flags subflow 10.0.3.2" "id 0 subflow"
+		userspace_pm_chk_dump_addr "${ns2}" "" "id 0 subflow"
 		chk_join_nr 1 1 1
 		chk_mptcp_info subflows 1 subflows 1
 		chk_subflows_total 2 2

-- 
2.55.0

[PATCH mptcp-net v4 8/8] mptcp: pm: restrict in-kernel worker actions to this PM

[Matthieu Baerts (NGI0)]

No need to check them for the userspace PM.

Fixes: a49eb8ae95b8 ("mptcp: pm: worker: split in-kernel and common tasks")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
v3: use !mptcp_pm_is_userspace(), to handle WIP BPF MPTCP PM
---
 net/mptcp/pm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index e9a538296200..e38fe835abc0 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1154,7 +1154,9 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
 		pm->status &= ~BIT(MPTCP_PM_RM_ADDR_RECEIVED);
 		mptcp_pm_rm_addr_recv(msk);
 	}
-	__mptcp_pm_kernel_worker(msk);
+
+	if (!mptcp_pm_is_userspace(msk))
+		__mptcp_pm_kernel_worker(msk);
 
 	spin_unlock_bh(&msk->pm.lock);
 }

-- 
2.55.0

Re: [PATCH mptcp-net v4 1/8] mptcp: pm: userspace: properly handle the ID0 case

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: pm: userspace: ID 0 entry breaks mptcp_pm_userspace_get_local_id()
--

commit 626a00aaeaa0d778f8ba3b1e9d1ca84367bbdac3
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

mptcp: pm: userspace: properly handle the ID0 case

This commit adds the initial subflow's local address (with ID 0 and port 0)
to the userspace path manager's local address list when a connection is
created so that ID 0 can be handled uniformly with other local addresses.

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index f723a134356f3..45faee894ac3d 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -707,6 +707,26 @@ int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
>  	return ret;
>  }
>  
> +/* Add the initial local address (ID0) to the local list: easier that way */
> +void mptcp_pm_userspace_created(struct mptcp_sock *msk, const struct sock *ssk)
> +{
> +	struct mptcp_pm_addr_entry *entry;
> +
> +	entry = sock_kmalloc((struct sock *)msk, sizeof(*entry), GFP_ATOMIC);
> +	/* Fine not to handle the ID0 case in memory pressure */
> +	if (!entry)
> +		return;
> +
> +	memset(entry, 0, sizeof(*entry));
> +	mptcp_local_address((struct sock_common *)ssk, &entry->addr);
> +	entry->addr.port = 0; /* msk port */
> +
> +	spin_lock_bh(&msk->pm.lock);
> +	list_add_tail_rcu(&entry->list, &msk->pm.userspace_pm_local_addr_list);
> +	/* The initial address ID doesn't increment local_addr_used */
> +	spin_unlock_bh(&msk->pm.lock);
> +}

[Severity: High]
Does adding an entry with entry->addr.id == 0 to
msk->pm.userspace_pm_local_addr_list cause a regression in
mptcp_pm_userspace_get_local_id() when allocating a local ID for a new,
unannounced address?

When mptcp_pm_get_local_id() -> mptcp_pm_userspace_get_local_id() is called
for a subflow whose local IP address differs from the initial subflow's IP
address and is not yet in userspace_pm_local_addr_list, skc->addr.id is 0 and
it calls mptcp_userspace_pm_append_new_local_addr(msk, skc, true):

net/mptcp/pm_userspace.c:mptcp_pm_userspace_get_local_id() {
	...
	if (skc->addr.port == msk_sport)
		skc->addr.port = 0;

	return mptcp_userspace_pm_append_new_local_addr(msk, skc, true);
}

Inside mptcp_userspace_pm_append_new_local_addr(), the loop iterates over
userspace_pm_local_addr_list and encounters the initial ID 0 entry added by
mptcp_pm_userspace_created():

net/mptcp/pm_userspace.c:mptcp_userspace_pm_append_new_local_addr() {
	...
	mptcp_for_each_userspace_pm_addr(msk, e) {
		addr_match = mptcp_addresses_equal(&e->addr, &entry->addr, true);
		if (addr_match && entry->addr.id == 0 && needs_id)
			entry->addr.id = e->addr.id;
		id_match = (e->addr.id == entry->addr.id);
		if (addr_match || id_match)
			break;
		__set_bit(e->addr.id, id_bitmap);
	}

	if (!addr_match && !id_match) {
		...
	} else if (addr_match && id_match) {
		ret = entry->addr.id;
	}

append_err:
	spin_unlock_bh(&msk->pm.lock);
	return ret;
}

Because the new address does not match the initial subflow's address,
addr_match is false and entry->addr.id remains 0. However, e->addr.id is
also 0 on the initial entry, so id_match becomes true and the loop breaks
with addr_match == false and id_match == true, causing the function to
return -EINVAL instead of assigning a new ID from id_bitmap.

Although the v2 changelog notes mention handling needs_id where the addresses
do not match but the IDs (0) do, that check does not appear to be present in
this commit or later in the series.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-mptcp-pm-userspace-id0-case-v4-0-555066fa4292@kernel.org?part=1

Re: [PATCH mptcp-net v4 6/8] mptcp: pm: userspace: don't dump initial ID0

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: pm: userspace: is_init_id0() skips re-added ID 0 without port
--

commit 06caddae470be561eadf1499e52b7564e7ab99b8
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

mptcp: pm: userspace: don't dump initial ID0

This patch skips the initial ID 0 address entry in
mptcp_userspace_pm_dump_addr() by checking is_init_id0(entry) so that it is
not dumped when the connection is first created, while intending to allow it
to be dumped if removed and re-added later.

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 0f6400bc3bf8e..282d7f57c32ab 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -637,7 +637,7 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
>  	lock_sock(sk);
>  	spin_lock_bh(&msk->pm.lock);
>  	mptcp_for_each_userspace_pm_addr(msk, entry) {
> -		if (test_bit(entry->addr.id, bitmap->map))
> +		if (is_init_id0(entry) || test_bit(entry->addr.id, bitmap->map))
>  			continue;

[Severity: Medium]
Does is_init_id0(entry) also skip a re-added ID 0 address when userspace does
not specify an explicit local port?

In net/mptcp/pm_userspace.c, is_init_id0() checks:

static bool is_init_id0(struct mptcp_pm_addr_entry *entry)
{
	return entry->addr.id == 0 && entry->addr.port == 0;
}

When the initial ID 0 entry is removed via mptcp_pm_nl_remove_doit() or
mptcp_pm_nl_subflow_destroy_doit(), and later re-added with a new subflow via
mptcp_pm_nl_subflow_create_doit() (or announced via
mptcp_pm_nl_announce_doit()) without the optional MPTCP_PM_ADDR_ATTR_PORT
attribute on the local address, mptcp_pm_parse_entry() leaves
entry.addr.port == 0:

net/mptcp/pm_userspace.c:mptcp_pm_nl_subflow_create_doit() {
	...
	err = mptcp_pm_parse_entry(laddr, info, true, &entry);
	...
	entry.flags |= MPTCP_PM_ADDR_FLAG_SUBFLOW;
	...
	err = mptcp_userspace_pm_append_new_local_addr(msk, &entry, false);
	...
	local.addr = entry.addr;
	local.flags = entry.flags;
	local.ifindex = entry.ifindex;
	...
	err = __mptcp_subflow_connect(sk, &local, &addr_r);
}

Because mptcp_userspace_pm_append_new_local_addr() stores the entry before
__mptcp_subflow_connect() runs on a stack copy, the entry in
msk->pm.userspace_pm_local_addr_list retains entry->addr.id == 0 and
entry->addr.port == 0.

When mptcp_userspace_pm_dump_addr() subsequently iterates over the list,
wouldn't is_init_id0(entry) return true for this re-added ID 0 entry and skip
it during the dump, contrary to the commit description?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-mptcp-pm-userspace-id0-case-v4-0-555066fa4292@kernel.org?part=6

Re: [PATCH mptcp-net v4 0/8] mptcp: pm: userspace: properly deal with the ID0 case

[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): 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! ✅
- Perf: Unstable: 1 failed test(s): validation:sched-bbr ⚠️ 
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/40961992710

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


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

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

For more details:

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


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

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

Re: [PATCH mptcp-net v4 1/8] mptcp: pm: userspace: properly handle the ID0 case

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] A userspace-PM connection can no longer get a new local address
  ID allocated automatically.
- [Medium] The ID0 entry is shared by the initial subflow and by any
  later subflow created from the initial IP with id 0.
- [Low] mptcp_pm_userspace_created() appends the ID0 entry without
  checking for an existing entry with the same address, and without…
- [Low] Rejected after validation (baseline false).
- [Low] Rejected after validation (baseline false).

--

mptcp: pm: userspace: properly handle the ID0 case

When a connection is created, this commit adds the initial local address
(ID0, msk port) to the userspace PM local address list. ID0 can then be
handled like any other entry instead of as a special case. Later patches
in the series remove the existing ID0 exceptions.

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index f723a134356f3..45faee894ac3d 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -707,6 +707,26 @@ int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
>  	return ret;
>  }
>  
> +/* Add the initial local address (ID0) to the local list: easier that way */
> +void mptcp_pm_userspace_created(struct mptcp_sock *msk, const struct sock *ssk)
> +{
> +	struct mptcp_pm_addr_entry *entry;
> +
> +	entry = sock_kmalloc((struct sock *)msk, sizeof(*entry), GFP_ATOMIC);
> +	/* Fine not to handle the ID0 case in memory pressure */
> +	if (!entry)
> +		return;
> +
> +	memset(entry, 0, sizeof(*entry));
> +	mptcp_local_address((struct sock_common *)ssk, &entry->addr);
> +	entry->addr.port = 0; /* msk port */
> +
> +	spin_lock_bh(&msk->pm.lock);
> +	list_add_tail_rcu(&entry->list, &msk->pm.userspace_pm_local_addr_list);

[Severity: High]
Now that this entry sits first in userspace_pm_local_addr_list, can the
kernel still allocate an implicit ID for a new local address?

For any address other than the initial one, mptcp_pm_get_local_id() calls
ops->get_local_id() with skc_local.addr.id = 0. Then
mptcp_pm_userspace_get_local_id() calls
mptcp_userspace_pm_append_new_local_addr(msk, skc, true). The first entry
that loop sees is this ID0 entry:

	mptcp_for_each_userspace_pm_addr(msk, e) {
		addr_match = mptcp_addresses_equal(&e->addr, &entry->addr, true);
		...
		id_match = (e->addr.id == entry->addr.id);
		if (addr_match || id_match)
			break;
		__set_bit(e->addr.id, id_bitmap);
	}

addr_match is false, but id_match is (0 == 0), so the loop breaks. The
!addr_match && !id_match branch does not run, and neither does the
addr_match && id_match branch. The function returns -EINVAL and never
reaches find_next_zero_bit(..., 1).

Would subflow_token_join_request() then reject an incoming MP_JOIN to any
local address that is not the initial one and not already in the list,
and bump MPTCP_MIB_MPJOINNOIDFOUND?

The same seems to apply to outgoing subflows with an IPADDRANY local
address (local_id -1). They go through
subflow_chk_local_id()->mptcp_pm_get_local_id(), so they would fail in
subflow_rebuild_header() whenever routing picks a source other than the
initial address.

The v2 changelog says "Handle needs_id where the addrs don't match, but
the IDs (0) does". I couldn't find that handling in this commit, and the
append loop looks unchanged at the end of the series.

[Severity: Low]
Can a second entry for the initial address end up in the list? This
insertion doesn't look for an existing entry with the same address, and
doesn't check MPTCP_PM_DESTROYING.

The msk can be found by token before mptcp_pm_new_connection() runs:

- On the client, mptcp_connect() calls mptcp_token_new_connect() at
  connect() time. mptcp_pm_new_connection() only runs later, from
  mptcp_finish_connect() after the SYN-ACK.

- On the server, mptcp_sk_clone_init() calls mptcp_token_accept() shortly
  before subflow_syn_recv_sock() calls mptcp_pm_new_connection().

In that window, mptcp_userspace_pm_append_new_local_addr() would accept
MPTCP_PM_CMD_ANNOUNCE {initial addr, id X}, because the list is still
empty. This function would then add {initial addr, id 0} as well.

Lookups would return the first match (id X), and a later SUBFLOW_CREATE
{initial addr, id 0} would be rejected. Should the insertion check for a
matching entry under pm.lock?

> +	/* The initial address ID doesn't increment local_addr_used */

[Severity: Medium]
The initial subflow shares this entry with any later subflow created from
the initial IP with id 0. mptcp_userspace_pm_delete_local_addr() still
unlinks and frees whatever entry the lookup returns. Can the ID0 entry be
freed while the initial subflow is still using ID0?

The first case is mptcp_pm_nl_subflow_create_doit() with {initial addr,
port 0, id 0}. That matches the existing entry (addr_match && id_match),
so no new entry is created. If __mptcp_subflow_connect() then fails
synchronously (for example with -ENETUNREACH), the error path runs:

	if (err) {
		GENL_SET_ERR_MSG_FMT(info, "connect error: %d", err);

		spin_lock_bh(&msk->pm.lock);
		mptcp_userspace_pm_delete_local_addr(msk, &entry);
		spin_unlock_bh(&msk->pm.lock);
	}

This frees the pre-existing ID0 entry, which this call never allocated.

The second case is mptcp_pm_nl_subflow_destroy_doit() on a secondary
subflow from the initial IP that uses an ephemeral port. The
port-agnostic lookup returns the ID0 entry, and it gets freed.

In this commit, mptcp_userspace_pm_delete_local_addr() also decrements
local_addr_used, which was never incremented for this entry. Later in the
series, "mptcp: pm: userspace: ID0 is not part of local_addr_used" fixes
that decrement, but not the free.

Later in the series, "mptcp: pm: userspace: no ID0 exception for
RM_ADDR" and "mptcp: pm: userspace: allow announcing ID0 addr" are
applied. After that, once the entry is gone:

- RM_ADDR id 0 fails with "address with id 0 not found".
- ANNOUNCE or SUBFLOW_CREATE with {other addr, id 0} is accepted, even
  though the initial address still owns ID0.

Doesn't that bring back what the commit message says it prevents: "no
other addresses and ports can get this special ID while the initial IP
address and port is used"?

> +	spin_unlock_bh(&msk->pm.lock);
> +}

[Severity: Low]
With the ID0 entry in the list, SUBFLOW_CREATE with id 0 from a
non-initial address now returns -EINVAL, because of the id-only match in
mptcp_userspace_pm_append_new_local_addr(). The commit message describes
this as intended.

At this commit, though, the "userspace pm create id 0 subflow" test in
tools/testing/selftests/net/mptcp/mptcp_join.sh still expects it to
succeed. Would that break bisection?

Later in the series, "selftests: mptcp: join: new ID0 subflow from the
right IP" updates the test to expect the failure and to use 10.0.1.2 for
the ID0 subflow.

[Severity: Low]
At this commit, the dump and GET output also include the new ID0 entry.
That breaks the exact-match expectations in the "add & remove address"
and "create destroy subflow" selftests.

RM_ADDR id 0 also still goes through
mptcp_userspace_pm_remove_id_zero_address(), which leaves this entry in
the list.

Both are addressed later in the series:

- "mptcp: pm: userspace: don't dump initial ID0" skips is_init_id0()
  entries in mptcp_userspace_pm_dump_addr().
- "mptcp: pm: userspace: no ID0 exception for RM_ADDR" removes
  mptcp_userspace_pm_remove_id_zero_address().

Could the series be reordered so that each step stays consistent for
bisection?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-mptcp-pm-userspace-id0-case-v4-0-555066fa4292%40kernel.org

Re: [PATCH mptcp-net v4 2/8] mptcp: pm: userspace: lookup: match port in priority

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Joins on a local address with a different port are now
  rejected.

--

mptcp: pm: userspace: lookup: match port in priority

This patch changes mptcp_userspace_pm_lookup_addr() so that, when the
address matches, an entry with the exact same port is returned first.
Otherwise it falls back to an entry where one of the two ports is 0. If
both ports are non-zero and different, the lookup now returns NULL. The
old lookup matched on the address alone.

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 45faee894ac3d..68a02b15a4c53 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -31,13 +31,19 @@ static struct mptcp_pm_addr_entry *
>  mptcp_userspace_pm_lookup_addr(struct mptcp_sock *msk,
>  			       const struct mptcp_addr_info *addr)
>  {
> -	struct mptcp_pm_addr_entry *entry;
> +	struct mptcp_pm_addr_entry *entry, *wildcard = NULL;
>
>  	mptcp_for_each_userspace_pm_addr(msk, entry) {
> -		if (mptcp_addresses_equal(&entry->addr, addr, false))
> -			return entry;
> +		if (mptcp_addresses_equal(&entry->addr, addr, false)) {
> +			if (addr->port == entry->addr.port)
> +				return entry;
> +
> +			if (!wildcard && (!addr->port || !entry->addr.port))
> +				wildcard = entry;
> +		}
>  	}
> -	return NULL;
> +
> +	return wildcard;
>  }

[Severity: Medium]
With this change, can an incoming MP_JOIN to a local address with a
different port be rejected?

Take a userspace PM connection where the daemon announced B:X (id 5,
X != 0) on a non-initial address B. The peer then sends MP_JOIN to B:P,
where P is the msk port (listener on a wildcard address). Before this
patch, the lookup matched B:X on the address alone and the join got
id 5. Now both ports are non-zero and different, so the lookup returns
NULL.

On a lookup miss, mptcp_pm_userspace_get_local_id() falls back to
appending a new entry with needs_id set. The skc id is 0, because
mptcp_pm_get_local_id() sets it that way:

    if (skc->addr.port == msk_sport)
        skc->addr.port = 0;

    return mptcp_userspace_pm_append_new_local_addr(msk, skc, true);

The parent commit added an ID0 entry to every userspace PM connection
in mptcp_pm_userspace_created():

    memset(entry, 0, sizeof(*entry));
    mptcp_local_address((struct sock_common *)ssk, &entry->addr);
    entry->addr.port = 0; /* msk port */

In mptcp_userspace_pm_append_new_local_addr(), the loop reaches that
ID0 entry with addr_match false (different address). Since the
requested id is 0, id_match is true:

    id_match = (e->addr.id == entry->addr.id);
    if (addr_match || id_match)
        break;

Neither the "!addr_match && !id_match" branch nor the
"addr_match && id_match" branch applies after this, so the function
returns -EINVAL without allocating an ID.

subflow_token_join_request() then sees the negative id and drops the
join:

    local_id = mptcp_pm_get_local_id(msk, (struct sock_common *)req);
    if (local_id < 0) {
        SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINNOIDFOUND);

Any subflow whose address only appears in entries with a different
non-zero port would take the same path. The underlying problem is that
the needs_id append matches id 0 against the ID0 entry. This patch is
what turns the former lookup hit into a miss.

No later patch in the series appears to change the append loop or the
lookup. Only the local_addr_used accounting around it changes. Could
the id 0 case be handled in the append loop, or could the lookup keep
an address-only fallback for this case?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-mptcp-pm-userspace-id0-case-v4-0-555066fa4292%40kernel.org

Re: [PATCH mptcp-net v4 3/8] mptcp: pm: userspace: ID0 is not part of local_addr_used

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Automatic local-ID allocation always fails while the initial
  ID0 entry is in the list.
- [Medium] If __mptcp_subflow_connect() fails,
  mptcp_pm_nl_subflow_create_doit() calls…

Pre-existing issues:
- [Medium] mptcp_pm_nl_remove_doit() (non-zero ID path; after "mptcp: pm:
  userspace: no ID0 exception for RM_ADDR", all IDs) unlinks the entry…

--

mptcp: pm: userspace: ID0 is not part of local_addr_used

This commit stops the userspace PM's local_addr_used counter from
counting the initial ID0 entry, matching the in-kernel PM. A new
is_init_id0() helper skips the counter update both when a local address
entry is appended and when it is deleted.

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 68a02b15a4c53..74969b86fb812 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c

[ ... ]

> @@ -99,7 +104,9 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>
>  		e->addr.id = id;
>  		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
> -		msk->pm.local_addr_used++;
> +		/* Just to avoid this counter not to decrease when deleted */
> +		if (!is_init_id0(e))
> +			msk->pm.local_addr_used++;

[Severity: Medium]
This isn't a bug introduced by this patch, but is this increment ever
balanced when the address is removed with MPTCP_PM_CMD_REMOVE?

mptcp_pm_nl_remove_doit() unlinks and frees the entry and never touches
local_addr_used:

net/mptcp/pm_userspace.c:mptcp_pm_nl_remove_doit() {
    ...
	list_del_rcu(&match->list);
	spin_unlock_bh(&msk->pm.lock);

	mptcp_userspace_pm_remove_addr_entry(msk, match);

	release_sock(sk);

	kfree_rcu_mightsleep(match);
    ...
}

After an ANNOUNCE with id X and then a REMOVE with id X, would the counter
stay one too high for the rest of the connection? Once the entry is gone,
a later SUBFLOW_DESTROY can't balance it either. With "mptcp: pm:
userspace: no ID0 exception for RM_ADDR" later in the series, every ID
goes through this same path.

mptcp_diag_fill_info() in net/mptcp/sockopt.c reports the stale value to
userspace as mptcpi_local_addr_used.

>  		ret = e->addr.id;
>  	} else if (addr_match && id_match) {
>  		ret = entry->addr.id;

[Severity: Medium]
mptcp_pm_userspace_created() now adds the ID0 entry to
userspace_pm_local_addr_list for every userspace PM connection. Can the
automatic ID allocation in this function still succeed with that entry in
the list?

When the lookup misses, mptcp_pm_userspace_get_local_id() calls this
function with needs_id = true and skc->addr.id = 0. In the loop:

	mptcp_for_each_userspace_pm_addr(msk, e) {
		addr_match = mptcp_addresses_equal(&e->addr, &entry->addr, true);
		if (addr_match && entry->addr.id == 0 && needs_id)
			entry->addr.id = e->addr.id;
		id_match = (e->addr.id == entry->addr.id);
		if (addr_match || id_match)
			break;
		__set_bit(e->addr.id, id_bitmap);
	}

When e is the ID0 entry, addr_match is false but id_match is true
(0 == 0), so the loop breaks. Neither branch below runs. The function
returns -EINVAL and never reaches find_next_zero_bit().

Would subflow_token_join_request() then reject every incoming MP_JOIN to
a local address that is neither the initial address nor already in the
list, with MPTCP_MIB_MPJOINNOIDFOUND, even when non-zero IDs are free?
subflow_chk_local_id() looks like it could hit the same failure.

Before this series the list started empty, so this only happened if the
daemon had explicitly created an id 0 entry. Nothing later in the series
appears to change mptcp_userspace_pm_append_new_local_addr() or
mptcp_pm_userspace_get_local_id().

> @@ -125,12 +132,15 @@ static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
>  	if (!entry)
>  		return -EINVAL;
>
> +	/* The initial address ID doesn't increment local_addr_used */
> +	if (!is_init_id0(entry))
> +		msk->pm.local_addr_used--;
> +
>  	/* TODO: a refcount is needed because the entry can
>  	 * be used multiple times (e.g. fullmesh mode).
>  	 */
>  	list_del_rcu(&entry->list);
>  	sock_kfree_s(sk, entry, sizeof(*entry));

[Severity: Medium]
This skips the counter decrement for the ID0 entry. Can the ID0 entry
itself still be freed here while the initial subflow is alive?

mptcp_pm_nl_subflow_create_doit() calls this function on any connect
failure:

	err = __mptcp_subflow_connect(sk, &local, &addr_r);
	release_sock(sk);

	if (err) {
		GENL_SET_ERR_MSG_FMT(info, "connect error: %d", err);

		spin_lock_bh(&msk->pm.lock);
		mptcp_userspace_pm_delete_local_addr(msk, &entry);
		spin_unlock_bh(&msk->pm.lock);
	}

Take a SUBFLOW_CREATE from the initial local address with port 0 and id 0
(or no local id). mptcp_userspace_pm_append_new_local_addr() matches the
ID0 entry from mptcp_pm_userspace_created() and takes the
addr_match && id_match branch, so it allocates nothing.

If __mptcp_subflow_connect() then fails synchronously, the ID0 entry is
unlinked and freed: mptcp_userspace_pm_lookup_addr() returns it. Such
failures include -ENOTCONN from the "sent the request too early" check, a
bind error, or a connect error.

At the end of the series, mptcp_pm_nl_remove_doit() for id 0 depends on
mptcp_userspace_pm_lookup_addr_by_id(). Would RM_ADDR for ID0 then fail
with "address with id 0 not found"?

The ID0 reservation would also be lost, so another address could take
id 0. Backup flags set on the initial address would no longer be stored.

Before this series, the same error path already freed announced entries
that it had only matched. The ID0 case comes from earlier patches in this
series.

Should this path only delete the entry when the append call actually
allocated it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-mptcp-pm-userspace-id0-case-v4-0-555066fa4292%40kernel.org

Re: [PATCH mptcp-net v4 4/8] mptcp: pm: userspace: allow announcing ID0 addr

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says an ANNOUNCE with ID 0 and a non-
  matching address 'will be returned' an error by…
- [Medium] On the active side, mptcp_connect() calls
  mptcp_token_new_connect() (protocol.c), which adds the msk to the…
- [Low] At this commit, MPTCP_PM_CMD_ANNOUNCE with ID 0 for the initial
  address succeeds.

--

mptcp: pm: userspace: allow announcing ID0 addr

This commit removes the explicit rejection of address ID 0 in
mptcp_pm_nl_announce_doit(), so a userspace PM can re-announce the
initial address with ID 0. To catch an ID 0 used with a different
address, it relies on mptcp_userspace_pm_append_new_local_addr()
returning an error.

Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.4.1-13 [1]

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 74969b86fb812..66dbf67fd6d80 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -239,12 +239,6 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
>  	if (err < 0)
>  		goto announce_err;
>  
> -	if (addr_val.addr.id == 0) {
> -		NL_SET_ERR_MSG_ATTR(info->extack, addr, "invalid addr id");
> -		err = -EINVAL;
> -		goto announce_err;
> -	}
> -

[Severity: Medium]

The commit message says:

  If the ID is set to 0, but the address doesn't match with the existing
  one, an error will be returned by
  mptcp_userspace_pm_append_new_local_addr().

Is that still true when the ID0 entry is missing from
userspace_pm_local_addr_list but the initial subflow is still alive?

Here needs_id is false and the list has no ID0 entry. So
mptcp_userspace_pm_append_new_local_addr() sets neither addr_match nor
id_match, and it takes this branch:

		} else {
			id = entry->addr.id;
		}
		...
		e->addr.id = id;
		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);

That adds a different address B with ID 0. mptcp_pm_nl_announce_doit()
then calls mptcp_pm_announced_alloc() and mptcp_pm_announce_addr(). The
result is ADD_ADDR id 0 for B while the initial subflow still uses ID 0
on address A, and no RM_ADDR is sent first.

The entry can go missing in at least two ways. The first is when
mptcp_pm_userspace_created() fails to allocate it. sock_kmalloc() also
fails when optmem_max is exceeded:

	entry = sock_kmalloc((struct sock *)msk, sizeof(*entry), GFP_ATOMIC);
	/* Fine not to handle the ID0 case in memory pressure */
	if (!entry)
		return;

The second is a failed MPTCP_PM_CMD_SUBFLOW_CREATE from the initial
address A with port 0 and id 0. That request matches the ID0 entry. If
__mptcp_subflow_connect() then fails (for example -ENETUNREACH or a bind
error), the error path in mptcp_pm_nl_subflow_create_doit() runs:

	if (err) {
		...
		mptcp_userspace_pm_delete_local_addr(msk, &entry);
		...
	}

That lookup is by address, so it frees the existing ID0 entry. A
SUBFLOW_DESTROY of a secondary subflow from A can delete it the same way,
through the port-0 wildcard match in mptcp_userspace_pm_lookup_addr().

Before this patch, the addr_val.addr.id == 0 check rejected these
requests no matter what the list held. The later patches in the series
don't seem to change these paths.

[Severity: Medium]

Can this also leave two ID 0 entries in the list on the active side?

mptcp_connect()->mptcp_token_new_connect() adds the msk to the token hash
at connect() time. From that point mptcp_userspace_pm_get_sock() can find
it, and the token can be read through MPTCP_INFO or mptcp_diag. The ID0
entry is only added later, when the SYN-ACK is processed:

mptcp_finish_connect()
  mptcp_pm_new_connection()
    mptcp_pm_userspace_created()

During that window userspace_pm_local_addr_list is empty. An ANNOUNCE
with ID 0 for any address B gets through
mptcp_userspace_pm_append_new_local_addr() and stays in the list.

When the connection is established, mptcp_pm_userspace_created() appends
the real initial address A with ID 0. It does not check for an existing
ID 0 entry:

	spin_lock_bh(&msk->pm.lock);
	list_add_tail_rcu(&entry->list, &msk->pm.userspace_pm_local_addr_list);

The list then holds two different addresses with ID 0.
mptcp_userspace_pm_lookup_addr_by_id(0) would return B.

Before this patch, ANNOUNCE with id 0 was always rejected. The same
request through SUBFLOW_CREATE cleans up after itself: there,
__mptcp_subflow_connect() returns -ENOTCONN and the error path deletes
the entry. This state can still be reached at the end of the series.

[Severity: Low]

With this patch alone, an ANNOUNCE with ID 0 for the initial address goes
through mptcp_pm_announced_alloc(). That allocates an anno entry, arms
the ADD_ADDR retransmission timer and increments add_addr_signaled. A
REMOVE with id 0 still takes the shortcut in mptcp_pm_nl_remove_doit():

	if (id_val == 0) {
		err = mptcp_userspace_pm_remove_id_zero_address(msk);
		goto out;
	}

mptcp_userspace_pm_remove_id_zero_address() only calls
mptcp_pm_remove_addr(). It never calls mptcp_pm_announced_remove() or
changes add_addr_signaled, and it returns -EINVAL if no subflow has
local_id 0.

Could mptcp_pm_add_addr_timer() then resend ADD_ADDR id 0 after RM_ADDR
id 0 has been sent, with add_addr_signaled drifting as well?

The later patch "mptcp: pm: userspace: no ID0 exception for RM_ADDR"
seems to fix this. It removes the shortcut, so id 0 goes through
mptcp_userspace_pm_remove_addr_entry(), which calls
mptcp_pm_announced_remove() and adjusts add_addr_signaled. Only the state
between the two patches is affected, or a backport that takes this patch
without the next one.

Would it make sense to put the RM_ADDR patch before this one in the
series?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-mptcp-pm-userspace-id0-case-v4-0-555066fa4292%40kernel.org

Re: [PATCH mptcp-net v4 5/8] mptcp: pm: userspace: no ID0 exception for RM_ADDR

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch removes the `id_val == 0` special case in
  `mptcp_pm_nl_remove_doit()` (net/mptcp/pm_userspace.c).
- [Low] This patch carries `Fixes: 84c531f54ad9` (so stable tooling will
  select it), but it only works on top of commit "mptcp: pm: userspace:…

--

mptcp: pm: userspace: no ID0 exception for RM_ADDR

This commit removes the id 0 special case from mptcp_pm_nl_remove_doit().
That special case scanned the subflows for local_id 0 and sent RM_ADDR(0).
ID0 removal now uses the regular lookup in the userspace PM local address
list, which relies on an earlier patch in the series adding the ID0 entry
when the connection is created.

> After the first patch of this series, the initial ID0 address is present
> in the local addr list when the connection has been created.

[Severity: Low]
This patch has a Fixes: tag, so stable tooling may pick it up. It only
works on top of "mptcp: pm: userspace: properly handle the ID0 case".
Once the patch is in git history, will "the first patch of this series"
still point to that prerequisite?

Without the prerequisite, mptcp_pm_userspace_created() does not exist and
no ID0 entry is added to userspace_pm_local_addr_list. In that case
mptcp_pm_nl_remove_doit() would return -EINVAL for every REMOVE with
id 0. The patch only deletes code, so it would still apply cleanly on its
own.

Could the prerequisite be named by subject or commit id, or recorded as a
stable dependency?

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 66dbf67fd6d80..0f6400bc3bf8e 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c

[ ... ]

> @@ -343,11 +312,6 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
>  
>  	sk = (struct sock *)msk;
>  
> -	if (id_val == 0) {
> -		err = mptcp_userspace_pm_remove_id_zero_address(msk);
> -		goto out;
> -	}
> -
>  	lock_sock(sk);
>  
>  	spin_lock_bh(&msk->pm.lock);

[Severity: Medium]
Without this fallback, REMOVE with id 0 only works if
mptcp_userspace_pm_lookup_addr_by_id(msk, 0) still finds the ID0 entry.
The removed mptcp_userspace_pm_remove_id_zero_address() did not use the
list. It sent RM_ADDR(0) whenever a subflow with local_id 0 existed.

Is the ID0 entry guaranteed to be present while the initial subflow is
alive? There seem to be three ways for it to go missing.

First, mptcp_pm_userspace_created() ignores allocation failure:

net/mptcp/pm_userspace.c:mptcp_pm_userspace_created() {
	entry = sock_kmalloc((struct sock *)msk, sizeof(*entry), GFP_ATOMIC);
	/* Fine not to handle the ID0 case in memory pressure */
	if (!entry)
		return;
	...
}

mptcp_pm_new_connection() still emits MPTCP_EVENT_CREATED in that case.
Is that comment still accurate after this patch?

Second, userspace can create a second subflow from the initial IP with
id 0 and no port, as the "userspace pm create id 0 subflow" selftest
does. mptcp_userspace_pm_append_new_local_addr() reuses the ID0 entry,
and the new subflow gets local_id 0 with an ephemeral port. A
SUBFLOW_DESTROY for that subflow then goes through:

mptcp_pm_nl_subflow_destroy_doit()
  mptcp_userspace_pm_delete_local_addr()
    mptcp_userspace_pm_lookup_addr()

In the lookup, the port 0 ID0 entry matches as the wildcard:

		if (!wildcard && (!addr->port || !entry->addr.port))
			wildcard = entry;

So the ID0 entry is freed even though the initial subflow still uses it.

Third, a SUBFLOW_CREATE with the initial IP, id 0 and port 0 also reuses
the ID0 entry. If __mptcp_subflow_connect() then fails, for example with
-ENOTCONN when the connection is not fully established yet,
mptcp_pm_nl_subflow_create_doit() rolls back with:

		mptcp_userspace_pm_delete_local_addr(msk, &entry);

This deletes the ID0 entry that it reused but did not create.

In each case, wouldn't the next REMOVE with id 0 fail with -EINVAL
("address with id 0 not found")? Before this patch, the same request
sent RM_ADDR(0). None of the later patches in the series seem to add a
fallback or protect the ID0 entry.

Would it make sense to keep a subflow scan fallback for id 0 when no
entry is found? Another option is to make the ID0 entry reliable: handle
the allocation failure, and do not delete the entry through the wildcard
or rollback paths while a local_id 0 subflow exists.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-mptcp-pm-userspace-id0-case-v4-0-555066fa4292%40kernel.org

Re: [PATCH mptcp-net v4 6/8] mptcp: pm: userspace: don't dump initial ID0

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says "If the address is removed, then re-
  added later with a new subflow, it can be dumped." The code does not…
- [Medium] This patch hides the initial ID0 entry from
  MPTCP_PM_CMD_GET_ADDR dumps to avoid changing previous behaviour and…

--

mptcp: pm: userspace: don't dump initial ID0

An earlier patch in the series adds the initial ID0 local address to the
userspace PM local address list. This patch skips that entry in the
MPTCP_PM_CMD_GET_ADDR dump, so the dump output stays as it was and matches
the in-kernel PM.

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 0f6400bc3bf8e..282d7f57c32ab 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -637,7 +637,7 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
>  	lock_sock(sk);
>  	spin_lock_bh(&msk->pm.lock);
>  	mptcp_for_each_userspace_pm_addr(msk, entry) {
> -		if (test_bit(entry->addr.id, bitmap->map))
> +		if (is_init_id0(entry) || test_bit(entry->addr.id, bitmap->map))

[Severity: Medium]
The commit message says:

  If the address is removed, then re-added later with a new subflow, it
  can be dumped.

Does the code do that? is_init_id0() only checks the id and the port:

static bool is_init_id0(struct mptcp_pm_addr_entry *entry)
{
	return entry->addr.id == 0 && entry->addr.port == 0;
}

mptcp_pm_userspace_created() memsets the initial entry and sets port to 0.
Nothing else marks it, so it looks the same as an ID0 entry that userspace
adds later.

Take this sequence:

  MPTCP_PM_CMD_REMOVE, loc-id 0
    mptcp_pm_nl_remove_doit() deletes the initial entry

  MPTCP_PM_CMD_SUBFLOW_CREATE, laddr id 0, no local port
  (or MPTCP_PM_CMD_ANNOUNCE, id 0, no port)
    mptcp_pm_parse_entry() memsets the entry, port stays 0
    mptcp_userspace_pm_append_new_local_addr(msk, &entry, false)
      keeps id 0

The re-added entry has id 0 and port 0. Wouldn't every dump skip it, even
though GET by id, RM_ADDR and the local ID lookup still find it?

The other claim, "Not to change the previous behaviour", also looks wrong
for ID0 entries that userspace creates. Before this series, a
SUBFLOW_CREATE with lid 0 and no port appeared in the dump. The old
selftest checked for "id 0 flags subflow 10.0.3.2". With this patch that
entry is never dumped. It is only dumped if userspace sets a non-zero
local port.

The v3 changelog says the v2 flag-based check was replaced by this id/port
check, but the commit message wasn't updated. Could the initial entry be
marked explicitly in mptcp_pm_userspace_created()? The other option is to
reword the commit message to say that all ID0 entries with port 0 are
hidden.

A later patch in the series, "selftests: mptcp: join: new ID0 subflow from
the right IP", changes the selftest to expect an empty dump. That fixes the
selftest. It doesn't fix the mismatch between the code and the commit
message, or the uAPI change that the message doesn't mention.

>  			continue;
>  
>  		if (mptcp_pm_genl_fill_addr(msg, cb, entry) < 0)

[Severity: Medium]
Should mptcp_userspace_pm_get_addr() also filter the initial ID0? The do
path of MPTCP_PM_CMD_GET_ADDR still looks up id 0 with no is_init_id0()
check:

net/mptcp/pm_userspace.c:mptcp_userspace_pm_get_addr() {
    ...
	entry = mptcp_userspace_pm_lookup_addr_by_id(msk, id);
	if (entry) {
		*addr = *entry;
		ret = 0;
	}
    ...
}

mptcp_pm_userspace_created() now adds the initial address to the list. So
a GET_ADDR with a token and id 0 always returns that address. Before this
series, the same request failed with -EINVAL unless userspace had created
an ID0 entry.

The in-kernel PM never returns id 0 here. mptcp_pm_nl_get_addr() ->
__lookup_addr_by_id() only searches the endpoints, and endpoint IDs start
at 1.

For the same connection, the dump now hides an entry that GET returns. Is
that split intended? If it is, could the commit message say so? The
"previous behaviour" and in-kernel PM reasons are only applied to the dump.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-mptcp-pm-userspace-id0-case-v4-0-555066fa4292%40kernel.org

Re: [PATCH mptcp-net v4 7/8] selftests: mptcp: join: new ID0 subflow from the right IP

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In userspace_tests()
  (tools/testing/selftests/net/mptcp/mptcp_join.sh, test "userspace pm…

--

selftests: mptcp: join: new ID0 subflow from the right IP

This patch updates the "userspace pm create id 0 subflow" test so that the
ID0 subflow is created from the initial address 10.0.1.2. It still tries
10.0.3.2 first, which the kernel is now expected to reject. The expected
dump output becomes an empty list, because the initial address is not
dumped.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index c3ccce96b45b1..190ab48acbc61 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -4214,10 +4214,11 @@ userspace_tests()
>  		wait_event ns2 MPTCP_LIB_EVENT_ESTABLISHED 1
>  		chk_mptcp_info subflows 0 subflows 0
>  		chk_subflows_total 1 1
> -		userspace_pm_add_sf $ns2 10.0.3.2 0
> +		# from an IP not linked to ID0: failure expected, no new MPJ
> +		userspace_pm_add_sf $ns2 10.0.3.2 0 2>/dev/null
> +		userspace_pm_add_sf $ns2 10.0.1.2 0
>  		wait_event ns2 MPTCP_LIB_EVENT_SUB_ESTABLISHED 1
> -		userspace_pm_chk_dump_addr "${ns2}" \
> -			"id 0 flags subflow 10.0.3.2" "id 0 subflow"
> +		userspace_pm_chk_dump_addr "${ns2}" "" "id 0 subflow"
>  		chk_join_nr 1 1 1
>  		chk_mptcp_info subflows 1 subflows 1
>  		chk_subflows_total 2 2

[Severity: Medium]
Does anything here check that the 10.0.3.2 request actually fails?

userspace_pm_add_sf() returns the exit status of pm_nl_ctl csf. That
status is 1 when the kernel rejects the request. Here it is ignored and
stderr goes to /dev/null. The status of the 10.0.1.2 call is not checked
either.

All the checks that follow only count subflows:

    wait_event ns2 MPTCP_LIB_EVENT_SUB_ESTABLISHED 1
    chk_join_nr 1 1 1
    chk_mptcp_info subflows 1 subflows 1
    chk_subflows_total 2 2

So they cannot tell which of the two requests created the extra subflow.

The new empty dump expectation cannot tell them apart either.
mptcp_userspace_pm_dump_addr() skips every entry that matches
is_init_id0(), and that check ignores the address:

net/mptcp/pm_userspace.c:is_init_id0() {
	return entry->addr.id == 0 && entry->addr.port == 0;
}

Suppose the initial ID0 entry were missing from the userspace PM list.
That can happen if the GFP_ATOMIC sock_kmalloc() in
mptcp_pm_userspace_created() fails, or if a later change breaks that path.
In that case, mptcp_userspace_pm_append_new_local_addr() would accept the
10.0.3.2 request and create an ID0 subflow from the wrong IP. The
10.0.1.2 request would then fail with -EINVAL, because the ID matches but
the address does not.

Would every assertion above still pass in that case?

The commit message says the wrong IP is kept:

    "to check that the kernel is correctly not allowing the creation of
    an ID0 subflow with the wrong IP address"

As written, though, the test passes whether or not the kernel enforces
this. It only fails if both requests succeed.

Could the failure be asserted explicitly, for example:

	if userspace_pm_add_sf $ns2 10.0.3.2 0 2>/dev/null; then
		fail_test "ID0 subflow from wrong IP accepted"
	fi

Alternatively, could the test check that the saddr4 of the SUB_ESTABLISHED
event is 10.0.1.2?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-mptcp-pm-userspace-id0-case-v4-0-555066fa4292%40kernel.org

Re: [PATCH mptcp-net v4 8/8] mptcp: pm: restrict in-kernel worker actions to this PM

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit has a `Fixes:` tag (a49eb8ae95b8, "mptcp: pm: worker:
  split in-kernel and common tasks") but fixes no reachable bug.
- [Low] The new check in `mptcp_pm_worker()` (net/mptcp/pm.c:1158) uses
  `msk->pm.pm_type` to decide whether the kernel-only status bits get…

--

mptcp: pm: restrict in-kernel worker actions to this PM

This commit gates the call to __mptcp_pm_kernel_worker() in
mptcp_pm_worker() so it only runs when the socket is not using the
userspace PM.

> Fixes: a49eb8ae95b8 ("mptcp: pm: worker: split in-kernel and common tasks")

[Severity: Low]
Is this Fixes: tag needed? When pm_type and pm.ops both point to the
userspace PM, __mptcp_pm_kernel_worker() seems to do nothing already,
because none of the three bits it handles can be set.

MPTCP_PM_ADD_ADDR_RECEIVED is only scheduled from the in-kernel branch of
mptcp_pm_add_addr_received(). The userspace branch calls
mptcp_pm_announce_addr() and mptcp_pm_add_addr_send_ack() instead.

MPTCP_PM_ESTABLISHED and MPTCP_PM_SUBFLOW_ESTABLISHED are only scheduled
when pm->work_pending is true. mptcp_pm_data_reset() zeroes work_pending
via the reset struct_group, and only mptcp_pm_kernel_init() sets it to
true. mptcp_pm_subflow_check_next() also returns early for the userspace
PM.

Also, a49eb8ae95b8 only moved code around. The old mptcp_pm_worker() in
pm_netlink.c already tested these bits for every PM type.

The commit message says:

    No need to check them for the userspace PM.

That reads like a cleanup. Could the tag be dropped and the patch sent to
net-next instead? If there is a real failure behind it, could the commit
message describe it?

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index e9a538296200a..e38fe835abc0b 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -1154,7 +1154,9 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  		pm->status &= ~BIT(MPTCP_PM_RM_ADDR_RECEIVED);
>  		mptcp_pm_rm_addr_recv(msk);
>  	}
> -	__mptcp_pm_kernel_worker(msk);
> +
> +	if (!mptcp_pm_is_userspace(msk))
> +		__mptcp_pm_kernel_worker(msk);

[Severity: Low]
This check uses msk->pm.pm_type. The code that sets MPTCP_PM_ESTABLISHED
and MPTCP_PM_SUBFLOW_ESTABLISHED uses pm->work_pending instead, and that
flag follows msk->pm.ops. Can these two disagree?

mptcp_pm_data_reset() reads pm_type and the path_manager string
separately, without a lock:

net/mptcp/pm.c:mptcp_pm_data_reset() {
	const char *pm_name = mptcp_get_path_manager(net);
	u8 pm_type = mptcp_get_pm_type(net);
    ...
	WRITE_ONCE(pm->pm_type, pm_type);

	rcu_read_lock();
	mptcp_pm_ops_init(msk, pm_name);
    ...
}

The string is only read later, in mptcp_pm_ops_init()->mptcp_pm_find().
The sysctl handlers in net/mptcp/ctrl.c also update the two pernet fields
with separate stores. proc_pm_type() stores pm_type through
proc_dou8vec_minmax() and then calls mptcp_set_path_manager().
proc_path_manager() does the same in the opposite order.

Suppose a write of net.mptcp.pm_type=1 lands between those two reads. The
socket then ends up with pm_type == MPTCP_PM_TYPE_USERSPACE and ops ==
&mptcp_pm_kernel. mptcp_pm_kernel_init() sets work_pending without
checking pm_type, even though its comment says:

net/mptcp/pm_kernel.c:mptcp_pm_kernel_init() {
	/* pm->work_pending must be only be set to 'true' when
	 * pm->pm_type is set to MPTCP_PM_TYPE_KERNEL
	 */
	WRITE_ONCE(pm->work_pending,
    ...
}

mptcp_pm_fully_established(), mptcp_pm_subflow_established() and
mptcp_pm_add_addr_echoed() can then schedule MPTCP_PM_ESTABLISHED or
MPTCP_PM_SUBFLOW_ESTABLISHED.

Before this patch, mptcp_pm_worker() always cleared and handled those
bits. With the new check, won't they stay set for the rest of the
connection?

If so, mptcp_pm_schedule_work() would return false for them from then on,
and the kernel PM would never act on that socket. Every later
mptcp_pm_worker() call would also pass the MPTCP_PM_WORK_MASK test and
take pm.lock for nothing.

The pm_type/ops mismatch itself is older than this patch. Would checking
msk->pm.ops == &mptcp_pm_kernel here, or setting pm_type from the ops
that actually get selected, keep both sides consistent?

>  
>  	spin_unlock_bh(&msk->pm.lock);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-mptcp-pm-userspace-id0-case-v4-0-555066fa4292%40kernel.org