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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
