* [PATCH mptcp-next v7 1/6] mptcp: sched: change scheduler sysctl atomically
2026-09-23 9:49 [PATCH mptcp-next v7 0/6] mptcp: avoid data-races around the sysctls Gang Yan
@ 2026-09-23 9:49 ` Gang Yan
2026-09-23 9:49 ` [PATCH mptcp-next v7 2/6] mptcp: pm: change path_manager " Gang Yan
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Gang Yan @ 2026-09-23 9:49 UTC (permalink / raw)
To: mptcp
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 09779428e9ea..eb066f2152f7 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3275,8 +3275,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 4bf04f9ecbd9..c5972e5cd3ca 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -818,7 +818,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);
@@ -1170,6 +1170,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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH mptcp-next v7 2/6] mptcp: pm: change path_manager sysctl atomically
2026-09-23 9:49 [PATCH mptcp-next v7 0/6] mptcp: avoid data-races around the sysctls Gang Yan
2026-09-23 9:49 ` [PATCH mptcp-next v7 1/6] mptcp: sched: change scheduler sysctl atomically Gang Yan
@ 2026-09-23 9:49 ` Gang Yan
2026-09-23 9:49 ` [PATCH mptcp-next v7 3/6] mptcp: use READ_ONCE() over sysctls Gang Yan
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Gang Yan @ 2026-09-23 9:49 UTC (permalink / raw)
To: mptcp
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 c5972e5cd3ca..9f0484286a90 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -817,7 +817,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);
@@ -1168,7 +1168,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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH mptcp-next v7 3/6] mptcp: use READ_ONCE() over sysctls
2026-09-23 9:49 [PATCH mptcp-next v7 0/6] mptcp: avoid data-races around the sysctls Gang Yan
2026-09-23 9:49 ` [PATCH mptcp-next v7 1/6] mptcp: sched: change scheduler sysctl atomically Gang Yan
2026-09-23 9:49 ` [PATCH mptcp-next v7 2/6] mptcp: pm: change path_manager " Gang Yan
@ 2026-09-23 9:49 ` Gang Yan
2026-09-23 9:49 ` [PATCH mptcp-next v7 4/6] mptcp: pm: use WRITE_ONCE() for the pm_type sysctl Gang Yan
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Gang Yan @ 2026-09-23 9:49 UTC (permalink / raw)
To: mptcp
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH mptcp-next v7 4/6] mptcp: pm: use WRITE_ONCE() for the pm_type sysctl
2026-09-23 9:49 [PATCH mptcp-next v7 0/6] mptcp: avoid data-races around the sysctls Gang Yan
` (2 preceding siblings ...)
2026-09-23 9:49 ` [PATCH mptcp-next v7 3/6] mptcp: use READ_ONCE() over sysctls Gang Yan
@ 2026-09-23 9:49 ` Gang Yan
2026-09-23 9:49 ` [PATCH mptcp-next v7 5/6] Squash-to "mptcp: pm: init and release mptcp_pm_ops" Gang Yan
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Gang Yan @ 2026-09-23 9:49 UTC (permalink / raw)
To: mptcp
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH mptcp-next v7 5/6] Squash-to "mptcp: pm: init and release mptcp_pm_ops"
2026-09-23 9:49 [PATCH mptcp-next v7 0/6] mptcp: avoid data-races around the sysctls Gang Yan
` (3 preceding siblings ...)
2026-09-23 9:49 ` [PATCH mptcp-next v7 4/6] mptcp: pm: use WRITE_ONCE() for the pm_type sysctl Gang Yan
@ 2026-09-23 9:49 ` Gang Yan
2026-09-23 9:49 ` [PATCH mptcp-next v7 6/6] Squash to previous one Gang Yan
2026-09-23 10:52 ` [PATCH mptcp-next v7 0/6] mptcp: avoid data-races around the sysctls MPTCP CI
6 siblings, 0 replies; 10+ messages in thread
From: Gang Yan @ 2026-09-23 9:49 UTC (permalink / raw)
To: mptcp
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 where the per-net path manager can belong to an
unloadable module: a pernet slot must therefore hold a reference on
the ops it stores, which is what this patch adds.
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 9f0484286a90..33c9893921b6 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -817,7 +817,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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH mptcp-next v7 6/6] Squash to previous one
2026-09-23 9:49 [PATCH mptcp-next v7 0/6] mptcp: avoid data-races around the sysctls Gang Yan
` (4 preceding siblings ...)
2026-09-23 9:49 ` [PATCH mptcp-next v7 5/6] Squash-to "mptcp: pm: init and release mptcp_pm_ops" Gang Yan
@ 2026-09-23 9:49 ` Gang Yan
2026-09-23 10:11 ` sashiko-bot
2026-09-23 10:52 ` [PATCH mptcp-next v7 0/6] mptcp: avoid data-races around the sysctls MPTCP CI
6 siblings, 1 reply; 10+ messages in thread
From: Gang Yan @ 2026-09-23 9:49 UTC (permalink / raw)
To: mptcp
From: Gang Yan <yangang@kylinos.cn>
Apply RCU discipline to msk->pm.ops: mark it __rcu, read it through
rcu_dereference() inside RCU read sections, publish it with
rcu_assign_pointer() under pm.lock, and wait for a grace period
before dropping the reference on the ops being retired -- both when
it is replaced on a reset and at the final release, moved from
mptcp_destroy_common() (also reached on mptcp_disconnect()) to
mptcp_destroy(), the final close path: mptcp_token_destroy() has
already removed the socket from the token hash by then, so no new
reader can find the msk anymore.
Note that rcu_assign_pointer() before pm_ops->init(msk) does not
publish a partially initialised object: the ops are registered
immutable, init() only prepares the per-socket state, and the msk is
not reachable by readers until its token is registered.
Note that for the in-tree path managers none of this changes
behaviour: neither defines release(), their init() only re-sets
flags, and the per-socket state is freed unconditionally by
mptcp_pm_destroy() on every disconnect.
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 | 59 +++++++++++++++++++++++++++++++++-----------
net/mptcp/protocol.c | 4 +++
net/mptcp/protocol.h | 3 ++-
net/mptcp/subflow.c | 9 ++++++-
4 files changed, 59 insertions(+), 16 deletions(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 64244a1a01bc..66fb7c3d9c82 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,39 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
static void mptcp_pm_ops_init(struct mptcp_sock *msk,
struct mptcp_pm_ops *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;
+ struct mptcp_pm_ops *old;
+
+ spin_lock_bh(&msk->pm.lock);
+ old = rcu_dereference_protected(msk->pm.ops,
+ lockdep_is_held(&msk->pm.lock));
+ if (old != pm_ops)
+ rcu_assign_pointer(msk->pm.ops, pm_ops);
+ spin_unlock_bh(&msk->pm.lock);
+
+ if (old) {
+ synchronize_rcu();
+ if (old->release)
+ old->release(msk);
+ bpf_module_put(old, old->owner);
}
- msk->pm.ops = pm_ops;
- if (msk->pm.ops->init)
- msk->pm.ops->init(msk);
+ 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;
- msk->pm.ops = NULL;
+ 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);
+
+ synchronize_rcu();
if (pm_ops->release)
pm_ops->release(msk);
@@ -1195,8 +1219,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 +1226,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 +1234,16 @@ 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);
+ 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;
+ bpf_try_module_get(pm_ops, pm_ops->owner);
+ }
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 eb066f2152f7..48c0434ab0d3 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3759,6 +3759,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)
@@ -3828,6 +3831,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 33c9893921b6..309e2251f2f3 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -236,7 +236,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 */
@@ -1116,6 +1116,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 14aa82647c3d..4953cfa329e8 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;
}
@@ -638,12 +641,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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH mptcp-next v7 6/6] Squash to previous one
2026-09-23 9:49 ` [PATCH mptcp-next v7 6/6] Squash to previous one Gang Yan
@ 2026-09-23 10:11 ` sashiko-bot
2026-09-24 1:53 ` gang.yan
0 siblings, 1 reply; 10+ messages in thread
From: sashiko-bot @ 2026-09-23 10:11 UTC (permalink / raw)
To: Gang Yan; +Cc: mptcp
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: uaf and uninitialized access in pm_ops during disconnect
--
commit 89b80a47ef923bdc04a9069fdcf06de447841c5b
Author: Gang Yan <yangang@kylinos.cn>
Squash to previous one
This patch applies RCU discipline to msk->pm.ops, moving its initialization,
publication, and release to better align with RCU patterns. It updates readers
to use rcu_dereference() and writers to use rcu_assign_pointer() under the
PM lock.
Link: https://sashiko.dev/#/patchset/20260819125629.49823-1-gang.yan@linux.dev?part=5
> mptcp_destroy(), the final close path: mptcp_token_destroy() has
> already removed the socket from the token hash by then, so no new
> reader can find the msk anymore.
>
> Note that rcu_assign_pointer() before pm_ops->init(msk) does not
> publish a partially initialised object: the ops are registered
> immutable, init() only prepares the per-socket state, and the msk is
> not reachable by readers until its token is registered.
[Severity: High]
Is this assumption accurate? A concurrent SoftIRQ can receive an MP_JOIN
request, look up the socket via mptcp_token_get_sock(), increment its refcnt,
and drop the RCU read lock. Concurrently, the socket could be disconnected,
calling mptcp_disconnect() -> mptcp_pm_data_reset() -> mptcp_pm_ops_init().
The SoftIRQ thread could then re-acquire the RCU read lock and call
mptcp_pm_get_local_id(), accessing the actively tearing down or uninitialized
per-socket state.
[ ... ]
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 64244a1a01bca..66fb7c3d9c827 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
[ ... ]
> @@ -1158,23 +1166,39 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
> static void mptcp_pm_ops_init(struct mptcp_sock *msk,
> struct mptcp_pm_ops *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;
> + struct mptcp_pm_ops *old;
> +
> + spin_lock_bh(&msk->pm.lock);
> + old = rcu_dereference_protected(msk->pm.ops,
> + lockdep_is_held(&msk->pm.lock));
> + if (old != pm_ops)
> + rcu_assign_pointer(msk->pm.ops, pm_ops);
[Severity: High]
Does this publish the new ops to readers before its state is initialized via
init(msk) below? If a reader finds the socket, it could access the new ops
before its per-socket state is ready.
> + spin_unlock_bh(&msk->pm.lock);
> +
> + if (old) {
> + synchronize_rcu();
> + if (old->release)
> + old->release(msk);
[Severity: High]
If old == pm_ops, we skip the rcu_assign_pointer() above, meaning the ops
are still actively published in msk->pm.ops. Does calling old->release(msk)
here violate RCU removal rules by releasing the ops state while it is still
reachable by readers?
> + bpf_module_put(old, old->owner);
> }
>
> - msk->pm.ops = pm_ops;
> - if (msk->pm.ops->init)
> - msk->pm.ops->init(msk);
> + if (pm_ops->init)
> + pm_ops->init(msk);
>
> pr_debug("pm %s initialized\n", pm_ops->name);
> }
[ ... ]
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 14aa82647c3d1..4953cfa329e81 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);
[Severity: High]
This is where a concurrent reader accesses the PM ops using a cached msk
reference. Does this disprove the assumption in the commit message that
the socket is unreachable, since the reader already holds a reference to msk?
> if (local_id < 0) {
> SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINNOIDFOUND);
> + rcu_read_unlock();
> sock_put((struct sock *)msk);
> return NULL;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790147728.git.yangang@kylinos.cn?part=6
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH mptcp-next v7 6/6] Squash to previous one
2026-09-23 10:11 ` sashiko-bot
@ 2026-09-24 1:53 ` gang.yan
0 siblings, 0 replies; 10+ messages in thread
From: gang.yan @ 2026-09-24 1:53 UTC (permalink / raw)
To: sashiko-reviews; +Cc: mptcp
September 23, 2026 at 6:11 PM, sashiko-bot@kernel.org mailto:sashiko-bot@kernel.org wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] mptcp: uaf and uninitialized access in pm_ops during disconnect
> --
>
> commit 89b80a47ef923bdc04a9069fdcf06de447841c5b
> Author: Gang Yan <yangang@kylinos.cn>
>
> Squash to previous one
>
> This patch applies RCU discipline to msk->pm.ops, moving its initialization,
> publication, and release to better align with RCU patterns. It updates readers
> to use rcu_dereference() and writers to use rcu_assign_pointer() under the
> PM lock.
>
> Link: https://sashiko.dev/#/patchset/20260819125629.49823-1-gang.yan@linux.dev?part=5
>
> >
> > mptcp_destroy(), the final close path: mptcp_token_destroy() has
> > already removed the socket from the token hash by then, so no new
> > reader can find the msk anymore.
> >
> > Note that rcu_assign_pointer() before pm_ops->init(msk) does not
> > publish a partially initialised object: the ops are registered
> > immutable, init() only prepares the per-socket state, and the msk is
> > not reachable by readers until its token is registered.
> >
> [Severity: High]
> Is this assumption accurate? A concurrent SoftIRQ can receive an MP_JOIN
> request, look up the socket via mptcp_token_get_sock(), increment its refcnt,
> and drop the RCU read lock. Concurrently, the socket could be disconnected,
> calling mptcp_disconnect() -> mptcp_pm_data_reset() -> mptcp_pm_ops_init().
>
> The SoftIRQ thread could then re-acquire the RCU read lock and call
> mptcp_pm_get_local_id(), accessing the actively tearing down or uninitialized
> per-socket state.
The concurrent softirq reader you describe does exist -- that is
exactly the race this patch fixes. But note what that reader
consumes: mptcp_pm_get_local_id() consults the per-net endpoint
table (inside its own RCU section) and the msk addresses; it does
not read any of the per-socket state managed by init()/release()
(pm->work_pending, accept_addr, ...), which is only consumed by
the PM worker under the socket lock. So the reader cannot observe
the teardown/reset of the fields those callbacks manage. It's safe
for now.
>
> [ ... ]
>
> >
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index 64244a1a01bca..66fb7c3d9c827 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> >
> [ ... ]
>
> >
> > @@ -1158,23 +1166,39 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
> > static void mptcp_pm_ops_init(struct mptcp_sock *msk,
> > struct mptcp_pm_ops *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;
> > + struct mptcp_pm_ops *old;
> > +
> > + spin_lock_bh(&msk->pm.lock);
> > + old = rcu_dereference_protected(msk->pm.ops,
> > + lockdep_is_held(&msk->pm.lock));
> > + if (old != pm_ops)
> > + rcu_assign_pointer(msk->pm.ops, pm_ops);
> >
> [Severity: High]
> Does this publish the new ops to readers before its state is initialized via
> init(msk) below? If a reader finds the socket, it could access the new ops
> before its per-socket state is ready.
>
On the publish-before-init concern: pm_ops->init(msk) prepares
the per-socket state, not the ops -- the ops are registered
immutable, their callbacks are valid from registration. A reader
entering between the assign and init() would call valid ops code
and read fields it does not consume (see comments above); this
ordering also matches the previous, non-RCU code.
> >
> > + spin_unlock_bh(&msk->pm.lock);
> > +
> > + if (old) {
> > + synchronize_rcu();
> > + if (old->release)
> > + old->release(msk);
> >
> [Severity: High]
> If old == pm_ops, we skip the rcu_assign_pointer() above, meaning the ops
> are still actively published in msk->pm.ops. Does calling old->release(msk)
> here violate RCU removal rules by releasing the ops state while it is still
> reachable by readers?
>
On releasing the still-installed ops when old == pm_ops: no
in-tree PM defines release() today, so this is currently dead
code. For a future PM that does define it, the synchronize_rcu()
in front guarantees no in-flight reader is executing the ops when
release() runs, so no RCU removal rule is violated. It's also an old
issue raised by sashiko before, this code can address it correctly I
think (call .release before remove it).
> >
> > + bpf_module_put(old, old->owner);
> > }
> >
> > - msk->pm.ops = pm_ops;
> > - if (msk->pm.ops->init)
> > - msk->pm.ops->init(msk);
> > + if (pm_ops->init)
> > + pm_ops->init(msk);
> >
> > pr_debug("pm %s initialized\n", pm_ops->name);
> > }
> >
> [ ... ]
>
> >
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 14aa82647c3d1..4953cfa329e81 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);
> >
> [Severity: High]
> This is where a concurrent reader accesses the PM ops using a cached msk
> reference. Does this disprove the assumption in the commit message that
> the socket is unreachable, since the reader already holds a reference to msk?
>
It's the misleading of the commit message, as the first comment of this letter
said.
@Matt, @Geliang
So for now, I think all the patches in this series are ready for review. It’s
not urgent, so please take a look when you have time.
Thanks
Gang
> >
> > if (local_id < 0) {
> > SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINNOIDFOUND);
> > + rcu_read_unlock();
> > sock_put((struct sock *)msk);
> > return NULL;
> > }
> >
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790147728.git.yangang@kylinos.cn?part=6
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mptcp-next v7 0/6] mptcp: avoid data-races around the sysctls
2026-09-23 9:49 [PATCH mptcp-next v7 0/6] mptcp: avoid data-races around the sysctls Gang Yan
` (5 preceding siblings ...)
2026-09-23 9:49 ` [PATCH mptcp-next v7 6/6] Squash to previous one Gang Yan
@ 2026-09-23 10:52 ` MPTCP CI
6 siblings, 0 replies; 10+ messages in thread
From: MPTCP CI @ 2026-09-23 10:52 UTC (permalink / raw)
To: Gang Yan; +Cc: mptcp
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! ✅
- Perf: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/35847364262
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b4ac359f0edd
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1172096
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)
^ permalink raw reply [flat|nested] 10+ messages in thread