Re: [PATCH mptcp-next v4 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:
- [High] Use-After-Free of PM ops pointer due to premature RCU unlock.
- [High] Resource Leak and Lifecycle Violation in PM ops replacement.
- [High] Use-Before-Initialization of PM ops via RCU publishing.
--

--- Patch [6]: [PATCH mptcp-next v4 6/6] Squash to previous one ---
commit 300bc1205800c2307a03b20ff740b6c4f84c422e
Author: Gang Yan <yangang@kylinos.cn>

Squash to previous one

This commit applies RCU discipline to the msk->pm.ops pointer, marking
it __rcu and adding appropriate locks and synchronization.

Link: https://sashiko.dev/#/patchset/20260819125629.49823-1-gang.yan@linux.dev?part=5

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c

[ ... ]

> @@ -1158,23 +1166,46 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  static void mptcp_pm_ops_init(struct mptcp_sock *msk,
>  			      struct mptcp_pm_ops *pm_ops)
>  {
> +	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?

> +		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.

Also, if old == pm_ops, we still reach pm_ops->init(msk) here. Will
unconditionally calling init() on unchanged ops cause 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?

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