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

[PATCH mptcp-net 0/6] 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>
---
Matthieu Baerts (NGI0) (6):
      mptcp: pm: userspace: properly handle the ID0 case
      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                        | 69 +++++++++----------------
 net/mptcp/protocol.h                            |  1 +
 tools/testing/selftests/net/mptcp/mptcp_join.sh |  7 +--
 4 files changed, 37 insertions(+), 48 deletions(-)
---
base-commit: ddf9f880a057486230f16d6a4a8cab7e41c40da1
change-id: 20260724-mptcp-pm-userspace-id0-case-f074f64466c8

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

[PATCH mptcp-net 1/6] 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>
---
 net/mptcp/pm.c           |  4 ++++
 net/mptcp/pm_userspace.c | 23 ++++++++++++++++++++++-
 net/mptcp/protocol.h     |  1 +
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 05e29ce18b26..8421048f1a20 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -548,6 +548,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 73094bdbdbf2..6e6eeda91ade 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -55,7 +55,10 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 
 	spin_lock_bh(&msk->pm.lock);
 	mptcp_for_each_userspace_pm_addr(msk, e) {
-		addr_match = mptcp_addresses_equal(&e->addr, &entry->addr, true);
+		/* allow matching ID0 when no port is specified */
+		addr_match = mptcp_addresses_equal(&e->addr, &entry->addr,
+						   e->addr.id != 0 ||
+						   entry->addr.port != 0);
 		if (addr_match && entry->addr.id == 0 && needs_id)
 			entry->addr.id = e->addr.id;
 		id_match = (e->addr.id == entry->addr.id);
@@ -692,6 +695,24 @@ 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);
+
+	spin_lock_bh(&msk->pm.lock);
+	list_add_tail_rcu(&entry->list, &msk->pm.userspace_pm_local_addr_list);
+	spin_unlock_bh(&msk->pm.lock);
+}
+
 static void mptcp_pm_userspace_release(struct mptcp_sock *msk)
 {
 	mptcp_userspace_pm_free_local_addr_list(msk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index da40c6f3705f..087367cdd307 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1236,6 +1236,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.53.0

[PATCH mptcp-net 2/6] 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().

Fixes: 9ab4807c84a4 ("mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 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 6e6eeda91ade..4c96433590e3 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -212,12 +212,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.53.0

[PATCH mptcp-net 3/6] 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>
---
 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 4c96433590e3..9c7fab6a59bf 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -243,37 +243,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;
-}
-
 void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
 				struct mptcp_pm_addr_entry *entry)
 {
@@ -315,11 +284,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.53.0

[PATCH mptcp-net 4/6] 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>
---
 net/mptcp/pm_userspace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 9c7fab6a59bf..c4d5161eefa7 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -609,7 +609,9 @@ 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))
+		/* Ignore default ID0 & already sent */
+		if ((entry->addr.id == 0 && entry->flags == 0) ||
+		    test_bit(entry->addr.id, bitmap->map))
 			continue;
 
 		if (mptcp_pm_genl_fill_addr(msg, cb, entry) < 0)

-- 
2.53.0

[PATCH mptcp-net 5/6] 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 2413c832af03..0dc81eeb0636 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -4143,10 +4143,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.53.0

[PATCH mptcp-net 6/6] 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>
---
 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 8421048f1a20..9bb2da3956b9 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1138,7 +1138,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_kernel(msk))
+		__mptcp_pm_kernel_worker(msk);
 
 	spin_unlock_bh(&msk->pm.lock);
 }

-- 
2.53.0

Re: [PATCH mptcp-net 0/6] 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! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/30293699277

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


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 0/6] mptcp: pm: userspace: properly deal with the ID0 case

[Mat Martineau]

On Mon, 27 Jul 2026, Matthieu Baerts (NGI0) wrote:

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

The approach looks ok to me. Have you tested with mptcpd?

- Mat


> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Matthieu Baerts (NGI0) (6):
>      mptcp: pm: userspace: properly handle the ID0 case
>      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                        | 69 +++++++++----------------
> net/mptcp/protocol.h                            |  1 +
> tools/testing/selftests/net/mptcp/mptcp_join.sh |  7 +--
> 4 files changed, 37 insertions(+), 48 deletions(-)
> ---
> base-commit: ddf9f880a057486230f16d6a4a8cab7e41c40da1
> change-id: 20260724-mptcp-pm-userspace-id0-case-f074f64466c8
>
> Best regards,
> --
> Matthieu Baerts (NGI0) <matttbe@kernel.org>
>
>
>

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

[Matthieu Baerts]

On 01/08/2026 02:06, Mat Martineau wrote:
> On Mon, 27 Jul 2026, Matthieu Baerts (NGI0) wrote:
> 
>> 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.
>>
> 
> The approach looks ok to me. Have you tested with mptcpd?
No, but the "make check" is not validating that. What I'm changing here
is only about the ID0 case, which is quite specific, and cover by the
kernel selftests. I don't think I'm breaking anything here, but only a
full test suite would be able to check that, which we don't have with
mptcpd. I know that Gang is looking at adding some tests, but I guess
the first tests will not validate the IDs being used are the expected ones.

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

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

[Matthieu Baerts]

Replying to Sashiko's review:
>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>> index 73094bdbdbf27..6e6eeda91ade0 100644
>> --- a/net/mptcp/pm_userspace.c
>> +++ b/net/mptcp/pm_userspace.c
>> @@ -55,7 +55,10 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>>  
>>  	spin_lock_bh(&msk->pm.lock);
>>  	mptcp_for_each_userspace_pm_addr(msk, e) {
>> -		addr_match = mptcp_addresses_equal(&e->addr, &entry->addr, true);
>> +		/* allow matching ID0 when no port is specified */
>> +		addr_match = mptcp_addresses_equal(&e->addr, &entry->addr,
>> +						   e->addr.id != 0 ||
>> +						   entry->addr.port != 0);
>>  		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;
> 
> With ID 0 now always present in userspace_pm_local_addr_list, does this
> id_match check prematurely break the loop for new unassigned addresses?
> 
> If an unconfigured address is evaluated (e.g. from an incoming MP_JOIN
> request), entry->addr.id defaults to 0. Since the pre-added initial address
> (ID 0) is now in the list, e->addr.id == entry->addr.id (0 == 0) will be
> true, falsely detecting an ID collision and preventing the new address from
> being inserted.

I guess it meant "if the IP address is not the same". If it is, now the
right ID0 will be returned instead of a new one. If the address is
different, then yes, id_match should always be false when needs_id is
set. I will adapt that in a v2. (The bug was also present before when
the ID0 was in the list.)

>> @@ -692,6 +695,24 @@ 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);
>> +
>> +	spin_lock_bh(&msk->pm.lock);
>> +	list_add_tail_rcu(&entry->list, &msk->pm.userspace_pm_local_addr_list);
> 
> Does this code forget to increment msk->pm.local_addr_used?

No, on purpose.

> When this address is added here, the counter is not incremented. Later, if
> userspace creates a subflow using ID 0 and destroys it, or if subflow creation
> fails, mptcp_userspace_pm_delete_local_addr() will unconditionally decrement
> msk->pm.local_addr_used. This could cause the counter to underflow to 255
> and permanently remove the ID 0 address from the list.

Indeed, an exception is needed there. Will add that in v2.

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

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

Re: [PATCH mptcp-net 2/6] mptcp: pm: userspace: allow announcing ID0 addr

[Matthieu Baerts]

Replying to Sashiko's review:

>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>> index 6e6eeda91ade0..4c96433590e36 100644
>> --- a/net/mptcp/pm_userspace.c
>> +++ b/net/mptcp/pm_userspace.c
>> @@ -212,12 +212,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 userspace deletes the initial subflow via MPTCP_PM_CMD_SUBFLOW_DESTROY,
> which removes the ID 0 entry from the local address list, can userspace then
> announce an arbitrary IP address with ID 0?
> 
> Since the original ID 0 entry would be missing, it seems
> mptcp_userspace_pm_append_new_local_addr() wouldn't find an address or ID
> match:
> 
> net/mptcp/pm_userspace.c:mptcp_userspace_pm_append_new_local_addr() {
>     ...
> 	if (!addr_match && !id_match) {
> 		...
> 		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
> 		msk->pm.local_addr_used++;
> 		ret = e->addr.id;
> 	}
>     ...
> }
> 
> Would this incorrectly append a new entry with ID 0 and an arbitrary IP
> address, bypassing the validation mentioned in the commit message?

