[BUG?] tcp: potential bug in tcp_is_sackblock_valid()

[BUG?] tcp: potential bug in tcp_is_sackblock_valid()

[Yan, Zheng]

Hi all,

I found a check in tcp_is_sackblock_valid() is suspicious. It against
its comment and RFC. I think the correct check should be:

---
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 385c470..a5d01b1 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1124,7 +1124,7 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
                return 0;
 
        /* ...Then it's D-SACK, and must reside below snd_una completely */
-       if (!after(end_seq, tp->snd_una))
+       if (after(end_seq, tp->snd_una))
                return 0;
 
        if (!before(start_seq, tp->undo_marker))
---

I also checked /proc/net/netstat of my laptop, found TCPDSACKIgnoredOld
field is not zero. Maybe it's caused by the bug.

Regards
Yan, Zheng

Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()

[Herbert Xu]

On Fri, Sep 09, 2011 at 09:45:52AM +0800, Yan, Zheng wrote:
> Hi all,
> 
> I found a check in tcp_is_sackblock_valid() is suspicious. It against
> its comment and RFC. I think the correct check should be:
> 
> ---
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 385c470..a5d01b1 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -1124,7 +1124,7 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
>                 return 0;
>  
>         /* ...Then it's D-SACK, and must reside below snd_una completely */
> -       if (!after(end_seq, tp->snd_una))
> +       if (after(end_seq, tp->snd_una))
>                 return 0;
>  
>         if (!before(start_seq, tp->undo_marker))
> ---
> 
> I also checked /proc/net/netstat of my laptop, found TCPDSACKIgnoredOld
> field is not zero. Maybe it's caused by the bug.

Yes this looks like a typo.  Please resend your patch with a
description and signed-off-by line.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()

[Yan, Zheng]

On 09/09/2011 09:54 AM, Herbert Xu wrote:
> On Fri, Sep 09, 2011 at 09:45:52AM +0800, Yan, Zheng wrote:
>> Hi all,
>>
>> I found a check in tcp_is_sackblock_valid() is suspicious. It against
>> its comment and RFC. I think the correct check should be:
>>
>> ---
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index 385c470..a5d01b1 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -1124,7 +1124,7 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
>>                 return 0;
>>  
>>         /* ...Then it's D-SACK, and must reside below snd_una completely */
>> -       if (!after(end_seq, tp->snd_una))
>> +       if (after(end_seq, tp->snd_una))
>>                 return 0;
>>  
>>         if (!before(start_seq, tp->undo_marker))
>> ---
>>
>> I also checked /proc/net/netstat of my laptop, found TCPDSACKIgnoredOld
>> field is not zero. Maybe it's caused by the bug.
> 
> Yes this looks like a typo.  Please resend your patch with a
> description and signed-off-by line.
> 

I think the bug has a big influence on tcp DSACKs. It's better to
leave it to people who fully understand tcp code.

Thanks.

Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()

[Ilpo Järvinen]

On Fri, 9 Sep 2011, Yan, Zheng wrote:

> On 09/09/2011 09:54 AM, Herbert Xu wrote:
> > On Fri, Sep 09, 2011 at 09:45:52AM +0800, Yan, Zheng wrote:
> >> I found a check in tcp_is_sackblock_valid() is suspicious. It against
> >> its comment and RFC. I think the correct check should be:
> >> ---
> >> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> >> index 385c470..a5d01b1 100644
> >> --- a/net/ipv4/tcp_input.c
> >> +++ b/net/ipv4/tcp_input.c
> >> @@ -1124,7 +1124,7 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
> >>                 return 0;
> >>  
> >>         /* ...Then it's D-SACK, and must reside below snd_una completely */
> >> -       if (!after(end_seq, tp->snd_una))
> >> +       if (after(end_seq, tp->snd_una))
> >>                 return 0;
> >>  
> >>         if (!before(start_seq, tp->undo_marker))
> >> ---
> >>
> >> I also checked /proc/net/netstat of my laptop, found TCPDSACKIgnoredOld
> >> field is not zero. Maybe it's caused by the bug.

Hmm, some Ignored I was expecting to see even when nothing was wrong but I 
think it might have been the other counter... too long time already passed 
to remember all the details I've been thinking.

> > Yes this looks like a typo.  Please resend your patch with a
> > description and signed-off-by line.
> 
> I think the bug has a big influence on tcp DSACKs. It's better to
> leave it to people who fully understand tcp code.

Indeed, it looks like a double negative error.

-- 
 i.

Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()

[Eric Dumazet]

Le samedi 17 septembre 2011 à 11:30 -0700, Dave Taht a écrit :

> 
> As cerowrt is one of the few projects enabling sack/dsack/ecn by
> default, it would be good to know either
> 

dsack is enabled by default on linux, not only cerowt project.

> A) how to test to see if this problem is really a problem
> B) should we apply the obvious fix

Isnt Cerowrt a router, most of frames are forwarded anyway ?

This bug is relevant to linux hosts, receiving Duplicate Sacks (RFC
2883).

Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()

[Dave Taht]

