Skip to contentSkip to conversation

tcp: potential bug in tcp_is_sackblock_valid()

BUG? Applied Yan, Zheng 8 participants 13 messages Sep 9, 2011 – Sep 20, 2011 1 message not in the archive

Subsystems (3)
Networking Network drivers TCP
lore
Share & tools
lore

Yan, Zheng

author
#0 lore raw

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:

---

net/ipv4/tcp_input.c +1 -1
index 385c470..a5d01b1 100644
@@ -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

#1 lore raw
Quoted 21 lines · 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:

---

net/ipv4/tcp_input.c +1 -2
index 385c470..a5d01b1 100644
@@ -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))
---
Quoted 2 lines

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,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#2 lore raw
Quoted 19 lines · Herbert Xu
Quoted 18 lines · 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:

---

net/ipv4/tcp_input.c +1 -1
index 385c470..a5d01b1 100644
@@ -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;
Quoted 9 lines
Quoted 5 lines
net/ipv4/tcp_input.c +0 -1
        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.

#3 lore raw
Quoted 17 lines · Zheng
Quoted 16 lines · Herbert Xu
Quoted 15 lines · Zheng

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

net/ipv4/tcp_input.c +1 -1
index 385c470..a5d01b1 100644
@@ -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;
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
net/ipv4/tcp_input.c +0 -1
        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.

Quoted 5 lines
Quoted 2 lines

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.

Signature · 1 line
 i.

A message that is not in the archive (sent off-list or not yet received): CAA93jw7L9KyXVevZ8dqE+ExLYP9=CLMmj1gPD88ihBuC6ZovOQ@mail.gmail.com

#5 lore raw
Quoted 4 lines · Dave Taht

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.

Quoted 2 lines

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).

#6 lore raw
Quoted 7 lines · Eric Dumazet
Quoted 4 lines · Dave Taht

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

Quoted 6 lines

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

Quoted 2 lines

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.

Quoted 8 lines

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

Signature · 4 lines
Dave Täht
SKYPE: davetaht
US Tel: 1-239-829-5608
http://the-edge.blogspot.com
#7 lore raw
Quoted 2 lines · * 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

Acked-by
#8 lore raw
In reply to Yan, Zheng
Quoted 11 lines · 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:

---

net/ipv4/tcp_input.c +0 -0
index 385c470..a5d01b1 100644

net/ipv4/tcp_input.c static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,

Quoted 9 lines
net/ipv4/tcp_input.c +1 -2
@@ -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 <[email protected]>

This bug was introduced in 2.6.24 by commit 5b3c9882

maintainer
#9 lore raw

From: Eric Dumazet <[email protected]>
Date: Sun, 18 Sep 2011 21:35:44 +0200

Quoted 19 lines · Zheng
Quoted 18 lines

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:

---

net/ipv4/tcp_input.c +1 -1
index 385c470..a5d01b1 100644
@@ -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;
Quoted 6 lines

net/ipv4/tcp_input.c static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,

Quoted 2 lines
net/ipv4/tcp_input.c +0 -1
        if (!before(start_seq, tp->undo_marker))
---
Acked-by Eric Dumazet <[email protected]>

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.

author
#10 lore raw

PATCH 1/1 tcp: fix validation of D-SACK

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 <[email protected]>
Acked-by Eric Dumazet <[email protected]>

---

net/ipv4/tcp_input.c +1 -1
index ea0d218..21fab3e 100644
@@ -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))
#12 lore raw

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

Quoted 6 lines · David Miller
Quoted 5 lines

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<[email protected]>
Acked-by Eric Dumazet<[email protected]>

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

maintainer
#13 lore raw

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

From: Jan Ceuleers <[email protected]>
Date: Mon, 19 Sep 2011 19:29:16 +0200

Quoted 2 lines

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

Yes, I have.