Yes, but the RFC8684 doesn't specify if the ID0 can be replaced by
another address, once removed. But it says: "A host wishing to replace
an existing Address ID MUST first remove the existing one". In this
condition, better to let the responsibility to the userspace daemon.

I can add a note in the commit message.

> Does allowing ID 0 announcements here also create a state inconsistency when
> trying to remove them?
> 
> Looking at mptcp_pm_nl_remove_doit(), there is a hardcoded fast path for ID 0:
> 
> net/mptcp/pm_userspace.c:mptcp_pm_nl_remove_doit() {
>     ...
> 	if (id_val == 0) {
> 		err = mptcp_userspace_pm_remove_id_zero_address(msk);
> 		goto out;
> 	}
>     ...
> }
> 
> Because mptcp_userspace_pm_remove_id_zero_address() doesn't call
> mptcp_pm_announced_remove() and ignores msk->pm.anno_list, would this
> leave the announcement un-cancellable and leak the state in anno_list?
Removed in the next patch.

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

Re: [PATCH mptcp-net 3/6] mptcp: pm: userspace: no ID0 exception for RM_ADDR

[Matthieu Baerts]

Replying to Sashiko's review:

>> After the first patch of this series, the initial ID0 address is present
>> in the local addr list when the connection has been created.
> 
> Does adding the initial ID 0 address to the local address list cause
> secondary subflow creation to fail for incoming joins on unannounced
> addresses?
> 
> When a secondary subflow is initiated using a new address with ID 0,
> mptcp_userspace_pm_append_new_local_addr() loops over the list.
> 
> The new entry will match the ID 0 of the initial address (id_match = true)
> but its IP differs (addr_match = false).
> 
> This causes the loop to break, bypassing allocation and ID assignment,
> and returns -EINVAL.

Same comment as on patch 1/6.

> Does destroying the initial subflow (ID 0) via userspace PM cause an
> underflow of msk->pm.local_addr_used?
> 
> If the initial ID 0 connection address is added without incrementing
> the local_addr_used counter, later calling MPTCP_PM_CMD_SUBFLOW_DESTROY
> on ID 0 will unconditionally decrement it in
> mptcp_userspace_pm_delete_local_addr().
> 
> Could this wrap the 8-bit counter to 255 and corrupt the statistics
> exported to userspace?

Same comment as on patch 1/6.
>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>> index 4c96433590e36..9c7fab6a59bf4 100644
>> --- a/net/mptcp/pm_userspace.c
>> +++ b/net/mptcp/pm_userspace.c
> 
> [ ... ]
> 
>> @@ -315,11 +284,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);
> 
> This is a pre-existing issue, but does decrementing sk_omem_alloc eagerly
> while freeing memory via RCU allow a local user in a user namespace to
> bypass socket memory limits?
> 
> Later in mptcp_pm_nl_remove_doit():
> 
> 	kfree_rcu_mightsleep(match);
> 	/* Adjust sk_omem_alloc like sock_kfree_s() does, to match
> 	 * with allocation of this memory by sock_kmemdup()
> 	 */
> 	atomic_sub(sizeof(*match), &sk->sk_omem_alloc);
> 
> If a user with CAP_NET_ADMIN repeatedly announces and removes addresses in
> a tight loop via MPTCP_PM_CMD_ANNOUNCE and MPTCP_PM_CMD_REMOVE, the
> sk_omem_alloc counter is decremented immediately, but the actual freeing
> is deferred to RCU grace periods.
> 
> Could this potentially lead to memory exhaustion (OOM) on the host?

Mmh, I don't think we need to handle this case with CAP_NET_ADMIN, plus
the netlink communication will not allow too many loops I suppose.

(+ pre-existing issue: for a follow-up patch anyway)

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