[PATCH mptcp-next v4 0/6] mptcp: avoid data-races around the sysctls

[PATCH mptcp-next v4 0/6] mptcp: avoid data-races around the sysctls

[Gang Yan]

From: Gang Yan <yangang@kylinos.cn>

Changelog:
v4:
 - Patch 6 is new to this series, for addressing the sashiko's comments
   in [1] using rcu. Please note it should also be squashed to "mptcp:
   pm: init and release mptcp_pm_ops", but spilting it may ease the
   review.

v3:
 Link: https://patchwork.kernel.org/project/mptcp/cover/20260819125629.49823-1-gang.yan@linux.dev/

v2:
 Link: https://patchwork.kernel.org/project/mptcp/cover/20260818094825.48446-1-gang.yan@linux.dev/

v2:
 Link: https://patchwork.kernel.org/project/mptcp/cover/20260817012452.7519-1-gang.yan@linux.dev/

[1] https://sashiko.dev/#/patchset/20260819125629.49823-1-gang.yan@linux.dev?part=5
Gang Yan (5):
  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"
  Squash to previous one

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

 net/mptcp/ctrl.c     | 141 ++++++++++++++++++++++++++++++++-----------
 net/mptcp/pm.c       |  61 ++++++++++++++-----
 net/mptcp/protocol.c |   7 ++-
 net/mptcp/protocol.h |   9 +--
 net/mptcp/sched.c    |   2 +-
 net/mptcp/subflow.c  |   9 ++-
 6 files changed, 170 insertions(+), 59 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v4 1/6] 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 c2762d74f29d..4d7cb50ec61a 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 v4 2/6] 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 d7c5b50b34cc..69a38cb48977 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 v4 3/6] 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 v4 4/6] 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 v4 5/6] 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 69a38cb48977..64244a1a01bc 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

[PATCH mptcp-next v4 6/6] Squash to previous one

[Gang Yan]

From: Gang Yan <yangang@kylinos.cn>

Fix the issue sashiko mentioned in [1]. This patch applies the usual
RCU discipline to the pointer: mark it __rcu, read it via
rcu_dereference() inside RCU read sections, assign it via
rcu_assign_pointer(), wait for a grace period before dropping the
reference on the ops being replaced, and do the final release from
mptcp_destroy() -- after the last msk reference -- instead of
mptcp_destroy_common(), which is also reached on mptcp_disconnect().

Also, we need to hold the pm.lock before modify the pm.ops, and using
the status of lock to tell rcu it's safe.

Keep it as a separate patch to ease the review, but to be squashed into
the previous one, which is itself a squash-to for "mptcp: pm: init and
release mptcp_pm_ops".

[1] https://sashiko.dev/#/patchset/20260819125629.49823-1-gang.yan@linux.dev?part=5

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/pm.c       | 54 +++++++++++++++++++++++++++++++++++---------
 net/mptcp/protocol.c |  4 ++++
 net/mptcp/protocol.h |  3 ++-
 net/mptcp/subflow.c  |  9 +++++++-
 4 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 64244a1a01bc..ae75e68bc662 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -27,6 +27,14 @@ static LIST_HEAD(mptcp_pm_list);
 
 /* path manager helpers */
 
+static struct mptcp_pm_ops *mptcp_pm_rcu_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;
+}
+
 /* if sk is ipv4 or ipv6_only allows only same-family local and remote addresses,
  * otherwise allow any matching local/remote pair
  */
@@ -1049,7 +1057,7 @@ 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);
+	return mptcp_pm_rcu_deref(msk)->get_local_id(msk, &skc_local);
 }
 
 bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
@@ -1058,7 +1066,7 @@ 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_rcu_deref(msk)->get_priority(msk, &skc_local);
 }
 
 static void
@@ -1158,23 +1166,46 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
 static void mptcp_pm_ops_init(struct mptcp_sock *msk,
 			      struct mptcp_pm_ops *pm_ops)
 {
+	struct mptcp_pm_ops *old;
+	bool need_sync = false;
+
 	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 (msk->pm.ops->init)
-		msk->pm.ops->init(msk);
+	spin_lock_bh(&msk->pm.lock);
+	old = rcu_dereference_protected(msk->pm.ops,
+					lockdep_is_held(&msk->pm.lock));
+	if (old == pm_ops) {
+		need_sync = false;
+	} else {
+		rcu_assign_pointer(msk->pm.ops, pm_ops);
+		need_sync = !!old;
+	}
+	spin_unlock_bh(&msk->pm.lock);
+
+	if (need_sync)
+		synchronize_rcu();
+	if (old)
+		bpf_module_put(old, old->owner);
+
+	if (pm_ops->init)
+		pm_ops->init(msk);
 
 	pr_debug("pm %s initialized\n", pm_ops->name);
 }
 
