[PATCH nft 1/2] src: add dscp support

[PATCH nft 1/2] src: add dscp support

[Pablo Neira Ayuso]

This supports both IPv4:

 # nft --debug=netlink add rule filter forward ip dscp cs1 counter
 ip filter forward
  [ payload load 1b @ network header + 1 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x0000003f ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x00000008 ]
  [ counter pkts 0 bytes 0 ]

And IPv6:

 # nft --debug=netlink add rule ip6 filter forward ip6 dscp cs1 counter
 ip6 filter forward
  [ payload load 1b @ network header + 1 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x0000003f ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x00000008 ]
  [ counter pkts 0 bytes 0 ]

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 doc/nft.xml        | 11 ++++++++---
 include/datatype.h |  2 ++
 include/proto.h    |  3 ++-
 src/parser_bison.y |  5 +++--
 src/proto.c        | 44 +++++++++++++++++++++++++++++++++++++++++++-
 src/scanner.l      |  2 +-
 6 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/doc/nft.xml b/doc/nft.xml
index d51876c..4ede9e1 100644
--- a/doc/nft.xml
+++ b/doc/nft.xml
@@ -1337,9 +1337,9 @@ filter output oif eth0
 								<entry>integer (4 bit) FIXME scaling</entry>
 							</row>
 							<row>
-								<entry>tos</entry>
-								<entry>Type Of Service</entry>
-								<entry>FIXME</entry>
+								<entry>dscp</entry>
+								<entry>Differentiated Services Code Point</entry>
+								<entry>integer (6 bit)</entry>
 							</row>
 							<row>
 								<entry>length</entry>
@@ -1421,6 +1421,11 @@ filter output oif eth0
 								<entry></entry>
 							</row>
 							<row>
+								<entry>dscp</entry>
+								<entry>Differentiated Services Code Point</entry>
+								<entry>integer (6 bit)</entry>
+							</row>
+							<row>
 								<entry>flowlabel</entry>
 								<entry>Flow label</entry>
 								<entry></entry>
diff --git a/include/datatype.h b/include/datatype.h
index 07fedce..328e12a 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -40,6 +40,7 @@
  * @TYPE_ICMPV6_CODE:	icmpv6 code (integer subtype)
  * @TYPE_ICMPX_CODE:	icmpx code (integer subtype)
  * @TYPE_DEVGROUP:	devgroup code (integer subtype)
