diff mbox

updates to syncookies - timestamps not needed any more (freebsd)

Message ID 1373637885.10804.7.camel@edumazet-glaptop
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet July 12, 2013, 2:04 p.m. UTC
On Fri, 2013-07-12 at 10:41 +0200, Florian Westphal wrote:

> The main difference to what linux does is to avoid encoding the 'count'
> value (Linux doesn't reseed secret[], and relies on count to detect old
> cookies).
> 
> Not having the counter frees up space to encode tcp options in the cookie
> instead of the timestamp.

But still wscale and sack options are disabled.

lpq83:~# echo 0 >/proc/sys/net/ipv4/tcp_timestamps
lpq83:~# tcpdump -p -n -s 0 -i eth4

07:03:37.337563 IP 7.7.7.84.64131 > 7.7.7.83.22: S 1523884225:1523884225(0) win 29200 <mss 1460,sackOK,timestamp 74412758 0,nop,wscale 6>
07:03:37.337588 IP 7.7.7.83.22 > 7.7.7.84.64131: S 572330188:572330188(0) ack 1523884226 win 29200 <mss 1460>
07:03:37.337647 IP 7.7.7.84.64131 > 7.7.7.83.22: . ack 1 win 29200


BTW, following patch allows to test more easily syncookies behavior.

If sysctl_tcp_syncookies is set to 2, we always use syncookies.





--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Florian Westphal July 12, 2013, 2:25 p.m. UTC | #1
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2013-07-12 at 10:41 +0200, Florian Westphal wrote:
> 
> > The main difference to what linux does is to avoid encoding the 'count'
> > value (Linux doesn't reseed secret[], and relies on count to detect old
> > cookies).
> > 
> > Not having the counter frees up space to encode tcp options in the cookie
> > instead of the timestamp.
> 
> But still wscale and sack options are disabled.

Yes, in Linux sack and wscale will be encoded in the timestamp, as
cookie is already restricted to 24 bits due to counter.

Without the counter, that could be changed to allow sack/wscale even
with ts off.

> BTW, following patch allows to test more easily syncookies behavior.
> 
> If sysctl_tcp_syncookies is set to 2, we always use syncookies.

I think this change would be useful.

> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1462,7 +1462,8 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
>  	 * limitations, they conserve resources and peer is
>  	 * evidently real one.
>  	 */
> -	if (inet_csk_reqsk_queue_is_full(sk) && !isn) {
> +	if ((sysctl_tcp_syncookies == 2 ||
> +	     inet_csk_reqsk_queue_is_full(sk)) && !isn) {
>  		want_cookie = tcp_syn_flood_action(sk, skb, "TCP");
>  		if (!want_cookie)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet July 12, 2013, 2:32 p.m. UTC | #2
On Fri, 2013-07-12 at 16:25 +0200, Florian Westphal wrote:
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Fri, 2013-07-12 at 10:41 +0200, Florian Westphal wrote:
> > 
> > > The main difference to what linux does is to avoid encoding the 'count'
> > > value (Linux doesn't reseed secret[], and relies on count to detect old
> > > cookies).
> > > 
> > > Not having the counter frees up space to encode tcp options in the cookie
> > > instead of the timestamp.
> > 
> > But still wscale and sack options are disabled.
> 
> Yes, in Linux sack and wscale will be encoded in the timestamp, as
> cookie is already restricted to 24 bits due to counter.
> 
> Without the counter, that could be changed to allow sack/wscale even
> with ts off.

Another quick hack would be to allow sack being generated by the client.

If we receive sackOK in SYN, then syncookie SYNACK could contain sackOK,
if timestamps are not used.

Client would be allowed to use SACK in his ACK. Server would not
generate SACK, but would process incoming SACK.

Not sure what could break ?





--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller July 12, 2013, 11:37 p.m. UTC | #3
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 12 Jul 2013 07:32:43 -0700

> Another quick hack would be to allow sack being generated by the client.
> 
> If we receive sackOK in SYN, then syncookie SYNACK could contain sackOK,
> if timestamps are not used.
> 
> Client would be allowed to use SACK in his ACK. Server would not
> generate SACK, but would process incoming SACK.
> 
> Not sure what could break ?

This seems quite clumsy and would result in being able to use SACK only
in one direction.

There has to be a better way.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Frederic Sowa July 26, 2013, 6:45 a.m. UTC | #4
Hi Eric!

On Fri, Jul 12, 2013 at 07:04:45AM -0700, Eric Dumazet wrote:
> BTW, following patch allows to test more easily syncookies behavior.
> 
> If sysctl_tcp_syncookies is set to 2, we always use syncookies.
> 
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 35675e4..590659e 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1462,7 +1462,8 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
>  	 * limitations, they conserve resources and peer is
>  	 * evidently real one.
>  	 */
> -	if (inet_csk_reqsk_queue_is_full(sk) && !isn) {
> +	if ((sysctl_tcp_syncookies == 2 ||
> +	     inet_csk_reqsk_queue_is_full(sk)) && !isn) {
>  		want_cookie = tcp_syn_flood_action(sk, skb, "TCP");
>  		if (!want_cookie)
>  			goto drop;
> 

While cleaning up my patch directory I found this snippet. Perhaps you
could send it for inclusion for net-next? Three nice additions: a similar
change in tcp_ipv6.c and perhaps get rid of the warning messages printed
to the console in case of syncookies == 2? A small update to ip-sysctl.txt
wouldn't hurt either.

If you want to, I can take it and refresh it.

Thanks,

  Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet July 26, 2013, 12:56 p.m. UTC | #5
On Fri, 2013-07-26 at 08:45 +0200, Hannes Frederic Sowa wrote:
> Hi Eric!

Hi Hannes

> While cleaning up my patch directory I found this snippet. Perhaps you
> could send it for inclusion for net-next? Three nice additions: a similar
> change in tcp_ipv6.c and perhaps get rid of the warning messages printed
> to the console in case of syncookies == 2? A small update to ip-sysctl.txt
> wouldn't hurt either.
> 
> If you want to, I can take it and refresh it.

That would be a good idea, as I am currently traveling ;)

Thanks !


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 35675e4..590659e 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1462,7 +1462,8 @@  int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	 * limitations, they conserve resources and peer is
 	 * evidently real one.
 	 */
-	if (inet_csk_reqsk_queue_is_full(sk) && !isn) {
+	if ((sysctl_tcp_syncookies == 2 ||
+	     inet_csk_reqsk_queue_is_full(sk)) && !isn) {
 		want_cookie = tcp_syn_flood_action(sk, skb, "TCP");
 		if (!want_cookie)
 			goto drop;