The MPTCP protocol can acquire the subflow-level socket lock, and
cause the tcp backlog usage. When inserting new skb 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 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>
---
 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 f13eb7e23d03..52b6a0b22086 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1869,6 +1869,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

On Wed, 4 Oct 2023, Paolo Abeni wrote:

> The MPTCP protocol can acquire the subflow-level socket lock, and
> cause the tcp backlog usage. When inserting new skb 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 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>
> ---
> 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 f13eb7e23d03..52b6a0b22086 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1869,6 +1869,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) ||

Hi Paolo -

Shouldn't this be !mptcp_skb_can_collapse()?

Also, the subject line needs the 'o' in "coalescing".

- Mat

> 	    thtail->doff != th->doff ||
> 	    memcmp(thtail + 1, th + 1, hdrlen - sizeof(*th)))
> 		goto no_coalesce;
> -- 
> 2.41.0
>
>
>

On Tue, 2023-10-10 at 18:00 -0700, Mat Martineau wrote:
> On Wed, 4 Oct 2023, Paolo Abeni wrote:
> 
> > The MPTCP protocol can acquire the subflow-level socket lock, and
> > cause the tcp backlog usage. When inserting new skb 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 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>
> > ---
> > 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 f13eb7e23d03..52b6a0b22086 100644
> > --- a/net/ipv4/tcp_ipv4.c
> > +++ b/net/ipv4/tcp_ipv4.c
> > @@ -1869,6 +1869,7 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb,
> > #ifdef CONFIG_TLS_DEVICE
> > 	    tail->decrypted != skb->decrypted ||
> > #endif
> > +	    mptcp_skb_can_collapsen(tail, skb) ||
> 
> Hi Paolo -
> 
> Shouldn't this be !mptcp_skb_can_collapse()?
> 
> Also, the subject line needs the 'o' in "coalescing".

Indeed! thanks for checking!

I'll send a v2!

Cheers,

Paolo