On Sun, Sep 18, 2011 at 1:39 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le samedi 17 septembre 2011 à 11:30 -0700, Dave Taht a écrit :
>
>>
>> As cerowrt is one of the few projects enabling sack/dsack/ecn by
>> default, it would be good to know either
>>
>
> dsack is enabled by default on linux, not only cerowt project.
>
>> A) how to test to see if this problem is really a problem
>> B) should we apply the obvious fix
>
> Isnt Cerowrt a router, most of frames are forwarded anyway ?

A very large suite of network test tools runs on the router itself -
netperf, nuttcp, iperf, httping, etc.

netperf, for example, ships, enabled.

This is a means of being able to analyze performance of both the LFN
and wireless from a central point,
from a well defined platform,
without having a random desktop involved.

> This bug is relevant to linux hosts, receiving Duplicate Sacks (RFC
> 2883).
>
>
>
>
>
>



-- 
Dave Täht
SKYPE: davetaht
US Tel: 1-239-829-5608
http://the-edge.blogspot.com

Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()

[Hagen Paul Pfeifer]

* Dave Taht | 2011-09-18 14:04:44 [-0700]:

>A very large suite of network test tools runs on the router itself -
>netperf, nuttcp, iperf, httping, etc.

But then the router _act_ as an end-host - no forwarding.

HGN

Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()

[Eric Dumazet]

Le vendredi 09 septembre 2011 à 09:45 +0800, Yan, Zheng a écrit :
> Hi all,
> 
> I found a check in tcp_is_sackblock_valid() is suspicious. It against
> its comment and RFC. I think the correct check should be:
> 
> ---
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 385c470..a5d01b1 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -1124,7 +1124,7 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
>                 return 0;
>  
>         /* ...Then it's D-SACK, and must reside below snd_una completely */
> -       if (!after(end_seq, tp->snd_una))
> +       if (after(end_seq, tp->snd_una))
>                 return 0;
>  
>         if (!before(start_seq, tp->undo_marker))
> ---

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

This bug was introduced in 2.6.24 by commit 5b3c9882

Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 18 Sep 2011 21:35:44 +0200

> Le vendredi 09 septembre 2011 à 09:45 +0800, Yan, Zheng a écrit :
>> Hi all,
>> 
>> I found a check in tcp_is_sackblock_valid() is suspicious. It against
>> its comment and RFC. I think the correct check should be:
>> 
>> ---
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index 385c470..a5d01b1 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -1124,7 +1124,7 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
>>                 return 0;
>>  
>>         /* ...Then it's D-SACK, and must reside below snd_una completely */
>> -       if (!after(end_seq, tp->snd_una))
>> +       if (after(end_seq, tp->snd_una))
>>                 return 0;
>>  
>>         if (!before(start_seq, tp->undo_marker))
>> ---
> 
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> This bug was introduced in 2.6.24 by commit 5b3c9882

Yan, please repost this patch with proper commit message and signoffs
so I can apply it, thanks.

[PATCH] tcp: fix validation of D-SACK

[Yan, Zheng]

D-SACK is allowed to reside below snd_una. But the corresponding check
in tcp_is_sackblock_valid() is the exact opposite. It looks like a typo.

Signed-off-by: Zheng Yan <zheng.z.yan@intel.com>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index ea0d218..21fab3e 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1124,7 +1124,7 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
 		return 0;
 
 	/* ...Then it's D-SACK, and must reside below snd_una completely */
-	if (!after(end_seq, tp->snd_una))
+	if (after(end_seq, tp->snd_una))
 		return 0;
 
 	if (!before(start_seq, tp->undo_marker))

Re: [PATCH] tcp: fix validation of D-SACK

[David Miller]

From: "Yan, Zheng" <zheng.z.yan@intel.com>
Date: Mon, 19 Sep 2011 10:05:31 +0800

> D-SACK is allowed to reside below snd_una. But the corresponding check
> in tcp_is_sackblock_valid() is the exact opposite. It looks like a typo.
> 
> Signed-off-by: Zheng Yan <zheng.z.yan@intel.com>
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks.

Re: [PATCH] tcp: fix validation of D-SACK

[Jan Ceuleers]

On 09/19/2011 04:37 AM, David Miller wrote:
>> D-SACK is allowed to reside below snd_una. But the corresponding check
>> in tcp_is_sackblock_valid() is the exact opposite. It looks like a typo.
>>
>> Signed-off-by: Zheng Yan<zheng.z.yan@intel.com>
>> Acked-by: Eric Dumazet<eric.dumazet@gmail.com>
> Applied, thanks.

Dave,

Have you also queued it up for stable as per Eric's remark that this was 
introduced in 2.6.24?

Sorry if this is implied in your workflow.

Thanks, Jan

Re: [PATCH] tcp: fix validation of D-SACK

[David Miller]

From: Jan Ceuleers <jan.ceuleers@computer.org>
Date: Mon, 19 Sep 2011 19:29:16 +0200

> Have you also queued it up for stable as per Eric's remark that this
> was introduced in 2.6.24?

Yes, I have.