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

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?

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?

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.

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);
+               } 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 ?

Thanks
Gang


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