netdev - [PATCH] [TCP] MIB: Fine-tune reordered ACK's SACK block counting more

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.64.0709261653110.5141@kivilampi-30.cs.helsinki.fi>
Date:	Wed, 26 Sep 2007 17:07:04 +0300 (EEST)
From:	"Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
To:	David Miller <davem@...emloft.net>
cc:	Netdev <netdev@...r.kernel.org>
Subject: [PATCH] [TCP] MIB: Fine-tune reordered ACK's SACK block counting
 more

On Tue, 25 Sep 2007, David Miller wrote:

> From: "Ilpo_Järvinen" <ilpo.jarvinen@...sinki.fi>
> Date: Mon, 24 Sep 2007 12:04:07 +0300
> 
> > In case of ACK reordering, the SACK block might be valid in it's
> > time but is already obsoleted since we've received another kind
> > of confirmation about arrival of the segments through snd_una
> > advancement of an earlier packet.
> > 
> > I didn't bother to build distinguishing of valid and invalid
> > SACK blocks but simply made reordered SACK blocks that are too
> > old always not counted regardless of their "real" validity which
> > could be determined by using the ack field of the reordered
> > packet (won't be significant IMHO).
> > 
> > DSACKs can very well be considered useful even in this situation,
> > so won't do any of this for them.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
> 
> This looks fine to me, applied.
> 
> If the skipped case is interesting we can add another MIB
> stat for it :-)

Hmm, thought a bit more about it... How about this then? No additional 
counter. Compile tested. Not that I see a large benefit from it, in theory 
could help logging in some malicious attempt cases; we safely discarded in 
such case anyway... ...of course if somebody is paranoid enough one might 
rejoice in it more than I do... :-) Buggy cases will very likely show up 
regardless this reordering corner-case.


From: =?ISO-8859-1?q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@...sinki.fi>
Date: Wed, 26 Sep 2007 14:17:12 +0300
Subject: [PATCH] [TCP] MIB: Fine-tune reordered ACK's SACK block counting more

Check the SACK block against the state that TCP was in at the
time of that ACK. Not really since we can summon just snd_una
back and not snd_nxt but that shouldn't be a problem. Reused
checking code which avoids adding other nasty wrap, etc. holes.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
---
 net/ipv4/tcp_input.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 259f517..95c8a4a 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1063,7 +1063,7 @@ static void tcp_update_reordering(struct sock *sk, const int metric,
  * be used as an exaggerated estimate.
  */
 static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
-				  u32 start_seq, u32 end_seq)
+				  u32 start_seq, u32 end_seq, u32 snd_una)
 {
 	/* Too far in future, or reversed (interpretation is ambiguous) */
 	if (after(end_seq, tp->snd_nxt) || !before(start_seq, end_seq))
@@ -1076,14 +1076,14 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
 	/* In outstanding window? ...This is valid exit for DSACKs too.
 	 * start_seq == snd_una is non-sensical (see comments above)
 	 */
-	if (after(start_seq, tp->snd_una))
+	if (after(start_seq, snd_una))
 		return 1;
 
 	if (!is_dsack || !tp->undo_marker)
 		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, snd_una))
 		return 0;
 
 	if (!before(start_seq, tp->undo_marker))
@@ -1244,16 +1244,21 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 		int fack_count;
 		int dup_sack = (found_dup_sack && (i == first_sack_index));
 
-		if (!tcp_is_sackblock_valid(tp, dup_sack, start_seq, end_seq)) {
+		if (!tcp_is_sackblock_valid(tp, dup_sack, start_seq, end_seq,
+					    tp->snd_una)) {
 			if (dup_sack) {
 				if (!tp->undo_marker)
 					NET_INC_STATS_BH(LINUX_MIB_TCPDSACKIGNOREDNOUNDO);
 				else
 					NET_INC_STATS_BH(LINUX_MIB_TCPDSACKIGNOREDOLD);
 			} else {
-				/* Don't count olds caused by ACK reordering */
+				/* Don't count old SACK blocks caused by ACK
+				 * reordering if it was valid back then
+				 */
 				if ((TCP_SKB_CB(ack_skb)->ack_seq != tp->snd_una) &&
-				    !after(end_seq, tp->snd_una))
+				    tcp_is_sackblock_valid(tp, dup_sack,
+							   start_seq, end_seq,
+							   TCP_SKB_CB(ack_skb)->ack_seq))
 					continue;
 				NET_INC_STATS_BH(LINUX_MIB_TCPSACKDISCARD);
 			}
-- 
1.5.0.6