| Message ID | 925dcab4e637ab2ad2d947baa937a62146040802.1622044966.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Mat Martineau |
| Headers | show |
| Series | [mptcp-net] tcp: ensure that backlog coalescing don't break MPTCP DSS | expand |
On Wed, 26 May 2021, Paolo Abeni wrote: > Currently the backlog coalescing does check for MPTCP validation: > a relevant DSS could be lost leading to MPTCP stream corruption. > > The above is quite infrequent since the MPTCP subflows are not > exposed to user-space but the MPTCP still acquires the ssk socket > lock on some events. > > Fix the issue adding the missing test. > > Fixes: 85712484110d ("tcp: coalesce/collapse must respect MPTCP extensions") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > this can possibly fix issues/196, but it's very hard to demonstrate, > even with pktdrill > --- > 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 4f5b68a90be9..6b033593d069 100644 > --- a/net/ipv4/tcp_ipv4.c > +++ b/net/ipv4/tcp_ipv4.c > @@ -1822,6 +1822,7 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb) > > if (TCP_SKB_CB(tail)->end_seq != TCP_SKB_CB(skb)->seq || > TCP_SKB_CB(tail)->ip_dsfield != TCP_SKB_CB(skb)->ip_dsfield || > + !mptcp_skb_can_collapse(tail, skb) || > ((TCP_SKB_CB(tail)->tcp_flags | > TCP_SKB_CB(skb)->tcp_flags) & (TCPHDR_SYN | TCPHDR_RST | TCPHDR_URG)) || > !((TCP_SKB_CB(tail)->tcp_flags & > -- > 2.26.3 Yeah, definitely could lose MPTCP header information here. Since mptcp_incoming_options() hasn't been called yet, skbs in the backlog queue will never have the MPTCP skb extensions that mptcp_skb_can_collapse() relies on. Is it worth it to add a function that will do a subset of the mptcp_incoming_options() work to populate the skb extension for mptcp subflow socks? (and modify mptcp_incoming_options() to detect existing mpexts?) -- Mat Martineau Intel
On Wed, 2021-05-26 at 16:27 -0700, Mat Martineau wrote: > On Wed, 26 May 2021, Paolo Abeni wrote: > > > Currently the backlog coalescing does check for MPTCP validation: > > a relevant DSS could be lost leading to MPTCP stream corruption. > > > > The above is quite infrequent since the MPTCP subflows are not > > exposed to user-space but the MPTCP still acquires the ssk socket > > lock on some events. > > > > Fix the issue adding the missing test. > > > > Fixes: 85712484110d ("tcp: coalesce/collapse must respect MPTCP extensions") > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > --- > > this can possibly fix issues/196, but it's very hard to demonstrate, > > even with pktdrill > > --- > > 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 4f5b68a90be9..6b033593d069 100644 > > --- a/net/ipv4/tcp_ipv4.c > > +++ b/net/ipv4/tcp_ipv4.c > > @@ -1822,6 +1822,7 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb) > > > > if (TCP_SKB_CB(tail)->end_seq != TCP_SKB_CB(skb)->seq || > > TCP_SKB_CB(tail)->ip_dsfield != TCP_SKB_CB(skb)->ip_dsfield || > > + !mptcp_skb_can_collapse(tail, skb) || > > ((TCP_SKB_CB(tail)->tcp_flags | > > TCP_SKB_CB(skb)->tcp_flags) & (TCPHDR_SYN | TCPHDR_RST | TCPHDR_URG)) || > > !((TCP_SKB_CB(tail)->tcp_flags & > > -- > > 2.26.3 > > Yeah, definitely could lose MPTCP header information here. > > Since mptcp_incoming_options() hasn't been called yet, skbs in the backlog > queue will never have the MPTCP skb extensions that > mptcp_skb_can_collapse() relies on. whoops, I missed that point!!! > Is it worth it to add a function that will do a subset of the > mptcp_incoming_options() work to populate the skb extension for mptcp > subflow socks? (and modify mptcp_incoming_options() to detect existing > mpexts?) I was wondering about memcmp-aring the TCP options carried by the two skbs, then I noted that the existing code already does: thtail->doff != th->doff || memcmp(thtail + 1, th + 1, hdrlen - sizeof(*th))) Skbs with different mapping can't really be merged, and the suspected issue is really not possible. I think this patch can be dropped, thanks! Paolo
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 4f5b68a90be9..6b033593d069 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -1822,6 +1822,7 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb) if (TCP_SKB_CB(tail)->end_seq != TCP_SKB_CB(skb)->seq || TCP_SKB_CB(tail)->ip_dsfield != TCP_SKB_CB(skb)->ip_dsfield || + !mptcp_skb_can_collapse(tail, skb) || ((TCP_SKB_CB(tail)->tcp_flags | TCP_SKB_CB(skb)->tcp_flags) & (TCPHDR_SYN | TCPHDR_RST | TCPHDR_URG)) || !((TCP_SKB_CB(tail)->tcp_flags &
Currently the backlog coalescing does check for MPTCP validation: a relevant DSS could be lost leading to MPTCP stream corruption. The above is quite infrequent since the MPTCP subflows are not exposed to user-space but the MPTCP still acquires the ssk socket lock on some events. Fix the issue adding the missing test. Fixes: 85712484110d ("tcp: coalesce/collapse must respect MPTCP extensions") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- this can possibly fix issues/196, but it's very hard to demonstrate, even with pktdrill --- net/ipv4/tcp_ipv4.c | 1 + 1 file changed, 1 insertion(+)