[PATCH nft 0/7] rework tcp option handling

netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH nft 0/7] rework tcp option handling
@ 2020-11-05 14:11 Florian Westphal
  2020-11-05 14:11 ` [PATCH nft 1/7] parser: merge sack-perm/sack-permitted and maxseg/mss Florian Westphal
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: Florian Westphal @ 2020-11-05 14:11 UTC (permalink / raw)
  To: netfilter-devel

This reworks how tcp options are handled in nft internally.
First patches refactor and condense code.

In particular, it removes the duplication of 'sack-perm'/permitted
maxseg/mss lexer keys -- synproxy and tcp option used different tokens,
leading to confusing sytax errors when using the 'wrong' word in the
'wrong' place.

patch 5 is the first one with a new feature: it allows to check for
presence of any tcp option kind, i.e. 'tcp option $number'.
patch 6 and 7 add 'raw' payload matching for tcp options to allow
testing for tcp options that do not have an internal template.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH nft 1/7] parser: merge sack-perm/sack-permitted and maxseg/mss
  2020-11-05 14:11 [PATCH nft 0/7] rework tcp option handling Florian Westphal
@ 2020-11-05 14:11 ` Florian Westphal
  2020-11-05 15:22   ` Jeremy Sowden
  2020-11-05 14:11 ` [PATCH nft 2/7] tcpopts: clean up parser -> tcpopt.c plumbing Florian Westphal
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Florian Westphal @ 2020-11-05 14:11 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

One was added by the tcp option parsing ocde, the other by synproxy.

So we have:
synproxy ... sack-perm
synproxy ... mss

and

tcp option maxseg
tcp option sack-permitted

This kills the extra tokens on the scanner/parser side,
so sack-perm and sack-permitted can both be used.

Likewise, 'synproxy maxseg' and 'tcp option mss size 42' will
work too.  On the output side, the shorter form is now preferred,
i.e. sack-perm and mss.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 doc/payload-expression.txt    |  8 ++++----
 src/parser_bison.y            | 13 ++++++-------
 src/scanner.l                 |  8 ++++----
 src/tcpopt.c                  |  2 +-
 tests/py/any/tcpopt.t         |  4 ++--
 tests/py/any/tcpopt.t.json    |  8 ++++----
 tests/py/any/tcpopt.t.payload | 12 ++++++------
 7 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/doc/payload-expression.txt b/doc/payload-expression.txt
index 93d4d22f59f5..9df20a18ae8a 100644
--- a/doc/payload-expression.txt
+++ b/doc/payload-expression.txt
@@ -525,13 +525,13 @@ nftables currently supports matching (finding) a given ipv6 extension header, TC
 *dst* {*nexthdr* | *hdrlength*}
 *mh* {*nexthdr* | *hdrlength* | *checksum* | *type*}
 *srh* {*flags* | *tag* | *sid* | *seg-left*}
-*tcp option* {*eol* | *noop* | *maxseg* | *window* | *sack-permitted* | *sack* | *sack0* | *sack1* | *sack2* | *sack3* | *timestamp*} 'tcp_option_field'
+*tcp option* {*eol* | *noop* | *maxseg* | *window* | *sack-perm* | *sack* | *sack0* | *sack1* | *sack2* | *sack3* | *timestamp*} 'tcp_option_field'
 *ip option* { lsrr | ra | rr | ssrr } 'ip_option_field'
 
 The following syntaxes are valid only in a relational expression with boolean type on right-hand side for checking header existence only:
 [verse]
 *exthdr* {*hbh* | *frag* | *rt* | *dst* | *mh*}
-*tcp option* {*eol* | *noop* | *maxseg* | *window* | *sack-permitted* | *sack* | *sack0* | *sack1* | *sack2* | *sack3* | *timestamp*}
+*tcp option* {*eol* | *noop* | *maxseg* | *window* | *sack-perm* | *sack* | *sack0* | *sack1* | *sack2* | *sack3* | *timestamp*}
 *ip option* { lsrr | ra | rr | ssrr }
 
 .IPv6 extension headers
@@ -568,7 +568,7 @@ kind, length, size
 |window|
 TCP Window Scaling |
 kind, length, count
-|sack-permitted|
+|sack-perm |
 TCP SACK permitted |
 kind, length
 |sack|
@@ -611,7 +611,7 @@ type, length, ptr, addr
 
 .finding TCP options
 --------------------
-filter input tcp option sack-permitted kind 1 counter
+filter input tcp option sack-perm kind 1 counter
 --------------------
 
 .matching IPv6 exthdr
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 9bf4f71f1f66..8c37f895167e 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -233,7 +233,6 @@ int nft_lex(void *, void *, void *);
 %token SYNPROXY			"synproxy"
 %token MSS			"mss"
 %token WSCALE			"wscale"
-%token SACKPERM			"sack-perm"
 
 %token TYPEOF			"typeof"
 
@@ -400,14 +399,13 @@ int nft_lex(void *, void *, void *);
 %token OPTION			"option"
 %token ECHO			"echo"
 %token EOL			"eol"
-%token MAXSEG			"maxseg"
 %token NOOP			"noop"
 %token SACK			"sack"
 %token SACK0			"sack0"
 %token SACK1			"sack1"
 %token SACK2			"sack2"
 %token SACK3			"sack3"
-%token SACK_PERMITTED		"sack-permitted"
+%token SACK_PERM		"sack-permitted"
 %token TIMESTAMP		"timestamp"
 %token KIND			"kind"
 %token COUNT			"count"
@@ -3279,7 +3277,7 @@ synproxy_arg		:	MSS	NUM
 			{
 				$<stmt>0->synproxy.flags |= NF_SYNPROXY_OPT_TIMESTAMP;
 			}
-			|	SACKPERM
+			|	SACK_PERM
 			{
 				$<stmt>0->synproxy.flags |= NF_SYNPROXY_OPT_SACK_PERM;
 			}
@@ -3334,7 +3332,7 @@ synproxy_ts		:	/* empty */	{ $$ = 0; }
 			;
 
 synproxy_sack		:	/* empty */	{ $$ = 0; }
-			|	SACKPERM
+			|	SACK_PERM
 			{
 				$$ = NF_SYNPROXY_OPT_SACK_PERM;
 			}
@@ -5216,9 +5214,10 @@ tcp_hdr_field		:	SPORT		{ $$ = TCPHDR_SPORT; }
 
 tcp_hdr_option_type	:	EOL		{ $$ = TCPOPTHDR_EOL; }
 			|	NOOP		{ $$ = TCPOPTHDR_NOOP; }
-			|	MAXSEG		{ $$ = TCPOPTHDR_MAXSEG; }
+			|	MSS  	  	{ $$ = TCPOPTHDR_MAXSEG; }
+			|	SACK_PERM	{ $$ = TCPOPTHDR_SACK_PERMITTED; }
 			|	WINDOW		{ $$ = TCPOPTHDR_WINDOW; }
-			|	SACK_PERMITTED	{ $$ = TCPOPTHDR_SACK_PERMITTED; }
+			|	WSCALE		{ $$ = TCPOPTHDR_WINDOW; }
 			|	SACK		{ $$ = TCPOPTHDR_SACK0; }
 			|	SACK0		{ $$ = TCPOPTHDR_SACK0; }
 			|	SACK1		{ $$ = TCPOPTHDR_SACK1; }
diff --git a/src/scanner.l b/src/scanner.l
index 7afd9bfb8893..516c648f1c1f 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -421,14 +421,16 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 
 "echo"			{ return ECHO; }
 "eol"			{ return EOL; }
-"maxseg"		{ return MAXSEG; }
+"maxseg"		{ return MSS; }
+"mss"			{ return MSS; }
 "noop"			{ return NOOP; }
 "sack"			{ return SACK; }
 "sack0"			{ return SACK0; }
 "sack1"			{ return SACK1; }
 "sack2"			{ return SACK2; }
 "sack3"			{ return SACK3; }
-"sack-permitted"	{ return SACK_PERMITTED; }
+"sack-permitted"	{ return SACK_PERM; }
+"sack-perm"		{ return SACK_PERM; }
 "timestamp"		{ return TIMESTAMP; }
 "time"			{ return TIME; }
 
@@ -565,9 +567,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "osf"			{ return OSF; }
 
 "synproxy"		{ return SYNPROXY; }
-"mss"			{ return MSS; }
 "wscale"		{ return WSCALE; }
-"sack-perm"		{ return SACKPERM; }
 
 "notrack"		{ return NOTRACK; }
 
diff --git a/src/tcpopt.c b/src/tcpopt.c
index ec305d9466d5..6dbaa9e6dd17 100644
--- a/src/tcpopt.c
+++ b/src/tcpopt.c
@@ -55,7 +55,7 @@ static const struct exthdr_desc tcpopt_window = {
 };
 
 static const struct exthdr_desc tcpopt_sack_permitted = {
-	.name		= "sack-permitted",
+	.name		= "sack-perm",
 	.type		= TCPOPT_SACK_PERMITTED,
 	.templates	= {
 		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0, 8),
diff --git a/tests/py/any/tcpopt.t b/tests/py/any/tcpopt.t
index 08b1dcb3c489..5f21d4989fea 100644
--- a/tests/py/any/tcpopt.t
+++ b/tests/py/any/tcpopt.t
@@ -12,8 +12,8 @@ tcp option maxseg size 1;ok
 tcp option window kind 1;ok
 tcp option window length 1;ok
 tcp option window count 1;ok
-tcp option sack-permitted kind 1;ok
-tcp option sack-permitted length 1;ok
+tcp option sack-perm kind 1;ok
+tcp option sack-perm length 1;ok
 tcp option sack kind 1;ok
 tcp option sack length 1;ok
 tcp option sack left 1;ok
diff --git a/tests/py/any/tcpopt.t.json b/tests/py/any/tcpopt.t.json
index 48eb339cee35..2c6236a1a152 100644
--- a/tests/py/any/tcpopt.t.json
+++ b/tests/py/any/tcpopt.t.json
@@ -126,14 +126,14 @@
     }
 ]
 
