Re: [PATCH v2 bpf 3/6] mptcp: bpf: fix type confusion in bpf_mptcp_sock_from_subflow()

Hi Kuniyuki,

(+cc MPTCP ML)

On 05/05/2026 07:43, Kuniyuki Iwashima wrote:
> Could you follow up on the comments by Sashiko ?

Thank you for having forwarded this!

> On Mon, May 4, 2026 at 9:14 PM <[email protected]> wrote:

(...)

>>> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
>>> index 8a16672b94e2..4cc16cbeb328 100644
>>> --- a/net/mptcp/bpf.c
>>> +++ b/net/mptcp/bpf.c
>>> @@ -14,7 +14,7 @@
>>>
>>>  struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
>>>  {
>>> -     if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
>>> +     if (sk && sk_fullsock(sk) && sk_is_tcp(sk) && sk_is_mptcp(sk))
>>>               return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
>>
>> Could this cause a regression by dereferencing a NULL pointer when accessed
>> concurrently?
>>
>> In subflow_ulp_fallback(), icsk->icsk_ulp_data is set to NULL prior to
>> clearing tcp_sk(sk)->is_mptcp. If a BPF program triggers in this window,
>> sk_is_mptcp() would return true but mptcp_subflow_ctx(sk) would return NULL,
>> leading to a NULL pointer dereference.

This would need to be confirmed, but from what I see, that's not a new
bug introduced by the patch here.

>> Additionally, could ctx->conn be NULL when read here?
>>
>> During subflow_ulp_init(), the context is allocated and is_mptcp is set to 1,
>> but ctx->conn is initially NULL. Passing NULL to mptcp_sk(ctx->conn) triggers
>> a kernel panic on CONFIG_DEBUG_NET builds. On non-debug builds, it returns a
>> non-NULL offset pointer, which bypasses BPF verifier NULL checks and causes
>> invalid memory accesses.

Same here: not a new bug.

>> Could this introduce a regression with a use-after-free of the mptcp_sock
>> during teardown?
>>
>> During subflow_ulp_release(), sock_put(ctx->conn) drops the reference to the
>> parent mptcp_sock and may free it. However, ctx->conn is not cleared, and the
>> subflow context remains valid until the RCU grace period ends.
>>
>> If a BPF tracing program triggers after sock_put(), could this retrieve
>> and return the dangling ctx->conn pointer, enabling the BPF program to read
>> from freed kernel slab memory?
Same here: not a new bug.

Sashiko is good at finding potential existing bugs that predate a patch,
which is good, but someone has to fix them :)

Because the issues raised here are not new, I suggest not blocking this
patch. I will check if someone can look at it quickly.

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