* [BUG?] tcp: potential bug in tcp_is_sackblock_valid()
@ 2011-09-09 1:45 Yan, Zheng
2011-09-09 1:54 ` Herbert Xu
2011-09-18 19:35 ` Eric Dumazet
0 siblings, 2 replies; 13+ messages in thread
From: Yan, Zheng @ 2011-09-09 1:45 UTC (permalink / raw)
To: netdev, davem, eric.dumazet, herbert,
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()
2011-09-09 1:45 [BUG?] tcp: potential bug in tcp_is_sackblock_valid() Yan, Zheng
@ 2011-09-09 1:54 ` Herbert Xu
2011-09-09 2:10 ` Yan, Zheng
2011-09-18 19:35 ` Eric Dumazet
1 sibling, 1 reply; 13+ messages in thread
From: Herbert Xu @ 2011-09-09 1:54 UTC (permalink / raw)
To: Yan, Zheng; +Cc: netdev, davem, eric.dumazet, sfr
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()
2011-09-09 1:54 ` Herbert Xu
@ 2011-09-09 2:10 ` Yan, Zheng
2011-09-09 8:03 ` Ilpo Järvinen
0 siblings, 1 reply; 13+ messages in thread
From: Yan, Zheng @ 2011-09-09 2:10 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev, davem, eric.dumazet, sfr
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()
2011-09-09 2:10 ` Yan, Zheng
@ 2011-09-09 8:03 ` Ilpo Järvinen
[not found] ` <CAA93jw7L9KyXVevZ8dqE+ExLYP9=CLMmj1gPD88ihBuC6ZovOQ@mail.gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Ilpo Järvinen @ 2011-09-09 8:03 UTC (permalink / raw)
To: Yan, Zheng; +Cc: Herbert Xu, netdev, davem, eric.dumazet, sfr
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()
2011-09-09 1:45 [BUG?] tcp: potential bug in tcp_is_sackblock_valid() Yan, Zheng
2011-09-09 1:54 ` Herbert Xu
@ 2011-09-18 19:35 ` Eric Dumazet
2011-09-19 1:07 ` David Miller
1 sibling, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2011-09-18 19:35 UTC (permalink / raw)
To: Yan, Zheng; +Cc: netdev, davem, herbert, sfr
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()
[not found] ` <CAA93jw7L9KyXVevZ8dqE+ExLYP9=CLMmj1gPD88ihBuC6ZovOQ@mail.gmail.com>
@ 2011-09-18 20:39 ` Eric Dumazet
2011-09-18 21:04 ` Dave Taht
0 siblings, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2011-09-18 20:39 UTC (permalink / raw)
To: Dave Taht; +Cc: Ilpo Järvinen, Yan, Zheng, Herbert Xu, netdev, davem, sfr
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).
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()
2011-09-18 20:39 ` Eric Dumazet
@ 2011-09-18 21:04 ` Dave Taht
2011-09-18 21:20 ` Hagen Paul Pfeifer
0 siblings, 1 reply; 13+ messages in thread
From: Dave Taht @ 2011-09-18 21:04 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ilpo Järvinen, Yan, Zheng, Herbert Xu, netdev, davem, sfr
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()
2011-09-18 21:04 ` Dave Taht
@ 2011-09-18 21:20 ` Hagen Paul Pfeifer
0 siblings, 0 replies; 13+ messages in thread
From: Hagen Paul Pfeifer @ 2011-09-18 21:20 UTC (permalink / raw)
To: Dave Taht
Cc: Eric Dumazet, Ilpo Järvinen, Yan, Zheng, Herbert Xu, netdev,
davem, sfr
* 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG?] tcp: potential bug in tcp_is_sackblock_valid()
2011-09-18 19:35 ` Eric Dumazet
@ 2011-09-19 1:07 ` David Miller
2011-09-19 2:05 ` [PATCH] tcp: fix validation of D-SACK Yan, Zheng
0 siblings, 1 reply; 13+ messages in thread
From: David Miller @ 2011-09-19 1:07 UTC (permalink / raw)
To: eric.dumazet; +Cc: zheng.z.yan, netdev, herbert, sfr
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] tcp: fix validation of D-SACK
2011-09-19 1:07 ` David Miller
@ 2011-09-19 2:05 ` Yan, Zheng
2011-09-19 2:37 ` David Miller
0 siblings, 1 reply; 13+ messages in thread
From: Yan, Zheng @ 2011-09-19 2:05 UTC (permalink / raw)
To: netdev; +Cc: David Miller, eric.dumazet
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))
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] tcp: fix validation of D-SACK
2011-09-19 2:05 ` [PATCH] tcp: fix validation of D-SACK Yan, Zheng
@ 2011-09-19 2:37 ` David Miller
2011-09-19 17:29 ` Jan Ceuleers
0 siblings, 1 reply; 13+ messages in thread
From: David Miller @ 2011-09-19 2:37 UTC (permalink / raw)
To: zheng.z.yan; +Cc: netdev, eric.dumazet
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tcp: fix validation of D-SACK
2011-09-19 2:37 ` David Miller
@ 2011-09-19 17:29 ` Jan Ceuleers
2011-09-20 18:50 ` David Miller
0 siblings, 1 reply; 13+ messages in thread
From: Jan Ceuleers @ 2011-09-19 17:29 UTC (permalink / raw)
To: David Miller; +Cc: zheng.z.yan, netdev, eric.dumazet
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tcp: fix validation of D-SACK
2011-09-19 17:29 ` Jan Ceuleers
@ 2011-09-20 18:50 ` David Miller
0 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2011-09-20 18:50 UTC (permalink / raw)
To: jan.ceuleers; +Cc: zheng.z.yan, netdev, eric.dumazet
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-09-20 18:50 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-09 1:45 [BUG?] tcp: potential bug in tcp_is_sackblock_valid() Yan, Zheng
2011-09-09 1:54 ` Herbert Xu
2011-09-09 2:10 ` Yan, Zheng
2011-09-09 8:03 ` Ilpo Järvinen
[not found] ` <CAA93jw7L9KyXVevZ8dqE+ExLYP9=CLMmj1gPD88ihBuC6ZovOQ@mail.gmail.com>
2011-09-18 20:39 ` Eric Dumazet
2011-09-18 21:04 ` Dave Taht
2011-09-18 21:20 ` Hagen Paul Pfeifer
2011-09-18 19:35 ` Eric Dumazet
2011-09-19 1:07 ` David Miller
2011-09-19 2:05 ` [PATCH] tcp: fix validation of D-SACK Yan, Zheng
2011-09-19 2:37 ` David Miller
2011-09-19 17:29 ` Jan Ceuleers
2011-09-20 18:50 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).