+ * @TYPE_DSCP:		Differentiated Services Code Point (integer subtype)
  */
 enum datatypes {
 	TYPE_INVALID,
@@ -78,6 +79,7 @@ enum datatypes {
 	TYPE_ICMPV6_CODE,
 	TYPE_ICMPX_CODE,
 	TYPE_DEVGROUP,
+	TYPE_DSCP,
 	__TYPE_MAX
 };
 #define TYPE_MAX		(__TYPE_MAX - 1)
diff --git a/include/proto.h b/include/proto.h
index a43bf98..41af0c1 100644
--- a/include/proto.h
+++ b/include/proto.h
@@ -176,7 +176,7 @@ enum ip_hdr_fields {
 	IPHDR_INVALID,
 	IPHDR_VERSION,
 	IPHDR_HDRLENGTH,
-	IPHDR_TOS,
+	IPHDR_DSCP,
 	IPHDR_LENGTH,
 	IPHDR_ID,
 	IPHDR_FRAG_OFF,
@@ -214,6 +214,7 @@ enum ip6_hdr_fields {
 	IP6HDR_INVALID,
 	IP6HDR_VERSION,
 	IP6HDR_PRIORITY,
+	IP6HDR_DSCP,
 	IP6HDR_FLOWLABEL,
 	IP6HDR_LENGTH,
 	IP6HDR_NEXTHDR,
diff --git a/src/parser_bison.y b/src/parser_bison.y
index e72d24d..7a5d7f8 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -246,7 +246,7 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %token IP			"ip"
 %token HDRVERSION		"version"
 %token HDRLENGTH		"hdrlength"
-%token TOS			"tos"
+%token DSCP			"dscp"
 %token LENGTH			"length"
 %token FRAG_OFF			"frag-off"
 %token TTL			"ttl"
@@ -2172,7 +2172,7 @@ ip_hdr_expr		:	IP	ip_hdr_field
 
 ip_hdr_field		:	HDRVERSION	{ $$ = IPHDR_VERSION; }
 			|	HDRLENGTH	{ $$ = IPHDR_HDRLENGTH; }
-			|	TOS		{ $$ = IPHDR_TOS; }
+			|	DSCP		{ $$ = IPHDR_DSCP; }
 			|	LENGTH		{ $$ = IPHDR_LENGTH; }
 			|	ID		{ $$ = IPHDR_ID; }
 			|	FRAG_OFF	{ $$ = IPHDR_FRAG_OFF; }
@@ -2219,6 +2219,7 @@ ip6_hdr_expr		:	IP6	ip6_hdr_field
 
 ip6_hdr_field		:	HDRVERSION	{ $$ = IP6HDR_VERSION; }
 			|	PRIORITY	{ $$ = IP6HDR_PRIORITY; }
+			|	DSCP		{ $$ = IP6HDR_DSCP; }
 			|	FLOWLABEL	{ $$ = IP6HDR_FLOWLABEL; }
 			|	LENGTH		{ $$ = IP6HDR_LENGTH; }
 			|	NEXTHDR		{ $$ = IP6HDR_NEXTHDR; }
diff --git a/src/proto.c b/src/proto.c
index 28b93cb..0e5932d 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -483,6 +483,46 @@ const struct proto_desc proto_sctp = {
  */
 
 #include <netinet/ip.h>
+
+static const struct symbol_table dscp_type_tbl = {
+	.symbols	= {
+		SYMBOL("cs0",	0x00),
+		SYMBOL("cs1",	0x08),
+		SYMBOL("cs2",	0x10),
+		SYMBOL("cs3",	0x18),
+		SYMBOL("cs4",	0x20),
+		SYMBOL("cs5",	0x28),
+		SYMBOL("cs6",	0x30),
+		SYMBOL("cs7",	0x38),
+		SYMBOL("be",	0x00),
+		SYMBOL("af11",	0x0a),
+		SYMBOL("af12",	0x0c),
+		SYMBOL("af13",	0x0e),
+		SYMBOL("af21",	0x12),
+		SYMBOL("af22",	0x14),
+		SYMBOL("af23",	0x16),
+		SYMBOL("af31",	0x1a),
+		SYMBOL("af32",	0x1c),
+		SYMBOL("af33",	0x1e),
+		SYMBOL("af41",	0x22),
+		SYMBOL("af42",	0x24),
+		SYMBOL("af43",	0x26),
+		SYMBOL("ef",	0x2e),
+		SYMBOL_LIST_END
+	},
+};
+
+static const struct datatype dscp_type = {
+	.type		= TYPE_DSCP,
+	.name		= "dscp_type",
+	.desc		= "differentiated services code point",
+	.byteorder	= BYTEORDER_BIG_ENDIAN,
+	.size		= 6,
+	.basetype	= &integer_type,
+	.basefmt	= "0x%.2Zx",
+	.sym_tbl	= &dscp_type_tbl,
+};
+
 #define IPHDR_FIELD(__name, __member) \
 	HDR_FIELD(__name, struct iphdr, __member)
 #define IPHDR_ADDR(__name, __member) \
@@ -506,7 +546,7 @@ const struct proto_desc proto_ip = {
 	.templates	= {
 		[IPHDR_VERSION]		= HDR_BITFIELD("version", &integer_type, 4, 4),
 		[IPHDR_HDRLENGTH]	= HDR_BITFIELD("hdrlength", &integer_type, 0, 4),
-		[IPHDR_TOS]		= IPHDR_FIELD("tos",		tos),
+		[IPHDR_DSCP]		= HDR_BITFIELD("dscp", &dscp_type, 8, 6),
 		[IPHDR_LENGTH]		= IPHDR_FIELD("length",		tot_len),
 		[IPHDR_ID]		= IPHDR_FIELD("id",		id),
 		[IPHDR_FRAG_OFF]	= IPHDR_FIELD("frag-off",	frag_off),
@@ -604,6 +644,7 @@ const struct proto_desc proto_ip6 = {
 	.templates	= {
 		[IP6HDR_VERSION]	= HDR_BITFIELD("version", &integer_type, 0, 4),
 		[IP6HDR_PRIORITY]	= HDR_BITFIELD("priority", &integer_type, 4, 4),
+		[IP6HDR_DSCP]		= HDR_BITFIELD("dscp", &dscp_type, 8, 6),
 		[IP6HDR_FLOWLABEL]	= IP6HDR_FIELD("flowlabel",	flow_lbl),
 		[IP6HDR_LENGTH]		= IP6HDR_FIELD("length",	payload_len),
 		[IP6HDR_NEXTHDR]	= INET_PROTOCOL("nexthdr", struct ipv6hdr, nexthdr),
@@ -808,4 +849,5 @@ static void __init proto_init(void)
 	datatype_register(&arpop_type);
 	datatype_register(&ethertype_type);
 	datatype_register(&icmp6_type_type);
+	datatype_register(&dscp_type);
 }
diff --git a/src/scanner.l b/src/scanner.l
index 04fddc9..cb7ec4c 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -361,7 +361,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "ip"			{ return IP; }
 "version"		{ return HDRVERSION; }
 "hdrlength"		{ return HDRLENGTH; }
-"tos"			{ return TOS; }
+"dscp"			{ return DSCP; }
 "length"		{ return LENGTH; }
 "frag-off"		{ return FRAG_OFF; }
 "ttl"			{ return TTL; }
-- 
2.1.4

[PATCH nft 2/2] src: add ecn support

[Pablo Neira Ayuso]

This supports both IPv4:

 # nft --debug=netlink add rule ip filter forward ip ecn ce counter
 ip filter forward
  [ payload load 1b @ network header + 1 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x000000c0 ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x000000c0 ]
  [ counter pkts 0 bytes 0 ]

For IPv6:

 # nft --debug=netlink add rule ip6 filter forward ip6 ecn ce counter
 ip6 filter forward
  [ payload load 1b @ network header + 1 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x000000c0 ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x000000c0 ]
  [ counter pkts 0 bytes 0 ]

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 doc/nft.xml        | 10 ++++++++++
 include/datatype.h |  1 +
 include/proto.h    |  2 ++
 src/parser_bison.y |  3 +++
 src/proto.c        | 24 ++++++++++++++++++++++++
 src/scanner.l      |  1 +
 6 files changed, 41 insertions(+)

diff --git a/doc/nft.xml b/doc/nft.xml
index 4ede9e1..1bce4b3 100644
--- a/doc/nft.xml
+++ b/doc/nft.xml
@@ -1342,6 +1342,11 @@ filter output oif eth0
 								<entry>integer (6 bit)</entry>
 							</row>
 							<row>
+								<entry>ecn</entry>
+								<entry>Explicit Congestion Notification</entry>
+								<entry>integer (2 bit)</entry>
+							</row>
+							<row>
 								<entry>length</entry>
 								<entry>Total packet length</entry>
 								<entry>integer (16 bit)</entry>
@@ -1426,6 +1431,11 @@ filter output oif eth0
 								<entry>integer (6 bit)</entry>
 							</row>
 							<row>
+								<entry>ecn</entry>
+								<entry>Explicit Congestion Notification</entry>
+								<entry>integer (2 bit)</entry>
+							</row>
+							<row>
 								<entry>flowlabel</entry>
 								<entry>Flow label</entry>
 								<entry></entry>
diff --git a/include/datatype.h b/include/datatype.h
index 328e12a..31925b4 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -80,6 +80,7 @@ enum datatypes {
 	TYPE_ICMPX_CODE,
 	TYPE_DEVGROUP,
 	TYPE_DSCP,
+	TYPE_ECN,
 	__TYPE_MAX
 };
 #define TYPE_MAX		(__TYPE_MAX - 1)
diff --git a/include/proto.h b/include/proto.h
index 41af0c1..914c292 100644
--- a/include/proto.h
+++ b/include/proto.h
@@ -177,6 +177,7 @@ enum ip_hdr_fields {
 	IPHDR_VERSION,
 	IPHDR_HDRLENGTH,
 	IPHDR_DSCP,
+	IPHDR_ECN,
 	IPHDR_LENGTH,
 	IPHDR_ID,
 	IPHDR_FRAG_OFF,
@@ -215,6 +216,7 @@ enum ip6_hdr_fields {
 	IP6HDR_VERSION,
 	IP6HDR_PRIORITY,
 	IP6HDR_DSCP,
+	IP6HDR_ECN,
 	IP6HDR_FLOWLABEL,
 	IP6HDR_LENGTH,
 	IP6HDR_NEXTHDR,
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 7a5d7f8..67c6009 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -247,6 +247,7 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %token HDRVERSION		"version"
 %token HDRLENGTH		"hdrlength"
 %token DSCP			"dscp"
+%token ECN			"ecn"
 %token LENGTH			"length"
 %token FRAG_OFF			"frag-off"
 %token TTL			"ttl"
@@ -2173,6 +2174,7 @@ ip_hdr_expr		:	IP	ip_hdr_field
 ip_hdr_field		:	HDRVERSION	{ $$ = IPHDR_VERSION; }
 			|	HDRLENGTH	{ $$ = IPHDR_HDRLENGTH; }
 			|	DSCP		{ $$ = IPHDR_DSCP; }
+			|	ECN		{ $$ = IPHDR_ECN; }
 			|	LENGTH		{ $$ = IPHDR_LENGTH; }
 			|	ID		{ $$ = IPHDR_ID; }
 			|	FRAG_OFF	{ $$ = IPHDR_FRAG_OFF; }
@@ -2220,6 +2222,7 @@ ip6_hdr_expr		:	IP6	ip6_hdr_field
 ip6_hdr_field		:	HDRVERSION	{ $$ = IP6HDR_VERSION; }
 			|	PRIORITY	{ $$ = IP6HDR_PRIORITY; }
 			|	DSCP		{ $$ = IP6HDR_DSCP; }
+			|	ECN		{ $$ = IP6HDR_ECN; }
 			|	FLOWLABEL	{ $$ = IP6HDR_FLOWLABEL; }
 			|	LENGTH		{ $$ = IP6HDR_LENGTH; }
 			|	NEXTHDR		{ $$ = IP6HDR_NEXTHDR; }
diff --git a/src/proto.c b/src/proto.c
index 0e5932d..a1f5e75 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -523,6 +523,27 @@ static const struct datatype dscp_type = {
 	.sym_tbl	= &dscp_type_tbl,
 };
 
+static const struct symbol_table ecn_type_tbl = {
+	.symbols	= {
+		SYMBOL("non-ect",	0x00),
+		SYMBOL("ect1",		0x01),
+		SYMBOL("ect0",		0x02),
+		SYMBOL("ce",		0x03),
+		SYMBOL_LIST_END
+	},
+};
+
+static const struct datatype ecn_type = {
+	.type		= TYPE_ECN,
+	.name		= "ecn_type",
+	.desc		= "explicit congestion notification",
+	.byteorder	= BYTEORDER_BIG_ENDIAN,
+	.size		= 2,
+	.basetype	= &integer_type,
+	.basefmt	= "0x%.1Zx",
+	.sym_tbl	= &ecn_type_tbl,
+};
+
 #define IPHDR_FIELD(__name, __member) \
 	HDR_FIELD(__name, struct iphdr, __member)
 #define IPHDR_ADDR(__name, __member) \
@@ -547,6 +568,7 @@ const struct proto_desc proto_ip = {
 		[IPHDR_VERSION]		= HDR_BITFIELD("version", &integer_type, 4, 4),
 		[IPHDR_HDRLENGTH]	= HDR_BITFIELD("hdrlength", &integer_type, 0, 4),
 		[IPHDR_DSCP]		= HDR_BITFIELD("dscp", &dscp_type, 8, 6),
+		[IPHDR_ECN]		= HDR_BITFIELD("ecn", &ecn_type, 14, 2),
 		[IPHDR_LENGTH]		= IPHDR_FIELD("length",		tot_len),
 		[IPHDR_ID]		= IPHDR_FIELD("id",		id),
 		[IPHDR_FRAG_OFF]	= IPHDR_FIELD("frag-off",	frag_off),
@@ -645,6 +667,7 @@ const struct proto_desc proto_ip6 = {
 		[IP6HDR_VERSION]	= HDR_BITFIELD("version", &integer_type, 0, 4),
 		[IP6HDR_PRIORITY]	= HDR_BITFIELD("priority", &integer_type, 4, 4),
 		[IP6HDR_DSCP]		= HDR_BITFIELD("dscp", &dscp_type, 8, 6),
+		[IP6HDR_ECN]		= HDR_BITFIELD("ecn", &ecn_type, 14, 2),
 		[IP6HDR_FLOWLABEL]	= IP6HDR_FIELD("flowlabel",	flow_lbl),
 		[IP6HDR_LENGTH]		= IP6HDR_FIELD("length",	payload_len),
 		[IP6HDR_NEXTHDR]	= INET_PROTOCOL("nexthdr", struct ipv6hdr, nexthdr),
@@ -850,4 +873,5 @@ static void __init proto_init(void)
 	datatype_register(&ethertype_type);
 	datatype_register(&icmp6_type_type);
 	datatype_register(&dscp_type);
+	datatype_register(&ecn_type);
 }
diff --git a/src/scanner.l b/src/scanner.l
index cb7ec4c..54063bf 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -362,6 +362,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "version"		{ return HDRVERSION; }
 "hdrlength"		{ return HDRLENGTH; }
 "dscp"			{ return DSCP; }
+"ecn"			{ return ECN; }
 "length"		{ return LENGTH; }
 "frag-off"		{ return FRAG_OFF; }
 "ttl"			{ return TTL; }
-- 
2.1.4

Re: [PATCH nft 1/2] src: add dscp support

[Patrick McHardy]

On 25.11, Pablo Neira Ayuso wrote:
>  #define TYPE_MAX		(__TYPE_MAX - 1)
> diff --git a/include/proto.h b/include/proto.h
> index a43bf98..41af0c1 100644
> --- a/include/proto.h
> +++ b/include/proto.h
> @@ -176,7 +176,7 @@ enum ip_hdr_fields {
>  	IPHDR_INVALID,
>  	IPHDR_VERSION,
>  	IPHDR_HDRLENGTH,
> -	IPHDR_TOS,
> +	IPHDR_DSCP,

Shouldn't we keep both? I think it should work to do that or at least not be
difficult to make it work.

> diff --git a/src/proto.c b/src/proto.c
> index 28b93cb..0e5932d 100644
> --- a/src/proto.c
> +++ b/src/proto.c
> ...
> +static const struct datatype dscp_type = {
> +	.type		= TYPE_DSCP,
> +	.name		= "dscp_type",

We usually only add a _type suffix if that's really a part of the name,
pkt dccp_pkttype.

Re: [PATCH nft 2/2] src: add ecn support

[Patrick McHardy]

On 25.11, Pablo Neira Ayuso wrote:
> diff --git a/src/proto.c b/src/proto.c
> index 0e5932d..a1f5e75 100644
> --- a/src/proto.c
> +++ b/src/proto.c
> @@ -523,6 +523,27 @@ static const struct datatype dscp_type = {
> +...
> +static const struct datatype ecn_type = {
> +	.type		= TYPE_ECN,
> +	.name		= "ecn_type",

Same here (_type).

Otherwise looks good to me, thanks.

Re: [PATCH nft 1/2] src: add dscp support

[Pablo Neira Ayuso]

On Wed, Nov 25, 2015 at 08:01:25PM +0000, Patrick McHardy wrote:
> On 25.11, Pablo Neira Ayuso wrote:
> >  #define TYPE_MAX		(__TYPE_MAX - 1)
> > diff --git a/include/proto.h b/include/proto.h
> > index a43bf98..41af0c1 100644
> > --- a/include/proto.h
> > +++ b/include/proto.h
> > @@ -176,7 +176,7 @@ enum ip_hdr_fields {
> >  	IPHDR_INVALID,
> >  	IPHDR_VERSION,
> >  	IPHDR_HDRLENGTH,
> > -	IPHDR_TOS,
> > +	IPHDR_DSCP,
> 
> Shouldn't we keep both? I think it should work to do that or at least not be
> difficult to make it work.

Last time we discussed this, people arised concerns related to the
fact that this is obsolete, and nobody should be using this these
days.

https://patchwork.ozlabs.org/patch/352016/

Re: [PATCH nft 1/2] src: add dscp support

[Patrick McHardy]

On 25.11, Pablo Neira Ayuso wrote:
> On Wed, Nov 25, 2015 at 08:01:25PM +0000, Patrick McHardy wrote:
> > On 25.11, Pablo Neira Ayuso wrote:
> > >  #define TYPE_MAX		(__TYPE_MAX - 1)
> > > diff --git a/include/proto.h b/include/proto.h
> > > index a43bf98..41af0c1 100644
> > > --- a/include/proto.h
> > > +++ b/include/proto.h
> > > @@ -176,7 +176,7 @@ enum ip_hdr_fields {
> > >  	IPHDR_INVALID,
> > >  	IPHDR_VERSION,
> > >  	IPHDR_HDRLENGTH,
> > > -	IPHDR_TOS,
> > > +	IPHDR_DSCP,
> > 
> > Shouldn't we keep both? I think it should work to do that or at least not be
> > difficult to make it work.
> 
> Last time we discussed this, people arised concerns related to the
> fact that this is obsolete, and nobody should be using this these
> days.

I don't really buy into these arguments. We support lots of obsolete things,
the fact is that even though it is obsoleted, it still exists and for old
devices might even be the only thing they support.

Re: [PATCH nft 1/2] src: add dscp support

[Pablo Neira Ayuso]

On Wed, Nov 25, 2015 at 08:45:13PM +0000, Patrick McHardy wrote:
> On 25.11, Pablo Neira Ayuso wrote:
> > On Wed, Nov 25, 2015 at 08:01:25PM +0000, Patrick McHardy wrote:
> > > On 25.11, Pablo Neira Ayuso wrote:
> > > >  #define TYPE_MAX		(__TYPE_MAX - 1)
> > > > diff --git a/include/proto.h b/include/proto.h
> > > > index a43bf98..41af0c1 100644
> > > > --- a/include/proto.h
> > > > +++ b/include/proto.h
> > > > @@ -176,7 +176,7 @@ enum ip_hdr_fields {
> > > >  	IPHDR_INVALID,
> > > >  	IPHDR_VERSION,
> > > >  	IPHDR_HDRLENGTH,
> > > > -	IPHDR_TOS,
> > > > +	IPHDR_DSCP,
> > > 
> > > Shouldn't we keep both? I think it should work to do that or at least not be
> > > difficult to make it work.
> > 
> > Last time we discussed this, people arised concerns related to the
> > fact that this is obsolete, and nobody should be using this these
> > days.
> 
> I don't really buy into these arguments. We support lots of obsolete things,
> the fact is that even though it is obsoleted, it still exists and for old
> devices might even be the only thing they support.

ToS breaks useful things like ECN, and the more I keep reading docs on
the Internet, the more problem I have to see how the user can benefit
from this.

Re: [PATCH nft 1/2] src: add dscp support

[Jan Engelhardt]

On Wednesday 2015-11-25 21:01, Patrick McHardy wrote:
>On 25.11, Pablo Neira Ayuso wrote:
>>  #define TYPE_MAX		(__TYPE_MAX - 1)
>> diff --git a/include/proto.h b/include/proto.h
>> index a43bf98..41af0c1 100644
>> --- a/include/proto.h
>> +++ b/include/proto.h
>> @@ -176,7 +176,7 @@ enum ip_hdr_fields {
>>  	IPHDR_INVALID,
>>  	IPHDR_VERSION,
>>  	IPHDR_HDRLENGTH,
>> -	IPHDR_TOS,
>> +	IPHDR_DSCP,
>
>Shouldn't we keep both?

>From what I read from RFC 2474, DSCP is the 6-bit portion.
The 8-bit field is known as
  (a) TOS per IPv4 RFC
  (b) TC (Traffic Class) per IPv6 RFC 2460
  (c) DS per RFC 2472

I would therefore recommend against IPHDR_DSCP, and instead go
with either of IPHDR_TOS, IPHDR_DS, IPHDR_DIFFSERV, or even
some mnemonic involving traffic class.

Re: [PATCH nft 1/2] src: add dscp support

[Patrick McHardy]

On 25.11, Pablo Neira Ayuso wrote:
> On Wed, Nov 25, 2015 at 08:45:13PM +0000, Patrick McHardy wrote:
> > On 25.11, Pablo Neira Ayuso wrote:
> > > On Wed, Nov 25, 2015 at 08:01:25PM +0000, Patrick McHardy wrote:
> > > > On 25.11, Pablo Neira Ayuso wrote:
> > > > >  #define TYPE_MAX		(__TYPE_MAX - 1)
> > > > > diff --git a/include/proto.h b/include/proto.h
> > > > > index a43bf98..41af0c1 100644
> > > > > --- a/include/proto.h
> > > > > +++ b/include/proto.h
> > > > > @@ -176,7 +176,7 @@ enum ip_hdr_fields {
> > > > >  	IPHDR_INVALID,
> > > > >  	IPHDR_VERSION,
> > > > >  	IPHDR_HDRLENGTH,
> > > > > -	IPHDR_TOS,
> > > > > +	IPHDR_DSCP,
> > > > 
> > > > Shouldn't we keep both? I think it should work to do that or at least not be
> > > > difficult to make it work.
> > > 
> > > Last time we discussed this, people arised concerns related to the
> > > fact that this is obsolete, and nobody should be using this these
> > > days.
> > 
> > I don't really buy into these arguments. We support lots of obsolete things,
> > the fact is that even though it is obsoleted, it still exists and for old
> > devices might even be the only thing they support.
> 
> ToS breaks useful things like ECN, and the more I keep reading docs on
> the Internet, the more problem I have to see how the user can benefit
> from this.

We *match* on ToS, that cannot possibly break anything. Also I'm unsure how
this could break ECN even otherwise, ToS does not even use the ECN bits.

Its quite simple, if the user has old devices that set ToS values, he will
be able to match on that without manually converting it to DSCP values.
Given that our current tos definition is also not to practical for that
since its too broad, I don't really care that much, although I think it
should rather be fixed than simply thrown out.

Re: [PATCH nft 1/2] src: add dscp support

[Patrick McHardy]

On 26.11, Jan Engelhardt wrote:
> 
> On Wednesday 2015-11-25 21:01, Patrick McHardy wrote:
> >On 25.11, Pablo Neira Ayuso wrote:
> >>  #define TYPE_MAX		(__TYPE_MAX - 1)
> >> diff --git a/include/proto.h b/include/proto.h
> >> index a43bf98..41af0c1 100644
> >> --- a/include/proto.h
> >> +++ b/include/proto.h
> >> @@ -176,7 +176,7 @@ enum ip_hdr_fields {
> >>  	IPHDR_INVALID,
> >>  	IPHDR_VERSION,
> >>  	IPHDR_HDRLENGTH,
> >> -	IPHDR_TOS,
> >> +	IPHDR_DSCP,
> >
> >Shouldn't we keep both?
> 
> >From what I read from RFC 2474, DSCP is the 6-bit portion.
> The 8-bit field is known as
>   (a) TOS per IPv4 RFC
>   (b) TC (Traffic Class) per IPv6 RFC 2460
>   (c) DS per RFC 2472
> 
> I would therefore recommend against IPHDR_DSCP, and instead go
> with either of IPHDR_TOS, IPHDR_DS, IPHDR_DIFFSERV, or even
> some mnemonic involving traffic class.

But we actually do refer to the 6 bit sub-field and the further 2 bit ECN
sub-field, they are seperate fields with Pablo's patch.

Re: [PATCH nft 1/2] src: add dscp support

[Pablo Neira Ayuso]

On Wed, Nov 25, 2015 at 11:23:23PM +0000, Patrick McHardy wrote:
> On 25.11, Pablo Neira Ayuso wrote:
> > On Wed, Nov 25, 2015 at 08:45:13PM +0000, Patrick McHardy wrote:
> > > On 25.11, Pablo Neira Ayuso wrote:
> > > > On Wed, Nov 25, 2015 at 08:01:25PM +0000, Patrick McHardy wrote:
> > > > > On 25.11, Pablo Neira Ayuso wrote:
> > > > > >  #define TYPE_MAX		(__TYPE_MAX - 1)
> > > > > > diff --git a/include/proto.h b/include/proto.h
> > > > > > index a43bf98..41af0c1 100644
> > > > > > --- a/include/proto.h
> > > > > > +++ b/include/proto.h
> > > > > > @@ -176,7 +176,7 @@ enum ip_hdr_fields {
> > > > > >  	IPHDR_INVALID,
> > > > > >  	IPHDR_VERSION,
> > > > > >  	IPHDR_HDRLENGTH,
> > > > > > -	IPHDR_TOS,
> > > > > > +	IPHDR_DSCP,
> > > > > 
> > > > > Shouldn't we keep both? I think it should work to do that or at least not be
> > > > > difficult to make it work.
> > > > 
> > > > Last time we discussed this, people arised concerns related to the
> > > > fact that this is obsolete, and nobody should be using this these
> > > > days.
> > > 
> > > I don't really buy into these arguments. We support lots of obsolete things,
> > > the fact is that even though it is obsoleted, it still exists and for old
> > > devices might even be the only thing they support.
> > 
> > ToS breaks useful things like ECN, and the more I keep reading docs on
> > the Internet, the more problem I have to see how the user can benefit
> > from this.
> 
> We *match* on ToS, that cannot possibly break anything. Also I'm unsure how
> this could break ECN even otherwise, ToS does not even use the ECN bits.

ToS bits overlap with ECN bits, from that original ToS 8 bit-field now
we use 6 bit for DSCP and 2 bits for ECN.

> Its quite simple, if the user has old devices that set ToS values, he will
> be able to match on that without manually converting it to DSCP values.
> Given that our current tos definition is also not to practical for that
> since its too broad, I don't really care that much, although I think it
> should rather be fixed than simply thrown out.

People that designed DSCP and ECN did not care about having some
reasonable backward compatible behaviour wrt. ToS. They just changed
the semantics of those bits long time ago.

I can explore keeping this backward if you like, we can probably
accept ToS from the parser, then map it to DSCP, but that will no
achieve what the user expected on the network. I'm usually reticent to
break old stuff, but in this case I would skip.

Cheers,
Pablo

Re: [PATCH nft 1/2] src: add dscp support

[Patrick McHardy]

On 26.11, Pablo Neira Ayuso wrote:
> > > ToS breaks useful things like ECN, and the more I keep reading docs on
> > > the Internet, the more problem I have to see how the user can benefit
> > > from this.
> > 
> > We *match* on ToS, that cannot possibly break anything. Also I'm unsure how
> > this could break ECN even otherwise, ToS does not even use the ECN bits.
> 
> ToS bits overlap with ECN bits, from that original ToS 8 bit-field now
> we use 6 bit for DSCP and 2 bits for ECN.

Sure, but our ToS definition is wrong anyway, the ToS-bits are actually
3 + 3 + 2 unused bits (ECN).

> > Its quite simple, if the user has old devices that set ToS values, he will
> > be able to match on that without manually converting it to DSCP values.
> > Given that our current tos definition is also not to practical for that
> > since its too broad, I don't really care that much, although I think it
> > should rather be fixed than simply thrown out.
> 
> People that designed DSCP and ECN did not care about having some
> reasonable backward compatible behaviour wrt. ToS. They just changed
> the semantics of those bits long time ago.
> 
> I can explore keeping this backward if you like, we can probably
> accept ToS from the parser, then map it to DSCP, but that will no
> achieve what the user expected on the network. I'm usually reticent to
> break old stuff, but in this case I would skip.

My thought was more fixing our ToS field definition, at that point the user
can use whatever is actually used within his network. I mean, sure, you can
map them to DSCP, but if you're using old devices that only support the
ToS definitions its a lot easier to use the same values instead of mapping
them.

I don't know. I think it should be fairly easy to fix, so I'd prefer that
way I guess. Your choice.

Re: [PATCH nft 1/2] src: add dscp support

[Pablo Neira Ayuso]

On Thu, Nov 26, 2015 at 09:54:45AM +0000, Patrick McHardy wrote:
> On 26.11, Pablo Neira Ayuso wrote:
> > > > ToS breaks useful things like ECN, and the more I keep reading docs on
> > > > the Internet, the more problem I have to see how the user can benefit
> > > > from this.
> > > 
> > > We *match* on ToS, that cannot possibly break anything. Also I'm unsure how
> > > this could break ECN even otherwise, ToS does not even use the ECN bits.
> > 
> > ToS bits overlap with ECN bits, from that original ToS 8 bit-field now
> > we use 6 bit for DSCP and 2 bits for ECN.
> 
> Sure, but our ToS definition is wrong anyway, the ToS-bits are actually
> 3 + 3 + 2 unused bits (ECN).

That's right according to the original ToS definition, but looking a
bit further on the RFCs, someone outthere extended this, so I found
this:

See 22. Historical Definitions for the IPv4 TOS Octet from
https://tools.ietf.org/html/rfc3168.

It refers to https://tools.ietf.org/html/rfc1349 that defines
Minimize Monetary Cost TOS Value and it extends it to one extra bit
(from 3 to 4, scratching one from the unused bits).

Bottom line says "Because of this unstable history, the definition of
the ECN field in this document cannot be guaranteed to be backwards
compatible with all past uses of these two bits."

Anyway, looking at the Linux header definitions, we have the

#define IPTOS_MINCOST           0x02

The mincost thing is there.

#define IPTOS_TOS_MASK          0x1E
#define IPTOS_TOS(tos)          ((tos)&IPTOS_TOS_MASK)

So it seems our headers are considering ToS is 4 bits too.

Cheers,
Pablo

Re: [PATCH nft 1/2] src: add dscp support

[Patrick McHardy]

On 26.11, Pablo Neira Ayuso wrote:
> On Thu, Nov 26, 2015 at 09:54:45AM +0000, Patrick McHardy wrote:
> > On 26.11, Pablo Neira Ayuso wrote:
> > > > > ToS breaks useful things like ECN, and the more I keep reading docs on
> > > > > the Internet, the more problem I have to see how the user can benefit
> > > > > from this.
> > > > 
> > > > We *match* on ToS, that cannot possibly break anything. Also I'm unsure how
> > > > this could break ECN even otherwise, ToS does not even use the ECN bits.
> > > 
> > > ToS bits overlap with ECN bits, from that original ToS 8 bit-field now
> > > we use 6 bit for DSCP and 2 bits for ECN.
> > 
> > Sure, but our ToS definition is wrong anyway, the ToS-bits are actually
> > 3 + 3 + 2 unused bits (ECN).
> 
> That's right according to the original ToS definition, but looking a
> bit further on the RFCs, someone outthere extended this, so I found
> this:
> 
> See 22. Historical Definitions for the IPv4 TOS Octet from
> https://tools.ietf.org/html/rfc3168.
> 
> It refers to https://tools.ietf.org/html/rfc1349 that defines
> Minimize Monetary Cost TOS Value and it extends it to one extra bit
> (from 3 to 4, scratching one from the unused bits).
> 
> Bottom line says "Because of this unstable history, the definition of
> the ECN field in this document cannot be guaranteed to be backwards
> compatible with all past uses of these two bits."
> 
> Anyway, looking at the Linux header definitions, we have the
> 
> #define IPTOS_MINCOST           0x02
> 
> The mincost thing is there.
> 
> #define IPTOS_TOS_MASK          0x1E
> #define IPTOS_TOS(tos)          ((tos)&IPTOS_TOS_MASK)
> 
> So it seems our headers are considering ToS is 4 bits too.

Hmm Ok its really a mess. Let's get rid of it for now and we might reconsider
if a user turns up that can make a valid case.