netdev - Re: [PATCH 2/2 net-next] tcp: sk_add_backlog() is too agressive for TCP

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]

Date:	Tue, 24 Apr 2012 11:40:27 +0300 (EEST)
From:	"Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
To:	David Miller <davem@...emloft.net>
cc:	Eric Dumazet <eric.dumazet@...il.com>, rick.jones2@...com,
	Netdev <netdev@...r.kernel.org>, therbert@...gle.com,
	ncardwell@...gle.com, maze@...gle.com,
	Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH 2/2 net-next] tcp: sk_add_backlog() is too agressive for
 TCP

On Tue, 24 Apr 2012, David Miller wrote:

> From: "Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
> Date: Tue, 24 Apr 2012 11:21:18 +0300 (EEST)
> 
> > On Tue, 24 Apr 2012, David Miller wrote:
> > 
> >> That makes this a non-starter since we must therefore remember all of
> >> the SACK boundaries in the original packets.
> > 
> > GRO works because TCP tends to use rather constant MSS, right? ...Since 
> > ACKs and SACKs are nothing more than reflection of those MSS boundaries of 
> > the opposite direction I don't find that as impossible as you do because 
> > the same kind of "mss" assumption can be applied there. But GRO has made 
> > this somewhat messier now because the receiver doesn't any more generate 
> > ACK per MSS or ACK per 2*MSS but that could be "fixed" by offloading the 
> > ACK sending when responding to a GROed packet.
> 
> We're talking about accumulating ACKs on GRO not data packets.

So am I... :-). ...Code speaks more than thousands of words:

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 8bb6ade..33b87b2 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2820,7 +2820,11 @@ found:
 	flush |= (__force int)(flags & TCP_FLAG_CWR);
 	flush |= (__force int)((flags ^ tcp_flag_word(th2)) &
 		  ~(TCP_FLAG_CWR | TCP_FLAG_FIN | TCP_FLAG_PSH));
-	flush |= (__force int)(th->ack_seq ^ th2->ack_seq);
+
+	ackgap = skb_shinfo(p)->ack_size;
+	ackdiff = th2->ack_seq - th->ack_seq;
+	flush |= (ackdiff - 1) >= ackgap;
+
 	for (i = sizeof(*th); i < thlen; i += 4)
 		flush |= *(u32 *)((u8 *)th + i) ^
 			 *(u32 *)((u8 *)th2 + i);


...Obviously Data and ACK couldn't be GROed at the same time practically 
(would allow reusing the gso_size field for ack_size). ...But why exactly 
you think this is not possible or viable solution if fully implemented?

And the problem I mentioned in the previous mail (in the terms of this 
code fragment) is that ackdiff is no longer MSS or 2*MSS because of GRO 
for the opposite direction doesn't trigger all those ACKs a non-GRO 
receiver would.

-- 
 i.