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

[gang.yan@linux.dev]

September 4, 2026 at 5:51 PM, sashiko-bot@kernel.org mailto:sashiko-bot@kernel.org  wrote:


> 
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Bypassing RCU grace period in mptcp_pm_ops_init() when the PM remains unchanged causes Use-After-Free and data races.
> - [High] Publishing new PM ops before initializing its per-socket state exposes uninitialized memory to RCU readers.
> 
> Pre-existing issues:
> - [High] Module reference count underflow when falling back to the default path manager.
> --
> 
> commit 261318f67b36cf52f532df8226134e77ddd65cd0
> Author: Gang Yan <yangang@kylinos.cn>
> 
> Squash to previous one
> 
> This patch updates the MPTCP path manager operations to use RCU for concurrent
> reads. It modifies mptcp_pm_ops_init() and mptcp_pm_ops_release() to safely
> handle the assignment and release of the ops pointer, and adds RCU read locks
> in the subflow paths.
> 
> > 
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >  index 64244a1a01bca..d581d11b350f2 100644
> >  --- a/net/mptcp/pm.c
> >  +++ b/net/mptcp/pm.c
> > 
> [ ... ]
> 
> > 
> > @@ -1158,23 +1166,44 @@ 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;
> >  + bool need_sync = false;
> >  +
> >  + 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 code publish the new PM operations pointer to readers before its
> per-socket state is initialized?
> 
> If msk->pm.ops is updated via rcu_assign_pointer() here, existing RCU readers
> like subflow_token_join_request() could dereference the new pm_ops and execute
> its methods before pm_ops->init(msk) has run below. For example, if a reader
> calls mptcp_pm_get_local_id() and then mptcp_pm_is_backup() in the same RCU
> section, it might see the new uninitialized pm_ops for the second call.
> 
> > 
> > + need_sync = !!old;
> >  }
> >  + spin_unlock_bh(&msk->pm.lock);
> >  
> >  - msk->pm.ops = pm_ops;
> >  - if (msk->pm.ops->init)
> >  - msk->pm.ops->init(msk);
> >  + if (need_sync)
> >  + synchronize_rcu();
> >  + if (old) {
> >  + if (old->release)
> >  + old->release(msk);
> > 
> [Severity: High]
> Could this bypass the required RCU grace period and cause a use-after-free?
> 
> If old == pm_ops, need_sync is set to false, skipping synchronize_rcu().
> However, the code still calls old->release(msk) immediately here. If
> concurrent lockless RCU readers (e.g., in subflow_token_join_request())
> are actively executing path manager methods on the socket, old->release(msk)
> can free the per-socket state while they are using it.
> 
> > 
> > + bpf_module_put(old, old->owner);
> >  + }
> >  +
> >  + if (pm_ops->init)
> >  + pm_ops->init(msk);
> >  
> >  pr_debug("pm %s initialized\n", pm_ops->name);
> >  }
> > 
> [ ... ]
> 
> > 
> > @@ -1211,8 +1239,15 @@ 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;
> > 
> [Severity: High]
> This is a pre-existing issue, but could this fallback cause a module reference
> count underflow?
> 
> If bpf_try_module_get() fails or the requested PM is NULL, pm_ops is set to
> &mptcp_pm_kernel without incrementing its module reference count.
> 
> Later, mptcp_pm_ops_release() unconditionally calls
> bpf_module_put(pm_ops, pm_ops->owner). Since the reference was never acquired,
> this causes an underflow of the MPTCP module's reference count. If MPTCP is
> built as a module, this could allow the module to be unloaded while sockets
> are still alive.
> 
> > 
> > + }
> >  rcu_read_unlock();
> >  +
> >  + mptcp_pm_ops_init(msk, pm_ops);
> >  }

Will be handled in the next version.

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