tcp: check mptcp-level constraints for backlog coalescing

[PATCH mptcp-net v2] tcp: check mptcp-level constraints for backlog coalescing

Posted by Paolo Abeni 6 months, 2 weeks ago

The MPTCP protocol can acquire the subflow-level socket lock and
cause the tcp backlog usage. When inserting new skbs into the
backlog, the stack will try to coalesce them.

Currently, we have no check in place to ensure that such coalescing
will respect the MPTCP-level DSS, and that may cause data stream
corruption, as reported by Christoph.

Address the issue by adding the relevant admission check for coalescing
in tcp_add_backlog().

Note the issue is not easy to reproduce, as the MPTCP protocol tries
hard to avoid acquiring the subflow-level socket lock.

Fixes: 648ef4b88673 ("mptcp: Implement MPTCP receive path")
Reported-by: Christoph Paasch <cpaasch@apple.com>
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/420
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
  - !coalesce (mat)
  - typo in commit message (mat)
---
 net/ipv4/tcp_ipv4.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index a441740616d7..4d66a8ab3b98 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1870,6 +1870,7 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb,
 #ifdef CONFIG_TLS_DEVICE
 	    tail->decrypted != skb->decrypted ||
 #endif
+	    !mptcp_skb_can_collapse(tail, skb) ||
 	    thtail->doff != th->doff ||
 	    memcmp(thtail + 1, th + 1, hdrlen - sizeof(*th)))
 		goto no_coalesce;
-- 
2.41.0

Re: [PATCH mptcp-net v2] tcp: check mptcp-level constraints for backlog coalescing

Posted by Matthieu Baerts 6 months, 2 weeks ago

Hi Paolo, Mat,

On 11/10/2023 17:23, Paolo Abeni wrote:
> The MPTCP protocol can acquire the subflow-level socket lock and
> cause the tcp backlog usage. When inserting new skbs into the
> backlog, the stack will try to coalesce them.
> 
> Currently, we have no check in place to ensure that such coalescing
> will respect the MPTCP-level DSS, and that may cause data stream
> corruption, as reported by Christoph.
> 
> Address the issue by adding the relevant admission check for coalescing
> in tcp_add_backlog().
> 
> Note the issue is not easy to reproduce, as the MPTCP protocol tries
> hard to avoid acquiring the subflow-level socket lock.

Thank you for the patch and the review!

Now in our tree (fixes for -net):

New patches for t/upstream-net and t/upstream:
- 1d6459245311: tcp: check mptcp-level constraints for backlog coalescing
- Results: 08ccf8391935..b490362cfb13 (export-net)
- Results: a3a484f2dee3..54aab2945697 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20231012T083551
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20231012T083551

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-net v2] tcp: check mptcp-level constraints for backlog coalescing

Posted by Mat Martineau 6 months, 2 weeks ago

On Wed, 11 Oct 2023, Paolo Abeni wrote:

> The MPTCP protocol can acquire the subflow-level socket lock and
> cause the tcp backlog usage. When inserting new skbs into the
> backlog, the stack will try to coalesce them.
>
> Currently, we have no check in place to ensure that such coalescing
> will respect the MPTCP-level DSS, and that may cause data stream
> corruption, as reported by Christoph.
>
> Address the issue by adding the relevant admission check for coalescing
> in tcp_add_backlog().
>
> Note the issue is not easy to reproduce, as the MPTCP protocol tries
> hard to avoid acquiring the subflow-level socket lock.
>
> Fixes: 648ef4b88673 ("mptcp: Implement MPTCP receive path")
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/420
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
>  - !coalesce (mat)
>  - typo in commit message (mat)

Thanks Paolo, v2 LGTM:

Reviewed-by: Mat Martineau <martineau@kernel.org>

> ---
> net/ipv4/tcp_ipv4.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index a441740616d7..4d66a8ab3b98 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1870,6 +1870,7 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb,
> #ifdef CONFIG_TLS_DEVICE
> 	    tail->decrypted != skb->decrypted ||
> #endif
> +	    !mptcp_skb_can_collapse(tail, skb) ||
> 	    thtail->doff != th->doff ||
> 	    memcmp(thtail + 1, th + 1, hdrlen - sizeof(*th)))
> 		goto no_coalesce;
> -- 
> 2.41.0
>
>
>