From: Geliang Tang <tanggeliang@kylinos.cn>

sk_is_mptcp() looks like it can be called using any struct sock *, but
it only gives valid results with struct tcp_sock *. This patch adds a
sk_is_tcp() check for it.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/453
Suggested-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 63972d712a1d..30754bebd5fb 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -124,7 +124,7 @@ void mptcp_init(void);
 
 static inline bool sk_is_mptcp(const struct sock *sk)
 {
-	return tcp_sk(sk)->is_mptcp;
+	return sk_is_tcp(sk) ? tcp_sk(sk)->is_mptcp : false;
 }
 
 static inline bool rsk_is_mptcp(const struct request_sock *req)
-- 
2.40.1

On Mon, 5 Feb 2024, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> sk_is_mptcp() looks like it can be called using any struct sock *, but
> it only gives valid results with struct tcp_sock *. This patch adds a
> sk_is_tcp() check for it.
>

Hi Geliang -

This is exactly what I asked for in #453, however in our meeting last week 
Paolo was concerned about the overhead of the extra checks when most of 
the call sites are known to be tcp_socks (especially in the TCP code).

Paolo, what do you think about only adding the sk_is_tcp() check when 
CONFIG_DEBUG_NET is enabled? (kind of like Matthieu's "check the protocol 
with DEBUG_NET" series)


- Mat


> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/453
> Suggested-by: Mat Martineau <martineau@kernel.org>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> include/net/mptcp.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 63972d712a1d..30754bebd5fb 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -124,7 +124,7 @@ void mptcp_init(void);
>
> static inline bool sk_is_mptcp(const struct sock *sk)
> {
> -	return tcp_sk(sk)->is_mptcp;
> +	return sk_is_tcp(sk) ? tcp_sk(sk)->is_mptcp : false;
> }
>
> static inline bool rsk_is_mptcp(const struct request_sock *req)
> -- 
> 2.40.1
>
>
>

On Tue, Feb 13, 2024 at 5:52 PM Mat Martineau <martineau@kernel.org> wrote:
>
> On Mon, 5 Feb 2024, Geliang Tang wrote:
>
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> >
> > sk_is_mptcp() looks like it can be called using any struct sock *, but
> > it only gives valid results with struct tcp_sock *. This patch adds a
> > sk_is_tcp() check for it.
> >
>
> Hi Geliang -
>
> This is exactly what I asked for in #453, however in our meeting last week
> Paolo was concerned about the overhead of the extra checks when most of
> the call sites are known to be tcp_socks (especially in the TCP code).
>
> Paolo, what do you think about only adding the sk_is_tcp() check when
> CONFIG_DEBUG_NET is enabled? (kind of like Matthieu's "check the protocol
> with DEBUG_NET" series)
>

Ok, we talked about it in the meeting today and Matthieu's "mptcp:
check the protocol in tcp_sk() with DEBUG_NET" patch will cover the
possible misuse of this function in the mptcp code (where the main
concern is confusion between MPTCP and subflow sockets). Will drop
this patch and update the github issue.

- Mat