From: Jeremy Sowden <jeremy@azazel.net>
To: Florian Westphal <fw@strlen.de>
Cc: netfilter-devel@vger.kernel.org
Subject: Re: [PATCH nft 1/7] parser: merge sack-perm/sack-permitted and maxseg/mss
Date: Thu, 5 Nov 2020 15:22:56 +0000 [thread overview]
Message-ID: <20201105152256.GA3399@azazel.net> (raw)
In-Reply-To: <20201105141144.31430-2-fw@strlen.de>
[-- 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 --]
next prev parent reply other threads:[~2020-11-05 15:23 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201105152256.GA3399@azazel.net \
--to=jeremy@azazel.net \
--cc=fw@strlen.de \
--cc=netfilter-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).