| Message ID | 20180929101518.843-1-ffmancera@riseup.net |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [1/2,nft,v2] src: osf: add ttl option support | expand |
Please send a v3 including tests/py. More comments below. On Sat, Sep 29, 2018 at 12:15:17PM +0200, Fernando Fernandez Mancera wrote: > Add support for ttl option in "osf" expression. Example: > > table ip foo { > chain bar { > type filter hook input priority filter; policy accept; > osf ttl nocheck name "Linux" Listing and output should match, ie. what you list should work with nft -f, see below. > diff --git a/src/parser_bison.y b/src/parser_bison.y > index 831090b..a7ec858 100644 > --- a/src/parser_bison.y > +++ b/src/parser_bison.y > @@ -739,6 +739,7 @@ int nft_lex(void *, void *, void *); > %type <val> fib_tuple fib_result fib_flag > > %type <expr> osf_expr > +%type <val> osf_ttl > %destructor { expr_free($$); } osf_expr > > %type <val> markup_format > @@ -3112,9 +3113,21 @@ fib_tuple : fib_flag DOT fib_tuple > | fib_flag > ; > > -osf_expr : OSF NAME > +osf_expr : OSF osf_ttl NAME > { > - $$ = osf_expr_alloc(&@$); > + $$ = osf_expr_alloc(&@$, $2); > + } > + ; > + > +osf_ttl : /* empty */ { $$ = 0; } > + | STRING > + { > + if (!strcmp($1, "ttl-global")) This should be "global". But I would suggest you rename this to "loose". > + $$ = 1; Can we use NFT_OSF_* definitions, instead of magic number? > + else if (!strcmp($1, "ttl-nocheck")) This should be "nocheck". But I'd suggest you rename this to "skip" > + $$ = 2; Same here, avoid magic number, use definition. > + else > + $$ = 3; Same thing. > } > ; > diff --git a/src/osf.c b/src/osf.c > index 85c9573..c7dd25f 100644 > --- a/src/osf.c > +++ b/src/osf.c > @@ -5,13 +5,32 @@ > #include <osf.h> > #include <json.h> > > +static const char *osf_ttl_int_to_str(const uint8_t ttl) > +{ > + if (ttl == 1) > + return "ttl global"; > + else if (ttl == 2) > + return "ttl nocheck"; > + > + return ""; > +} > + > static void osf_expr_print(const struct expr *expr, struct output_ctx *octx) > { > - nft_print(octx, "osf name"); > + const char *ttl_str; > + > + ttl_str = osf_ttl_int_to_str(expr->osf.ttl); > + nft_print(octx, "osf %s name", ttl_str); > } > > static void osf_expr_clone(struct expr *new, const struct expr *expr) > { > + new->osf.ttl = expr->osf.ttl; > +} > + > +static bool osf_expr_cmp(const struct expr *e1, const struct expr *e2) > +{ > + return e1->osf.ttl == e2->osf.ttl; > } > > static const struct expr_ops osf_expr_ops = { > @@ -19,10 +38,11 @@ static const struct expr_ops osf_expr_ops = { > .name = "osf", > .print = osf_expr_print, > .clone = osf_expr_clone, > + .cmp = osf_expr_cmp, > .json = osf_expr_json, BTW, could you extend json to support 'ttl' too?
On 10/15/18 2:47 PM, Pablo Neira Ayuso wrote: > Please send a v3 including tests/py. More comments below. > > On Sat, Sep 29, 2018 at 12:15:17PM +0200, Fernando Fernandez Mancera wrote: >> Add support for ttl option in "osf" expression. Example: >> >> table ip foo { >> chain bar { >> type filter hook input priority filter; policy accept; >> osf ttl nocheck name "Linux" > > Listing and output should match, ie. what you list should work with > nft -f, see below. > >> diff --git a/src/parser_bison.y b/src/parser_bison.y >> index 831090b..a7ec858 100644 >> --- a/src/parser_bison.y >> +++ b/src/parser_bison.y >> @@ -739,6 +739,7 @@ int nft_lex(void *, void *, void *); >> %type <val> fib_tuple fib_result fib_flag >> >> %type <expr> osf_expr >> +%type <val> osf_ttl >> %destructor { expr_free($$); } osf_expr >> >> %type <val> markup_format >> @@ -3112,9 +3113,21 @@ fib_tuple : fib_flag DOT fib_tuple >> | fib_flag >> ; >> >> -osf_expr : OSF NAME >> +osf_expr : OSF osf_ttl NAME >> { >> - $$ = osf_expr_alloc(&@$); >> + $$ = osf_expr_alloc(&@$, $2); >> + } >> + ; >> + >> +osf_ttl : /* empty */ { $$ = 0; } >> + | STRING >> + { >> + if (!strcmp($1, "ttl-global")) > > This should be "global". But I would suggest you rename this to "loose". > >> + $$ = 1; > > Can we use NFT_OSF_* definitions, instead of magic number? > >> + else if (!strcmp($1, "ttl-nocheck")) > > This should be "nocheck". But I'd suggest you rename this to "skip" > >> + $$ = 2; > > Same here, avoid magic number, use definition. > >> + else >> + $$ = 3; > > Same thing. > >> } >> ; >> diff --git a/src/osf.c b/src/osf.c >> index 85c9573..c7dd25f 100644 >> --- a/src/osf.c >> +++ b/src/osf.c >> @@ -5,13 +5,32 @@ >> #include <osf.h> >> #include <json.h> >> >> +static const char *osf_ttl_int_to_str(const uint8_t ttl) >> +{ >> + if (ttl == 1) >> + return "ttl global"; >> + else if (ttl == 2) >> + return "ttl nocheck"; >> + >> + return ""; >> +} >> + >> static void osf_expr_print(const struct expr *expr, struct output_ctx *octx) >> { >> - nft_print(octx, "osf name"); >> + const char *ttl_str; >> + >> + ttl_str = osf_ttl_int_to_str(expr->osf.ttl); >> + nft_print(octx, "osf %s name", ttl_str); >> } >> >> static void osf_expr_clone(struct expr *new, const struct expr *expr) >> { >> + new->osf.ttl = expr->osf.ttl; >> +} >> + >> +static bool osf_expr_cmp(const struct expr *e1, const struct expr *e2) >> +{ >> + return e1->osf.ttl == e2->osf.ttl; >> } >> >> static const struct expr_ops osf_expr_ops = { >> @@ -19,10 +38,11 @@ static const struct expr_ops osf_expr_ops = { >> .name = "osf", >> .print = osf_expr_print, >> .clone = osf_expr_clone, >> + .cmp = osf_expr_cmp, >> .json = osf_expr_json, > > BTW, could you extend json to support 'ttl' too? > Hi Pablo, thanks for this review. All changes seem fine to me :-)
Comments below. On 10/15/18 2:47 PM, Pablo Neira Ayuso wrote: > Please send a v3 including tests/py. More comments below. > > On Sat, Sep 29, 2018 at 12:15:17PM +0200, Fernando Fernandez Mancera wrote: >> Add support for ttl option in "osf" expression. Example: >> >> table ip foo { >> chain bar { >> type filter hook input priority filter; policy accept; >> osf ttl nocheck name "Linux" > > Listing and output should match, ie. what you list should work with > nft -f, see below. > >> diff --git a/src/parser_bison.y b/src/parser_bison.y >> index 831090b..a7ec858 100644 >> --- a/src/parser_bison.y >> +++ b/src/parser_bison.y >> @@ -739,6 +739,7 @@ int nft_lex(void *, void *, void *); >> %type <val> fib_tuple fib_result fib_flag >> >> %type <expr> osf_expr >> +%type <val> osf_ttl >> %destructor { expr_free($$); } osf_expr >> >> %type <val> markup_format >> @@ -3112,9 +3113,21 @@ fib_tuple : fib_flag DOT fib_tuple >> | fib_flag >> ; >> >> -osf_expr : OSF NAME >> +osf_expr : OSF osf_ttl NAME >> { >> - $$ = osf_expr_alloc(&@$); >> + $$ = osf_expr_alloc(&@$, $2); >> + } >> + ; >> + >> +osf_ttl : /* empty */ { $$ = 0; } >> + | STRING >> + { >> + if (!strcmp($1, "ttl-global")) > > This should be "global". But I would suggest you rename this to "loose". > >> + $$ = 1; > > Can we use NFT_OSF_* definitions, instead of magic number? > >> + else if (!strcmp($1, "ttl-nocheck")) > > This should be "nocheck". But I'd suggest you rename this to "skip" > >> + $$ = 2; > > Same here, avoid magic number, use definition. > >> + else >> + $$ = 3; > > Same thing. > I am going to add the necessary NFT_OSF_* definitions in the nf_tables.h header. The options format is "ttl-*" because "global" or "loose" could be confusing. Do you prefer the option without "ttl-"? Thanks.
On Mon, Oct 22, 2018 at 05:35:42PM +0200, Fernando Fernandez Mancera wrote: > I am going to add the necessary NFT_OSF_* definitions in the nf_tables.h Just add a copy of nf_osf.h to nftables tree. We cannot mangle nf_tables.h, it's a copy from the original header to ensure sources compile with any kernel version. > header. The options format is "ttl-*" because "global" or "loose" could be > confusing. Do you prefer the option without "ttl-"? Yes, the ttl- is not needed. Make sure what you list via 'nft list ruleset > file' can be loaded via 'nft -f file'
El 22 de octubre de 2018 20:38:13 CEST, Pablo Neira Ayuso <pablo@netfilter.org> escribió: >On Mon, Oct 22, 2018 at 05:35:42PM +0200, Fernando Fernandez Mancera >wrote: >> I am going to add the necessary NFT_OSF_* definitions in the >nf_tables.h > >Just add a copy of nf_osf.h to nftables tree. We cannot mangle >nf_tables.h, it's a copy from the original header to ensure sources >compile with any kernel version. > >> header. The options format is "ttl-*" because "global" or "loose" >could be >> confusing. Do you prefer the option without "ttl-"? > >Yes, the ttl- is not needed. > >Make sure what you list via 'nft list ruleset > file' can be loaded >via 'nft -f file' The patch is already done but tests/py is giving me errors when using ct mark and maps with osf. Should I send it? I am trying to solve this anyway. Thanks :-)
On Mon, Oct 22, 2018 at 09:38:31PM +0200, Fernando Fernandez Mancera wrote: > El 22 de octubre de 2018 20:38:13 CEST, Pablo Neira Ayuso <pablo@netfilter.org> escribió: > >On Mon, Oct 22, 2018 at 05:35:42PM +0200, Fernando Fernandez Mancera > >wrote: > >> I am going to add the necessary NFT_OSF_* definitions in the > >nf_tables.h > > > >Just add a copy of nf_osf.h to nftables tree. We cannot mangle > >nf_tables.h, it's a copy from the original header to ensure sources > >compile with any kernel version. > > > >> header. The options format is "ttl-*" because "global" or "loose" > >could be > >> confusing. Do you prefer the option without "ttl-"? > > > >Yes, the ttl- is not needed. > > > >Make sure what you list via 'nft list ruleset > file' can be loaded > >via 'nft -f file' > > The patch is already done but tests/py is giving me errors when > using ct mark and maps with osf. Should I send it? You can send a preview / RFC.
diff --git a/include/expression.h b/include/expression.h index fb52abf..a794128 100644 --- a/include/expression.h +++ b/include/expression.h @@ -345,6 +345,10 @@ struct expr { uint8_t direction; uint8_t spnum; } xfrm; + struct { + /* EXPR_OSF */ + uint8_t ttl; + } osf; }; }; diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h index 169c2ab..1003942 100644 --- a/include/linux/netfilter/nf_tables.h +++ b/include/linux/netfilter/nf_tables.h @@ -939,10 +939,12 @@ enum nft_socket_keys { * enum nft_osf_attributes - nf_tables osf expression netlink attributes * * @NFTA_OSF_DREG: destination register (NLA_U32: nft_registers) + * @NFTA_OSF_TTL: Value of the TTL osf option (NLA_U8) */ enum nft_osf_attributes { NFTA_OSF_UNSPEC, NFTA_OSF_DREG, + NFTA_OSF_TTL, __NFTA_OSF_MAX }; #define NFT_OSF_MAX (__NFTA_OSF_MAX - 1) diff --git a/include/osf.h b/include/osf.h index 54cdd4a..23ea34d 100644 --- a/include/osf.h +++ b/include/osf.h @@ -1,7 +1,7 @@ #ifndef NFTABLES_OSF_H #define NFTABLES_OSF_H -struct expr *osf_expr_alloc(const struct location *loc); +struct expr *osf_expr_alloc(const struct location *loc, const uint8_t ttl); extern int nfnl_osf_load_fingerprints(struct netlink_ctx *ctx, int del); diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c index 0a6ebe0..d474893 100644 --- a/src/netlink_delinearize.c +++ b/src/netlink_delinearize.c @@ -655,8 +655,11 @@ static void netlink_parse_osf(struct netlink_parse_ctx *ctx, { enum nft_registers dreg; struct expr *expr; + uint8_t ttl; + + ttl = nftnl_expr_get_u8(nle, NFTNL_EXPR_OSF_TTL); + expr = osf_expr_alloc(loc, ttl); - expr = osf_expr_alloc(loc); dreg = netlink_parse_register(nle, NFTNL_EXPR_OSF_DREG); netlink_set_register(ctx, dreg, expr); } diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c index 0ac51bd..0c8f5fe 100644 --- a/src/netlink_linearize.c +++ b/src/netlink_linearize.c @@ -227,6 +227,7 @@ static void netlink_gen_osf(struct netlink_linearize_ctx *ctx, nle = alloc_nft_expr("osf"); netlink_put_register(nle, NFTNL_EXPR_OSF_DREG, dreg); + nftnl_expr_set_u8(nle, NFTNL_EXPR_OSF_TTL, expr->osf.ttl); nftnl_rule_add_expr(ctx->nlr, nle); } diff --git a/src/osf.c b/src/osf.c index 85c9573..c7dd25f 100644 --- a/src/osf.c +++ b/src/osf.c @@ -5,13 +5,32 @@ #include <osf.h> #include <json.h> +static const char *osf_ttl_int_to_str(const uint8_t ttl) +{ + if (ttl == 1) + return "ttl global"; + else if (ttl == 2) + return "ttl nocheck"; + + return ""; +} + static void osf_expr_print(const struct expr *expr, struct output_ctx *octx) { - nft_print(octx, "osf name"); + const char *ttl_str; + + ttl_str = osf_ttl_int_to_str(expr->osf.ttl); + nft_print(octx, "osf %s name", ttl_str); } static void osf_expr_clone(struct expr *new, const struct expr *expr) { + new->osf.ttl = expr->osf.ttl; +} + +static bool osf_expr_cmp(const struct expr *e1, const struct expr *e2) +{ + return e1->osf.ttl == e2->osf.ttl; } static const struct expr_ops osf_expr_ops = { @@ -19,10 +38,11 @@ static const struct expr_ops osf_expr_ops = { .name = "osf", .print = osf_expr_print, .clone = osf_expr_clone, + .cmp = osf_expr_cmp, .json = osf_expr_json, }; -struct expr *osf_expr_alloc(const struct location *loc) +struct expr *osf_expr_alloc(const struct location *loc, const uint8_t ttl) { unsigned int len = NFT_OSF_MAXGENRELEN * BITS_PER_BYTE; const struct datatype *type = &string_type; @@ -31,5 +51,7 @@ struct expr *osf_expr_alloc(const struct location *loc) expr = expr_alloc(loc, &osf_expr_ops, type, BYTEORDER_HOST_ENDIAN, len); + expr->osf.ttl = ttl; + return expr; } diff --git a/src/parser_bison.y b/src/parser_bison.y index 831090b..a7ec858 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -739,6 +739,7 @@ int nft_lex(void *, void *, void *); %type <val> fib_tuple fib_result fib_flag %type <expr> osf_expr +%type <val> osf_ttl %destructor { expr_free($$); } osf_expr %type <val> markup_format @@ -3112,9 +3113,21 @@ fib_tuple : fib_flag DOT fib_tuple | fib_flag ; -osf_expr : OSF NAME +osf_expr : OSF osf_ttl NAME { - $$ = osf_expr_alloc(&@$); + $$ = osf_expr_alloc(&@$, $2); + } + ; + +osf_ttl : /* empty */ { $$ = 0; } + | STRING + { + if (!strcmp($1, "ttl-global")) + $$ = 1; + else if (!strcmp($1, "ttl-nocheck")) + $$ = 2; + else + $$ = 3; } ;
Add support for ttl option in "osf" expression. Example: table ip foo { chain bar { type filter hook input priority filter; policy accept; osf ttl nocheck name "Linux" } } Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net> --- v1: initial patch v2: use "ttl-global" or "ttl-nocheck" instead of "1" or "2" --- include/expression.h | 4 ++++ include/linux/netfilter/nf_tables.h | 2 ++ include/osf.h | 2 +- src/netlink_delinearize.c | 5 ++++- src/netlink_linearize.c | 1 + src/osf.c | 26 ++++++++++++++++++++++++-- src/parser_bison.y | 17 +++++++++++++++-- 7 files changed, 51 insertions(+), 6 deletions(-)