-static void mptcp_pm_ops_release(struct mptcp_sock *msk)
+void mptcp_pm_ops_release(struct mptcp_sock *msk)
 {
-	struct mptcp_pm_ops *pm_ops = msk->pm.ops;
+	struct mptcp_pm_ops *pm_ops;
+
+	spin_lock_bh(&msk->pm.lock);
+	pm_ops = rcu_dereference_protected(msk->pm.ops,
+					   lockdep_is_held(&msk->pm.lock));
+	rcu_assign_pointer(msk->pm.ops, NULL);
+	spin_unlock_bh(&msk->pm.lock);
 
-	msk->pm.ops = NULL;
 	if (pm_ops->release)
 		pm_ops->release(msk);
 
@@ -1195,8 +1226,6 @@ void mptcp_pm_destroy(struct mptcp_sock *msk)
 	 * can be reused (mptcp_disconnect()) and re-selected to a different PM
 	 */
 	mptcp_userspace_pm_free_local_addr_list(msk);
-
-	mptcp_pm_ops_release(msk);
 }
 
 void mptcp_pm_data_reset(struct mptcp_sock *msk)
@@ -1204,6 +1233,7 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
 	const struct net *net = sock_net((struct sock *)msk);
 	u8 pm_type = mptcp_get_pm_type(net);
 	struct mptcp_pm_data *pm = &msk->pm;
+	struct mptcp_pm_ops *pm_ops;
 
 	memset(&pm->reset, 0, sizeof(pm->reset));
 	pm->rm_list_tx.nr = 0;
@@ -1211,8 +1241,10 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
 	WRITE_ONCE(pm->pm_type, pm_type);
 
 	rcu_read_lock();
-	mptcp_pm_ops_init(msk, mptcp_get_path_manager(net));
+	pm_ops = mptcp_get_path_manager(net);
 	rcu_read_unlock();
+
+	mptcp_pm_ops_init(msk, pm_ops);
 }
 
 void mptcp_pm_data_init(struct mptcp_sock *msk)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 4d7cb50ec61a..6a139f28c2ae 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3746,6 +3746,9 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
 		inet_sk(nsk)->pinet6 = mptcp_inet6_sk(nsk);
 #endif
 
+	msk = mptcp_sk(nsk);
+	RCU_INIT_POINTER(msk->pm.ops, NULL);
+
 	__mptcp_init_sock(nsk);
 
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
@@ -3815,6 +3818,7 @@ 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);
 }
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 1deefb8d3fcc..534f83781265 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -221,7 +221,7 @@ struct mptcp_pm_data {
 	struct mptcp_addr_info remote;
 	struct list_head anno_list;
 	struct list_head userspace_pm_local_addr_list;
-	struct mptcp_pm_ops *ops;
+	struct mptcp_pm_ops __rcu *ops;	/* RCU: read via mptcp_pm_rcu_deref() */
 
 	spinlock_t	lock;		/*protects the whole PM data */
 
@@ -1101,6 +1101,7 @@ void __init mptcp_pm_init(void);
 void mptcp_pm_data_init(struct mptcp_sock *msk);
 void mptcp_pm_data_reset(struct mptcp_sock *msk);
 void mptcp_pm_destroy(struct mptcp_sock *msk);
+void mptcp_pm_ops_release(struct mptcp_sock *msk);
 int mptcp_pm_parse_addr(struct nlattr *attr, struct genl_info *info,
 			struct mptcp_addr_info *addr);
 int mptcp_pm_parse_entry(struct nlattr *attr, struct genl_info *info,
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index af81ad5e699d..c4a1169fab07 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -94,14 +94,17 @@ 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();
 
 	return msk;
 }
@@ -634,12 +637,16 @@ static int subflow_chk_local_id(struct sock *sk)
 	if (likely(subflow->local_id >= 0))
 		return 0;
 
+	rcu_read_lock();
 	err = mptcp_pm_get_local_id(msk, (struct sock_common *)sk);
-	if (err < 0)
+	if (err < 0) {
+		rcu_read_unlock();
 		return err;
+	}
 
 	subflow_set_local_id(subflow, err);
 	subflow->request_bkup = mptcp_pm_is_backup(msk, (struct sock_common *)sk);
+	rcu_read_unlock();
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH mptcp-next v4 6/6] Squash to previous one

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Use-After-Free of PM ops pointer due to premature RCU unlock.
- [High] Resource Leak and Lifecycle Violation in PM ops replacement.
- [High] Use-Before-Initialization of PM ops via RCU publishing.
--