-# tcp option sack-permitted kind 1
+# tcp option sack-perm kind 1
 [
     {
         "match": {
             "left": {
                 "tcp option": {
                     "field": "kind",
-                    "name": "sack-permitted"
+                    "name": "sack-perm"
                 }
             },
             "op": "==",
@@ -142,14 +142,14 @@
     }
 ]
 
-# tcp option sack-permitted length 1
+# tcp option sack-perm length 1
 [
     {
         "match": {
             "left": {
                 "tcp option": {
                     "field": "length",
-                    "name": "sack-permitted"
+                    "name": "sack-perm"
                 }
             },
             "op": "==",
diff --git a/tests/py/any/tcpopt.t.payload b/tests/py/any/tcpopt.t.payload
index 63751cf26e75..f63076ae497e 100644
--- a/tests/py/any/tcpopt.t.payload
+++ b/tests/py/any/tcpopt.t.payload
@@ -166,42 +166,42 @@ inet
   [ exthdr load tcpopt 1b @ 3 + 2 => reg 1 ]
   [ cmp eq reg 1 0x00000001 ]
 
-# tcp option sack-permitted kind 1
+# tcp option sack-perm kind 1
 ip 
   [ meta load l4proto => reg 1 ]
   [ cmp eq reg 1 0x00000006 ]
   [ exthdr load tcpopt 1b @ 4 + 0 => reg 1 ]
   [ cmp eq reg 1 0x00000001 ]
 
-# tcp option sack-permitted kind 1
+# tcp option sack-perm kind 1
 ip6 
   [ meta load l4proto => reg 1 ]
   [ cmp eq reg 1 0x00000006 ]
   [ exthdr load tcpopt 1b @ 4 + 0 => reg 1 ]
   [ cmp eq reg 1 0x00000001 ]
 
-# tcp option sack-permitted kind 1
+# tcp option sack-perm kind 1
 inet 
   [ meta load l4proto => reg 1 ]
   [ cmp eq reg 1 0x00000006 ]
   [ exthdr load tcpopt 1b @ 4 + 0 => reg 1 ]
   [ cmp eq reg 1 0x00000001 ]
 
-# tcp option sack-permitted length 1
+# tcp option sack-perm length 1
 ip 
   [ meta load l4proto => reg 1 ]
   [ cmp eq reg 1 0x00000006 ]
   [ exthdr load tcpopt 1b @ 4 + 1 => reg 1 ]
   [ cmp eq reg 1 0x00000001 ]
 
-# tcp option sack-permitted length 1
+# tcp option sack-perm length 1
 ip6 
   [ meta load l4proto => reg 1 ]
   [ cmp eq reg 1 0x00000006 ]
   [ exthdr load tcpopt 1b @ 4 + 1 => reg 1 ]
   [ cmp eq reg 1 0x00000001 ]
 
-# tcp option sack-permitted length 1
+# tcp option sack-perm length 1
 inet 
   [ meta load l4proto => reg 1 ]
   [ cmp eq reg 1 0x00000006 ]
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH nft 1/7] parser: merge sack-perm/sack-permitted and maxseg/mss
  2020-11-05 14:11 ` [PATCH nft 1/7] parser: merge sack-perm/sack-permitted and maxseg/mss Florian Westphal
@ 2020-11-05 15:22   ` Jeremy Sowden
  2020-11-05 15:45     ` Florian Westphal
  0 siblings, 1 reply; 14+ messages in thread
From: Jeremy Sowden @ 2020-11-05 15:22 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

[-- Attachment #1: Type: text/plain, Size: 9220 bytes --]

On 2020-11-05, at 15:11:38 +0100, Florian Westphal wrote:
> One was added by the tcp option parsing ocde, the other by synproxy.
>
> So we have:
> synproxy ... sack-perm
> synproxy ... mss
>
> and
>
> tcp option maxseg
> tcp option sack-permitted
>
> This kills the extra tokens on the scanner/parser side,
> so sack-perm and sack-permitted can both be used.
>
> Likewise, 'synproxy maxseg' and 'tcp option mss size 42' will
> work too.  On the output side, the shorter form is now preferred,
> i.e. sack-perm and mss.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  doc/payload-expression.txt    |  8 ++++----
>  src/parser_bison.y            | 13 ++++++-------
>  src/scanner.l                 |  8 ++++----
>  src/tcpopt.c                  |  2 +-
>  tests/py/any/tcpopt.t         |  4 ++--
>  tests/py/any/tcpopt.t.json    |  8 ++++----
>  tests/py/any/tcpopt.t.payload | 12 ++++++------
>  7 files changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/doc/payload-expression.txt b/doc/payload-expression.txt
> index 93d4d22f59f5..9df20a18ae8a 100644
> --- a/doc/payload-expression.txt
> +++ b/doc/payload-expression.txt
> @@ -525,13 +525,13 @@ nftables currently supports matching (finding) a given ipv6 extension header, TC
>  *dst* {*nexthdr* | *hdrlength*}
>  *mh* {*nexthdr* | *hdrlength* | *checksum* | *type*}
>  *srh* {*flags* | *tag* | *sid* | *seg-left*}
> -*tcp option* {*eol* | *noop* | *maxseg* | *window* | *sack-permitted* | *sack* | *sack0* | *sack1* | *sack2* | *sack3* | *timestamp*} 'tcp_option_field'
> +*tcp option* {*eol* | *noop* | *maxseg* | *window* | *sack-perm* | *sack* | *sack0* | *sack1* | *sack2* | *sack3* | *timestamp*} 'tcp_option_field'
>  *ip option* { lsrr | ra | rr | ssrr } 'ip_option_field'
>
>  The following syntaxes are valid only in a relational expression with boolean type on right-hand side for checking header existence only:
>  [verse]
>  *exthdr* {*hbh* | *frag* | *rt* | *dst* | *mh*}
> -*tcp option* {*eol* | *noop* | *maxseg* | *window* | *sack-permitted* | *sack* | *sack0* | *sack1* | *sack2* | *sack3* | *timestamp*}
> +*tcp option* {*eol* | *noop* | *maxseg* | *window* | *sack-perm* | *sack* | *sack0* | *sack1* | *sack2* | *sack3* | *timestamp*}
>  *ip option* { lsrr | ra | rr | ssrr }
>
>  .IPv6 extension headers
> @@ -568,7 +568,7 @@ kind, length, size
>  |window|
>  TCP Window Scaling |
>  kind, length, count
> -|sack-permitted|
> +|sack-perm |
>  TCP SACK permitted |
>  kind, length
>  |sack|
> @@ -611,7 +611,7 @@ type, length, ptr, addr
>
>  .finding TCP options
>  --------------------
> -filter input tcp option sack-permitted kind 1 counter
> +filter input tcp option sack-perm kind 1 counter
>  --------------------
>
>  .matching IPv6 exthdr
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index 9bf4f71f1f66..8c37f895167e 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -233,7 +233,6 @@ int nft_lex(void *, void *, void *);
>  %token SYNPROXY			"synproxy"
>  %token MSS			"mss"
>  %token WSCALE			"wscale"
> -%token SACKPERM			"sack-perm"
>
>  %token TYPEOF			"typeof"
>
> @@ -400,14 +399,13 @@ int nft_lex(void *, void *, void *);
>  %token OPTION			"option"
>  %token ECHO			"echo"
>  %token EOL			"eol"
> -%token MAXSEG			"maxseg"
>  %token NOOP			"noop"
>  %token SACK			"sack"
>  %token SACK0			"sack0"
>  %token SACK1			"sack1"
>  %token SACK2			"sack2"
>  %token SACK3			"sack3"
> -%token SACK_PERMITTED		"sack-permitted"
> +%token SACK_PERM		"sack-permitted"
>  %token TIMESTAMP		"timestamp"
>  %token KIND			"kind"
>  %token COUNT			"count"
> @@ -3279,7 +3277,7 @@ synproxy_arg		:	MSS	NUM
>  			{
>  				$<stmt>0->synproxy.flags |= NF_SYNPROXY_OPT_TIMESTAMP;
>  			}
> -			|	SACKPERM
> +			|	SACK_PERM
>  			{
>  				$<stmt>0->synproxy.flags |= NF_SYNPROXY_OPT_SACK_PERM;
>  			}
> @@ -3334,7 +3332,7 @@ synproxy_ts		:	/* empty */	{ $$ = 0; }
>  			;
>
>  synproxy_sack		:	/* empty */	{ $$ = 0; }
> -			|	SACKPERM
> +			|	SACK_PERM
>  			{
>  				$$ = NF_SYNPROXY_OPT_SACK_PERM;
>  			}
> @@ -5216,9 +5214,10 @@ tcp_hdr_field		:	SPORT		{ $$ = TCPHDR_SPORT; }
>
>  tcp_hdr_option_type	:	EOL		{ $$ = TCPOPTHDR_EOL; }
>  			|	NOOP		{ $$ = TCPOPTHDR_NOOP; }
> -			|	MAXSEG		{ $$ = TCPOPTHDR_MAXSEG; }
> +			|	MSS  	  	{ $$ = TCPOPTHDR_MAXSEG; }
> +			|	SACK_PERM	{ $$ = TCPOPTHDR_SACK_PERMITTED; }
>  			|	WINDOW		{ $$ = TCPOPTHDR_WINDOW; }
> -			|	SACK_PERMITTED	{ $$ = TCPOPTHDR_SACK_PERMITTED; }
> +			|	WSCALE		{ $$ = TCPOPTHDR_WINDOW; }

Did you mean to add this here?

>  			|	SACK		{ $$ = TCPOPTHDR_SACK0; }
>  			|	SACK0		{ $$ = TCPOPTHDR_SACK0; }
>  			|	SACK1		{ $$ = TCPOPTHDR_SACK1; }
> diff --git a/src/scanner.l b/src/scanner.l
> index 7afd9bfb8893..516c648f1c1f 100644
> --- a/src/scanner.l
> +++ b/src/scanner.l
> @@ -421,14 +421,16 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
>
>  "echo"			{ return ECHO; }
>  "eol"			{ return EOL; }
> -"maxseg"		{ return MAXSEG; }
> +"maxseg"		{ return MSS; }
> +"mss"			{ return MSS; }
>  "noop"			{ return NOOP; }
>  "sack"			{ return SACK; }
>  "sack0"			{ return SACK0; }
>  "sack1"			{ return SACK1; }
>  "sack2"			{ return SACK2; }
>  "sack3"			{ return SACK3; }
> -"sack-permitted"	{ return SACK_PERMITTED; }
> +"sack-permitted"	{ return SACK_PERM; }
> +"sack-perm"		{ return SACK_PERM; }
>  "timestamp"		{ return TIMESTAMP; }
>  "time"			{ return TIME; }
>
> @@ -565,9 +567,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
>  "osf"			{ return OSF; }
>
>  "synproxy"		{ return SYNPROXY; }
> -"mss"			{ return MSS; }
>  "wscale"		{ return WSCALE; }
> -"sack-perm"		{ return SACKPERM; }
>
>  "notrack"		{ return NOTRACK; }
>
> diff --git a/src/tcpopt.c b/src/tcpopt.c
> index ec305d9466d5..6dbaa9e6dd17 100644
> --- a/src/tcpopt.c
> +++ b/src/tcpopt.c
> @@ -55,7 +55,7 @@ static const struct exthdr_desc tcpopt_window = {
>  };
>
>  static const struct exthdr_desc tcpopt_sack_permitted = {
> -	.name		= "sack-permitted",
> +	.name		= "sack-perm",
>  	.type		= TCPOPT_SACK_PERMITTED,
>  	.templates	= {
>  		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0, 8),
> diff --git a/tests/py/any/tcpopt.t b/tests/py/any/tcpopt.t
> index 08b1dcb3c489..5f21d4989fea 100644
> --- a/tests/py/any/tcpopt.t
> +++ b/tests/py/any/tcpopt.t
> @@ -12,8 +12,8 @@ tcp option maxseg size 1;ok
>  tcp option window kind 1;ok
>  tcp option window length 1;ok
>  tcp option window count 1;ok
> -tcp option sack-permitted kind 1;ok
> -tcp option sack-permitted length 1;ok
> +tcp option sack-perm kind 1;ok
> +tcp option sack-perm length 1;ok
>  tcp option sack kind 1;ok
>  tcp option sack length 1;ok
>  tcp option sack left 1;ok
> diff --git a/tests/py/any/tcpopt.t.json b/tests/py/any/tcpopt.t.json
> index 48eb339cee35..2c6236a1a152 100644
> --- a/tests/py/any/tcpopt.t.json
> +++ b/tests/py/any/tcpopt.t.json
> @@ -126,14 +126,14 @@
>      }
>  ]
>
> -# tcp option sack-permitted kind 1
> +# tcp option sack-perm kind 1
>  [
>      {
>          "match": {
>              "left": {
>                  "tcp option": {
>                      "field": "kind",
> -                    "name": "sack-permitted"
> +                    "name": "sack-perm"
>                  }
>              },
>              "op": "==",
> @@ -142,14 +142,14 @@
>      }
>  ]
>
> -# tcp option sack-permitted length 1
> +# tcp option sack-perm length 1
>  [
>      {
>          "match": {
>              "left": {
>                  "tcp option": {
>                      "field": "length",
> -                    "name": "sack-permitted"
> +                    "name": "sack-perm"
>                  }
>              },
>              "op": "==",
> diff --git a/tests/py/any/tcpopt.t.payload b/tests/py/any/tcpopt.t.payload
> index 63751cf26e75..f63076ae497e 100644
> --- a/tests/py/any/tcpopt.t.payload
> +++ b/tests/py/any/tcpopt.t.payload
> @@ -166,42 +166,42 @@ inet
>    [ exthdr load tcpopt 1b @ 3 + 2 => reg 1 ]
>    [ cmp eq reg 1 0x00000001 ]
>
> -# tcp option sack-permitted kind 1
> +# tcp option sack-perm kind 1
>  ip
>    [ meta load l4proto => reg 1 ]
>    [ cmp eq reg 1 0x00000006 ]
>    [ exthdr load tcpopt 1b @ 4 + 0 => reg 1 ]
>    [ cmp eq reg 1 0x00000001 ]
>
> -# tcp option sack-permitted kind 1
> +# tcp option sack-perm kind 1
>  ip6
>    [ meta load l4proto => reg 1 ]
>    [ cmp eq reg 1 0x00000006 ]
>    [ exthdr load tcpopt 1b @ 4 + 0 => reg 1 ]
>    [ cmp eq reg 1 0x00000001 ]
>
> -# tcp option sack-permitted kind 1
> +# tcp option sack-perm kind 1
>  inet
>    [ meta load l4proto => reg 1 ]
>    [ cmp eq reg 1 0x00000006 ]
>    [ exthdr load tcpopt 1b @ 4 + 0 => reg 1 ]
>    [ cmp eq reg 1 0x00000001 ]
>
> -# tcp option sack-permitted length 1
> +# tcp option sack-perm length 1
>  ip
>    [ meta load l4proto => reg 1 ]
>    [ cmp eq reg 1 0x00000006 ]
>    [ exthdr load tcpopt 1b @ 4 + 1 => reg 1 ]
>    [ cmp eq reg 1 0x00000001 ]
>
> -# tcp option sack-permitted length 1
> +# tcp option sack-perm length 1
>  ip6
>    [ meta load l4proto => reg 1 ]
>    [ cmp eq reg 1 0x00000006 ]
>    [ exthdr load tcpopt 1b @ 4 + 1 => reg 1 ]
>    [ cmp eq reg 1 0x00000001 ]
>
> -# tcp option sack-permitted length 1
> +# tcp option sack-perm length 1
>  inet
>    [ meta load l4proto => reg 1 ]
>    [ cmp eq reg 1 0x00000006 ]
> --
> 2.26.2
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH nft 1/7] parser: merge sack-perm/sack-permitted and maxseg/mss
  2020-11-05 15:22   ` Jeremy Sowden
@ 2020-11-05 15:45     ` Florian Westphal
  0 siblings, 0 replies; 14+ messages in thread
From: Florian Westphal @ 2020-11-05 15:45 UTC (permalink / raw)
  To: Jeremy Sowden; +Cc: Florian Westphal, netfilter-devel

Jeremy Sowden <jeremy@azazel.net> wrote:
> > -			|	MAXSEG		{ $$ = TCPOPTHDR_MAXSEG; }
> > +			|	MSS  	  	{ $$ = TCPOPTHDR_MAXSEG; }
> > +			|	SACK_PERM	{ $$ = TCPOPTHDR_SACK_PERMITTED; }
> >  			|	WINDOW		{ $$ = TCPOPTHDR_WINDOW; }
> > -			|	SACK_PERMITTED	{ $$ = TCPOPTHDR_SACK_PERMITTED; }
> > +			|	WSCALE		{ $$ = TCPOPTHDR_WINDOW; }
> 
> Did you mean to add this here?

Right, this could be a distinct change.

ATM there is
... synproxy ... wscale
and
... tcp option window ...

Like the sack-perm vs. sack-permitted mess it probably
makes sense to get rid of one of those tokens so both work
interchangeably, i.e.

tcp option wscale
and
tcp option window
are the same.

I will remove this chunk before pushing it out.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH nft 2/7] tcpopts: clean up parser -> tcpopt.c plumbing
  2020-11-05 14:11 [PATCH nft 0/7] rework tcp option handling Florian Westphal
  2020-11-05 14:11 ` [PATCH nft 1/7] parser: merge sack-perm/sack-permitted and maxseg/mss Florian Westphal
@ 2020-11-05 14:11 ` Florian Westphal
  2020-11-05 14:11 ` [PATCH nft 3/7] tcpopt: rename noop to nop Florian Westphal
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Florian Westphal @ 2020-11-05 14:11 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

tcpopt template mapping is asymmetric:
one mapping is to match dumped netlink exthdr expression to the original
tcp option template.

This struct is indexed by the raw, on-write kind/type number.

The other mapping maps parsed options to the tcp option template.
Remove the latter.  The parser is changed to translate the textual
option name, e.g. "maxseg" to the on-wire number.

This avoids the second mapping, it will also allow to more easily
support raw option matching in a followup patch.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 doc/payload-expression.txt |  4 +-
 include/tcpopt.h           | 35 ++++++++-------
 src/parser_bison.y         | 28 ++++++------
 src/parser_json.c          | 10 ++---
 src/scanner.l              |  3 +-
 src/tcpopt.c               | 92 +++++++++++++++-----------------------
 6 files changed, 76 insertions(+), 96 deletions(-)

diff --git a/doc/payload-expression.txt b/doc/payload-expression.txt
index 9df20a18ae8a..2fa394ea966f 100644
--- a/doc/payload-expression.txt
+++ b/doc/payload-expression.txt
@@ -525,13 +525,13 @@ nftables currently supports matching (finding) a given ipv6 extension header, TC
 *dst* {*nexthdr* | *hdrlength*}
 *mh* {*nexthdr* | *hdrlength* | *checksum* | *type*}
 *srh* {*flags* | *tag* | *sid* | *seg-left*}
-*tcp option* {*eol* | *noop* | *maxseg* | *window* | *sack-perm* | *sack* | *sack0* | *sack1* | *sack2* | *sack3* | *timestamp*} 'tcp_option_field'
+*tcp option* {*eol* | *nop* | *maxseg* | *window* | *sack-perm* | *sack* | *sack0* | *sack1* | *sack2* | *sack3* | *timestamp*} 'tcp_option_field'
 *ip option* { lsrr | ra | rr | ssrr } 'ip_option_field'
 
 The following syntaxes are valid only in a relational expression with boolean type on right-hand side for checking header existence only:
 [verse]
 *exthdr* {*hbh* | *frag* | *rt* | *dst* | *mh*}
-*tcp option* {*eol* | *noop* | *maxseg* | *window* | *sack-perm* | *sack* | *sack0* | *sack1* | *sack2* | *sack3* | *timestamp*}
+*tcp option* {*eol* | *nop* | *maxseg* | *window* | *sack-perm* | *sack* | *sack0* | *sack1* | *sack2* | *sack3* | *timestamp*}
 *ip option* { lsrr | ra | rr | ssrr }
 
 .IPv6 extension headers
diff --git a/include/tcpopt.h b/include/tcpopt.h
index ffdbcb028125..7f3fbb8b0c7d 100644
--- a/include/tcpopt.h
+++ b/include/tcpopt.h
@@ -6,7 +6,7 @@
 #include <statement.h>
 
 extern struct expr *tcpopt_expr_alloc(const struct location *loc,
-				      uint8_t type, uint8_t field);
+				      unsigned int kind, unsigned int field);
 
 extern void tcpopt_init_raw(struct expr *expr, uint8_t type,
 			    unsigned int offset, unsigned int len,
@@ -15,21 +15,22 @@ extern void tcpopt_init_raw(struct expr *expr, uint8_t type,
 extern bool tcpopt_find_template(struct expr *expr, const struct expr *mask,
 				 unsigned int *shift);
 
-enum tcpopt_hdr_types {
-	TCPOPTHDR_INVALID,
-	TCPOPTHDR_EOL,
-	TCPOPTHDR_NOOP,
-	TCPOPTHDR_MAXSEG,
-	TCPOPTHDR_WINDOW,
-	TCPOPTHDR_SACK_PERMITTED,
-	TCPOPTHDR_SACK0,
-	TCPOPTHDR_SACK1,
-	TCPOPTHDR_SACK2,
-	TCPOPTHDR_SACK3,
-	TCPOPTHDR_TIMESTAMP,
-	TCPOPTHDR_ECHO,
-	TCPOPTHDR_ECHO_REPLY,
-	__TCPOPTHDR_MAX
+/* TCP option numbers used on wire */
+enum tcpopt_kind {
+	TCPOPT_KIND_EOL = 0,
+	TCPOPT_KIND_NOP = 1,
+	TCPOPT_KIND_MAXSEG = 2,
+	TCPOPT_KIND_WINDOW = 3,
+	TCPOPT_KIND_SACK_PERMITTED = 4,
+	TCPOPT_KIND_SACK = 5,
+	TCPOPT_KIND_TIMESTAMP = 8,
+	TCPOPT_KIND_ECHO = 8,
+	__TCPOPT_KIND_MAX,
+
+	/* extra oob info, internal to nft */
+	TCPOPT_KIND_SACK1 = 256,
+	TCPOPT_KIND_SACK2 = 257,
+	TCPOPT_KIND_SACK3 = 258,
 };
 
 enum tcpopt_hdr_fields {
@@ -44,6 +45,6 @@ enum tcpopt_hdr_fields {
 	TCPOPTHDR_FIELD_TSECR,
 };
 
-extern const struct exthdr_desc *tcpopthdr_protocols[__TCPOPTHDR_MAX];
+extern const struct exthdr_desc *tcpopt_protocols[__TCPOPT_KIND_MAX];
 
 #endif /* NFTABLES_TCPOPT_H */
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 8c37f895167e..379a6acd18e6 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -399,7 +399,7 @@ int nft_lex(void *, void *, void *);
 %token OPTION			"option"
 %token ECHO			"echo"
 %token EOL			"eol"
-%token NOOP			"noop"
+%token NOP			"nop"
 %token SACK			"sack"
 %token SACK0			"sack0"
 %token SACK1			"sack1"
@@ -5212,19 +5212,19 @@ tcp_hdr_field		:	SPORT		{ $$ = TCPHDR_SPORT; }
 			|	URGPTR		{ $$ = TCPHDR_URGPTR; }
 			;
 
-tcp_hdr_option_type	:	EOL		{ $$ = TCPOPTHDR_EOL; }
-			|	NOOP		{ $$ = TCPOPTHDR_NOOP; }
-			|	MSS  	  	{ $$ = TCPOPTHDR_MAXSEG; }
-			|	SACK_PERM	{ $$ = TCPOPTHDR_SACK_PERMITTED; }
-			|	WINDOW		{ $$ = TCPOPTHDR_WINDOW; }
-			|	WSCALE		{ $$ = TCPOPTHDR_WINDOW; }
-			|	SACK		{ $$ = TCPOPTHDR_SACK0; }
-			|	SACK0		{ $$ = TCPOPTHDR_SACK0; }
-			|	SACK1		{ $$ = TCPOPTHDR_SACK1; }
-			|	SACK2		{ $$ = TCPOPTHDR_SACK2; }
-			|	SACK3		{ $$ = TCPOPTHDR_SACK3; }
-			|	ECHO		{ $$ = TCPOPTHDR_ECHO; }
-			|	TIMESTAMP	{ $$ = TCPOPTHDR_TIMESTAMP; }
+tcp_hdr_option_type	:	EOL		{ $$ = TCPOPT_KIND_EOL; }
+			|	NOP		{ $$ = TCPOPT_KIND_NOP; }
+			|	MSS  	  	{ $$ = TCPOPT_KIND_MAXSEG; }
+			|	SACK_PERM	{ $$ = TCPOPT_KIND_SACK_PERMITTED; }
+			|	WINDOW		{ $$ = TCPOPT_KIND_WINDOW; }
+			|	WSCALE		{ $$ = TCPOPT_KIND_WINDOW; }
+			|	SACK		{ $$ = TCPOPT_KIND_SACK; }
+			|	SACK0		{ $$ = TCPOPT_KIND_SACK; }
+			|	SACK1		{ $$ = TCPOPT_KIND_SACK1; }
+			|	SACK2		{ $$ = TCPOPT_KIND_SACK2; }
+			|	SACK3		{ $$ = TCPOPT_KIND_SACK3; }
+			|	ECHO		{ $$ = TCPOPT_KIND_ECHO; }
+			|	TIMESTAMP	{ $$ = TCPOPT_KIND_TIMESTAMP; }
 			;
 
 tcp_hdr_option_field	:	KIND		{ $$ = TCPOPTHDR_FIELD_KIND; }
diff --git a/src/parser_json.c b/src/parser_json.c
index 136239121427..c68b64d9f636 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -458,9 +458,9 @@ static int json_parse_tcp_option_type(const char *name, int *val)
 {
 	unsigned int i;
 
-	for (i = 0; i < array_size(tcpopthdr_protocols); i++) {
-		if (tcpopthdr_protocols[i] &&
-		    !strcmp(tcpopthdr_protocols[i]->name, name)) {
+	for (i = 0; i < array_size(tcpopt_protocols); i++) {
+		if (tcpopt_protocols[i] &&
+		    !strcmp(tcpopt_protocols[i]->name, name)) {
 			if (val)
 				*val = i;
 			return 0;
@@ -469,7 +469,7 @@ static int json_parse_tcp_option_type(const char *name, int *val)
 	/* special case for sack0 - sack3 */
 	if (sscanf(name, "sack%u", &i) == 1 && i < 4) {
 		if (val)
-			*val = TCPOPTHDR_SACK0 + i;
+			*val = TCPOPT_KIND_SACK + i;
 		return 0;
 	}
 	return 1;
@@ -478,7 +478,7 @@ static int json_parse_tcp_option_type(const char *name, int *val)
 static int json_parse_tcp_option_field(int type, const char *name, int *val)
 {
 	unsigned int i;
-	const struct exthdr_desc *desc = tcpopthdr_protocols[type];
+	const struct exthdr_desc *desc = tcpopt_protocols[type];
 
 	for (i = 0; i < array_size(desc->templates); i++) {
 		if (desc->templates[i].token &&
diff --git a/src/scanner.l b/src/scanner.l
index 516c648f1c1f..8bde1fbe912d 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -423,7 +423,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "eol"			{ return EOL; }
 "maxseg"		{ return MSS; }
 "mss"			{ return MSS; }
-"noop"			{ return NOOP; }
+"nop"			{ return NOP; }
+"noop"			{ return NOP; }
 "sack"			{ return SACK; }
 "sack0"			{ return SACK0; }
 "sack1"			{ return SACK1; }
diff --git a/src/tcpopt.c b/src/tcpopt.c
index 6dbaa9e6dd17..8d5bdec5399e 100644
--- a/src/tcpopt.c
+++ b/src/tcpopt.c
@@ -20,7 +20,7 @@ static const struct proto_hdr_template tcpopt_unknown_template =
 			   __offset, __len)
 static const struct exthdr_desc tcpopt_eol = {
 	.name		= "eol",
-	.type		= TCPOPT_EOL,
+	.type		= TCPOPT_KIND_EOL,
 	.templates	= {
 		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",  0,    8),
 	},
@@ -28,7 +28,7 @@ static const struct exthdr_desc tcpopt_eol = {
 
 static const struct exthdr_desc tcpopt_nop = {
 	.name		= "noop",
-	.type		= TCPOPT_NOP,
+	.type		= TCPOPT_KIND_NOP,
 	.templates	= {
 		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0,   8),
 	},
@@ -36,7 +36,7 @@ static const struct exthdr_desc tcpopt_nop = {
 
 static const struct exthdr_desc tcptopt_maxseg = {
 	.name		= "maxseg",
-	.type		= TCPOPT_MAXSEG,
+	.type		= TCPOPT_KIND_MAXSEG,
 	.templates	= {
 		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0,  8),
 		[TCPOPTHDR_FIELD_LENGTH]	= PHT("length", 8,  8),
@@ -46,7 +46,7 @@ static const struct exthdr_desc tcptopt_maxseg = {
 
 static const struct exthdr_desc tcpopt_window = {
 	.name		= "window",
-	.type		= TCPOPT_WINDOW,
+	.type		= TCPOPT_KIND_WINDOW,
 	.templates	= {
 		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0,  8),
 		[TCPOPTHDR_FIELD_LENGTH]	= PHT("length", 8,  8),
@@ -56,7 +56,7 @@ static const struct exthdr_desc tcpopt_window = {
 
 static const struct exthdr_desc tcpopt_sack_permitted = {
 	.name		= "sack-perm",
-	.type		= TCPOPT_SACK_PERMITTED,
+	.type		= TCPOPT_KIND_SACK_PERMITTED,
 	.templates	= {
 		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0, 8),
 		[TCPOPTHDR_FIELD_LENGTH]	= PHT("length", 8, 8),
@@ -65,7 +65,7 @@ static const struct exthdr_desc tcpopt_sack_permitted = {
 
 static const struct exthdr_desc tcpopt_sack = {
 	.name		= "sack",
-	.type		= TCPOPT_SACK,
+	.type		= TCPOPT_KIND_SACK,
 	.templates	= {
 		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0,   8),
 		[TCPOPTHDR_FIELD_LENGTH]		= PHT("length", 8,   8),
@@ -76,7 +76,7 @@ static const struct exthdr_desc tcpopt_sack = {
 
 static const struct exthdr_desc tcpopt_timestamp = {
 	.name		= "timestamp",
-	.type		= TCPOPT_TIMESTAMP,
+	.type		= TCPOPT_KIND_TIMESTAMP,
 	.templates	= {
 		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0,  8),
 		[TCPOPTHDR_FIELD_LENGTH]	= PHT("length", 8,  8),
@@ -86,19 +86,14 @@ static const struct exthdr_desc tcpopt_timestamp = {
 };
 #undef PHT
 
-#define TCPOPT_OBSOLETE ((struct exthdr_desc *)NULL)
-#define TCPOPT_ECHO 6
-#define TCPOPT_ECHO_REPLY 7
-static const struct exthdr_desc *tcpopt_protocols[] = {
-	[TCPOPT_EOL]		= &tcpopt_eol,
-	[TCPOPT_NOP]		= &tcpopt_nop,
-	[TCPOPT_MAXSEG]		= &tcptopt_maxseg,
-	[TCPOPT_WINDOW]		= &tcpopt_window,
-	[TCPOPT_SACK_PERMITTED]	= &tcpopt_sack_permitted,
-	[TCPOPT_SACK]		= &tcpopt_sack,
-	[TCPOPT_ECHO]		= TCPOPT_OBSOLETE,
-	[TCPOPT_ECHO_REPLY]	= TCPOPT_OBSOLETE,
-	[TCPOPT_TIMESTAMP]	= &tcpopt_timestamp,
+const struct exthdr_desc *tcpopt_protocols[] = {
+	[TCPOPT_KIND_EOL]		= &tcpopt_eol,
+	[TCPOPT_KIND_NOP]		= &tcpopt_nop,
+	[TCPOPT_KIND_MAXSEG]		= &tcptopt_maxseg,
+	[TCPOPT_KIND_WINDOW]		= &tcpopt_window,
+	[TCPOPT_KIND_SACK_PERMITTED]	= &tcpopt_sack_permitted,
+	[TCPOPT_KIND_SACK]		= &tcpopt_sack,
+	[TCPOPT_KIND_TIMESTAMP]		= &tcpopt_timestamp,
 };
 
 static unsigned int calc_offset(const struct exthdr_desc *desc,
@@ -136,51 +131,34 @@ static unsigned int calc_offset_reverse(const struct exthdr_desc *desc,
 	}
 }
 
-const struct exthdr_desc *tcpopthdr_protocols[__TCPOPTHDR_MAX] = {
-	[TCPOPTHDR_EOL]			= &tcpopt_eol,
-	[TCPOPTHDR_NOOP]		= &tcpopt_nop,
-	[TCPOPTHDR_MAXSEG]		= &tcptopt_maxseg,
-	[TCPOPTHDR_WINDOW]		= &tcpopt_window,
-	[TCPOPTHDR_SACK_PERMITTED]	= &tcpopt_sack_permitted,
-	[TCPOPTHDR_SACK0]		= &tcpopt_sack,
-	[TCPOPTHDR_SACK1]		= &tcpopt_sack,
-	[TCPOPTHDR_SACK2]		= &tcpopt_sack,
-	[TCPOPTHDR_SACK3]		= &tcpopt_sack,
-	[TCPOPTHDR_ECHO]		= TCPOPT_OBSOLETE,
-	[TCPOPTHDR_ECHO_REPLY]		= TCPOPT_OBSOLETE,
-	[TCPOPTHDR_TIMESTAMP]		= &tcpopt_timestamp,
-};
-
-static uint8_t tcpopt_optnum[] = {
-	[TCPOPTHDR_SACK0]	= 0,
-	[TCPOPTHDR_SACK1]	= 1,
-	[TCPOPTHDR_SACK2]	= 2,
-	[TCPOPTHDR_SACK3]	= 3,
-};
-
-static uint8_t tcpopt_find_optnum(uint8_t optnum)
-{
-	if (optnum > TCPOPTHDR_SACK3)
-		return 0;
-
-	return tcpopt_optnum[optnum];
-}
-
-struct expr *tcpopt_expr_alloc(const struct location *loc, uint8_t type,
-			       uint8_t field)
+struct expr *tcpopt_expr_alloc(const struct location *loc,
+			       unsigned int kind,
+			       unsigned int field)
 {
 	const struct proto_hdr_template *tmpl;
 	const struct exthdr_desc *desc;
+	uint8_t optnum = 0;
 	struct expr *expr;
-	uint8_t optnum;
 
-	desc = tcpopthdr_protocols[type];
+	switch (kind) {
+	case TCPOPT_KIND_SACK1:
+		kind = TCPOPT_KIND_SACK;
+		optnum = 1;
+		break;
+	case TCPOPT_KIND_SACK2:
+		kind = TCPOPT_KIND_SACK;
+		optnum = 2;
+		break;
+	case TCPOPT_KIND_SACK3:
+		kind = TCPOPT_KIND_SACK;
+		optnum = 3;
+	}
+
+	desc = tcpopt_protocols[kind];
 	tmpl = &desc->templates[field];
 	if (!tmpl)
 		return NULL;
 
-	optnum = tcpopt_find_optnum(type);
-
 	expr = expr_alloc(loc, EXPR_EXTHDR, tmpl->dtype,
 			  BYTEORDER_BIG_ENDIAN, tmpl->len);
 	expr->exthdr.desc   = desc;
@@ -206,7 +184,7 @@ void tcpopt_init_raw(struct expr *expr, uint8_t type, unsigned int offset,
 	assert(type < array_size(tcpopt_protocols));
 	expr->exthdr.desc = tcpopt_protocols[type];
 	expr->exthdr.flags = flags;
-	assert(expr->exthdr.desc != TCPOPT_OBSOLETE);
+	assert(expr->exthdr.desc != NULL);
 
 	for (i = 0; i < array_size(expr->exthdr.desc->templates); ++i) {
 		tmpl = &expr->exthdr.desc->templates[i];
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH nft 3/7] tcpopt: rename noop to nop
  2020-11-05 14:11 [PATCH nft 0/7] rework tcp option handling Florian Westphal
  2020-11-05 14:11 ` [PATCH nft 1/7] parser: merge sack-perm/sack-permitted and maxseg/mss Florian Westphal
  2020-11-05 14:11 ` [PATCH nft 2/7] tcpopts: clean up parser -> tcpopt.c plumbing Florian Westphal
@ 2020-11-05 14:11 ` Florian Westphal
  2020-11-05 14:11 ` [PATCH nft 4/7] tcpopt: split tcpopt_hdr_fields into per-option enum Florian Westphal
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Florian Westphal @ 2020-11-05 14:11 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

'nop' is the tcp padding "option". "noop" is retained for compatibility
on parser side.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 doc/payload-expression.txt    |  4 ++--
 src/tcpopt.c                  |  2 +-
 tests/py/any/tcpopt.t         |  2 +-
 tests/py/any/tcpopt.t.json    |  4 ++--
 tests/py/any/tcpopt.t.payload | 16 +---------------
 5 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/doc/payload-expression.txt b/doc/payload-expression.txt
index 2fa394ea966f..3cfa7791edac 100644
--- a/doc/payload-expression.txt
+++ b/doc/payload-expression.txt
@@ -559,8 +559,8 @@ Segment Routing Header
 |eol|
 End if option list|
 kind
-|noop|
-1 Byte TCP No-op options |
+|nop|
+1 Byte TCP Nop padding option |
 kind
 |maxseg|
 TCP Maximum Segment Size|
diff --git a/src/tcpopt.c b/src/tcpopt.c
index 8d5bdec5399e..17cb580d0ead 100644
--- a/src/tcpopt.c
+++ b/src/tcpopt.c
@@ -27,7 +27,7 @@ static const struct exthdr_desc tcpopt_eol = {
 };
 
 static const struct exthdr_desc tcpopt_nop = {
-	.name		= "noop",
+	.name		= "nop",
 	.type		= TCPOPT_KIND_NOP,
 	.templates	= {
 		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0,   8),
diff --git a/tests/py/any/tcpopt.t b/tests/py/any/tcpopt.t
index 5f21d4989fea..1d42de8746cd 100644
--- a/tests/py/any/tcpopt.t
+++ b/tests/py/any/tcpopt.t
@@ -5,7 +5,7 @@
 *inet;test-inet;input
 
 tcp option eol kind 1;ok
-tcp option noop kind 1;ok
+tcp option nop kind 1;ok
 tcp option maxseg kind 1;ok
 tcp option maxseg length 1;ok
 tcp option maxseg size 1;ok
diff --git a/tests/py/any/tcpopt.t.json b/tests/py/any/tcpopt.t.json
index 2c6236a1a152..b15e36ee7f4c 100644
--- a/tests/py/any/tcpopt.t.json
+++ b/tests/py/any/tcpopt.t.json
@@ -14,14 +14,14 @@
     }
 ]
 
-# tcp option noop kind 1
+# tcp option nop kind 1
 [
     {
         "match": {
             "left": {
                 "tcp option": {
                     "field": "kind",
-                    "name": "noop"
+                    "name": "nop"
                 }
             },
             "op": "==",
diff --git a/tests/py/any/tcpopt.t.payload b/tests/py/any/tcpopt.t.payload
index f63076ae497e..9c480c8bd06b 100644
--- a/tests/py/any/tcpopt.t.payload
+++ b/tests/py/any/tcpopt.t.payload
@@ -19,21 +19,7 @@ inet
   [ exthdr load tcpopt 1b @ 0 + 0 => reg 1 ]
   [ cmp eq reg 1 0x00000001 ]
 
-# tcp option noop kind 1
-ip 
-  [ meta load l4proto => reg 1 ]
-  [ cmp eq reg 1 0x00000006 ]
-  [ exthdr load tcpopt 1b @ 1 + 0 => reg 1 ]
-  [ cmp eq reg 1 0x00000001 ]
-
-# tcp option noop kind 1
-ip6 
-  [ meta load l4proto => reg 1 ]
-  [ cmp eq reg 1 0x00000006 ]
-  [ exthdr load tcpopt 1b @ 1 + 0 => reg 1 ]
-  [ cmp eq reg 1 0x00000001 ]
-
-# tcp option noop kind 1
+# tcp option nop kind 1
 inet 
   [ meta load l4proto => reg 1 ]
   [ cmp eq reg 1 0x00000006 ]
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH nft 4/7] tcpopt: split tcpopt_hdr_fields into per-option enum
  2020-11-05 14:11 [PATCH nft 0/7] rework tcp option handling Florian Westphal
                   ` (2 preceding siblings ...)
  2020-11-05 14:11 ` [PATCH nft 3/7] tcpopt: rename noop to nop Florian Westphal
@ 2020-11-05 14:11 ` Florian Westphal
  2020-11-05 14:11 ` [PATCH nft 5/7] tcpopt: allow to check for presence of any tcp option Florian Westphal
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Florian Westphal @ 2020-11-05 14:11 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

Currently we're limited to ten template fields in exthdr_desc struct.
Using a single enum for all tpc option fields thus won't work
indefinitely (TCPOPTHDR_FIELD_TSECR is 9) when new option templates get
added.

Fortunately we can just use one enum per tcp option to avoid this.
As a side effect this also allows to simplify the sack offset
calculations.  Rather than computing that on-the-fly, just add extra
fields to the SACK template.

expr->exthdr.offset now holds the 'raw' value, filled in from the option
template. This would ease implementation of 'raw option matching'
using offset and length to load from the option.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/tcpopt.h          |  46 +++++++++++----
 src/evaluate.c            |  16 ++---
 src/exthdr.c              |   1 +
 src/ipopt.c               |   2 +-
 src/netlink_delinearize.c |   2 +-
 src/netlink_linearize.c   |   4 +-
 src/parser_bison.y        |  18 +++---
 src/parser_json.c         |  36 ++++++++++--
 src/tcpopt.c              | 119 ++++++++++++++++----------------------
 9 files changed, 139 insertions(+), 105 deletions(-)

diff --git a/include/tcpopt.h b/include/tcpopt.h
index 7f3fbb8b0c7d..667c8a7725d8 100644
--- a/include/tcpopt.h
+++ b/include/tcpopt.h
@@ -33,16 +33,42 @@ enum tcpopt_kind {
 	TCPOPT_KIND_SACK3 = 258,
 };
 
-enum tcpopt_hdr_fields {
-	TCPOPTHDR_FIELD_INVALID,
-	TCPOPTHDR_FIELD_KIND,
-	TCPOPTHDR_FIELD_LENGTH,
-	TCPOPTHDR_FIELD_SIZE,
-	TCPOPTHDR_FIELD_COUNT,
-	TCPOPTHDR_FIELD_LEFT,
-	TCPOPTHDR_FIELD_RIGHT,
-	TCPOPTHDR_FIELD_TSVAL,
-	TCPOPTHDR_FIELD_TSECR,
+/* Internal identifiers */
+enum tcpopt_common {
+	TCPOPT_COMMON_KIND,
+	TCPOPT_COMMON_LENGTH,
+};
+
+enum tcpopt_maxseg {
+	TCPOPT_MAXSEG_KIND,
+	TCPOPT_MAXSEG_LENGTH,
+	TCPOPT_MAXSEG_SIZE,
+};
+
+enum tcpopt_timestamp {
+	TCPOPT_TS_KIND,
+	TCPOPT_TS_LENGTH,
+	TCPOPT_TS_TSVAL,
+	TCPOPT_TS_TSECR,
+};
+
+enum tcpopt_windowscale {
+	TCPOPT_WINDOW_KIND,
+	TCPOPT_WINDOW_LENGTH,
+	TCPOPT_WINDOW_COUNT,
+};
+
+enum tcpopt_hdr_field_sack {
+	TCPOPT_SACK_KIND,
+	TCPOPT_SACK_LENGTH,
+	TCPOPT_SACK_LEFT,
+	TCPOPT_SACK_RIGHT,
+	TCPOPT_SACK_LEFT1,
+	TCPOPT_SACK_RIGHT1,
+	TCPOPT_SACK_LEFT2,
+	TCPOPT_SACK_RIGHT2,
+	TCPOPT_SACK_LEFT3,
+	TCPOPT_SACK_RIGHT3,
 };
 
 extern const struct exthdr_desc *tcpopt_protocols[__TCPOPT_KIND_MAX];
diff --git a/src/evaluate.c b/src/evaluate.c
index af52ab181b29..76b25b408d55 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -477,7 +477,7 @@ static void expr_evaluate_bits(struct eval_ctx *ctx, struct expr **exprp)
 					  &extra_len);
 		break;
 	case EXPR_EXTHDR:
-		shift = expr_offset_shift(expr, expr->exthdr.tmpl->offset,
+		shift = expr_offset_shift(expr, expr->exthdr.offset,
 					  &extra_len);
 		break;
 	default:
@@ -530,18 +530,16 @@ static int __expr_evaluate_exthdr(struct eval_ctx *ctx, struct expr **exprp)
 	if (expr_evaluate_primary(ctx, exprp) < 0)
 		return -1;
 
-	if (expr->exthdr.tmpl->offset % BITS_PER_BYTE != 0 ||
+	if (expr->exthdr.offset % BITS_PER_BYTE != 0 ||
 	    expr->len % BITS_PER_BYTE != 0)
 		expr_evaluate_bits(ctx, exprp);
 
 	switch (expr->exthdr.op) {
 	case NFT_EXTHDR_OP_TCPOPT: {
 		static const unsigned int max_tcpoptlen = (15 * 4 - 20) * BITS_PER_BYTE;
-		unsigned int totlen = 0;
+		unsigned int totlen;
 
-		totlen += expr->exthdr.tmpl->offset;
-		totlen += expr->exthdr.tmpl->len;
-		totlen += expr->exthdr.offset;
+		totlen = expr->exthdr.tmpl->len + expr->exthdr.offset;
 
 		if (totlen > max_tcpoptlen)
 			return expr_error(ctx->msgs, expr,
@@ -551,11 +549,9 @@ static int __expr_evaluate_exthdr(struct eval_ctx *ctx, struct expr **exprp)
 	}
 	case NFT_EXTHDR_OP_IPV4: {
 		static const unsigned int max_ipoptlen = 40 * BITS_PER_BYTE;
-		unsigned int totlen = 0;
+		unsigned int totlen;
 
-		totlen += expr->exthdr.tmpl->offset;
-		totlen += expr->exthdr.tmpl->len;
-		totlen += expr->exthdr.offset;
+		totlen = expr->exthdr.offset + expr->exthdr.tmpl->len;
 
 		if (totlen > max_ipoptlen)
 			return expr_error(ctx->msgs, expr,
diff --git a/src/exthdr.c b/src/exthdr.c
index 0b23e0d38b91..e1b7f14d4194 100644
--- a/src/exthdr.c
+++ b/src/exthdr.c
@@ -193,6 +193,7 @@ struct expr *exthdr_expr_alloc(const struct location *loc,
 			  BYTEORDER_BIG_ENDIAN, tmpl->len);
 	expr->exthdr.desc = desc;
 	expr->exthdr.tmpl = tmpl;
+	expr->exthdr.offset = tmpl->offset;
 	return expr;
 }
 
diff --git a/src/ipopt.c b/src/ipopt.c
index b3d0279d673a..7ecb8b9c8e32 100644
--- a/src/ipopt.c
+++ b/src/ipopt.c
@@ -102,7 +102,7 @@ struct expr *ipopt_expr_alloc(const struct location *loc, uint8_t type,
 	expr->exthdr.desc   = desc;
 	expr->exthdr.tmpl   = tmpl;
 	expr->exthdr.op     = NFT_EXTHDR_OP_IPV4;
-	expr->exthdr.offset = calc_offset(desc, tmpl, ptr);
+	expr->exthdr.offset = tmpl->offset + calc_offset(desc, tmpl, ptr);
 
 	return expr;
 }
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 32ec07a09121..9faddde63974 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -809,8 +809,8 @@ static void netlink_parse_numgen(struct netlink_parse_ctx *ctx,
 				 const struct location *loc,
 				 const struct nftnl_expr *nle)
 {
-	enum nft_registers dreg;
 	uint32_t type, until, offset;
+	enum nft_registers dreg;
 	struct expr *expr;
 
 	type  = nftnl_expr_get_u32(nle, NFTNL_EXPR_NG_TYPE);
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 23cf54639303..a37e4b940973 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -200,7 +200,7 @@ static void netlink_gen_exthdr(struct netlink_linearize_ctx *ctx,
 			       const struct expr *expr,
 			       enum nft_registers dreg)
 {
-	unsigned int offset = expr->exthdr.tmpl->offset + expr->exthdr.offset;
+	unsigned int offset = expr->exthdr.offset;
 	struct nftnl_expr *nle;
 
 	nle = alloc_nft_expr("exthdr");
@@ -966,7 +966,7 @@ static void netlink_gen_exthdr_stmt(struct netlink_linearize_ctx *ctx,
 
 	expr = stmt->exthdr.expr;
 
-	offset = expr->exthdr.tmpl->offset + expr->exthdr.offset;
+	offset = expr->exthdr.offset;
 
 	nle = alloc_nft_expr("exthdr");
 	netlink_put_register(nle, NFTNL_EXPR_EXTHDR_SREG, sreg);
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 379a6acd18e6..e8df98b8949a 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -5195,7 +5195,7 @@ tcp_hdr_expr		:	TCP	tcp_hdr_field
 			}
 			|	TCP	OPTION	tcp_hdr_option_type
 			{
-				$$ = tcpopt_expr_alloc(&@$, $3, TCPOPTHDR_FIELD_KIND);
+				$$ = tcpopt_expr_alloc(&@$, $3, TCPOPT_COMMON_KIND);
 				$$->exthdr.flags = NFT_EXTHDR_F_PRESENT;
 			}
 			;
@@ -5227,14 +5227,14 @@ tcp_hdr_option_type	:	EOL		{ $$ = TCPOPT_KIND_EOL; }
 			|	TIMESTAMP	{ $$ = TCPOPT_KIND_TIMESTAMP; }
 			;
 
-tcp_hdr_option_field	:	KIND		{ $$ = TCPOPTHDR_FIELD_KIND; }
-			|	LENGTH		{ $$ = TCPOPTHDR_FIELD_LENGTH; }
-			|	SIZE		{ $$ = TCPOPTHDR_FIELD_SIZE; }
-			|	COUNT		{ $$ = TCPOPTHDR_FIELD_COUNT; }
-			|	LEFT		{ $$ = TCPOPTHDR_FIELD_LEFT; }
-			|	RIGHT		{ $$ = TCPOPTHDR_FIELD_RIGHT; }
-			|	TSVAL		{ $$ = TCPOPTHDR_FIELD_TSVAL; }
-			|	TSECR		{ $$ = TCPOPTHDR_FIELD_TSECR; }
+tcp_hdr_option_field	:	KIND		{ $$ = TCPOPT_COMMON_KIND; }
+			|	LENGTH		{ $$ = TCPOPT_COMMON_LENGTH; }
+			|	SIZE		{ $$ = TCPOPT_MAXSEG_SIZE; }
+			|	COUNT		{ $$ = TCPOPT_WINDOW_COUNT; }
+			|	LEFT		{ $$ = TCPOPT_SACK_LEFT; }
+			|	RIGHT		{ $$ = TCPOPT_SACK_RIGHT; }
+			|	TSVAL		{ $$ = TCPOPT_TS_TSVAL; }
+			|	TSECR		{ $$ = TCPOPT_TS_TSECR; }
 			;
 
 dccp_hdr_expr		:	DCCP	dccp_hdr_field
diff --git a/src/parser_json.c b/src/parser_json.c
index c68b64d9f636..6e1333659f81 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -468,8 +468,10 @@ static int json_parse_tcp_option_type(const char *name, int *val)
 	}
 	/* special case for sack0 - sack3 */
 	if (sscanf(name, "sack%u", &i) == 1 && i < 4) {
-		if (val)
-			*val = TCPOPT_KIND_SACK + i;
+		if (val && i == 0)
+			*val = TCPOPT_KIND_SACK;
+		else if (val && i > 0)
+			*val = TCPOPT_KIND_SACK1 + i - 1;
 		return 0;
 	}
 	return 1;
@@ -477,12 +479,38 @@ static int json_parse_tcp_option_type(const char *name, int *val)
 
 static int json_parse_tcp_option_field(int type, const char *name, int *val)
 {
+	const struct exthdr_desc *desc;
+	unsigned int block = 0;
 	unsigned int i;
-	const struct exthdr_desc *desc = tcpopt_protocols[type];
+
+	switch (type) {
+	case TCPOPT_KIND_SACK1:
+		type = TCPOPT_KIND_SACK;
+		block = 1;
+		break;
+	case TCPOPT_KIND_SACK2:
+		type = TCPOPT_KIND_SACK;
+		block = 2;
+		break;
+	case TCPOPT_KIND_SACK3:
+		type = TCPOPT_KIND_SACK;
+		block = 3;
+		break;
+	}
+
+	if (type < 0 || type >= (int)array_size(tcpopt_protocols))
+		return 1;
+
+	desc = tcpopt_protocols[type];
 
 	for (i = 0; i < array_size(desc->templates); i++) {
 		if (desc->templates[i].token &&
 		    !strcmp(desc->templates[i].token, name)) {
+			if (block) {
+				block--;
+				continue;
+			}
+
 			if (val)
 				*val = i;
 			return 0;
@@ -587,7 +615,7 @@ static struct expr *json_parse_tcp_option_expr(struct json_ctx *ctx,
 
 	if (json_unpack(root, "{s:s}", "field", &field)) {
 		expr = tcpopt_expr_alloc(int_loc, descval,
-					 TCPOPTHDR_FIELD_KIND);
+					 TCPOPT_COMMON_KIND);
 		expr->exthdr.flags = NFT_EXTHDR_F_PRESENT;
 
 		return expr;
diff --git a/src/tcpopt.c b/src/tcpopt.c
index 17cb580d0ead..d1dd13b868e8 100644
--- a/src/tcpopt.c
+++ b/src/tcpopt.c
@@ -22,7 +22,7 @@ static const struct exthdr_desc tcpopt_eol = {
 	.name		= "eol",
 	.type		= TCPOPT_KIND_EOL,
 	.templates	= {
-		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",  0,    8),
+		[TCPOPT_COMMON_KIND]		= PHT("kind",  0,    8),
 	},
 };
 
@@ -30,7 +30,7 @@ static const struct exthdr_desc tcpopt_nop = {
 	.name		= "nop",
 	.type		= TCPOPT_KIND_NOP,
 	.templates	= {
-		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0,   8),
+		[TCPOPT_COMMON_KIND]		= PHT("kind",  0,    8),
 	},
 };
 
@@ -38,9 +38,9 @@ static const struct exthdr_desc tcptopt_maxseg = {
 	.name		= "maxseg",
 	.type		= TCPOPT_KIND_MAXSEG,
 	.templates	= {
-		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0,  8),
-		[TCPOPTHDR_FIELD_LENGTH]	= PHT("length", 8,  8),
-		[TCPOPTHDR_FIELD_SIZE]		= PHT("size",  16, 16),
+		[TCPOPT_MAXSEG_KIND]	= PHT("kind",   0,  8),
+		[TCPOPT_MAXSEG_LENGTH]	= PHT("length", 8,  8),
+		[TCPOPT_MAXSEG_SIZE]	= PHT("size",  16, 16),
 	},
 };
 
@@ -48,9 +48,9 @@ static const struct exthdr_desc tcpopt_window = {
 	.name		= "window",
 	.type		= TCPOPT_KIND_WINDOW,
 	.templates	= {
-		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0,  8),
-		[TCPOPTHDR_FIELD_LENGTH]	= PHT("length", 8,  8),
-		[TCPOPTHDR_FIELD_COUNT]		= PHT("count", 16,  8),
+		[TCPOPT_WINDOW_KIND]	= PHT("kind",   0,  8),
+		[TCPOPT_WINDOW_LENGTH]	= PHT("length", 8,  8),
+		[TCPOPT_WINDOW_COUNT]	= PHT("count", 16,  8),
 	},
 };
 
@@ -58,8 +58,8 @@ static const struct exthdr_desc tcpopt_sack_permitted = {
 	.name		= "sack-perm",
 	.type		= TCPOPT_KIND_SACK_PERMITTED,
 	.templates	= {
-		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0, 8),
-		[TCPOPTHDR_FIELD_LENGTH]	= PHT("length", 8, 8),
+		[TCPOPT_COMMON_KIND]	= PHT("kind",   0, 8),
+		[TCPOPT_COMMON_LENGTH]	= PHT("length", 8, 8),
 	},
 };
 
@@ -67,10 +67,16 @@ static const struct exthdr_desc tcpopt_sack = {
 	.name		= "sack",
 	.type		= TCPOPT_KIND_SACK,
 	.templates	= {
-		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0,   8),
-		[TCPOPTHDR_FIELD_LENGTH]		= PHT("length", 8,   8),
-		[TCPOPTHDR_FIELD_LEFT]		= PHT("left",  16,  32),
-		[TCPOPTHDR_FIELD_RIGHT]		= PHT("right", 48,  32),
+		[TCPOPT_SACK_KIND]	= PHT("kind",   0,   8),
+		[TCPOPT_SACK_LENGTH]	= PHT("length", 8,   8),
+		[TCPOPT_SACK_LEFT]	= PHT("left",  16,  32),
+		[TCPOPT_SACK_RIGHT]	= PHT("right", 48,  32),
+		[TCPOPT_SACK_LEFT1]	= PHT("left",  80,  32),
+		[TCPOPT_SACK_RIGHT1]	= PHT("right", 112,  32),
+		[TCPOPT_SACK_LEFT2]	= PHT("left",  144,  32),
+		[TCPOPT_SACK_RIGHT2]	= PHT("right", 176,  32),
+		[TCPOPT_SACK_LEFT3]	= PHT("left",  208,  32),
+		[TCPOPT_SACK_RIGHT3]	= PHT("right", 240,  32),
 	},
 };
 
@@ -78,12 +84,13 @@ static const struct exthdr_desc tcpopt_timestamp = {
 	.name		= "timestamp",
 	.type		= TCPOPT_KIND_TIMESTAMP,
 	.templates	= {
-		[TCPOPTHDR_FIELD_KIND]		= PHT("kind",   0,  8),
-		[TCPOPTHDR_FIELD_LENGTH]	= PHT("length", 8,  8),
-		[TCPOPTHDR_FIELD_TSVAL]		= PHT("tsval",  16, 32),
-		[TCPOPTHDR_FIELD_TSECR]		= PHT("tsecr",  48, 32),
+		[TCPOPT_TS_KIND]	= PHT("kind",   0,  8),
+		[TCPOPT_TS_LENGTH]	= PHT("length", 8,  8),
+		[TCPOPT_TS_TSVAL]	= PHT("tsval",  16, 32),
+		[TCPOPT_TS_TSECR]	= PHT("tsecr",  48, 32),
 	},
 };
+
 #undef PHT
 
 const struct exthdr_desc *tcpopt_protocols[] = {
@@ -96,65 +103,43 @@ const struct exthdr_desc *tcpopt_protocols[] = {
 	[TCPOPT_KIND_TIMESTAMP]		= &tcpopt_timestamp,
 };
 
-static unsigned int calc_offset(const struct exthdr_desc *desc,
-				const struct proto_hdr_template *tmpl,
-				unsigned int num)
-{
-	if (!desc || tmpl == &tcpopt_unknown_template)
-		return 0;
-
-	switch (desc->type) {
-	case TCPOPT_SACK:
-		/* Make sure, offset calculations only apply to left and right
-		 * fields
-		 */
-		return (tmpl->offset < 16) ? 0 : num * 64;
-	default:
-		return 0;
-	}
-}
-
-
-static unsigned int calc_offset_reverse(const struct exthdr_desc *desc,
-					const struct proto_hdr_template *tmpl,
-					unsigned int offset)
-{
-	if (!desc || tmpl == &tcpopt_unknown_template)
-		return offset;
-
-	switch (desc->type) {
-	case TCPOPT_SACK:
-		/* We can safely ignore the first left/right field */
-		return offset < 80 ? offset : (offset % 64);
-	default:
-		return offset;
-	}
-}
-
 struct expr *tcpopt_expr_alloc(const struct location *loc,
 			       unsigned int kind,
 			       unsigned int field)
 {
 	const struct proto_hdr_template *tmpl;
-	const struct exthdr_desc *desc;
-	uint8_t optnum = 0;
+	const struct exthdr_desc *desc = NULL;
 	struct expr *expr;
 
 	switch (kind) {
 	case TCPOPT_KIND_SACK1:
 		kind = TCPOPT_KIND_SACK;
-		optnum = 1;
+		if (field == TCPOPT_SACK_LEFT)
+			field = TCPOPT_SACK_LEFT1;
+		else if (field == TCPOPT_SACK_RIGHT)
+			field = TCPOPT_SACK_RIGHT1;
 		break;
 	case TCPOPT_KIND_SACK2:
 		kind = TCPOPT_KIND_SACK;
-		optnum = 2;
+		if (field == TCPOPT_SACK_LEFT)
+			field = TCPOPT_SACK_LEFT2;
+		else if (field == TCPOPT_SACK_RIGHT)
+			field = TCPOPT_SACK_RIGHT2;
 		break;
 	case TCPOPT_KIND_SACK3:
 		kind = TCPOPT_KIND_SACK;
-		optnum = 3;
+		if (field == TCPOPT_SACK_LEFT)
+			field = TCPOPT_SACK_LEFT3;
+		else if (field == TCPOPT_SACK_RIGHT)
+			field = TCPOPT_SACK_RIGHT3;
+		break;
 	}
 
-	desc = tcpopt_protocols[kind];
+	if (kind < array_size(tcpopt_protocols))
+		desc = tcpopt_protocols[kind];
+
+	if (!desc)
+		return NULL;
 	tmpl = &desc->templates[field];
 	if (!tmpl)
 		return NULL;
@@ -164,34 +149,32 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
 	expr->exthdr.desc   = desc;
 	expr->exthdr.tmpl   = tmpl;
 	expr->exthdr.op     = NFT_EXTHDR_OP_TCPOPT;
-	expr->exthdr.offset = calc_offset(desc, tmpl, optnum);
+	expr->exthdr.offset = tmpl->offset;
 
 	return expr;
 }
 
-void tcpopt_init_raw(struct expr *expr, uint8_t type, unsigned int offset,
+void tcpopt_init_raw(struct expr *expr, uint8_t type, unsigned int off,
 		     unsigned int len, uint32_t flags)
 {
 	const struct proto_hdr_template *tmpl;
-	unsigned int i, off;
+	unsigned int i;
 
 	assert(expr->etype == EXPR_EXTHDR);
 
 	expr->len = len;
 	expr->exthdr.flags = flags;
-	expr->exthdr.offset = offset;
+	expr->exthdr.offset = off;
+
+	if (type >= array_size(tcpopt_protocols))
+		return;
 
-	assert(type < array_size(tcpopt_protocols));
 	expr->exthdr.desc = tcpopt_protocols[type];
 	expr->exthdr.flags = flags;
 	assert(expr->exthdr.desc != NULL);
 
 	for (i = 0; i < array_size(expr->exthdr.desc->templates); ++i) {
 		tmpl = &expr->exthdr.desc->templates[i];
-		/* We have to reverse calculate the offset for the sack options
-		 * at this point
-		 */
-		off = calc_offset_reverse(expr->exthdr.desc, tmpl, offset);
 		if (tmpl->offset != off || tmpl->len != len)
 			continue;
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH nft 5/7] tcpopt: allow to check for presence of any tcp option
  2020-11-05 14:11 [PATCH nft 0/7] rework tcp option handling Florian Westphal
                   ` (3 preceding siblings ...)
  2020-11-05 14:11 ` [PATCH nft 4/7] tcpopt: split tcpopt_hdr_fields into per-option enum Florian Westphal
@ 2020-11-05 14:11 ` Florian Westphal
  2020-11-05 19:11   ` Jeremy Sowden
  2020-11-05 14:11 ` [PATCH nft 6/7] tcp: add raw tcp option match support Florian Westphal
  2020-11-05 14:11 ` [PATCH nft 7/7] json: " Florian Westphal
  6 siblings, 1 reply; 14+ messages in thread
From: Florian Westphal @ 2020-11-05 14:11 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

nft currently doesn't allow to check for presence of arbitrary tcp options.
Only known options where nft provides a template can be tested for.

This allows to test for presence of raw protocol values as well.

Example:

tcp option 42 exists

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/expression.h          |  3 +-
 src/exthdr.c                  | 12 ++++++++
 src/ipopt.c                   |  1 +
 src/netlink_linearize.c       |  2 +-
 src/parser_bison.y            |  7 +++++
 src/tcpopt.c                  | 42 +++++++++++++++++++++++----
 tests/py/any/tcpopt.t         |  2 ++
 tests/py/any/tcpopt.t.payload | 53 +++--------------------------------
 8 files changed, 65 insertions(+), 57 deletions(-)

diff --git a/include/expression.h b/include/expression.h
index b039882cf1d1..894a68d2e822 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -311,7 +311,8 @@ struct expr {
 			/* EXPR_EXTHDR */
 			const struct exthdr_desc	*desc;
 			const struct proto_hdr_template	*tmpl;
-			unsigned int			offset;
+			uint16_t			offset;
+			uint8_t				raw_type;
 			enum nft_exthdr_op		op;
 			unsigned int			flags;
 		} exthdr;
diff --git a/src/exthdr.c b/src/exthdr.c
index e1b7f14d4194..8995ad1775a0 100644
--- a/src/exthdr.c
+++ b/src/exthdr.c
@@ -52,6 +52,13 @@ static void exthdr_expr_print(const struct expr *expr, struct output_ctx *octx)
 		 */
 		unsigned int offset = expr->exthdr.offset / 64;
 
+		if (expr->exthdr.desc == NULL &&
+		    expr->exthdr.offset == 0 &&
+		    expr->exthdr.flags & NFT_EXTHDR_F_PRESENT) {
+			nft_print(octx, "tcp option %d", expr->exthdr.raw_type);
+			return;
+		}
+
 		nft_print(octx, "tcp option %s", expr->exthdr.desc->name);
 		if (expr->exthdr.flags & NFT_EXTHDR_F_PRESENT)
 			return;
@@ -79,6 +86,7 @@ static bool exthdr_expr_cmp(const struct expr *e1, const struct expr *e2)
 	return e1->exthdr.desc == e2->exthdr.desc &&
 	       e1->exthdr.tmpl == e2->exthdr.tmpl &&
 	       e1->exthdr.op == e2->exthdr.op &&
+	       e1->exthdr.raw_type == e2->exthdr.raw_type &&
 	       e1->exthdr.flags == e2->exthdr.flags;
 }
 
@@ -89,6 +97,7 @@ static void exthdr_expr_clone(struct expr *new, const struct expr *expr)
 	new->exthdr.offset = expr->exthdr.offset;
 	new->exthdr.op = expr->exthdr.op;
 	new->exthdr.flags = expr->exthdr.flags;
+	new->exthdr.raw_type = expr->exthdr.raw_type;
 }
 
 #define NFTNL_UDATA_EXTHDR_DESC 0
@@ -192,6 +201,7 @@ struct expr *exthdr_expr_alloc(const struct location *loc,
 	expr = expr_alloc(loc, EXPR_EXTHDR, tmpl->dtype,
 			  BYTEORDER_BIG_ENDIAN, tmpl->len);
 	expr->exthdr.desc = desc;
+	expr->exthdr.raw_type = desc ? desc->type : 0;
 	expr->exthdr.tmpl = tmpl;
 	expr->exthdr.offset = tmpl->offset;
 	return expr;
@@ -270,6 +280,8 @@ void exthdr_init_raw(struct expr *expr, uint8_t type,
 	unsigned int i;
 
 	assert(expr->etype == EXPR_EXTHDR);
+	expr->exthdr.raw_type = type;
+
 	if (op == NFT_EXTHDR_OP_TCPOPT)
 		return tcpopt_init_raw(expr, type, offset, len, flags);
 	if (op == NFT_EXTHDR_OP_IPV4)
diff --git a/src/ipopt.c b/src/ipopt.c
index 7ecb8b9c8e32..5f9f908c0b34 100644
--- a/src/ipopt.c
+++ b/src/ipopt.c
@@ -103,6 +103,7 @@ struct expr *ipopt_expr_alloc(const struct location *loc, uint8_t type,
 	expr->exthdr.tmpl   = tmpl;
 	expr->exthdr.op     = NFT_EXTHDR_OP_IPV4;
 	expr->exthdr.offset = tmpl->offset + calc_offset(desc, tmpl, ptr);
+	expr->exthdr.raw_type = desc->type;
 
 	return expr;
 }
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index a37e4b940973..05af8bb1b485 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -206,7 +206,7 @@ static void netlink_gen_exthdr(struct netlink_linearize_ctx *ctx,
 	nle = alloc_nft_expr("exthdr");
 	netlink_put_register(nle, NFTNL_EXPR_EXTHDR_DREG, dreg);
 	nftnl_expr_set_u8(nle, NFTNL_EXPR_EXTHDR_TYPE,
-			  expr->exthdr.desc->type);
+			  expr->exthdr.raw_type);
 	nftnl_expr_set_u32(nle, NFTNL_EXPR_EXTHDR_OFFSET, offset / BITS_PER_BYTE);
 	nftnl_expr_set_u32(nle, NFTNL_EXPR_EXTHDR_LEN,
 			   div_round_up(expr->len, BITS_PER_BYTE));
diff --git a/src/parser_bison.y b/src/parser_bison.y
index e8df98b8949a..393f66862810 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -5225,6 +5225,13 @@ tcp_hdr_option_type	:	EOL		{ $$ = TCPOPT_KIND_EOL; }
 			|	SACK3		{ $$ = TCPOPT_KIND_SACK3; }
 			|	ECHO		{ $$ = TCPOPT_KIND_ECHO; }
 			|	TIMESTAMP	{ $$ = TCPOPT_KIND_TIMESTAMP; }
+			|	NUM		{
+				if ($1 > 255) {
+					erec_queue(error(&@1, "value too large"), state->msgs);
+					YYERROR;
+				}
+				$$ = $1;
+			}
 			;
 
 tcp_hdr_option_field	:	KIND		{ $$ = TCPOPT_COMMON_KIND; }
diff --git a/src/tcpopt.c b/src/tcpopt.c
index d1dd13b868e8..1cf97a563bc2 100644
--- a/src/tcpopt.c
+++ b/src/tcpopt.c
@@ -103,6 +103,19 @@ const struct exthdr_desc *tcpopt_protocols[] = {
 	[TCPOPT_KIND_TIMESTAMP]		= &tcpopt_timestamp,
 };
 
+/**
+ * tcpopt_expr_alloc - allocate tcp option extension expression
+ *
+ * @loc: location from parser
+ * @kind: raw tcp option value to find in packet
+ * @field: highlevel field to find in the option if @kind is present in packet
+ *
+ * Allocate a new tcp option expression.
+ * @kind is the raw option value to find in the packet.
+ * Exception: SACK may use extra OOB data that is mangled here.
+ *
+ * @field is the optional field to extract from the @type option.
+ */
 struct expr *tcpopt_expr_alloc(const struct location *loc,
 			       unsigned int kind,
 			       unsigned int field)
@@ -138,8 +151,22 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
 	if (kind < array_size(tcpopt_protocols))
 		desc = tcpopt_protocols[kind];
 
-	if (!desc)
-		return NULL;
+	if (!desc) {
+		if (field != TCPOPT_COMMON_KIND || kind > 255)
+			return NULL;
+
+		expr = expr_alloc(loc, EXPR_EXTHDR, &integer_type,
+				  BYTEORDER_BIG_ENDIAN, 8);
+
+		desc = tcpopt_protocols[TCPOPT_NOP];
+		tmpl = &desc->templates[field];
+		expr->exthdr.desc   = desc;
+		expr->exthdr.tmpl   = tmpl;
+		expr->exthdr.op = NFT_EXTHDR_OP_TCPOPT;
+		expr->exthdr.raw_type = kind;
+		return expr;
+	}
+
 	tmpl = &desc->templates[field];
 	if (!tmpl)
 		return NULL;
@@ -149,6 +176,7 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
 	expr->exthdr.desc   = desc;
 	expr->exthdr.tmpl   = tmpl;
 	expr->exthdr.op     = NFT_EXTHDR_OP_TCPOPT;
+	expr->exthdr.raw_type = desc->type;
 	expr->exthdr.offset = tmpl->offset;
 
 	return expr;
@@ -165,6 +193,10 @@ void tcpopt_init_raw(struct expr *expr, uint8_t type, unsigned int off,
 	expr->len = len;
 	expr->exthdr.flags = flags;
 	expr->exthdr.offset = off;
+	expr->exthdr.op = NFT_EXTHDR_OP_TCPOPT;
+
+	if (flags & NFT_EXTHDR_F_PRESENT)
+		datatype_set(expr, &boolean_type);
 
 	if (type >= array_size(tcpopt_protocols))
 		return;
@@ -178,12 +210,10 @@ void tcpopt_init_raw(struct expr *expr, uint8_t type, unsigned int off,
 		if (tmpl->offset != off || tmpl->len != len)
 			continue;
 
-		if (flags & NFT_EXTHDR_F_PRESENT)
-			datatype_set(expr, &boolean_type);
-		else
+		if ((flags & NFT_EXTHDR_F_PRESENT) == 0)
 			datatype_set(expr, tmpl->dtype);
+
 		expr->exthdr.tmpl = tmpl;
-		expr->exthdr.op   = NFT_EXTHDR_OP_TCPOPT;
 		break;
 	}
 }
diff --git a/tests/py/any/tcpopt.t b/tests/py/any/tcpopt.t
index 1d42de8746cd..7b17014b3003 100644
--- a/tests/py/any/tcpopt.t
+++ b/tests/py/any/tcpopt.t
@@ -30,6 +30,7 @@ tcp option timestamp kind 1;ok
 tcp option timestamp length 1;ok
 tcp option timestamp tsval 1;ok
 tcp option timestamp tsecr 1;ok
+tcp option 255 missing;ok
 
 tcp option foobar;fail
 tcp option foo bar;fail
@@ -38,6 +39,7 @@ tcp option eol left 1;fail
 tcp option eol left 1;fail
 tcp option sack window;fail
 tcp option sack window 1;fail
+tcp option 256 exists;fail
 
 tcp option window exists;ok
 tcp option window missing;ok
diff --git a/tests/py/any/tcpopt.t.payload b/tests/py/any/tcpopt.t.payload
index 9c480c8bd06b..34f8e26c4409 100644
--- a/tests/py/any/tcpopt.t.payload
+++ b/tests/py/any/tcpopt.t.payload
@@ -509,20 +509,6 @@ inet
   [ exthdr load tcpopt 4b @ 8 + 2 => reg 1 ]
   [ cmp eq reg 1 0x01000000 ]
 
-# tcp option timestamp tsecr 1
-ip 
-  [ meta load l4proto => reg 1 ]
-  [ cmp eq reg 1 0x00000006 ]
-  [ exthdr load tcpopt 4b @ 8 + 6 => reg 1 ]
-  [ cmp eq reg 1 0x01000000 ]
-
-# tcp option timestamp tsecr 1
-ip6 
-  [ meta load l4proto => reg 1 ]
-  [ cmp eq reg 1 0x00000006 ]
-  [ exthdr load tcpopt 4b @ 8 + 6 => reg 1 ]
-  [ cmp eq reg 1 0x01000000 ]
-
 # tcp option timestamp tsecr 1
 inet 
   [ meta load l4proto => reg 1 ]
@@ -530,19 +516,12 @@ inet
   [ exthdr load tcpopt 4b @ 8 + 6 => reg 1 ]
   [ cmp eq reg 1 0x01000000 ]
 
-# tcp option window exists
-ip 
-  [ meta load l4proto => reg 1 ]
-  [ cmp eq reg 1 0x00000006 ]
-  [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
-  [ cmp eq reg 1 0x00000001 ]
-
-# tcp option window exists
-ip6 
+# tcp option 255 missing
+inet
   [ meta load l4proto => reg 1 ]
   [ cmp eq reg 1 0x00000006 ]
-  [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
-  [ cmp eq reg 1 0x00000001 ]
+  [ exthdr load tcpopt 1b @ 255 + 0 present => reg 1 ]
+  [ cmp eq reg 1 0x00000000 ]
 
 # tcp option window exists
 inet 
@@ -551,20 +530,6 @@ inet
   [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
   [ cmp eq reg 1 0x00000001 ]
 
-# tcp option window missing
-ip 
-  [ meta load l4proto => reg 1 ]
-  [ cmp eq reg 1 0x00000006 ]
-  [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
-  [ cmp eq reg 1 0x00000000 ]
-
-# tcp option window missing
-ip6 
-  [ meta load l4proto => reg 1 ]
-  [ cmp eq reg 1 0x00000006 ]
-  [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
-  [ cmp eq reg 1 0x00000000 ]
-
 # tcp option window missing
 inet 
   [ meta load l4proto => reg 1 ]
@@ -572,16 +537,6 @@ inet
   [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
   [ cmp eq reg 1 0x00000000 ]
 
-# tcp option maxseg size set 1360
-ip 
-  [ immediate reg 1 0x00005005 ]
-  [ exthdr write tcpopt reg 1 => 2b @ 2 + 2 ]
-
-# tcp option maxseg size set 1360
-ip6 
-  [ immediate reg 1 0x00005005 ]
-  [ exthdr write tcpopt reg 1 => 2b @ 2 + 2 ]
-
 # tcp option maxseg size set 1360
 inet 
   [ immediate reg 1 0x00005005 ]
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH nft 5/7] tcpopt: allow to check for presence of any tcp option
  2020-11-05 14:11 ` [PATCH nft 5/7] tcpopt: allow to check for presence of any tcp option Florian Westphal
@ 2020-11-05 19:11   ` Jeremy Sowden
  2020-11-05 20:57     ` Jeremy Sowden
  0 siblings, 1 reply; 14+ messages in thread
From: Jeremy Sowden @ 2020-11-05 19:11 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

[-- Attachment #1: Type: text/plain, Size: 10844 bytes --]

On 2020-11-05, at 15:11:42 +0100, Florian Westphal wrote:
> nft currently doesn't allow to check for presence of arbitrary tcp options.
> Only known options where nft provides a template can be tested for.
>
> This allows to test for presence of raw protocol values as well.
>
> Example:
>
> tcp option 42 exists
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  include/expression.h          |  3 +-
>  src/exthdr.c                  | 12 ++++++++
>  src/ipopt.c                   |  1 +
>  src/netlink_linearize.c       |  2 +-
>  src/parser_bison.y            |  7 +++++
>  src/tcpopt.c                  | 42 +++++++++++++++++++++++----
>  tests/py/any/tcpopt.t         |  2 ++
>  tests/py/any/tcpopt.t.payload | 53 +++--------------------------------
>  8 files changed, 65 insertions(+), 57 deletions(-)
>
> diff --git a/include/expression.h b/include/expression.h
> index b039882cf1d1..894a68d2e822 100644
> --- a/include/expression.h
> +++ b/include/expression.h
> @@ -311,7 +311,8 @@ struct expr {
>  			/* EXPR_EXTHDR */
>  			const struct exthdr_desc	*desc;
>  			const struct proto_hdr_template	*tmpl;
> -			unsigned int			offset;
> +			uint16_t			offset;
> +			uint8_t				raw_type;
>  			enum nft_exthdr_op		op;
>  			unsigned int			flags;
>  		} exthdr;
> diff --git a/src/exthdr.c b/src/exthdr.c
> index e1b7f14d4194..8995ad1775a0 100644
> --- a/src/exthdr.c
> +++ b/src/exthdr.c
> @@ -52,6 +52,13 @@ static void exthdr_expr_print(const struct expr *expr, struct output_ctx *octx)
>  		 */
>  		unsigned int offset = expr->exthdr.offset / 64;
>
> +		if (expr->exthdr.desc == NULL &&
> +		    expr->exthdr.offset == 0 &&
> +		    expr->exthdr.flags & NFT_EXTHDR_F_PRESENT) {
> +			nft_print(octx, "tcp option %d", expr->exthdr.raw_type);
> +			return;
> +		}
> +
>  		nft_print(octx, "tcp option %s", expr->exthdr.desc->name);
>  		if (expr->exthdr.flags & NFT_EXTHDR_F_PRESENT)
>  			return;
> @@ -79,6 +86,7 @@ static bool exthdr_expr_cmp(const struct expr *e1, const struct expr *e2)
>  	return e1->exthdr.desc == e2->exthdr.desc &&
>  	       e1->exthdr.tmpl == e2->exthdr.tmpl &&
>  	       e1->exthdr.op == e2->exthdr.op &&
> +	       e1->exthdr.raw_type == e2->exthdr.raw_type &&
>  	       e1->exthdr.flags == e2->exthdr.flags;
>  }
>
> @@ -89,6 +97,7 @@ static void exthdr_expr_clone(struct expr *new, const struct expr *expr)
>  	new->exthdr.offset = expr->exthdr.offset;
>  	new->exthdr.op = expr->exthdr.op;
>  	new->exthdr.flags = expr->exthdr.flags;
> +	new->exthdr.raw_type = expr->exthdr.raw_type;
>  }
>
>  #define NFTNL_UDATA_EXTHDR_DESC 0
> @@ -192,6 +201,7 @@ struct expr *exthdr_expr_alloc(const struct location *loc,
>  	expr = expr_alloc(loc, EXPR_EXTHDR, tmpl->dtype,
>  			  BYTEORDER_BIG_ENDIAN, tmpl->len);
>  	expr->exthdr.desc = desc;
> +	expr->exthdr.raw_type = desc ? desc->type : 0;
>  	expr->exthdr.tmpl = tmpl;
>  	expr->exthdr.offset = tmpl->offset;
>  	return expr;
> @@ -270,6 +280,8 @@ void exthdr_init_raw(struct expr *expr, uint8_t type,
>  	unsigned int i;
>
>  	assert(expr->etype == EXPR_EXTHDR);
> +	expr->exthdr.raw_type = type;
> +
>  	if (op == NFT_EXTHDR_OP_TCPOPT)
>  		return tcpopt_init_raw(expr, type, offset, len, flags);
>  	if (op == NFT_EXTHDR_OP_IPV4)
> diff --git a/src/ipopt.c b/src/ipopt.c
> index 7ecb8b9c8e32..5f9f908c0b34 100644
> --- a/src/ipopt.c
> +++ b/src/ipopt.c
> @@ -103,6 +103,7 @@ struct expr *ipopt_expr_alloc(const struct location *loc, uint8_t type,
>  	expr->exthdr.tmpl   = tmpl;
>  	expr->exthdr.op     = NFT_EXTHDR_OP_IPV4;
>  	expr->exthdr.offset = tmpl->offset + calc_offset(desc, tmpl, ptr);
> +	expr->exthdr.raw_type = desc->type;
>
>  	return expr;
>  }
> diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
> index a37e4b940973..05af8bb1b485 100644
> --- a/src/netlink_linearize.c
> +++ b/src/netlink_linearize.c
> @@ -206,7 +206,7 @@ static void netlink_gen_exthdr(struct netlink_linearize_ctx *ctx,
>  	nle = alloc_nft_expr("exthdr");
>  	netlink_put_register(nle, NFTNL_EXPR_EXTHDR_DREG, dreg);
>  	nftnl_expr_set_u8(nle, NFTNL_EXPR_EXTHDR_TYPE,
> -			  expr->exthdr.desc->type);
> +			  expr->exthdr.raw_type);
>  	nftnl_expr_set_u32(nle, NFTNL_EXPR_EXTHDR_OFFSET, offset / BITS_PER_BYTE);
>  	nftnl_expr_set_u32(nle, NFTNL_EXPR_EXTHDR_LEN,
>  			   div_round_up(expr->len, BITS_PER_BYTE));
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index e8df98b8949a..393f66862810 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -5225,6 +5225,13 @@ tcp_hdr_option_type	:	EOL		{ $$ = TCPOPT_KIND_EOL; }
>  			|	SACK3		{ $$ = TCPOPT_KIND_SACK3; }
>  			|	ECHO		{ $$ = TCPOPT_KIND_ECHO; }
>  			|	TIMESTAMP	{ $$ = TCPOPT_KIND_TIMESTAMP; }
> +			|	NUM		{
> +				if ($1 > 255) {
> +					erec_queue(error(&@1, "value too large"), state->msgs);
> +					YYERROR;
> +				}
> +				$$ = $1;
> +			}
>  			;
>
>  tcp_hdr_option_field	:	KIND		{ $$ = TCPOPT_COMMON_KIND; }
> diff --git a/src/tcpopt.c b/src/tcpopt.c
> index d1dd13b868e8..1cf97a563bc2 100644
> --- a/src/tcpopt.c
> +++ b/src/tcpopt.c
> @@ -103,6 +103,19 @@ const struct exthdr_desc *tcpopt_protocols[] = {
>  	[TCPOPT_KIND_TIMESTAMP]		= &tcpopt_timestamp,
>  };
>
> +/**
> + * tcpopt_expr_alloc - allocate tcp option extension expression
> + *
> + * @loc: location from parser
> + * @kind: raw tcp option value to find in packet
> + * @field: highlevel field to find in the option if @kind is present in packet
> + *
> + * Allocate a new tcp option expression.
> + * @kind is the raw option value to find in the packet.
> + * Exception: SACK may use extra OOB data that is mangled here.
> + *
> + * @field is the optional field to extract from the @type option.
> + */
>  struct expr *tcpopt_expr_alloc(const struct location *loc,
>  			       unsigned int kind,
>  			       unsigned int field)
> @@ -138,8 +151,22 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
>  	if (kind < array_size(tcpopt_protocols))
>  		desc = tcpopt_protocols[kind];
>
> -	if (!desc)
> -		return NULL;
> +	if (!desc) {
> +		if (field != TCPOPT_COMMON_KIND || kind > 255)
> +			return NULL;
> +
> +		expr = expr_alloc(loc, EXPR_EXTHDR, &integer_type,
> +				  BYTEORDER_BIG_ENDIAN, 8);
> +
> +		desc = tcpopt_protocols[TCPOPT_NOP];
> +		tmpl = &desc->templates[field];
> +		expr->exthdr.desc   = desc;
> +		expr->exthdr.tmpl   = tmpl;
> +		expr->exthdr.op = NFT_EXTHDR_OP_TCPOPT;
> +		expr->exthdr.raw_type = kind;
> +		return expr;
> +	}
> +
>  	tmpl = &desc->templates[field];
>  	if (!tmpl)
>  		return NULL;
> @@ -149,6 +176,7 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
>  	expr->exthdr.desc   = desc;
>  	expr->exthdr.tmpl   = tmpl;
>  	expr->exthdr.op     = NFT_EXTHDR_OP_TCPOPT;
> +	expr->exthdr.raw_type = desc->type;
>  	expr->exthdr.offset = tmpl->offset;
>
>  	return expr;
> @@ -165,6 +193,10 @@ void tcpopt_init_raw(struct expr *expr, uint8_t type, unsigned int off,
>  	expr->len = len;
>  	expr->exthdr.flags = flags;
>  	expr->exthdr.offset = off;
> +	expr->exthdr.op = NFT_EXTHDR_OP_TCPOPT;
> +
> +	if (flags & NFT_EXTHDR_F_PRESENT)
> +		datatype_set(expr, &boolean_type);
>
>  	if (type >= array_size(tcpopt_protocols))
>  		return;
> @@ -178,12 +210,10 @@ void tcpopt_init_raw(struct expr *expr, uint8_t type, unsigned int off,
>  		if (tmpl->offset != off || tmpl->len != len)
>  			continue;
>
> -		if (flags & NFT_EXTHDR_F_PRESENT)
> -			datatype_set(expr, &boolean_type);
> -		else
> +		if ((flags & NFT_EXTHDR_F_PRESENT) == 0)
>  			datatype_set(expr, tmpl->dtype);
> +
>  		expr->exthdr.tmpl = tmpl;
> -		expr->exthdr.op   = NFT_EXTHDR_OP_TCPOPT;
>  		break;
>  	}
>  }
> diff --git a/tests/py/any/tcpopt.t b/tests/py/any/tcpopt.t
> index 1d42de8746cd..7b17014b3003 100644
> --- a/tests/py/any/tcpopt.t
> +++ b/tests/py/any/tcpopt.t
> @@ -30,6 +30,7 @@ tcp option timestamp kind 1;ok
>  tcp option timestamp length 1;ok
>  tcp option timestamp tsval 1;ok
>  tcp option timestamp tsecr 1;ok
> +tcp option 255 missing;ok
>
>  tcp option foobar;fail
>  tcp option foo bar;fail
> @@ -38,6 +39,7 @@ tcp option eol left 1;fail
>  tcp option eol left 1;fail
>  tcp option sack window;fail
>  tcp option sack window 1;fail
> +tcp option 256 exists;fail
>
>  tcp option window exists;ok
>  tcp option window missing;ok
> diff --git a/tests/py/any/tcpopt.t.payload b/tests/py/any/tcpopt.t.payload
> index 9c480c8bd06b..34f8e26c4409 100644
> --- a/tests/py/any/tcpopt.t.payload
> +++ b/tests/py/any/tcpopt.t.payload
> @@ -509,20 +509,6 @@ inet
>    [ exthdr load tcpopt 4b @ 8 + 2 => reg 1 ]
>    [ cmp eq reg 1 0x01000000 ]
>
> -# tcp option timestamp tsecr 1
> -ip
> -  [ meta load l4proto => reg 1 ]
> -  [ cmp eq reg 1 0x00000006 ]
> -  [ exthdr load tcpopt 4b @ 8 + 6 => reg 1 ]
> -  [ cmp eq reg 1 0x01000000 ]
> -
> -# tcp option timestamp tsecr 1
> -ip6
> -  [ meta load l4proto => reg 1 ]
> -  [ cmp eq reg 1 0x00000006 ]
> -  [ exthdr load tcpopt 4b @ 8 + 6 => reg 1 ]
> -  [ cmp eq reg 1 0x01000000 ]
> -

You are removing unrelated duplicate payloads.  Is it worth doing that
separately?

>  # tcp option timestamp tsecr 1
>  inet
>    [ meta load l4proto => reg 1 ]
> @@ -530,19 +516,12 @@ inet
>    [ exthdr load tcpopt 4b @ 8 + 6 => reg 1 ]
>    [ cmp eq reg 1 0x01000000 ]
>
> -# tcp option window exists
> -ip
> -  [ meta load l4proto => reg 1 ]
> -  [ cmp eq reg 1 0x00000006 ]
> -  [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
> -  [ cmp eq reg 1 0x00000001 ]
> -
> -# tcp option window exists
> -ip6
> +# tcp option 255 missing
> +inet
>    [ meta load l4proto => reg 1 ]
>    [ cmp eq reg 1 0x00000006 ]
> -  [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
> -  [ cmp eq reg 1 0x00000001 ]
> +  [ exthdr load tcpopt 1b @ 255 + 0 present => reg 1 ]
> +  [ cmp eq reg 1 0x00000000 ]
>
>  # tcp option window exists
>  inet
> @@ -551,20 +530,6 @@ inet
>    [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
>    [ cmp eq reg 1 0x00000001 ]
>
> -# tcp option window missing
> -ip
> -  [ meta load l4proto => reg 1 ]
> -  [ cmp eq reg 1 0x00000006 ]
> -  [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
> -  [ cmp eq reg 1 0x00000000 ]
> -
> -# tcp option window missing
> -ip6
> -  [ meta load l4proto => reg 1 ]
> -  [ cmp eq reg 1 0x00000006 ]
> -  [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
> -  [ cmp eq reg 1 0x00000000 ]
> -
>  # tcp option window missing
>  inet
>    [ meta load l4proto => reg 1 ]
> @@ -572,16 +537,6 @@ inet
>    [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
>    [ cmp eq reg 1 0x00000000 ]
>
> -# tcp option maxseg size set 1360
> -ip
> -  [ immediate reg 1 0x00005005 ]
> -  [ exthdr write tcpopt reg 1 => 2b @ 2 + 2 ]
> -
> -# tcp option maxseg size set 1360
> -ip6
> -  [ immediate reg 1 0x00005005 ]
> -  [ exthdr write tcpopt reg 1 => 2b @ 2 + 2 ]
> -
>  # tcp option maxseg size set 1360
>  inet
>    [ immediate reg 1 0x00005005 ]
> --
> 2.26.2
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH nft 5/7] tcpopt: allow to check for presence of any tcp option
  2020-11-05 19:11   ` Jeremy Sowden
@ 2020-11-05 20:57     ` Jeremy Sowden
  2020-11-09 11:10       ` Florian Westphal
  0 siblings, 1 reply; 14+ messages in thread
From: Jeremy Sowden @ 2020-11-05 20:57 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

[-- Attachment #1: Type: text/plain, Size: 12075 bytes --]

On 2020-11-05, at 19:11:46 +0000, Jeremy Sowden wrote:
> On 2020-11-05, at 15:11:42 +0100, Florian Westphal wrote:
> > nft currently doesn't allow to check for presence of arbitrary tcp options.
> > Only known options where nft provides a template can be tested for.
> >
> > This allows to test for presence of raw protocol values as well.
> >
> > Example:
> >
> > tcp option 42 exists
> >
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  include/expression.h          |  3 +-
> >  src/exthdr.c                  | 12 ++++++++
> >  src/ipopt.c                   |  1 +
> >  src/netlink_linearize.c       |  2 +-
> >  src/parser_bison.y            |  7 +++++
> >  src/tcpopt.c                  | 42 +++++++++++++++++++++++----
> >  tests/py/any/tcpopt.t         |  2 ++
> >  tests/py/any/tcpopt.t.payload | 53 +++--------------------------------
> >  8 files changed, 65 insertions(+), 57 deletions(-)
> >
> > diff --git a/include/expression.h b/include/expression.h
> > index b039882cf1d1..894a68d2e822 100644
> > --- a/include/expression.h
> > +++ b/include/expression.h
> > @@ -311,7 +311,8 @@ struct expr {
> >  			/* EXPR_EXTHDR */
> >  			const struct exthdr_desc	*desc;
> >  			const struct proto_hdr_template	*tmpl;
> > -			unsigned int			offset;
> > +			uint16_t			offset;
> > +			uint8_t				raw_type;
> >  			enum nft_exthdr_op		op;
> >  			unsigned int			flags;
> >  		} exthdr;
> > diff --git a/src/exthdr.c b/src/exthdr.c
> > index e1b7f14d4194..8995ad1775a0 100644
> > --- a/src/exthdr.c
> > +++ b/src/exthdr.c
> > @@ -52,6 +52,13 @@ static void exthdr_expr_print(const struct expr *expr, struct output_ctx *octx)
> >  		 */
> >  		unsigned int offset = expr->exthdr.offset / 64;
> >
> > +		if (expr->exthdr.desc == NULL &&
> > +		    expr->exthdr.offset == 0 &&
> > +		    expr->exthdr.flags & NFT_EXTHDR_F_PRESENT) {
> > +			nft_print(octx, "tcp option %d", expr->exthdr.raw_type);
> > +			return;
> > +		}
> > +
> >  		nft_print(octx, "tcp option %s", expr->exthdr.desc->name);
> >  		if (expr->exthdr.flags & NFT_EXTHDR_F_PRESENT)
> >  			return;
> > @@ -79,6 +86,7 @@ static bool exthdr_expr_cmp(const struct expr *e1, const struct expr *e2)
> >  	return e1->exthdr.desc == e2->exthdr.desc &&
> >  	       e1->exthdr.tmpl == e2->exthdr.tmpl &&
> >  	       e1->exthdr.op == e2->exthdr.op &&
> > +	       e1->exthdr.raw_type == e2->exthdr.raw_type &&
> >  	       e1->exthdr.flags == e2->exthdr.flags;
> >  }
> >
> > @@ -89,6 +97,7 @@ static void exthdr_expr_clone(struct expr *new, const struct expr *expr)
> >  	new->exthdr.offset = expr->exthdr.offset;
> >  	new->exthdr.op = expr->exthdr.op;
> >  	new->exthdr.flags = expr->exthdr.flags;
> > +	new->exthdr.raw_type = expr->exthdr.raw_type;
> >  }
> >
> >  #define NFTNL_UDATA_EXTHDR_DESC 0
> > @@ -192,6 +201,7 @@ struct expr *exthdr_expr_alloc(const struct location *loc,
> >  	expr = expr_alloc(loc, EXPR_EXTHDR, tmpl->dtype,
> >  			  BYTEORDER_BIG_ENDIAN, tmpl->len);
> >  	expr->exthdr.desc = desc;
> > +	expr->exthdr.raw_type = desc ? desc->type : 0;
> >  	expr->exthdr.tmpl = tmpl;
> >  	expr->exthdr.offset = tmpl->offset;
> >  	return expr;
> > @@ -270,6 +280,8 @@ void exthdr_init_raw(struct expr *expr, uint8_t type,
> >  	unsigned int i;
> >
> >  	assert(expr->etype == EXPR_EXTHDR);
> > +	expr->exthdr.raw_type = type;
> > +
> >  	if (op == NFT_EXTHDR_OP_TCPOPT)
> >  		return tcpopt_init_raw(expr, type, offset, len, flags);
> >  	if (op == NFT_EXTHDR_OP_IPV4)
> > diff --git a/src/ipopt.c b/src/ipopt.c
> > index 7ecb8b9c8e32..5f9f908c0b34 100644
> > --- a/src/ipopt.c
> > +++ b/src/ipopt.c
> > @@ -103,6 +103,7 @@ struct expr *ipopt_expr_alloc(const struct location *loc, uint8_t type,
> >  	expr->exthdr.tmpl   = tmpl;
> >  	expr->exthdr.op     = NFT_EXTHDR_OP_IPV4;
> >  	expr->exthdr.offset = tmpl->offset + calc_offset(desc, tmpl, ptr);
> > +	expr->exthdr.raw_type = desc->type;
> >
> >  	return expr;
> >  }
> > diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
> > index a37e4b940973..05af8bb1b485 100644
> > --- a/src/netlink_linearize.c
> > +++ b/src/netlink_linearize.c
> > @@ -206,7 +206,7 @@ static void netlink_gen_exthdr(struct netlink_linearize_ctx *ctx,
> >  	nle = alloc_nft_expr("exthdr");
> >  	netlink_put_register(nle, NFTNL_EXPR_EXTHDR_DREG, dreg);
> >  	nftnl_expr_set_u8(nle, NFTNL_EXPR_EXTHDR_TYPE,
> > -			  expr->exthdr.desc->type);
> > +			  expr->exthdr.raw_type);
> >  	nftnl_expr_set_u32(nle, NFTNL_EXPR_EXTHDR_OFFSET, offset / BITS_PER_BYTE);
> >  	nftnl_expr_set_u32(nle, NFTNL_EXPR_EXTHDR_LEN,
> >  			   div_round_up(expr->len, BITS_PER_BYTE));
> > diff --git a/src/parser_bison.y b/src/parser_bison.y
> > index e8df98b8949a..393f66862810 100644
> > --- a/src/parser_bison.y
> > +++ b/src/parser_bison.y
> > @@ -5225,6 +5225,13 @@ tcp_hdr_option_type	:	EOL		{ $$ = TCPOPT_KIND_EOL; }
> >  			|	SACK3		{ $$ = TCPOPT_KIND_SACK3; }
> >  			|	ECHO		{ $$ = TCPOPT_KIND_ECHO; }
> >  			|	TIMESTAMP	{ $$ = TCPOPT_KIND_TIMESTAMP; }
> > +			|	NUM		{
> > +				if ($1 > 255) {
> > +					erec_queue(error(&@1, "value too large"), state->msgs);
> > +					YYERROR;
> > +				}
> > +				$$ = $1;
> > +			}
> >  			;
> >
> >  tcp_hdr_option_field	:	KIND		{ $$ = TCPOPT_COMMON_KIND; }
> > diff --git a/src/tcpopt.c b/src/tcpopt.c
> > index d1dd13b868e8..1cf97a563bc2 100644
> > --- a/src/tcpopt.c
> > +++ b/src/tcpopt.c
> > @@ -103,6 +103,19 @@ const struct exthdr_desc *tcpopt_protocols[] = {
> >  	[TCPOPT_KIND_TIMESTAMP]		= &tcpopt_timestamp,
> >  };
> >
> > +/**
> > + * tcpopt_expr_alloc - allocate tcp option extension expression
> > + *
> > + * @loc: location from parser
> > + * @kind: raw tcp option value to find in packet
> > + * @field: highlevel field to find in the option if @kind is present in packet
> > + *
> > + * Allocate a new tcp option expression.
> > + * @kind is the raw option value to find in the packet.
> > + * Exception: SACK may use extra OOB data that is mangled here.
> > + *
> > + * @field is the optional field to extract from the @type option.
> > + */
> >  struct expr *tcpopt_expr_alloc(const struct location *loc,
> >  			       unsigned int kind,
> >  			       unsigned int field)
> > @@ -138,8 +151,22 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
> >  	if (kind < array_size(tcpopt_protocols))
> >  		desc = tcpopt_protocols[kind];
> >
> > -	if (!desc)
> > -		return NULL;
> > +	if (!desc) {
> > +		if (field != TCPOPT_COMMON_KIND || kind > 255)
> > +			return NULL;
> > +
> > +		expr = expr_alloc(loc, EXPR_EXTHDR, &integer_type,
> > +				  BYTEORDER_BIG_ENDIAN, 8);
> > +
> > +		desc = tcpopt_protocols[TCPOPT_NOP];
> > +		tmpl = &desc->templates[field];
> > +		expr->exthdr.desc   = desc;
> > +		expr->exthdr.tmpl   = tmpl;
> > +		expr->exthdr.op = NFT_EXTHDR_OP_TCPOPT;
> > +		expr->exthdr.raw_type = kind;
> > +		return expr;
> > +	}
> > +
> >  	tmpl = &desc->templates[field];
> >  	if (!tmpl)
> >  		return NULL;
> > @@ -149,6 +176,7 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
> >  	expr->exthdr.desc   = desc;
> >  	expr->exthdr.tmpl   = tmpl;
> >  	expr->exthdr.op     = NFT_EXTHDR_OP_TCPOPT;
> > +	expr->exthdr.raw_type = desc->type;
> >  	expr->exthdr.offset = tmpl->offset;
> >
> >  	return expr;
> > @@ -165,6 +193,10 @@ void tcpopt_init_raw(struct expr *expr, uint8_t type, unsigned int off,
> >  	expr->len = len;
> >  	expr->exthdr.flags = flags;
> >  	expr->exthdr.offset = off;
> > +	expr->exthdr.op = NFT_EXTHDR_OP_TCPOPT;
> > +
> > +	if (flags & NFT_EXTHDR_F_PRESENT)
> > +		datatype_set(expr, &boolean_type);
> >
> >  	if (type >= array_size(tcpopt_protocols))
> >  		return;
> > @@ -178,12 +210,10 @@ void tcpopt_init_raw(struct expr *expr, uint8_t type, unsigned int off,
> >  		if (tmpl->offset != off || tmpl->len != len)
> >  			continue;
> >
> > -		if (flags & NFT_EXTHDR_F_PRESENT)
> > -			datatype_set(expr, &boolean_type);
> > -		else
> > +		if ((flags & NFT_EXTHDR_F_PRESENT) == 0)
> >  			datatype_set(expr, tmpl->dtype);
> > +
> >  		expr->exthdr.tmpl = tmpl;
> > -		expr->exthdr.op   = NFT_EXTHDR_OP_TCPOPT;
> >  		break;
> >  	}
> >  }
> > diff --git a/tests/py/any/tcpopt.t b/tests/py/any/tcpopt.t
> > index 1d42de8746cd..7b17014b3003 100644
> > --- a/tests/py/any/tcpopt.t
> > +++ b/tests/py/any/tcpopt.t
> > @@ -30,6 +30,7 @@ tcp option timestamp kind 1;ok
> >  tcp option timestamp length 1;ok
> >  tcp option timestamp tsval 1;ok
> >  tcp option timestamp tsecr 1;ok
> > +tcp option 255 missing;ok
> >
> >  tcp option foobar;fail
> >  tcp option foo bar;fail
> > @@ -38,6 +39,7 @@ tcp option eol left 1;fail
> >  tcp option eol left 1;fail
> >  tcp option sack window;fail
> >  tcp option sack window 1;fail
> > +tcp option 256 exists;fail
> >
> >  tcp option window exists;ok
> >  tcp option window missing;ok
> > diff --git a/tests/py/any/tcpopt.t.payload b/tests/py/any/tcpopt.t.payload
> > index 9c480c8bd06b..34f8e26c4409 100644
> > --- a/tests/py/any/tcpopt.t.payload
> > +++ b/tests/py/any/tcpopt.t.payload
> > @@ -509,20 +509,6 @@ inet
> >    [ exthdr load tcpopt 4b @ 8 + 2 => reg 1 ]
> >    [ cmp eq reg 1 0x01000000 ]
> >
> > -# tcp option timestamp tsecr 1
> > -ip
> > -  [ meta load l4proto => reg 1 ]
> > -  [ cmp eq reg 1 0x00000006 ]
> > -  [ exthdr load tcpopt 4b @ 8 + 6 => reg 1 ]
> > -  [ cmp eq reg 1 0x01000000 ]
> > -
> > -# tcp option timestamp tsecr 1
> > -ip6
> > -  [ meta load l4proto => reg 1 ]
> > -  [ cmp eq reg 1 0x00000006 ]
> > -  [ exthdr load tcpopt 4b @ 8 + 6 => reg 1 ]
> > -  [ cmp eq reg 1 0x01000000 ]
> > -
>
> You are removing unrelated duplicate payloads.  Is it worth doing that
> separately?

This seems to dedup' the payloads correctly:

  perl -nle '
    our @payload;
    our $rule;
    if (/^# (.+)/) {
      if ($rule ne $1) {
        print for @payload;
        $rule = $1
      }
      @payload = ()
    }
    push @payload, $_;
    END { print for @payload }
  ' tests/py/any/tcpopt.t.payload > tests/py/any/tcpopt.t.payload.tmp
  mv tests/py/any/tcpopt.t.payload.tmp tests/py/any/tcpopt.t.payload

One could use perl's -i switch, but the printing of the final payload
will be to stdout and not in-place.

> >  # tcp option timestamp tsecr 1
> >  inet
> >    [ meta load l4proto => reg 1 ]
> > @@ -530,19 +516,12 @@ inet
> >    [ exthdr load tcpopt 4b @ 8 + 6 => reg 1 ]
> >    [ cmp eq reg 1 0x01000000 ]
> >
> > -# tcp option window exists
> > -ip
> > -  [ meta load l4proto => reg 1 ]
> > -  [ cmp eq reg 1 0x00000006 ]
> > -  [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
> > -  [ cmp eq reg 1 0x00000001 ]
> > -
> > -# tcp option window exists
> > -ip6
> > +# tcp option 255 missing
> > +inet
> >    [ meta load l4proto => reg 1 ]
> >    [ cmp eq reg 1 0x00000006 ]
> > -  [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
> > -  [ cmp eq reg 1 0x00000001 ]
> > +  [ exthdr load tcpopt 1b @ 255 + 0 present => reg 1 ]
> > +  [ cmp eq reg 1 0x00000000 ]
> >
> >  # tcp option window exists
> >  inet
> > @@ -551,20 +530,6 @@ inet
> >    [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
> >    [ cmp eq reg 1 0x00000001 ]
> >
> > -# tcp option window missing
> > -ip
> > -  [ meta load l4proto => reg 1 ]
> > -  [ cmp eq reg 1 0x00000006 ]
> > -  [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
> > -  [ cmp eq reg 1 0x00000000 ]
> > -
> > -# tcp option window missing
> > -ip6
> > -  [ meta load l4proto => reg 1 ]
> > -  [ cmp eq reg 1 0x00000006 ]
> > -  [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
> > -  [ cmp eq reg 1 0x00000000 ]
> > -
> >  # tcp option window missing
> >  inet
> >    [ meta load l4proto => reg 1 ]
> > @@ -572,16 +537,6 @@ inet
> >    [ exthdr load tcpopt 1b @ 3 + 0 present => reg 1 ]
> >    [ cmp eq reg 1 0x00000000 ]
> >
> > -# tcp option maxseg size set 1360
> > -ip
> > -  [ immediate reg 1 0x00005005 ]
> > -  [ exthdr write tcpopt reg 1 => 2b @ 2 + 2 ]
> > -
> > -# tcp option maxseg size set 1360
> > -ip6
> > -  [ immediate reg 1 0x00005005 ]
> > -  [ exthdr write tcpopt reg 1 => 2b @ 2 + 2 ]
> > -
> >  # tcp option maxseg size set 1360
> >  inet
> >    [ immediate reg 1 0x00005005 ]
> > --
> > 2.26.2
> >
> >



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH nft 5/7] tcpopt: allow to check for presence of any tcp option
  2020-11-05 20:57     ` Jeremy Sowden
@ 2020-11-09 11:10       ` Florian Westphal
  2020-11-09 11:38         ` Jeremy Sowden
  0 siblings, 1 reply; 14+ messages in thread
From: Florian Westphal @ 2020-11-09 11:10 UTC (permalink / raw)
  To: Jeremy Sowden; +Cc: Florian Westphal, netfilter-devel

Jeremy Sowden <jeremy@azazel.net> wrote:
> This seems to dedup' the payloads correctly:
> 
>   perl -nle '
>     our @payload;
>     our $rule;
>     if (/^# (.+)/) {
>       if ($rule ne $1) {
>         print for @payload;
>         $rule = $1
>       }
>       @payload = ()
>     }
>     push @payload, $_;
>     END { print for @payload }
>   ' tests/py/any/tcpopt.t.payload > tests/py/any/tcpopt.t.payload.tmp
>   mv tests/py/any/tcpopt.t.payload.tmp tests/py/any/tcpopt.t.payload
> 
> One could use perl's -i switch, but the printing of the final payload
> will be to stdout and not in-place.

Feel free to send a patch that weeds out all duplicates.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH nft 5/7] tcpopt: allow to check for presence of any tcp option
  2020-11-09 11:10       ` Florian Westphal
@ 2020-11-09 11:38         ` Jeremy Sowden
  0 siblings, 0 replies; 14+ messages in thread
From: Jeremy Sowden @ 2020-11-09 11:38 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

On 2020-11-09, at 12:10:14 +0100, Florian Westphal wrote:
> Jeremy Sowden <jeremy@azazel.net> wrote:
> > This seems to dedup' the payloads correctly:
> >
> >   perl -nle '
> >     our @payload;
> >     our $rule;
> >     if (/^# (.+)/) {
> >       if ($rule ne $1) {
> >         print for @payload;
> >         $rule = $1
> >       }
> >       @payload = ()
> >     }
> >     push @payload, $_;
> >     END { print for @payload }
> >   ' tests/py/any/tcpopt.t.payload > tests/py/any/tcpopt.t.payload.tmp
> >   mv tests/py/any/tcpopt.t.payload.tmp tests/py/any/tcpopt.t.payload
> >
> > One could use perl's -i switch, but the printing of the final payload
> > will be to stdout and not in-place.
>
> Feel free to send a patch that weeds out all duplicates.

Yup, will do.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH nft 6/7] tcp: add raw tcp option match support
  2020-11-05 14:11 [PATCH nft 0/7] rework tcp option handling Florian Westphal
                   ` (4 preceding siblings ...)
  2020-11-05 14:11 ` [PATCH nft 5/7] tcpopt: allow to check for presence of any tcp option Florian Westphal
@ 2020-11-05 14:11 ` Florian Westphal
  2020-11-05 14:11 ` [PATCH nft 7/7] json: " Florian Westphal
  6 siblings, 0 replies; 14+ messages in thread
From: Florian Westphal @ 2020-11-05 14:11 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

tcp option @42,16,4 (@kind,offset,length).

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 doc/payload-expression.txt    |  6 ++++++
 src/exthdr.c                  | 13 +++++++++----
 src/parser_bison.y            |  5 +++++
 src/tcpopt.c                  |  2 ++
 tests/py/any/tcpopt.t         |  2 ++
 tests/py/any/tcpopt.t.payload |  7 +++++++
 6 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/doc/payload-expression.txt b/doc/payload-expression.txt
index 3cfa7791edac..ffd1b671637a 100644
--- a/doc/payload-expression.txt
+++ b/doc/payload-expression.txt
@@ -591,6 +591,12 @@ TCP Timestamps |
 kind, length, tsval, tsecr
 |============================
 
+TCP option matching also supports raw expression syntax to access arbitrary options:
+[verse]
+*tcp option*
+[verse]
+*tcp option* *@*'number'*,*'offset'*,*'length'
+
 .IP Options
 [options="header"]
 |==================
diff --git a/src/exthdr.c b/src/exthdr.c
index 8995ad1775a0..5eb66529b5d7 100644
--- a/src/exthdr.c
+++ b/src/exthdr.c
@@ -52,10 +52,15 @@ static void exthdr_expr_print(const struct expr *expr, struct output_ctx *octx)
 		 */
 		unsigned int offset = expr->exthdr.offset / 64;
 
-		if (expr->exthdr.desc == NULL &&
-		    expr->exthdr.offset == 0 &&
-		    expr->exthdr.flags & NFT_EXTHDR_F_PRESENT) {
-			nft_print(octx, "tcp option %d", expr->exthdr.raw_type);
+		if (expr->exthdr.desc == NULL) {
+			if (expr->exthdr.offset == 0 &&
+			    expr->exthdr.flags & NFT_EXTHDR_F_PRESENT) {
+				nft_print(octx, "tcp option %d", expr->exthdr.raw_type);
+				return;
+			}
+
+			nft_print(octx, "tcp option @%u,%u,%u", expr->exthdr.raw_type,
+								expr->exthdr.offset, expr->len);
 			return;
 		}
 
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 393f66862810..079d8ebe121f 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -5198,6 +5198,11 @@ tcp_hdr_expr		:	TCP	tcp_hdr_field
 				$$ = tcpopt_expr_alloc(&@$, $3, TCPOPT_COMMON_KIND);
 				$$->exthdr.flags = NFT_EXTHDR_F_PRESENT;
 			}
+			|	TCP	OPTION	AT tcp_hdr_option_type	COMMA	NUM	COMMA	NUM
+			{
+				$$ = tcpopt_expr_alloc(&@$, $4, 0);
+				tcpopt_init_raw($$, $4, $6, $8, 0);
+			}
 			;
 
 tcp_hdr_field		:	SPORT		{ $$ = TCPHDR_SPORT; }
diff --git a/src/tcpopt.c b/src/tcpopt.c
index 1cf97a563bc2..05b5ee6e3a0b 100644
--- a/src/tcpopt.c
+++ b/src/tcpopt.c
@@ -197,6 +197,8 @@ void tcpopt_init_raw(struct expr *expr, uint8_t type, unsigned int off,
 
 	if (flags & NFT_EXTHDR_F_PRESENT)
 		datatype_set(expr, &boolean_type);
+	else
+		datatype_set(expr, &integer_type);
 
 	if (type >= array_size(tcpopt_protocols))
 		return;
diff --git a/tests/py/any/tcpopt.t b/tests/py/any/tcpopt.t
index 7b17014b3003..e759ac6132d9 100644
--- a/tests/py/any/tcpopt.t
+++ b/tests/py/any/tcpopt.t
@@ -31,6 +31,7 @@ tcp option timestamp length 1;ok
 tcp option timestamp tsval 1;ok
 tcp option timestamp tsecr 1;ok
 tcp option 255 missing;ok
+tcp option @255,8,8 255;ok
 
 tcp option foobar;fail
 tcp option foo bar;fail
@@ -40,6 +41,7 @@ tcp option eol left 1;fail
 tcp option sack window;fail
 tcp option sack window 1;fail
 tcp option 256 exists;fail
+tcp option @255,8,8 256;fail
 
 tcp option window exists;ok
 tcp option window missing;ok
diff --git a/tests/py/any/tcpopt.t.payload b/tests/py/any/tcpopt.t.payload
index 34f8e26c4409..cddba613a088 100644
--- a/tests/py/any/tcpopt.t.payload
+++ b/tests/py/any/tcpopt.t.payload
@@ -523,6 +523,13 @@ inet
   [ exthdr load tcpopt 1b @ 255 + 0 present => reg 1 ]
   [ cmp eq reg 1 0x00000000 ]
 
+# tcp option @255,8,8 255
+inet
+  [ meta load l4proto => reg 1 ]
+  [ cmp eq reg 1 0x00000006 ]
+  [ exthdr load tcpopt 1b @ 255 + 1 => reg 1 ]
+  [ cmp eq reg 1 0x000000ff ]
+
 # tcp option window exists
 inet 
   [ meta load l4proto => reg 1 ]
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH nft 7/7] json: tcp: add raw tcp option match support
  2020-11-05 14:11 [PATCH nft 0/7] rework tcp option handling Florian Westphal
                   ` (5 preceding siblings ...)
  2020-11-05 14:11 ` [PATCH nft 6/7] tcp: add raw tcp option match support Florian Westphal
@ 2020-11-05 14:11 ` Florian Westphal
  6 siblings, 0 replies; 14+ messages in thread
From: Florian Westphal @ 2020-11-05 14:11 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

To similar change as in previous one, this time for the
jason (de)serialization.

Re-uses the raw payload match syntax, i.e. base,offset,length.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/json.c                 | 22 ++++++++--------
 src/parser_json.c          | 52 ++++++++++++++++++++++++++------------
 tests/py/any/tcpopt.t.json | 34 +++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 26 deletions(-)

diff --git a/src/json.c b/src/json.c
index 3c4654d6dada..ac3b1c833d86 100644
--- a/src/json.c
+++ b/src/json.c
@@ -665,30 +665,32 @@ json_t *map_expr_json(const struct expr *expr, struct output_ctx *octx)
 json_t *exthdr_expr_json(const struct expr *expr, struct output_ctx *octx)
 {
 	const char *desc = expr->exthdr.desc ?
-			   expr->exthdr.desc->name :
-			   "unknown-exthdr";
+			   expr->exthdr.desc->name : NULL;
 	const char *field = expr->exthdr.tmpl->token;
 	json_t *root;
 	bool is_exists = expr->exthdr.flags & NFT_EXTHDR_F_PRESENT;
 
 	if (expr->exthdr.op == NFT_EXTHDR_OP_TCPOPT) {
+		static const char *offstrs[] = { "", "1", "2", "3" };
 		unsigned int offset = expr->exthdr.offset / 64;
+		const char *offstr = "";
 
-		if (offset) {
-			const char *offstrs[] = { "0", "1", "2", "3" };
-			const char *offstr = "";
-
+		if (desc) {
 			if (offset < 4)
 				offstr = offstrs[offset];
 
 			root = json_pack("{s:s+}", "name", desc, offstr);
+
+			if (!is_exists)
+				json_object_set_new(root, "field", json_string(field));
 		} else {
-			root = json_pack("{s:s}", "name", desc);
+			root = json_pack("{s:i, s:i, s:i}",
+					 "base", expr->exthdr.raw_type,
+					 "offset", expr->exthdr.offset,
+					 "len", expr->len);
+			is_exists = false;
 		}
 
-		if (!is_exists)
-			json_object_set_new(root, "field", json_string(field));
-
 		return json_pack("{s:o}", "tcp option", root);
 	}
 	if (expr->exthdr.op == NFT_EXTHDR_OP_IPV4) {
diff --git a/src/parser_json.c b/src/parser_json.c
index 6e1333659f81..b1de56a4a0d2 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -502,6 +502,8 @@ static int json_parse_tcp_option_field(int type, const char *name, int *val)
 		return 1;
 
 	desc = tcpopt_protocols[type];
+	if (!desc)
+		return 1;
 
 	for (i = 0; i < array_size(desc->templates); i++) {
 		if (desc->templates[i].token &&
@@ -601,30 +603,48 @@ static struct expr *json_parse_payload_expr(struct json_ctx *ctx,
 static struct expr *json_parse_tcp_option_expr(struct json_ctx *ctx,
 					       const char *type, json_t *root)
 {
+	int fieldval, kind, offset, len;
 	const char *desc, *field;
-	int descval, fieldval;
 	struct expr *expr;
 
-	if (json_unpack_err(ctx, root, "{s:s}", "name", &desc))
-		return NULL;
-
-	if (json_parse_tcp_option_type(desc, &descval)) {
-		json_error(ctx, "Unknown tcp option name '%s'.", desc);
-		return NULL;
-	}
+	if (!json_unpack(root, "{s:i, s:i, s:i}",
+			"base", &kind, "offset", &offset, "len", &len)) {
+		uint32_t flag = 0;
 
-	if (json_unpack(root, "{s:s}", "field", &field)) {
-		expr = tcpopt_expr_alloc(int_loc, descval,
+		expr = tcpopt_expr_alloc(int_loc, kind,
 					 TCPOPT_COMMON_KIND);
-		expr->exthdr.flags = NFT_EXTHDR_F_PRESENT;
 
+		if (kind < 0 || kind > 255)
+			return NULL;
+
+		if (offset == TCPOPT_COMMON_KIND && len == 8)
+			flag = NFT_EXTHDR_F_PRESENT;
+
+		tcpopt_init_raw(expr, kind, offset, len, flag);
 		return expr;
+	} else if (!json_unpack(root, "{s:s}", "name", &desc)) {
+		if (json_parse_tcp_option_type(desc, &kind)) {
+			json_error(ctx, "Unknown tcp option name '%s'.", desc);
+			return NULL;
+		}
+
+		if (json_unpack(root, "{s:s}", "field", &field)) {
+			expr = tcpopt_expr_alloc(int_loc, kind,
+						 TCPOPT_COMMON_KIND);
+			expr->exthdr.flags = NFT_EXTHDR_F_PRESENT;
+			return expr;
+		}
+
+		if (json_parse_tcp_option_field(kind, field, &fieldval)) {
+			json_error(ctx, "Unknown tcp option field '%s'.", field);
+			return NULL;
+		}
+
+		return tcpopt_expr_alloc(int_loc, kind, fieldval);
 	}
-	if (json_parse_tcp_option_field(descval, field, &fieldval)) {
-		json_error(ctx, "Unknown tcp option field '%s'.", field);
-		return NULL;
-	}
-	return tcpopt_expr_alloc(int_loc, descval, fieldval);
+
+	json_error(ctx, "Invalid tcp option expression properties.");
+	return NULL;
 }
 
 static int json_parse_ip_option_type(const char *name, int *val)
diff --git a/tests/py/any/tcpopt.t.json b/tests/py/any/tcpopt.t.json
index b15e36ee7f4c..139e97d8f043 100644
--- a/tests/py/any/tcpopt.t.json
+++ b/tests/py/any/tcpopt.t.json
@@ -414,6 +414,40 @@
     }
 ]
 
+# tcp option 255 missing
+[
+    {
+        "match": {
+            "left": {
+                "tcp option": {
+                    "base": 255,
+                    "len": 8,
+                    "offset": 0
+                }
+            },
+            "op": "==",
+            "right": false
+        }
+    }
+]
+
+# tcp option @255,8,8 255
+[
+    {
+        "match": {
+            "left": {
+                "tcp option": {
+                    "base": 255,
+                    "len": 8,
+                    "offset": 8
+                }
+            },
+            "op": "==",
+            "right": 255
+        }
+    }
+]
+
 # tcp option window exists
 [
     {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-11-09 11:38 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-05 14:11 [PATCH nft 0/7] rework tcp option handling Florian Westphal
2020-11-05 14:11 ` [PATCH nft 1/7] parser: merge sack-perm/sack-permitted and maxseg/mss Florian Westphal
2020-11-05 15:22   ` Jeremy Sowden
2020-11-05 15:45     ` Florian Westphal
2020-11-05 14:11 ` [PATCH nft 2/7] tcpopts: clean up parser -> tcpopt.c plumbing Florian Westphal
2020-11-05 14:11 ` [PATCH nft 3/7] tcpopt: rename noop to nop Florian Westphal
2020-11-05 14:11 ` [PATCH nft 4/7] tcpopt: split tcpopt_hdr_fields into per-option enum Florian Westphal
2020-11-05 14:11 ` [PATCH nft 5/7] tcpopt: allow to check for presence of any tcp option Florian Westphal
2020-11-05 19:11   ` Jeremy Sowden
2020-11-05 20:57     ` Jeremy Sowden
2020-11-09 11:10       ` Florian Westphal
2020-11-09 11:38         ` Jeremy Sowden
2020-11-05 14:11 ` [PATCH nft 6/7] tcp: add raw tcp option match support Florian Westphal
2020-11-05 14:11 ` [PATCH nft 7/7] json: " Florian Westphal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).