From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH] tcp: Reallocate headroom if it would overflow csum_start Date: Thu, 11 Apr 2013 08:49:54 -0700 Message-ID: <1365695394.3887.162.camel@edumazet-glaptop> References: Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, netdev@vger.kernel.org To: Thomas Graf Return-path: Received: from mail-pd0-f172.google.com ([209.85.192.172]:34991 "EHLO mail-pd0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936524Ab3DKPt6 (ORCPT ); Thu, 11 Apr 2013 11:49:58 -0400 Received: by mail-pd0-f172.google.com with SMTP id 5so922844pdd.17 for ; Thu, 11 Apr 2013 08:49:58 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Thu, 2013-04-11 at 13:19 +0200, Thomas Graf wrote: > If a TCP retransmission gets partially ACKed and collapsed multiple > times it is possible for the headroom to grow beyond 64K which will > overflow the 16bit skb->csum_start which is based on the start of > the headroom. It has been observed rarely in the wild with IPoIB due > to the 64K MTU. > > Verify if the acking and collapsing resulted in a headroom exceeding > what csum_start can cover and reallocate the headroom if so. > > LLNL has been running the patch for a while and has not seen the > problem occur since. > > A big thank you to Jim Foraker and the team at > LLNL for helping out with the investigation and testing. > > Reported-by: Jim Foraker > Signed-off-by: Thomas Graf > --- > v2: reallocate headroom instead of preventing further collapsing > > net/ipv4/tcp_output.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index b44cf81..bf6ceb7 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -2388,8 +2388,11 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb) > */ > TCP_SKB_CB(skb)->when = tcp_time_stamp; > > - /* make sure skb->data is aligned on arches that require it */ > - if (unlikely(NET_IP_ALIGN && ((unsigned long)skb->data & 3))) { > + /* make sure skb->data is aligned on arches that require it > + * and check if ack-trimming & collapsing extended the headroom > + * beyond what csum_start can cover. */ > + if (unlikely(NET_IP_ALIGN && ((unsigned long)skb->data & 3) || > + skb_headroom(skb) >= 0xFFFF)) { > struct sk_buff *nskb = __pskb_copy(skb, MAX_TCP_HEADER, > GFP_ATOMIC); > return nskb ? tcp_transmit_skb(sk, nskb, 0, GFP_ATOMIC) : Strange... It was tested on an arch with NET_IP_ALIGN == 2 I presume ? This fix should also be done for other arches (x86 for example) I would code the condition like that instead if ((NET_IP_ALIGN && ((unsigned long)skb->data & 3)) || skb_headroom(skb) >= 0xFFFF)