--- Patch [6]: [PATCH mptcp-next v4 6/6] Squash to previous one ---
commit 300bc1205800c2307a03b20ff740b6c4f84c422e
Author: Gang Yan <yangang@kylinos.cn>

Squash to previous one

This commit applies RCU discipline to the msk->pm.ops pointer, marking
it __rcu and adding appropriate locks and synchronization.

Link: https://sashiko.dev/#/patchset/20260819125629.49823-1-gang.yan@linux.dev?part=5

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c

[ ... ]

> @@ -1158,23 +1166,46 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  static void mptcp_pm_ops_init(struct mptcp_sock *msk,
>  			      struct mptcp_pm_ops *pm_ops)
>  {
> +	struct mptcp_pm_ops *old;
> +	bool need_sync = false;
> +
>  	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 (msk->pm.ops->init)
> -		msk->pm.ops->init(msk);
> +	spin_lock_bh(&msk->pm.lock);
> +	old = rcu_dereference_protected(msk->pm.ops,
> +					lockdep_is_held(&msk->pm.lock));
> +	if (old == pm_ops) {
> +		need_sync = false;
> +	} else {
> +		rcu_assign_pointer(msk->pm.ops, pm_ops);

[Severity: High]
Does this publish the new pm_ops pointer before it is fully initialized?

The rcu_assign_pointer() call exposes pm_ops to concurrent readers before
pm_ops->init(msk) is called. Could concurrent access during the grace period
execute callbacks on a partially initialized state?

> +		need_sync = !!old;
> +	}
> +	spin_unlock_bh(&msk->pm.lock);
> +
> +	if (need_sync)
> +		synchronize_rcu();
> +	if (old)
> +		bpf_module_put(old, old->owner);
> +
> +	if (pm_ops->init)
> +		pm_ops->init(msk);

[Severity: High]
When replacing an existing path manager, does this code leak the old resources?

The code calls bpf_module_put(old, old->owner) but doesn't seem to call
old->release(msk) to free the actual path manager resources.

Also, if old == pm_ops, we still reach pm_ops->init(msk) here. Will
unconditionally calling init() on unchanged ops cause a double-allocation?

>  	pr_debug("pm %s initialized\n", pm_ops->name);
>  }

[ ... ]

> @@ -1204,6 +1233,7 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
>  	const struct net *net = sock_net((struct sock *)msk);
>  	u8 pm_type = mptcp_get_pm_type(net);
>  	struct mptcp_pm_data *pm = &msk->pm;
> +	struct mptcp_pm_ops *pm_ops;
>  
>  	memset(&pm->reset, 0, sizeof(pm->reset));
>  	pm->rm_list_tx.nr = 0;
> @@ -1211,8 +1241,10 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
>  	WRITE_ONCE(pm->pm_type, pm_type);
>  
>  	rcu_read_lock();
> -	mptcp_pm_ops_init(msk, mptcp_get_path_manager(net));
> +	pm_ops = mptcp_get_path_manager(net);
>  	rcu_read_unlock();
> +
> +	mptcp_pm_ops_init(msk, pm_ops);
>  }

[Severity: High]
Can dropping the RCU read lock here lead to a use-after-free?

The rcu_read_unlock() invalidates the pm_ops pointer before it is passed to
mptcp_pm_ops_init(), which then dereferences it (via pm_ops->owner) in
bpf_try_module_get(). 

Should the lock be held across the initialization, or should the module
reference be taken before dropping the read lock?

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

Re: [PATCH mptcp-next v4 6/6] Squash to previous one

[gang.yan]

August 24, 2026 at 3:58 PM, sashiko-bot@kernel.org mailto:sashiko-bot@kernel.org  wrote:

