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

Hi Gang,

On 20/08/2026 08:36, [email protected] wrote:
> August 19, 2026 at 9:12 PM, [email protected] mailto:[email protected]  wrote:
> 
>> [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?
>>
> 
> Hi Matt, Geliang
> 
> @Matt, Could you help me to trigger the CI?

It looks like GitHub was sick that day. I reset the tag manually.

> And I think it is a pre-existing bug: 
> 
> When it calls msk->pm.ops->get_local_id(msk, &skc_local) between
> 'mptcp_distroy_common' (which calls mptcp_pm_ops_release) and
> 'mptcp_pm_data_reset'. The pm.ops will be NULL and cause a NULL deref, right?

Can you first check if this can happen? I thought that
mptcp_destroy_common would first close all subflows, remove the token,
then release everything linked to the PM → so the PM will no longer be
called at that point for this msk, right? (I didn't check)

But maybe this will change when the BPF PM will be fully implemented? I
guess no because msk->pm.ops should only be called from events linked to
the network, so not after mptcp_destroy_common?

> If I'm right, I think we can using rcu to solve this based on this series, like:
> 
>   - Add a helper to get pm.ops, and then call the get_local_id/get_priority under rcu_lock:
> '''
> +static struct mptcp_pm_ops *mptcp_pm_deref(struct mptcp_sock *msk)
> +{
> +       struct mptcp_pm_ops *pm_ops;
> +
> +       pm_ops = rcu_dereference(msk->pm.ops);
> +       return pm_ops ? pm_ops : &mptcp_pm_kernel;
> +}
> +
> 
> bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
>  
>         mptcp_local_address((struct sock_common *)skc, &skc_local);
>  
> -       return msk->pm.ops->get_priority(msk, &skc_local);
> +       return mptcp_pm_deref(msk)->get_priority(msk, &skc_local);
>  }
>  
> So does the mptcp_pm_get_local_id like this.

I think you should then rename the helpers, to make it clear they need
to be used from a RCU read section, and to get a warning when
backporting code around that.

> static struct mptcp_sock *subflow_token_join_request(struct request_sock *req)
>                 return NULL;
>         }
>  
> +       rcu_read_lock();
>         local_id = mptcp_pm_get_local_id(msk, (struct sock_common *)req);
>         if (local_id < 0) {
>                 SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINNOIDFOUND);
> +               rcu_read_unlock();
>                 sock_put((struct sock *)msk);
>                 return NULL;
>         }
>         subflow_req->local_id = local_id;
>         subflow_req->request_bkup = mptcp_pm_is_backup(msk, (struct sock_common *)req);
> +       rcu_read_unlock();
>  
> '''
> 
>   - Refactor the mptcp_pm_ops_init to support reuse socket, and put the
>     mptcp_pm_ops_release into mptcp_destroy:
> '''
>  static void mptcp_pm_ops_init(struct mptcp_sock *msk,
>                               struct mptcp_pm_ops *pm_ops)
>  {
> +       struct mptcp_pm_ops *old = msk->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;
>         }
>  
> -       msk->pm.ops = pm_ops;
> +       if (old) {
> +               if (old == pm_ops) {
> +                       mptcp_pm_ops_release(msk);

Maybe return here, no need to re-init, right?

> +               } else {
> +                       rcu_assign_pointer(msk->pm.ops, pm_ops);
> +                       synchronize_rcu();
> +                       bpf_module_put(old, old->owner);
> +               }
> +       } else {
> +               rcu_assign_pointer(msk->pm.ops, pm_ops);
> +       }
> +
>         if (msk->pm.ops->init)
>                 msk->pm.ops->init(msk);
>  
> -       pr_debug("pm %s initialized\n", pm_ops->name);
> +       pr_debug("pm %s initialized\n", msk->pm.ops->name);
>  }
>  
> 
> 
> static void mptcp_destroy(struct sock *sk)
>         /* allow the following to close even the initial subflow */
>         msk->free_first = 1;
>         mptcp_destroy_common(msk);
> +       mptcp_pm_ops_release(msk);
>         sk_sockets_allocated_dec(sk);
>  }
> 
> '''
> 
> But I think it is a large fix, do you have any idea?
> 
> And as I said at the beginning, it seeems like not a issue attached to this issue,
> could you review the v3 code with ignoring it ?

Yes, the 4 first patches can be reviewed.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.