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

[gang.yan@linux.dev]

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
>