[PATCH mptcp-next 0/3] mptcp: pm: guard the lockless MP_JOIN path against a NULL pm.ops

[PATCH mptcp-next 0/3] mptcp: pm: guard the lockless MP_JOIN path against a NULL pm.ops

[Shardul Bankar]

The pluggable path-manager series converts mptcp_pm_get_local_id() and
mptcp_pm_is_backup() to dereference msk->pm.ops. Both are reached from
subflow_token_join_request() on an inbound MP_JOIN, in softirq and holding
only a reference to the target msk (from mptcp_token_get_sock()), without the
socket lock. A concurrent close()/disconnect() can clear pm.ops via
mptcp_pm_ops_release() in between, so the join path dereferences a NULL
pm.ops (a KASAN null-ptr-deref of ->get_priority in softirq).

Found by an MPTCP protocol-flow harness extending BRF (arXiv:2305.08782),
which drives inbound MP_JOINs against a connection under teardown.

Guard both readers with READ_ONCE() and bail out when pm.ops is NULL, paired
with WRITE_ONCE() in mptcp_pm_ops_release(). The affected commits are still in
the export tree, so this is three squash-to fixups, one per introducing commit.

Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
Shardul Bankar (3):
      Squash to "mptcp: pm: init and release mptcp_pm_ops"
      Squash to "mptcp: pm: add get_local_id() interface"
      Squash to "mptcp: pm: add get_priority() interface"

 net/mptcp/pm.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)
---
base-commit: e6ce9ffe5d02034a38e8bca7b78a7f58aff8caae
change-id: 20260711-mptcp_pm_ops_null_deref_join-a2f8e56832b4

Best regards,
--  
Shardul Bankar <shardul.b@mpiricsoftware.com>

[PATCH mptcp-next 1/3] Squash to "mptcp: pm: init and release mptcp_pm_ops"

[Shardul Bankar]

msk->pm.ops is cleared here on the teardown path, but it is read
locklessly from the softirq MP_JOIN path: mptcp_pm_get_local_id() and
mptcp_pm_is_backup() are reached from subflow_token_join_request() on an
inbound MP_JOIN, holding only a reference to the target msk (from
mptcp_token_get_sock()) and without the socket lock. Annotate the store
with WRITE_ONCE() to pair with the READ_ONCE() on those readers.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
 net/mptcp/pm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 033f783d49902..4d8f8c999b1de 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1158,7 +1158,10 @@ static void mptcp_pm_ops_release(struct mptcp_sock *msk)
 {
 	struct mptcp_pm_ops *pm_ops = msk->pm.ops;
 
-	msk->pm.ops = NULL;
+	/* Paired with READ_ONCE() in the lockless softirq MP_JOIN readers
+	 * mptcp_pm_is_backup() / mptcp_pm_get_local_id().
+	 */
+	WRITE_ONCE(msk->pm.ops, NULL);
 	if (pm_ops->release)
 		pm_ops->release(msk);
 

-- 
2.34.1

[PATCH mptcp-next 2/3] Squash to "mptcp: pm: add get_local_id() interface"

[Shardul Bankar]

mptcp_pm_get_local_id() now dereferences msk->pm.ops, but it is reached
from subflow_token_join_request() on an inbound MP_JOIN, in softirq and
holding only a reference to the target msk (from mptcp_token_get_sock()),
without the socket lock. A concurrent close()/disconnect() can clear
pm.ops via mptcp_pm_ops_release() in between.

Read pm.ops once with READ_ONCE() and return an error if it is NULL; the
join is then rejected, which is the expected outcome on a closing
connection.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
 net/mptcp/pm.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 4d8f8c999b1de..4fcd9d30b3df2 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1029,6 +1029,7 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 {
 	struct mptcp_pm_addr_entry skc_local = { 0 };
 	struct mptcp_addr_info msk_local;
+	struct mptcp_pm_ops *pm_ops;
 
 	if (WARN_ON_ONCE(!msk))
 		return -1;
@@ -1044,7 +1045,14 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 	skc_local.addr.id = 0;
 	skc_local.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
 
-	return msk->pm.ops->get_local_id(msk, &skc_local);
+	/* See mptcp_pm_is_backup(): reached locklessly from the softirq
+	 * MP_JOIN path, racing a concurrent mptcp_pm_ops_release().
+	 */
+	pm_ops = READ_ONCE(msk->pm.ops);
+	if (!pm_ops)
+		return -1;
+
+	return pm_ops->get_local_id(msk, &skc_local);
 }
 
 bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)

-- 
2.34.1

[PATCH mptcp-next 3/3] Squash to "mptcp: pm: add get_priority() interface"

[Shardul Bankar]

mptcp_pm_is_backup() now dereferences msk->pm.ops, but like
mptcp_pm_get_local_id() it is reached from subflow_token_join_request()
on an inbound MP_JOIN, in softirq and holding only a reference to the
target msk, without the socket lock. A concurrent close()/disconnect()
clearing pm.ops via mptcp_pm_ops_release() between the token lookup and
the dereference triggers a NULL pointer dereference of ->get_priority.

Read pm.ops once with READ_ONCE() and return false if it is NULL, the
safe default for the backup hint (the join is already rejected by
mptcp_pm_get_local_id() if it observed the same NULL).

Found by an MPTCP protocol-flow harness extending BRF (arXiv:2305.08782),
which drives inbound MP_JOINs against a connection under teardown.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
 net/mptcp/pm.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 4fcd9d30b3df2..12cb2f7b5af92 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1058,10 +1058,20 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
 {
 	struct mptcp_addr_info skc_local;
+	struct mptcp_pm_ops *pm_ops;
+
+	/* Paired with WRITE_ONCE() in mptcp_pm_ops_release(). An inbound
+	 * MP_JOIN reaches this via subflow_token_join_request() on a msk
+	 * obtained from mptcp_token_get_sock(), i.e. holding only a refcount
+	 * and no socket lock, so a concurrent teardown may NULL pm.ops.
+	 */
+	pm_ops = READ_ONCE(msk->pm.ops);
+	if (!pm_ops)
+		return false;
 
 	mptcp_local_address((struct sock_common *)skc, &skc_local);
 
-	return msk->pm.ops->get_priority(msk, &skc_local);
+	return pm_ops->get_priority(msk, &skc_local);
 }
 
 static void mptcp_pm_subflows_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)

-- 
2.34.1

Re: [PATCH mptcp-next 0/3] mptcp: pm: guard the lockless MP_JOIN path against a NULL pm.ops

[MPTCP CI]

Hi Shardul,

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/29147647219

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


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)