> >  + struct mptcp_pm_ops *old;
> >  + bool need_sync = false;
> >  +
> >  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 (msk->pm.ops->init)
> >  - msk->pm.ops->init(msk);
> >  + spin_lock_bh(&msk->pm.lock);
> >  + old = rcu_dereference_protected(msk->pm.ops,
> >  + lockdep_is_held(&msk->pm.lock));
> >  + if (old == pm_ops) {
> >  + need_sync = false;
> >  + } else {
> >  + rcu_assign_pointer(msk->pm.ops, pm_ops);
> > 
> [Severity: High]
> Does this publish the new pm_ops pointer before it is fully initialized?
> 
> The rcu_assign_pointer() call exposes pm_ops to concurrent readers before
> pm_ops->init(msk) is called. Could concurrent access during the grace period
> execute callbacks on a partially initialized state?
> 

Hi

The ordering is safe, because what pm_ops->init() initialises is the per-socket
PM state (msk->pm.*), not the ops themselves.

The ops are statically registered and unchangeable -- their callbacks are valid
from registration, so the published pointer never refers to a half-initialised
object.


> > 
> > + need_sync = !!old;
> >  + }
> >  + spin_unlock_bh(&msk->pm.lock);
> >  +
> >  + if (need_sync)
> >  + synchronize_rcu();
> >  + if (old)
> >  + bpf_module_put(old, old->owner);
> >  +
> >  + if (pm_ops->init)
> >  + pm_ops->init(msk);
> > 
> [Severity: High]
> When replacing an existing path manager, does this code leak the old resources?
> 
> The code calls bpf_module_put(old, old->owner) but doesn't seem to call
> old->release(msk) to free the actual path manager resources.

On the missing old->release(): on the disconnect path the per-socket
PM resources are already freed before we get there --
mptcp_pm_destroy() unconditionally calls
mptcp_userspace_pm_free_local_addr_list() (and the announced list)
from mptcp_destroy_common(), so by the time mptcp_pm_data_reset()
swaps the ops there is nothing left for old->release() to free. The
kernel PM has no per-socket resources and no release callback today,
and the userspace PM's release only frees the local address list,
which is covered by the above.


> 
> Also, if old == pm_ops, we still reach pm_ops->init(msk) here. Will
> unconditionally calling init() on unchanged ops cause a double-allocation?
> 

that is the pre-existing behaviour -- the socket was just torn down by
disconnect() and the PM flags (work_pending, accept_addr, ...) need to be
rebuilt from the current sysctl values. mptcp_pm_kernel_init() only sets
those flags, it does not allocate, so re-running it is an idempotent
reset, not a double allocation.


> > 
> > pr_debug("pm %s initialized\n", pm_ops->name);
> >  }
> > 
> [ ... ]
> 
> > 
> > @@ -1204,6 +1233,7 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
> >  const struct net *net = sock_net((struct sock *)msk);
> >  u8 pm_type = mptcp_get_pm_type(net);
> >  struct mptcp_pm_data *pm = &msk->pm;
> >  + struct mptcp_pm_ops *pm_ops;
> >  
> >  memset(&pm->reset, 0, sizeof(pm->reset));
> >  pm->rm_list_tx.nr = 0;
> >  @@ -1211,8 +1241,10 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
> >  WRITE_ONCE(pm->pm_type, pm_type);
> >  
> >  rcu_read_lock();
> >  - mptcp_pm_ops_init(msk, mptcp_get_path_manager(net));
> >  + pm_ops = mptcp_get_path_manager(net);
> >  rcu_read_unlock();
> >  +
> >  + mptcp_pm_ops_init(msk, pm_ops);
> >  }
> > 
> [Severity: High]
> Can dropping the RCU read lock here lead to a use-after-free?
> 
> The rcu_read_unlock() invalidates the pm_ops pointer before it is passed to
> mptcp_pm_ops_init(), which then dereferences it (via pm_ops->owner) in
> bpf_try_module_get(). 
> 
> Should the lock be held across the initialization, or should the module
> reference be taken before dropping the read lock?

Yes, and I think it can be fixed through this:

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index ae75e68bc662..769b084ed9cd 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1169,11 +1169,6 @@ static void mptcp_pm_ops_init(struct mptcp_sock *msk,
        struct mptcp_pm_ops *old;
        bool need_sync = false;
 
-       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;
-       }
-
        spin_lock_bh(&msk->pm.lock);
        old = rcu_dereference_protected(msk->pm.ops,
                                        lockdep_is_held(&msk->pm.lock));
@@ -1242,6 +1237,11 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
 
        rcu_read_lock();
        pm_ops = mptcp_get_path_manager(net);
+       if (!pm_ops || !bpf_try_module_get(pm_ops, pm_ops->owner)) {
+               pr_warn_once("pm %s fails, fallback to default pm",
+                            pm_ops ? pm_ops->name : NULL);
+               pm_ops = &mptcp_pm_kernel;
+       }
        rcu_read_unlock();
 
        mptcp_pm_ops_init(msk, pm_ops);


WDYT? @Maintainers. If yes, it can be will done in v5.

Thanks
Gang

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

Re: [PATCH mptcp-next v4 0/6] 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): 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/32704090606

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


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)