Re: [PATCH mptcp-next v6 5/6] Squash-to "mptcp: pm: init and release mptcp_pm_ops"

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Contradictory claim in the commit message regarding module unloading.
--

commit 954ec9b5378fa18a4b6badb35d44bfbe2edbd12a
Author: Gang Yan <yangang@kylinos.cn>

Squash-to "mptcp: pm: init and release mptcp_pm_ops"

This commit introduces mptcp_pm_ops lifetime handling on sockets and per-net
path managers. It modifies path manager initialization and cleanup routines
to use operations structs directly and take module references.

> This commit introduces the mptcp_pm_ops lifetime handling on sockets
> (mptcp_pm_ops_init/release taking a module reference), and would then
> be the first one whose per-net path managers can be unloaded while a
> pernet still stores them.

[ ... ]

> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> index 87491b961bf27..6379a9f481ac6 100644
> --- a/net/mptcp/ctrl.c
> +++ b/net/mptcp/ctrl.c
[ ... ]
> @@ -224,15 +222,22 @@ static int proc_blackhole_detect_timeout(const struct ctl_table *table,
>  
>  static int mptcp_set_path_manager(struct mptcp_pernet *pernet, const char *name)
>  {
> -	struct mptcp_pm_ops *pm_ops;
> +	struct mptcp_pm_ops *pm_ops, *prev;
>  	int ret = 0;
>  
>  	rcu_read_lock();
>  	pm_ops = mptcp_pm_find(name);
> -	if (pm_ops)
> -		xchg(&pernet->path_manager, pm_ops);
> -	else
> +	if (pm_ops) {
> +		if (bpf_try_module_get(pm_ops, pm_ops->owner)) {
> +			prev = xchg(&pernet->path_manager, pm_ops);

[Severity: Medium]
Does this code contradict the commit message regarding module unloading?

The commit message claims that per-net path managers "can be unloaded while a
pernet still stores them." However, here in mptcp_set_path_manager() (as well
as in mptcp_pernet_set_defaults()), bpf_try_module_get() is explicitly called
before storing the pm_ops pointer in the per-net structure.

This increments the module's reference count, which fundamentally prevents the
module from being unloaded (e.g., via rmmod) as long as the netns uses it.

Could the commit message or the implementation be clarified to resolve this
mismatch?

> +			if (prev)
> +				bpf_module_put(prev, prev->owner);
> +		} else {
> +			ret = -EBUSY;
> +		}
> +	} else {
>  		ret = -ENOENT;
> +	}
>  	rcu_read_unlock();
>  
>  	return ret;

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