Re: [PATCH mptcp-next 1/2] mptcp: use sk_is_tcp helper

[Matthieu Baerts <matttbe@kernel.org>]

On 10/12/2025 11:04, Geliang Tang wrote:
> Hi Matt,
> 
> On Wed, 2025-12-10 at 10:54 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> Thank you for the patches.
>>
>> On 10/12/2025 09:12, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> This patch enhances the validation by using sk_is_tcp to determine
>>> whether
>>> the socket is a TCP socket. This approach is more rigorous than
>>> simply
>>> checking if sk_protocol equals IPPROTO_TCP, as the helper also
>>> verifies
>>> sk_family and sk_type.
>>>
>>> Note: The modifications in the bpf_sk_stream_memory_free section
>>> should be
>>> squashed into the commit "bpf: Export mptcp packet scheduler
>>> helpers".
>>>
>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>> ---
>>>  net/mptcp/bpf.c      | 4 ++--
>>>  net/mptcp/protocol.h | 2 +-
>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
>>> index b261551f5d1b..372a21b96495 100644
>>> --- a/net/mptcp/bpf.c
>>> +++ b/net/mptcp/bpf.c
>>> @@ -193,7 +193,7 @@ static struct bpf_struct_ops
>>> bpf_mptcp_sched_ops = {
>>>  
>>>  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))
>>
>> I know that BPF has a few extra "hidden" checks: are you adding this
>> because you saw issues where the protocol could be set to TCP, but
>> the
>> family and type are not the expected ones?
> 
> No, let's drop this patch then. 

OK!

> How do you think about patch 2? Is sk_is_msk() a good name? I will
> directly include patch 2 in the KTLS series.

I think sk_is_mptcp() would have been a better name, but it is already
used. It might be good to rename sk_is_mptcp() to tp_is_mptcp(), but I
don't know if it is wise to re-use sk_is_mptcp() for another purpose for
the backports... So sk_is_msk() looks good at the end I think.

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