Re: [PATCH mptcp-next v2 2/5] mptcp: pm: change path_manager sysctl atomically

August 18, 2026 at 6:00 PM, [email protected] mailto:[email protected]  wrote:


> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Missing module reference counting when setting the path manager sysctl leads to a Use-After-Free.
> - [High] Missing module reference counting when setting the scheduler sysctl leads to a Use-After-Free.
> --

Hi, Matt

As described in [1], it is safe for now.

Do you have some suggestions here? Or we can ignore these.

Thanks
Gang

> 
> --- Patch [2]: [PATCH mptcp-next v2 2/5] mptcp: pm: change path_manager sysctl atomically ---
> commit c42ca00640178ef2df1ab9f51d1f65a9b722ec86
> Author: Gang Yan <[email protected]>
> 
> mptcp: pm: change path_manager sysctl atomically
> 
> 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. This patch replaces the string with a
> pointer to the immutable struct mptcp_pm_ops.
> 
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/626
> 
> > 
> > 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:
> > 
> [Severity: High]
> Since this patch follows the scheduler change in the previous patch, does
> mptcp_set_scheduler() have the same module reference counting issue?
> 
> If mptcp_set_scheduler() assigns pernet->scheduler without acquiring a
> module reference, and the BPF map is unloaded, could a new MPTCP socket
> creation trigger mptcp_init_sock() which calls mptcp_get_scheduler(),
> causing a Use-After-Free when it dereferences the freed sched pointer?
> 
> > 
> > diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> >  index 479b31eb30079..c0481b09c1a17 100644
> >  --- a/net/mptcp/ctrl.c
> >  +++ b/net/mptcp/ctrl.c
> > 
> [ ... ]
> 
> > 
> > @@ -85,9 +85,14 @@ 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)
> >  +void mptcp_get_path_manager(const struct net *net, char *name)
> >  {
> >  - return mptcp_get_pernet(net)->path_manager;
> >  + struct mptcp_pm_ops *pm_ops;
> >  +
> >  + rcu_read_lock();
> >  + pm_ops = rcu_dereference(mptcp_get_pernet(net)->path_manager);
> >  + strscpy(name, pm_ops ? pm_ops->name : "kernel", MPTCP_PM_NAME_MAX);
> > 
> [Severity: High]
> If the module was unloaded and the pointer was not pinned, would this
> dereference of pm_ops->name result in a Use-After-Free?
> 
> > 
> > + rcu_read_unlock();
> >  }
> > 
> [ ... ]
> 
> > 
> > @@ -201,7 +206,7 @@ static int proc_blackhole_detect_timeout(const struct ctl_table *table,
> > 
> [ ... ]
> 
> > 
> > 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);
> > 
> [Severity: High]
> Does this assignment need to pin the module (e.g., via bpf_try_module_get())
> before saving the pointer?
> 
> If a user with CAP_NET_ADMIN sets a BPF struct_ops map as the default path
> manager, and then unloads the BPF program, the map is deleted and the
> struct mptcp_pm_ops memory is freed.
> 
> Could this leave a dangling pointer in pernet->path_manager that causes the
> Use-After-Free mentioned above when mptcp_get_path_manager() is subsequently
> called?
> 
> > 
> > else
> >  ret = -ENOENT;
> >  rcu_read_unlock();
> > 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
>