[PATCH mptcp-next v3 0/5] mptcp: avoid data-races around the sysctls

[PATCH mptcp-next v3 0/5] mptcp: avoid data-races around the sysctls

[Gang Yan]

From: Gang Yan <yangang@kylinos.cn>

Changelog:
v3:
  Patch 1:
    - Carries the full module refcount handling that was previously
      deferred to a squash-to patch.
    - mptcp_get_scheduler now returns the ops pointer directly instead
      of copying the name, so mptcp_init_sock() no longer goes through a
      redundant mptcp_sched_find() list walk.
    - A new pernet-level helper 'mptcp_pernet_sched' is shared with
      proc_scheduler().
    - mptcp_net_init releases the default reference on the
      mptcp_pernet_new_table error path.
    - Simplify the commit message.

  Patch 2:
    - Introduce the pernet-level helper mptcp_pernet_pm, like sched
      does.
    - Commit message shows why no moudule reference is taken here for
      addressing sashiko's comments.

  Patch 3:
    - Remove the WRITE_ONCE() part and the tag of Tao Cui.

  Patch 4:
    - It is new in this seires. Followed by Matt's suggestions, add
      comments before pm_type was changes.

  Patch 5:
    - Release the PM reference on the mptcp_net_init errpr path as sched
      does.
    - mptcp_pm_ops_init() now takes the ops pointer directly instead of
      the name, and mptcp_get_path_manager() returns the ops pointer:
      the redundant mptcp_pm_find() list walk from the name is avoided,
      mirroring the scheduler side earlier in this series. 

v2:
  - Split the code around bpf to patch 4 and patch 5.
  - Put WRITE_ONCE(pernet->pm_type, pm_type) into patch 3 and add
    tag of Tao Cui.
  Link: https://patchwork.kernel.org/project/mptcp/cover/20260818094825.48446-1-gang.yan@linux.dev/
v1:
  Link: https://patchwork.kernel.org/project/mptcp/cover/20260817012452.7519-1-gang.yan@linux.dev/

Gang Yan (4):
  mptcp: sched: change scheduler sysctl atomically
  mptcp: pm: change path_manager sysctl atomically
  mptcp: pm: use WRITE_ONCE() for the pm_type sysctl
  Squash to "mptcp: pm: init and release mptcp_pm_ops"

Matthieu Baerts (NGI0) (1):
  mptcp: use READ_ONCE() over sysctls

 net/mptcp/ctrl.c     | 141 ++++++++++++++++++++++++++++++++-----------
 net/mptcp/pm.c       |  11 ++--
 net/mptcp/protocol.c |   3 +-
 net/mptcp/protocol.h |   6 +-
 net/mptcp/sched.c    |   2 +-
 5 files changed, 115 insertions(+), 48 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v3 1/5] mptcp: sched: change scheduler sysctl atomically

[Gang Yan]

From: Gang Yan <yangang@kylinos.cn>

The per-netns scheduler name is stored as an inline char[] buffer and
updated via strscpy() from the sysctl handler. A concurrent reader (e.g.
mptcp_init_sock() resolving the default scheduler) can observe a
half-written name, which is also flagged by KCSAN. READ_ONCE() does not
help here as it cannot read a multi-byte string atomically.

Following the tcp_congestion_control() model, store a pointer to the
immutable struct mptcp_sched_ops instead of the name string. A pointer
store is a single atomic word, so readers always observe a consistent
value, and mptcp_get_scheduler() can now return the ops directly
instead of going through mptcp_sched_find() again.

Assisted-by: Claude:GLM5.2
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/626
Co-developed-by: Tao Cui <cuitao@kylinos.cn>
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
Signed-off-by: Gang Yan <yangang@kylinos.cn>
---
 net/mptcp/ctrl.c     | 63 +++++++++++++++++++++++++++++++++++---------
 net/mptcp/protocol.c |  3 +--
 net/mptcp/protocol.h |  3 ++-
 net/mptcp/sched.c    |  2 +-
 4 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index 63c5747f0f63..7d0f3421bd04 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -39,7 +39,7 @@ struct mptcp_pernet {
 	u8 allow_join_initial_addr_port;
 	u8 pm_type;
 	u8 add_addr_v6_port_drop_ts;
-	char scheduler[MPTCP_SCHED_NAME_MAX];
+	struct mptcp_sched_ops __rcu *scheduler;
 	char path_manager[MPTCP_PM_NAME_MAX];
 };
 
@@ -90,9 +90,17 @@ const char *mptcp_get_path_manager(const struct net *net)
 	return mptcp_get_pernet(net)->path_manager;
 }
 
-const char *mptcp_get_scheduler(const struct net *net)
+static struct mptcp_sched_ops *mptcp_pernet_sched(struct mptcp_pernet *pernet)
 {
-	return mptcp_get_pernet(net)->scheduler;
+	struct mptcp_sched_ops *sched;
+
+	sched = rcu_dereference(pernet->scheduler);
+	return sched ? sched : &mptcp_sched_default;
+}
+
+struct mptcp_sched_ops *mptcp_get_scheduler(const struct net *net)
+{
+	return mptcp_pernet_sched(mptcp_get_pernet(net));
 }
 
 unsigned int mptcp_add_addr_v6_port_drop_ts(const struct net *net)
@@ -112,23 +120,33 @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
 	pernet->allow_join_initial_addr_port = 1;
 	pernet->stale_loss_cnt = 4;
 	pernet->pm_type = MPTCP_PM_TYPE_KERNEL;
-	strscpy(pernet->scheduler, "default", sizeof(pernet->scheduler));
+
+	if (bpf_try_module_get(&mptcp_sched_default, mptcp_sched_default.owner))
+		RCU_INIT_POINTER(pernet->scheduler, &mptcp_sched_default);
+
 	strscpy(pernet->path_manager, "kernel", sizeof(pernet->path_manager));
 	pernet->add_addr_v6_port_drop_ts = 1;
 }
 
 #ifdef CONFIG_SYSCTL
