| Message ID | 20180709144452.7338-1-ecklm94@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [v4,nft] Set/print standard chain prios with textual names | expand |
Hi, On Mon, Jul 09, 2018 at 04:44:53PM +0200, Máté Eckl wrote: [...] > Example: > nft> add table ip x > nft> add chain ip x y { type filter hook prerouting priority raw; } > nft> add chain ip x z { type filter hook prerouting priority mangle + 1; } Nice stuff. > nft> add chain ip x w { type filter hook prerouting priority dstnat - 5; } I'd suggest prenat instead of dstnat, probably? I understand this is leaking the definition we have in the kernel, but this is expected to be used by non-programmers, so I wonder if we can offer a better tag for this. > nft> add chain ip x r { type filter hook prerouting priority filter + 10; } > nft> add chain ip x t { type filter hook prerouting priority security; } > nft> add chain ip x q { type filter hook prerouting priority srcnat + 11; } I'd suggest postnat instead of srcnat. BTW, srcnat only makes sense from postrouting, I think it would it be possible to reject things that make no sense from there, like srcnat with prerouting as in the example above. More comments below. > nft> add chain ip x h { type filter hook prerouting priority 15; } > nft> > nft> add flowtable ip x y { hook ingress priority filter + 5 ; devices = {enp0s31f6}; } > nft> > nft> add table arp x > nft> add chain arp x y { type filter hook input priority filter + 5; } > nft> > nft> list ruleset > table ip x { > flowtable y { > hook ingress priority filter + 5 > devices = { enp0s31f6 } > } > > chain y { > type filter hook prerouting priority raw; policy accept; > } > > chain z { > type filter hook prerouting priority mangle + 1; policy accept; > } > > chain w { > type filter hook prerouting priority dstnat - 5; policy accept; > } > > chain r { > type filter hook prerouting priority filter + 10; policy accept; > } > > chain t { > type filter hook prerouting priority security; policy accept; > } > > chain q { > type filter hook prerouting priority 111; policy accept; > } > > chain h { > type filter hook prerouting priority 15; policy accept; > } > } > table arp x { > chain y { > type filter hook input priority filter + 5; policy accept; > } > } > nft> > nft> add chain ip x h { type filter hook prerouting priority first; } > Error: 'first' is invalid for priority in this context. > add chain ip x h { type filter hook prerouting priority first; } > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > nft> add chain arp x y { type filter hook input priority raw; } > Error: 'raw' is invalid for priority in this context. > add chain arp x y { type filter hook input priority raw; } > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > nft> add flowtable ip x y { hook ingress priority magle; devices = {enp0s31f6}; } > Error: 'magle' is invalid for priority. > add flowtable ip x y { hook ingress priority magle; devices = {enp0s31f6}; } > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Signed-off-by: Máté Eckl <ecklm94@gmail.com> > --- > v4: > - fix snat and dnat conflict with existing tokens > - remove static char array from chain_prio2str > - make numerical priority printing available via -nnn nft flag > - add docs about priority names > - check compatibility of standard prio names and table family > - handle flowtables > > doc/nft.xml | 68 ++++++++++++++++++++++-- > include/rule.h | 5 ++ > src/evaluate.c | 56 ++++++++++++++++++++ > src/parser_bison.y | 36 ++++++++++--- > src/rule.c | 129 ++++++++++++++++++++++++++++++++++++++++++--- > src/scanner.l | 2 + > 6 files changed, 281 insertions(+), 15 deletions(-) > > diff --git a/doc/nft.xml b/doc/nft.xml > index dc93a8c..01cc1d1 100644 > --- a/doc/nft.xml > +++ b/doc/nft.xml > @@ -109,7 +109,7 @@ vi:ts=4 sw=4 > Show data numerically. When used once (the default behaviour), skip > lookup of addresses to symbolic names. Use twice to also show Internet > services (port numbers) numerically. Use three times to also show > - protocols and UIDs/GIDs numerically. > + protocols, UIDs/GIDs and priorities numerically. > </para> > </listitem> > </varlistentry> > @@ -856,10 +856,68 @@ add table inet mytable > </itemizedlist> > </para> > <para> > - The <literal>priority</literal> parameter accepts a signed integer value which specifies the order in which chains with same <literal>hook</literal> value are traversed. The ordering is ascending, i.e. lower priority values have precedence over higher ones. > + The <literal>priority</literal> parameter accepts a signed integer value > + or a standard priority name which specifies the order in which chains > + with same <literal>hook</literal> value are traversed. The ordering is > + ascending, i.e. lower priority values have precedence over higher ones. > + <table frame="all"> > + <title>Standard priority names, values and families they are supported in</title> > + <tgroup cols="3" align="left" colsep="1" rowsep="1"> > + <colspec colname="c1"/> > + <colspec colname="c2" align="right"/> > + <colspec colname="c3"/> > + <thead> > + <row> > + <entry>Name</entry> > + <entry>Value</entry> > + <entry>Families</entry> > + </row> > + </thead> > + <tbody> > + <row> > + <entry>raw</entry> > + <entry>-300</entry> > + <entry>ip, ip6, inet</entry> > + </row> > + <row> > + <entry>mangle</entry> > + <entry>-150</entry> > + <entry>ip, ip6, inet</entry> > + </row> > + <row> > + <entry>dstnat</entry> > + <entry>-100</entry> > + <entry>ip, ip6, inet, bridge</entry> > + </row> > + <row> > + <entry>filter</entry> > + <entry>0</entry> > + <entry>ip, ip6, inet, arp, bridge, netdev</entry> > + </row> > + <row> > + <entry>security</entry> > + <entry>50</entry> > + <entry>ip, ip6, inet</entry> > + </row> > + <row> > + <entry>srcnat</entry> > + <entry>100</entry> > + <entry>ip, ip6, inet, bridge</entry> > + </row> > + </tbody> > + </tgroup> > + </table> > + Basic arithmetic expressions (addition and substraction) can also > + be achieved with these standard names to ease relative prioritizing, > + eg. <literal>mangle - 5</literal> stands for <literal>-155</literal>. > + Values will also be printid like this untill the value is not further > + than 10 form the standard value. > </para> > <para> > - Base chains also allow to set the chain's <literal>policy</literal>, i.e. what happens to packets not explicitly accepted or refused in contained rules. Supported policy values are <literal>accept</literal> (which is the default) or <literal>drop</literal>. > + Base chains also allow to set the chain's <literal>policy</literal>, i.e. > + what happens to packets not explicitly accepted or refused in contained > + rules. Supported policy values are <literal>accept</literal> (which is > + the default) or <literal>drop</literal>. > </para> > </refsect1> > > @@ -1379,6 +1437,10 @@ table inet filter { > hybrid IPv4/IPv6 tables. > > When no address family is specified, <literal>ip</literal> is used by default. > + > + The <literal>priority</literal> can be a signed integer or <literal>filter</literal> > + which stands for 0. Addition and substraction can be used to set relative priority > + eg. filter + 5 equals to 5. > </para> > > <variablelist> > diff --git a/include/rule.h b/include/rule.h > index 909ff36..cee9832 100644 > --- a/include/rule.h > +++ b/include/rule.h > @@ -171,6 +171,7 @@ enum chain_flags { > * @flags: chain flags > * @hookstr: unified and human readable hook name (base chains) > * @hooknum: hook number (base chains) > + * @priostr: hook priority for standard priority name use (base chains) > * @priority: hook priority (base chains) > * @policy: default chain policy (base chains) > * @type: chain type > @@ -185,6 +186,7 @@ struct chain { > uint32_t flags; > const char *hookstr; > unsigned int hooknum; > + const char *priostr; > int priority; > int policy; > const char *type; > @@ -193,6 +195,8 @@ struct chain { > struct list_head rules; > }; > > +#define STD_PRIO_BUFSIZE 100 > +extern int std_prio_lookup(const char *std_prio_name, int family); > extern const char *chain_type_name_lookup(const char *name); > extern const char *chain_hookname_lookup(const char *name); > extern struct chain *chain_alloc(const char *name); > @@ -357,6 +361,7 @@ struct flowtable { > struct location location; > const char * hookstr; > unsigned int hooknum; > + const char *priostr; > int priority; > const char **dev_array; > struct expr *dev_expr; > diff --git a/src/evaluate.c b/src/evaluate.c > index c4ee3cc..c6de5e7 100644 > --- a/src/evaluate.c > +++ b/src/evaluate.c > @@ -17,6 +17,7 @@ > #include <linux/netfilter.h> > #include <linux/netfilter_arp.h> > #include <linux/netfilter/nf_tables.h> > +#include <linux/netfilter_ipv4.h> > #include <netinet/ip_icmp.h> > #include <netinet/icmp6.h> > #include <net/ethernet.h> > @@ -2868,6 +2869,46 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set) > return 0; > } > > +static bool parse_evaluate_priority(const char *priostr, int *prioptr, > + int family) { > + char *endptr = NULL, *numstart = NULL; > + int priority; > + > + priority = (int)strtol(priostr, &endptr, 10); > + /* prio_spec */ > + if (priostr != endptr) { > + *prioptr = priority; > + return true; > + } > + > + /* STRING */ > + priority = std_prio_lookup(priostr, family); > + if (priority != NF_IP_PRI_LAST) { > + *prioptr = priority; > + return true; > + } > + > + /* STRUNG PLUS NUM | STRING DASH NUM */ > + numstart = strchr(priostr, '+'); > + if (!numstart) > + numstart = strchr(priostr, '-'); > + if (numstart) { > + char prioname[STD_PRIO_BUFSIZE]; > + > + snprintf(prioname, numstart - priostr + 1, "%s", priostr); > + priority = std_prio_lookup(prioname, family); > + if (priority != NF_IP_PRI_LAST) { > + priority += (int)strtol(numstart, &endptr, 10); > + if (endptr == priostr + strlen(priostr)) { > + *prioptr = priority; > + return true; > + } > + } > + } > + > + return false; > +} > + > static uint32_t str2hooknum(uint32_t family, const char *hook); > > static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft) > @@ -2884,6 +2925,13 @@ static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft) > if (ft->hooknum == NF_INET_NUMHOOKS) > return chain_error(ctx, ft, "invalid hook %s", ft->hookstr); > > + if (!ft->priostr) > + return chain_error(ctx, ft, > + "flowtable priority must be specified."); > + if (!parse_evaluate_priority(ft->priostr, &ft->priority, NFPROTO_NETDEV)) > + return chain_error(ctx, ft, "'%s' is invalid priority.", > + ft->priostr); > + > if (!ft->dev_expr) > return chain_error(ctx, ft, "Unbound flowtable not allowed (must specify devices)"); > > @@ -3038,6 +3086,14 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain) > chain->hookstr); > } > > + if (!chain->priostr) > + return chain_error(ctx, chain, "chain priority must be specified."); > + if (!parse_evaluate_priority(chain->priostr, &chain->priority, > + chain->handle.family)) > + return chain_error(ctx, chain, > + "'%s' is invalid priority in this context.", > + chain->priostr); > + > list_for_each_entry(rule, &chain->rules, list) { > handle_merge(&rule->handle, &chain->handle); > if (rule_evaluate(ctx, rule) < 0) > diff --git a/src/parser_bison.y b/src/parser_bison.y > index 98bfeba..2b7d7cc 100644 > --- a/src/parser_bison.y > +++ b/src/parser_bison.y > @@ -182,6 +182,8 @@ int nft_lex(void *, void *, void *); > %token AT "@" > %token VMAP "vmap" > > +%token PLUS "+" > + > %token INCLUDE "include" > %token DEFINE "define" > %token REDEFINE "redefine" > @@ -522,6 +524,7 @@ int nft_lex(void *, void *, void *); > %type <handle> set_spec setid_spec set_identifier flowtable_identifier obj_spec objid_spec obj_identifier > %destructor { handle_free(&$$); } set_spec setid_spec set_identifier obj_spec objid_spec obj_identifier > %type <val> family_spec family_spec_explicit chain_policy prio_spec > +%type <string> str_prio_spec > > %type <string> dev_spec quota_unit > %destructor { xfree($$); } dev_spec quota_unit > @@ -1633,7 +1636,7 @@ flowtable_block_alloc : /* empty */ > flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > | flowtable_block common_block > | flowtable_block stmt_separator > - | flowtable_block HOOK STRING PRIORITY prio_spec stmt_separator > + | flowtable_block HOOK STRING PRIORITY str_prio_spec stmt_separator > { > $$->hookstr = chain_hookname_lookup($3); > if ($$->hookstr == NULL) { > @@ -1644,7 +1647,7 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > } > xfree($3); > > - $$->priority = $5; > + $$->priostr = $5; > } > | flowtable_block DEVICES '=' flowtable_expr stmt_separator > { > @@ -1766,7 +1769,7 @@ type_identifier : STRING { $$ = $1; } > | CLASSID { $$ = xstrdup("classid"); } > ; > > -hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > +hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY str_prio_spec > { > const char *chain_type = chain_type_name_lookup($2); > > @@ -1789,13 +1792,34 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > xfree($4); > > $<chain>0->dev = $5; > - $<chain>0->priority = $7; > + $<chain>0->priostr = $7; > $<chain>0->flags |= CHAIN_F_BASECHAIN; > } > ; > > -prio_spec : NUM { $$ = $1; } > - | DASH NUM { $$ = -$2; } > +str_prio_spec : prio_spec > + { > + char buf[STD_PRIO_BUFSIZE]; > + snprintf(buf, STD_PRIO_BUFSIZE, "%d", (int)$1); > + $$ = xstrdup(buf); > + } > + | STRING { $$ = xstrdup($1); } > + | STRING PLUS NUM > + { > + char buf[STD_PRIO_BUFSIZE]; > + snprintf(buf, STD_PRIO_BUFSIZE, "%s+%d",$1, (int)$3); Could you store the string plus offset instead of building this string that you need to parse again from the evaluation phase? Probably you could reuse the existing priority integer field, then, if the label is non-NULL, then it means the priority integer becomes an offset. Thanks for working on this ! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 10, 2018 at 12:10:22PM +0200, Pablo Neira Ayuso wrote: > Hi, > > On Mon, Jul 09, 2018 at 04:44:53PM +0200, Máté Eckl wrote: > [...] > > Example: > > nft> add table ip x > > nft> add chain ip x y { type filter hook prerouting priority raw; } > > nft> add chain ip x z { type filter hook prerouting priority mangle + 1; } > > Nice stuff. > > > nft> add chain ip x w { type filter hook prerouting priority dstnat - 5; } > > I'd suggest prenat instead of dstnat, probably? I understand this is > leaking the definition we have in the kernel, but this is expected to > be used by non-programmers, so I wonder if we can offer a better tag > for this. Destination nat (dnat/dstnat) is a well-known expression among sysadmins and netadmins so I think this is better than prenat which just seems to be a new word for the same thing. > > nft> add chain ip x r { type filter hook prerouting priority filter + 10; } > > nft> add chain ip x t { type filter hook prerouting priority security; } > > nft> add chain ip x q { type filter hook prerouting priority srcnat + 11; } > > I'd suggest postnat instead of srcnat. Same as for dstnat. > BTW, srcnat only makes sense from postrouting, I think it would it be > possible to reject things that make no sense from there, like srcnat > with prerouting as in the example above. I'll look after this. > More comments below. > > > nft> add chain ip x h { type filter hook prerouting priority 15; } > > nft> > > nft> add flowtable ip x y { hook ingress priority filter + 5 ; devices = {enp0s31f6}; } > > nft> > > nft> add table arp x > > nft> add chain arp x y { type filter hook input priority filter + 5; } > > nft> > > nft> list ruleset > > table ip x { > > flowtable y { > > hook ingress priority filter + 5 > > devices = { enp0s31f6 } > > } > > > > chain y { > > type filter hook prerouting priority raw; policy accept; > > } > > > > chain z { > > type filter hook prerouting priority mangle + 1; policy accept; > > } > > > > chain w { > > type filter hook prerouting priority dstnat - 5; policy accept; > > } > > > > chain r { > > type filter hook prerouting priority filter + 10; policy accept; > > } > > > > chain t { > > type filter hook prerouting priority security; policy accept; > > } > > > > chain q { > > type filter hook prerouting priority 111; policy accept; > > } > > > > chain h { > > type filter hook prerouting priority 15; policy accept; > > } > > } > > table arp x { > > chain y { > > type filter hook input priority filter + 5; policy accept; > > } > > } > > nft> > > nft> add chain ip x h { type filter hook prerouting priority first; } > > Error: 'first' is invalid for priority in this context. > > add chain ip x h { type filter hook prerouting priority first; } > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > nft> add chain arp x y { type filter hook input priority raw; } > > Error: 'raw' is invalid for priority in this context. > > add chain arp x y { type filter hook input priority raw; } > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > nft> add flowtable ip x y { hook ingress priority magle; devices = {enp0s31f6}; } > > Error: 'magle' is invalid for priority. > > add flowtable ip x y { hook ingress priority magle; devices = {enp0s31f6}; } > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > Signed-off-by: Máté Eckl <ecklm94@gmail.com> > > --- > > v4: > > - fix snat and dnat conflict with existing tokens > > - remove static char array from chain_prio2str > > - make numerical priority printing available via -nnn nft flag > > - add docs about priority names > > - check compatibility of standard prio names and table family > > - handle flowtables > > > > doc/nft.xml | 68 ++++++++++++++++++++++-- > > include/rule.h | 5 ++ > > src/evaluate.c | 56 ++++++++++++++++++++ > > src/parser_bison.y | 36 ++++++++++--- > > src/rule.c | 129 ++++++++++++++++++++++++++++++++++++++++++--- > > src/scanner.l | 2 + > > 6 files changed, 281 insertions(+), 15 deletions(-) > > > > diff --git a/doc/nft.xml b/doc/nft.xml > > index dc93a8c..01cc1d1 100644 > > --- a/doc/nft.xml > > +++ b/doc/nft.xml > > @@ -109,7 +109,7 @@ vi:ts=4 sw=4 > > Show data numerically. When used once (the default behaviour), skip > > lookup of addresses to symbolic names. Use twice to also show Internet > > services (port numbers) numerically. Use three times to also show > > - protocols and UIDs/GIDs numerically. > > + protocols, UIDs/GIDs and priorities numerically. > > </para> > > </listitem> > > </varlistentry> > > @@ -856,10 +856,68 @@ add table inet mytable > > </itemizedlist> > > </para> > > <para> > > - The <literal>priority</literal> parameter accepts a signed integer value which specifies the order in which chains with same <literal>hook</literal> value are traversed. The ordering is ascending, i.e. lower priority values have precedence over higher ones. > > + The <literal>priority</literal> parameter accepts a signed integer value > > + or a standard priority name which specifies the order in which chains > > + with same <literal>hook</literal> value are traversed. The ordering is > > + ascending, i.e. lower priority values have precedence over higher ones. > > + <table frame="all"> > > + <title>Standard priority names, values and families they are supported in</title> > > + <tgroup cols="3" align="left" colsep="1" rowsep="1"> > > + <colspec colname="c1"/> > > + <colspec colname="c2" align="right"/> > > + <colspec colname="c3"/> > > + <thead> > > + <row> > > + <entry>Name</entry> > > + <entry>Value</entry> > > + <entry>Families</entry> > > + </row> > > + </thead> > > + <tbody> > > + <row> > > + <entry>raw</entry> > > + <entry>-300</entry> > > + <entry>ip, ip6, inet</entry> > > + </row> > > + <row> > > + <entry>mangle</entry> > > + <entry>-150</entry> > > + <entry>ip, ip6, inet</entry> > > + </row> > > + <row> > > + <entry>dstnat</entry> > > + <entry>-100</entry> > > + <entry>ip, ip6, inet, bridge</entry> > > + </row> > > + <row> > > + <entry>filter</entry> > > + <entry>0</entry> > > + <entry>ip, ip6, inet, arp, bridge, netdev</entry> > > + </row> > > + <row> > > + <entry>security</entry> > > + <entry>50</entry> > > + <entry>ip, ip6, inet</entry> > > + </row> > > + <row> > > + <entry>srcnat</entry> > > + <entry>100</entry> > > + <entry>ip, ip6, inet, bridge</entry> > > + </row> > > + </tbody> > > + </tgroup> > > + </table> > > + Basic arithmetic expressions (addition and substraction) can also > > + be achieved with these standard names to ease relative prioritizing, > > + eg. <literal>mangle - 5</literal> stands for <literal>-155</literal>. > > + Values will also be printid like this untill the value is not further > > + than 10 form the standard value. > > </para> > > <para> > > - Base chains also allow to set the chain's <literal>policy</literal>, i.e. what happens to packets not explicitly accepted or refused in contained rules. Supported policy values are <literal>accept</literal> (which is the default) or <literal>drop</literal>. > > + Base chains also allow to set the chain's <literal>policy</literal>, i.e. > > + what happens to packets not explicitly accepted or refused in contained > > + rules. Supported policy values are <literal>accept</literal> (which is > > + the default) or <literal>drop</literal>. > > </para> > > </refsect1> > > > > @@ -1379,6 +1437,10 @@ table inet filter { > > hybrid IPv4/IPv6 tables. > > > > When no address family is specified, <literal>ip</literal> is used by default. > > + > > + The <literal>priority</literal> can be a signed integer or <literal>filter</literal> > > + which stands for 0. Addition and substraction can be used to set relative priority > > + eg. filter + 5 equals to 5. > > </para> > > > > <variablelist> > > diff --git a/include/rule.h b/include/rule.h > > index 909ff36..cee9832 100644 > > --- a/include/rule.h > > +++ b/include/rule.h > > @@ -171,6 +171,7 @@ enum chain_flags { > > * @flags: chain flags > > * @hookstr: unified and human readable hook name (base chains) > > * @hooknum: hook number (base chains) > > + * @priostr: hook priority for standard priority name use (base chains) > > * @priority: hook priority (base chains) > > * @policy: default chain policy (base chains) > > * @type: chain type > > @@ -185,6 +186,7 @@ struct chain { > > uint32_t flags; > > const char *hookstr; > > unsigned int hooknum; > > + const char *priostr; > > int priority; > > int policy; > > const char *type; > > @@ -193,6 +195,8 @@ struct chain { > > struct list_head rules; > > }; > > > > +#define STD_PRIO_BUFSIZE 100 > > +extern int std_prio_lookup(const char *std_prio_name, int family); > > extern const char *chain_type_name_lookup(const char *name); > > extern const char *chain_hookname_lookup(const char *name); > > extern struct chain *chain_alloc(const char *name); > > @@ -357,6 +361,7 @@ struct flowtable { > > struct location location; > > const char * hookstr; > > unsigned int hooknum; > > + const char *priostr; > > int priority; > > const char **dev_array; > > struct expr *dev_expr; > > diff --git a/src/evaluate.c b/src/evaluate.c > > index c4ee3cc..c6de5e7 100644 > > --- a/src/evaluate.c > > +++ b/src/evaluate.c > > @@ -17,6 +17,7 @@ > > #include <linux/netfilter.h> > > #include <linux/netfilter_arp.h> > > #include <linux/netfilter/nf_tables.h> > > +#include <linux/netfilter_ipv4.h> > > #include <netinet/ip_icmp.h> > > #include <netinet/icmp6.h> > > #include <net/ethernet.h> > > @@ -2868,6 +2869,46 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set) > > return 0; > > } > > > > +static bool parse_evaluate_priority(const char *priostr, int *prioptr, > > + int family) { > > + char *endptr = NULL, *numstart = NULL; > > + int priority; > > + > > + priority = (int)strtol(priostr, &endptr, 10); > > + /* prio_spec */ > > + if (priostr != endptr) { > > + *prioptr = priority; > > + return true; > > + } > > + > > + /* STRING */ > > + priority = std_prio_lookup(priostr, family); > > + if (priority != NF_IP_PRI_LAST) { > > + *prioptr = priority; > > + return true; > > + } > > + > > + /* STRUNG PLUS NUM | STRING DASH NUM */ > > + numstart = strchr(priostr, '+'); > > + if (!numstart) > > + numstart = strchr(priostr, '-'); > > + if (numstart) { > > + char prioname[STD_PRIO_BUFSIZE]; > > + > > + snprintf(prioname, numstart - priostr + 1, "%s", priostr); > > + priority = std_prio_lookup(prioname, family); > > + if (priority != NF_IP_PRI_LAST) { > > + priority += (int)strtol(numstart, &endptr, 10); > > + if (endptr == priostr + strlen(priostr)) { > > + *prioptr = priority; > > + return true; > > + } > > + } > > + } > > + > > + return false; > > +} > > + > > static uint32_t str2hooknum(uint32_t family, const char *hook); > > > > static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft) > > @@ -2884,6 +2925,13 @@ static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft) > > if (ft->hooknum == NF_INET_NUMHOOKS) > > return chain_error(ctx, ft, "invalid hook %s", ft->hookstr); > > > > + if (!ft->priostr) > > + return chain_error(ctx, ft, > > + "flowtable priority must be specified."); > > + if (!parse_evaluate_priority(ft->priostr, &ft->priority, NFPROTO_NETDEV)) > > + return chain_error(ctx, ft, "'%s' is invalid priority.", > > + ft->priostr); > > + > > if (!ft->dev_expr) > > return chain_error(ctx, ft, "Unbound flowtable not allowed (must specify devices)"); > > > > @@ -3038,6 +3086,14 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain) > > chain->hookstr); > > } > > > > + if (!chain->priostr) > > + return chain_error(ctx, chain, "chain priority must be specified."); > > + if (!parse_evaluate_priority(chain->priostr, &chain->priority, > > + chain->handle.family)) > > + return chain_error(ctx, chain, > > + "'%s' is invalid priority in this context.", > > + chain->priostr); > > + > > list_for_each_entry(rule, &chain->rules, list) { > > handle_merge(&rule->handle, &chain->handle); > > if (rule_evaluate(ctx, rule) < 0) > > diff --git a/src/parser_bison.y b/src/parser_bison.y > > index 98bfeba..2b7d7cc 100644 > > --- a/src/parser_bison.y > > +++ b/src/parser_bison.y > > @@ -182,6 +182,8 @@ int nft_lex(void *, void *, void *); > > %token AT "@" > > %token VMAP "vmap" > > > > +%token PLUS "+" > > + > > %token INCLUDE "include" > > %token DEFINE "define" > > %token REDEFINE "redefine" > > @@ -522,6 +524,7 @@ int nft_lex(void *, void *, void *); > > %type <handle> set_spec setid_spec set_identifier flowtable_identifier obj_spec objid_spec obj_identifier > > %destructor { handle_free(&$$); } set_spec setid_spec set_identifier obj_spec objid_spec obj_identifier > > %type <val> family_spec family_spec_explicit chain_policy prio_spec > > +%type <string> str_prio_spec > > > > %type <string> dev_spec quota_unit > > %destructor { xfree($$); } dev_spec quota_unit > > @@ -1633,7 +1636,7 @@ flowtable_block_alloc : /* empty */ > > flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > > | flowtable_block common_block > > | flowtable_block stmt_separator > > - | flowtable_block HOOK STRING PRIORITY prio_spec stmt_separator > > + | flowtable_block HOOK STRING PRIORITY str_prio_spec stmt_separator > > { > > $$->hookstr = chain_hookname_lookup($3); > > if ($$->hookstr == NULL) { > > @@ -1644,7 +1647,7 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > > } > > xfree($3); > > > > - $$->priority = $5; > > + $$->priostr = $5; > > } > > | flowtable_block DEVICES '=' flowtable_expr stmt_separator > > { > > @@ -1766,7 +1769,7 @@ type_identifier : STRING { $$ = $1; } > > | CLASSID { $$ = xstrdup("classid"); } > > ; > > > > -hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > > +hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY str_prio_spec > > { > > const char *chain_type = chain_type_name_lookup($2); > > > > @@ -1789,13 +1792,34 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > > xfree($4); > > > > $<chain>0->dev = $5; > > - $<chain>0->priority = $7; > > + $<chain>0->priostr = $7; > > $<chain>0->flags |= CHAIN_F_BASECHAIN; > > } > > ; > > > > -prio_spec : NUM { $$ = $1; } > > - | DASH NUM { $$ = -$2; } > > +str_prio_spec : prio_spec > > + { > > + char buf[STD_PRIO_BUFSIZE]; > > + snprintf(buf, STD_PRIO_BUFSIZE, "%d", (int)$1); > > + $$ = xstrdup(buf); > > + } > > + | STRING { $$ = xstrdup($1); } > > + | STRING PLUS NUM > > + { > > + char buf[STD_PRIO_BUFSIZE]; > > + snprintf(buf, STD_PRIO_BUFSIZE, "%s+%d",$1, (int)$3); > > Could you store the string plus offset instead of building this > string that you need to parse again from the evaluation phase? > > Probably you could reuse the existing priority integer field, then, if > the label is non-NULL, then it means the priority integer becomes an > offset. This seems to be a good idea, I'll be on it. > Thanks for working on this ! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 10, 2018 at 12:52:25PM +0200, Máté Eckl wrote: [...] > Destination nat (dnat/dstnat) is a well-known expression among sysadmins and > netadmins so I think this is better than prenat which just seems to be a new > word for the same thing. ack. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > BTW, srcnat only makes sense from postrouting, I think it would it be > > possible to reject things that make no sense from there, like srcnat > > with prerouting as in the example above. > > I'll look after this. What do you think about this compatibility "matrix"? static bool std_prio_family_hook_compat(int prio, int family, int hook) { switch(prio) { case NF_IP_PRI_FILTER: switch(family) { case NFPROTO_INET: case NFPROTO_IPV4: case NFPROTO_IPV6: case NFPROTO_ARP: case NFPROTO_BRIDGE: case NFPROTO_NETDEV: return true; default: return false; } case NF_IP_PRI_RAW: case NF_IP_PRI_MANGLE: case NF_IP_PRI_SECURITY: // For these I didn't find any info about which hook // they can make sense in switch(family) { case NFPROTO_INET: case NFPROTO_IPV4: case NFPROTO_IPV6: return true; default: return false; } case NF_IP_PRI_NAT_DST: switch(family) { case NFPROTO_INET: case NFPROTO_IPV4: case NFPROTO_IPV6: case NFPROTO_BRIDGE: switch(hook) { case NF_INET_PRE_ROUTING: return true; default: return false; } default: return false; } case NF_IP_PRI_NAT_SRC: switch(family) { case NFPROTO_INET: case NFPROTO_IPV4: case NFPROTO_IPV6: case NFPROTO_BRIDGE: switch(hook) { case NF_INET_POST_ROUTING: return true; default: return false; } default: return false; } default: return false; } } -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 13, 2018 at 12:22:51PM +0200, Máté Eckl wrote: > > > BTW, srcnat only makes sense from postrouting, I think it would it be > > > possible to reject things that make no sense from there, like srcnat > > > with prerouting as in the example above. > > > > I'll look after this. > > What do you think about this compatibility "matrix"? Looks fine, one comment though regarding bridge: include/linux/netfilter_bridge.h: NF_BR_PRI_NAT_DST_OTHER = 100, include/linux/netfilter_bridge.h: NF_BR_PRI_NAT_SRC = 300, include/linux/netfilter_bridge.h: NF_BR_PRI_NAT_DST_BRIDGED = -300, Unfortunately I think we'll need these too, ie. we cannot reuse NF_IP_PRI_NAT_SRC. > static bool std_prio_family_hook_compat(int prio, int family, int hook) > { > switch(prio) { > case NF_IP_PRI_FILTER: > switch(family) { > case NFPROTO_INET: > case NFPROTO_IPV4: > case NFPROTO_IPV6: > case NFPROTO_ARP: > case NFPROTO_BRIDGE: > case NFPROTO_NETDEV: > return true; > default: > return false; > } > case NF_IP_PRI_RAW: > case NF_IP_PRI_MANGLE: > case NF_IP_PRI_SECURITY: > // For these I didn't find any info about which hook > // they can make sense in > switch(family) { > case NFPROTO_INET: > case NFPROTO_IPV4: > case NFPROTO_IPV6: > return true; > default: > return false; > } > case NF_IP_PRI_NAT_DST: > switch(family) { > case NFPROTO_INET: > case NFPROTO_IPV4: > case NFPROTO_IPV6: > case NFPROTO_BRIDGE: > switch(hook) { > case NF_INET_PRE_ROUTING: > return true; > default: > return false; > } > default: > return false; > } > case NF_IP_PRI_NAT_SRC: > switch(family) { > case NFPROTO_INET: > case NFPROTO_IPV4: > case NFPROTO_IPV6: > case NFPROTO_BRIDGE: > switch(hook) { > case NF_INET_POST_ROUTING: > return true; > default: > return false; > } > default: > return false; > } > default: > return false; > } > } -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Fri, Jul 13, 2018 at 12:22:51PM +0200, Máté Eckl wrote: > > > > BTW, srcnat only makes sense from postrouting, I think it would it be > > > > possible to reject things that make no sense from there, like srcnat > > > > with prerouting as in the example above. > > > > > > I'll look after this. > > > > What do you think about this compatibility "matrix"? > > Looks fine, one comment though regarding bridge: > > include/linux/netfilter_bridge.h: NF_BR_PRI_NAT_DST_OTHER = 100, > include/linux/netfilter_bridge.h: NF_BR_PRI_NAT_SRC = 300, > include/linux/netfilter_bridge.h: NF_BR_PRI_NAT_DST_BRIDGED = -300, > > Unfortunately I think we'll need these too, ie. we cannot reuse > NF_IP_PRI_NAT_SRC. BR_NAT isn't "nat" family though, they are normal 'filter' types. I think it would be fine to just use 'filter + 300'. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 13, 2018 at 12:45:34PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Fri, Jul 13, 2018 at 12:22:51PM +0200, Máté Eckl wrote: > > > > > BTW, srcnat only makes sense from postrouting, I think it would it be > > > > > possible to reject things that make no sense from there, like srcnat > > > > > with prerouting as in the example above. > > > > > > > > I'll look after this. > > > > > > What do you think about this compatibility "matrix"? > > > > Looks fine, one comment though regarding bridge: > > > > include/linux/netfilter_bridge.h: NF_BR_PRI_NAT_DST_OTHER = 100, > > include/linux/netfilter_bridge.h: NF_BR_PRI_NAT_SRC = 300, > > include/linux/netfilter_bridge.h: NF_BR_PRI_NAT_DST_BRIDGED = -300, > > > > Unfortunately I think we'll need these too, ie. we cannot reuse > > NF_IP_PRI_NAT_SRC. > > BR_NAT isn't "nat" family though, they are normal 'filter' types. > > I think it would be fine to just use 'filter + 300'. OK, let's do that then. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 13, 2018 at 12:59:14PM +0200, Pablo Neira Ayuso wrote: > On Fri, Jul 13, 2018 at 12:45:34PM +0200, Florian Westphal wrote: > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > On Fri, Jul 13, 2018 at 12:22:51PM +0200, Máté Eckl wrote: > > > > > > BTW, srcnat only makes sense from postrouting, I think it would it be > > > > > > possible to reject things that make no sense from there, like srcnat > > > > > > with prerouting as in the example above. > > > > > > > > > > I'll look after this. > > > > > > > > What do you think about this compatibility "matrix"? > > > > > > Looks fine, one comment though regarding bridge: > > > > > > include/linux/netfilter_bridge.h: NF_BR_PRI_NAT_DST_OTHER = 100, > > > include/linux/netfilter_bridge.h: NF_BR_PRI_NAT_SRC = 300, > > > include/linux/netfilter_bridge.h: NF_BR_PRI_NAT_DST_BRIDGED = -300, Oh. These are not exposed to the nft includes. > > > Unfortunately I think we'll need these too, ie. we cannot reuse > > > NF_IP_PRI_NAT_SRC. > > > > BR_NAT isn't "nat" family though, they are normal 'filter' types. > > > > I think it would be fine to just use 'filter + 300'. > > OK, let's do that then. But that means that this solution cannot support bridge family at all. Or BRNF stands for something that can be interpreted as filter? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Máté Eckl <ecklm94@gmail.com> wrote: > But that means that this solution cannot support bridge family at all. Or BRNF > stands for something that can be interpreted as filter? Currently bridge family has no special hooks, they are all 'filter'. So it would be fine to only support numbers in my opinion. We can revisit it later if needed. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 13, 2018 at 02:38:19PM +0200, Florian Westphal wrote: > Máté Eckl <ecklm94@gmail.com> wrote: > > But that means that this solution cannot support bridge family at all. Or BRNF > > stands for something that can be interpreted as filter? > > Currently bridge family has no special hooks, they are all 'filter'. > So it would be fine to only support numbers in my opinion. > > We can revisit it later if needed. I looked it up in iptables and it uses NF_BR_PRI_FILTER_BRIDGED for filter tables so it would be inapproppriate to translate 0 to filter here. So yes, maybe we should leave bridge family alone. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 10, 2018 at 12:10:22PM +0200, Pablo Neira Ayuso wrote: > > diff --git a/src/parser_bison.y b/src/parser_bison.y > > index 98bfeba..2b7d7cc 100644 > > --- a/src/parser_bison.y > > +++ b/src/parser_bison.y > > @@ -182,6 +182,8 @@ int nft_lex(void *, void *, void *); > > %token AT "@" > > %token VMAP "vmap" > > > > +%token PLUS "+" > > + > > %token INCLUDE "include" > > %token DEFINE "define" > > %token REDEFINE "redefine" > > @@ -522,6 +524,7 @@ int nft_lex(void *, void *, void *); > > %type <handle> set_spec setid_spec set_identifier flowtable_identifier obj_spec objid_spec obj_identifier > > %destructor { handle_free(&$$); } set_spec setid_spec set_identifier obj_spec objid_spec obj_identifier > > %type <val> family_spec family_spec_explicit chain_policy prio_spec > > +%type <string> str_prio_spec > > > > %type <string> dev_spec quota_unit > > %destructor { xfree($$); } dev_spec quota_unit > > @@ -1633,7 +1636,7 @@ flowtable_block_alloc : /* empty */ > > flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > > | flowtable_block common_block > > | flowtable_block stmt_separator > > - | flowtable_block HOOK STRING PRIORITY prio_spec stmt_separator > > + | flowtable_block HOOK STRING PRIORITY str_prio_spec stmt_separator > > { > > $$->hookstr = chain_hookname_lookup($3); > > if ($$->hookstr == NULL) { > > @@ -1644,7 +1647,7 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > > } > > xfree($3); > > > > - $$->priority = $5; > > + $$->priostr = $5; > > } > > | flowtable_block DEVICES '=' flowtable_expr stmt_separator > > { > > @@ -1766,7 +1769,7 @@ type_identifier : STRING { $$ = $1; } > > | CLASSID { $$ = xstrdup("classid"); } > > ; > > > > -hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > > +hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY str_prio_spec > > { > > const char *chain_type = chain_type_name_lookup($2); > > > > @@ -1789,13 +1792,34 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > > xfree($4); > > > > $<chain>0->dev = $5; > > - $<chain>0->priority = $7; > > + $<chain>0->priostr = $7; > > $<chain>0->flags |= CHAIN_F_BASECHAIN; > > } > > ; > > > > -prio_spec : NUM { $$ = $1; } > > - | DASH NUM { $$ = -$2; } > > +str_prio_spec : prio_spec > > + { > > + char buf[STD_PRIO_BUFSIZE]; > > + snprintf(buf, STD_PRIO_BUFSIZE, "%d", (int)$1); > > + $$ = xstrdup(buf); > > + } > > + | STRING { $$ = xstrdup($1); } > > + | STRING PLUS NUM > > + { > > + char buf[STD_PRIO_BUFSIZE]; > > + snprintf(buf, STD_PRIO_BUFSIZE, "%s+%d",$1, (int)$3); > > Could you store the string plus offset instead of building this > string that you need to parse again from the evaluation phase? > > Probably you could reuse the existing priority integer field, then, if > the label is non-NULL, then it means the priority integer becomes an > offset. I thought about different possibilities to do this, and I think the diff below does this with the less possible code duplication (it's only the parser, the other components would not be duplicated). And for now it does not even work for some obscure reason. (The chain pointer is not tha same at the evaluation phase as in the parser, so the values are in the bad place...) Of course the evaluation is simpler. I personally prefer my former solution as it is more general and results in less code duplication so I would stay with that. What do you think? diff --git a/src/parser_bison.y b/src/parser_bison.y index 98bfeba..db55cc5 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -182,6 +182,8 @@ int nft_lex(void *, void *, void *); %token AT "@" %token VMAP "vmap" +%token PLUS "+" + %token INCLUDE "include" %token DEFINE "define" %token REDEFINE "redefine" @@ -1633,7 +1635,7 @@ flowtable_block_alloc : /* empty */ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } | flowtable_block common_block | flowtable_block stmt_separator - | flowtable_block HOOK STRING PRIORITY prio_spec stmt_separator + | flowtable_block HOOK STRING PRIORITY flowtable_prio_spec stmt_separator { $$->hookstr = chain_hookname_lookup($3); if ($$->hookstr == NULL) { @@ -1643,8 +1645,6 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } YYERROR; } xfree($3); - - $$->priority = $5; } | flowtable_block DEVICES '=' flowtable_expr stmt_separator { @@ -1652,6 +1652,23 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } } ; +flowtable_prio_spec: prio_spec + { + $<flowtable>0->priority = $1; + } + | STRING { $<flowtable>0->priostr = xstrdup($1); } + | STRING PLUS NUM + { + $<flowtable>0->priostr = xstrdup($1); + $<flowtable>0->priority = $3; + } + | STRING DASH NUM + { + $<flowtable>0->priostr = xstrdup($1); + $<flowtable>0->priority = -$3; + } + ; + flowtable_expr : '{' flowtable_list_expr '}' { $2->location = @$; @@ -1766,7 +1783,7 @@ type_identifier : STRING { $$ = $1; } | CLASSID { $$ = xstrdup("classid"); } ; -hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec +hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY hook_prio_spec { const char *chain_type = chain_type_name_lookup($2); @@ -1789,11 +1806,27 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec xfree($4); $<chain>0->dev = $5; - $<chain>0->priority = $7; $<chain>0->flags |= CHAIN_F_BASECHAIN; } ; +hook_prio_spec : prio_spec + { + $<chain>0->priority = $1; + } + | STRING { $<chain>0->priostr = xstrdup($1); } + | STRING PLUS NUM + { + $<chain>0->priostr = xstrdup($1); + $<chain>0->priority = $3; + } + | STRING DASH NUM + { + $<chain>0->priostr = xstrdup($1); + $<chain>0->priority = -$3; + } + ; + prio_spec : NUM { $$ = $1; } | DASH NUM { $$ = -$2; } ; -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 16, 2018 at 09:58:44AM +0200, Máté Eckl wrote: > On Tue, Jul 10, 2018 at 12:10:22PM +0200, Pablo Neira Ayuso wrote: > > > diff --git a/src/parser_bison.y b/src/parser_bison.y > > > index 98bfeba..2b7d7cc 100644 > > > --- a/src/parser_bison.y > > > +++ b/src/parser_bison.y > > > @@ -182,6 +182,8 @@ int nft_lex(void *, void *, void *); > > > %token AT "@" > > > %token VMAP "vmap" > > > > > > +%token PLUS "+" > > > + > > > %token INCLUDE "include" > > > %token DEFINE "define" > > > %token REDEFINE "redefine" > > > @@ -522,6 +524,7 @@ int nft_lex(void *, void *, void *); > > > %type <handle> set_spec setid_spec set_identifier flowtable_identifier obj_spec objid_spec obj_identifier > > > %destructor { handle_free(&$$); } set_spec setid_spec set_identifier obj_spec objid_spec obj_identifier > > > %type <val> family_spec family_spec_explicit chain_policy prio_spec > > > +%type <string> str_prio_spec > > > > > > %type <string> dev_spec quota_unit > > > %destructor { xfree($$); } dev_spec quota_unit > > > @@ -1633,7 +1636,7 @@ flowtable_block_alloc : /* empty */ > > > flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > > > | flowtable_block common_block > > > | flowtable_block stmt_separator > > > - | flowtable_block HOOK STRING PRIORITY prio_spec stmt_separator > > > + | flowtable_block HOOK STRING PRIORITY str_prio_spec stmt_separator > > > { > > > $$->hookstr = chain_hookname_lookup($3); > > > if ($$->hookstr == NULL) { > > > @@ -1644,7 +1647,7 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > > > } > > > xfree($3); > > > > > > - $$->priority = $5; > > > + $$->priostr = $5; > > > } > > > | flowtable_block DEVICES '=' flowtable_expr stmt_separator > > > { > > > @@ -1766,7 +1769,7 @@ type_identifier : STRING { $$ = $1; } > > > | CLASSID { $$ = xstrdup("classid"); } > > > ; > > > > > > -hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > > > +hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY str_prio_spec > > > { > > > const char *chain_type = chain_type_name_lookup($2); > > > > > > @@ -1789,13 +1792,34 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > > > xfree($4); > > > > > > $<chain>0->dev = $5; > > > - $<chain>0->priority = $7; > > > + $<chain>0->priostr = $7; > > > $<chain>0->flags |= CHAIN_F_BASECHAIN; > > > } > > > ; > > > > > > -prio_spec : NUM { $$ = $1; } > > > - | DASH NUM { $$ = -$2; } > > > +str_prio_spec : prio_spec > > > + { > > > + char buf[STD_PRIO_BUFSIZE]; > > > + snprintf(buf, STD_PRIO_BUFSIZE, "%d", (int)$1); > > > + $$ = xstrdup(buf); > > > + } > > > + | STRING { $$ = xstrdup($1); } > > > + | STRING PLUS NUM > > > + { > > > + char buf[STD_PRIO_BUFSIZE]; > > > + snprintf(buf, STD_PRIO_BUFSIZE, "%s+%d",$1, (int)$3); > > > > Could you store the string plus offset instead of building this > > string that you need to parse again from the evaluation phase? > > > > Probably you could reuse the existing priority integer field, then, if > > the label is non-NULL, then it means the priority integer becomes an > > offset. > > I thought about different possibilities to do this, and I think the diff below > does this with the less possible code duplication (it's only the parser, the > other components would not be duplicated). And for now it does not even work for > some obscure reason. (The chain pointer is not tha same at the evaluation phase > as in the parser, so the values are in the bad place...) > Of course the evaluation is simpler. > > I personally prefer my former solution as it is more general and results in less > code duplication so I would stay with that. > What do you think? Do you want me to go on with this Pablo? I can make this work, but I don't think this is better than what I've already done. > diff --git a/src/parser_bison.y b/src/parser_bison.y > index 98bfeba..db55cc5 100644 > --- a/src/parser_bison.y > +++ b/src/parser_bison.y > @@ -182,6 +182,8 @@ int nft_lex(void *, void *, void *); > %token AT "@" > %token VMAP "vmap" > > +%token PLUS "+" > + > %token INCLUDE "include" > %token DEFINE "define" > %token REDEFINE "redefine" > @@ -1633,7 +1635,7 @@ flowtable_block_alloc : /* empty */ > flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > | flowtable_block common_block > | flowtable_block stmt_separator > - | flowtable_block HOOK STRING PRIORITY prio_spec stmt_separator > + | flowtable_block HOOK STRING PRIORITY flowtable_prio_spec stmt_separator > { > $$->hookstr = chain_hookname_lookup($3); > if ($$->hookstr == NULL) { > @@ -1643,8 +1645,6 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > YYERROR; > } > xfree($3); > - > - $$->priority = $5; > } > | flowtable_block DEVICES '=' flowtable_expr stmt_separator > { > @@ -1652,6 +1652,23 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > } > ; > > +flowtable_prio_spec: prio_spec > + { > + $<flowtable>0->priority = $1; > + } > + | STRING { $<flowtable>0->priostr = xstrdup($1); } > + | STRING PLUS NUM > + { > + $<flowtable>0->priostr = xstrdup($1); > + $<flowtable>0->priority = $3; > + } > + | STRING DASH NUM > + { > + $<flowtable>0->priostr = xstrdup($1); > + $<flowtable>0->priority = -$3; > + } > + ; > + > flowtable_expr : '{' flowtable_list_expr '}' > { > $2->location = @$; > @@ -1766,7 +1783,7 @@ type_identifier : STRING { $$ = $1; } > | CLASSID { $$ = xstrdup("classid"); } > ; > > -hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > +hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY hook_prio_spec > { > const char *chain_type = chain_type_name_lookup($2); > > @@ -1789,11 +1806,27 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > xfree($4); > > $<chain>0->dev = $5; > - $<chain>0->priority = $7; > $<chain>0->flags |= CHAIN_F_BASECHAIN; > } > ; > > +hook_prio_spec : prio_spec > + { > + $<chain>0->priority = $1; > + } > + | STRING { $<chain>0->priostr = xstrdup($1); } > + | STRING PLUS NUM > + { > + $<chain>0->priostr = xstrdup($1); > + $<chain>0->priority = $3; > + } > + | STRING DASH NUM > + { > + $<chain>0->priostr = xstrdup($1); > + $<chain>0->priority = -$3; > + } > + ; > + > prio_spec : NUM { $$ = $1; } > | DASH NUM { $$ = -$2; } > ; -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 13, 2018 at 02:43:46PM +0200, Máté Eckl wrote: > On Fri, Jul 13, 2018 at 02:38:19PM +0200, Florian Westphal wrote: > > Máté Eckl <ecklm94@gmail.com> wrote: > > > But that means that this solution cannot support bridge family at all. Or BRNF > > > stands for something that can be interpreted as filter? > > > > Currently bridge family has no special hooks, they are all 'filter'. > > So it would be fine to only support numbers in my opinion. > > > > We can revisit it later if needed. > > I looked it up in iptables and it uses NF_BR_PRI_FILTER_BRIDGED for filter > tables so it would be inapproppriate to translate 0 to filter here. > > So yes, maybe we should leave bridge family alone. What is your opinion about this Pablo? Is it okay to omit bridge tables out of this for now? To implement this for them I think we should expose NF_BR_PRI_* values to the uapi or something like that. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 27, 2018 at 04:25:21PM +0200, Máté Eckl wrote: > On Fri, Jul 13, 2018 at 02:43:46PM +0200, Máté Eckl wrote: > > On Fri, Jul 13, 2018 at 02:38:19PM +0200, Florian Westphal wrote: > > > Máté Eckl <ecklm94@gmail.com> wrote: > > > > But that means that this solution cannot support bridge family at all. Or BRNF > > > > stands for something that can be interpreted as filter? > > > > > > Currently bridge family has no special hooks, they are all 'filter'. > > > So it would be fine to only support numbers in my opinion. > > > > > > We can revisit it later if needed. > > > > I looked it up in iptables and it uses NF_BR_PRI_FILTER_BRIDGED for filter > > tables so it would be inapproppriate to translate 0 to filter here. > > > > So yes, maybe we should leave bridge family alone. > > What is your opinion about this Pablo? Is it okay to omit bridge tables out of > this for now? I think outcome from this discussion is to display bridge priorities using numbers we use in the kernel, right? ie. do not translate to 0. > To implement this for them I think we should expose NF_BR_PRI_* values to the > uapi or something like that. Right, this would need to be exposed through uapi at some point. You can meanwhile keep an internal copy in nftables, I mean, you don't need to wait until this is patch gets into the kernel. So we speed up things a bit. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 27, 2018 at 04:21:46PM +0200, Máté Eckl wrote: > On Mon, Jul 16, 2018 at 09:58:44AM +0200, Máté Eckl wrote: > > On Tue, Jul 10, 2018 at 12:10:22PM +0200, Pablo Neira Ayuso wrote: > > > > diff --git a/src/parser_bison.y b/src/parser_bison.y > > > > index 98bfeba..2b7d7cc 100644 > > > > --- a/src/parser_bison.y > > > > +++ b/src/parser_bison.y > > > > @@ -182,6 +182,8 @@ int nft_lex(void *, void *, void *); > > > > %token AT "@" > > > > %token VMAP "vmap" > > > > > > > > +%token PLUS "+" > > > > + > > > > %token INCLUDE "include" > > > > %token DEFINE "define" > > > > %token REDEFINE "redefine" > > > > @@ -522,6 +524,7 @@ int nft_lex(void *, void *, void *); > > > > %type <handle> set_spec setid_spec set_identifier flowtable_identifier obj_spec objid_spec obj_identifier > > > > %destructor { handle_free(&$$); } set_spec setid_spec set_identifier obj_spec objid_spec obj_identifier > > > > %type <val> family_spec family_spec_explicit chain_policy prio_spec > > > > +%type <string> str_prio_spec > > > > > > > > %type <string> dev_spec quota_unit > > > > %destructor { xfree($$); } dev_spec quota_unit > > > > @@ -1633,7 +1636,7 @@ flowtable_block_alloc : /* empty */ > > > > flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > > > > | flowtable_block common_block > > > > | flowtable_block stmt_separator > > > > - | flowtable_block HOOK STRING PRIORITY prio_spec stmt_separator > > > > + | flowtable_block HOOK STRING PRIORITY str_prio_spec stmt_separator > > > > { > > > > $$->hookstr = chain_hookname_lookup($3); > > > > if ($$->hookstr == NULL) { > > > > @@ -1644,7 +1647,7 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > > > > } > > > > xfree($3); > > > > > > > > - $$->priority = $5; > > > > + $$->priostr = $5; > > > > } > > > > | flowtable_block DEVICES '=' flowtable_expr stmt_separator > > > > { > > > > @@ -1766,7 +1769,7 @@ type_identifier : STRING { $$ = $1; } > > > > | CLASSID { $$ = xstrdup("classid"); } > > > > ; > > > > > > > > -hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > > > > +hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY str_prio_spec > > > > { > > > > const char *chain_type = chain_type_name_lookup($2); > > > > > > > > @@ -1789,13 +1792,34 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > > > > xfree($4); > > > > > > > > $<chain>0->dev = $5; > > > > - $<chain>0->priority = $7; > > > > + $<chain>0->priostr = $7; > > > > $<chain>0->flags |= CHAIN_F_BASECHAIN; > > > > } > > > > ; > > > > > > > > -prio_spec : NUM { $$ = $1; } > > > > - | DASH NUM { $$ = -$2; } > > > > +str_prio_spec : prio_spec > > > > + { > > > > + char buf[STD_PRIO_BUFSIZE]; > > > > + snprintf(buf, STD_PRIO_BUFSIZE, "%d", (int)$1); > > > > + $$ = xstrdup(buf); > > > > + } > > > > + | STRING { $$ = xstrdup($1); } > > > > + | STRING PLUS NUM > > > > + { > > > > + char buf[STD_PRIO_BUFSIZE]; > > > > + snprintf(buf, STD_PRIO_BUFSIZE, "%s+%d",$1, (int)$3); > > > > > > Could you store the string plus offset instead of building this > > > string that you need to parse again from the evaluation phase? > > > > > > Probably you could reuse the existing priority integer field, then, if > > > the label is non-NULL, then it means the priority integer becomes an > > > offset. > > > > I thought about different possibilities to do this, and I think the diff below > > does this with the less possible code duplication (it's only the parser, the > > other components would not be duplicated). And for now it does not even work for > > some obscure reason. (The chain pointer is not tha same at the evaluation phase > > as in the parser, so the values are in the bad place...) > > Of course the evaluation is simpler. > > > > I personally prefer my former solution as it is more general and results in less > > code duplication so I would stay with that. > > What do you think? > > Do you want me to go on with this Pablo? I can make this work, but I don't think > this is better than what I've already done. If you're refering to the string parsing in the evaluation phase that this patch is doing, I would indeed prefer an approach that which doesn't need any tinkering with str*() functions to do late tokenization from there. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jul 28, 2018 at 12:14:57PM +0200, Pablo Neira Ayuso wrote: > On Fri, Jul 27, 2018 at 04:21:46PM +0200, Máté Eckl wrote: > > On Mon, Jul 16, 2018 at 09:58:44AM +0200, Máté Eckl wrote: > > > On Tue, Jul 10, 2018 at 12:10:22PM +0200, Pablo Neira Ayuso wrote: > > > > > diff --git a/src/parser_bison.y b/src/parser_bison.y > > > > > index 98bfeba..2b7d7cc 100644 > > > > > --- a/src/parser_bison.y > > > > > +++ b/src/parser_bison.y > > > > > @@ -182,6 +182,8 @@ int nft_lex(void *, void *, void *); > > > > > %token AT "@" > > > > > %token VMAP "vmap" > > > > > > > > > > +%token PLUS "+" > > > > > + > > > > > %token INCLUDE "include" > > > > > %token DEFINE "define" > > > > > %token REDEFINE "redefine" > > > > > @@ -522,6 +524,7 @@ int nft_lex(void *, void *, void *); > > > > > %type <handle> set_spec setid_spec set_identifier flowtable_identifier obj_spec objid_spec obj_identifier > > > > > %destructor { handle_free(&$$); } set_spec setid_spec set_identifier obj_spec objid_spec obj_identifier > > > > > %type <val> family_spec family_spec_explicit chain_policy prio_spec > > > > > +%type <string> str_prio_spec > > > > > > > > > > %type <string> dev_spec quota_unit > > > > > %destructor { xfree($$); } dev_spec quota_unit > > > > > @@ -1633,7 +1636,7 @@ flowtable_block_alloc : /* empty */ > > > > > flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > > > > > | flowtable_block common_block > > > > > | flowtable_block stmt_separator > > > > > - | flowtable_block HOOK STRING PRIORITY prio_spec stmt_separator > > > > > + | flowtable_block HOOK STRING PRIORITY str_prio_spec stmt_separator > > > > > { > > > > > $$->hookstr = chain_hookname_lookup($3); > > > > > if ($$->hookstr == NULL) { > > > > > @@ -1644,7 +1647,7 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > > > > > } > > > > > xfree($3); > > > > > > > > > > - $$->priority = $5; > > > > > + $$->priostr = $5; > > > > > } > > > > > | flowtable_block DEVICES '=' flowtable_expr stmt_separator > > > > > { > > > > > @@ -1766,7 +1769,7 @@ type_identifier : STRING { $$ = $1; } > > > > > | CLASSID { $$ = xstrdup("classid"); } > > > > > ; > > > > > > > > > > -hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > > > > > +hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY str_prio_spec > > > > > { > > > > > const char *chain_type = chain_type_name_lookup($2); > > > > > > > > > > @@ -1789,13 +1792,34 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > > > > > xfree($4); > > > > > > > > > > $<chain>0->dev = $5; > > > > > - $<chain>0->priority = $7; > > > > > + $<chain>0->priostr = $7; > > > > > $<chain>0->flags |= CHAIN_F_BASECHAIN; > > > > > } > > > > > ; > > > > > > > > > > -prio_spec : NUM { $$ = $1; } > > > > > - | DASH NUM { $$ = -$2; } > > > > > +str_prio_spec : prio_spec > > > > > + { > > > > > + char buf[STD_PRIO_BUFSIZE]; > > > > > + snprintf(buf, STD_PRIO_BUFSIZE, "%d", (int)$1); > > > > > + $$ = xstrdup(buf); > > > > > + } > > > > > + | STRING { $$ = xstrdup($1); } > > > > > + | STRING PLUS NUM > > > > > + { > > > > > + char buf[STD_PRIO_BUFSIZE]; > > > > > + snprintf(buf, STD_PRIO_BUFSIZE, "%s+%d",$1, (int)$3); > > > > > > > > Could you store the string plus offset instead of building this > > > > string that you need to parse again from the evaluation phase? > > > > > > > > Probably you could reuse the existing priority integer field, then, if > > > > the label is non-NULL, then it means the priority integer becomes an > > > > offset. > > > > > > I thought about different possibilities to do this, and I think the diff below > > > does this with the less possible code duplication (it's only the parser, the > > > other components would not be duplicated). And for now it does not even work for > > > some obscure reason. (The chain pointer is not tha same at the evaluation phase > > > as in the parser, so the values are in the bad place...) > > > Of course the evaluation is simpler. > > > > > > I personally prefer my former solution as it is more general and results in less > > > code duplication so I would stay with that. > > > What do you think? > > > > Do you want me to go on with this Pablo? I can make this work, but I don't think > > this is better than what I've already done. > > If you're refering to the string parsing in the evaluation phase that > this patch is doing, I would indeed prefer an approach that which > doesn't need any tinkering with str*() functions to do late > tokenization from there. I've been trying to achieve what you want, but I cannot find a way to do so. I am struggling with the parser. Could you help with it? I thought it is similar to what I was doing on another topic but it is not. Find the diff [2] of the parser and gdb session [1] below. It seems that after it assigns mangle to priostr and 81 to priority, when it gets to the hook_spec part 81 is the value of first_line... I have tried it with $<chain>0 $<chain>$ $<chain>-1 $<chain>1, but all of them caused some sort of memory corruption... It is certainly a misuse of the parser, but it is quite deep. [1]: gdb --args nft add chain ip x z "{ type filter hook prerouting priority mangle + 81; }" [...] (gdb) break parser_bison.c:6871 No source file named parser_bison.c. Make breakpoint pending on future shared library load? (y or [n]) y Breakpoint 1 (parser_bison.c:6871) pending. (gdb) break parser_bison.c:6830 No source file named parser_bison.c. Make breakpoint pending on future shared library load? (y or [n]) y Breakpoint 2 (parser_bison.c:6830) pending. (gdb) run Starting program: /usr/bin/nft add chain ip x z \{\ type\ filter\ hook\ prerouting\ priority\ mangle\ +\ 81\;\ \} Breakpoint 1, nft_parse (nft=0x55555575aa20, scanner=0x55555575b3a0, state=0x55555575aaf0) at parser_bison.c:6875 6875 break; (gdb) print *(yyvsp[-3].chain) $1 = {list = {next = 0x6974756f72657270, prev = 0x7ffff600676e}, handle = {family = 0, table = {location = {indesc = 0x21, {{token_offset = 111516266160493, line_offset = 140737337281280, first_line = 0, last_line = 0, first_column = 33, last_column = 0}, {nle = 0x656c676e616d}}}, name = 0x656c676e616d <error: Cannot access memory at address 0x656c676e616d>}, chain = {location = {indesc = 0x7ffff6fecb00 <main_arena+96>, {{token_offset = 0, line_offset = 225, first_line = 4143893248, last_line = 32767, first_column = 4143893248, last_column = 32767}, {nle = 0x0}}}, name = 0x0}, set = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, name = 0x0}, obj = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, name = 0x0}, flowtable = 0x0, handle = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, id = 0}, position = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 224, last_column = 0}, {nle = 0x0}}}, id = 32}, index = {location = {indesc = 0x736f6d736f63, {{token_offset = 0, line_offset = 0, first_line = 529, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, id = 0}, set_id = 0}, location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, refcnt = 0, flags = 0, hookstr = 0x0, hooknum = 0, priostr = 0x55555575b5c0 "mangle", priority = 81, policy = 0, type = 0x0, dev = 0x0, scope = {parent = 0x0, symbols = {next = 0x0, prev = 0x0}}, rules = {next = 0x0, prev = 0x0}} (gdb) continue Continuing. Breakpoint 2, nft_parse (nft=0x55555575aa20, scanner=0x55555575b3a0, state=0x55555575aaf0) at parser_bison.c:6854 6854 break; (gdb) print *(yyvsp[-7].chain) $2 = {list = {next = 0x0, prev = 0x0}, handle = {family = 0, table = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, name = 0x0}, chain = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 93824994358720, first_line = 81, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, name = 0x0}, set = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, name = 0x0}, obj = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, name = 0x0}, flowtable = 0x0, handle = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, id = 0}, position = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, id = 0}, index = {location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, id = 0}, set_id = 0}, location = {indesc = 0x0, {{token_offset = 0, line_offset = 0, first_line = 0, last_line = 0, first_column = 0, last_column = 0}, {nle = 0x0}}}, refcnt = 1, flags = 1, hookstr = 0x7ffff7b9c476 "prerouting", hooknum = 0, priostr = 0x0, priority = 0, policy = -1, type = 0x55555575b5e0 "filter", dev = 0x0, scope = {parent = 0x55555575b090, symbols = {next = 0x55555575b8c0, prev = 0x55555575b8c0}}, rules = {next = 0x55555575b8d0, prev = 0x55555575b8d0}} (gdb) [2]: diff --git a/src/parser_bison.y b/src/parser_bison.y index 98bfeba..ee76301 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -182,6 +182,8 @@ int nft_lex(void *, void *, void *); %token AT "@" %token VMAP "vmap" +%token PLUS "+" + %token INCLUDE "include" %token DEFINE "define" %token REDEFINE "redefine" @@ -1633,7 +1635,7 @@ flowtable_block_alloc : /* empty */ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } | flowtable_block common_block | flowtable_block stmt_separator - | flowtable_block HOOK STRING PRIORITY prio_spec stmt_separator + | flowtable_block HOOK STRING PRIORITY flowtable_prio_spec stmt_separator { $$->hookstr = chain_hookname_lookup($3); if ($$->hookstr == NULL) { @@ -1643,8 +1645,6 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } YYERROR; } xfree($3); - - $$->priority = $5; } | flowtable_block DEVICES '=' flowtable_expr stmt_separator { @@ -1652,6 +1652,20 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } } ; +flowtable_prio_spec: prio_spec { $<flowtable>0->priority = $1; } + | STRING { $<flowtable>0->priostr = xstrdup($1); } + | STRING PLUS NUM + { + $<flowtable>0->priostr = xstrdup($1); + $<flowtable>0->priority = $3; + } + | STRING DASH NUM + { + $<flowtable>0->priostr = xstrdup($1); + $<flowtable>0->priority = -$3; + } + ; + flowtable_expr : '{' flowtable_list_expr '}' { $2->location = @$; @@ -1766,7 +1780,7 @@ type_identifier : STRING { $$ = $1; } | CLASSID { $$ = xstrdup("classid"); } ; -hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec +hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY hook_prio_spec { const char *chain_type = chain_type_name_lookup($2); @@ -1789,11 +1803,24 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec xfree($4); $<chain>0->dev = $5; - $<chain>0->priority = $7; $<chain>0->flags |= CHAIN_F_BASECHAIN; } ; +hook_prio_spec : prio_spec { $<chain>0->priority = $1; } + | STRING { $<chain>0->priostr = xstrdup($1); } + | STRING PLUS NUM + { + $<chain>0->priostr = xstrdup($1); + $<chain>0->priority = $3; + } + | STRING DASH NUM + { + $<chain>0->priostr = xstrdup($1); + $<chain>0->priority = -$3; + } + ; + prio_spec : NUM { $$ = $1; } | DASH NUM { $$ = -$2; } ; -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 01, 2018 at 06:50:14PM +0200, Máté Eckl wrote: [...] > > > > > > > > > > Could you store the string plus offset instead of building this > > > > > string that you need to parse again from the evaluation phase? > > > > > > > > > > Probably you could reuse the existing priority integer field, then, if > > > > > the label is non-NULL, then it means the priority integer becomes an > > > > > offset. I tried another way of doing this. I think it will be good if you don't mind adding a new attribute to the parser's union. I have attached the diff for the parser, I'd like to test it a bit more before sending a new version of the patch. I also changed the chain and flowtable priority attributes to prio_spec and this way it is quite simple in the parser and in evaluate.c. diff --git a/src/parser_bison.y b/src/parser_bison.y index 98bfeba..73af3bc 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -153,6 +153,7 @@ int nft_lex(void *, void *, void *); const struct datatype *datatype; struct handle_spec handle_spec; struct position_spec position_spec; + struct prio_spec prio_spec; const struct exthdr_desc *exthdr_desc; } @@ -182,6 +183,8 @@ int nft_lex(void *, void *, void *); %token AT "@" %token VMAP "vmap" +%token PLUS "+" + %token INCLUDE "include" %token DEFINE "define" %token REDEFINE "redefine" @@ -522,6 +525,7 @@ int nft_lex(void *, void *, void *); %type <handle> set_spec setid_spec set_identifier flowtable_identifier obj_spec objid_spec obj_identifier %destructor { handle_free(&$$); } set_spec setid_spec set_identifier obj_spec objid_spec obj_identifier %type <val> family_spec family_spec_explicit chain_policy prio_spec +%type <prio_spec> extended_prio_spec %type <string> dev_spec quota_unit %destructor { xfree($$); } dev_spec quota_unit @@ -1633,7 +1637,7 @@ flowtable_block_alloc : /* empty */ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } | flowtable_block common_block | flowtable_block stmt_separator - | flowtable_block HOOK STRING PRIORITY prio_spec stmt_separator + | flowtable_block HOOK STRING PRIORITY extended_prio_spec stmt_separator { $$->hookstr = chain_hookname_lookup($3); if ($$->hookstr == NULL) { @@ -1766,7 +1770,7 @@ type_identifier : STRING { $$ = $1; } | CLASSID { $$ = xstrdup("classid"); } ; -hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec +hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY extended_prio_spec { const char *chain_type = chain_type_name_lookup($2); @@ -1788,9 +1792,37 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec } xfree($4); - $<chain>0->dev = $5; - $<chain>0->priority = $7; - $<chain>0->flags |= CHAIN_F_BASECHAIN; + $<chain>0->dev = $5; + $<chain>0->priority = $7; + $<chain>0->flags |= CHAIN_F_BASECHAIN; + } + ; + +extended_prio_spec : prio_spec + { + struct prio_spec spec = {0}; + spec.num = $1; + $$ = spec; + } + | STRING + { + struct prio_spec spec = {0}; + spec.str = xstrdup($1); + $$ = spec; + } + | STRING PLUS NUM + { + struct prio_spec spec = {0}; + spec.num = $3; + spec.str = xstrdup($1); + $$ = spec; + } + | STRING DASH NUM + { + struct prio_spec spec = {0}; + spec.num = -$3; + spec.str = xstrdup($1); + $$ = spec; } ;
On Thu, Aug 02, 2018 at 06:49:59PM +0200, Máté Eckl wrote: > On Wed, Aug 01, 2018 at 06:50:14PM +0200, Máté Eckl wrote: > [...] > > > > > > > > > > > > Could you store the string plus offset instead of building this > > > > > > string that you need to parse again from the evaluation phase? > > > > > > > > > > > > Probably you could reuse the existing priority integer field, then, if > > > > > > the label is non-NULL, then it means the priority integer becomes an > > > > > > offset. > > I tried another way of doing this. I think it will be good if you don't mind > adding a new attribute to the parser's union. I have attached the diff for the > parser, I'd like to test it a bit more before sending a new version of the > patch. > > I also changed the chain and flowtable priority attributes to prio_spec and this > way it is quite simple in the parser and in evaluate.c. Thanks, this looks fine indeed. > diff --git a/src/parser_bison.y b/src/parser_bison.y > index 98bfeba..73af3bc 100644 > --- a/src/parser_bison.y > +++ b/src/parser_bison.y > @@ -153,6 +153,7 @@ int nft_lex(void *, void *, void *); > const struct datatype *datatype; > struct handle_spec handle_spec; > struct position_spec position_spec; > + struct prio_spec prio_spec; > const struct exthdr_desc *exthdr_desc; > } > > @@ -182,6 +183,8 @@ int nft_lex(void *, void *, void *); > %token AT "@" > %token VMAP "vmap" > > +%token PLUS "+" > + > %token INCLUDE "include" > %token DEFINE "define" > %token REDEFINE "redefine" > @@ -522,6 +525,7 @@ int nft_lex(void *, void *, void *); > %type <handle> set_spec setid_spec set_identifier flowtable_identifier obj_spec objid_spec obj_identifier > %destructor { handle_free(&$$); } set_spec setid_spec set_identifier obj_spec objid_spec obj_identifier > %type <val> family_spec family_spec_explicit chain_policy prio_spec > +%type <prio_spec> extended_prio_spec > > %type <string> dev_spec quota_unit > %destructor { xfree($$); } dev_spec quota_unit > @@ -1633,7 +1637,7 @@ flowtable_block_alloc : /* empty */ > flowtable_block : /* empty */ { $$ = $<flowtable>-1; } > | flowtable_block common_block > | flowtable_block stmt_separator > - | flowtable_block HOOK STRING PRIORITY prio_spec stmt_separator > + | flowtable_block HOOK STRING PRIORITY extended_prio_spec stmt_separator > { > $$->hookstr = chain_hookname_lookup($3); > if ($$->hookstr == NULL) { > @@ -1766,7 +1770,7 @@ type_identifier : STRING { $$ = $1; } > | CLASSID { $$ = xstrdup("classid"); } > ; > > -hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > +hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY extended_prio_spec > { > const char *chain_type = chain_type_name_lookup($2); > > @@ -1788,9 +1792,37 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec > } > xfree($4); > > - $<chain>0->dev = $5; > - $<chain>0->priority = $7; > - $<chain>0->flags |= CHAIN_F_BASECHAIN; > + $<chain>0->dev = $5; > + $<chain>0->priority = $7; > + $<chain>0->flags |= CHAIN_F_BASECHAIN; > + } > + ; > + > +extended_prio_spec : prio_spec > + { > + struct prio_spec spec = {0}; > + spec.num = $1; > + $$ = spec; > + } > + | STRING > + { > + struct prio_spec spec = {0}; > + spec.str = xstrdup($1); > + $$ = spec; > + } > + | STRING PLUS NUM > + { > + struct prio_spec spec = {0}; > + spec.num = $3; > + spec.str = xstrdup($1); > + $$ = spec; > + } > + | STRING DASH NUM > + { > + struct prio_spec spec = {0}; > + spec.num = -$3; > + spec.str = xstrdup($1); > + $$ = spec; > } > ; > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/doc/nft.xml b/doc/nft.xml index dc93a8c..01cc1d1 100644 --- a/doc/nft.xml +++ b/doc/nft.xml @@ -109,7 +109,7 @@ vi:ts=4 sw=4 Show data numerically. When used once (the default behaviour), skip lookup of addresses to symbolic names. Use twice to also show Internet services (port numbers) numerically. Use three times to also show - protocols and UIDs/GIDs numerically. + protocols, UIDs/GIDs and priorities numerically. </para> </listitem> </varlistentry> @@ -856,10 +856,68 @@ add table inet mytable </itemizedlist> </para> <para> - The <literal>priority</literal> parameter accepts a signed integer value which specifies the order in which chains with same <literal>hook</literal> value are traversed. The ordering is ascending, i.e. lower priority values have precedence over higher ones. + The <literal>priority</literal> parameter accepts a signed integer value + or a standard priority name which specifies the order in which chains + with same <literal>hook</literal> value are traversed. The ordering is + ascending, i.e. lower priority values have precedence over higher ones. + <table frame="all"> + <title>Standard priority names, values and families they are supported in</title> + <tgroup cols="3" align="left" colsep="1" rowsep="1"> + <colspec colname="c1"/> + <colspec colname="c2" align="right"/> + <colspec colname="c3"/> + <thead> + <row> + <entry>Name</entry> + <entry>Value</entry> + <entry>Families</entry> + </row> + </thead> + <tbody> + <row> + <entry>raw</entry> + <entry>-300</entry> + <entry>ip, ip6, inet</entry> + </row> + <row> + <entry>mangle</entry> + <entry>-150</entry> + <entry>ip, ip6, inet</entry> + </row> + <row> + <entry>dstnat</entry> + <entry>-100</entry> + <entry>ip, ip6, inet, bridge</entry> + </row> + <row> + <entry>filter</entry> + <entry>0</entry> + <entry>ip, ip6, inet, arp, bridge, netdev</entry> + </row> + <row> + <entry>security</entry> + <entry>50</entry> + <entry>ip, ip6, inet</entry> + </row> + <row> + <entry>srcnat</entry> + <entry>100</entry> + <entry>ip, ip6, inet, bridge</entry> + </row> + </tbody> + </tgroup> + </table> + Basic arithmetic expressions (addition and substraction) can also + be achieved with these standard names to ease relative prioritizing, + eg. <literal>mangle - 5</literal> stands for <literal>-155</literal>. + Values will also be printid like this untill the value is not further + than 10 form the standard value. </para> <para> - Base chains also allow to set the chain's <literal>policy</literal>, i.e. what happens to packets not explicitly accepted or refused in contained rules. Supported policy values are <literal>accept</literal> (which is the default) or <literal>drop</literal>. + Base chains also allow to set the chain's <literal>policy</literal>, i.e. + what happens to packets not explicitly accepted or refused in contained + rules. Supported policy values are <literal>accept</literal> (which is + the default) or <literal>drop</literal>. </para> </refsect1> @@ -1379,6 +1437,10 @@ table inet filter { hybrid IPv4/IPv6 tables. When no address family is specified, <literal>ip</literal> is used by default. + + The <literal>priority</literal> can be a signed integer or <literal>filter</literal> + which stands for 0. Addition and substraction can be used to set relative priority + eg. filter + 5 equals to 5. </para> <variablelist> diff --git a/include/rule.h b/include/rule.h index 909ff36..cee9832 100644 --- a/include/rule.h +++ b/include/rule.h @@ -171,6 +171,7 @@ enum chain_flags { * @flags: chain flags * @hookstr: unified and human readable hook name (base chains) * @hooknum: hook number (base chains) + * @priostr: hook priority for standard priority name use (base chains) * @priority: hook priority (base chains) * @policy: default chain policy (base chains) * @type: chain type @@ -185,6 +186,7 @@ struct chain { uint32_t flags; const char *hookstr; unsigned int hooknum; + const char *priostr; int priority; int policy; const char *type; @@ -193,6 +195,8 @@ struct chain { struct list_head rules; }; +#define STD_PRIO_BUFSIZE 100 +extern int std_prio_lookup(const char *std_prio_name, int family); extern const char *chain_type_name_lookup(const char *name); extern const char *chain_hookname_lookup(const char *name); extern struct chain *chain_alloc(const char *name); @@ -357,6 +361,7 @@ struct flowtable { struct location location; const char * hookstr; unsigned int hooknum; + const char *priostr; int priority; const char **dev_array; struct expr *dev_expr; diff --git a/src/evaluate.c b/src/evaluate.c index c4ee3cc..c6de5e7 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -17,6 +17,7 @@ #include <linux/netfilter.h> #include <linux/netfilter_arp.h> #include <linux/netfilter/nf_tables.h> +#include <linux/netfilter_ipv4.h> #include <netinet/ip_icmp.h> #include <netinet/icmp6.h> #include <net/ethernet.h> @@ -2868,6 +2869,46 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set) return 0; } +static bool parse_evaluate_priority(const char *priostr, int *prioptr, + int family) { + char *endptr = NULL, *numstart = NULL; + int priority; + + priority = (int)strtol(priostr, &endptr, 10); + /* prio_spec */ + if (priostr != endptr) { + *prioptr = priority; + return true; + } + + /* STRING */ + priority = std_prio_lookup(priostr, family); + if (priority != NF_IP_PRI_LAST) { + *prioptr = priority; + return true; + } + + /* STRUNG PLUS NUM | STRING DASH NUM */ + numstart = strchr(priostr, '+'); + if (!numstart) + numstart = strchr(priostr, '-'); + if (numstart) { + char prioname[STD_PRIO_BUFSIZE]; + + snprintf(prioname, numstart - priostr + 1, "%s", priostr); + priority = std_prio_lookup(prioname, family); + if (priority != NF_IP_PRI_LAST) { + priority += (int)strtol(numstart, &endptr, 10); + if (endptr == priostr + strlen(priostr)) { + *prioptr = priority; + return true; + } + } + } + + return false; +} + static uint32_t str2hooknum(uint32_t family, const char *hook); static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft) @@ -2884,6 +2925,13 @@ static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft) if (ft->hooknum == NF_INET_NUMHOOKS) return chain_error(ctx, ft, "invalid hook %s", ft->hookstr); + if (!ft->priostr) + return chain_error(ctx, ft, + "flowtable priority must be specified."); + if (!parse_evaluate_priority(ft->priostr, &ft->priority, NFPROTO_NETDEV)) + return chain_error(ctx, ft, "'%s' is invalid priority.", + ft->priostr); + if (!ft->dev_expr) return chain_error(ctx, ft, "Unbound flowtable not allowed (must specify devices)"); @@ -3038,6 +3086,14 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain) chain->hookstr); } + if (!chain->priostr) + return chain_error(ctx, chain, "chain priority must be specified."); + if (!parse_evaluate_priority(chain->priostr, &chain->priority, + chain->handle.family)) + return chain_error(ctx, chain, + "'%s' is invalid priority in this context.", + chain->priostr); + list_for_each_entry(rule, &chain->rules, list) { handle_merge(&rule->handle, &chain->handle); if (rule_evaluate(ctx, rule) < 0) diff --git a/src/parser_bison.y b/src/parser_bison.y index 98bfeba..2b7d7cc 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -182,6 +182,8 @@ int nft_lex(void *, void *, void *); %token AT "@" %token VMAP "vmap" +%token PLUS "+" + %token INCLUDE "include" %token DEFINE "define" %token REDEFINE "redefine" @@ -522,6 +524,7 @@ int nft_lex(void *, void *, void *); %type <handle> set_spec setid_spec set_identifier flowtable_identifier obj_spec objid_spec obj_identifier %destructor { handle_free(&$$); } set_spec setid_spec set_identifier obj_spec objid_spec obj_identifier %type <val> family_spec family_spec_explicit chain_policy prio_spec +%type <string> str_prio_spec %type <string> dev_spec quota_unit %destructor { xfree($$); } dev_spec quota_unit @@ -1633,7 +1636,7 @@ flowtable_block_alloc : /* empty */ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } | flowtable_block common_block | flowtable_block stmt_separator - | flowtable_block HOOK STRING PRIORITY prio_spec stmt_separator + | flowtable_block HOOK STRING PRIORITY str_prio_spec stmt_separator { $$->hookstr = chain_hookname_lookup($3); if ($$->hookstr == NULL) { @@ -1644,7 +1647,7 @@ flowtable_block : /* empty */ { $$ = $<flowtable>-1; } } xfree($3); - $$->priority = $5; + $$->priostr = $5; } | flowtable_block DEVICES '=' flowtable_expr stmt_separator { @@ -1766,7 +1769,7 @@ type_identifier : STRING { $$ = $1; } | CLASSID { $$ = xstrdup("classid"); } ; -hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec +hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY str_prio_spec { const char *chain_type = chain_type_name_lookup($2); @@ -1789,13 +1792,34 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec xfree($4); $<chain>0->dev = $5; - $<chain>0->priority = $7; + $<chain>0->priostr = $7; $<chain>0->flags |= CHAIN_F_BASECHAIN; } ; -prio_spec : NUM { $$ = $1; } - | DASH NUM { $$ = -$2; } +str_prio_spec : prio_spec + { + char buf[STD_PRIO_BUFSIZE]; + snprintf(buf, STD_PRIO_BUFSIZE, "%d", (int)$1); + $$ = xstrdup(buf); + } + | STRING { $$ = xstrdup($1); } + | STRING PLUS NUM + { + char buf[STD_PRIO_BUFSIZE]; + snprintf(buf, STD_PRIO_BUFSIZE, "%s+%d",$1, (int)$3); + $$ = xstrdup(buf); + } + | STRING DASH NUM + { + char buf[STD_PRIO_BUFSIZE]; + snprintf(buf, STD_PRIO_BUFSIZE, "%s-%d", $1, (int)$3); + $$ = xstrdup(buf); + } + ; + +prio_spec : NUM { $$ = $1; } + | DASH NUM { $$ = -$2; } ; dev_spec : DEVICE STRING { $$ = $2; } diff --git a/src/rule.c b/src/rule.c index 56b956a..c2222fe 100644 --- a/src/rule.c +++ b/src/rule.c @@ -28,6 +28,7 @@ #include <netinet/ip.h> #include <linux/netfilter.h> #include <linux/netfilter_arp.h> +#include <linux/netfilter_ipv4.h> void handle_free(struct handle *h) { @@ -671,6 +672,7 @@ void chain_free(struct chain *chain) xfree(chain->type); if (chain->dev != NULL) xfree(chain->dev); + xfree(chain->priostr); xfree(chain); } @@ -769,9 +771,112 @@ const char *chain_policy2str(uint32_t policy) return "unknown"; } +struct prio_tag { + int val; + const char *str; +}; + +const static struct prio_tag std_prios[] = { + { NF_IP_PRI_RAW, "raw" }, + { NF_IP_PRI_MANGLE, "mangle" }, + { NF_IP_PRI_NAT_DST, "dstnat" }, + { NF_IP_PRI_FILTER, "filter" }, + { NF_IP_PRI_SECURITY, "security" }, + { NF_IP_PRI_NAT_SRC, "srcnat" }, +}; + +static bool std_prio_family_compat(int prio, int family) +{ + switch(prio) { + case NF_IP_PRI_FILTER: + switch(family) { + case NFPROTO_INET: + case NFPROTO_IPV4: + case NFPROTO_IPV6: + case NFPROTO_ARP: + case NFPROTO_BRIDGE: + case NFPROTO_NETDEV: + return true; + default: + return false; + } + case NF_IP_PRI_RAW: + case NF_IP_PRI_MANGLE: + case NF_IP_PRI_SECURITY: + switch(family) { + case NFPROTO_INET: + case NFPROTO_IPV4: + case NFPROTO_IPV6: + return true; + default: + return false; + } + case NF_IP_PRI_NAT_DST: + case NF_IP_PRI_NAT_SRC: + switch(family) { + case NFPROTO_INET: + case NFPROTO_IPV4: + case NFPROTO_IPV6: + case NFPROTO_BRIDGE: + return true; + default: + return false; + } + default: + return false; + } +} + +int std_prio_lookup(const char *std_prio_name, int family) +{ + long unsigned int i; + + for (i = 0; i < array_size(std_prios); ++i) { + if (strcmp(std_prios[i].str, std_prio_name) == 0 && + std_prio_family_compat(std_prios[i].val, family)) + return std_prios[i].val; + } + return NF_IP_PRI_LAST; +} + +static const char *prio2str(char *buf, size_t bufsize, int family, + int prio, int numeric) +{ + const int reach = 10; + size_t i; + int std_prio, offset; + const char *std_prio_str; + + if (numeric != NFT_NUMERIC_ALL) { + for (i = 0; i < array_size(std_prios); ++i) { + std_prio = std_prios[i].val; + std_prio_str = std_prios[i].str; + if (abs(prio - std_prio) <= reach) { + if (!std_prio_family_compat(std_prio, family)) + break; + offset = prio - std_prio; + strncpy(buf, std_prio_str, bufsize); + if (offset > 0) + snprintf(buf + strlen(buf), + bufsize - strlen(buf), " + %d", + offset); + else if (offset < 0) + snprintf(buf + strlen(buf), + bufsize - strlen(buf), " - %d", + -offset); + return buf; + } + } + } + snprintf(buf, bufsize, "%d", prio); + return buf; +} + static void chain_print_declaration(const struct chain *chain, struct output_ctx *octx) { + char priobuf[STD_PRIO_BUFSIZE]; + nft_print(octx, "\tchain %s {", chain->handle.chain.name); if (octx->handle > 0) nft_print(octx, " # handle %" PRIu64, chain->handle.handle.id); @@ -781,8 +886,11 @@ static void chain_print_declaration(const struct chain *chain, hooknum2str(chain->handle.family, chain->hooknum)); if (chain->dev != NULL) nft_print(octx, " device %s", chain->dev); - nft_print(octx, " priority %d; policy %s;\n", - chain->priority, chain_policy2str(chain->policy)); + nft_print(octx, " priority %s; policy %s;\n", + prio2str(priobuf, sizeof(priobuf), + chain->handle.family, + chain->priority, octx->numeric), + chain_policy2str(chain->policy)); } } @@ -802,13 +910,18 @@ static void chain_print(const struct chain *chain, struct output_ctx *octx) void chain_print_plain(const struct chain *chain, struct output_ctx *octx) { + char priobuf[STD_PRIO_BUFSIZE]; + nft_print(octx, "chain %s %s %s", family2str(chain->handle.family), chain->handle.table.name, chain->handle.chain.name); if (chain->flags & CHAIN_F_BASECHAIN) { - nft_print(octx, " { type %s hook %s priority %d; policy %s; }", + nft_print(octx, " { type %s hook %s priority %s; policy %s; }", chain->type, chain->hookstr, - chain->priority, chain_policy2str(chain->policy)); + prio2str(priobuf, sizeof(priobuf), + chain->handle.family, + chain->priority, octx->numeric), + chain_policy2str(chain->policy)); } if (octx->handle > 0) nft_print(octx, " # handle %" PRIu64, chain->handle.handle.id); @@ -1636,6 +1749,7 @@ void flowtable_free(struct flowtable *flowtable) if (--flowtable->refcnt > 0) return; handle_free(&flowtable->handle); + xfree(flowtable->priostr); xfree(flowtable); } @@ -1648,6 +1762,7 @@ static void flowtable_print_declaration(const struct flowtable *flowtable, struct print_fmt_options *opts, struct output_ctx *octx) { + char priobuf[STD_PRIO_BUFSIZE]; int i; nft_print(octx, "%sflowtable", opts->tab); @@ -1660,10 +1775,12 @@ static void flowtable_print_declaration(const struct flowtable *flowtable, nft_print(octx, " %s {%s", flowtable->handle.flowtable, opts->nl); - nft_print(octx, "%s%shook %s priority %d%s", + nft_print(octx, "%s%shook %s priority %s%s", opts->tab, opts->tab, hooknum2str(NFPROTO_NETDEV, flowtable->hooknum), - flowtable->priority, opts->stmt_separator); + prio2str(priobuf, sizeof(priobuf), NFPROTO_NETDEV, + flowtable->priority, octx->numeric), + opts->stmt_separator); nft_print(octx, "%s%sdevices = { ", opts->tab, opts->tab); for (i = 0; i < flowtable->dev_array_len; i++) { diff --git a/src/scanner.l b/src/scanner.l index ed01b5e..4fb3a39 100644 --- a/src/scanner.l +++ b/src/scanner.l @@ -232,6 +232,8 @@ addrstring ({macaddr}|{ip4addr}|{ip6addr}) "=" { return '='; } "vmap" { return VMAP; } +"+" { return PLUS; } + "include" { return INCLUDE; } "define" { return DEFINE; } "redefine" { return REDEFINE; }
This patch adds the possibility to use textual names to set the chain priority to standard values so that numeric values do not need to be learnt any more for basic usage. Basic arithmetic can also be done with them to ease the addition of relatively higher/lower priority chains. Addition and substraction is possible. Values are also printed with their friendly name within the range of <basicprio> +- 10. Also numeric printing is supported in case of -nnn option (numeric == NFT_NUMERIC_ALL) Not all names apply to every family. The supported names are based on the tables they have in their x_tableas implementation. These are the following: ip,ip6,inet filter nat (dstnat, srcnat) mangle raw security arp, netdev, flowtables filter bridge filter nat (dstnat, srcnat) broute (no corresponding priority value) Example: nft> add table ip x nft> add chain ip x y { type filter hook prerouting priority raw; } nft> add chain ip x z { type filter hook prerouting priority mangle + 1; } nft> add chain ip x w { type filter hook prerouting priority dstnat - 5; } nft> add chain ip x r { type filter hook prerouting priority filter + 10; } nft> add chain ip x t { type filter hook prerouting priority security; } nft> add chain ip x q { type filter hook prerouting priority srcnat + 11; } nft> add chain ip x h { type filter hook prerouting priority 15; } nft> nft> add flowtable ip x y { hook ingress priority filter + 5 ; devices = {enp0s31f6}; } nft> nft> add table arp x nft> add chain arp x y { type filter hook input priority filter + 5; } nft> nft> list ruleset table ip x { flowtable y { hook ingress priority filter + 5 devices = { enp0s31f6 } } chain y { type filter hook prerouting priority raw; policy accept; } chain z { type filter hook prerouting priority mangle + 1; policy accept; } chain w { type filter hook prerouting priority dstnat - 5; policy accept; } chain r { type filter hook prerouting priority filter + 10; policy accept; } chain t { type filter hook prerouting priority security; policy accept; } chain q { type filter hook prerouting priority 111; policy accept; } chain h { type filter hook prerouting priority 15; policy accept; } } table arp x { chain y { type filter hook input priority filter + 5; policy accept; } } nft> nft> add chain ip x h { type filter hook prerouting priority first; } Error: 'first' is invalid for priority in this context. add chain ip x h { type filter hook prerouting priority first; } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ nft> add chain arp x y { type filter hook input priority raw; } Error: 'raw' is invalid for priority in this context. add chain arp x y { type filter hook input priority raw; } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ nft> add flowtable ip x y { hook ingress priority magle; devices = {enp0s31f6}; } Error: 'magle' is invalid for priority. add flowtable ip x y { hook ingress priority magle; devices = {enp0s31f6}; } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Signed-off-by: Máté Eckl <ecklm94@gmail.com> --- v4: - fix snat and dnat conflict with existing tokens - remove static char array from chain_prio2str - make numerical priority printing available via -nnn nft flag - add docs about priority names - check compatibility of standard prio names and table family - handle flowtables doc/nft.xml | 68 ++++++++++++++++++++++-- include/rule.h | 5 ++ src/evaluate.c | 56 ++++++++++++++++++++ src/parser_bison.y | 36 ++++++++++--- src/rule.c | 129 ++++++++++++++++++++++++++++++++++++++++++--- src/scanner.l | 2 + 6 files changed, 281 insertions(+), 15 deletions(-)