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

[sashiko-bot@kernel.org]

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);
>  }

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