[PATCH] TCP ECN bits and TCP_RESERVED_BITS macro

[PATCH] TCP ECN bits and TCP_RESERVED_BITS macro

[Guillaume Morin]

Hi folks,

As most people here know, RFC3168 adds two new bits to the TCP header
(ECE and CWR). Those were reserved in RFC793 (standard for TCP).

Since RFC3168 is an proposed standard and ECN is now used widely on
Linux systems, I'd like to suggest the modification of the
TCP_RESERVED_BITS. This change seems logical wrt

include/linux/tcp.h the tcphdr struct :
    __u16   doff:4,
	        res1:4,
	        cwr:1,  
	        ece:1,

This change is pretty harmless, since, this macro is used in

1) include/net/tcp_ecn.h. I've patched the related part even if it would
have work without. It is just cleaner.

2) netfilter: 
 - in the LOG target, it won't break them. I'll submit patches
   to netfilter-devel to display TCP ECN bits just like any other TCP flags
   (which will ease the LOG readings)
 - In the unclean target where the current value breaks the module.

Patch against 2.4.14-pre6


diff -uNr linux/include/linux/tcp.h linux-new-tcprb/include/linux/tcp.h
--- linux/include/linux/tcp.h	Sat Apr 28 00:48:20 2001
+++ linux-new-tcprb/include/linux/tcp.h	Wed Oct 31 14:51:40 2001
@@ -110,7 +110,7 @@
 	TCP_FLAG_RST = __constant_htonl(0x00040000), 
 	TCP_FLAG_SYN = __constant_htonl(0x00020000), 
 	TCP_FLAG_FIN = __constant_htonl(0x00010000),
-	TCP_RESERVED_BITS = __constant_htonl(0x0FC00000),
+	TCP_RESERVED_BITS = __constant_htonl(0x0F000000),
 	TCP_DATA_OFFSET = __constant_htonl(0xF0000000)
 }; 
 
diff -uNr linux/include/net/tcp_ecn.h linux-new-tcprb/include/net/tcp_ecn.h
--- linux/include/net/tcp_ecn.h	Wed Oct 31 14:57:53 2001
+++ linux-new-tcprb/include/net/tcp_ecn.h	Wed Oct 31 14:50:46 2001
@@ -3,7 +3,7 @@
 
 #include <net/inet_ecn.h>
 
-#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH)|TCP_FLAG_ECE|TCP_FLAG_CWR)
+#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH))
 
 #define	TCP_ECN_OK		1
 #define TCP_ECN_QUEUE_CWR	2


All comments welcome.

-- 
Guillaume Morin <guillaume@morinfr.org>

         Do you worry that you're not liked ? How long till you break
                                (Our Lady Peace)

Re: [PATCH] TCP ECN bits and TCP_RESERVED_BITS macro

[Neil Spring]

The line in your patch:

> -#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH)|TCP_FLAG_ECE|TCP_FLAG_CWR)
> +#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH))

is, I believe, a very bad idea.  This preprocessor constant
is used so that exceptional ECN processing (to handle ECE
or CWR) can be done on the slow path.  This change would
almost certainly break Linux's ECN implementation.

> 2) netfilter: 

I see no "2" patch to netfilter code.

-neil


