Re: [PATCH mptcp-next v2 1/5] mptcp: sched: change scheduler sysctl atomically

August 18, 2026 at 5:59 PM, [email protected] mailto:[email protected]  wrote:


> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Use-After-Free and module reference leak due to missing reference counting when setting the default MPTCP scheduler
> - [Medium] Redundant scheduler list traversal during socket initialization defeats pointer caching
> --
> 
> --- Patch [1]: [PATCH mptcp-next v2 1/5] mptcp: sched: change scheduler sysctl atomically ---
> commit d5b1780c033b9909a1b30fbd814812af5c25ed2f
> Author: Gang Yan <[email protected]>
> 
> mptcp: sched: change scheduler sysctl atomically
> 
> This commit changes the per-netns scheduler storage from an inline string
> to an RCU-protected pointer to an mptcp_sched_ops struct. It updates the
> sysctl read/write paths and socket initialization to use this pointer,
> aiming to prevent concurrent readers from observing half-written names.
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/626
> 
> > 
> > diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> >  index 63c5747f0f63c..479b31eb30079 100644
> >  --- a/net/mptcp/ctrl.c
> >  +++ b/net/mptcp/ctrl.c
> > 
> [ ... ]
> 
> > 
> > @@ -126,7 +133,7 @@ static int mptcp_set_scheduler(char *scheduler, const char *name)
> >  rcu_read_lock();
> >  sched = mptcp_sched_find(name);
> >  if (sched)
> >  - strscpy(scheduler, name, MPTCP_SCHED_NAME_MAX);
> >  + xchg(&pernet->scheduler, sched);
> > 
> [Severity: High]
> When replacing the scheduler pointer here with xchg(), does this leak the
> module reference for the previous scheduler returned by xchg()?
> 
> Also, do we need to acquire a module reference for the newly assigned sched
> via bpf_try_module_get() before storing it? Without a reference, could the
> module be unloaded, leaving pernet->scheduler pointing to freed memory that
> would trigger a use-after-free when dereferenced by a concurrent reader in
> proc_scheduler()?

Hi Matt,

As far as I know, for linux-master branch, the sched ops cannot be torndown, and
the only static default scheduler is used, so no reference counting is needed.

It is safe for now, the pm is as the same. Right?

> 
> [ ... ]
> 
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> >  index c2762d74f29d7..82df0400d0f94 100644
> >  --- a/net/mptcp/protocol.c
> >  +++ b/net/mptcp/protocol.c
> > 
> [ ... ]
> 
> > 
> > @@ -3271,8 +3272,8 @@ 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)));
> >  + mptcp_get_scheduler(net, sched_name);
> >  + ret = mptcp_init_sched(mptcp_sk(sk), mptcp_sched_find(sched_name));
> > 
> [Severity: Medium]
> Since pernet->scheduler now stores the mptcp_sched_ops pointer directly,
> could we retrieve and use that pointer here directly? Extracting the name
> via mptcp_get_scheduler() just to perform a redundant list traversal with
> mptcp_sched_find() seems to defeat the purpose of caching the operations
> pointer.
>

That's a good point, will done in v3.

Thanks
Gang
 
> > 
> > rcu_read_unlock();
> >  if (ret)
> >  return ret;
> > 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
>