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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] msk->pm.ops is accessed concurrently without the socket lock or RCU protection during MP_JOIN processing, which can race with mptcp_disconnect() leading to a NULL pointer dereference or execution of freed module code.
--

--- Patch [5]: [PATCH mptcp-next v3 5/5] Squash to "mptcp: pm: init and release mptcp_pm_ops" ---
commit f7c7fb9e99182263f4dea26b6331ed438faaf974
Author: Gang Yan <[email protected]>

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

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/pm.c b/net/mptcp/pm.c
> index 69a38cb48977a..64244a1a01bca 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -1155,13 +1155,11 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  	spin_unlock_bh(&msk->pm.lock);
>  }
>  
> -static void mptcp_pm_ops_init(struct mptcp_sock *msk, const char *pm_name)
> +static void mptcp_pm_ops_init(struct mptcp_sock *msk,
> +			      struct mptcp_pm_ops *pm_ops)
>  {

[Severity: Critical]
Since this series introduces mptcp_pm_ops lifetime handling and module
references, is there a race condition with concurrent MP_JOIN requests?

When an incoming MP_JOIN request is processed in softirq context,
subflow_token_join_request() gets the socket via mptcp_token_get_sock(),
which doesn't hold lock_sock() or RCU. It then calls:

net/mptcp/pm.c:mptcp_pm_get_local_id() {
    ...
    return msk->pm.ops->get_local_id(msk, &skc_local);
}

Concurrently, if a local user closes the socket and triggers
mptcp_disconnect(msk), it calls mptcp_pm_ops_release() which clears the ops
and drops the module reference:

net/mptcp/pm.c:mptcp_pm_ops_release() {
    ...
    msk->pm.ops = NULL;
    ...
    bpf_module_put(pm_ops, pm_ops->owner);
}

Could this lead to a NULL pointer dereference or executing freed module code
if subflow_token_join_request() executes concurrently with a socket
disconnect?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5