On Thu, Jun 07, 2018 at 02:19:02AM +0200, Pablo Neira Ayuso wrote:
> Hi Máté,
>
> Thanks for working on this.
>
> See comments below.
>
> On Wed, Jun 06, 2018 at 09:33:56PM +0200, Máté Eckl wrote:
> > v2:
> > - more comprehensive names
> > - expose basic priorities used by iptables
> > - use arithmetics with new names (+-)
> > - print friendly names with arithmetics with an epsilon of 10
> >
> > -- 8< --
> > This patch adds the possibility to use textual names to set the chain priority
> > to basic 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.
> >
> > Example:
> > nft> add table x
> > nft> add chain x y { type filter hook prerouting priority mangleprio; }
> > nft> add chain x z { type filter hook prerouting priority mangleprio + 1; }
> > nft> add chain x w { type filter hook prerouting priority mangleprio - 5; }
> > nft> add chain x r { type filter hook prerouting priority filterprio + 10; }
> > nft> add chain x t { type filter hook prerouting priority rawprio; }
> > nft> add chain x q { type filter hook prerouting priority rawprio + 11; }
> > nft> list ruleset
> > table ip x {
> > chain y {
> > type filter hook prerouting priority mangleprio; policy accept;
> > }
> >
> > chain z {
> > type filter hook prerouting priority mangleprio + 1; policy accept;
> > }
> >
> > chain w {
> > type filter hook prerouting priority mangleprio - 5; policy accept;
> > }
> >
> > chain r {
> > type filter hook prerouting priority filterprio + 10; policy accept;
> > }
> >
> > chain t {
> > type filter hook prerouting priority rawprio; policy accept;
> > }
> >
> > chain q {
> > type filter hook prerouting priority -289; policy accept;
> > }
> > }
> >
> > Signed-off-by: Máté Eckl <ecklm94@xxxxxxxxx>
> > ---
> > src/parser_bison.y | 27 ++++++++++++++++++++++++---
> > src/rule.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
> > src/scanner.l | 9 +++++++++
> > 3 files changed, 75 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/parser_bison.y b/src/parser_bison.y
> > index 33915ed..10b4f96 100644
> > --- a/src/parser_bison.y
> > +++ b/src/parser_bison.y
> > @@ -21,6 +21,7 @@
> > #include <linux/netfilter/nf_conntrack_tuple_common.h>
> > #include <linux/netfilter/nf_nat.h>
> > #include <linux/netfilter/nf_log.h>
> > +#include <linux/netfilter_ipv4.h>
> > #include <netinet/ip_icmp.h>
> > #include <netinet/icmp6.h>
> > #include <libnftnl/common.h>
> > @@ -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"
> > @@ -313,6 +316,13 @@ int nft_lex(void *, void *, void *);
> > %token NEXTHDR "nexthdr"
> > %token HOPLIMIT "hoplimit"
> >
> > +%token PRIO_RAW "rawprio"
> > +%token PRIO_MANGLE "mangleprio"
> > +%token PRIO_NAT_DST "dstnatprio"
> > +%token PRIO_FILTER "filterprio"
> > +%token PRIO_SECURITY "securityprio"
> > +%token PRIO_NAT_SRC "srcnatprio"
>
> I would use:
>
> raw
> mangle
> dstnat
> filter
> security
> srcnat
>
> for the priority tags.
Ok.
> > %token ICMP6 "icmpv6"
> > %token PPTR "param-problem"
> > %token MAXDELAY "max-delay"
> > @@ -520,7 +530,7 @@ int nft_lex(void *, void *, void *);
> > %destructor { handle_free(&$$); } table_spec tableid_spec chain_spec chainid_spec flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec
> > %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 <val> family_spec family_spec_explicit chain_policy prio_spec prio_base
> >
> > %type <string> dev_spec quota_unit
> > %destructor { xfree($$); } dev_spec quota_unit
> > @@ -1792,8 +1802,19 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec
> > }
> > ;
> >
> > -prio_spec : NUM { $$ = $1; }
> > - | DASH NUM { $$ = -$2; }
> > +prio_spec : NUM { $$ = $1; }
> > + | DASH NUM { $$ = -$2; }
> > + | prio_base
> > + | prio_base PLUS NUM { $$ = $1 + $3; }
> > + | prio_base DASH NUM { $$ = $1 - $3; }
> > + ;
> > +
> > +prio_base : PRIO_RAW { $$ = NF_IP_PRI_RAW; }
> > + | PRIO_MANGLE { $$ = NF_IP_PRI_MANGLE; }
> > + | PRIO_NAT_DST { $$ = NF_IP_PRI_NAT_DST; }
> > + | PRIO_FILTER { $$ = NF_IP_PRI_FILTER; }
> > + | PRIO_SECURITY { $$ = NF_IP_PRI_SECURITY; }
> > + | PRIO_NAT_SRC { $$ = NF_IP_PRI_NAT_SRC; }
> > ;
> >
> > dev_spec : DEVICE STRING { $$ = $2; }
> > diff --git a/src/rule.c b/src/rule.c
> > index 3e8dea4..afb9510 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)
> > {
> > @@ -752,6 +753,43 @@ const char *chain_policy2str(uint32_t policy)
> > return "unknown";
> > }
> >
> > +static bool chain_prio2str_helper(char *dst, const int origo, const char* origostr,
> > + const int prio, const int e)
>
> Interesting code, this chain_prio2str_helper() function.
>
> > +{
> > + char t[15];
> > +
> > + t[0] = 0;
> > + if (prio >= origo - e && prio <= origo + e) {
> > + int tp = prio - origo;
>
> 'tp' is the offset, right?
tp originally standed for 'tail priority' but offset is a better word for this.
> > + if (tp != 0) {
> > + sprintf(t, " %c %d",tp > 0 ? '+' : '-', abs(tp));
> > + }
> > + sprintf(dst, "%s%s", origostr, t);
> > + return true;
> > + }
> > + return false;
>
>
> 'origo' is the standard priority, right? I would rename this to
> standard_prio.
yes, ok.
> 'e' looks like the reach for approximate matching, so I would rename
> it to 'reach'. This is a fixed value, 10 in this patch, so better
> define a constant with a meaningful name. Any reason for choosing 10,
> BTW? Don't worry, I understand this may be just an arbitrary value you
> just picked.
e is a shorthand for epsilon it can also be thought as a radius, but reach is
also fine.
10 is just an intentional value. I thought that, if this is for indicating that
I want to relatively set the priority to a standard one, it is not likely to set
it further than 10 as that would mean that I need eg. mangling in more than 20
chains with different priorities which is far not average use, so not something
that I wanted to support with this patch originally.
Whatever this number is, I think it is important not to make the user think
about any overlap, or original value. If it is too big, the actual value of
standard priorities may get important again in average use.
> > +}
> > +
> > +static const char *chain_prio2str(int prio)
> > +{
> > + static char pe[256];
> > + const int e = 10;
> > +
> > + pe[0] = 0;
> > + if(chain_prio2str_helper(pe, NF_IP_PRI_RAW, "rawprio", prio, e) ||
> > + chain_prio2str_helper(pe, NF_IP_PRI_MANGLE, "mangleprio", prio, e) ||
> > + chain_prio2str_helper(pe, NF_IP_PRI_NAT_DST, "dstnatprio", prio, e) ||
> > + chain_prio2str_helper(pe, NF_IP_PRI_FILTER, "filterprio", prio, e) ||
> > + chain_prio2str_helper(pe, NF_IP_PRI_SECURITY, "securityprio", prio, e) ||
> > + chain_prio2str_helper(pe, NF_IP_PRI_NAT_SRC, "srcnatprio", prio, e)
>
> Probably place this in an array and loop over it.
Like this? I don't know how common or accepted is it to use inline struct
definitions, but I think this is the only better way then the actual.
static const char *chain_prio2str(int prio)
{
static char pe[256];
const int reach = 10;
size_t i;
struct tmp {
int val;
const char *str;
};
const static struct tmp prios[] = {
{ NF_IP_PRI_RAW, "raw" },
{ NF_IP_PRI_MANGLE, "mangle" },
{ NF_IP_PRI_NAT_DST, "dnat" },
{ NF_IP_PRI_FILTER, "filter" },
{ NF_IP_PRI_SECURITY, "security" },
{ NF_IP_PRI_NAT_SRC, "snat" },
};
pe[0] = 0;
for (i = 0; i < sizeof(prios); ++i) {
if (chain_prio2str_helper(pe, prios[i].val, prios[i].str, prio, reach))
return pe;
}
sprintf(pe, "%d", prio);
return pe;
}
>
> > + )
> > + return pe;
> > + else {
> > + sprintf(pe, "%d", prio);
> > + return pe;
> > + }
> > +}
> > +
> > static void chain_print_declaration(const struct chain *chain,
> > struct output_ctx *octx)
> > {
> > @@ -764,8 +802,8 @@ 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",
> > + chain_prio2str(chain->priority), chain_policy2str(chain->policy));
> > }
> > }
> >
> > @@ -789,9 +827,9 @@ void chain_print_plain(const struct chain *chain, struct output_ctx *octx)
> > 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));
> > + chain_prio2str(chain->priority), chain_policy2str(chain->policy));
> > }
> > if (octx->handle > 0)
> > nft_print(octx, " # handle %" PRIu64, chain->handle.handle.id);
> > diff --git a/src/scanner.l b/src/scanner.l
> > index 416bd27..887e80c 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; }
> > @@ -427,6 +429,13 @@ addrstring ({macaddr}|{ip4addr}|{ip6addr})
> > "nexthdr" { return NEXTHDR; }
> > "hoplimit" { return HOPLIMIT; }
> >
> > +"rawprio" { return PRIO_RAW; }
> > +"mangleprio" { return PRIO_MANGLE; }
> > +"dstnatprio" { return PRIO_NAT_DST; }
> > +"filterprio" { return PRIO_FILTER; }
> > +"securityprio" { return PRIO_SECURITY; }
> > +"srcnatprio" { return PRIO_NAT_SRC; }
>
> Any way we can avoid defining tokens?
I just want to avoid strcmp-ing every name and associating their value manually.
Is there a way, I can do this without tokens?
If I use these without the 'prio' suffix as you suggested, some of them are also
defined (snat, dnat), and the others may become quite general and thus reusable.
This would be them:
"raw" { return RAW; }
"mangle" { return MANGLE; }
"filter" { return FILTER; }
"security" { return SECURITY; }
The problem with this is that these keywords will cause problem where these are
expected as strings (like filter chain definition...).
>
> > "icmpv6" { return ICMP6; }
> > "param-problem" { return PPTR; }
> > "max-delay" { return MAXDELAY; }
> > --
> > ecklm
> >
> > --
> > 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
--
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
