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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] BPF Path Manager per-socket resources leak on socket disconnect or PM swap due to omitted `release()` callback.
--

commit 2b82d535eb907aa1641a5d2cebbd47571b430146
Author: Gang Yan <yangang@kylinos.cn>
Subject: Squash to previous one

This commit updates the MPTCP Path Manager operations to use RCU
protection for the pm.ops pointer. It modifies the assignment,
dereferencing, and cleanup paths to follow RCU reader/writer disciplines
and handle concurrent socket operations.

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

> Note also that the swap path does not call old->release() and that
> init() runs again when the ops is unchanged: the per-socket PM
> resources are already freed by mptcp_pm_destroy() before the reset
> swaps the ops, and mptcp_pm_kernel_init() only re-sets flags from
> the current sysctl, it does not allocate.

[Severity: High]
Does this assumption hold true for custom BPF-based path managers?

While mptcp_pm_destroy() cleans up the in-tree kernel PM resources, it
would not free custom BPF map allocations if the PM's release() callback is
skipped.

> 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,41 @@ 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 (need_sync)
> +		synchronize_rcu();
> +	if (old)
> +		bpf_module_put(old, old->owner);
> +
> +	if (pm_ops->init)
> +		pm_ops->init(msk);

[Severity: High]
Since the swap path skips calling old->release(), can this cause an
unbounded memory leak if a socket is repeatedly disconnected and
reconnected?

An unprivileged local user can repeatedly call connect(AF_UNSPEC) followed
by connect() on an MPTCP socket. This triggers mptcp_pm_data_reset() which
calls mptcp_pm_ops_init() again. If a custom BPF PM allocates resources in
its init() callback, those resources will leak each time the socket is
disconnected and reinitialized.

>  	pr_debug("pm %s initialized\n", pm_ops->name);
>  }

[ ... ]

> @@ -1195,8 +1221,6 @@ void mptcp_pm_destroy(struct mptcp_sock *msk)
>  	 * can be reused (mptcp_disconnect()) and re-selected to a different PM
>  	 */
>  	mptcp_userspace_pm_free_local_addr_list(msk);
> -
> -	mptcp_pm_ops_release(msk);
>  }

[Severity: High]
By removing the mptcp_pm_ops_release() call from mptcp_pm_destroy() in
net/mptcp/pm.c, does this prevent custom BPF path managers from freeing
their per-socket resources when an MPTCP socket is disconnected?

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