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

[Matthieu Baerts <matttbe@kernel.org>]

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.