add sk_is_msk helper

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

Posted by Geliang Tang 3 days, 22 hours ago

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))
 		return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
 
 	return NULL;
@@ -294,7 +294,7 @@ __bpf_kfunc static bool bpf_sk_stream_memory_free(const struct sock *sk__ign)
 	const struct sock *sk = sk__ign;
 
 	if (sk && sk_fullsock(sk) &&
-	    sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
+	    sk_is_tcp(sk) && sk_is_mptcp(sk))
 		return sk_stream_memory_free(sk);
 
 	return NULL;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index cd5266099993..f418a0a0fa51 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -399,7 +399,7 @@ static inline void msk_owned_by_me(const struct mptcp_sock *msk)
 #undef tcp_sk
 #define tcp_sk(ptr) ({								\
 	typeof(ptr) _ptr = (ptr);						\
-	WARN_ON(_ptr->sk_protocol != IPPROTO_TCP);				\
+	WARN_ON(!sk_is_tcp(_ptr));						\
 	container_of_const(_ptr, struct tcp_sock, inet_conn.icsk_inet.sk);	\
 })
 #define mptcp_sk(ptr) ({						\
-- 
2.51.0

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

Posted by Matthieu Baerts 3 days, 20 hours ago

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?

If not, it might be harder to justify these patches upstream.

>  		return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
>  
>  	return NULL;
> @@ -294,7 +294,7 @@ __bpf_kfunc static bool bpf_sk_stream_memory_free(const struct sock *sk__ign)
>  	const struct sock *sk = sk__ign;
>  
>  	if (sk && sk_fullsock(sk) &&
> -	    sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
> +	    sk_is_tcp(sk) && sk_is_mptcp(sk))
>  		return sk_stream_memory_free(sk);
>  
>  	return NULL;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index cd5266099993..f418a0a0fa51 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -399,7 +399,7 @@ static inline void msk_owned_by_me(const struct mptcp_sock *msk)
>  #undef tcp_sk
>  #define tcp_sk(ptr) ({								\
>  	typeof(ptr) _ptr = (ptr);						\
> -	WARN_ON(_ptr->sk_protocol != IPPROTO_TCP);				\
> +	WARN_ON(!sk_is_tcp(_ptr));						\

I don't think we should change that here. That will potentially cause
issues with kunit, but also the check here is mainly to catch that
'tcp_sk()' is not called with an MPTCP socket. So only the protocol
check is needed.

>  	container_of_const(_ptr, struct tcp_sock, inet_conn.icsk_inet.sk);	\
>  })
>  #define mptcp_sk(ptr) ({						\

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

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

Posted by Geliang Tang 3 days, 20 hours ago

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. 

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

Thanks,
-Geliang

> 
> If not, it might be harder to justify these patches upstream.
> 
> >  		return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
> >  
> >  	return NULL;
> > @@ -294,7 +294,7 @@ __bpf_kfunc static bool
> > bpf_sk_stream_memory_free(const struct sock *sk__ign)
> >  	const struct sock *sk = sk__ign;
> >  
> >  	if (sk && sk_fullsock(sk) &&
> > -	    sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
> > +	    sk_is_tcp(sk) && sk_is_mptcp(sk))
> >  		return sk_stream_memory_free(sk);
> >  
> >  	return NULL;
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index cd5266099993..f418a0a0fa51 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -399,7 +399,7 @@ static inline void msk_owned_by_me(const struct
> > mptcp_sock *msk)
> >  #undef tcp_sk
> >  #define tcp_sk(ptr)
> > ({								\
> >  	typeof(ptr) _ptr =
> > (ptr);						\
> > -	WARN_ON(_ptr->sk_protocol !=
> > IPPROTO_TCP);				\
> > +	WARN_ON(!sk_is_tcp(_ptr));				
> > 		\
> 
> I don't think we should change that here. That will potentially cause
> issues with kunit, but also the check here is mainly to catch that
> 'tcp_sk()' is not called with an MPTCP socket. So only the protocol
> check is needed.
> 
> >  	container_of_const(_ptr, struct tcp_sock,
> > inet_conn.icsk_inet.sk);	\
> >  })
> >  #define mptcp_sk(ptr)
> > ({						\
> 
> Cheers,
> Matt

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

Posted by Matthieu Baerts 3 days, 20 hours ago

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.