On Wed, Oct 31, 2001 at 03:27:17PM +0100, Guillaume Morin wrote:
> Hi folks,
> 
> As most people here know, RFC3168 adds two new bits to the TCP header
> (ECE and CWR). Those were reserved in RFC793 (standard for TCP).
> 
> Since RFC3168 is an proposed standard and ECN is now used widely on
> Linux systems, I'd like to suggest the modification of the
> TCP_RESERVED_BITS. This change seems logical wrt
> 
> include/linux/tcp.h the tcphdr struct :
>     __u16   doff:4,
> 	        res1:4,
> 	        cwr:1,  
> 	        ece:1,
> 
> This change is pretty harmless, since, this macro is used in
> 
> 1) include/net/tcp_ecn.h. I've patched the related part even if it would
> have work without. It is just cleaner.
> 
> 2) netfilter: 
>  - in the LOG target, it won't break them. I'll submit patches
>    to netfilter-devel to display TCP ECN bits just like any other TCP flags
>    (which will ease the LOG readings)
>  - In the unclean target where the current value breaks the module.
> 
> Patch against 2.4.14-pre6
> 
> 
> diff -uNr linux/include/linux/tcp.h linux-new-tcprb/include/linux/tcp.h
> --- linux/include/linux/tcp.h	Sat Apr 28 00:48:20 2001
> +++ linux-new-tcprb/include/linux/tcp.h	Wed Oct 31 14:51:40 2001
> @@ -110,7 +110,7 @@
>  	TCP_FLAG_RST = __constant_htonl(0x00040000), 
>  	TCP_FLAG_SYN = __constant_htonl(0x00020000), 
>  	TCP_FLAG_FIN = __constant_htonl(0x00010000),
> -	TCP_RESERVED_BITS = __constant_htonl(0x0FC00000),
> +	TCP_RESERVED_BITS = __constant_htonl(0x0F000000),
>  	TCP_DATA_OFFSET = __constant_htonl(0xF0000000)
>  }; 
>  
> diff -uNr linux/include/net/tcp_ecn.h linux-new-tcprb/include/net/tcp_ecn.h
> --- linux/include/net/tcp_ecn.h	Wed Oct 31 14:57:53 2001
> +++ linux-new-tcprb/include/net/tcp_ecn.h	Wed Oct 31 14:50:46 2001
> @@ -3,7 +3,7 @@
>  
>  #include <net/inet_ecn.h>
>  
> -#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH)|TCP_FLAG_ECE|TCP_FLAG_CWR)
> +#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH))
>  
>  #define	TCP_ECN_OK		1
>  #define TCP_ECN_QUEUE_CWR	2
> 
> 
> All comments welcome.
> 
> -- 
> Guillaume Morin <guillaume@morinfr.org>
> 
>          Do you worry that you're not liked ? How long till you break
>                                 (Our Lady Peace)
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] TCP ECN bits and TCP_RESERVED_BITS macro

[Guillaume Morin]

Dans un message du 31 oct à 15:43, Neil Spring écrivait :
> The line in your patch:
> 
> > -#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH)|TCP_FLAG_ECE|TCP_FLAG_CWR)
> > +#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH))
> 
> is, I believe, a very bad idea.  This preprocessor constant
> is used so that exceptional ECN processing (to handle ECE
> or CWR) can be done on the slow path.  This change would
> almost certainly break Linux's ECN implementation.

Well it is not. I already had that discussion in private with someone
else. 
In previous versions (changed in a 14pre), there was two definitions for
TCP_HP_BITS. Now there is _just_ this one.

~(OLD_TCP_RESERVED_BITS)|TCP_FLAG_ECE_|TCP_FLAG_CWR == ~(NEW_TCP_RESERVED_BITS)

with
OLD_TCP_RESERVED_BITS = __constant_htonl(0x0FC00000),
NEW_TCP_RESERVED_BITS = __constant_htonl(0x0F000000),

So you see that I did _not_ change the value of TCP_HP_BITS. You surely
have noticed that change was not 'needed' since 
~(NEW_TCP_RESERVED_BITS) == (~(NEW_TCP_RESERVED_BITS)|TCP_FLAG_ECE|TCP_FLAG_CWR)

But it is _much_ cleaner that way (at least imho)

> I see no "2" patch to netfilter code.

As I've stated, this change fixes the unclean target. Furthermore, some
cosmetic changes must be done to the LOG targets. It is trivial and
should be posted to netfilter-devel. I will do it as soon as this patch
will be accepted. But if you want to see a preview of those, just drop
me a message.

Regards,

-- 
Guillaume Morin <guillaume@morinfr.org>

          5 years from now everyone will be running free GNU on their
           200 MIPS, 64M SPARCstation-5 (Andy Tanenbaum, 30 Jan 1992)

Re: [PATCH] TCP ECN bits and TCP_RESERVED_BITS macro

[Neil Spring]

> Dans un message du 31 oct ? 15:43, Neil Spring ?crivait :
> > The line in your patch:
> > > -#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH)|TCP_FLAG_ECE|TCP_FLAG_CWR)
> > > +#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH))
> >
> > is, I believe, a very bad idea.  This preprocessor constant
>
> Well it is not. 

You're absolutely right.  And it is more clean, even if
it confused me at first.  Thanks for the explanation.

-neil
nethack told me to be careful, full moon tonight, but I
didn't listen.