:p
atchew
Login
Recently, Paolo fixed a bug where a TCP-specific helper was used with an MPTCP socket [1]. The bug was not detected by fuzzer or static analysis. Following this, it has been suggested to add a check, only in debug mode. This is what this series is doing. The series has been split to be upstreamed: a preparation patch for MPTCP, the modification for TCP, then for MPTCP. It is not clear if it would be OK to add that upstream. If not, we can squash these three patches in "DO-NOT-MERGE: mptcp: improve code coverage for CI" commit we have in our export tree. Note that the MPTCP Token kUnit test needs to be adapted for this new check. This is what is done in patch 1/3. Currently, 'WARN_ON()' helper is used. Maybe we want to use the '_ONCE' version? I didn't use it because this is a DEBUG check, but I can if we prefer. Or something similar. Link: https://lore.kernel.org/mptcp/35875ef9cb7194563b580e14c71cc8cb065f846c.1706043786.git.pabeni@redhat.com/ [1] Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Matthieu Baerts (NGI0) (3): mptcp: token kunit: set protocol tcp: check the protocol with DEBUG_NET mptcp: check the protocol with DEBUG_NET include/linux/tcp.h | 9 +++++++++ net/mptcp/protocol.h | 9 +++++++++ net/mptcp/token_test.c | 7 ++++++- 3 files changed, 24 insertions(+), 1 deletion(-) --- base-commit: 12837771007b672717901b90aa9eb97a0a8c31fa change-id: 20240131-mptcp-check-protocol-e32e53d04a75 Best regards, -- Matthieu Baerts (NGI0) <matttbe@kernel.org>
As it would be done when initiating an MPTCP sock. This is not strictly needed for this test, but it will be when a later patch will check if the right protocol is being used when calling mptcp_sk(). Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- net/mptcp/token_test.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/net/mptcp/token_test.c b/net/mptcp/token_test.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/token_test.c +++ b/net/mptcp/token_test.c @@ -XXX,XX +XXX,XX @@ static struct mptcp_subflow_context *build_ctx(struct kunit *test) static struct mptcp_sock *build_msk(struct kunit *test) { struct mptcp_sock *msk; + struct sock *sk; msk = kunit_kzalloc(test, sizeof(struct mptcp_sock), GFP_USER); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, msk); refcount_set(&((struct sock *)msk)->sk_refcnt, 1); sock_net_set((struct sock *)msk, &init_net); + sk = (struct sock *)msk; + /* be sure the token helpers can dereference sk->sk_prot */ - ((struct sock *)msk)->sk_prot = &tcp_prot; + sk->sk_prot = &tcp_prot; + sk->sk_protocol = IPPROTO_MPTCP; + return msk; } -- 2.43.0
Fuzzers and static checkers might not detect when tcp_sk() is used with a non tcp_sock structure. This kind of mistake already happened a few times with MPTCP, when wrongly using TCP-specific helpers with mptcp_sock pointers. On the other hand, there are many 'tcp_xxx()' helpers that are taking a 'struct sock' as arguments, and some of them are only looking at fields from 'struct sock', and nothing from 'struct tcp_sock'. It is then tempting to use them with a 'struct mptcp_sock'. So a new simple check is done when CONFIG_DEBUG_NET is enabled. tcp_sk() is then used as an inlined function, like before commit e9d9da91548b ("tcp: preserve const qualifier in tcp_sk()"). Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- include/linux/tcp.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/include/linux/tcp.h b/include/linux/tcp.h index XXXXXXX..XXXXXXX 100644 --- a/include/linux/tcp.h +++ b/include/linux/tcp.h @@ -XXX,XX +XXX,XX @@ enum tsq_flags { TCPF_ACK_DEFERRED = BIT(TCP_ACK_DEFERRED), }; +#ifdef CONFIG_DEBUG_NET +static inline struct tcp_sock *tcp_sk(const struct sock *sk) +{ + WARN_ON(sk->sk_protocol != IPPROTO_TCP); + + return (struct tcp_sock *)sk; +} +#else #define tcp_sk(ptr) container_of_const(ptr, struct tcp_sock, inet_conn.icsk_inet.sk) +#endif /* Variant of tcp_sk() upgrading a const sock to a read/write tcp socket. * Used in context of (lockless) tcp listeners. -- 2.43.0
Fuzzers and static checkers might not detect when mptcp_sk() is used with a non mptcp_sock structure. This is similar to the parent commit, where it is easy to use mptcp_sk() with a TCP sock, e.g. with a subflow sk. So a new simple check is done when CONFIG_DEBUG_NET is enabled. mptcp_sk() is then used as an inlined function, like before commit 403a40f2304d ("mptcp: preserve const qualifier in mptcp_sk()"). Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- net/mptcp/protocol.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -XXX,XX +XXX,XX @@ static inline void msk_owned_by_me(const struct mptcp_sock *msk) sock_owned_by_me((const struct sock *)msk); } +#ifdef CONFIG_DEBUG_NET +static inline struct mptcp_sock *mptcp_sk(const struct sock *sk) +{ + WARN_ON(sk->sk_protocol != IPPROTO_MPTCP); + + return (struct mptcp_sock *)sk; +} +#else #define mptcp_sk(ptr) container_of_const(ptr, struct mptcp_sock, sk.icsk_inet.sk) +#endif /* the msk socket don't use the backlog, also account for the bulk * free memory -- 2.43.0
Recently, Paolo fixed a bug where a TCP-specific helper was used with an MPTCP socket [1]. The bug was not detected by fuzzer or static analysis. Following this, it has been suggested to add a check, only in debug mode. This is what this series is doing. The series has been split to be upstreamed: a preparation patch for MPTCP, the modification for TCP, then for MPTCP. It is not clear if it would be OK to add that upstream. If not, we can squash these three patches in "DO-NOT-MERGE: mptcp: improve code coverage for CI" commit we have in our export tree. Note that the MPTCP Token kUnit test needs to be adapted for this new check. This is what is done in patch 1/3. Link: https://lore.kernel.org/mptcp/35875ef9cb7194563b580e14c71cc8cb065f846c.1706043786.git.pabeni@redhat.com/ [1] Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Changes in v2: - Patch 2 and 3 have been modified, please the changelog on each patch - Link to v1: https://lore.kernel.org/r/20240131-mptcp-check-protocol-v1-0-a06067f0bd08@kernel.org --- Matthieu Baerts (NGI0) (3): mptcp: token kunit: set protocol mptcp: check the protocol in tcp_sk() with DEBUG_NET mptcp: check the protocol in mptcp_sk() with DEBUG_NET net/mptcp/protocol.h | 14 ++++++++++++++ net/mptcp/token_test.c | 7 ++++++- 2 files changed, 20 insertions(+), 1 deletion(-) --- base-commit: 12837771007b672717901b90aa9eb97a0a8c31fa change-id: 20240131-mptcp-check-protocol-e32e53d04a75 Best regards, -- Matthieu Baerts (NGI0) <matttbe@kernel.org>
As it would be done when initiating an MPTCP sock. This is not strictly needed for this test, but it will be when a later patch will check if the right protocol is being used when calling mptcp_sk(). Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- net/mptcp/token_test.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/net/mptcp/token_test.c b/net/mptcp/token_test.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/token_test.c +++ b/net/mptcp/token_test.c @@ -XXX,XX +XXX,XX @@ static struct mptcp_subflow_context *build_ctx(struct kunit *test) static struct mptcp_sock *build_msk(struct kunit *test) { struct mptcp_sock *msk; + struct sock *sk; msk = kunit_kzalloc(test, sizeof(struct mptcp_sock), GFP_USER); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, msk); refcount_set(&((struct sock *)msk)->sk_refcnt, 1); sock_net_set((struct sock *)msk, &init_net); + sk = (struct sock *)msk; + /* be sure the token helpers can dereference sk->sk_prot */ - ((struct sock *)msk)->sk_prot = &tcp_prot; + sk->sk_prot = &tcp_prot; + sk->sk_protocol = IPPROTO_MPTCP; + return msk; } -- 2.43.0
Fuzzers and static checkers might not detect when tcp_sk() is used with a non tcp_sock structure. This kind of mistake already happened a few times with MPTCP: when wrongly using TCP-specific helpers with mptcp_sock pointers. On the other hand, there are many 'tcp_xxx()' helpers that are taking a 'struct sock' pointer as arguments, and some of them are only looking at fields from 'struct sock', and nothing from 'struct tcp_sock'. It is then tempting to use them with a 'struct mptcp_sock'. So a new simple check is done when CONFIG_DEBUG_NET is enabled to tell kernel devs when a non-TCP socket is being used as a TCP one. 'tcp_sk()' macro is then re-defined to add a WARN when an unexpected socket is being used. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Notes: - v2: - Move from include/linux/tcp.h to net/mptcp/protocol.h: specific to TCP (Paolo) - Use a macro instead of an inlined function (Paolo) - Adapt the commit message after the recent changes. --- net/mptcp/protocol.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -XXX,XX +XXX,XX @@ static inline void msk_owned_by_me(const struct mptcp_sock *msk) sock_owned_by_me((const struct sock *)msk); } +#ifdef CONFIG_DEBUG_NET +/* MPTCP-specific: we might (indirectly) call this helper with the wrong sk */ +#undef tcp_sk +#define tcp_sk(ptr) ({ \ + WARN_ON(ptr->sk_protocol != IPPROTO_TCP); \ + container_of_const(ptr, struct tcp_sock, inet_conn.icsk_inet.sk); \ +}) +#endif + #define mptcp_sk(ptr) container_of_const(ptr, struct mptcp_sock, sk.icsk_inet.sk) /* the msk socket don't use the backlog, also account for the bulk -- 2.43.0
Fuzzers and static checkers might not detect when mptcp_sk() is used with a non mptcp_sock structure. This is similar to the parent commit, where it is easy to use mptcp_sk() with a TCP sock, e.g. with a subflow sk. So a new simple check is done when CONFIG_DEBUG_NET is enabled to tell kernel devs when a non-MPTCP socket is being used as an MPTCP one. 'mptcp_sk()' macro is then defined differently: with an extra WARN to complain when an unexpected socket is being used. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Notes: v2: - Use a macro instead of an inlined function (Paolo) --- net/mptcp/protocol.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -XXX,XX +XXX,XX @@ static inline void msk_owned_by_me(const struct mptcp_sock *msk) WARN_ON(ptr->sk_protocol != IPPROTO_TCP); \ container_of_const(ptr, struct tcp_sock, inet_conn.icsk_inet.sk); \ }) -#endif +#define mptcp_sk(ptr) ({ \ + WARN_ON(ptr->sk_protocol != IPPROTO_MPTCP); \ + container_of_const(ptr, struct mptcp_sock, sk.icsk_inet.sk); \ +}) +#else /* !CONFIG_DEBUG_NET */ #define mptcp_sk(ptr) container_of_const(ptr, struct mptcp_sock, sk.icsk_inet.sk) +#endif /* the msk socket don't use the backlog, also account for the bulk * free memory -- 2.43.0