[PATCH] tcp: Reallocate headroom if it would overflow csum_start

[PATCH] tcp: Reallocate headroom if it would overflow csum_start

[Thomas Graf]

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 <foraker1@llnl.gov> and the team at
LLNL for helping out with the investigation and testing.

Reported-by: Jim Foraker <foraker1@llnl.gov>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
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) :
-- 
1.7.11.7

Re: [PATCH] tcp: Reallocate headroom if it would overflow csum_start

[Sergei Shtylyov]

Hello.

On 11-04-2013 15:19, 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 <foraker1@llnl.gov> and the team at
> LLNL for helping out with the investigation and testing.

> Reported-by: Jim Foraker <foraker1@llnl.gov>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
[...]

    Minor formatting nit.

> 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. */

    The preferred multi-line comment style in the networking code:

/* bla
  * bla
  */

WBR, Sergei

Re: [PATCH] tcp: Reallocate headroom if it would overflow csum_start

[Eric Dumazet]

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 <foraker1@llnl.gov> and the team at
> LLNL for helping out with the investigation and testing.
> 
> Reported-by: Jim Foraker <foraker1@llnl.gov>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
> 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)

Re: [PATCH] tcp: Reallocate headroom if it would overflow csum_start

[Ben Hutchings]

On Thu, 2013-04-11 at 08:49 -0700, Eric Dumazet wrote:
> 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 <foraker1@llnl.gov> and the team at
> > LLNL for helping out with the investigation and testing.
> > 
> > Reported-by: Jim Foraker <foraker1@llnl.gov>
> > Signed-off-by: Thomas Graf <tgraf@suug.ch>
> > ---
> > 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)

You dropped the unlikely() and added redundant parentheses, which may be
clearer but is still equivalent.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] tcp: Reallocate headroom if it would overflow csum_start

[Eric Dumazet]

On Thu, 2013-04-11 at 18:52 +0100, Ben Hutchings wrote:
> On Thu, 2013-04-11 at 08:49 -0700, Eric Dumazet wrote:
> > 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 <foraker1@llnl.gov> and the team at
> > > LLNL for helping out with the investigation and testing.
> > > 
> > > Reported-by: Jim Foraker <foraker1@llnl.gov>
> > > Signed-off-by: Thomas Graf <tgraf@suug.ch>
> > > ---
> > > 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)
> 
> You dropped the unlikely() and added redundant parentheses, which may be
> clearer but is still equivalent.

I see what you mean...

I just don't like

if (A && B || C)

I prefer in this case

if ((A && B) || C)

Then add the unlikely() if we really care in this _ultra_ slow path

if (unlikely((A && B) || C))