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

[gang.yan@linux.dev]

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
>