-static int mptcp_set_scheduler(char *scheduler, const char *name)
+static int mptcp_set_scheduler(struct mptcp_pernet *pernet, const char *name)
 {
-	struct mptcp_sched_ops *sched;
+	struct mptcp_sched_ops *sched, *prev;
 	int ret = 0;
 
 	rcu_read_lock();
 	sched = mptcp_sched_find(name);
-	if (sched)
-		strscpy(scheduler, name, MPTCP_SCHED_NAME_MAX);
-	else
+	if (sched) {
+		if (bpf_try_module_get(sched, sched->owner)) {
+			prev = xchg(&pernet->scheduler, sched);
+			if (prev)
+				bpf_module_put(prev, prev->owner);
+		} else {
+			ret = -EBUSY;
+		}
+	} else {
 		ret = -ENOENT;
+	}
 	rcu_read_unlock();
 
 	return ret;
@@ -137,7 +155,9 @@ static int mptcp_set_scheduler(char *scheduler, const char *name)
 static int proc_scheduler(const struct ctl_table *ctl, int write,
 			  void *buffer, size_t *lenp, loff_t *ppos)
 {
-	char (*scheduler)[MPTCP_SCHED_NAME_MAX] = ctl->data;
+	struct mptcp_pernet *pernet = container_of(ctl->data,
+						   struct mptcp_pernet,
+						   scheduler);
 	char val[MPTCP_SCHED_NAME_MAX];
 	struct ctl_table tbl = {
 		.data = val,
@@ -145,11 +165,13 @@ static int proc_scheduler(const struct ctl_table *ctl, int write,
 	};
 	int ret;
 
-	strscpy(val, *scheduler, MPTCP_SCHED_NAME_MAX);
+	rcu_read_lock();
+	strscpy(val, mptcp_pernet_sched(pernet)->name, MPTCP_SCHED_NAME_MAX);
+	rcu_read_unlock();
 
 	ret = proc_dostring(&tbl, write, buffer, lenp, ppos);
 	if (write && ret == 0)
-		ret = mptcp_set_scheduler(*scheduler, val);
+		ret = mptcp_set_scheduler(pernet, val);
 
 	return ret;
 }
@@ -563,18 +585,33 @@ void mptcp_active_detect_blackhole(struct sock *ssk, bool expired)
 static int __net_init mptcp_net_init(struct net *net)
 {
 	struct mptcp_pernet *pernet = mptcp_get_pernet(net);
+	int ret;
 
 	mptcp_pernet_set_defaults(pernet);
 
-	return mptcp_pernet_new_table(net, pernet);
+	ret = mptcp_pernet_new_table(net, pernet);
+	if (ret) {
+		struct mptcp_sched_ops *sched;
+
+		sched = rcu_dereference_protected(pernet->scheduler, true);
+		if (sched)
+			bpf_module_put(sched, sched->owner);
+	}
+
+	return ret;
 }
 
 /* Note: the callback will only be called per extra netns */
 static void __net_exit mptcp_net_exit(struct net *net)
 {
 	struct mptcp_pernet *pernet = mptcp_get_pernet(net);
+	struct mptcp_sched_ops *sched;
 
 	mptcp_pernet_del_table(pernet);
+
+	sched = rcu_dereference_protected(pernet->scheduler, true);
+	if (sched)
+		bpf_module_put(sched, sched->owner);
 }
 
 static struct pernet_operations mptcp_pernet_ops = {
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f879b1061f2d..1cbc6b1448bc 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3271,8 +3271,7 @@ static int mptcp_init_sock(struct sock *sk)
 		return -ENOMEM;
 
 	rcu_read_lock();
-	ret = mptcp_init_sched(mptcp_sk(sk),
-			       mptcp_sched_find(mptcp_get_scheduler(net)));
+	ret = mptcp_init_sched(mptcp_sk(sk), mptcp_get_scheduler(net));
 	rcu_read_unlock();
 	if (ret)
 		return ret;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7e168e450fb0..6447945be93c 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -803,7 +803,7 @@ unsigned int mptcp_stale_loss_cnt(const struct net *net);
 unsigned int mptcp_close_timeout(const struct sock *sk);
 int mptcp_get_pm_type(const struct net *net);
 const char *mptcp_get_path_manager(const struct net *net);
-const char *mptcp_get_scheduler(const struct net *net);
+struct mptcp_sched_ops *mptcp_get_scheduler(const struct net *net);
 unsigned int mptcp_add_addr_v6_port_drop_ts(const struct net *net);
 
 void mptcp_active_disable(struct sock *sk);
@@ -1155,6 +1155,7 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
 
 /* the default path manager, used in mptcp_pm_unregister */
 extern struct mptcp_pm_ops mptcp_pm_kernel;
+extern struct mptcp_sched_ops mptcp_sched_default;
 
 struct mptcp_pm_ops *mptcp_pm_find(const char *name);
 int mptcp_pm_register(struct mptcp_pm_ops *pm_ops);
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index 1e59072d478c..0d13ee46ffdf 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -40,7 +40,7 @@ static int mptcp_sched_default_get_retrans(struct mptcp_sock *msk)
 	return 0;
 }
 
-static struct mptcp_sched_ops mptcp_sched_default = {
+struct mptcp_sched_ops mptcp_sched_default = {
 	.get_send	= mptcp_sched_default_get_send,
 	.get_retrans	= mptcp_sched_default_get_retrans,
 	.name		= "default",
-- 
2.43.0

[PATCH mptcp-next v3 2/5] mptcp: pm: change path_manager sysctl atomically

[Gang Yan]

From: Gang Yan <yangang@kylinos.cn>

The per-netns path manager name is stored as an inline char[] buffer and
updated via strscpy() from the sysctl handler. A concurrent reader can
observe a half-written name (KCSAN), which READ_ONCE() cannot fix for a
multi-byte string.

Following the tcp_congestion_control() model (and the scheduler change
in the previous patch), store a pointer to the immutable
struct mptcp_pm_ops instead of the name string.

No module reference is taken on the path manager ops for now, as they
can only be registered from built-in code on one side, and on the other
side the reference counting will be introduced by the last patch of
this series, together with the BPF path manager support.

Assisted-by: Claude:GLM5.2
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/626
Co-developed-by: Tao Cui <cuitao@kylinos.cn>
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
Signed-off-by: Gang Yan <yangang@kylinos.cn>
---
 net/mptcp/ctrl.c     | 33 +++++++++++++++++++++++----------
 net/mptcp/pm.c       |  3 ++-
 net/mptcp/protocol.h |  3 +--
 3 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index 7d0f3421bd04..76ff2a41ba38 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -40,7 +40,7 @@ struct mptcp_pernet {
 	u8 pm_type;
 	u8 add_addr_v6_port_drop_ts;
 	struct mptcp_sched_ops __rcu *scheduler;
-	char path_manager[MPTCP_PM_NAME_MAX];
+	struct mptcp_pm_ops __rcu *path_manager;
 };
 
 static struct mptcp_pernet *mptcp_get_pernet(const struct net *net)
@@ -85,9 +85,20 @@ int mptcp_get_pm_type(const struct net *net)
 	return mptcp_get_pernet(net)->pm_type;
 }
 
-const char *mptcp_get_path_manager(const struct net *net)
+static struct mptcp_pm_ops *mptcp_pernet_pm(struct mptcp_pernet *pernet)
 {
-	return mptcp_get_pernet(net)->path_manager;
+	struct mptcp_pm_ops *pm_ops;
+
+	pm_ops = rcu_dereference(pernet->path_manager);
+	return pm_ops ? pm_ops : &mptcp_pm_kernel;
+}
+
+void mptcp_get_path_manager(const struct net *net, char *name)
+{
+	rcu_read_lock();
+	strscpy(name, mptcp_pernet_pm(mptcp_get_pernet(net))->name,
+		MPTCP_PM_NAME_MAX);
+	rcu_read_unlock();
 }
 
 static struct mptcp_sched_ops *mptcp_pernet_sched(struct mptcp_pernet *pernet)
@@ -124,7 +135,8 @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
 	if (bpf_try_module_get(&mptcp_sched_default, mptcp_sched_default.owner))
 		RCU_INIT_POINTER(pernet->scheduler, &mptcp_sched_default);
 
-	strscpy(pernet->path_manager, "kernel", sizeof(pernet->path_manager));
+	RCU_INIT_POINTER(pernet->path_manager, &mptcp_pm_kernel);
+
 	pernet->add_addr_v6_port_drop_ts = 1;
 }
 
@@ -210,7 +222,7 @@ static int proc_blackhole_detect_timeout(const struct ctl_table *table,
 	return ret;
 }
 
-static int mptcp_set_path_manager(char *path_manager, const char *name)
+static int mptcp_set_path_manager(struct mptcp_pernet *pernet, const char *name)
 {
 	struct mptcp_pm_ops *pm_ops;
 	int ret = 0;
@@ -218,7 +230,7 @@ static int mptcp_set_path_manager(char *path_manager, const char *name)
 	rcu_read_lock();
 	pm_ops = mptcp_pm_find(name);
 	if (pm_ops)
-		strscpy(path_manager, name, MPTCP_PM_NAME_MAX);
+		xchg(&pernet->path_manager, pm_ops);
 	else
 		ret = -ENOENT;
 	rcu_read_unlock();
@@ -232,7 +244,6 @@ static int proc_path_manager(const struct ctl_table *ctl, int write,
 	struct mptcp_pernet *pernet = container_of(ctl->data,
 						   struct mptcp_pernet,
 						   path_manager);
-	char (*path_manager)[MPTCP_PM_NAME_MAX] = ctl->data;
 	char pm_name[MPTCP_PM_NAME_MAX];
 	const struct ctl_table tbl = {
 		.data = pm_name,
@@ -240,11 +251,13 @@ static int proc_path_manager(const struct ctl_table *ctl, int write,
 	};
 	int ret;
 
-	strscpy(pm_name, *path_manager, MPTCP_PM_NAME_MAX);
+	rcu_read_lock();
+	strscpy(pm_name, mptcp_pernet_pm(pernet)->name, MPTCP_PM_NAME_MAX);
+	rcu_read_unlock();
 
 	ret = proc_dostring(&tbl, write, buffer, lenp, ppos);
 	if (write && ret == 0) {
-		ret = mptcp_set_path_manager(*path_manager, pm_name);
+		ret = mptcp_set_path_manager(pernet, pm_name);
 		if (ret == 0) {
 			u8 pm_type = __MPTCP_PM_TYPE_NR;
 
@@ -276,7 +289,7 @@ static int proc_pm_type(const struct ctl_table *ctl, int write,
 			pm_name = "kernel";
 		else if (pm_type == MPTCP_PM_TYPE_USERSPACE)
 			pm_name = "userspace";
-		mptcp_set_path_manager(pernet->path_manager, pm_name);
+		mptcp_set_path_manager(pernet, pm_name);
 	}
 
 	return ret;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index ba7c6f80a183..09f99bcd827c 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1204,7 +1204,7 @@ void mptcp_pm_destroy(struct mptcp_sock *msk)
 void mptcp_pm_data_reset(struct mptcp_sock *msk)
 {
 	const struct net *net = sock_net((struct sock *)msk);
-	const char *pm_name = mptcp_get_path_manager(net);
+	char pm_name[MPTCP_PM_NAME_MAX];
 	u8 pm_type = mptcp_get_pm_type(net);
 	struct mptcp_pm_data *pm = &msk->pm;
 
@@ -1213,6 +1213,7 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
 	pm->rm_list_rx.nr = 0;
 	WRITE_ONCE(pm->pm_type, pm_type);
 
+	mptcp_get_path_manager(net, pm_name);
 	rcu_read_lock();
 	mptcp_pm_ops_init(msk, pm_name);
 	rcu_read_unlock();
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 6447945be93c..bbcf0ecd72d1 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -802,7 +802,7 @@ int mptcp_allow_join_id0(const struct net *net);
 unsigned int mptcp_stale_loss_cnt(const struct net *net);
 unsigned int mptcp_close_timeout(const struct sock *sk);
 int mptcp_get_pm_type(const struct net *net);
-const char *mptcp_get_path_manager(const struct net *net);
+void mptcp_get_path_manager(const struct net *net, char *name);
 struct mptcp_sched_ops *mptcp_get_scheduler(const struct net *net);
 unsigned int mptcp_add_addr_v6_port_drop_ts(const struct net *net);
 
@@ -1153,7 +1153,6 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 			   bool echo);
 int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
 
-/* the default path manager, used in mptcp_pm_unregister */
 extern struct mptcp_pm_ops mptcp_pm_kernel;
 extern struct mptcp_sched_ops mptcp_sched_default;
 
-- 
2.43.0

[PATCH mptcp-next v3 3/5] mptcp: use READ_ONCE() over sysctls

[Gang Yan]

From: "Matthieu Baerts (NGI0)" <matttbe@kernel.org>

To avoid KCSAN issues.

This patch is in theory for -net, and will need to be split in multiple
patches, with different Fixes tags. But I prefer to wait for Eric's
patches, as I noticed he already started to modify mptcp_is_enabled:

  https://lore.kernel.org/CANn89iLdwhhwLyO6zRjWMEY3t9g60ZE8ZhOVx33ucg_uRETbmQ@mail.gmail.com

Still, keeping this patch in this series, not to forget about it.

Reported-by: Eric Dumazet <edumazet@google.com>
Closes: https://lore.kernel.org/CANn89iL=os-60kDKqMDdyiXuPF5CG=eejS0vmthwpDGXz_Bp8A@mail.gmail.com
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/ctrl.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index 76ff2a41ba38..5a75f9b76d15 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -50,39 +50,39 @@ static struct mptcp_pernet *mptcp_get_pernet(const struct net *net)
 
 int mptcp_is_enabled(const struct net *net)
 {
-	return mptcp_get_pernet(net)->mptcp_enabled;
+	return READ_ONCE(mptcp_get_pernet(net)->mptcp_enabled);
 }
 
 unsigned int mptcp_get_add_addr_timeout(const struct net *net)
 {
-	return mptcp_get_pernet(net)->add_addr_timeout;
+	return READ_ONCE(mptcp_get_pernet(net)->add_addr_timeout);
 }
 
 int mptcp_is_checksum_enabled(const struct net *net)
 {
-	return mptcp_get_pernet(net)->checksum_enabled;
+	return READ_ONCE(mptcp_get_pernet(net)->checksum_enabled);
 }
 
 int mptcp_allow_join_id0(const struct net *net)
 {
-	return mptcp_get_pernet(net)->allow_join_initial_addr_port;
+	return READ_ONCE(mptcp_get_pernet(net)->allow_join_initial_addr_port);
 }
 
 unsigned int mptcp_stale_loss_cnt(const struct net *net)
 {
-	return mptcp_get_pernet(net)->stale_loss_cnt;
+	return READ_ONCE(mptcp_get_pernet(net)->stale_loss_cnt);
 }
 
 unsigned int mptcp_close_timeout(const struct sock *sk)
 {
 	if (sock_flag(sk, SOCK_DEAD))
 		return TCP_TIMEWAIT_LEN;
-	return mptcp_get_pernet(sock_net(sk))->close_timeout;
+	return READ_ONCE(mptcp_get_pernet(sock_net(sk))->close_timeout);
 }
 
 int mptcp_get_pm_type(const struct net *net)
 {
-	return mptcp_get_pernet(net)->pm_type;
+	return READ_ONCE(mptcp_get_pernet(net)->pm_type);
 }
 
 static struct mptcp_pm_ops *mptcp_pernet_pm(struct mptcp_pernet *pernet)
@@ -586,7 +586,7 @@ void mptcp_active_detect_blackhole(struct sock *ssk, bool expired)
 
 	net = sock_net(ssk);
 	timeouts = inet_csk(ssk)->icsk_retransmits;
-	to_max = mptcp_get_pernet(net)->syn_retrans_before_tcp_fallback;
+	to_max = READ_ONCE(mptcp_get_pernet(net)->syn_retrans_before_tcp_fallback);
 
 	if (timeouts == to_max || (timeouts < to_max && expired)) {
 		subflow->mpc_drop = 1;
-- 
2.43.0

[PATCH mptcp-next v3 4/5] mptcp: pm: use WRITE_ONCE() for the pm_type sysctl

[Gang Yan]

From: Gang Yan <yangang@kylinos.cn>

Write pernet->pm_type with WRITE_ONCE() in proc_path_manager(), pairing
it with the READ_ONCE() readers introduced earlier in this series.

Note that updating net.mptcp.path_manager swaps the ops pointer first,
then writes the derived pm_type: a socket created in between may see
the new ops with the old pm_type. As the net.mptcp.pm_type knob is
deprecated since v6.15 and will be removed, the race is not fixed on
purpose; document it above the assignment so it does not get reported
again.

Suggested-by: Matthieu Baerts <matttbe@kernel.org>
Assisted-by: Claude:GLM5.2
Co-developed-by: Tao Cui <cuitao@kylinos.cn>
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
Signed-off-by: Gang Yan <yangang@kylinos.cn>
---
 net/mptcp/ctrl.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index 5a75f9b76d15..87491b961bf2 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -265,7 +265,13 @@ static int proc_path_manager(const struct ctl_table *ctl, int write,
 				pm_type = MPTCP_PM_TYPE_KERNEL;
 			else if (strncmp(pm_name, "userspace", MPTCP_PM_NAME_MAX) == 0)
 				pm_type = MPTCP_PM_TYPE_USERSPACE;
-			pernet->pm_type = pm_type;
+
+			/* Pre-existing race: two sequential writes, a socket
+			 * created in between may see the new ops with the old
+			 * pm_type. The knob is deprecated since v6.15 and will
+			 * be removed: not fixed on purpose.
+			 */
+			WRITE_ONCE(pernet->pm_type, pm_type);
 		}
 	}
 
-- 
2.43.0

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

[Gang Yan]

From: Gang Yan <yangang@kylinos.cn>

This commit introduces the mptcp_pm_ops lifetime handling on sockets
(mptcp_pm_ops_init/release taking a module reference), and would then
be the first one whose per-net path managers can be unloaded while a
pernet still stores them.

mptcp_pm_ops_init() also takes the ops pointer directly instead of the
name, and mptcp_get_path_manager() returns the ops: the redundant
mptcp_pm_find() list walk from the name is avoided, as done for the
scheduler side earlier in this series.

Assisted-by: Claude:GLM5.2
Co-developed-by: Tao Cui <cuitao@kylinos.cn>
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
Signed-off-by: Gang Yan <yangang@kylinos.cn>
---
 net/mptcp/ctrl.c     | 35 +++++++++++++++++++++++++----------
 net/mptcp/pm.c       | 12 ++++--------
 net/mptcp/protocol.h |  2 +-
 3 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index 87491b961bf2..6379a9f481ac 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -93,12 +93,9 @@ static struct mptcp_pm_ops *mptcp_pernet_pm(struct mptcp_pernet *pernet)
 	return pm_ops ? pm_ops : &mptcp_pm_kernel;
 }
 
-void mptcp_get_path_manager(const struct net *net, char *name)
+struct mptcp_pm_ops *mptcp_get_path_manager(const struct net *net)
 {
-	rcu_read_lock();
-	strscpy(name, mptcp_pernet_pm(mptcp_get_pernet(net))->name,
-		MPTCP_PM_NAME_MAX);
-	rcu_read_unlock();
+	return mptcp_pernet_pm(mptcp_get_pernet(net));
 }
 
 static struct mptcp_sched_ops *mptcp_pernet_sched(struct mptcp_pernet *pernet)
@@ -135,7 +132,8 @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
 	if (bpf_try_module_get(&mptcp_sched_default, mptcp_sched_default.owner))
 		RCU_INIT_POINTER(pernet->scheduler, &mptcp_sched_default);
 
-	RCU_INIT_POINTER(pernet->path_manager, &mptcp_pm_kernel);
+	if (bpf_try_module_get(&mptcp_pm_kernel, mptcp_pm_kernel.owner))
+		RCU_INIT_POINTER(pernet->path_manager, &mptcp_pm_kernel);
 
 	pernet->add_addr_v6_port_drop_ts = 1;
 }
@@ -224,15 +222,22 @@ static int proc_blackhole_detect_timeout(const struct ctl_table *table,
 
 static int mptcp_set_path_manager(struct mptcp_pernet *pernet, const char *name)
 {
-	struct mptcp_pm_ops *pm_ops;
+	struct mptcp_pm_ops *pm_ops, *prev;
 	int ret = 0;
 
 	rcu_read_lock();
 	pm_ops = mptcp_pm_find(name);
-	if (pm_ops)
-		xchg(&pernet->path_manager, pm_ops);
-	else
+	if (pm_ops) {
+		if (bpf_try_module_get(pm_ops, pm_ops->owner)) {
+			prev = xchg(&pernet->path_manager, pm_ops);
+			if (prev)
+				bpf_module_put(prev, prev->owner);
+		} else {
+			ret = -EBUSY;
+		}
+	} else {
 		ret = -ENOENT;
+	}
 	rcu_read_unlock();
 
 	return ret;
@@ -611,10 +616,15 @@ static int __net_init mptcp_net_init(struct net *net)
 	ret = mptcp_pernet_new_table(net, pernet);
 	if (ret) {
 		struct mptcp_sched_ops *sched;
+		struct mptcp_pm_ops *pm;
 
 		sched = rcu_dereference_protected(pernet->scheduler, true);
 		if (sched)
 			bpf_module_put(sched, sched->owner);
+
+		pm = rcu_dereference_protected(pernet->path_manager, true);
+		if (pm)
+			bpf_module_put(pm, pm->owner);
 	}
 
 	return ret;
@@ -625,12 +635,17 @@ static void __net_exit mptcp_net_exit(struct net *net)
 {
 	struct mptcp_pernet *pernet = mptcp_get_pernet(net);
 	struct mptcp_sched_ops *sched;
+	struct mptcp_pm_ops *pm;
 
 	mptcp_pernet_del_table(pernet);
 
 	sched = rcu_dereference_protected(pernet->scheduler, true);
 	if (sched)
 		bpf_module_put(sched, sched->owner);
+
+	pm = rcu_dereference_protected(pernet->path_manager, true);
+	if (pm)
+		bpf_module_put(pm, pm->owner);
 }
 
 static struct pernet_operations mptcp_pernet_ops = {
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 09f99bcd827c..76158f0ac5a0 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1155,13 +1155,11 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
 	spin_unlock_bh(&msk->pm.lock);
 }
 
-static void mptcp_pm_ops_init(struct mptcp_sock *msk, const char *pm_name)
+static void mptcp_pm_ops_init(struct mptcp_sock *msk,
+			      struct mptcp_pm_ops *pm_ops)
 {
-	struct mptcp_pm_ops *pm_ops;
-
-	pm_ops = mptcp_pm_find(pm_name);
 	if (!pm_ops || !bpf_try_module_get(pm_ops, pm_ops->owner)) {
-		pr_warn_once("pm %s fails, fallback to default pm", pm_name);
+		pr_warn_once("pm %s fails, fallback to default pm", pm_ops->name);
 		pm_ops = &mptcp_pm_kernel;
 	}
 
@@ -1204,7 +1202,6 @@ void mptcp_pm_destroy(struct mptcp_sock *msk)
 void mptcp_pm_data_reset(struct mptcp_sock *msk)
 {
 	const struct net *net = sock_net((struct sock *)msk);
-	char pm_name[MPTCP_PM_NAME_MAX];
 	u8 pm_type = mptcp_get_pm_type(net);
 	struct mptcp_pm_data *pm = &msk->pm;
 
@@ -1213,9 +1210,8 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
 	pm->rm_list_rx.nr = 0;
 	WRITE_ONCE(pm->pm_type, pm_type);
 
-	mptcp_get_path_manager(net, pm_name);
 	rcu_read_lock();
-	mptcp_pm_ops_init(msk, pm_name);
+	mptcp_pm_ops_init(msk, mptcp_get_path_manager(net));
 	rcu_read_unlock();
 }
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index bbcf0ecd72d1..1deefb8d3fcc 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -802,7 +802,7 @@ int mptcp_allow_join_id0(const struct net *net);
 unsigned int mptcp_stale_loss_cnt(const struct net *net);
 unsigned int mptcp_close_timeout(const struct sock *sk);
 int mptcp_get_pm_type(const struct net *net);
-void mptcp_get_path_manager(const struct net *net, char *name);
+struct mptcp_pm_ops *mptcp_get_path_manager(const struct net *net);
 struct mptcp_sched_ops *mptcp_get_scheduler(const struct net *net);
 unsigned int mptcp_add_addr_v6_port_drop_ts(const struct net *net);
 
-- 
2.43.0

Re: [PATCH mptcp-next v3 5/5] Squash to "mptcp: pm: init and release mptcp_pm_ops"

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] msk->pm.ops is accessed concurrently without the socket lock or RCU protection during MP_JOIN processing, which can race with mptcp_disconnect() leading to a NULL pointer dereference or execution of freed module code.
--

--- Patch [5]: [PATCH mptcp-next v3 5/5] Squash to "mptcp: pm: init and release mptcp_pm_ops" ---
commit f7c7fb9e99182263f4dea26b6331ed438faaf974
Author: Gang Yan <yangang@kylinos.cn>

Squash to "mptcp: pm: init and release mptcp_pm_ops"

This commit introduces the mptcp_pm_ops lifetime handling on sockets
(mptcp_pm_ops_init/release taking a module reference), and would then
be the first one whose per-net path managers can be unloaded while a
pernet still stores them.

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 69a38cb48977a..64244a1a01bca 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -1155,13 +1155,11 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  	spin_unlock_bh(&msk->pm.lock);
>  }
>  
> -static void mptcp_pm_ops_init(struct mptcp_sock *msk, const char *pm_name)
> +static void mptcp_pm_ops_init(struct mptcp_sock *msk,
> +			      struct mptcp_pm_ops *pm_ops)
>  {

[Severity: Critical]
Since this series introduces mptcp_pm_ops lifetime handling and module
references, is there a race condition with concurrent MP_JOIN requests?

When an incoming MP_JOIN request is processed in softirq context,
subflow_token_join_request() gets the socket via mptcp_token_get_sock(),
which doesn't hold lock_sock() or RCU. It then calls:

net/mptcp/pm.c:mptcp_pm_get_local_id() {
    ...
    return msk->pm.ops->get_local_id(msk, &skc_local);
}

Concurrently, if a local user closes the socket and triggers
mptcp_disconnect(msk), it calls mptcp_pm_ops_release() which clears the ops
and drops the module reference:

net/mptcp/pm.c:mptcp_pm_ops_release() {
    ...
    msk->pm.ops = NULL;
    ...
    bpf_module_put(pm_ops, pm_ops->owner);
}

Could this lead to a NULL pointer dereference or executing freed module code
if subflow_token_join_request() executes concurrently with a socket
disconnect?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819125629.49823-1-gang.yan@linux.dev?part=5

Re: [PATCH mptcp-next v3 5/5] Squash to "mptcp: pm: init and release mptcp_pm_ops"

[gang.yan]

August 19, 2026 at 9:12 PM, sashiko-bot@kernel.org mailto:sashiko-bot@kernel.org  wrote:

> [Severity: Critical]
> Since this series introduces mptcp_pm_ops lifetime handling and module
> references, is there a race condition with concurrent MP_JOIN requests?
> 
> When an incoming MP_JOIN request is processed in softirq context,
> subflow_token_join_request() gets the socket via mptcp_token_get_sock(),
> which doesn't hold lock_sock() or RCU. It then calls:
> 
> net/mptcp/pm.c:mptcp_pm_get_local_id() {
>  ...
>  return msk->pm.ops->get_local_id(msk, &skc_local);
> }
> 
> Concurrently, if a local user closes the socket and triggers
> mptcp_disconnect(msk), it calls mptcp_pm_ops_release() which clears the ops
> and drops the module reference:
> 
> net/mptcp/pm.c:mptcp_pm_ops_release() {
>  ...
>  msk->pm.ops = NULL;
>  ...
>  bpf_module_put(pm_ops, pm_ops->owner);
> }
> 
> Could this lead to a NULL pointer dereference or executing freed module code
> if subflow_token_join_request() executes concurrently with a socket
> disconnect?
> 

Hi Matt, Geliang

@Matt, Could you help me to trigger the CI?

And I think it is a pre-existing bug: 

When it calls msk->pm.ops->get_local_id(msk, &skc_local) between
'mptcp_distroy_common' (which calls mptcp_pm_ops_release) and
'mptcp_pm_data_reset'. The pm.ops will be NULL and cause a NULL deref, right?

If I'm right, I think we can using rcu to solve this based on this series, like:

  - Add a helper to get pm.ops, and then call the get_local_id/get_priority under rcu_lock:
'''
+static struct mptcp_pm_ops *mptcp_pm_deref(struct mptcp_sock *msk)
+{
+       struct mptcp_pm_ops *pm_ops;
+
+       pm_ops = rcu_dereference(msk->pm.ops);
+       return pm_ops ? pm_ops : &mptcp_pm_kernel;
+}
+

bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
 
        mptcp_local_address((struct sock_common *)skc, &skc_local);
 
-       return msk->pm.ops->get_priority(msk, &skc_local);
+       return mptcp_pm_deref(msk)->get_priority(msk, &skc_local);
 }
 
So does the mptcp_pm_get_local_id like this.

static struct mptcp_sock *subflow_token_join_request(struct request_sock *req)
                return NULL;
        }
 
+       rcu_read_lock();
        local_id = mptcp_pm_get_local_id(msk, (struct sock_common *)req);
        if (local_id < 0) {
                SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINNOIDFOUND);
+               rcu_read_unlock();
                sock_put((struct sock *)msk);
                return NULL;
        }
        subflow_req->local_id = local_id;
        subflow_req->request_bkup = mptcp_pm_is_backup(msk, (struct sock_common *)req);
+       rcu_read_unlock();
 
'''

  - Refactor the mptcp_pm_ops_init to support reuse socket, and put the
    mptcp_pm_ops_release into mptcp_destroy:
'''
 static void mptcp_pm_ops_init(struct mptcp_sock *msk,
                              struct mptcp_pm_ops *pm_ops)
 {
+       struct mptcp_pm_ops *old = msk->pm.ops;
+
        if (!pm_ops || !bpf_try_module_get(pm_ops, pm_ops->owner)) {
                pr_warn_once("pm %s fails, fallback to default pm", pm_ops->name);
                pm_ops = &mptcp_pm_kernel;
        }
 
-       msk->pm.ops = pm_ops;
+       if (old) {
+               if (old == pm_ops) {
+                       mptcp_pm_ops_release(msk);
+               } else {
+                       rcu_assign_pointer(msk->pm.ops, pm_ops);
+                       synchronize_rcu();
+                       bpf_module_put(old, old->owner);
+               }
+       } else {
+               rcu_assign_pointer(msk->pm.ops, pm_ops);
+       }
+
        if (msk->pm.ops->init)
                msk->pm.ops->init(msk);
 
-       pr_debug("pm %s initialized\n", pm_ops->name);
+       pr_debug("pm %s initialized\n", msk->pm.ops->name);
 }
 


static void mptcp_destroy(struct sock *sk)
        /* allow the following to close even the initial subflow */
        msk->free_first = 1;
        mptcp_destroy_common(msk);
+       mptcp_pm_ops_release(msk);
        sk_sockets_allocated_dec(sk);
 }

'''

But I think it is a large fix, do you have any idea?

And as I said at the beginning, it seeems like not a issue attached to this issue,
could you review the v3 code with ignoring it ?

Thanks
Gang


> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260819125629.49823-1-gang.yan@linux.dev?part=5
>

Re: [PATCH mptcp-next v3 0/5] mptcp: avoid data-races around the sysctls

[MPTCP CI]

Hi Gang,

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): Unstable: 1 failed test(s): selftest_simult_flows ⚠️ 
- 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/32342598821

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


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-next v3 5/5] Squash to "mptcp: pm: init and release mptcp_pm_ops"

[Matthieu Baerts]

Hi Gang,

On 20/08/2026 08:36, gang.yan@linux.dev wrote:
> August 19, 2026 at 9:12 PM, sashiko-bot@kernel.org mailto:sashiko-bot@kernel.org  wrote:
> 
>> [Severity: Critical]
>> Since this series introduces mptcp_pm_ops lifetime handling and module
>> references, is there a race condition with concurrent MP_JOIN requests?
>>
>> When an incoming MP_JOIN request is processed in softirq context,
>> subflow_token_join_request() gets the socket via mptcp_token_get_sock(),
>> which doesn't hold lock_sock() or RCU. It then calls:
>>
>> net/mptcp/pm.c:mptcp_pm_get_local_id() {
>>  ...
>>  return msk->pm.ops->get_local_id(msk, &skc_local);
>> }
>>
>> Concurrently, if a local user closes the socket and triggers
>> mptcp_disconnect(msk), it calls mptcp_pm_ops_release() which clears the ops
>> and drops the module reference:
>>
>> net/mptcp/pm.c:mptcp_pm_ops_release() {
>>  ...
>>  msk->pm.ops = NULL;
>>  ...
>>  bpf_module_put(pm_ops, pm_ops->owner);
>> }
>>
>> Could this lead to a NULL pointer dereference or executing freed module code
>> if subflow_token_join_request() executes concurrently with a socket
>> disconnect?
>>
> 
> Hi Matt, Geliang
> 
> @Matt, Could you help me to trigger the CI?

It looks like GitHub was sick that day. I reset the tag manually.

> And I think it is a pre-existing bug: 
> 
> When it calls msk->pm.ops->get_local_id(msk, &skc_local) between
> 'mptcp_distroy_common' (which calls mptcp_pm_ops_release) and
> 'mptcp_pm_data_reset'. The pm.ops will be NULL and cause a NULL deref, right?

Can you first check if this can happen? I thought that
mptcp_destroy_common would first close all subflows, remove the token,
then release everything linked to the PM → so the PM will no longer be
called at that point for this msk, right? (I didn't check)

But maybe this will change when the BPF PM will be fully implemented? I
guess no because msk->pm.ops should only be called from events linked to
the network, so not after mptcp_destroy_common?

> If I'm right, I think we can using rcu to solve this based on this series, like:
> 
>   - Add a helper to get pm.ops, and then call the get_local_id/get_priority under rcu_lock:
> '''
> +static struct mptcp_pm_ops *mptcp_pm_deref(struct mptcp_sock *msk)
> +{
> +       struct mptcp_pm_ops *pm_ops;
> +
> +       pm_ops = rcu_dereference(msk->pm.ops);
> +       return pm_ops ? pm_ops : &mptcp_pm_kernel;
> +}
> +
> 
> bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
>  
>         mptcp_local_address((struct sock_common *)skc, &skc_local);
>  
> -       return msk->pm.ops->get_priority(msk, &skc_local);
> +       return mptcp_pm_deref(msk)->get_priority(msk, &skc_local);
>  }
>  
> So does the mptcp_pm_get_local_id like this.

I think you should then rename the helpers, to make it clear they need
to be used from a RCU read section, and to get a warning when
backporting code around that.

> static struct mptcp_sock *subflow_token_join_request(struct request_sock *req)
>                 return NULL;
>         }
>  
> +       rcu_read_lock();
>         local_id = mptcp_pm_get_local_id(msk, (struct sock_common *)req);
>         if (local_id < 0) {
>                 SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINNOIDFOUND);
> +               rcu_read_unlock();
>                 sock_put((struct sock *)msk);
>                 return NULL;
>         }
>         subflow_req->local_id = local_id;
>         subflow_req->request_bkup = mptcp_pm_is_backup(msk, (struct sock_common *)req);
> +       rcu_read_unlock();
>  
> '''
> 
>   - Refactor the mptcp_pm_ops_init to support reuse socket, and put the
>     mptcp_pm_ops_release into mptcp_destroy:
> '''
>  static void mptcp_pm_ops_init(struct mptcp_sock *msk,
>                               struct mptcp_pm_ops *pm_ops)
>  {
> +       struct mptcp_pm_ops *old = msk->pm.ops;
> +
>         if (!pm_ops || !bpf_try_module_get(pm_ops, pm_ops->owner)) {
>                 pr_warn_once("pm %s fails, fallback to default pm", pm_ops->name);
>                 pm_ops = &mptcp_pm_kernel;
>         }
>  
> -       msk->pm.ops = pm_ops;
> +       if (old) {
> +               if (old == pm_ops) {
> +                       mptcp_pm_ops_release(msk);

Maybe return here, no need to re-init, right?

> +               } else {
> +                       rcu_assign_pointer(msk->pm.ops, pm_ops);
> +                       synchronize_rcu();
> +                       bpf_module_put(old, old->owner);
> +               }
> +       } else {
> +               rcu_assign_pointer(msk->pm.ops, pm_ops);
> +       }
> +
>         if (msk->pm.ops->init)
>                 msk->pm.ops->init(msk);
>  
> -       pr_debug("pm %s initialized\n", pm_ops->name);
> +       pr_debug("pm %s initialized\n", msk->pm.ops->name);
>  }
>  
> 
> 
> static void mptcp_destroy(struct sock *sk)
>         /* allow the following to close even the initial subflow */
>         msk->free_first = 1;
>         mptcp_destroy_common(msk);
> +       mptcp_pm_ops_release(msk);
>         sk_sockets_allocated_dec(sk);
>  }
> 
> '''
> 
> But I think it is a large fix, do you have any idea?
> 
> And as I said at the beginning, it seeems like not a issue attached to this issue,
> could you review the v3 code with ignoring it ?

Yes, the 4 first patches can be reviewed.

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

Re: [PATCH mptcp-next v3 5/5] Squash to "mptcp: pm: init and release mptcp_pm_ops"

[gang.yan]

August 20, 2026 at 5:07 PM, "Matthieu Baerts" <matttbe@kernel.org mailto:matttbe@kernel.org?to=%22Matthieu%20Baerts%22%20%3Cmatttbe%40kernel.org%3E > wrote:

> Can you first check if this can happen? I thought that
> mptcp_destroy_common would first close all subflows, remove the token,
> then release everything linked to the PM → so the PM will no longer be
> called at that point for this msk, right? (I didn't check)
>

Hi Matt,

Thans for looking at this and kindly remind.
Yes, I just checked the order you described is right, but I thought it
could happen in such order:

CPU A (softirq)                                 CPU B(disconnect)
->subflow_token_join_request          
->mptcp_token_get_sock()
                                              ->mptcp_destroy_common()
                                              ->pm_ops_release
->mptcp_pm_get_local_id():
  msk->pm.ops->get_local_id()   <- NULL deref

WDYT?  And, I think the window seems too narrow, we may not see it in practice,
maybe a NULL check is enough? 

Thanks
Gang

Re: [PATCH mptcp-next v3 5/5] Squash to "mptcp: pm: init and release mptcp_pm_ops"

[Matthieu Baerts]

On 20/08/2026 12:16, gang.yan@linux.dev wrote:
> August 20, 2026 at 5:07 PM, "Matthieu Baerts" <matttbe@kernel.org mailto:matttbe@kernel.org?to=%22Matthieu%20Baerts%22%20%3Cmatttbe%40kernel.org%3E > wrote:
> 
>> Can you first check if this can happen? I thought that
>> mptcp_destroy_common would first close all subflows, remove the token,
>> then release everything linked to the PM → so the PM will no longer be
>> called at that point for this msk, right? (I didn't check)
>>
> 
> Hi Matt,
> 
> Thans for looking at this and kindly remind.
> Yes, I just checked the order you described is right, but I thought it
> could happen in such order:
> 
> CPU A (softirq)                                 CPU B(disconnect)
> ->subflow_token_join_request          
> ->mptcp_token_get_sock()
>                                               ->mptcp_destroy_common()
>                                               ->pm_ops_release
> ->mptcp_pm_get_local_id():
>   msk->pm.ops->get_local_id()   <- NULL deref

Ah yes, indeed, subflow_token_join_request doesn't hold the msk lock.

> WDYT?  And, I think the window seems too narrow, we may not see it in practice,
> maybe a NULL check is enough? 

Would it be safe to have a NULL check? Could you have something like this?

CPU A                              CPU B
-> if (ops)
                                    -> mptcp_destroy_common()
                                    -> pm_ops_release()
->     msk->pm.ops->get_local_id()

The BPF module might have been unloaded.

(Again, I didn't check in details, only looking at your reply.)

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

Re: [PATCH mptcp-next v3 5/5] Squash to "mptcp: pm: init and release mptcp_pm_ops"

[gang.yan]

August 20, 2026 at 7:06 PM, "Matthieu Baerts" <matttbe@kernel.org mailto:matttbe@kernel.org?to=%22Matthieu%20Baerts%22%20%3Cmatttbe%40kernel.org%3E > wrote:


> 
> On 20/08/2026 12:16, gang.yan@linux.dev wrote:
> 
> > 
> > August 20, 2026 at 5:07 PM, "Matthieu Baerts" <matttbe@kernel.org mailto:matttbe@kernel.org?to=%22Matthieu%20Baerts%22%20%3Cmatttbe%40kernel.org%3E > wrote:
> >  
> > 
> > > 
> > > Can you first check if this can happen? I thought that
> > >  mptcp_destroy_common would first close all subflows, remove the token,
> > >  then release everything linked to the PM → so the PM will no longer be
> > >  called at that point for this msk, right? (I didn't check)
> > > 
> >  
> >  Hi Matt,
> >  
> >  Thans for looking at this and kindly remind.
> >  Yes, I just checked the order you described is right, but I thought it
> >  could happen in such order:
> >  
> >  CPU A (softirq) CPU B(disconnect)
> >  ->subflow_token_join_request 
> >  ->mptcp_token_get_sock()
> >  ->mptcp_destroy_common()
> >  ->pm_ops_release
> >  ->mptcp_pm_get_local_id():
> >  msk->pm.ops->get_local_id() <- NULL deref
> > 
> Ah yes, indeed, subflow_token_join_request doesn't hold the msk lock.
> 
> > 
> > WDYT? And, I think the window seems too narrow, we may not see it in practice,
> >  maybe a NULL check is enough?
> > 
> Would it be safe to have a NULL check? Could you have something like this?
> 
> CPU A CPU B
> -> if (ops)
>  -> mptcp_destroy_common()
>  -> pm_ops_release()
> -> msk->pm.ops->get_local_id()
> 
> The BPF module might have been unloaded.
Hi Matt

Yes, sorry for the silly mistake.

I’ll check if RCU can help us address this problem, and update the patch.

Thanks
Gang

> 
> (Again, I didn't check in details, only looking at your reply.)
> 
> Cheers,
> Matt
> -- 
> Sponsored by the NGI0 Core fund.
>