[libnftnl PATCH 0/2] chain: Support per chain rules list

[Phil Sutter]

This series implements a rule list in chains to allow for per chain rule
caches in iptables-nft as well as nftables.

A second patch then adds utility functions for chain and rule lookups,
preparing for further optimizing these tasks in a transparent way since
users won't open-code the chain/rule list traversal anymore.

Phil Sutter (2):
  chain: Support per chain rules list
  chain: Add lookup functions for chain list and rules in chain

 include/internal.h   |   1 +
 include/libnftnl/chain.h |  17 +
 include/rule.h   |  26 
 src/chain.c  | 132 ++-
 src/libnftnl.map |  13 
 src/rule.c   |  22 ---
 6 files changed, 188 insertions(+), 23 deletions(-)
 create mode 100644 include/rule.h

-- 
2.19.0

[libnftnl PATCH 2/2] chain: Add lookup functions for chain list and rules in chain

[Phil Sutter]

For now, these lookup functions simply iterate over the linked list
until they find the right entry. In future, they may make use of more
optimized data structures behind the curtains.

Signed-off-by: Phil Sutter 
---
 include/libnftnl/chain.h |  2 ++
 src/chain.c  | 28 
 src/libnftnl.map |  3 +++
 3 files changed, 33 insertions(+)

diff --git a/include/libnftnl/chain.h b/include/libnftnl/chain.h
index f04f61056cc7c..64e10e91aaefe 100644
--- a/include/libnftnl/chain.h
+++ b/include/libnftnl/chain.h
@@ -76,6 +76,7 @@ int nftnl_chain_nlmsg_parse(const struct nlmsghdr *nlh, 
struct nftnl_chain *t);
 int nftnl_rule_foreach(struct nftnl_chain *c,
  int (*cb)(struct nftnl_rule *r, void *data),
  void *data);
+struct nftnl_rule *nftnl_rule_lookup_byindex(struct nftnl_chain *c, uint32_t 
index);
 
 struct nftnl_rule_iter;
 
@@ -89,6 +90,7 @@ struct nftnl_chain_list *nftnl_chain_list_alloc(void);
 void nftnl_chain_list_free(struct nftnl_chain_list *list);
 int nftnl_chain_list_is_empty(const struct nftnl_chain_list *list);
 int nftnl_chain_list_foreach(struct nftnl_chain_list *chain_list, int 
(*cb)(struct nftnl_chain *t, void *data), void *data);
+struct nftnl_chain *nftnl_chain_list_lookup_byname(struct nftnl_chain_list 
*chain_list, const char *chain);
 
 void nftnl_chain_list_add(struct nftnl_chain *r, struct nftnl_chain_list 
*list);
 void nftnl_chain_list_add_tail(struct nftnl_chain *r, struct nftnl_chain_list 
*list);
diff --git a/src/chain.c b/src/chain.c
index c8b7f9ba12618..8668fb7d1494d 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -734,6 +734,20 @@ int nftnl_rule_foreach(struct nftnl_chain *c,
return 0;
 }
 
+EXPORT_SYMBOL(nftnl_rule_lookup_byindex);
+struct nftnl_rule *
+nftnl_rule_lookup_byindex(struct nftnl_chain *c, uint32_t index)
+{
+   struct nftnl_rule *r;
+
+   list_for_each_entry(r, >rule_list, head) {
+   if (!index)
+   return r;
+   index--;
+   }
+   return NULL;
+}
+
 struct nftnl_rule_iter {
const struct nftnl_chain*c;
struct nftnl_rule   *cur;
@@ -856,6 +870,20 @@ int nftnl_chain_list_foreach(struct nftnl_chain_list 
*chain_list,
return 0;
 }
 
+EXPORT_SYMBOL(nftnl_chain_list_lookup_byname);
+struct nftnl_chain *
+nftnl_chain_list_lookup_byname(struct nftnl_chain_list *chain_list,
+  const char *chain)
+{
+   struct nftnl_chain *c;
+
+   list_for_each_entry(c, _list->list, head) {
+   if (!strcmp(chain, c->name))
+   return c;
+   }
+   return NULL;
+}
+
 struct nftnl_chain_list_iter {
const struct nftnl_chain_list   *list;
struct nftnl_chain  *cur;
diff --git a/src/libnftnl.map b/src/libnftnl.map
index 96d5b5f1cec49..0d3be32263eee 100644
--- a/src/libnftnl.map
+++ b/src/libnftnl.map
@@ -345,4 +345,7 @@ LIBNFTNL_12 {
   nftnl_rule_iter_create;
   nftnl_rule_iter_next;
   nftnl_rule_iter_destroy;
+
+  nftnl_chain_list_lookup_byname;
+  nftnl_rule_lookup_byindex;
 } LIBNFTNL_11;
-- 
2.19.0

[iptables PATCH] extensions: libipt_realm: Document allowed realm values

[Phil Sutter]

Older versions of iptables allowed for negative realm values by accident
(they would be cast to unsigned). While this was clearly a bug, document
the fixed behaviour.

Signed-off-by: Phil Sutter 
---
 extensions/libipt_realm.man | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/extensions/libipt_realm.man b/extensions/libipt_realm.man
index a40b1adc72ba2..72dff9b2e4212 100644
--- a/extensions/libipt_realm.man
+++ b/extensions/libipt_realm.man
@@ -5,3 +5,5 @@ setups involving dynamic routing protocols like BGP.
 Matches a given realm number (and optionally mask). If not a number, value
 can be a named realm from /etc/iproute2/rt_realms (mask can not be used in
 that case).
+Both value and mask are four byte unsigned integers and may be specified in
+decimal, hex (by prefixing with "0x") or octal (if a leading zero is given).
-- 
2.19.0

Re: RFC: Designing per chain rule cache support in libnftnl

[Phil Sutter]

Hi,

On Wed, Nov 28, 2018 at 02:51:54PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Nov 28, 2018 at 02:21:01PM +0100, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Fri, Nov 23, 2018 at 01:35:17PM +0100, Pablo Neira Ayuso wrote:
> > > On Fri, Nov 23, 2018 at 12:25:45PM +0100, Florian Westphal wrote:
> > > > Phil Sutter  wrote:
> > > > > > If user doesn't want it cleared at nftnl_chain_free() time they can
> > > > > > always allocate a new nftnl_rule_list and splice to that list.
> > > > > 
> > > > > Good point. What do you think about the simple approach of 
> > > > > introducing:
> > > > > 
> > > > > | struct nftnl_rule_list *nftnl_chain_get_rule_list(const struct 
> > > > > nftnl_chain *);
> > > > 
> > > > Looks fine to me.
> > > > 
> > > > > This would allow to reuse nftnl_rule_list routines from 
> > > > > libnftnl/rule.h.
> > > > > One potential problem I see is that users may try to call
> > > > > nftnl_rule_list_free(). Can we prevent that somehow?
> > > > 
> > > > Document that nftnl_rule_list_free() pairs with nftnl_rule_list_alloc() 
> > > > :-)
> > > > 
> > > > I don't think its an issue.
> > > > We could add a 'bool make_free_no_op' to nftnl_rule_list and set that to
> > > > true for nftnl_rule_list structures that are allocated indirectly on
> > > > behalf of nftnl_chain struct, but I think thats taking things too far.
> > > 
> > > Can we have an interface similar to nftnl_rule_add_expr() to add rules
> > > to chains?
> > > 
> > > So we add list field to nftnl_chain, and this new interface to
> > > add/delete rules.
> > 
> > I didn't look at struct nftnl_rule yet. OK, that seems rather different
> > from what I had in mind. So I guess your idea would be to add a field of
> > type struct list_head instead of struct nftnl_rule_list and implement
> > struct nftnl_rule_iter and relevant functions?
> 
> Yes. We would make explicit the relation between the objects, which
> makes sense to me. So far only nftnl_rule and nftnl_expr are basically
> "linked" in some way.
> 
> Would this approach for you?

Yes, that's fine with me. My idea was to reuse the nftnl_rule_list API,
but creating chains' rule lists in a consistent manner with respect to
rules' expression lists is probably more important long-term.

Thanks, Phil

Re: RFC: Designing per chain rule cache support in libnftnl

[Phil Sutter]

Hi Pablo,

On Fri, Nov 23, 2018 at 01:35:17PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Nov 23, 2018 at 12:25:45PM +0100, Florian Westphal wrote:
> > Phil Sutter  wrote:
> > > > If user doesn't want it cleared at nftnl_chain_free() time they can
> > > > always allocate a new nftnl_rule_list and splice to that list.
> > > 
> > > Good point. What do you think about the simple approach of introducing:
> > > 
> > > | struct nftnl_rule_list *nftnl_chain_get_rule_list(const struct 
> > > nftnl_chain *);
> > 
> > Looks fine to me.
> > 
> > > This would allow to reuse nftnl_rule_list routines from libnftnl/rule.h.
> > > One potential problem I see is that users may try to call
> > > nftnl_rule_list_free(). Can we prevent that somehow?
> > 
> > Document that nftnl_rule_list_free() pairs with nftnl_rule_list_alloc() :-)
> > 
> > I don't think its an issue.
> > We could add a 'bool make_free_no_op' to nftnl_rule_list and set that to
> > true for nftnl_rule_list structures that are allocated indirectly on
> > behalf of nftnl_chain struct, but I think thats taking things too far.
> 
> Can we have an interface similar to nftnl_rule_add_expr() to add rules
> to chains?
> 
> So we add list field to nftnl_chain, and this new interface to
> add/delete rules.

I didn't look at struct nftnl_rule yet. OK, that seems rather different
from what I had in mind. So I guess your idea would be to add a field of
type struct list_head instead of struct nftnl_rule_list and implement
struct nftnl_rule_iter and relevant functions?

> We can probably deprecate the existing list interface if we follow
> that procedure after a bit of time in favour of this one.

OK, cool.

Thanks, Phil

[iptables PATCH] xtables: Don't use native nftables comments

[Phil Sutter]

The problem with converting libxt_comment into nftables comment is that
rules change when parsing from kernel due to comment match being moved
to the end of the match list. And since match ordering matters, the rule
may not be found anymore when checking or deleting. Apart from that,
iptables-nft didn't support multiple comments per rule anymore. This is
a compatibility issue without technical reason.

Leave conversion from nftables comment to libxt_comment in place so we
don't break running systems during an update.

Signed-off-by: Phil Sutter 
---
 extensions/libxt_comment.t |  2 ++
 iptables/nft-ipv4.c| 14 +++---
 iptables/nft-ipv6.c| 14 +++---
 iptables/nft.c | 27 ---
 iptables/nft.h |  1 -
 5 files changed, 8 insertions(+), 50 deletions(-)

diff --git a/extensions/libxt_comment.t b/extensions/libxt_comment.t
index f12cd66841e7f..f0c8fb999401b 100644
--- a/extensions/libxt_comment.t
+++ b/extensions/libxt_comment.t
@@ -1,6 +1,8 @@
 :INPUT,FORWARD,OUTPUT
 -m comment;;FAIL
 -m comment --comment;;FAIL
+-p tcp -m tcp --dport 22 -m comment --comment foo;=;OK
+-p tcp -m comment --comment foo -m tcp --dport 22;=;OK
 #
 # it fails with 256 characters
 #
diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c
index ffb439b4a1128..4497eb9b9347c 100644
--- a/iptables/nft-ipv4.c
+++ b/iptables/nft-ipv4.c
@@ -77,17 +77,9 @@ static int nft_ipv4_add(struct nftnl_rule *r, void *data)
add_compat(r, cs->fw.ip.proto, cs->fw.ip.invflags & XT_INV_PROTO);
 
for (matchp = cs->matches; matchp; matchp = matchp->next) {
-   /* Use nft built-in comments support instead of comment match */
-   if (strcmp(matchp->match->name, "comment") == 0) {
-   ret = add_comment(r, (char *)matchp->match->m->data);
-   if (ret < 0)
-   goto try_match;
-   } else {
-try_match:
-   ret = add_match(r, matchp->match->m);
-   if (ret < 0)
-   return ret;
-   }
+   ret = add_match(r, matchp->match->m);
+   if (ret < 0)
+   return ret;
}
 
/* Counters need to me added before the target, otherwise they are
diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c
index 7bacee4ab3a21..cacb1c9e141f2 100644
--- a/iptables/nft-ipv6.c
+++ b/iptables/nft-ipv6.c
@@ -66,17 +66,9 @@ static int nft_ipv6_add(struct nftnl_rule *r, void *data)
add_compat(r, cs->fw6.ipv6.proto, cs->fw6.ipv6.invflags & XT_INV_PROTO);
 
for (matchp = cs->matches; matchp; matchp = matchp->next) {
-   /* Use nft built-in comments support instead of comment match */
-   if (strcmp(matchp->match->name, "comment") == 0) {
-   ret = add_comment(r, (char *)matchp->match->m->data);
-   if (ret < 0)
-   goto try_match;
-   } else {
-try_match:
-   ret = add_match(r, matchp->match->m);
-   if (ret < 0)
-   return ret;
-   }
+   ret = add_match(r, matchp->match->m);
+   if (ret < 0)
+   return ret;
}
 
/* Counters need to me added before the target, otherwise they are
diff --git a/iptables/nft.c b/iptables/nft.c
index 0223c0ed10001..7b6fb2b10686d 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1129,33 +1129,6 @@ enum udata_type {
 };
 #define UDATA_TYPE_MAX (__UDATA_TYPE_MAX - 1)
 
-int add_comment(struct nftnl_rule *r, const char *comment)
-{
-   struct nftnl_udata_buf *udata;
-   uint32_t len;
-
-   if (nftnl_rule_get_data(r, NFTNL_RULE_USERDATA, ))
-   return -EALREADY;
-
-   udata = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
-   if (!udata)
-   return -ENOMEM;
-
-   if (strnlen(comment, 255) == 255)
-   return -ENOSPC;
-
-   if (!nftnl_udata_put_strz(udata, UDATA_TYPE_COMMENT, comment))
-   return -ENOMEM;
-
-   nftnl_rule_set_data(r, NFTNL_RULE_USERDATA,
-   nftnl_udata_buf_data(udata),
-   nftnl_udata_buf_len(udata));
-
-   nftnl_udata_buf_free(udata);
-
-   return 0;
-}
-
 static int parse_udata_cb(const struct nftnl_udata *attr, void *data)
 {
unsigned char *value = nftnl_udata_get(attr);
diff --git a/iptables/nft.h b/iptables/nft.h
index 711199948a89f..bf60ab3943659 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -121,7 +121,6 @@ int add_match(struct nftnl_rule *r, struct xt_entry_match 
*m);
 int add_target(struct nftnl_rule *r, struct xt_entry_target *t);
 int add_jumpto(struct nftnl_rule *r, const char *name, int verdict);

[iptables PATCH] ebtables: Use xtables_exit_err()

[Phil Sutter]

When e.g. ebtables-nft detects an incompatible table, a stray '.' was
printed as last line of output:

| # ebtables-nft -L
| table `filter' is incompatible, use 'nft' tool.
| .

This comes from ebtables' own exit_err callback. Instead use the common
one which also provides useful version information.

While being at it, align the final error message in xtables_eb_main()
with how the others print it.

Signed-off-by: Phil Sutter 
---
 iptables/xtables-eb-standalone.c |  2 +-
 iptables/xtables-eb.c| 15 ++-
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/iptables/xtables-eb-standalone.c b/iptables/xtables-eb-standalone.c
index 84ce0b60a7076..fb3daba0bd604 100644
--- a/iptables/xtables-eb-standalone.c
+++ b/iptables/xtables-eb-standalone.c
@@ -54,7 +54,7 @@ int xtables_eb_main(int argc, char *argv[])
ret = nft_commit();
 
if (!ret)
-   fprintf(stderr, "%s\n", nft_strerror(errno));
+   fprintf(stderr, "ebtables: %s\n", nft_strerror(errno));
 
exit(!ret);
 }
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index f1aba555186eb..efc1f16ac6364 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -291,23 +291,12 @@ struct option ebt_original_options[] =
{ 0 }
 };
 
-static void __attribute__((__noreturn__,format(printf,2,3)))
-ebt_print_error(enum xtables_exittype status, const char *format, ...)
-{
-   va_list l;
-
-   va_start(l, format);
-   vfprintf(stderr, format, l);
-   fprintf(stderr, ".\n");
-   va_end(l);
-   exit(-1);
-}
-
+extern void xtables_exit_error(enum xtables_exittype status, const char *msg, 
...) __attribute__((noreturn, format(printf,2,3)));
 struct xtables_globals ebtables_globals = {
.option_offset  = 0,
.program_version= IPTABLES_VERSION,
.orig_opts  = ebt_original_options,
-   .exit_err   = ebt_print_error,
+   .exit_err   = xtables_exit_error,
.compat_rev = nft_compatible_revision,
 };
 
-- 
2.19.0

Re: RFC: Designing per chain rule cache support in libnftnl

[Phil Sutter]

On Fri, Nov 23, 2018 at 07:49:49AM +0100, Florian Westphal wrote:
> Phil Sutter  wrote:
> > In order to improve performance in 'nft -f' as well as xtables-restore
> > with very large rulesets, we need to store rules by chain they belong
> > to. In order to avoid pointless code duplication, this should be
> > supported by libnftnl.
> 
> Unfortunately we still need to change lookup algorithm as well
> (hash, tree?), linear list scan is too expensive.
> 
> We might even need multiple internal ways to keep track of the chains,
> e.g. to accelerate insert/delete-by-index :-/

That's right. I would "hide" these details within struct nftnl_rule_list
though and provide appropriate lookup routines.

For now, I'm focussing on the API, if we get it right the data structure
behind it is replaceable/extensible at will.

> > Looking into the topic, it seems like extending struct nftnl_chain is
> > the most straightforward way to go. My idea is to embed an
> > nftnl_rule_list in there, though I'm unsure how to best do that in
> > practice:
> > 
> > We could either add a field of type struct nftnl_rule_list which would
> > have to be initialized/cleared in nftnl_chain_alloc() and
> > nftnl_chain_free(). This would be accompanied by a function to retrieve
> > the pointer to that field so the existing rule_list routines may be used
> > with it.
> > 
> > Another option would be to add a pointer to a struct nftnl_rule_list.
> > Having a function to retrieve a pointer to that pointer, the rule_list
> > could be initialized/cleared by users on demand.
> > 
> > What do you consider more practical? Is there a third option I didn't
> > think of yet?
> 
> I'd vote for the former (embed nftnl_rule_list).

OK, thanks.

> If user doesn't want it cleared at nftnl_chain_free() time they can
> always allocate a new nftnl_rule_list and splice to that list.

Good point. What do you think about the simple approach of introducing:

| struct nftnl_rule_list *nftnl_chain_get_rule_list(const struct nftnl_chain *);

This would allow to reuse nftnl_rule_list routines from libnftnl/rule.h.
One potential problem I see is that users may try to call
nftnl_rule_list_free(). Can we prevent that somehow?

A more fool-proof (but somewhat tedious) solution would be to duplicate
nftnl_rule_list API for use on an nftnl_chain. But I don't quite like
that.

Cheers, Phil

[iptables PATCH] arptables: Support --set-counters option

[Phil Sutter]

Relevant code for this was already present (short option '-c'), just the
long option definition was missing.

While being at it, add '-c' to help text.

Signed-off-by: Phil Sutter 
---
 iptables/xtables-arp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 5a9924ca56442..2f369d9aadb01 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -144,6 +144,7 @@ static struct option original_opts[] = {
{ "help", 2, 0, 'h' },
{ "line-numbers", 0, 0, '0' },
{ "modprobe", 1, 0, 'M' },
+   { "set-counters", 1, 0, 'c' },
{ 0 }
 };
 
@@ -481,7 +482,7 @@ exit_printhelp(void)
 "  --line-numbers  print line numbers when listing\n"
 "  --exact -x  expand numbers (display exact values)\n"
 "  --modprobe=try to insert modules using this 
command\n"
-"  --set-counters PKTS BYTES   set the counter during insert/append\n"
+"  --set-counters -c PKTS BYTESset the counter during insert/append\n"
 "[!] --version -V  print package version.\n");
printf(" opcode strings: \n");
 for (i = 0; i < NUMOPCODES; i++)
-- 
2.19.0

RFC: Designing per chain rule cache support in libnftnl

[Phil Sutter]

Hi,

In order to improve performance in 'nft -f' as well as xtables-restore
with very large rulesets, we need to store rules by chain they belong
to. In order to avoid pointless code duplication, this should be
supported by libnftnl.

Looking into the topic, it seems like extending struct nftnl_chain is
the most straightforward way to go. My idea is to embed an
nftnl_rule_list in there, though I'm unsure how to best do that in
practice:

We could either add a field of type struct nftnl_rule_list which would
have to be initialized/cleared in nftnl_chain_alloc() and
nftnl_chain_free(). This would be accompanied by a function to retrieve
the pointer to that field so the existing rule_list routines may be used
with it.

Another option would be to add a pointer to a struct nftnl_rule_list.
Having a function to retrieve a pointer to that pointer, the rule_list
could be initialized/cleared by users on demand.

What do you consider more practical? Is there a third option I didn't
think of yet?

Thanks, Phil

Re: Different namespaces share the same xtables lock

[Phil Sutter]

Hi Wenxian,

On Thu, Nov 15, 2018 at 11:16:49PM +0800, wenxian li wrote:
> I was running iptables on different namespaces and met such error
> "Another app is currently holding the xtables lock. Perhaps you want
> to use the -w option?".
> 
> After googling it, I found this enhancement introduces the lock
> mechanism: "xtables: Add locking to prevent concurrent instances".
> Based on the commit log, different namespaces should have their own
> xtables locks.
> 
> However, based on my test, it looks like different namespaces share
> the same lock.
> The CLIs below can easily reproduce the issue.
> 
> ip netns exec test1  iptables -L -nv -t mangle   & ip netns exec test2
>  iptables -L -nv -t mangle & ip netns exec test3  iptables -L -nv -t
> mangle &
> 
> I'm wondering is it an expected behavior? Or some kind of bug?

The xtables-lock is filesystem-based (via flock()), default lock file
name is '/run/xtables.lock'. So despite you run in a different network
namespace, the concurrent command invocations will see each other.

I am not sure whether this restriction is necessary or not - on one hand
different netns' rulesets should not interfere, on the other it might be
the ABI which needs protection.

Cheers, Phil

[iptables PATCH v2] xtables: Introduce per table chain caches

[Phil Sutter]

Being able to omit the previously obligatory table name check when
iterating over the chain cache might help restore performance with large
rulesets in xtables-save and -restore.

There is one subtle quirk in the code: flush_chain_cache() did free the
global chain cache if not called with a table name but didn't if a table
name was given even if it emptied the chain cache. In other places,
chain_cache being non-NULL prevented a cache update from happening, so
this patch establishes the same behaviour (for each individual chain
cache) since otherwise unexpected cache updates lead to weird problems.

Signed-off-by: Phil Sutter 
---
Changes since v1:
- Get rid of a stray closing brace (a leftover from debug output
  removal).
---
 iptables/nft-shared.h  |   3 +-
 iptables/nft.c | 160 +
 iptables/nft.h |  10 ++-
 iptables/xtables-restore.c |  16 ++--
 iptables/xtables-save.c|  12 +--
 5 files changed, 95 insertions(+), 106 deletions(-)

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index e3ecdb4d23df3..9a61d8d2863e3 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -251,7 +251,8 @@ struct nftnl_chain_list;
 
 struct nft_xt_restore_cb {
void (*table_new)(struct nft_handle *h, const char *table);
-   struct nftnl_chain_list *(*chain_list)(struct nft_handle *h);
+   struct nftnl_chain_list *(*chain_list)(struct nft_handle *h,
+  const char *table);
void (*chain_del)(struct nftnl_chain_list *clist, const char *curtable,
  const char *chain);
int (*chain_user_flush)(struct nft_handle *h,
diff --git a/iptables/nft.c b/iptables/nft.c
index e8538d38e0109..5e55ec13d0da5 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -673,15 +673,17 @@ nft_chain_builtin_find(struct builtin_table *t, const 
char *chain)
 static void nft_chain_builtin_init(struct nft_handle *h,
   struct builtin_table *table)
 {
-   struct nftnl_chain_list *list = nft_chain_list_get(h);
+   struct nftnl_chain_list *list = nft_chain_list_get(h, table->name);
struct nftnl_chain *c;
int i;
 
+   if (!list)
+   return;
+
/* Initialize built-in chains if they don't exist yet */
for (i=0; i < NF_INET_NUMHOOKS && table->chains[i].name != NULL; i++) {
 
-   c = nft_chain_list_find(list, table->name,
-   table->chains[i].name);
+   c = nft_chain_list_find(list, table->chains[i].name);
if (c != NULL)
continue;
 
@@ -782,27 +784,33 @@ static void flush_rule_cache(struct nft_handle *h, const 
char *tablename)
 
 static int __flush_chain_cache(struct nftnl_chain *c, void *data)
 {
-   const char *tablename = data;
-
-   if (!strcmp(nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE), tablename)) {
-   nftnl_chain_list_del(c);
-   nftnl_chain_free(c);
-   }
+   nftnl_chain_list_del(c);
+   nftnl_chain_free(c);
 
return 0;
 }
 
 static void flush_chain_cache(struct nft_handle *h, const char *tablename)
 {
-   if (!h->chain_cache)
-   return;
+   int i;
 
-   if (tablename) {
-   nftnl_chain_list_foreach(h->chain_cache, __flush_chain_cache,
-(void *)tablename);
-   } else {
-   nftnl_chain_list_free(h->chain_cache);
-   h->chain_cache = NULL;
+   for (i = 0; i < NFT_TABLE_MAX; i++) {
+   if (h->tables[i].name == NULL)
+   continue;
+
+   if (tablename && strcmp(h->tables[i].name, tablename))
+   continue;
+
+   if (h->tables[i].chain_cache) {
+   if (tablename) {
+   
nftnl_chain_list_foreach(h->tables[i].chain_cache,
+__flush_chain_cache, 
NULL);
+   break;
+   } else {
+   nftnl_chain_list_free(h->tables[i].chain_cache);
+   h->tables[i].chain_cache = NULL;
+   }
+   }
}
 }
 
@@ -1271,8 +1279,9 @@ nft_rule_print_save(const struct nftnl_rule *r, enum 
nft_rule_print type,
 
 static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 {
+   struct nft_handle *h = data;
+   struct builtin_table *t;
struct nftnl_chain *c;
-   struct nftnl_chain_list *list = data;
 
c = nftnl_chain_alloc();
if (c == NULL)
@@ -1281,7 +1290,18 @@ static int nftnl_chain_list_cb(const struct nlmsghdr 
*nlh, void *data)
if (nftnl_chain_nlmsg_parse(nlh, c) < 0)
goto out;
 
-   nf

[iptables PATCH] xtables: Introduce per table chain caches

[Phil Sutter]

Being able to omit the previously obligatory table name check when
iterating over the chain cache might help restore performance with large
rulesets in xtables-save and -restore.

There is one subtle quirk in the code: flush_chain_cache() did free the
global chain cache if not called with a table name but didn't if a table
name was given even if it emptied the chain cache. In other places,
chain_cache being non-NULL prevented a cache update from happening, so
this patch establishes the same behaviour (for each individual chain
cache) since otherwise unexpected cache updates lead to weird problems.

Signed-off-by: Phil Sutter 
---
 iptables/nft-shared.h  |   3 +-
 iptables/nft.c | 161 +
 iptables/nft.h |  10 ++-
 iptables/xtables-restore.c |  16 ++--
 iptables/xtables-save.c|  12 +--
 5 files changed, 96 insertions(+), 106 deletions(-)

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index e3ecdb4d23df3..9a61d8d2863e3 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -251,7 +251,8 @@ struct nftnl_chain_list;
 
 struct nft_xt_restore_cb {
void (*table_new)(struct nft_handle *h, const char *table);
-   struct nftnl_chain_list *(*chain_list)(struct nft_handle *h);
+   struct nftnl_chain_list *(*chain_list)(struct nft_handle *h,
+  const char *table);
void (*chain_del)(struct nftnl_chain_list *clist, const char *curtable,
  const char *chain);
int (*chain_user_flush)(struct nft_handle *h,
diff --git a/iptables/nft.c b/iptables/nft.c
index e8538d38e0109..e320e073ac054 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -673,15 +673,18 @@ nft_chain_builtin_find(struct builtin_table *t, const 
char *chain)
 static void nft_chain_builtin_init(struct nft_handle *h,
   struct builtin_table *table)
 {
-   struct nftnl_chain_list *list = nft_chain_list_get(h);
+   struct nftnl_chain_list *list = nft_chain_list_get(h, table->name);
struct nftnl_chain *c;
int i;
 
+   if (!list)
+   return;
+   }
+
/* Initialize built-in chains if they don't exist yet */
for (i=0; i < NF_INET_NUMHOOKS && table->chains[i].name != NULL; i++) {
 
-   c = nft_chain_list_find(list, table->name,
-   table->chains[i].name);
+   c = nft_chain_list_find(list, table->chains[i].name);
if (c != NULL)
continue;
 
@@ -782,27 +785,33 @@ static void flush_rule_cache(struct nft_handle *h, const 
char *tablename)
 
 static int __flush_chain_cache(struct nftnl_chain *c, void *data)
 {
-   const char *tablename = data;
-
-   if (!strcmp(nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE), tablename)) {
-   nftnl_chain_list_del(c);
-   nftnl_chain_free(c);
-   }
+   nftnl_chain_list_del(c);
+   nftnl_chain_free(c);
 
return 0;
 }
 
 static void flush_chain_cache(struct nft_handle *h, const char *tablename)
 {
-   if (!h->chain_cache)
-   return;
+   int i;
 
-   if (tablename) {
-   nftnl_chain_list_foreach(h->chain_cache, __flush_chain_cache,
-(void *)tablename);
-   } else {
-   nftnl_chain_list_free(h->chain_cache);
-   h->chain_cache = NULL;
+   for (i = 0; i < NFT_TABLE_MAX; i++) {
+   if (h->tables[i].name == NULL)
+   continue;
+
+   if (tablename && strcmp(h->tables[i].name, tablename))
+   continue;
+
+   if (h->tables[i].chain_cache) {
+   if (tablename) {
+   
nftnl_chain_list_foreach(h->tables[i].chain_cache,
+__flush_chain_cache, 
NULL);
+   break;
+   } else {
+   nftnl_chain_list_free(h->tables[i].chain_cache);
+   h->tables[i].chain_cache = NULL;
+   }
+   }
}
 }
 
@@ -1271,8 +1280,9 @@ nft_rule_print_save(const struct nftnl_rule *r, enum 
nft_rule_print type,
 
 static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 {
+   struct nft_handle *h = data;
+   struct builtin_table *t;
struct nftnl_chain *c;
-   struct nftnl_chain_list *list = data;
 
c = nftnl_chain_alloc();
if (c == NULL)
@@ -1281,7 +1291,18 @@ static int nftnl_chain_list_cb(const struct nlmsghdr 
*nlh, void *data)
if (nftnl_chain_nlmsg_parse(nlh, c) < 0)
goto out;
 
-   nftnl_chain_list_add_tail(c, list);
+   t = nft_table_builtin_find(h,
+   nftnl_chain_ge

[ebtables PATCH] extensions: among: Fix bitmask check

[Phil Sutter]

Boolean AND was applied instead of binary one, causing the exclamation
mark to be printed whenever info->bitmask was non-zero. In practice,
this leads to incorrect output if e.g. --among-src was given with an
inverted match as well as --among-dst with a non-inverted one. Output
would then list both matches as inverted.

Signed-off-by: Phil Sutter 
---
 extensions/ebt_among.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/extensions/ebt_among.c b/extensions/ebt_among.c
index b1560e8f09e8d..30c098cf69f96 100644
--- a/extensions/ebt_among.c
+++ b/extensions/ebt_among.c
@@ -436,14 +436,14 @@ static void print(const struct ebt_u_entry *entry,
 
if (info->wh_dst_ofs) {
printf("--among-dst ");
-   if (info->bitmask && EBT_AMONG_DST_NEG) {
+   if (info->bitmask & EBT_AMONG_DST_NEG) {
printf("! ");
}
wormhash_printout(ebt_among_wh_dst(info));
}
if (info->wh_src_ofs) {
printf("--among-src ");
-   if (info->bitmask && EBT_AMONG_SRC_NEG) {
+   if (info->bitmask & EBT_AMONG_SRC_NEG) {
printf("! ");
}
wormhash_printout(ebt_among_wh_src(info));
-- 
2.19.0

[nft PATCH] nft.8: Clarify 'index' option of add rule command

[Phil Sutter]

Documentation for add rule command might trick readers into believing
the optional 'index' argument does not need to be that of an existing
rule. This false assumption is fueled by the fact that iptables allows
to insert with last rule number + 1 to actually append to a chain.
Change the relevant sentence to clarify that.

While being at it, drop the deprecated 'position' option from
documentation - since this will likely go away at some point, don't
encourage users to use it although they should notice that they
shoudn't.

Signed-off-by: Phil Sutter 
---
 doc/nft.txt | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/doc/nft.txt b/doc/nft.txt
index 030e67e950fee..fca91450f5b8f 100644
--- a/doc/nft.txt
+++ b/doc/nft.txt
@@ -404,7 +404,7 @@ values are *accept* (which is the default) or *drop*.
 RULES
 -
 [verse]
-{add | insert} *rule* ['family'] 'table' 'chain' [ {handle | position} 
'handle' | index 'index' ] 'statement' ... [ comment 'comment' ]
+{add | insert} *rule* ['family'] 'table' 'chain' [ handle 'handle' | index 
'index' ] 'statement' ... [ comment 'comment' ]
 replace *rule* ['family'] 'table' 'chain' handle 'handle' 'statement' ... [ 
comment 'comment' ]
 delete *rule* ['family'] 'table' 'chain' handle 'handle'
 
@@ -413,15 +413,13 @@ ip family is used. Rules are constructed from two kinds 
of components according
 to a set of grammatical rules: expressions and statements.
 
 The add and insert commands support an optional location specifier, which is
-either a 'handle' of an existing rule or an 'index' (starting at zero).
+either a 'handle' or the 'index' (starting at zero) of an existing rule.
 Internally, rule locations are always identified by 'handle' and the 
translation
 from 'index' happens in userspace. This has two potential implications in case 
a
 concurrent ruleset change happens after the translation was done: The effective
 rule index might change if a rule was inserted or deleted before the referred
 one. If the referred rule was deleted, the command is rejected by the kernel
 just as if an invalid 'handle' was given.
-The old name "position" in place of "handle" is deprecated
-and should not be used anymore.
 
 A 'comment' is a single word or a double-quoted (") multi-word string which can
 be used to make notes regarding the actual rule. *Note:* If you use bash for
-- 
2.19.0

[iptables PATCH 2/3] xtables: Clarify error message when deleting by index

[Phil Sutter]

Trying to delete a rule by index from a non-existent chain leads to a
somewhat confusing error message:

| # iptables-nft -D foobar 1
| iptables: Index of deletion too big.

Fix this by performing chain existence checks for CMD_DELETE_NUM, too.

Signed-off-by: Phil Sutter 
---
 iptables/xtables.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/iptables/xtables.c b/iptables/xtables.c
index 429bd652cc439..24a6e234bcf4b 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -1040,6 +1040,7 @@ void do_parse(struct nft_handle *h, int argc, char 
*argv[],
 
if (p->command == CMD_APPEND ||
p->command == CMD_DELETE ||
+   p->command == CMD_DELETE_NUM ||
p->command == CMD_CHECK ||
p->command == CMD_INSERT ||
p->command == CMD_REPLACE) {
-- 
2.19.0

[iptables PATCH 0/3] A few minor fixes

[Phil Sutter]

The first two deal with incorrect/unexpected error messages, only the
last one fixes a "real" issue.

Phil Sutter (3):
  xtables: Fix typo in do_command() error message
  xtables: Clarify error message when deleting by index
  xtables: Fix error return code in nft_chain_user_rename()

 iptables/nft.c  | 4 ++--
 iptables/tests/shell/testcases/iptables/0004-return-codes_0 | 4 
 iptables/xtables.c  | 3 ++-
 3 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.19.0

[iptables PATCH 3/3] xtables: Fix error return code in nft_chain_user_rename()

[Phil Sutter]

If the chain to rename wasn't found, the function would return -1 which
got interpreted as success.

Signed-off-by: Phil Sutter 
---
 iptables/nft.c  | 4 ++--
 iptables/tests/shell/testcases/iptables/0004-return-codes_0 | 4 
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index e2a4902448680..28b08ce8e7bdd 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1755,14 +1755,14 @@ int nft_chain_user_rename(struct nft_handle *h,const 
char *chain,
c = nft_chain_find(h, table, chain);
if (c == NULL) {
errno = ENOENT;
-   return -1;
+   return 0;
}
handle = nftnl_chain_get_u64(c, NFTNL_CHAIN_HANDLE);
 
/* Now prepare the new name for the chain */
c = nftnl_chain_alloc();
if (c == NULL)
-   return -1;
+   return 0;
 
nftnl_chain_set(c, NFTNL_CHAIN_TABLE, (char *)table);
nftnl_chain_set(c, NFTNL_CHAIN_NAME, (char *)newname);
diff --git a/iptables/tests/shell/testcases/iptables/0004-return-codes_0 
b/iptables/tests/shell/testcases/iptables/0004-return-codes_0
index 34dffeee4604a..5b6e1f6f1bc7a 100755
--- a/iptables/tests/shell/testcases/iptables/0004-return-codes_0
+++ b/iptables/tests/shell/testcases/iptables/0004-return-codes_0
@@ -23,6 +23,10 @@ cmd 1 iptables -N foo
 # iptables-nft allows this - bug or feature?
 #cmd 2 iptables -N "invalid name"
 
+# test chain rename
+cmd 0 iptables -E foo bar
+cmd 1 iptables -E foo bar
+
 # test rule adding
 cmd 0 iptables -A INPUT -j ACCEPT
 cmd 1 iptables -A noexist -j ACCEPT
-- 
2.19.0

[iptables PATCH 1/3] xtables: Fix typo in do_command() error message

[Phil Sutter]

This checks p->chain for existence, not cs->jumpto. Fixes this bogus
error message:

| # iptables-nft -t nat -A FORWARD -j ACCEPT
| iptables v1.8.1 (nf_tables): Chain 'ACCEPT' does not exist

Fixes: b6a06c1a215f8 ("xtables: Align return codes with legacy iptables")
Signed-off-by: Phil Sutter 
---
 iptables/xtables.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/iptables/xtables.c b/iptables/xtables.c
index 0038804e288c6..429bd652cc439 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -1065,7 +1065,7 @@ void do_parse(struct nft_handle *h, int argc, char 
*argv[],
 
if (!p->xlate && !nft_chain_exists(h, p->table, p->chain))
xtables_error(OTHER_PROBLEM,
- "Chain '%s' does not exist", cs->jumpto);
+ "Chain '%s' does not exist", p->chain);
 
if (!p->xlate && !cs->target && strlen(cs->jumpto) > 0 &&
!nft_chain_exists(h, p->table, cs->jumpto))
-- 
2.19.0

[nft PATCH] doc: Fix for make distcheck

[Phil Sutter]

When building from a separate build directory, a2x did not find the
source file nft.txt. Using '$<' instead fixes this.

Fixes: 3bacae9e4a1e3 ("doc: Review man page building in Makefile.am")
Signed-off-by: Phil Sutter 
---
 doc/Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/Makefile.am b/doc/Makefile.am
index 503d6cd80051c..01e1af90bbf0c 100644
--- a/doc/Makefile.am
+++ b/doc/Makefile.am
@@ -14,7 +14,7 @@ ASCIIDOC_INCLUDES = \
 ASCIIDOCS = ${ASCIIDOC_MAIN} ${ASCIIDOC_INCLUDES}
 
 nft.8: ${ASCIIDOCS}
-   ${AM_V_GEN}${A2X} ${A2X_OPTS_MANPAGE} nft.txt
+   ${AM_V_GEN}${A2X} ${A2X_OPTS_MANPAGE} $<
 
 .adoc.3:
${AM_V_GEN}${A2X} ${A2X_OPTS_MANPAGE} $<
-- 
2.19.0

[iptables PATCH v2] xtables: Fix for matching rules with wildcard interfaces

[Phil Sutter]

Due to xtables_parse_interface() and parse_ifname() being misaligned
regarding interface mask setting, rules containing a wildcard interface
added with iptables-nft could neither be checked nor deleted.

As suggested, introduce extensions/iptables.t to hold checks for
built-in selectors. This file is picked up by iptables-test.py as-is.
The only limitation is that iptables is being used for it, so no
ip6tables-specific things can be tested with it (for now).

Signed-off-by: Phil Sutter 
---
Changes since v1:
- Introduce extensions/iptables.t instead of (yet another) script in
  iptables/tests.
---
 extensions/iptables.t | 4 
 iptables/nft-shared.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)
 create mode 100644 extensions/iptables.t

diff --git a/extensions/iptables.t b/extensions/iptables.t
new file mode 100644
index 0..65456ee9874d7
--- /dev/null
+++ b/extensions/iptables.t
@@ -0,0 +1,4 @@
+:FORWARD
+-i alongifacename0;=;OK
+-i thisinterfaceistoolong0;;FAIL
+-i eth+ -o alongifacename+;=;OK
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 492e4ec124a79..7b8ca5e4becaf 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -249,7 +249,7 @@ static void parse_ifname(const char *name, unsigned int 
len, char *dst, unsigned
return;
dst[len++] = 0;
if (mask)
-   memset(mask, 0xff, len + 1);
+   memset(mask, 0xff, len - 2);
 }
 
 int parse_meta(struct nftnl_expr *e, uint8_t key, char *iniface,
-- 
2.19.0

Re: [PATCH nft] json: fix json_events_cb() declaration when libjansson is not present

[Phil Sutter]

Hey Laura,

On Wed, Oct 31, 2018 at 12:54:18PM +0100, Laura Garcia Liebana wrote:
> When nftables is configured without libjansson support, the following
> compilation error is shown:
> 
> monitor.c: In function ‘netlink_echo_callback’:
> monitor.c:910:10: error: too many arguments to function ‘json_events_cb’
>return json_events_cb(nlh, _monh);
>   ^~
> 
> This patch makes a declaration of the json_events_cb() function
> consistent.
> 
> Fixes: bb32d8db9a12 ("JSON: Add support for echo option")
> 
> Signed-off-by: Laura Garcia Liebana 

Oops, thanks for catching this!

Cheers, Phil

[nft PATCH] py: Adjust Nftables class to output flags changes

[Phil Sutter]

Introduce setter/getter methods for each introduced output flag. Ignore
NFT_CTX_OUTPUT_NUMERIC_ALL for now since it's main purpose is for
internal use.

Adjust the script in tests/py accordingly: Due to the good defaults,
only numeric proto output has to be selected - this is not a must, but
allows for the test cases to remain unchanged.

Signed-off-by: Phil Sutter 
---
 py/nftables.py   | 220 ---
 tests/py/nft-test.py |  29 +++---
 2 files changed, 153 insertions(+), 96 deletions(-)

diff --git a/py/nftables.py b/py/nftables.py
index d85bbb2ffeeda..6891cb1ce177b 100644
--- a/py/nftables.py
+++ b/py/nftables.py
@@ -33,11 +33,17 @@ class Nftables:
 "segtree":   0x40,
 }
 
-numeric_levels = {
-"none": 0,
-"addr": 1,
-"port": 2,
-"all":  3,
+output_flags = {
+"reversedns": (1 << 0),
+"service":(1 << 1),
+"stateless":  (1 << 2),
+"handle": (1 << 3),
+"json":   (1 << 4),
+"echo":   (1 << 5),
+"guid":   (1 << 6),
+"numeric_proto":  (1 << 7),
+"numeric_prio":   (1 << 8),
+"numeric_symbol": (1 << 9),
 }
 
 def __init__(self, sofile="libnftables.so"):
@@ -58,40 +64,12 @@ class Nftables:
 self.nft_ctx_new.restype = c_void_p
 self.nft_ctx_new.argtypes = [c_int]
 
-self.nft_ctx_output_get_handle = lib.nft_ctx_output_get_handle
-self.nft_ctx_output_get_handle.restype = c_bool
-self.nft_ctx_output_get_handle.argtypes = [c_void_p]
+self.nft_ctx_output_get_flags = lib.nft_ctx_output_get_flags
+self.nft_ctx_output_get_flags.restype = c_uint
+self.nft_ctx_output_get_flags.argtypes = [c_void_p]
 
-self.nft_ctx_output_set_handle = lib.nft_ctx_output_set_handle
-self.nft_ctx_output_set_handle.argtypes = [c_void_p, c_bool]
-
-self.nft_ctx_output_get_echo = lib.nft_ctx_output_get_echo
-self.nft_ctx_output_get_echo.restype = c_bool
-self.nft_ctx_output_get_echo.argtypes = [c_void_p]
-
-self.nft_ctx_output_set_echo = lib.nft_ctx_output_set_echo
-self.nft_ctx_output_set_echo.argtypes = [c_void_p, c_bool]
-
-self.nft_ctx_output_get_numeric = lib.nft_ctx_output_get_numeric
-self.nft_ctx_output_get_numeric.restype = c_int
-self.nft_ctx_output_get_numeric.argtypes = [c_void_p]
-
-self.nft_ctx_output_set_numeric = lib.nft_ctx_output_set_numeric
-self.nft_ctx_output_set_numeric.argtypes = [c_void_p, c_int]
-
-self.nft_ctx_output_get_stateless = lib.nft_ctx_output_get_stateless
-self.nft_ctx_output_get_stateless.restype = c_bool
-self.nft_ctx_output_get_stateless.argtypes = [c_void_p]
-
-self.nft_ctx_output_set_stateless = lib.nft_ctx_output_set_stateless
-self.nft_ctx_output_set_stateless.argtypes = [c_void_p, c_bool]
-
-self.nft_ctx_output_get_json = lib.nft_ctx_output_get_json
-self.nft_ctx_output_get_json.restype = c_bool
-self.nft_ctx_output_get_json.argtypes = [c_void_p]
-
-self.nft_ctx_output_set_json = lib.nft_ctx_output_set_json
-self.nft_ctx_output_set_json.argtypes = [c_void_p, c_bool]
+self.nft_ctx_output_set_flags = lib.nft_ctx_output_set_flags
+self.nft_ctx_output_set_flags.argtypes = [c_void_p, c_uint]
 
 self.nft_ctx_output_get_debug = lib.nft_ctx_output_get_debug
 self.nft_ctx_output_get_debug.restype = c_int
@@ -128,12 +106,77 @@ class Nftables:
 self.nft_ctx_buffer_output(self.__ctx)
 self.nft_ctx_buffer_error(self.__ctx)
 
+def __get_output_flag(self, name):
+flag = self.output_flags[name]
+return self.nft_ctx_output_get_flags(self.__ctx) & flag
+
+def __set_output_flag(self, name, val):
+flag = self.output_flags[name]
+flags = self.nft_ctx_output_get_flags(self.__ctx)
+if val:
+new_flags = flags | flag
+else:
+new_flags = flags & ~flag
+self.nft_ctx_output_set_flags(self.__ctx, new_flags)
+return flags & flag
+
+def get_reversedns_output(self):
+"""Get the current state of reverse DNS output.
+
+Returns a boolean indicating whether reverse DNS lookups are performed
+for IP addresses in output.
+"""
+return self.__get_output_flag("reversedns")
+
+def set_reversedns_output(self, val):
+"""Enable or disable reverse DNS output.
+
+Accepts a boolean turning reverse DNS lookups in output on or off.
+
+Returns the previous value.
+"""

Re: [iptables PATCH] xtables: Fix for matching rules with wildcard interfaces

[Phil Sutter]

Hi Pablo,

On Tue, Oct 30, 2018 at 06:01:19PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Oct 30, 2018 at 05:57:53PM +0100, Phil Sutter wrote:
> > Due to xtables_parse_interface() and parse_ifname() being misaligned
> > regarding interface mask setting, rules containing a wildcard interface
> > added with iptables-nft could neither be checked nor deleted.
> > 
> > Signed-off-by: Phil Sutter 
> > ---
> >  iptables/nft-shared.c|  2 +-
> >  .../shell/testcases/nft-only/0004wildcard-iface_0| 12 
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> >  create mode 100755 
> > iptables/tests/shell/testcases/nft-only/0004wildcard-iface_0
> > 
> > diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
> > index 492e4ec124a79..7b8ca5e4becaf 100644
> > --- a/iptables/nft-shared.c
> > +++ b/iptables/nft-shared.c
> > @@ -249,7 +249,7 @@ static void parse_ifname(const char *name, unsigned int 
> > len, char *dst, unsigned
> > return;
> > dst[len++] = 0;
> > if (mask)
> > -   memset(mask, 0xff, len + 1);
> > +   memset(mask, 0xff, len - 2);
> >  }
> >  
> >  int parse_meta(struct nftnl_expr *e, uint8_t key, char *iniface,
> > diff --git a/iptables/tests/shell/testcases/nft-only/0004wildcard-iface_0 
> > b/iptables/tests/shell/testcases/nft-only/0004wildcard-iface_0
> > new file mode 100755
> > index 0..b7c398ecbb29c
> > --- /dev/null
> > +++ b/iptables/tests/shell/testcases/nft-only/0004wildcard-iface_0
> > @@ -0,0 +1,12 @@
> > +#!/bin/bash
> > +
> > +# Make sure rules containing wildcard interfaces are found again.
> > +
> > +set -e
> > +
> > +[[ $XT_MULTI == */xtables-nft-multi ]] || { echo "skip $XT_MULTI"; exit 0; 
> > }
> > +
> > +lname='alongifacename+'
> > +$XT_MULTI iptables -A FORWARD -i eth+ -o $lname -j ACCEPT
> > +$XT_MULTI iptables -C FORWARD -i eth+ -o $lname -j ACCEPT
> > +$XT_MULTI iptables -D FORWARD -i eth+ -o $lname -j ACCEPT
> 
> Suggestion: Probably we can catch this through tests/py/, just a
> suggestion. -C and -D operations, very much look the same from
> interface perspective, so just checking for -I then -D should be fine
> as tests/py.

Yes, testing for -C and -D is kind of redundant, though shouldn't matter
much as it's just one more command. What do you mean with tests/py?
There's no such thing in iptables repository? :)

Thanks, Phil

[iptables PATCH] xtables: Fix for matching rules with wildcard interfaces

[Phil Sutter]

Due to xtables_parse_interface() and parse_ifname() being misaligned
regarding interface mask setting, rules containing a wildcard interface
added with iptables-nft could neither be checked nor deleted.

Signed-off-by: Phil Sutter 
---
 iptables/nft-shared.c|  2 +-
 .../shell/testcases/nft-only/0004wildcard-iface_0| 12 
 2 files changed, 13 insertions(+), 1 deletion(-)
 create mode 100755 iptables/tests/shell/testcases/nft-only/0004wildcard-iface_0

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 492e4ec124a79..7b8ca5e4becaf 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -249,7 +249,7 @@ static void parse_ifname(const char *name, unsigned int 
len, char *dst, unsigned
return;
dst[len++] = 0;
if (mask)
-   memset(mask, 0xff, len + 1);
+   memset(mask, 0xff, len - 2);
 }
 
 int parse_meta(struct nftnl_expr *e, uint8_t key, char *iniface,
diff --git a/iptables/tests/shell/testcases/nft-only/0004wildcard-iface_0 
b/iptables/tests/shell/testcases/nft-only/0004wildcard-iface_0
new file mode 100755
index 0..b7c398ecbb29c
--- /dev/null
+++ b/iptables/tests/shell/testcases/nft-only/0004wildcard-iface_0
@@ -0,0 +1,12 @@
+#!/bin/bash
+
+# Make sure rules containing wildcard interfaces are found again.
+
+set -e
+
+[[ $XT_MULTI == */xtables-nft-multi ]] || { echo "skip $XT_MULTI"; exit 0; }
+
+lname='alongifacename+'
+$XT_MULTI iptables -A FORWARD -i eth+ -o $lname -j ACCEPT
+$XT_MULTI iptables -C FORWARD -i eth+ -o $lname -j ACCEPT
+$XT_MULTI iptables -D FORWARD -i eth+ -o $lname -j ACCEPT
-- 
2.19.0

Re: [PATCH 3/3 nft,v2] expression: always print range expression numerically

[Phil Sutter]

On Mon, Oct 29, 2018 at 09:58:00PM +0100, Pablo Neira Ayuso wrote:
> Otherwise we end up displaying things that we cannot parse as input.
> Moreover, in a range, it's relevant to the user the values that are
> enclosed in the range, so let's print this numerically.
> 
> Fixes: baa4e0e3fa5f ("src: add NFT_CTX_OUTPUT_NUMERIC_PROTO")
> Reported-by: Phil Sutter 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

Thanks a lot!

Re: [PATCH 1/3 nft,v3] src: get rid of nft_ctx_output_{get,set}_numeric()

[Phil Sutter]

On Mon, Oct 29, 2018 at 09:57:58PM +0100, Pablo Neira Ayuso wrote:
> This patch adds NFT_CTX_OUTPUT_NUMERIC_SYMBOL, which replaces the last
> client of the numeric level approach.
> 
> This patch updates `-n' option semantics to display all output
> numerically.
> 
> Note that monitor code was still using the -n option to skip printing
> the process name, this patch updates that path too to print it
> inconditionally to simplify things.
> 
> Given the numeric levels have no more clients after this patch, remove
> that code.
> 
> Update several tests/shell not to use -nn.
> 
> This patch adds NFT_CTX_OUTPUT_NUMERIC_ALL which enables all flags to
> provide a fully numerical output.
> 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

Re: [PATCH nft] src: get rid of nft_ctx_output_{get,set}_numeric()

[Phil Sutter]

On Mon, Oct 29, 2018 at 06:31:10PM +0100, Pablo Neira Ayuso wrote:
> @Phil, thinking here we could probably get rid of
> NFT_CTX_OUTPUT_NUMERIC_PROTO, since it is contained already in
> NFT_CTX_OUTPUT_NUMERIC_SYMBOL.
> 
> There's no option for -p anymore, so we could simply things a bit
> before.

I think it's feasible to provide cmdline options for each bit '-n'
enables. Not only because this gives users control, it also simplifies
documentation a bit, like:

| -a - numeric a
| -b - numeric b
| -n - enable all numeric options from above

:)

[...]
> > diff --git a/include/nftables/libnftables.h b/include/nftables/libnftables.h
> > index fb81edc0df07..82b4181de0cc 100644
> > --- a/include/nftables/libnftables.h
> > +++ b/include/nftables/libnftables.h
[...]
> > @@ -54,13 +47,12 @@ enum {
> > NFT_CTX_OUTPUT_GUID = (1 << 6),
> > NFT_CTX_OUTPUT_NUMERIC_PROTO= (1 << 7),
> > NFT_CTX_OUTPUT_NUMERIC_PRIO = (1 << 8),
> > +   NFT_CTX_OUTPUT_NUMERIC_SYMBOL   = (1 << 9),
> >  };

+ #define NFT_CTX_OUTPUT_NUMERIC_ALL (NFT_CTX_OUTPUT_NUMERIC_PROTO |
+ NFT_CTX_OUTPUT_NUMERIC_PRIO |
+ NFT_CTX_OUTPUT_NUMERIC_SYMBOL)

[...]

> > diff --git a/src/main.c b/src/main.c
> > index 883261fc9d8b..952ce356490f 100644
> > --- a/src/main.c
> > +++ b/src/main.c
[...]
> > @@ -229,14 +226,10 @@ int main(int argc, char * const *argv)
> > }
> > break;
> > case OPT_NUMERIC:
> > -   numeric = nft_ctx_output_get_numeric(nft);
> > -   if (numeric == NFT_NUMERIC_ALL) {
> > -   fprintf(stderr, "Too many numeric options "
> > -   "used, max. %u\n",
> > -   NFT_NUMERIC_ALL);
> > -   exit(EXIT_FAILURE);
> > -   }
> > -   nft_ctx_output_set_numeric(nft, numeric + 1);
> > +   numeric = true;
> > +   output_flags |= (NFT_CTX_OUTPUT_NUMERIC_PROTO |
> > +NFT_CTX_OUTPUT_NUMERIC_PRIO |
> > +NFT_CTX_OUTPUT_NUMERIC_SYMBOL);

+ output_flags |= NFT_CTX_OUTPUT_NUMERIC_ALL;

> > @@ -298,6 +291,13 @@ int main(int argc, char * const *argv)
> > }
> > }
> >  
> > +   if (numeric &&
> > +   (output_flags &
> > +   (NFT_CTX_OUTPUT_REVERSEDNS |
> > +NFT_CTX_OUTPUT_SERVICE |
> > +NFT_CTX_OUTPUT_GUID)))
> > +   fprintf(stderr, "cannot combine `-n' with `-N', `-S', '-u'\n");
> > +

Why not? We could allow people to call 'nft -n -N' to make all numeric
but enable reverse DNS (IIRC). All we need to do is document that '-n'
is an alias for '-p -q ...' and that later flags overwrite earlier ones
(as usual with getopt()-based tools). Would simplify code a bit, too.

> > diff --git a/src/monitor.c b/src/monitor.c
> > index b2267e1f63e4..0e735ed5b1aa 100644
> > --- a/src/monitor.c
> > +++ b/src/monitor.c
> > @@ -835,11 +835,9 @@ static int netlink_events_newgen_cb(const struct 
> > nlmsghdr *nlh, int type,
> > }
> > if (genid >= 0) {
> > nft_mon_print(monh, "# new generation %d", genid);
> > -   if (pid >= 0) {
> > -   nft_mon_print(monh, " by process %d", pid);
> > -   if (!monh->ctx->nft->output.numeric)
> > -   nft_mon_print(monh, " (%s)", name);
> > -   }
> > +   if (pid >= 0)
> > +   nft_mon_print(monh, " by process %d (%s)", pid, name);
> > +
> > nft_mon_print(monh, "\n");
> > }
> >  

Yes, I think that's OK. People may still do 's/ (.*$//' if they don't
want it there.

Cheers, Phil

Re: [PATCH nft 1/2,v2] src: add NFT_CTX_OUTPUT_NUMERIC_PROTO

[Phil Sutter]

On Mon, Oct 29, 2018 at 03:31:37PM +0100, Pablo Neira Ayuso wrote:
> We keep printing layer 4 protocols as literals since we do not use
> /etc/protocols. This new flag allows us to print it as a number.
> 
> libnftables internally uses this to print layer 4 protocol as numbers
> when part of a range.
> 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

Re: [PATCH nft 2/2,v2] src: add -y to priority base chain nummerically

[Phil Sutter]

On Mon, Oct 29, 2018 at 03:31:38PM +0100, Pablo Neira Ayuso wrote:
> By default base chains are printed using default hook priority
> definitions. Add -y option to print them as numbers.
> 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

Re: [nft PATCH] JSON: Add support for echo option

[Phil Sutter]

Hi,

On Mon, Oct 29, 2018 at 04:19:03PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Oct 26, 2018 at 03:01:38PM +0200, Phil Sutter wrote:
> > The basic principle is to not return a JSON object freshly created from
> > netlink responses, but just update the existing user-provided one to
> > make sure callers get back exactly what they expect.
> 
> Applied, thanks Phil.
> 
> > To achieve that, keep the parsed JSON object around in a global variable
> > ('cur_root') and provide a custom callback to insert handles into it
> > from received netlink messages. The tricky bit here is updating rules
> > since unique identification is problematic. Therefore drop possibly
> > present handles from input and later assume updates are received in
> > order so the first rule not having a handle set is the right one.
> 
> You can use the netlink sequence number to correlate the rule that you
> are adding with the object that the kernel gives us. The kernel will
> reply including the sequence number of the original request in the
> reply you get. Like this, you will not need obj_info_matches() and
> such I think. In case this helps you simplify this code.

That's an interesting idea, but I don't think it would fly: netlink
seqnum is per batch and potentially there are multiple add commands
contained in it. So although I could make sure the kernel's reply
matches the current request, I still have to figure out which JSON add
command to update. Or am I missing something?

Thanks, Phil

Re: [nft PATCH] tests/shell: Add testcase for cache update problems

[Phil Sutter]

On Mon, Oct 29, 2018 at 04:20:52PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Oct 26, 2018 at 11:42:05AM +0200, Phil Sutter wrote:
> > The first test in there shows how the current cache update strategy
> > causes trouble. The second test shows that proposed "locking" of cache
> > when local entries are added is flawed, too.
> 
> Applied, thanks.
> 
> BTW, is your recent patch to lock the cache solving all caching issues?

Nope, part two of the testcase I submitted here exposes why it's flawed,
too. :(

Re: [PATCH nft] src: add -p to print layer 4 protocol numerically

[Phil Sutter]

Hi,

On Mon, Oct 29, 2018 at 02:10:27PM +0100, Pablo Neira Ayuso wrote:
> We keep printing layer 4 protocols as literals since we do not use
> /etc/protocols. Add -p option to print layer 4 protocols numerically.
> 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

One question:

[...]
> diff --git a/src/datatype.c b/src/datatype.c
> index 48eaca277757..2e957e60bb71 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -564,7 +564,7 @@ static void inet_protocol_type_print(const struct expr 
> *expr,
>  {
>   struct protoent *p;
>  
> - if (octx->numeric < NFT_NUMERIC_ALL) {
> + if (!nft_output_numeric_protocol(octx)) {
>   p = getprotobynumber(mpz_get_uint8(expr->value));
>   if (p != NULL) {
>   nft_print(octx, "%s", p->p_name);

In range_expression_print(), we did:

| octx->numeric += NFT_NUMERIC_ALL + 1

to avoid confusion with names containing dashes. I see that now the same
function just removes NFT_CTX_OUTPUT_SERVICE bit instead. Is that
sufficient? I guess users could still turn on reverse DNS while listing
interval sets with IP addresses, right?

Cheers, Phil

Re: [PATCH nft 4/5,v3] src: add nft_ctx_output_{get,set}_json() to nft_ctx_output_{get,set}_flags

[Phil Sutter]

On Mon, Oct 29, 2018 at 01:48:49PM +0100, Pablo Neira Ayuso wrote:
> Add NFT_CTX_OUTPUT_JSON flag and display output in json format.
> 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

Re: [PATCH nft 4/5,v2] src: add nft_ctx_output_{get,set}_json() to nft_ctx_output_{get,set}_flags

[Phil Sutter]

Hi,

On Mon, Oct 29, 2018 at 01:43:00PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Oct 29, 2018 at 01:29:32PM +0100, Phil Sutter wrote:
> > On Mon, Oct 29, 2018 at 12:33:39PM +0100, Pablo Neira Ayuso wrote:
[...]
> > > diff --git a/src/libnftables.c b/src/libnftables.c
> > > index 6dc1be3d5ef8..ff7a53d22ba4 100644
> > > --- a/src/libnftables.c
> > > +++ b/src/libnftables.c
> > > @@ -352,22 +352,6 @@ void nft_ctx_output_set_echo(struct nft_ctx *ctx, 
> > > bool val)
> > >   ctx->output.echo = val;
> > >  }
> > >  
> > > -bool nft_ctx_output_get_json(struct nft_ctx *ctx)
> > > -{
> > > -#ifdef HAVE_LIBJANSSON
> > > - return ctx->output.json;
> > > -#else
> > > - return false;
> > > -#endif
> > > -}
> > > -
> > > -void nft_ctx_output_set_json(struct nft_ctx *ctx, bool val)
> > > -{
> > > -#ifdef HAVE_LIBJANSSON
> > > - ctx->output.json = val;
> > > -#endif
> > > -}
> > > -
> > 
> > In above functions, I guarded output.json setting by whether JSON
> > support was built-in.
> 
> I'm going to do the same here but...
> 
> > [...]
> > > diff --git a/src/main.c b/src/main.c
> > > index 97b8746608a7..8ea07641734d 100644
> > > --- a/src/main.c
> > > +++ b/src/main.c
> > > @@ -271,7 +271,7 @@ int main(int argc, char * const *argv)
> > >   nft_ctx_output_set_echo(nft, true);
> > >   break;
> > >   case OPT_JSON:
> > > - nft_ctx_output_set_json(nft, true);
> > > + output_flags |= NFT_CTX_OUTPUT_JSON;
> > >   break;
> > >   case OPT_INVALID:
> > >   exit(EXIT_FAILURE);
> > 
> > Maybe we should do the same here? Otherwise if JSON wasn't enabled at
> > compile-time, calling 'nft -j' leads to no output at all. (Not sure if
> > silently falling back to standard output formatting is a better choice
> > after all. :)
> 
> ... I think failing here is json support is not built-in is the way to
> go - instead of silently ignoring it.
> 
> Would you mind send a follow up patch to change this behaviour? :-)

Will do!

Thanks, Phil

Re: [PATCH nft,v3 2/5] src: add nft_ctx_output_{get,set}_stateless() to nft_ctx_output_{get,flags}_flags

[Phil Sutter]

Hi,

On Mon, Oct 29, 2018 at 01:31:00PM +0100, Pablo Neira Ayuso wrote:
[...]
> @@ -1354,10 +1354,11 @@ json_t *objref_stmt_json(const struct stmt *stmt, 
> struct output_ctx *octx)
>  json_t *meter_stmt_json(const struct stmt *stmt, struct output_ctx *octx)
>  {
>   json_t *root, *tmp;
> + unsigned int flags;
>  
> - octx->stateless++;
> + nft_output_save_flags(octx, , NFT_CTX_OUTPUT_STATELESS);
>   tmp = stmt_print_json(stmt->meter.stmt, octx);
> - octx->stateless--;
> + nft_output_restore_flags(octx, flags);
>  
>   root = json_pack("{s:o, s:o, s:i}",
>"key", expr_print_json(stmt->meter.key, octx),

No objections, though I don't think this is any more clear than:

|  json_t *meter_stmt_json(const struct stmt *stmt, struct output_ctx *octx)
|  {
|   json_t *root, *tmp;
| + unsigned int flags = octx->flags;
|  
| - octx->stateless++;
| + octx->flags |= NFT_CTX_OUTPUT_STATELESS;
|   tmp = stmt_print_json(stmt->meter.stmt, octx);
| - octx->stateless--;
| + octx->flags = flags;

Cheers, Phil

Re: [PATCH] src: default to numeric UID and GID listing

[Phil Sutter]

On Mon, Oct 29, 2018 at 01:07:28PM +0100, Pablo Neira Ayuso wrote:
> Like iptables-save, print UID and GID as numeric values by default.
> 
> Add a new option `-u' to print the UID and GID names as defined by
> /etc/passwd and /etc/group.
> 
> Note that -n is ignored after this patch, since default are numeric
> printing for UID and GID.
> 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

Re: [PATCH nft 5/5,v2] src: add nft_ctx_output_{get,set}_echo() to nft_ctx_output_{get,set}_flags

[Phil Sutter]

On Mon, Oct 29, 2018 at 12:33:40PM +0100, Pablo Neira Ayuso wrote:
> Add NFT_CTX_OUTPUT_ECHO flag and echo the command that has been send to
> the kernel.
> 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

Re: [PATCH nft 4/5,v2] src: add nft_ctx_output_{get,set}_json() to nft_ctx_output_{get,set}_flags

[Phil Sutter]

Hi,

On Mon, Oct 29, 2018 at 12:33:39PM +0100, Pablo Neira Ayuso wrote:
> Add NFT_CTX_OUTPUT_JSON flag and display output in json format.
> 
> Signed-off-by: Pablo Neira Ayuso 
[...]
> diff --git a/doc/libnftables.adoc b/doc/libnftables.adoc
> index 8b7aee9af134..5a3562c3266c 100644
> --- a/doc/libnftables.adoc
> +++ b/doc/libnftables.adoc
[...]
> @@ -105,6 +103,8 @@ NFT_CTX_OUTPUT_STATELESS::
>   If stateless output has been requested then stateful data is not 
> printed. Stateful data refers to those objects that carry run-time data, eg. 
> the *counter* statement holds packet and byte counter values, making it 
> stateful.
>  NFT_CTX_OUTPUT_HANDLE::
>   Upon insertion into the ruleset, some elements are assigned a unique 
> handle for identification purposes. For example, when deleting a table or 
> chain, it may be identified either by name or handle. Rules on the other hand 
> must be deleted by handle because there is no other way to uniquely identify 
> them. These functions allow to control whether ruleset listings should 
> include handles or not.
> +NFT_CTX_OUTPUT_JSON::
> + If enabled at compile-time, libnftables accepts input in JSON format 
> and is able to print output in JSON format as well. See *libnftables-json*(5) 
> for a description of the supported schema. These functions control JSON 
> output format, input is auto-detected.

s/These functions control/This flag controls/

[...]
> diff --git a/src/libnftables.c b/src/libnftables.c
> index 6dc1be3d5ef8..ff7a53d22ba4 100644
> --- a/src/libnftables.c
> +++ b/src/libnftables.c
> @@ -352,22 +352,6 @@ void nft_ctx_output_set_echo(struct nft_ctx *ctx, bool 
> val)
>   ctx->output.echo = val;
>  }
>  
> -bool nft_ctx_output_get_json(struct nft_ctx *ctx)
> -{
> -#ifdef HAVE_LIBJANSSON
> - return ctx->output.json;
> -#else
> - return false;
> -#endif
> -}
> -
> -void nft_ctx_output_set_json(struct nft_ctx *ctx, bool val)
> -{
> -#ifdef HAVE_LIBJANSSON
> - ctx->output.json = val;
> -#endif
> -}
> -

In above functions, I guarded output.json setting by whether JSON
support was built-in.

[...]
> diff --git a/src/main.c b/src/main.c
> index 97b8746608a7..8ea07641734d 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -271,7 +271,7 @@ int main(int argc, char * const *argv)
>   nft_ctx_output_set_echo(nft, true);
>   break;
>   case OPT_JSON:
> - nft_ctx_output_set_json(nft, true);
> + output_flags |= NFT_CTX_OUTPUT_JSON;
>   break;
>   case OPT_INVALID:
>   exit(EXIT_FAILURE);

Maybe we should do the same here? Otherwise if JSON wasn't enabled at
compile-time, calling 'nft -j' leads to no output at all. (Not sure if
silently falling back to standard output formatting is a better choice
after all. :)

Thanks, Phil

Re: [PATCH nft 3/5,v2] src: add nft_ctx_output_{get,set}_handle() to nft_ctx_output_{get,set}_flags

[Phil Sutter]

On Mon, Oct 29, 2018 at 12:33:38PM +0100, Pablo Neira Ayuso wrote:
> Add NFT_CTX_OUTPUT_HANDLE flag and print handle that uniquely identify
> objects from new output flags interface.
> 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

One minor nit here as well:

[...]
> diff --git a/doc/libnftables.adoc b/doc/libnftables.adoc
> index c837c2d251bc..8b7aee9af134 100644
> --- a/doc/libnftables.adoc
> +++ b/doc/libnftables.adoc
[...]
> @@ -105,6 +103,8 @@ NFT_CTX_OUTPUT_SERVICE::
>   Print port numbers as services as described in the /etc/services file.
>  NFT_CTX_OUTPUT_STATELESS::
>   If stateless output has been requested then stateful data is not 
> printed. Stateful data refers to those objects that carry run-time data, eg. 
> the *counter* statement holds packet and byte counter values, making it 
> stateful.
> +NFT_CTX_OUTPUT_HANDLE::
> + Upon insertion into the ruleset, some elements are assigned a unique 
> handle for identification purposes. For example, when deleting a table or 
> chain, it may be identified either by name or handle. Rules on the other hand 
> must be deleted by handle because there is no other way to uniquely identify 
> them. These functions allow to control whether ruleset listings should 
> include handles or not.

s/These functions allow/This flag allows/

(Or maybe rewrite the whole last sentence to read: "This flag makes
ruleset listings include handle values.")

One thing to consider given the very long line above: When writing
documentation for man pages, I've usually adhered to 80 column max
width. Though searching for asciidoc style guides I encountered an
interesting alternative approach, namely to keep each sentence in a
separate line. The rationale was to clear up diffs since no reformatting
would happen and on the other hand to easily identify overlong sentences
one might want to break into two. I don't have a strong opinion here,
just wanted to share an interesting idea.

Thanks, Phil

Re: [PATCH nft 2/5,v2] src: add nft_ctx_output_{get,set}_stateless() to nft_ctx_output_{get,flags}_flags

[Phil Sutter]

Hi,

On Mon, Oct 29, 2018 at 12:33:37PM +0100, Pablo Neira Ayuso wrote:
> Add NFT_CTX_OUTPUT_STATELESS flag and enable stateless printing from new
> output flags interface.
> 
> Signed-off-by: Pablo Neira Ayuso 
> ---
> v2: Add nft_output_stateless()
> Fix missing conversion to use NFT_CTX_OUTPUT_STATELESS.
> Remove stateless field from struct output_ctx.

[...]

> diff --git a/src/statement.c b/src/statement.c
> index e50ac706402d..162922108020 100644
> --- a/src/statement.c
> +++ b/src/statement.c
> @@ -121,9 +121,9 @@ static void meter_stmt_print(const struct stmt *stmt, 
> struct output_ctx *octx)
>   expr_print(stmt->meter.key, octx);
>   nft_print(octx, " ");
>  
> - octx->stateless++;
> + octx->flags |= NFT_CTX_OUTPUT_STATELESS;
>   stmt_print(stmt->meter.stmt, octx);
> - octx->stateless--;
> + octx->flags &= ~NFT_CTX_OUTPUT_STATELESS;
>  
>   nft_print(octx, "} ");
>  

Are you sure this is safe? If meter_stmt_print() is called with
stateless output enabled, it will be disabled when the function returns.
I guess this should backup octx->flags and restore the old value before
returning to caller.

Same goes for other places were we do 'stateless++; something();
stateless--'.

Thanks, Phil

Re: [PATCH nft 1/5,v4] src: Revert --literal, add -S/--service

[Phil Sutter]

Hi,

On Mon, Oct 29, 2018 at 12:33:36PM +0100, Pablo Neira Ayuso wrote:
> This is a partial revert of b0f6a45b25dd1 ("src: add --literal option")
> which was added during the development cycle before 0.9.1 is released.
> 
> After looking at patch: https://patchwork.ozlabs.org/patch/969864/ that
> allows to print priority, uid, gid and protocols as numerics, I decided
> to revisit this to provide individual options to turn on literal
> printing.
> 
> What I'm proposing is to provide a good default for everyone, and
> provide options to turn on literal/numeric printing.
> 
> This patch adds nft_ctx_output_{set,get}_flags() and define two flags to
> enable reverse DNS lookups and to print ports as service names.
> 
> This patch introduces -S/--services, to print service names as per
> /etc/services.
> 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

Just one minor nit:

[...]
> diff --git a/doc/libnftables.adoc b/doc/libnftables.adoc
> index 0387652fa3c1..1c6ea0152d13 100644
> --- a/doc/libnftables.adoc
> +++ b/doc/libnftables.adoc
[...]
> @@ -91,6 +91,25 @@ The *nft_ctx_get_dry_run*() function returns the dry-run 
> setting's value contain
>  
>  The *nft_ctx_set_dry_run*() function sets the dry-run setting in 'ctx' to 
> the value of 'dry'.
>  
> +=== nft_ctx_output_get_flags() and nft_ctx_output_set_flags()
> +The flags setting controls the output format.
> +
> +
> +enum {
> +NFT_CTX_OUTPUT_REVERSEDNS  = (1 << 0),
> +NFT_CTX_OUTPUT_SERVICE = (1 << 1),
> +};
> +
> +
> +NFT_CTX_OUTPUT_REVERSEDNS::
> + Perform reverse DNS lookups are performed for IP addresses when 
> printing. Note that this may add significant delay to *list* commands 
> depending on DNS resolver speed.

s/Perform reverse/Reverse/

Thanks, Phil

[nft PATCH] nft.8: Document log level audit

[Phil Sutter]

Since this pseudo log level fundamentally changes behaviour of log
statement, dedicate this mode a separate paragraph.

Signed-off-by: Phil Sutter 
---
 doc/statements.txt | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/doc/statements.txt b/doc/statements.txt
index 39d9f14436171..51dd0b371f921 100644
--- a/doc/statements.txt
+++ b/doc/statements.txt
@@ -64,16 +64,26 @@ LOG STATEMENT
 [verse]
 *log* [prefix 'quoted_string'] [level 'syslog-level'] [flags 'log-flags']
 *log* group 'nflog_group' [prefix 'quoted_string'] [queue-threshold 'value'] 
[snaplen 'size']
+*log* level audit
 
 The log statement enables logging of matching packets. When this statement is
 used from a rule, the Linux kernel will print some information on all matching
 packets, such as header fields, via the kernel log (where it can be read with
-dmesg(1) or read in the syslog). If the group number is specified, the Linux
+dmesg(1) or read in the syslog).
+
+In the second form of invocation (if 'nflog_group' is specified), the Linux
 kernel will pass the packet to nfnetlink_log which will multicast the packet
 through a netlink socket to the specified multicast group. One or more 
userspace
 processes may subscribe to the group to receive the packets, see
-libnetfilter_queue documentation for details. This is a non-terminating
-statement, so the rule evaluation continues after the packet is logged.
+libnetfilter_queue documentation for details.
+
+In the third form of invocation (if level audit is specified), the Linux
+kernel writes a message into the audit buffer suitably formatted for reading
+with auditd. Therefore no further formatting options (such as prefix or flags)
+are allowed in this mode.
+
+This is a non-terminating statement, so the rule evaluation continues after
+the packet is logged.
 
 .log statement options
 [options="header"]
@@ -84,7 +94,7 @@ Log message prefix|
 quoted string
 |level|
 Syslog level of logging |
-string: emerg, alert, crit, err, warn [default], notice, info, debug
+string: emerg, alert, crit, err, warn [default], notice, info, debug, audit
 |group|
 NFLOG group to send messages to|
 unsigned integer (16 bit)
-- 
2.19.0

[nft PATCH] JSON: Add support for echo option

[Phil Sutter]

The basic principle is to not return a JSON object freshly created from
netlink responses, but just update the existing user-provided one to
make sure callers get back exactly what they expect.

To achieve that, keep the parsed JSON object around in a global variable
('cur_root') and provide a custom callback to insert handles into it
from received netlink messages. The tricky bit here is updating rules
since unique identification is problematic. Therefore drop possibly
present handles from input and later assume updates are received in
order so the first rule not having a handle set is the right one.

Signed-off-by: Phil Sutter 
---
 include/json.h  |  15 ++
 include/netlink.h   |   6 +
 include/nftables.h  |   1 +
 src/libnftables.c   |   4 +
 src/monitor.c   |  13 +-
 src/parser_json.c   | 280 ++--
 tests/json_echo/run-test.py | 211 +++
 7 files changed, 512 insertions(+), 18 deletions(-)
 create mode 100755 tests/json_echo/run-test.py

diff --git a/include/json.h b/include/json.h
index d2dc92d963693..8d45c3c32f136 100644
--- a/include/json.h
+++ b/include/json.h
@@ -7,6 +7,7 @@ struct chain;
 struct cmd;
 struct expr;
 struct netlink_ctx;
+struct nlmsghdr;
 struct rule;
 struct set;
 struct obj;
@@ -103,6 +104,10 @@ void monitor_print_obj_json(struct netlink_mon_handler 
*monh,
 void monitor_print_rule_json(struct netlink_mon_handler *monh,
 const char *cmd, struct rule *r);
 
+int json_events_cb(const struct nlmsghdr *nlh,
+  struct netlink_mon_handler *monh);
+void json_print_echo(struct nft_ctx *ctx);
+
 #else /* ! HAVE_LIBJANSSON */
 
 typedef void json_t;
@@ -234,6 +239,16 @@ static inline void monitor_print_rule_json(struct 
netlink_mon_handler *monh,
/* empty */
 }
 
+static inline int json_events_cb(const struct nlmsghdr *nlh)
+{
+   return -1;
+}
+
+static inline void json_print_echo(struct nft_ctx *ctx)
+{
+   /* empty */
+}
+
 #endif /* HAVE_LIBJANSSON */
 
 #endif /* NFTABLES_JSON_H */
diff --git a/include/netlink.h b/include/netlink.h
index 5ff129ed92298..a8528d5983a86 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -55,6 +55,12 @@ struct netlink_ctx {
 extern struct nftnl_expr *alloc_nft_expr(const char *name);
 extern void alloc_setelem_cache(const struct expr *set, struct nftnl_set *nls);
 
+extern struct nftnl_table *netlink_table_alloc(const struct nlmsghdr *nlh);
+extern struct nftnl_chain *netlink_chain_alloc(const struct nlmsghdr *nlh);
+extern struct nftnl_set *netlink_set_alloc(const struct nlmsghdr *nlh);
+extern struct nftnl_obj *netlink_obj_alloc(const struct nlmsghdr *nlh);
+extern struct nftnl_rule *netlink_rule_alloc(const struct nlmsghdr *nlh);
+
 struct nft_data_linearize {
uint32_tlen;
uint32_tvalue[4];
diff --git a/include/nftables.h b/include/nftables.h
index 25e78c80df7e0..1009e266752fd 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -53,6 +53,7 @@ struct nft_ctx {
uint32_tflags;
struct parser_state *state;
void*scanner;
+   void*json_root;
 };
 
 enum nftables_exit_codes {
diff --git a/src/libnftables.c b/src/libnftables.c
index e892083f8818c..2f67bb341aef6 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -467,6 +467,8 @@ err:
}
free(nlbuf);
 
+   if (!rc && nft->output.json && nft->output.echo)
+   json_print_echo(nft);
return rc;
 }
 
@@ -506,6 +508,8 @@ err:
nft->scanner = NULL;
}
 
+   if (!rc && nft->output.json && nft->output.echo)
+   json_print_echo(nft);
return rc;
 }
 
diff --git a/src/monitor.c b/src/monitor.c
index 14ccbc5fe04ca..88a61de4ed9f7 100644
--- a/src/monitor.c
+++ b/src/monitor.c
@@ -42,7 +42,7 @@
 
 #define nft_mon_print(monh, ...) nft_print(>ctx->nft->output, 
__VA_ARGS__)
 
-static struct nftnl_table *netlink_table_alloc(const struct nlmsghdr *nlh)
+struct nftnl_table *netlink_table_alloc(const struct nlmsghdr *nlh)
 {
struct nftnl_table *nlt;
 
@@ -55,7 +55,7 @@ static struct nftnl_table *netlink_table_alloc(const struct 
nlmsghdr *nlh)
return nlt;
 }
 
-static struct nftnl_chain *netlink_chain_alloc(const struct nlmsghdr *nlh)
+struct nftnl_chain *netlink_chain_alloc(const struct nlmsghdr *nlh)
 {
struct nftnl_chain *nlc;
 
@@ -68,7 +68,7 @@ static struct nftnl_chain *netlink_chain_alloc(const struct 
nlmsghdr *nlh)
return nlc;
 }
 
-static struct nftnl_set *netlink_set_alloc(const struct nlmsghdr *nlh)
+struct nftnl_set *netlink_set_alloc(const struct nlmsghdr *nlh)
 {
struct nftnl_set *nls;
 
@@ -94,7 +94,7 @@ static struct nftnl_set *netlink_setelem_alloc(const struct 
nlmsghdr *nlh)
return nls;
 }
 
-static struct nftnl_rule

[nft PATCH] tests/shell: Add testcase for cache update problems

[Phil Sutter]

The first test in there shows how the current cache update strategy
causes trouble. The second test shows that proposed "locking" of cache
when local entries are added is flawed, too.

Signed-off-by: Phil Sutter 
---
 .../shell/testcases/cache/0003_cache_update_0 | 29 +++
 1 file changed, 29 insertions(+)
 create mode 100755 tests/shell/testcases/cache/0003_cache_update_0

diff --git a/tests/shell/testcases/cache/0003_cache_update_0 
b/tests/shell/testcases/cache/0003_cache_update_0
new file mode 100755
index 0..deb45db2c43be
--- /dev/null
+++ b/tests/shell/testcases/cache/0003_cache_update_0
@@ -0,0 +1,29 @@
+#!/bin/bash
+
+set -e
+
+# Expose how naive cache update logic (i.e., drop cache and repopulate from
+# kernel ruleset) may mess things up. The following input does:
+#
+# list ruleset -> populate the cache, cache->genid is non-zero
+# add table ip t -> make kernel's genid increment (cache->genid remains
+#   unchanged)
+# add table ip t2; -> first command of batch, new table t2 is added to the 
cache
+# add chain ip t2 c -> second command of batch, triggers cache_update() which
+#  removes table t2 from it
+
+$NFT -i >/dev/null < cache would be locked without previous update
+# add chain ip t c -> table t is not found due to no cache update happening
+
+$NFT -i >/dev/null <

[nft PATCH] json: Work around segfault when encountering xt stmt

[Phil Sutter]

When trying to convert an xt stmt into JSON, print() callback was
called. Though the code in src/xt.c does not respect output_fp,
therefore buffer wasn't filled as expected making libjansson to puke:

| # nft -j list ruleset
| warning: stmt ops xt have no json callback
| nft: json.c:169: stmt_print_json: Assertion `__out' failed.
| Aborted (core dumped)

Avoid this by detecting xt stmt ops and returning a stub.

Signed-off-by: Phil Sutter 
---
 doc/libnftables-json.adoc | 7 +++
 src/json.c| 5 +
 2 files changed, 12 insertions(+)

diff --git a/doc/libnftables-json.adoc b/doc/libnftables-json.adoc
index ea5fbe818302f..414702c7b88a4 100644
--- a/doc/libnftables-json.adoc
+++ b/doc/libnftables-json.adoc
@@ -998,6 +998,13 @@ Assign connection tracking timeout policy.
 *ct timeout*::
CT timeout reference.
 
+=== XT
+[verse]
+*{ "xt": null }*
+
+This represents an xt statement from xtables compat interface. Sadly, at this
+point it is not possible to provide any further information about its content.
+
 == EXPRESSIONS
 Expressions are the building blocks of (most) statements. In their most basic
 form, they are just immediate values represented as JSON string, integer or
diff --git a/src/json.c b/src/json.c
index 431d38afc24ba..64735ebce4902 100644
--- a/src/json.c
+++ b/src/json.c
@@ -166,6 +166,11 @@ static json_t *stmt_print_json(const struct stmt *stmt, 
struct output_ctx *octx)
char buf[1024];
FILE *fp;
 
+   /* XXX: Can't be supported at this point:
+* xt_stmt_xlate() ignores output_fp. */
+   if (stmt->ops->type == STMT_XT)
+   return json_pack("{s:n}", "xt");
+
if (stmt->ops->json)
return stmt->ops->json(stmt, octx);
 
-- 
2.19.0

Re: [nft PATCH] mnl: Improve error checking in mnl_nft_event_listener()

[Phil Sutter]

Hi Pablo,

On Wed, Oct 24, 2018 at 06:35:45PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Oct 24, 2018 at 06:05:55PM +0200, Phil Sutter wrote:
> > When trying to adjust receive buffer size, the second call to
> > setsockopt() was not error-checked.
> > 
> > Signed-off-by: Phil Sutter 
> > ---
> >  src/mnl.c | 7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/mnl.c b/src/mnl.c
> > index 2be8ca14e50da..0d9b7ffc85c76 100644
> > --- a/src/mnl.c
> > +++ b/src/mnl.c
> > @@ -1425,8 +1425,11 @@ int mnl_nft_event_listener(struct mnl_socket 
> > *nf_sock, unsigned int debug_mask,
> >  */
> > ret = setsockopt(fd, SOL_SOCKET, SO_RCVBUF, ,
> >  sizeof(socklen_t));
> > -   nft_print(octx, "# Cannot set up netlink socket buffer size to 
> > %u bytes, falling back to %u bytes\n",
> > - NFTABLES_NLEVENT_BUFSIZ, bufsiz);
> > +   if (ret < 0)
> > +   nft_print(octx, "# Cannot increase netlink socket 
> > buffer size, expect message loss\n");
> > +   else
> > +   nft_print(octx, "# Cannot set up netlink socket buffer 
> > size to %u bytes, falling back to %u bytes\n",
> > + NFTABLES_NLEVENT_BUFSIZ, bufsiz);
> 
> Looks good.
> 
> Are you hitting this error message? With a large ruleset?

No, this originated from a covscan report complaining about the unused
assignment of 'ret' variable. Instead of eliminating the assignment, I
decided to make use of it instead.

Cheers, Phil

[nft PATCH] mnl: Improve error checking in mnl_nft_event_listener()

[Phil Sutter]

When trying to adjust receive buffer size, the second call to
setsockopt() was not error-checked.

Signed-off-by: Phil Sutter 
---
 src/mnl.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/mnl.c b/src/mnl.c
index 2be8ca14e50da..0d9b7ffc85c76 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -1425,8 +1425,11 @@ int mnl_nft_event_listener(struct mnl_socket *nf_sock, 
unsigned int debug_mask,
 */
ret = setsockopt(fd, SOL_SOCKET, SO_RCVBUF, ,
 sizeof(socklen_t));
-   nft_print(octx, "# Cannot set up netlink socket buffer size to 
%u bytes, falling back to %u bytes\n",
- NFTABLES_NLEVENT_BUFSIZ, bufsiz);
+   if (ret < 0)
+   nft_print(octx, "# Cannot increase netlink socket 
buffer size, expect message loss\n");
+   else
+   nft_print(octx, "# Cannot set up netlink socket buffer 
size to %u bytes, falling back to %u bytes\n",
+ NFTABLES_NLEVENT_BUFSIZ, bufsiz);
}
 
while (1) {
-- 
2.19.0

[nft PATCH] evaluate: Convert ranges of N-N to N

[Phil Sutter]

Trying to add a range of size 1 was previously not allowed:

| # nft add element ip t s '{ 40-40 }'
| Error: Range has zero or negative size
| add element ip t s { 40-40 }
|  ^

The error message is not correct: A range of N-K with K >= N consists of
K - N + 1 elements (N, N + 1, N + 2, ... K - 1, K). Therefore a range of
N-N consists of 1 (N - N + 1) elements, namely N.

Allow this in a simple way by reducing the range into a single element:

| # nft list set ip t s
| table ip t {
|   set s {
|   type inet_service
|   }
| }
| # nft add element ip t s '{ 40-40 }'
| # nft list set ip t s
| table ip t {
|   set s {
|   type inet_service
|   elements = { 40 }
|   }
| }
| # nft get element ip t s '{ 40-40 }'
| table ip t {
|   set s {
|   type inet_service
|   flags interval
|   elements = { 40 }
|   }
| }

Signed-off-by: Phil Sutter 
---
 src/evaluate.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 66e9293fd4ca8..1a5bdd38c4f10 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -858,6 +858,7 @@ static int expr_evaluate_range_expr(struct eval_ctx *ctx,
 static int expr_evaluate_range(struct eval_ctx *ctx, struct expr **expr)
 {
struct expr *range = *expr, *left, *right;
+   int diff;
 
if (expr_evaluate_range_expr(ctx, range, >left) < 0)
return -1;
@@ -867,9 +868,16 @@ static int expr_evaluate_range(struct eval_ctx *ctx, 
struct expr **expr)
return -1;
right = range->right;
 
-   if (mpz_cmp(left->value, right->value) >= 0)
+   diff = mpz_cmp(left->value, right->value);
+   if (diff > 0) {
return expr_error(ctx->msgs, range,
- "Range has zero or negative size");
+ "Range has negative size");
+   } else if (diff == 0) {
+   range->left = NULL;
+   expr_free(range);
+   *expr = left;
+   return expr_evaluate(ctx, expr);
+   }
 
range->dtype = left->dtype;
range->flags |= EXPR_F_CONSTANT;
-- 
2.19.0

[nft PATCH 2/3] json: Fix osf ttl support

[Phil Sutter]

Having to use numerical values for ttl property in JSON is not
practical as these values are arbitrary and meaningful only in
netfilter. Instead align JSON output/input with standard API, accepting
names for TTL matching strategy.

Also add missing documentation in libnftables-json man page and fix JSON
equivalent in tests/py.

Fixes: 03eafe098d5ee ("osf: add ttl option support")
Signed-off-by: Phil Sutter 
---
 doc/libnftables-json.adoc | 24 
 src/json.c| 13 -
 src/parser_json.c | 19 +++
 tests/py/inet/osf.t.json  | 35 ---
 4 files changed, 83 insertions(+), 8 deletions(-)

diff --git a/doc/libnftables-json.adoc b/doc/libnftables-json.adoc
index 98303b357f10c..ea5fbe818302f 100644
--- a/doc/libnftables-json.adoc
+++ b/doc/libnftables-json.adoc
@@ -1288,3 +1288,27 @@ 
 
 
 Construct a reference to packet's socket.
+
+=== OSF
+[verse]
+
+*{ "osf": {
+   "key":* 'OSF_KEY'*,
+   "ttl":* 'OSF_TTL'
+*}}*
+
+'OSF_KEY' := *"name"*
+'OSF_TTL' := *"loose"* | *"skip"*
+
+
+Perform OS fingerprinting. This expression is typically used in LHS of a 
*match*
+statement.
+
+*key*::
+   What part of the fingerprint info to match against. At this point, only
+   the OS name is supported.
+*ttl*::
+   Define how packet's TTL value is to be matched. This property is
+   optional. If omitted, TTL value has to match exactly. A value of *loose*
+   accepts TTL values less than the fingerprint one. A value of *skip*
+   omits TTL value comparison entirely.
diff --git a/src/json.c b/src/json.c
index cea9f19cd9982..d21536afd714e 100644
--- a/src/json.c
+++ b/src/json.c
@@ -857,7 +857,18 @@ json_t *socket_expr_json(const struct expr *expr, struct 
output_ctx *octx)
 
 json_t *osf_expr_json(const struct expr *expr, struct output_ctx *octx)
 {
-   return json_pack("{s:{s:i, s:s}}", "osf", "ttl", expr->osf.ttl, "key", 
"name");
+   json_t *root = json_pack("{s:s}", "key", "name");
+
+   switch (expr->osf.ttl) {
+   case 1:
+   json_object_set_new(root, "ttl", json_string("loose"));
+   break;
+   case 2:
+   json_object_set_new(root, "ttl", json_string("skip"));
+   break;
+   }
+
+   return json_pack("{s:o}", "osf", root);
 }
 
 json_t *xfrm_expr_json(const struct expr *expr, struct output_ctx *octx)
diff --git a/src/parser_json.c b/src/parser_json.c
index fc0dc9a9e4046..46a02fe14de03 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -375,14 +375,25 @@ static struct expr *json_parse_meta_expr(struct json_ctx 
*ctx,
 static struct expr *json_parse_osf_expr(struct json_ctx *ctx,
const char *type, json_t *root)
 {
-   const char *key;
-   uint8_t ttl;
+   const char *key, *ttl;
+   uint8_t ttlval = 0;
 
-   if (json_unpack_err(ctx, root, "{s:i, s:s}", "ttl", ttl,"key", ))
+   if (json_unpack_err(ctx, root, "{s:s}", "key", ))
return NULL;
 
+   if (!json_unpack(root, "{s:s}", "ttl", )) {
+   if (!strcmp(ttl, "loose")) {
+   ttlval = 1;
+   } else if (!strcmp(ttl, "skip")) {
+   ttlval = 2;
+   } else {
+   json_error(ctx, "Invalid osf ttl option '%s'.", ttl);
+   return NULL;
+   }
+   }
+
if (!strcmp(key, "name"))
-   return osf_expr_alloc(int_loc, ttl);
+   return osf_expr_alloc(int_loc, ttlval);
 
json_error(ctx, "Invalid osf key value.");
return NULL;
diff --git a/tests/py/inet/osf.t.json b/tests/py/inet/osf.t.json
index 45335cab30c0b..452f3023585b3 100644
--- a/tests/py/inet/osf.t.json
+++ b/tests/py/inet/osf.t.json
@@ -4,7 +4,6 @@
 "match": {
 "left": {
 "osf": {
-"ttl": 0,
 "key": "name"
 }
 },
@@ -14,13 +13,44 @@
 }
 ]
 
+# osf ttl loose name "Linux"
+[
+{
+"match": {
+"left": {
+"osf": {
+"key": "name",
+"ttl": "loose"
+}
+},
+"op": "==",
+"right": "Linux"
+}
+}
+]
+
+# osf ttl skip name "Linux"
+[
+{
+"match": {
+

[nft PATCH 1/3] include: Fix comment for struct eval_ctx

[Phil Sutter]

Previous change to that struct missed to update the comment.

Fixes: 00f777bfc414a ("src: pass struct nft_ctx through struct eval_ctx")
Signed-off-by: Phil Sutter 
---
 include/rule.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/rule.h b/include/rule.h
index 977f274842ef5..197efab497452 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -587,7 +587,7 @@ extern void cmd_free(struct cmd *cmd);
 /**
  * struct eval_ctx - evaluation context
  *
- * @nf_sock:   netlink socket (for caching)
+ * @nft:   nftables context
  * @msgs:  message queue
  * @cmd:   current command
  * @table: current table
-- 
2.19.0

[nft PATCH 3/3] json: Fix for recent changes to context structs

[Phil Sutter]

Commits introducing nft_ctx pointer to netlink and eval contexts did not
update JSON code accordingly.

Fixes: 00f777bfc414a ("src: pass struct nft_ctx through struct eval_ctx")
Fixes: 2dc07bcd7eaa5 ("src: pass struct nft_ctx through struct netlink_ctx")
Signed-off-by: Phil Sutter 
---
 src/json.c| 69 ---
 src/parser_json.c |  5 +---
 2 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/src/json.c b/src/json.c
index d21536afd714e..431d38afc24ba 100644
--- a/src/json.c
+++ b/src/json.c
@@ -212,8 +212,7 @@ static json_t *rule_print_json(struct output_ctx *octx,
return json_pack("{s:o}", "rule", root);
 }
 
-static json_t *chain_print_json(const struct output_ctx *octx,
-   const struct chain *chain)
+static json_t *chain_print_json(const struct chain *chain)
 {
json_t *root, *tmp;
 
@@ -265,7 +264,7 @@ static json_t *timeout_policy_json(uint8_t l4, const 
uint32_t *timeout)
return root ? : json_null();
 }
 
-static json_t *obj_print_json(struct output_ctx *octx, const struct obj *obj)
+static json_t *obj_print_json(const struct obj *obj)
 {
const char *rate_unit = NULL, *burst_unit = NULL;
const char *type = obj_type_name(obj->type);
@@ -406,8 +405,7 @@ static json_t *table_flags_json(const struct table *table)
return root;
 }
 
-static json_t *table_print_json(const struct output_ctx *octx,
-   const struct table *table)
+static json_t *table_print_json(const struct table *table)
 {
json_t *root, *tmp;
 
@@ -1450,17 +1448,17 @@ static json_t *table_print_json_full(struct netlink_ctx 
*ctx,
struct obj *obj;
struct set *set;
 
-   tmp = table_print_json(ctx->octx, table);
+   tmp = table_print_json(table);
json_array_append_new(root, tmp);
 
list_for_each_entry(obj, >objs, list) {
-   tmp = obj_print_json(ctx->octx, obj);
+   tmp = obj_print_json(obj);
json_array_append_new(root, tmp);
}
list_for_each_entry(set, >sets, list) {
if (set->flags & NFT_SET_ANONYMOUS)
continue;
-   tmp = set_print_json(ctx->octx, set);
+   tmp = set_print_json(>nft->output, set);
json_array_append_new(root, tmp);
}
list_for_each_entry(flowtable, >flowtables, list) {
@@ -1468,11 +1466,11 @@ static json_t *table_print_json_full(struct netlink_ctx 
*ctx,
json_array_append_new(root, tmp);
}
list_for_each_entry(chain, >chains, list) {
-   tmp = chain_print_json(ctx->octx, chain);
+   tmp = chain_print_json(chain);
json_array_append_new(root, tmp);
 
list_for_each_entry(rule, >rules, list) {
-   tmp = rule_print_json(ctx->octx, rule);
+   tmp = rule_print_json(>nft->output, rule);
json_array_append_new(root, tmp);
}
}
@@ -1486,7 +1484,7 @@ static json_t *do_list_ruleset_json(struct netlink_ctx 
*ctx, struct cmd *cmd)
json_t *root = json_array();
struct table *table;
 
-   list_for_each_entry(table, >cache->list, list) {
+   list_for_each_entry(table, >nft->cache.list, list) {
if (family != NFPROTO_UNSPEC &&
table->handle.family != family)
continue;
@@ -1503,12 +1501,12 @@ static json_t *do_list_tables_json(struct netlink_ctx 
*ctx, struct cmd *cmd)
json_t *root = json_array();
struct table *table;
 
-   list_for_each_entry(table, >cache->list, list) {
+   list_for_each_entry(table, >nft->cache.list, list) {
if (family != NFPROTO_UNSPEC &&
table->handle.family != family)
continue;
 
-   json_array_append_new(root, table_print_json(ctx->octx, table));
+   json_array_append_new(root, table_print_json(table));
}
 
return root;
@@ -1532,10 +1530,10 @@ static json_t *do_list_chain_json(struct netlink_ctx 
*ctx,
strcmp(cmd->handle.chain.name, chain->handle.chain.name))
continue;
 
-   json_array_append_new(root, chain_print_json(ctx->octx, chain));
+   json_array_append_new(root, chain_print_json(chain));
 
list_for_each_entry(rule, >rules, list) {
-   json_t *tmp = rule_print_json(ctx->octx, rule);
+   json_t *tmp = rule_print_json(>nft->output, rule);
 
json_array_append_new(root, tmp);
}
@@ -1550,13 +1548,13 @@ static json_t *do_list_chains_json(struct netlink_ctx 
*ctx, struct cmd 

[nft PATCH 0/3] Fix JSON API after recent other changes

[Phil Sutter]

Recent changes to context structs broke compiling with JSON support
enabled, patch 3 fixes this. While doing so, I noticed that struct
eval_ctx's comment wasn't updated - fixed by patch 1.

Finally, I didn't like how osf ttl support was implemented in JSON.
Patch 2 resolves this.

Phil Sutter (3):
  include: Fix comment for struct eval_ctx
  json: Fix osf ttl support
  json: Fix for recent changes to context structs

 doc/libnftables-json.adoc | 24 
 include/rule.h|  2 +-
 src/json.c| 82 +++
 src/parser_json.c | 24 
 tests/py/inet/osf.t.json  | 35 +++--
 5 files changed, 121 insertions(+), 46 deletions(-)

-- 
2.19.0

[iptables PATCH] xtables: Fix for spurious errors from iptables-translate

[Phil Sutter]

When aligning iptables-nft error messages with legacy ones, I missed
that translate tools shouldn't check for missing or duplicated chains.

Introduce a boolean in struct nft_xt_cmd_parse indicating we're "just"
translating and do_parse() should skip the checks.

Fixes: b6a06c1a215f8 ("xtables: Align return codes with legacy iptables")
Signed-off-by: Phil Sutter 
---
 iptables/nft-shared.h| 1 +
 iptables/xtables-translate.c | 1 +
 iptables/xtables.c   | 6 +++---
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index 1281f080bc31d..e3ecdb4d23df3 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -233,6 +233,7 @@ struct nft_xt_cmd_parse {
const char  *policy;
boolrestore;
int verbose;
+   boolxlate;
 };
 
 void do_parse(struct nft_handle *h, int argc, char *argv[],
diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
index f4c0f9cf5a181..849c53f30e155 100644
--- a/iptables/xtables-translate.c
+++ b/iptables/xtables-translate.c
@@ -216,6 +216,7 @@ static int do_command_xlate(struct nft_handle *h, int argc, 
char *argv[],
struct nft_xt_cmd_parse p = {
.table  = *table,
.restore= restore,
+   .xlate  = true,
};
struct iptables_command_state cs;
struct xtables_args args = {
diff --git a/iptables/xtables.c b/iptables/xtables.c
index e0343dbabf2b3..0038804e288c6 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -1063,16 +1063,16 @@ void do_parse(struct nft_handle *h, int argc, char 
*argv[],
   p->chain);
}
 
-   if (!nft_chain_exists(h, p->table, p->chain))
+   if (!p->xlate && !nft_chain_exists(h, p->table, p->chain))
xtables_error(OTHER_PROBLEM,
  "Chain '%s' does not exist", cs->jumpto);
 
-   if (!cs->target && strlen(cs->jumpto) > 0 &&
+   if (!p->xlate && !cs->target && strlen(cs->jumpto) > 0 &&
!nft_chain_exists(h, p->table, cs->jumpto))
xtables_error(PARAMETER_PROBLEM,
  "Chain '%s' does not exist", cs->jumpto);
}
-   if (p->command == CMD_NEW_CHAIN &&
+   if (!p->xlate && p->command == CMD_NEW_CHAIN &&
nft_chain_exists(h, p->table, p->chain))
xtables_error(OTHER_PROBLEM, "Chain already exists");
 }
-- 
2.19.0

Re: [nft PATCH] tests: shell: Extend get element test

[Phil Sutter]

Hi,

On Tue, Oct 23, 2018 at 11:28:28AM +0200, Pablo Neira Ayuso wrote:
> On Mon, Oct 22, 2018 at 11:14:32PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Mon, Oct 22, 2018 at 09:45:02PM +0200, Pablo Neira Ayuso wrote:
> > [...]
> > > > A bit of context illustrating why I think the code needs more than just
> > > > "more fixes": AFAIU, for each input element (which may be part of a
> > > > range or not), code asks the kernel for whether the element exists, then
> > > > get_set_decompose() is called to find the corresponding range. This
> > > > approach has a critical problem though: Given a set with elements 10 and
> > > > 20-30, asking for 10 and 20 will return the same two elements as asking
> > > > for 10-20 does. Though in the first case, we should return 10 and 20-30
> > > > while in the latter case we should return nothing.
> > > 
> > > With this patch:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git/commit/?id=3b18d5eba491b2328b31efa4235724a2354af010
> > > 
> > > I'm going to send a pull request for David now, I guess you are
> > > missing this kernel fix, so the _END interval flag is included when
> > > searching for the right hand side of a matching interval.
> > 
> > Ah! I wondered already why the test still fails although you had applied
> > a bunch of fixes. An additional kernel fix didn't come to mind. :)
> > 
> > > With this kernel patch plus your extended test reports I get this.
> > > 
> > > # ./run-tests.sh testcases/sets/0034get_element_0 
> > > I: using nft binary ./../../src/nft
> > > 
> > > W: [FAILED] testcases/sets/0034get_element_0: expected 0 but got 1
> > > ERROR: asked for '22-24, 26-28', expecting '20-30' but got '20-30, 20-30'
> > > 
> > > I: results: [OK] 0 [FAILED] 1 [TOTAL] 1
> > > 
> > > So, before applying your patch, I'm going to mangle it with this patch
> > > below. If we ask for 22-24 and 26-28, the result in 20-30 and 20-30.
> > > The command returns what is the interval container for 22-24 is 20-30,
> > > same thing for 26-28 which is 20-30.
> > 
> > Well, in theory, I'm fine with this. The expected outcome of 'get
> > element' command is a matter of definition, as long as it doesn't return
> > things which don't exist in the set. But I think the above is
> > inconsistent with regards to single element lookups:
> > 
> > | # nft list set ip t s
> > | table ip t {
> > |   set s {
> > |   type inet_service
> > |   flags interval
> > |   elements = { 10, 20-30, 40, 50-60 }
> > |   }
> > | }
> > | # nft get element ip t s '{ 25, 28 }'
> > | table ip t {
> > |   set s {
> > |   type inet_service
> > |   flags interval
> > |   elements = { 20-30 }
> > |   }
> > | }
> 
> Using current nftables git HEAD plus kernel patch, I'm getting:
> 
> # nft get element ip t s '{ 25, 28 }'
> table ip t {
> set s {
> type inet_service
> flags interval
> elements = { 20-30, 20-30 }
> }
> }

Oh, sorry. I did above test using sources which eliminate duplicates
from the return set (something I was playing with).

> > Here, although I ask for two elements, a single range is returned. So I
> > guess asking for two ranges belonging to the same range should return a
> > single range as well. Maybe a "simple" fix would be to drop duplicates
> > from the return set?
> > 
> > Anyway, from my point of view your solution is fine as well: If a
> > humanoid parser evaluates the results, it can easily spot duplicates and
> > interpret them right. If 'get element' command is used by a script to
> > check for existence of given ranges, it all boils down to a boolean
> > found or not found result, so duplicates shouldn't matter that much.
> 
> My idea was to provide list including exactly the same number of
> elements that have been requested, and in strict order, so you can
> quickly check what you request belongs to somewhere, eg.
> 
> # nft get element ip t s '{ 50, 20, 10 }'
> table ip t {
> set s {
> type inet_service
> flags interval
> elements = { 50-60, 20-30, 10 }
> }
> }
> 

Yes, that makes sense. Thanks for explaining your approach to how 'get
element' command should work!

> Not yet implemented, but we could add something like:
> 
> # nft get element ip t 

Re: [nft PATCH] tests: shell: Extend get element test

[Phil Sutter]

Hi Pablo,

On Mon, Oct 22, 2018 at 09:45:02PM +0200, Pablo Neira Ayuso wrote:
[...]
> > A bit of context illustrating why I think the code needs more than just
> > "more fixes": AFAIU, for each input element (which may be part of a
> > range or not), code asks the kernel for whether the element exists, then
> > get_set_decompose() is called to find the corresponding range. This
> > approach has a critical problem though: Given a set with elements 10 and
> > 20-30, asking for 10 and 20 will return the same two elements as asking
> > for 10-20 does. Though in the first case, we should return 10 and 20-30
> > while in the latter case we should return nothing.
> 
> With this patch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git/commit/?id=3b18d5eba491b2328b31efa4235724a2354af010
> 
> I'm going to send a pull request for David now, I guess you are
> missing this kernel fix, so the _END interval flag is included when
> searching for the right hand side of a matching interval.

Ah! I wondered already why the test still fails although you had applied
a bunch of fixes. An additional kernel fix didn't come to mind. :)

> With this kernel patch plus your extended test reports I get this.
> 
> # ./run-tests.sh testcases/sets/0034get_element_0 
> I: using nft binary ./../../src/nft
> 
> W: [FAILED] testcases/sets/0034get_element_0: expected 0 but got 1
> ERROR: asked for '22-24, 26-28', expecting '20-30' but got '20-30, 20-30'
> 
> I: results: [OK] 0 [FAILED] 1 [TOTAL] 1
> 
> So, before applying your patch, I'm going to mangle it with this patch
> below. If we ask for 22-24 and 26-28, the result in 20-30 and 20-30.
> The command returns what is the interval container for 22-24 is 20-30,
> same thing for 26-28 which is 20-30.

Well, in theory, I'm fine with this. The expected outcome of 'get
element' command is a matter of definition, as long as it doesn't return
things which don't exist in the set. But I think the above is
inconsistent with regards to single element lookups:

| # nft list set ip t s
| table ip t {
|   set s {
|   type inet_service
|   flags interval
|   elements = { 10, 20-30, 40, 50-60 }
|   }
| }
| # nft get element ip t s '{ 25, 28 }'
| table ip t {
|   set s {
|   type inet_service
|   flags interval
|   elements = { 20-30 }
|   }
| }

Here, although I ask for two elements, a single range is returned. So I
guess asking for two ranges belonging to the same range should return a
single range as well. Maybe a "simple" fix would be to drop duplicates
from the return set?

Anyway, from my point of view your solution is fine as well: If a
humanoid parser evaluates the results, it can easily spot duplicates and
interpret them right. If 'get element' command is used by a script to
check for existence of given ranges, it all boils down to a boolean
found or not found result, so duplicates shouldn't matter that much.

Thanks, Phil

[nft PATCH] tests: shell: Extend get element test

[Phil Sutter]

Despite the recent fixes, the test still fails. While trying to address
the remaining issues, I found more potentially problematic inputs so
extend the test by those.

Signed-off-by: Phil Sutter 
---
Hi,

A bit of context illustrating why I think the code needs more than just
"more fixes": AFAIU, for each input element (which may be part of a
range or not), code asks the kernel for whether the element exists, then
get_set_decompose() is called to find the corresponding range. This
approach has a critical problem though: Given a set with elements 10 and
20-30, asking for 10 and 20 will return the same two elements as asking
for 10-20 does. Though in the first case, we should return 10 and 20-30
while in the latter case we should return nothing.
---
 tests/shell/testcases/sets/0034get_element_0 | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/tests/shell/testcases/sets/0034get_element_0 
b/tests/shell/testcases/sets/0034get_element_0
index 2bfb527b8080f..b1f14476d90d6 100755
--- a/tests/shell/testcases/sets/0034get_element_0
+++ b/tests/shell/testcases/sets/0034get_element_0
@@ -27,10 +27,17 @@ check 15-18 ""
 
 # multiple single elements, ranges smaller than present
 check "10, 40" "10, 40"
+check "22-24, 26-28" 20-30
 check 21-29 20-30
 
+# mixed single elements and ranges
+check "10, 20" "10, 20-30"
+check "10, 22" "10, 20-30"
+check "10, 22-24" "10, 20-30"
+
 # non-existing ranges matching elements
 check 10-40 ""
+check 10-20 ""
 check 10-25 ""
 check 25-55 ""
 
-- 
2.19.0

Re: [nft PATCH] make cache persistent if local entries were added

[Phil Sutter]

On Sat, Oct 20, 2018 at 12:35:11PM +0200, Pablo Neira Ayuso wrote:
> On Sat, Oct 20, 2018 at 12:24:06PM +0200, Phil Sutter wrote:
> > JSON API as well as nft CLI allow to run multiple commands within the
> > same batch. Depending on the local cache state, a later command may
> > trigger a cache update which removes the local entry added by an earlier
> > command.
> > 
> > To overcome this, introduce a special genid value to set when local
> > entries are added to the cache which blocks all cache updates until the
> > batch has been committed to the kernel.
> 
> Probably we can make sure we issue a cache_update() by when we call
> chain_add_hash(), before adding the local object to the cache, then
> lock it? Or add assert() to _add_hash() functions to make sure cache
> is up to date? We need a valid cache before we can lock it, right?

The problem is that a batch commit outdates the local cache. An example
showing the problem is:

| % sudo nft -i
| nft> list ruleset
| nft> add table ip t
| nft> add table ip t2; add chain ip t2 c
| Error: Could not process rule: No such file or directory
| add table ip t2; add chain ip t2 c
|   ^^

With 'list ruleset', I just ensure cache->genid is not zero. The first
'add table' command increments kernel's genid. The 'add chain' command
triggers a cache update which removes table t2 from it.

> Do you have several examples that are triggering the problem that we
> can place in the test regression infrastructure?

I'll try to collect a few and will send a test case so we have something
to validate against.

Thanks, Phil

[nft PATCH] make cache persistent if local entries were added

[Phil Sutter]

JSON API as well as nft CLI allow to run multiple commands within the
same batch. Depending on the local cache state, a later command may
trigger a cache update which removes the local entry added by an earlier
command.

To overcome this, introduce a special genid value to set when local
entries are added to the cache which blocks all cache updates until the
batch has been committed to the kernel.

Signed-off-by: Phil Sutter 
---
 include/nftables.h |  2 ++
 include/rule.h | 12 
 src/evaluate.c |  6 +++---
 src/libnftables.c  |  2 ++
 src/monitor.c  |  4 ++--
 src/rule.c | 17 +
 6 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index 25e78c80df7e0..47d2c2bb036cc 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -37,6 +37,8 @@ struct nft_cache {
struct list_headlist;
uint32_tseqnum;
 };
+#define CACHE_LOCK_GENID   (uint16_t)-1
+#define CACHE_UNLOCK_GENID 1
 
 struct mnl_socket;
 struct parser_state;
diff --git a/include/rule.h b/include/rule.h
index 9e029899513fd..d5ddc78a7d3c4 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -216,7 +216,8 @@ extern const char *chain_hookname_lookup(const char *name);
 extern struct chain *chain_alloc(const char *name);
 extern struct chain *chain_get(struct chain *chain);
 extern void chain_free(struct chain *chain);
-extern void chain_add_hash(struct chain *chain, struct table *table);
+extern void chain_add_hash(struct chain *chain, struct table *table,
+  struct nft_cache *cache);
 extern struct chain *chain_lookup(const struct table *table,
  const struct handle *h);
 
@@ -299,7 +300,8 @@ extern struct set *set_alloc(const struct location *loc);
 extern struct set *set_get(struct set *set);
 extern void set_free(struct set *set);
 extern struct set *set_clone(const struct set *set);
-extern void set_add_hash(struct set *set, struct table *table);
+extern void set_add_hash(struct set *set, struct table *table,
+struct nft_cache *cache);
 extern struct set *set_lookup(const struct table *table, const char *name);
 extern struct set *set_lookup_global(uint32_t family, const char *table,
 const char *name, struct nft_cache *cache);
@@ -381,7 +383,8 @@ struct obj {
 struct obj *obj_alloc(const struct location *loc);
 extern struct obj *obj_get(struct obj *obj);
 void obj_free(struct obj *obj);
-void obj_add_hash(struct obj *obj, struct table *table);
+void obj_add_hash(struct obj *obj, struct table *table,
+ struct nft_cache *cache);
 struct obj *obj_lookup(const struct table *table, const char *name,
   uint32_t type);
 void obj_print(const struct obj *n, struct output_ctx *octx);
@@ -406,7 +409,8 @@ struct flowtable {
 extern struct flowtable *flowtable_alloc(const struct location *loc);
 extern struct flowtable *flowtable_get(struct flowtable *flowtable);
 extern void flowtable_free(struct flowtable *flowtable);
-extern void flowtable_add_hash(struct flowtable *flowtable, struct table 
*table);
+extern void flowtable_add_hash(struct flowtable *flowtable,
+  struct table *table, struct nft_cache *cache);
 
 void flowtable_print(const struct flowtable *n, struct output_ctx *octx);
 
diff --git a/src/evaluate.c b/src/evaluate.c
index db49a18d0150c..f2448296be0c0 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3014,7 +3014,7 @@ static int set_evaluate(struct eval_ctx *ctx, struct set 
*set)
ctx->set = NULL;
 
if (set_lookup(table, set->handle.set.name) == NULL)
-   set_add_hash(set_get(set), table);
+   set_add_hash(set_get(set), table, ctx->cache);
 
/* Default timeout value implies timeout support */
if (set->timeout)
@@ -3198,12 +3198,12 @@ static int chain_evaluate(struct eval_ctx *ctx, struct 
chain *chain)
if (chain_lookup(table, >cmd->handle) == NULL) {
chain = chain_alloc(NULL);
handle_merge(>handle, >cmd->handle);
-   chain_add_hash(chain, table);
+   chain_add_hash(chain, table, ctx->cache);
}
return 0;
} else {
if (chain_lookup(table, >handle) == NULL)
-   chain_add_hash(chain_get(chain), table);
+   chain_add_hash(chain_get(chain), table, ctx->cache);
}
 
if (chain->flags & CHAIN_F_BASECHAIN) {
diff --git a/src/libnftables.c b/src/libnftables.c
index 977763793e768..1726b0ee19b54 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -464,6 +464,7 @@ err:
}
erec_print_list(>output, , nft->debug_mask);
iface_cache_release();
+   nft->cache.

Re: [PATCH iptables] iptables-test: add -N option to exercise netns removal path

[Phil Sutter]

Hi,

On Sat, Oct 20, 2018 at 11:21:42AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 19, 2018 at 03:38:58PM +0200, Phil Sutter wrote:
> > On Fri, Oct 19, 2018 at 11:55:07AM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Oct 19, 2018 at 11:04:42AM +0200, Phil Sutter wrote:
> > > > Hi,
> > > > 
> > > > On Thu, Oct 18, 2018 at 08:33:07PM +0200, Pablo Neira Ayuso wrote:
> > > > [...]
> > > > > @@ -108,8 +111,15 @@ def run_test(iptables, rule, rule_save, res, 
> > > > > filename, lineno):
> > > > >  command = IPTABLES_SAVE
> > > > >  elif splitted[0] == IP6TABLES:
> > > > >  command = IP6TABLES_SAVE
> > > > > +
> > > > > +if netns:
> > > > > +path = "/sbin/ip"
> > > > > +command = "netns exec vm-iptable-test " + 
> > > > > EXECUTEABLE + " " + command
> > > > > +else:
> > > > > +path = os.path.abspath(os.path.curdir) + "/iptables/" + 
> > > > > EXECUTEABLE
> > > > > +
> > > > 
> > > > In netns case, doesn't this lead to calling xtables-*-multi from $PATH
> > > > instead of the local one we want to test?
> > > 
> > > Hm, right, will fix this.
> > 
> > I had another look: In main(), PATH is extended to include $PWD/iptables
> > as first component. So actually this shouldn't matter, but maybe better
> > to have it explicit.
> 
> You mean, we could remove lines that are updating PATH and have them
> explicit everywhere, right? If so, that's fine with it.

Yes, that's what I meant.

> I can have a look in a follow up patch, or may this affect this patch
> in some way I'm overlooking?

No, no secret insights I didn't tell you about. :D

Thanks, Phil

Re: [PATCH iptables] libxtables: expose new etherdb lookup function through libxtables API

[Phil Sutter]

On Fri, Oct 19, 2018 at 01:10:59PM +0200, Pablo Neira Ayuso wrote:
> This is used from extensions and included in libxtables, so we have to
> make them public.
> 
> Fixes: 31f1434dfe37 ("libxtables: Integrate getethertype.c from xtables core")
> Reported-by: Florian Westphal 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

Thanks for fixing this up!

Re: [PATCH iptables] libxtables: prefix exported new functions for etherdb lookups

[Phil Sutter]

On Fri, Oct 19, 2018 at 12:57:36PM +0200, Pablo Neira Ayuso wrote:
> To avoid symbol pollution, place them under the xt_ and xtables_ prefix
> name.
> 
> Reported-by: Florian Westphal 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

Re: [PATCH iptables] iptables-test: add -N option to exercise netns removal path

[Phil Sutter]

On Fri, Oct 19, 2018 at 11:55:07AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 19, 2018 at 11:04:42AM +0200, Phil Sutter wrote:
> > Hi,
> > 
> > On Thu, Oct 18, 2018 at 08:33:07PM +0200, Pablo Neira Ayuso wrote:
> > [...]
> > > @@ -108,8 +111,15 @@ def run_test(iptables, rule, rule_save, res, 
> > > filename, lineno):
> > >  command = IPTABLES_SAVE
> > >  elif splitted[0] == IP6TABLES:
> > >  command = IP6TABLES_SAVE
> > > +
> > > +if netns:
> > > +path = "/sbin/ip"
> > > +command = "netns exec vm-iptable-test " + EXECUTEABLE + 
> > > " " + command
> > > +else:
> > > +path = os.path.abspath(os.path.curdir) + "/iptables/" + 
> > > EXECUTEABLE
> > > +
> > 
> > In netns case, doesn't this lead to calling xtables-*-multi from $PATH
> > instead of the local one we want to test?
> 
> Hm, right, will fix this.

I had another look: In main(), PATH is extended to include $PWD/iptables
as first component. So actually this shouldn't matter, but maybe better
to have it explicit.

Cheers, Phil

Re: [PATCH iptables] iptables-test: add -N option to exercise netns removal path

[Phil Sutter]

Hi,

On Thu, Oct 18, 2018 at 08:33:07PM +0200, Pablo Neira Ayuso wrote:
[...]
> @@ -108,8 +111,15 @@ def run_test(iptables, rule, rule_save, res, filename, 
> lineno):
>  command = IPTABLES_SAVE
>  elif splitted[0] == IP6TABLES:
>  command = IP6TABLES_SAVE
> +
> +if netns:
> +path = "/sbin/ip"
> +command = "netns exec vm-iptable-test " + EXECUTEABLE + " " 
> + command
> +else:
> +path = os.path.abspath(os.path.curdir) + "/iptables/" + 
> EXECUTEABLE
> +

In netns case, doesn't this lead to calling xtables-*-multi from $PATH
instead of the local one we want to test?

Cheers, Phil

Re: [PATCH nft] src: remove opts field from struct xt_stmt

[Phil Sutter]

On Tue, Oct 16, 2018 at 08:58:20PM +0200, Pablo Neira Ayuso wrote:
> This is never used, ie. always NULL.
> 
> Reported-by: Phil Sutter 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 

Thanks for clearing this up!

Re: [PATCH libnftables] src: remove json support

[Phil Sutter]

Hey,

On Mon, Oct 15, 2018 at 04:45:38PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Oct 15, 2018 at 02:34:21PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Oct 15, 2018 at 02:08:07PM +0200, Phil Sutter wrote:
> > > On Mon, Oct 15, 2018 at 01:29:52PM +0200, Pablo Neira Ayuso wrote:
> > > > Subject: [PATCH libnftables] src: remove json support
> > >   ~~~
> > > 
> > > This is libnftnl, right? :)
> > > 
> > > Apart from that:
> > > 
> > > Acked-by: Phil Sutter 
> > 
> > Right, thanks!
> 
> Oh, I pushed out this patch without including your Acked-by.
> 
> Sorry about that, it was not intentional.

No problem, this way people won't find me that easily if the change
broke their scripts. ;)

Cheers, Phil

Re: [PATCH libnftables] src: remove json support

[Phil Sutter]

On Mon, Oct 15, 2018 at 01:29:52PM +0200, Pablo Neira Ayuso wrote:
> Subject: [PATCH libnftables] src: remove json support
  ~~~

This is libnftnl, right? :)

Apart from that:

Acked-by: Phil Sutter 

Cheers, Phil

[nft PATCH] xt: Fix for covscan warning in xt_stmt_xlate()

[Phil Sutter]

This does not fix a real issue, target or match field should never be
NULL. Also, I can't find a place where opts field is being assigned to.

Still, covscan sees the NULL check and assumes that if target or match
field is NULL *and* opts field is NULL as well, code ends up
dereferencing the NULL target or match field later on. Avoid this by
splitting the conditional so that later else cases are not hit.

Signed-off-by: Phil Sutter 
---
 src/xt.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/xt.c b/src/xt.c
index 95d0c5f24c07e..1dcd414144a48 100644
--- a/src/xt.c
+++ b/src/xt.c
@@ -32,8 +32,9 @@ void xt_stmt_xlate(const struct stmt *stmt)
 
switch (stmt->xt.type) {
case NFT_XT_MATCH:
-   if (stmt->xt.match == NULL && stmt->xt.opts) {
-   printf("%s", stmt->xt.opts);
+   if (stmt->xt.match == NULL) {
+   if (stmt->xt.opts)
+   printf("%s", stmt->xt.opts);
} else if (stmt->xt.match->xlate) {
struct xt_xlate_mt_params params = {
.ip = stmt->xt.entry,
@@ -51,8 +52,9 @@ void xt_stmt_xlate(const struct stmt *stmt)
break;
case NFT_XT_WATCHER:
case NFT_XT_TARGET:
-   if (stmt->xt.target == NULL && stmt->xt.opts) {
-   printf("%s", stmt->xt.opts);
+   if (stmt->xt.target == NULL) {
+   if (stmt->xt.opts)
+   printf("%s", stmt->xt.opts);
} else if (stmt->xt.target->xlate) {
struct xt_xlate_tg_params params = {
.ip = stmt->xt.entry,
-- 
2.19.0

[nft PATCH] json: Fix memleak in dup_stmt_json()

[Phil Sutter]

The variable 'root' is always assigned to after initialization, so there
is no point in initializing it upon declaration.

Fixes: e70354f53e9f6 ("libnftables: Implement JSON output support")
Signed-off-by: Phil Sutter 
---
 src/json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/json.c b/src/json.c
index f5d97c623fb3a..1ab2d431d0a2d 100644
--- a/src/json.c
+++ b/src/json.c
@@ -1131,7 +1131,7 @@ json_t *notrack_stmt_json(const struct stmt *stmt, struct 
output_ctx *octx)
 
 json_t *dup_stmt_json(const struct stmt *stmt, struct output_ctx *octx)
 {
-   json_t *root = json_object();
+   json_t *root;
 
if (stmt->dup.to) {
root = json_pack("{s:o}", "addr", expr_print_json(stmt->dup.to, 
octx));
-- 
2.19.0

[nft PATCH] parser_json: Fix for ineffective family value checks

[Phil Sutter]

Since handle->family is unsigned, checking for value < 0 never yields
true. Overcome this by changing parse_family() to return an error code
and write the parsed family value into a pointer passed as parameter.

The above change required a bit more cleanup to avoid passing pointers
to signed variables to the function. Also leverage json_parse_family() a
bit more to reduce code side.

Signed-off-by: Phil Sutter 
---
Note that this patch depends on the previously submitted "json: Add ct
timeout support" since the code added by that suffers from the same
problem.
---
 src/parser_json.c | 169 +++---
 1 file changed, 70 insertions(+), 99 deletions(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index 2a67f96774e9d..95933681049dd 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -180,7 +180,7 @@ static int json_unpack_stmt(struct json_ctx *ctx, json_t 
*root,
return 1;
 }
 
-static int parse_family(const char *name)
+static int parse_family(const char *name, uint32_t *family)
 {
unsigned int i;
struct {
@@ -195,13 +195,37 @@ static int parse_family(const char *name)
{ "netdev", NFPROTO_NETDEV }
};
 
+   assert(family);
+
for (i = 0; i < array_size(family_tbl); i++) {
-   if (!strcmp(name, family_tbl[i].name))
-   return family_tbl[i].val;
+   if (strcmp(name, family_tbl[i].name))
+   continue;
+
+   *family = family_tbl[i].val;
+   return 0;
}
return -1;
 }
 
+static int json_parse_family(struct json_ctx *ctx, json_t *root)
+{
+   const char *family;
+
+   if (!json_unpack(root, "{s:s}", "family", )) {
+   uint32_t familyval;
+
+   if (parse_family(family, ) ||
+   (familyval != NFPROTO_IPV6 &&
+familyval != NFPROTO_IPV4)) {
+   json_error(ctx, "Invalid family '%s'.", family);
+   return -1;
+   }
+   return familyval;
+   }
+
+   return NFPROTO_UNSPEC;
+}
+
 static bool is_keyword(const char *keyword)
 {
const char *keywords[] = {
@@ -629,19 +653,15 @@ static struct expr *json_parse_rt_expr(struct json_ctx 
*ctx,
{ "mtu", NFT_RT_TCPMSS },
{ "ipsec", NFT_RT_XFRM },
};
-   unsigned int i, familyval = NFPROTO_UNSPEC;
-   const char *key, *family = NULL;
+   const char *key;
+   unsigned int i;
+   int familyval;
 
if (json_unpack_err(ctx, root, "{s:s}", "key", ))
return NULL;
-   if (!json_unpack(root, "{s:s}", "family", )) {
-   familyval = parse_family(family);
-   if (familyval != NFPROTO_IPV4 &&
-   familyval != NFPROTO_IPV6) {
-   json_error(ctx, "Invalid RT family '%s'.", family);
-   return NULL;
-   }
-   }
+   familyval = json_parse_family(ctx, root);
+   if (familyval < 0)
+   return NULL;
 
for (i = 0; i < array_size(rt_key_tbl); i++) {
int val = rt_key_tbl[i].val;
@@ -685,26 +705,6 @@ static bool ct_key_is_dir(enum nft_ct_keys key)
return false;
 }
 
-static int json_parse_family(struct json_ctx *ctx, json_t *root)
-{
-   const char *family;
-
-   if (!json_unpack(root, "{s:s}", "family", )) {
-   int familyval = parse_family(family);
-
-   switch (familyval) {
-   case NFPROTO_IPV6:
-   case NFPROTO_IPV4:
-   return familyval;
-   default:
-   json_error(ctx, "Invalid family '%s'.", family);
-   return -1;
-   }
-   }
-
-   return NFPROTO_UNSPEC;
-}
-
 static struct expr *json_parse_ct_expr(struct json_ctx *ctx,
   const char *type, json_t *root)
 {
@@ -1674,7 +1674,6 @@ static struct stmt *json_parse_fwd_stmt(struct json_ctx 
*ctx,
const char *key, json_t *value)
 {
json_t *jaddr, *jdev;
-   const char *family;
struct stmt *stmt;
int familyval;
 
@@ -1689,21 +1688,15 @@ static struct stmt *json_parse_fwd_stmt(struct json_ctx 
*ctx,
goto out_err;
}
 
-   if (json_unpack(value, "{s:s, s:o}",
-   "family", , "addr", ))
-   return stmt;
-
-   familyval = parse_family(family);
-   switch (familyval) {
-   case NFPROTO_IPV4:
-   case NFPROTO_IPV6:
-   stmt->fwd.family = familyval;
-   break;
-   default:
-   json_error(ctx, "Invalid f

[nft PATCH] libnftables: Fix memleak in nft_parse_bison_filename()

[Phil Sutter]

Allocated scanner object leaks when returning to caller. For some odd
reason, this was missed by the commit referenced below.

Fixes: bd82e03e15df8 ("libnftables: Move scanner object into struct nft_ctx")
Signed-off-by: Phil Sutter 
---
 src/libnftables.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/libnftables.c b/src/libnftables.c
index 547aa59bf978c..23a262ca46fe3 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -420,15 +420,14 @@ static int nft_parse_bison_filename(struct nft_ctx *nft, 
const char *filename,
struct list_head *msgs, struct list_head 
*cmds)
 {
struct cmd *cmd;
-   void *scanner;
int ret;
 
parser_init(nft, nft->state, msgs, cmds);
-   scanner = scanner_init(nft->state);
-   if (scanner_read_file(scanner, filename, _location) < 0)
+   nft->scanner = scanner_init(nft->state);
+   if (scanner_read_file(nft->scanner, filename, _location) < 0)
return -1;
 
-   ret = nft_parse(nft, scanner, nft->state);
+   ret = nft_parse(nft, nft->scanner, nft->state);
if (ret != 0 || nft->state->nerrs > 0)
return -1;
 
-- 
2.19.0

[nft PATCH] Fix memleak in netlink_parse_fwd() error path

[Phil Sutter]

Make sure allocated 'stmt' is freed before returning to caller.

Fixes: 30d45266bf38b ("expr: extend fwd statement to support address and 
family")
Signed-off-by: Phil Sutter 
---
 src/netlink_delinearize.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 0a6ebe05ca7ca..cd058850f4df0 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1227,9 +1227,11 @@ static void netlink_parse_fwd(struct netlink_parse_ctx 
*ctx,
reg2 = netlink_parse_register(nle, NFTNL_EXPR_FWD_SREG_ADDR);
if (reg2) {
addr = netlink_get_register(ctx, loc, reg2);
-   if (addr == NULL)
-   return netlink_error(ctx, loc,
-"fwd statement has no 
output expression");
+   if (addr == NULL) {
+   netlink_error(ctx, loc,
+ "fwd statement has no output 
expression");
+   goto out_err;
+   }
 
switch (stmt->fwd.family) {
case AF_INET:
@@ -1241,8 +1243,9 @@ static void netlink_parse_fwd(struct netlink_parse_ctx 
*ctx,
  BYTEORDER_BIG_ENDIAN);
break;
default:
-   return netlink_error(ctx, loc,
-"fwd statement has no 
family");
+   netlink_error(ctx, loc,
+ "fwd statement has no family");
+   goto out_err;
}
stmt->fwd.addr = addr;
}
-- 
2.19.0

[nft PATCH 1/8] tests/py: Add missing JSON bits for inet/meta.t

[Phil Sutter]

Those were forgotten when renaming meta secpath to meta ipsec.

Fixes: 8f55ed41d0070 ("src: rename meta secpath to meta ipsec")
Signed-off-by: Phil Sutter 
---
 tests/py/inet/meta.t.json|  8 +---
 tests/py/inet/meta.t.json.output | 15 +++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/tests/py/inet/meta.t.json b/tests/py/inet/meta.t.json
index 77f46ab6ab9e5..5501f0bec6eda 100644
--- a/tests/py/inet/meta.t.json
+++ b/tests/py/inet/meta.t.json
@@ -185,14 +185,16 @@
 }
 ]
 
-# meta secpath exists
+# meta ipsec exists
 [
 {
 "match": {
 "left": {
-"meta": { "key": "secpath" }
+"meta": {
+"key": "ipsec"
+}
 },
-   "op": "==",
+"op": "==",
 "right": true
 }
 }
diff --git a/tests/py/inet/meta.t.json.output b/tests/py/inet/meta.t.json.output
index d0bb0a610e4e0..3e7dd2145e67f 100644
--- a/tests/py/inet/meta.t.json.output
+++ b/tests/py/inet/meta.t.json.output
@@ -36,3 +36,18 @@
 }
 ]
 
+# meta secpath missing
+[
+{
+"match": {
+"left": {
+"meta": {
+"key": "ipsec"
+}
+},
+"op": "==",
+"right": false
+}
+}
+]
+
-- 
2.19.0

[nft PATCH 6/8] monitor: Fix printing of ct objects

[Phil Sutter]

Monitor output is supposed to be single lined without tabs, but ct
object were printed with newlines and tabs hard-coded. Fixing this
wasn't too hard given that there is 'stmt_separator' to also include
semi-colons where required if newline was removed.

A more obvious mistake was position of object type in monitor output:
Like with other object types, it has to occur between command and table
spec. As a positive side-effect, this aligns ct objects better with
others (see obj_type_name_array for instance).

Signed-off-by: Phil Sutter 
---
 src/json.c|  2 -
 src/rule.c| 47 +++
 tests/monitor/testcases/object.t  | 33 +
 tests/shell/testcases/listing/0013objects_0   |  2 +-
 .../testcases/nft-f/0017ct_timeout_obj_0  |  2 +-
 .../nft-f/dumps/0017ct_timeout_obj_0.nft  |  2 +-
 6 files changed, 63 insertions(+), 25 deletions(-)
 create mode 100644 tests/monitor/testcases/object.t

diff --git a/src/json.c b/src/json.c
index b8d9333e877a8..a0a2b9db65b0b 100644
--- a/src/json.c
+++ b/src/json.c
@@ -282,7 +282,6 @@ static json_t *obj_print_json(struct output_ctx *octx, 
const struct obj *obj)
json_decref(tmp);
break;
case NFT_OBJECT_CT_HELPER:
-   type = "ct helper";
tmp = json_pack("{s:s, s:o, s:s}",
"type", obj->ct_helper.name, "protocol",
proto_name_json(obj->ct_helper.l4proto),
@@ -291,7 +290,6 @@ static json_t *obj_print_json(struct output_ctx *octx, 
const struct obj *obj)
json_decref(tmp);
break;
case NFT_OBJECT_CT_TIMEOUT:
-   type = "ct timeout";
tmp = timeout_policy_json(obj->ct_timeout.l4proto,
  obj->ct_timeout.timeout);
tmp = json_pack("{s:o, s:s, s:o}",
diff --git a/src/rule.c b/src/rule.c
index b00a17d652005..58ac94f6716ca 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1662,12 +1662,13 @@ static void print_proto_name_proto(uint8_t l4, struct 
output_ctx *octx)
 }
 
 static void print_proto_timeout_policy(uint8_t l4, const uint32_t *timeout,
+  struct print_fmt_options *opts,
   struct output_ctx *octx)
 {
bool comma = false;
unsigned int i;
 
-   nft_print(octx, "\t\tpolicy = {");
+   nft_print(octx, "%s%spolicy = { ", opts->tab, opts->tab);
for (i = 0; i < timeout_protocol[l4].array_size; i++) {
if (timeout[i] != timeout_protocol[l4].dflt_timeout[i]) {
if (comma)
@@ -1678,7 +1679,7 @@ static void print_proto_timeout_policy(uint8_t l4, const 
uint32_t *timeout,
comma = true;
}
}
-   nft_print(octx, "}");
+   nft_print(octx, " }%s", opts->stmt_separator);
 }
 
 static void obj_print_data(const struct obj *obj,
@@ -1695,8 +1696,8 @@ static void obj_print_data(const struct obj *obj,
nft_print(octx, "packets 0 bytes 0");
break;
}
-   nft_print(octx, "packets %" PRIu64 " bytes %" PRIu64 "",
- obj->counter.packets, obj->counter.bytes);
+   nft_print(octx, "packets %" PRIu64 " bytes %" PRIu64 "%s",
+ obj->counter.packets, obj->counter.bytes, opts->nl);
break;
case NFT_OBJECT_QUOTA: {
const char *data_unit;
@@ -1715,33 +1716,37 @@ static void obj_print_data(const struct obj *obj,
nft_print(octx, " used %" PRIu64 " %s",
  bytes, data_unit);
}
+   nft_print(octx, "%s", opts->nl);
}
break;
case NFT_OBJECT_CT_HELPER:
-   nft_print(octx, "ct helper %s {", obj->handle.obj.name);
+   nft_print(octx, " %s {", obj->handle.obj.name);
if (octx->handle > 0)
nft_print(octx, " # handle %" PRIu64, 
obj->handle.handle.id);
nft_print(octx, "%s", opts->nl);
-   nft_print(octx, "\t\ttype \"%s\" protocol ",
- obj->ct_helper.name);
+   nft_print(octx, "%s%stype \"%s\" protocol ",
+ opts->tab, opts->tab, obj->ct_helper.name);
print_proto_name_proto(obj->ct_helper.l4proto, octx);
-   nft_print(octx, "\n");
-   n

[nft PATCH 0/8] monitor: Use libnftables for JSON output

[Phil Sutter]

This series essentially moves nft monitor JSON output to libnftables (in
patch 7). Patch 8 enhances tests/monitor to get that tested as well (via
passing '-j' parameter to run-tests.sh). The leading six patches are
more or less prerequisites for the later ones.

Phil Sutter (8):
  tests/py: Add missing JSON bits for inet/meta.t
  json: Drop unused symbolic_constant_json() stub
  json: Add ct timeout support
  monitor: Drop fake XML support
  monitor: Drop 'update table' and 'update chain' cases
  monitor: Fix printing of ct objects
  monitor: Use libnftables JSON output
  tests: monitor: Test JSON output as well

 include/json.h|  57 +++-
 src/json.c|  86 -
 src/monitor.c | 293 +-
 src/parser_json.c |  82 -
 src/rule.c|  49 +--
 tests/monitor/README  |  27 +-
 tests/monitor/run-tests.sh|  45 ++-
 tests/monitor/testcases/object.t  |  46 +++
 tests/monitor/testcases/set-maps.t|   4 +
 tests/monitor/testcases/set-mixed.t   |   7 +
 tests/monitor/testcases/set-multiple.t|   5 +
 tests/monitor/testcases/set-simple.t  |  19 ++
 tests/monitor/testcases/simple.t  |   8 +
 tests/py/inet/meta.t.json |   8 +-
 tests/py/inet/meta.t.json.output  |  15 +
 tests/py/ip/objects.t.json|   7 +
 tests/shell/testcases/listing/0013objects_0   |   2 +-
 .../testcases/nft-f/0017ct_timeout_obj_0  |   2 +-
 .../nft-f/dumps/0017ct_timeout_obj_0.nft  |   2 +-
 19 files changed, 567 insertions(+), 197 deletions(-)
 create mode 100644 tests/monitor/testcases/object.t

-- 
2.19.0

[nft PATCH 7/8] monitor: Use libnftables JSON output

[Phil Sutter]

This switches 'nft monitor' JSON output from using libnftnl's to
libnftables' implementation.

Signed-off-by: Phil Sutter 
---
 include/json.h |  51 +
 src/json.c |  57 ++
 src/monitor.c  | 281 +
 src/rule.c |   2 -
 4 files changed, 251 insertions(+), 140 deletions(-)

diff --git a/include/json.h b/include/json.h
index 663375489bc51..d2dc92d963693 100644
--- a/include/json.h
+++ b/include/json.h
@@ -9,9 +9,11 @@ struct expr;
 struct netlink_ctx;
 struct rule;
 struct set;
+struct obj;
 struct stmt;
 struct symbol_table;
 struct table;
+struct netlink_mon_handler;
 
 #ifdef HAVE_LIBJANSSON
 
@@ -88,6 +90,19 @@ int nft_parse_json_buffer(struct nft_ctx *nft, const char 
*buf,
 int nft_parse_json_filename(struct nft_ctx *nft, const char *filename,
struct list_head *msgs, struct list_head *cmds);
 
+void monitor_print_table_json(struct netlink_mon_handler *monh,
+ const char *cmd, struct table *t);
+void monitor_print_chain_json(struct netlink_mon_handler *monh,
+ const char *cmd, struct chain *c);
+void monitor_print_set_json(struct netlink_mon_handler *monh,
+   const char *cmd, struct set *s);
+void monitor_print_element_json(struct netlink_mon_handler *monh,
+   const char *cmd, struct set *s);
+void monitor_print_obj_json(struct netlink_mon_handler *monh,
+   const char *cmd, struct obj *o);
+void monitor_print_rule_json(struct netlink_mon_handler *monh,
+const char *cmd, struct rule *r);
+
 #else /* ! HAVE_LIBJANSSON */
 
 typedef void json_t;
@@ -183,6 +198,42 @@ nft_parse_json_filename(struct nft_ctx *nft, const char 
*filename,
return -EINVAL;
 }
 
+static inline void monitor_print_table_json(struct netlink_mon_handler *monh,
+   const char *cmd, struct table *t)
+{
+   /* empty */
+}
+
+static inline void monitor_print_chain_json(struct netlink_mon_handler *monh,
+   const char *cmd, struct chain *c)
+{
+   /* empty */
+}
+
+static inline void monitor_print_set_json(struct netlink_mon_handler *monh,
+ const char *cmd, struct set *s)
+{
+   /* empty */
+}
+
+static inline void monitor_print_element_json(struct netlink_mon_handler *monh,
+ const char *cmd, struct set *s)
+{
+   /* empty */
+}
+
+static inline void monitor_print_obj_json(struct netlink_mon_handler *monh,
+ const char *cmd, struct obj *o)
+{
+   /* empty */
+}
+
+static inline void monitor_print_rule_json(struct netlink_mon_handler *monh,
+  const char *cmd, struct rule *r)
+{
+   /* empty */
+}
+
 #endif /* HAVE_LIBJANSSON */
 
 #endif /* NFTABLES_JSON_H */
diff --git a/src/json.c b/src/json.c
index a0a2b9db65b0b..f5d97c623fb3a 100644
--- a/src/json.c
+++ b/src/json.c
@@ -148,6 +148,19 @@ static json_t *set_print_json(struct output_ctx *octx, 
const struct set *set)
return json_pack("{s:o}", type, root);
 }
 
+/* XXX: Merge with set_print_json()? */
+static json_t *element_print_json(struct output_ctx *octx,
+ const struct set *set)
+{
+   json_t *root = expr_print_json(set->init, octx);
+
+   return json_pack("{s: {s:s, s:s, s:s, s:o}}", "element",
+"family", family2str(set->handle.family),
+"table", set->handle.table.name,
+"name", set->handle.set.name,
+"elem", root);
+}
+
 static json_t *stmt_print_json(const struct stmt *stmt, struct output_ctx 
*octx)
 {
char buf[1024];
@@ -1714,3 +1727,47 @@ int do_command_list_json(struct netlink_ctx *ctx, struct 
cmd *cmd)
json_decref(root);
return 0;
 }
+
+static void monitor_print_json(struct netlink_mon_handler *monh,
+  const char *cmd, json_t *obj)
+{
+   obj = json_pack("{s:o}", cmd, obj);
+   json_dumpf(obj, monh->ctx->octx->output_fp, 0);
+   json_decref(obj);
+}
+
+void monitor_print_table_json(struct netlink_mon_handler *monh,
+ const char *cmd, struct table *t)
+{
+   monitor_print_json(monh, cmd, table_print_json(monh->ctx->octx, t));
+}
+
+void monitor_print_chain_json(struct netlink_mon_handler *monh,
+ const char *cmd, struct chain *c)
+{
+   monitor_print_json(monh, cmd, chain_print_json(monh->ctx->octx, c));
+}
+
+void monitor_print_set_json(struct netlink_mon_handler *monh,
+   const char *cmd, struct set *s)
+{
+   monitor_print_json(monh,

[nft PATCH 4/8] monitor: Drop fake XML support

[Phil Sutter]

Since libnftnl doesn't support XML formatting, pretending to do so in
nft monitor is pointless.

Signed-off-by: Phil Sutter 
---
 src/monitor.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/src/monitor.c b/src/monitor.c
index 4310c3b8dc434..d75410888e3d0 100644
--- a/src/monitor.c
+++ b/src/monitor.c
@@ -203,7 +203,6 @@ static int netlink_events_table_cb(const struct nlmsghdr 
*nlh, int type,
  nftnl_table_get_u64(nlt, 
NFTNL_TABLE_HANDLE));
nft_mon_print(monh, "\n");
break;
-   case NFTNL_OUTPUT_XML:
case NFTNL_OUTPUT_JSON:
nftnl_table_fprintf(monh->ctx->octx->output_fp, nlt,
monh->format, netlink_msg2nftnl_of(type));
@@ -245,7 +244,6 @@ static int netlink_events_chain_cb(const struct nlmsghdr 
*nlh, int type,
break;
}
break;
-   case NFTNL_OUTPUT_XML:
case NFTNL_OUTPUT_JSON:
nftnl_chain_fprintf(monh->ctx->octx->output_fp, nlc,
monh->format, netlink_msg2nftnl_of(type));
@@ -292,7 +290,6 @@ static int netlink_events_set_cb(const struct nlmsghdr 
*nlh, int type,
break;
}
break;
-   case NFTNL_OUTPUT_XML:
case NFTNL_OUTPUT_JSON:
nftnl_set_fprintf(monh->ctx->octx->output_fp, nls,
  monh->format, netlink_msg2nftnl_of(type));
@@ -441,7 +438,6 @@ static int netlink_events_setelem_cb(const struct nlmsghdr 
*nlh, int type,
 
set_free(dummyset);
break;
-   case NFTNL_OUTPUT_XML:
case NFTNL_OUTPUT_JSON:
nftnl_set_fprintf(monh->ctx->octx->output_fp, nls,
  monh->format, netlink_msg2nftnl_of(type));
@@ -486,7 +482,6 @@ static int netlink_events_obj_cb(const struct nlmsghdr 
*nlh, int type,
break;
}
break;
-   case NFTNL_OUTPUT_XML:
case NFTNL_OUTPUT_JSON:
nftnl_obj_fprintf(monh->ctx->octx->output_fp, nlo,
  monh->format, netlink_msg2nftnl_of(type));
@@ -542,7 +537,6 @@ static int netlink_events_rule_cb(const struct nlmsghdr 
*nlh, int type,
break;
}
break;
-   case NFTNL_OUTPUT_XML:
case NFTNL_OUTPUT_JSON:
nftnl_rule_fprintf(monh->ctx->octx->output_fp, nlr,
   monh->format, netlink_msg2nftnl_of(type));
-- 
2.19.0

[nft PATCH 3/8] json: Add ct timeout support

[Phil Sutter]

Add support for printing and parsing ct timeout objects to JSON API.

Signed-off-by: Phil Sutter 
---
 src/json.c | 29 ++
 src/parser_json.c  | 82 +-
 tests/py/ip/objects.t.json |  7 
 3 files changed, 117 insertions(+), 1 deletion(-)

diff --git a/src/json.c b/src/json.c
index 0191a2ea7df0d..b8d9333e877a8 100644
--- a/src/json.c
+++ b/src/json.c
@@ -235,6 +235,23 @@ static json_t *proto_name_json(uint8_t proto)
return json_integer(proto);
 }
 
+static json_t *timeout_policy_json(uint8_t l4, const uint32_t *timeout)
+{
+   json_t *root = NULL;
+   unsigned int i;
+
+   for (i = 0; i < timeout_protocol[l4].array_size; i++) {
+   if (timeout[i] == timeout_protocol[l4].dflt_timeout[i])
+   continue;
+
+   if (!root)
+   root = json_object();
+   json_object_set(root, timeout_protocol[l4].state_to_name[i],
+   json_integer(timeout[i]));
+   }
+   return root ? : json_null();
+}
+
 static json_t *obj_print_json(struct output_ctx *octx, const struct obj *obj)
 {
const char *rate_unit = NULL, *burst_unit = NULL;
@@ -273,6 +290,18 @@ static json_t *obj_print_json(struct output_ctx *octx, 
const struct obj *obj)
json_object_update(root, tmp);
json_decref(tmp);
break;
+   case NFT_OBJECT_CT_TIMEOUT:
+   type = "ct timeout";
+   tmp = timeout_policy_json(obj->ct_timeout.l4proto,
+ obj->ct_timeout.timeout);
+   tmp = json_pack("{s:o, s:s, s:o}",
+   "protocol",
+   proto_name_json(obj->ct_timeout.l4proto),
+   "l3proto", family2str(obj->ct_timeout.l3proto),
+   "policy", tmp);
+   json_object_update(root, tmp);
+   json_decref(tmp);
+   break;
case NFT_OBJECT_LIMIT:
rate = obj->limit.rate;
burst = obj->limit.burst;
diff --git a/src/parser_json.c b/src/parser_json.c
index 9aadc33ed93a0..35464c9a83c83 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -2106,7 +2106,22 @@ static struct stmt *json_parse_cthelper_stmt(struct 
json_ctx *ctx,
stmt->objref.type = NFT_OBJECT_CT_HELPER;
stmt->objref.expr = json_parse_stmt_expr(ctx, value);
if (!stmt->objref.expr) {
-   json_error(ctx, "Invalid cthelper reference.");
+   json_error(ctx, "Invalid ct helper reference.");
+   stmt_free(stmt);
+   return NULL;
+   }
+   return stmt;
+}
+
+static struct stmt *json_parse_cttimeout_stmt(struct json_ctx *ctx,
+const char *key, json_t *value)
+{
+   struct stmt *stmt = objref_stmt_alloc(int_loc);
+
+   stmt->objref.type = NFT_OBJECT_CT_TIMEOUT;
+   stmt->objref.expr = json_parse_stmt_expr(ctx, value);
+   if (!stmt->objref.expr) {
+   json_error(ctx, "Invalid ct timeout reference.");
stmt_free(stmt);
return NULL;
}
@@ -2257,6 +2272,7 @@ static struct stmt *json_parse_stmt(struct json_ctx *ctx, 
json_t *root)
{ "set", json_parse_set_stmt },
{ "log", json_parse_log_stmt },
{ "ct helper", json_parse_cthelper_stmt },
+   { "ct timeout", json_parse_cttimeout_stmt },
{ "meter", json_parse_meter_stmt },
{ "queue", json_parse_queue_stmt },
{ "ct count", json_parse_connlimit_stmt },
@@ -2737,6 +2753,39 @@ static struct cmd *json_parse_cmd_add_flowtable(struct 
json_ctx *ctx,
return cmd_alloc(op, cmd_obj, , int_loc, flowtable);
 }
 
+static int json_parse_ct_timeout_policy(struct json_ctx *ctx,
+   json_t *root, struct obj *obj)
+{
+   json_t *tmp, *val;
+   const char *key;
+
+   if (!json_unpack(root, "{s:o}", "policy", ))
+   return 0;
+
+   if (json_is_object(tmp)) {
+   json_error(ctx, "Invalid ct timeout policy.");
+   return 1;
+   }
+
+   init_list_head(>ct_timeout.timeout_list);
+   json_object_foreach(tmp, key, val) {
+   struct timeout_state *ts;
+
+   if (!json_is_integer(val)) {
+   json_error(ctx, "Invalid ct timeout policy value for 
'%s'.", key);
+   return 1;
+   }
+
+   ts = xzalloc(sizeof(*ts));
+   ts->timeout_str = xstrdup(key);
+   ts-&g

[nft PATCH 8/8] tests: monitor: Test JSON output as well

[Phil Sutter]

Enhance monitor test suite to test check JSON output as well. Note that
for now there is no support for --echo output testing with JSON.

Signed-off-by: Phil Sutter 
---
 tests/monitor/README   | 27 +++-
 tests/monitor/run-tests.sh | 45 --
 tests/monitor/testcases/object.t   | 13 
 tests/monitor/testcases/set-maps.t |  4 +++
 tests/monitor/testcases/set-mixed.t|  7 
 tests/monitor/testcases/set-multiple.t |  5 +++
 tests/monitor/testcases/set-simple.t   | 19 +++
 tests/monitor/testcases/simple.t   |  8 +
 8 files changed, 117 insertions(+), 11 deletions(-)

diff --git a/tests/monitor/README b/tests/monitor/README
index 9c5e37f5c75c9..39096a7fae078 100644
--- a/tests/monitor/README
+++ b/tests/monitor/README
@@ -15,13 +15,14 @@ to be established manually, i.e. in order to test monitor 
output when adding a
 chain, the table containing it has to be created first. In between each
 testcase, rule set is flushed completely.
 
-Input and output lines are prefixed by 'I' and 'O', respectively. The prefix 
has
-to be separated from the rest of the line by whitespace. Consecutive input 
lines
-are passed to 'nft' together, hence lead to a single transaction.
+Input lines are prefixed by 'I'. Multiple consecutive input lines are passed to
+'nft' together, hence lead to a single transaction.
 
-Since in most cases output should be equal to input, there is a shortcut: If a
-line consists of 'O -' only, the test script uses all previous input lines as
-expected output directly.
+There are two types of output lines: Those for standard syntax, prefixed by 'O'
+and those for JSON output, prefixed by 'J'. For standard syntax output lines,
+there is a shortcut: If a line consists of 'O -' only, the test script uses all
+previous input lines as expected output directly. Of course this is not
+available for JSON output lines.
 
 Empty lines and those starting with '#' are ignored.
 
@@ -29,8 +30,8 @@ Test Script Semantics
 -
 
 The script iterates over all test case files, reading them line by line. It
-assumes that sections of 'I' lines alternate with sections of 'O' lines. After
-stripping the prefix, each line is appended to a temporary file. There are
+assumes that sections of 'I' lines alternate with sections of 'O'/'J' lines.
+After stripping the prefix, each line is appended to a temporary file. There 
are
 separate files for input and output lines.
 
 If a set of input and output lines is complete (i.e. upon encountering either a
@@ -46,3 +47,13 @@ Note: Running 'nft monitor' in background is prone to race 
conditions. Hence
 an artificial delay is introduced before calling 'nft -f' to allow for 'nft
 monitor' to complete initialization and another one before comparing the output
 to allow for 'nft monitor' to process the netlink events.
+
+By default, only standard syntax is being tested for, i.e. 'J'-prefixed lines
+are simply ignored. If JSON testing was requested (by passing '-j' flag to the
+test script), 'O'-prefixed lines in turn are ignored.
+
+There is one caveat with regards to JSON output: Since it always contains 
handle
+properties (if the given object possesses such) which is supposed to be
+arbitrary, there is a filter script which normalizes all handle values in
+monitor output to zero before comparison. Therefore expected output must have
+all handle properties present but with a value of zero.
diff --git a/tests/monitor/run-tests.sh b/tests/monitor/run-tests.sh
index 1adabda193949..f4089887b69aa 100755
--- a/tests/monitor/run-tests.sh
+++ b/tests/monitor/run-tests.sh
@@ -3,6 +3,7 @@
 cd $(dirname $0)
 nft=../../src/nft
 debug=false
+test_json=false
 
 mydiff() {
diff -w -I '^# ' "$@"
@@ -47,9 +48,16 @@ echo_output_append() {
}
[[ "$*" =~ ^add|replace|insert ]] && echo "$*" >>$output_file
 }
+json_output_filter() { # (filename)
+   # unify handle values
+   sed -i -e 's/\("handle":\) [0-9][0-9]*/\1 0/g' "$1"
+}
 monitor_run_test() {
monitor_output=$(mktemp -p $testdir)
-   $nft -nn monitor >$monitor_output &
+   monitor_args=""
+   $test_json && monitor_args="vm json"
+
+   $nft -nn monitor $monitor_args >$monitor_output &
monitor_pid=$!
 
sleep 0.5
@@ -67,6 +75,7 @@ monitor_run_test() {
sleep 0.5
kill $monitor_pid
wait >/dev/null 2>&1
+   $test_json && json_output_filter $monitor_output
if ! mydiff -q $monitor_output $output_file >/dev/null 2>&1; then
echo "monitor output differs!"
mydiff -u $output_file $monitor_output
@@ -99,7 +108,33 @@ echo_run_test() {
touch $output_file
 }
 
-for variant in monitor echo; do
+while [ -n "$1" ]; do
+   case "$1"

[nft PATCH 2/8] json: Drop unused symbolic_constant_json() stub

[Phil Sutter]

This seems like a left-over from day 1: Said function is static in
json.c, so there is no point in providing a stub when compiling with
JSON disabled.

Signed-off-by: Phil Sutter 
---
 include/json.h | 6 --
 1 file changed, 6 deletions(-)

diff --git a/include/json.h b/include/json.h
index 99b676446516b..663375489bc51 100644
--- a/include/json.h
+++ b/include/json.h
@@ -164,12 +164,6 @@ STMT_PRINT_STUB(tproxy)
 #undef EXPR_PRINT_STUB
 #undef JSON_PRINT_STUB
 
-static inline json_t *symbolic_constant_json(const struct symbol_table *tbl,
-const struct expr *expr)
-{
-   return NULL;
-}
-
 static inline int do_command_list_json(struct netlink_ctx *ctx, struct cmd 
*cmd)
 {
return -1;
-- 
2.19.0

[iptables PATCH] xtables: Remove target_maxnamelen field

[Phil Sutter]

This is a partial revert of commit 9f075031a1973 ("Combine
parse_target() and command_jump() implementations"): Upstream prefers to
reduce max chain name length of arptables by two characters instead of
the introduced struct xtables_globals field which requires to bump
library API version.

Fixes: 9f075031a1973 ("Combine parse_target() and command_jump() 
implementations")
Signed-off-by: Phil Sutter 
---
 include/xtables.h  | 1 -
 iptables/ip6tables.c   | 1 -
 iptables/iptables.c| 1 -
 iptables/xshared.c | 6 +++---
 iptables/xtables-arp.c | 1 -
 iptables/xtables.c | 1 -
 6 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/include/xtables.h b/include/xtables.h
index fd9e6c33516bc..bf169b08186f5 100644
--- a/include/xtables.h
+++ b/include/xtables.h
@@ -424,7 +424,6 @@ struct xtables_globals
struct option *opts;
void (*exit_err)(enum xtables_exittype status, const char *msg, ...) 
__attribute__((noreturn, format(printf,2,3)));
int (*compat_rev)(const char *name, uint8_t rev, int opt);
-   size_t target_maxnamelen;
 };
 
 #define XT_GETOPT_TABLEEND {.name = NULL, .has_arg = false}
diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 7a9cd643f8e76..fe089de4c85d7 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -125,7 +125,6 @@ struct xtables_globals ip6tables_globals = {
.orig_opts = original_opts,
.exit_err = ip6tables_exit_error,
.compat_rev = xtables_compatible_revision,
-   .target_maxnamelen = XT_EXTENSION_MAXNAMELEN,
 };
 
 /* Table of legal combinations of commands and options.  If any of the
diff --git a/iptables/iptables.c b/iptables/iptables.c
index b9ce64e6b32a6..f8041f56ce70d 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -124,7 +124,6 @@ struct xtables_globals iptables_globals = {
.orig_opts = original_opts,
.exit_err = iptables_exit_error,
.compat_rev = xtables_compatible_revision,
-   .target_maxnamelen = XT_EXTENSION_MAXNAMELEN,
 };
 
 /* Table of legal combinations of commands and options.  If any of the
diff --git a/iptables/xshared.c b/iptables/xshared.c
index d5365d9398e90..b16f5fa68e569 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -641,10 +641,10 @@ const char *xt_parse_target(const char *targetname)
xtables_error(PARAMETER_PROBLEM,
   "Invalid target name (too short)");
 
-   if (strlen(targetname) >= xt_params->target_maxnamelen)
+   if (strlen(targetname) >= XT_EXTENSION_MAXNAMELEN)
xtables_error(PARAMETER_PROBLEM,
-  "Invalid target name `%s' (%zu chars max)",
-  targetname, xt_params->target_maxnamelen - 1);
+  "Invalid target name `%s' (%u chars max)",
+  targetname, XT_EXTENSION_MAXNAMELEN - 1);
 
for (ptr = targetname; *ptr; ptr++)
if (isspace(*ptr))
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 4a782148d0f6a..6939a611df1bb 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -158,7 +158,6 @@ struct xtables_globals arptables_globals = {
.orig_opts  = original_opts,
.exit_err   = xtables_exit_error,
.compat_rev = nft_compatible_revision,
-   .target_maxnamelen  = sizeof(arpt_chainlabel),
 };
 
 /* Table of legal combinations of commands and options.  If any of the
diff --git a/iptables/xtables.c b/iptables/xtables.c
index c17e66f1a805f..e0343dbabf2b3 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -108,7 +108,6 @@ struct xtables_globals xtables_globals = {
.orig_opts = original_opts,
.exit_err = xtables_exit_error,
.compat_rev = nft_compatible_revision,
-   .target_maxnamelen = XT_EXTENSION_MAXNAMELEN,
 };
 
 /* Table of legal combinations of commands and options.  If any of the
-- 
2.19.0

Re: [nft PATCH] parser_bison: Fix for ECN keyword in LHS of relational

[Phil Sutter]

Hi Pablo,

On Wed, Oct 03, 2018 at 05:28:24PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Aug 24, 2018 at 01:26:57PM +0200, Phil Sutter wrote:
> > Of all possible TCP flags, 'ecn' is special since it is recognized by
> > lex as a keyword (there is a a field in IPv4 and IPv6 headers with the
> > same name). Therefore it is listed in keyword_expr, but that was
> > sufficient for RHS only. The following statement reproduces the issue:
> > 
> > | tcp flags & (syn | ecn) == (syn | ecn)
> > 
> > The solution is to limit binop expressions to accept an RHS expression
> > on RHS ("real" LHS expressions don't make much sense there anyway),
> > which then allows keyword_expr to occur there. In order to maintain the
> > recursive behaviour if braces are present, allow primary_rhs_expr to
> > consist of a basic_rhs_expr enclosed in braces. This in turn requires
> > for braced RHS part in relational_expr to be dropped, otherwise bison
> > complains about shift/reduce conflict.
> 
> Sorry, I think I misunderstood this email.
> 
> The following is:
> 
> nft add rule x y tcp flags & (syn | ecn) == (syn | ecn)
> 
> Same thing with:
> 
> nft add rule x y 'tcp flags and (fin | syn | rst | psh | ack | urg | 
> ecn | cwr) eq (fin | syn | rst | psh | ack | urg | ecn | cwr)
> 
> So, what is what we don't support anymore after your patch?

Yes, this was a misunderstanding. My patch doesn't limit functionality
in any way (or at least it shouldn't :) - the original problem was that
'ecn' is recognized by scanner.l as a keyword, not a generic string
(like the other flag names). The existing code handles this fine for
RHS, e.g.:

| tcp flags == ecn

But on LHS, it wasn't possible to use 'ecn'. Simple example:

| tcp flags & ecn == ecn

The problem here is that 'and_expr' in parser_bison.y allows only
'shift_expr' after the '&' sign, while the 'ecn' keyword is contained in
'keyword_expr' which in turn is contained by the '*_rhs_expr's only.

What my patch essentially does is change any of the binop expressions to
accept a *_rhs_expr on their RHS (i.e., after the binop-specific
symbol). This effectively makes the parser more strict: the rhs-variants
don't contain all expressions the non-rhs-variants do. But in this case
it should be correct, e.g. you wouldn't want to allow something like:

| tcp flags & tcp dport == 0

My patch though caused a shift/reduce conflict. I could solve it by
changing where the recursion (in bison) appears if braces are contained
in the input. So I didn't change how braces may be specified in input,
but "merely" what the parser resolves input containing braces into.

Cheers, Phil

[nft PATCH] tests: shell: Test 'get element' command

[Phil Sutter]

This command is currently broken when used in sets with ranges. Test
various variants against known data and check if output is as expected.

Signed-off-by: Phil Sutter 
---
 tests/shell/testcases/sets/0034get_element_0 | 37 
 1 file changed, 37 insertions(+)
 create mode 100755 tests/shell/testcases/sets/0034get_element_0

diff --git a/tests/shell/testcases/sets/0034get_element_0 
b/tests/shell/testcases/sets/0034get_element_0
new file mode 100755
index 0..2bfb527b8080f
--- /dev/null
+++ b/tests/shell/testcases/sets/0034get_element_0
@@ -0,0 +1,37 @@
+#!/bin/bash
+
+RC=0
+
+check() { # (elems, expected)
+   out=$($NFT get element ip t s "{ $1 }" 2>/dev/null)
+   out=$(grep "elements =" <<< "$out")
+   out="${out#* \{ }"
+   out="${out% \}}"
+   [[ "$out" == "$2" ]] && return
+   echo "ERROR: asked for '$1', expecting '$2' but got '$out'"
+   ((RC++))
+}
+
+RULESET="add table ip t
+add set ip t s { type inet_service; flags interval; }
+add element ip t s { 10, 20-30, 40, 50-60 }
+"
+
+$NFT -f - <<< "$RULESET"
+
+# simple cases, (non-)existing values and ranges
+check 10 10
+check 11 ""
+check 20-30 20-30
+check 15-18 ""
+
+# multiple single elements, ranges smaller than present
+check "10, 40" "10, 40"
+check 21-29 20-30
+
+# non-existing ranges matching elements
+check 10-40 ""
+check 10-25 ""
+check 25-55 ""
+
+exit $RC
-- 
2.19.0

Re: [libnftnl PATCH] expr: xfrm: Fix for unused variable warning

[Phil Sutter]

On Tue, Sep 25, 2018 at 12:18:50PM +0200, Florian Westphal wrote:
> Phil Sutter  wrote:
> > In nftnl_expr_xfrm_json_parse(), variable 'spnum' is apparently unused
> > so remove it.
> 
> It should be used, "spnum" parsing is missing.

I see. Máté, do you plan to add the missing bits?

Thanks, Phil

nft: Dubious code in get_set_decompose() of src/segtree.c

[Phil Sutter]

Hi Pablo,

When dealing with a covscan report for nft, I was pointed at the loop's
else-clause of get_set_decompose() as it overwrites 'left' without
freeing it first. The code in question is this:

| list_for_each_entry_safe(i, next, >init->expressions, list) {
| if (i->flags & EXPR_F_INTERVAL_END && left) {
| list_del(>list);
| list_del(>list);
| mpz_sub_ui(i->key->value, i->key->value, 1);
| new = range_expr_alloc(_location, left, i);
| compound_expr_add(new_init, new);
| left = NULL;
| } else {
| if (left) {
| left = get_set_interval_end(table,
| set->handle.set,
| left);
| compound_expr_add(new_init, left);
| }
| left = i;
| }
| }

IIUC, the else-clause should be hit for non-interval-end items, and the
call to get_set_interval_end() happens if we have two consecutive
non-interval-end items. I tried to trigger it, but failed, hence I
wonder if this situation is possible at all. Do you perhaps remember why
you put the code like this when implementing 'get element' command
(commit a43cc8d53096d)?

Thanks, Phil

[nft PATCH 4/5] tests: shell: Improve gen_chains() in 0021prio_0

[Phil Sutter]

Enhance the function to accept an optional fourth parameter specifying
the device name, then use it for netdev family. Also remove dubled empty
lines and instead put together what belongs together.

Signed-off-by: Phil Sutter 
---
 tests/shell/testcases/chains/0021prio_0 | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/tests/shell/testcases/chains/0021prio_0 
b/tests/shell/testcases/chains/0021prio_0
index 82f52e33cc2af..b54b6fae32c63 100755
--- a/tests/shell/testcases/chains/0021prio_0
+++ b/tests/shell/testcases/chains/0021prio_0
@@ -27,11 +27,13 @@ gen_chains () {
local family=$1
local hook=$2
local prioname=$3
+   local device=${4:+device $4}
 
for i in -11 -10 0 10 11
do
local offset=`format_offset $i`
-   $NFT add chain $family x `chainname $hook $prioname $offset` "{ 
type filter hook $hook priority $prioname $offset; }"
+   local chainname=`chainname $hook $prioname $offset`
+   $NFT add chain $family x $chainname "{ type filter hook $hook 
$device priority $prioname $offset; }"
done
 }
 
@@ -50,7 +52,6 @@ do
gen_chains $family postrouting srcnat
 done
 
-
 family=arp
 $NFT add table $family x
 for hook in input output
@@ -58,16 +59,9 @@ do
gen_chains $family $hook filter
 done
 
-
 family=netdev
 $NFT add table $family x
-hook=ingress
-prioname=filter
-for i in -11 -10 0 10 11
-do
-   offset=`format_offset $i`
-   $NFT add chain $family x `chainname $hook $prioname $offset` "{ type 
filter hook $hook device lo priority $prioname $offset; }"
-done
+gen_chains $family ingress filter lo
 
 family=bridge
 $NFT add table $family x
@@ -75,7 +69,6 @@ for hook in prerouting input forward output postrouting
 do
gen_chains $family $hook filter
 done
-
 gen_chains $family prerouting dstnat
 gen_chains $family output out
 gen_chains $family postrouting srcnat
-- 
2.19.0

[nft PATCH 2/5] tests: shell: Fix indenting in 0021prio_0

[Phil Sutter]

Pointless indenting doesn't increase readability, merely makes the
script seem more complicated than it actually is.

Signed-off-by: Phil Sutter 
---
 tests/shell/testcases/chains/0021prio_0 | 67 -
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/tests/shell/testcases/chains/0021prio_0 
b/tests/shell/testcases/chains/0021prio_0
index ada1d92a047a0..b6647ac24ad10 100755
--- a/tests/shell/testcases/chains/0021prio_0
+++ b/tests/shell/testcases/chains/0021prio_0
@@ -47,51 +47,50 @@ do
done
 
hook=prerouting
-   prioname=dstnat
-   gen_chains $family $hook $prioname
+   prioname=dstnat
+   gen_chains $family $hook $prioname
 
hook=postrouting
-   prioname=srcnat
-   gen_chains $family $hook $prioname
+   prioname=srcnat
+   gen_chains $family $hook $prioname
 done
 
 
 family=arp
-   $NFT add table $family x
-   for hook in input output
-   do
-   prioname=filter
-   gen_chains $family $hook $prioname
-   done
+$NFT add table $family x
+for hook in input output
+do
+   prioname=filter
+   gen_chains $family $hook $prioname
+done
 
 
 family=netdev
-   $NFT add table $family x
-   hook=ingress
-   prioname=filter
-   for i in -11 -10 0 10 11
-   do
-   offset=`format_offset $i`
-   $NFT add chain $family x `chainname $hook 
$prioname $offset` "{ type filter hook $hook device lo priority $prioname 
$offset; }"
-   done
+$NFT add table $family x
+hook=ingress
+prioname=filter
+for i in -11 -10 0 10 11
+do
+   offset=`format_offset $i`
+   $NFT add chain $family x `chainname $hook $prioname $offset` "{ type 
filter hook $hook device lo priority $prioname $offset; }"
+done
 
 family=bridge
-   $NFT add table $family x
-   for hook in prerouting input forward output postrouting
-   do
-   prioname=filter
-   gen_chains $family $hook $prioname
-   done
-
-   hook=prerouting
-   prioname=dstnat
-   gen_chains $family $hook $prioname
+$NFT add table $family x
+for hook in prerouting input forward output postrouting
+do
+   prioname=filter
+   gen_chains $family $hook $prioname
+done
 
-   hook=output
-   prioname=out
-   gen_chains $family $hook $prioname
+hook=prerouting
+prioname=dstnat
+gen_chains $family $hook $prioname
 
-   hook=postrouting
-   prioname=srcnat
-   gen_chains $family $hook $prioname
+hook=output
+prioname=out
+gen_chains $family $hook $prioname
 
+hook=postrouting
+prioname=srcnat
+gen_chains $family $hook $prioname
-- 
2.19.0

[nft PATCH 5/5] tests: shell: Improve performance of 0021prio_0

[Phil Sutter]

This test called nft binary 391 times and took about 38s to complete on
my testing VM. Improve this by writing all commands into a temporary
file for processing in a single nft call. Reduces run-time to about 4s.

Interestingly, piping the sub-process's output directly into 'nft -f -'
leads to spurious errors (parser complaining about perfectly fine
syntax). It seems like handling large input this way is not possible.

Signed-off-by: Phil Sutter 
---
 tests/shell/testcases/chains/0021prio_0 | 24 +---
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/tests/shell/testcases/chains/0021prio_0 
b/tests/shell/testcases/chains/0021prio_0
index b54b6fae32c63..e761297492baf 100755
--- a/tests/shell/testcases/chains/0021prio_0
+++ b/tests/shell/testcases/chains/0021prio_0
@@ -32,14 +32,22 @@ gen_chains () {
for i in -11 -10 0 10 11
do
local offset=`format_offset $i`
-   local chainname=`chainname $hook $prioname $offset`
-   $NFT add chain $family x $chainname "{ type filter hook $hook 
$device priority $prioname $offset; }"
+   local cmd="add chain $family x"
+   cmd+=" `chainname $hook $prioname $offset` {"
+   cmd+=" type filter hook $hook $device"
+   cmd+=" priority $prioname $offset; }"
+   echo "$cmd"
done
 }
 
+tmpfile=$(mktemp)
+trap "rm $tmpfile" EXIT
+
+(
+
 for family in ip ip6 inet
 do
-   $NFT add table $family x
+   echo "add table $family x"
for hook in prerouting input forward output postrouting
do
for prioname in raw mangle filter security
@@ -47,24 +55,23 @@ do
gen_chains $family $hook $prioname
done
done
-
gen_chains $family prerouting dstnat
gen_chains $family postrouting srcnat
 done
 
 family=arp
-$NFT add table $family x
+echo "add table $family x"
 for hook in input output
 do
gen_chains $family $hook filter
 done
 
 family=netdev
-$NFT add table $family x
+echo "add table $family x"
 gen_chains $family ingress filter lo
 
 family=bridge
-$NFT add table $family x
+echo "add table $family x"
 for hook in prerouting input forward output postrouting
 do
gen_chains $family $hook filter
@@ -72,3 +79,6 @@ done
 gen_chains $family prerouting dstnat
 gen_chains $family output out
 gen_chains $family postrouting srcnat
+
+) >$tmpfile
+$NFT -f $tmpfile
-- 
2.19.0

[nft PATCH 3/5] tests: shell: Drop one-time use variables in 0021prio_0

[Phil Sutter]

There is really no point in declaring a variable which is used just
once. Also mark function local variables as such to make sure they don't
overwrite global ones.

Signed-off-by: Phil Sutter 
---
 tests/shell/testcases/chains/0021prio_0 | 45 +
 1 file changed, 15 insertions(+), 30 deletions(-)

diff --git a/tests/shell/testcases/chains/0021prio_0 
b/tests/shell/testcases/chains/0021prio_0
index b6647ac24ad10..82f52e33cc2af 100755
--- a/tests/shell/testcases/chains/0021prio_0
+++ b/tests/shell/testcases/chains/0021prio_0
@@ -3,7 +3,7 @@
 set -e
 
 format_offset () {
-   i=$1
+   local i=$1
if ((i == 0))
then
echo ""
@@ -16,21 +16,21 @@ format_offset () {
 }
 
 chainname () {
-   hook=$1
-   prioname=$2
-   priooffset=$3
+   local hook=$1
+   local prioname=$2
+   local priooffset=$3
 
echo "${hook}${prioname}${priooffset}" | tr "\-+" "mp"
 }
 
 gen_chains () {
-   family=$1
-   hook=$2
-   prioname=$3
+   local family=$1
+   local hook=$2
+   local prioname=$3
 
for i in -11 -10 0 10 11
do
-   offset=`format_offset $i`
+   local offset=`format_offset $i`
$NFT add chain $family x `chainname $hook $prioname $offset` "{ 
type filter hook $hook priority $prioname $offset; }"
done
 }
@@ -46,13 +46,8 @@ do
done
done
 
-   hook=prerouting
-   prioname=dstnat
-   gen_chains $family $hook $prioname
-
-   hook=postrouting
-   prioname=srcnat
-   gen_chains $family $hook $prioname
+   gen_chains $family prerouting dstnat
+   gen_chains $family postrouting srcnat
 done
 
 
@@ -60,8 +55,7 @@ family=arp
 $NFT add table $family x
 for hook in input output
 do
-   prioname=filter
-   gen_chains $family $hook $prioname
+   gen_chains $family $hook filter
 done
 
 
@@ -79,18 +73,9 @@ family=bridge
 $NFT add table $family x
 for hook in prerouting input forward output postrouting
 do
-   prioname=filter
-   gen_chains $family $hook $prioname
+   gen_chains $family $hook filter
 done
 
-hook=prerouting
-prioname=dstnat
-gen_chains $family $hook $prioname
-
-hook=output
-prioname=out
-gen_chains $family $hook $prioname
-
-hook=postrouting
-prioname=srcnat
-gen_chains $family $hook $prioname
+gen_chains $family prerouting dstnat
+gen_chains $family output out
+gen_chains $family postrouting srcnat
-- 
2.19.0

[nft PATCH 1/5] parser_bison: Fix for chain prio name 'out'

[Phil Sutter]

Since 'out' is defined as a keyword in scanner.l, using it as a chain
priority name without quotes is not possible. Fix this by introducing
'extended_prio_name' in bison which may be either a string (as before)
or OUT, which is then converted into a string.

Fixes: c8a0e8c90e2d1 ("src: Set/print standard chain prios with textual names")
Signed-off-by: Phil Sutter 
---
 src/parser_bison.y | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 1c68b4f4420e7..831090b66e8ec 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -541,6 +541,8 @@ int nft_lex(void *, void *, void *);
 %destructor { handle_free(&$$); } set_spec setid_spec set_identifier obj_spec 
objid_spec obj_identifier
 %type family_spec family_spec_explicit chain_policy 
int_num
 %type   extended_prio_spec prio_spec
+%type  extended_prio_name
+%destructor { xfree($$); } extended_prio_name
 
 %type  dev_spec quota_unit
 %destructor { xfree($$); } dev_spec quota_unit
@@ -1861,26 +1863,33 @@ prio_spec   :   PRIORITY 
extended_prio_spec
}
;
 
+extended_prio_name :   OUT
+   {
+   $$ = strdup("out");
+   }
+   |   STRING
+   ;
+
 extended_prio_spec :   int_num
{
struct prio_spec spec = {0};
spec.num = $1;
$$ = spec;
}
-   |   STRING
+   |   extended_prio_name
{
struct prio_spec spec = {0};
spec.str = $1;
$$ = spec;
}
-   |   STRING PLUS NUM
+   |   extended_prio_name PLUS NUM
{
struct prio_spec spec = {0};
spec.num = $3;
spec.str = $1;
$$ = spec;
}
-   |   STRING DASH NUM
+   |   extended_prio_name DASH NUM
{
struct prio_spec spec = {0};
spec.num = -$3;
-- 
2.19.0

[nft PATCH 0/5] Fix and improve for 0021prio_0 in tests/shell

[Phil Sutter]

Patch 1 contains a fix for parser_bison.y to accept 'out' as priority
name (again a keyword vs. string issue).

The remaining patches deal with flaws in the test case itself, including
the whopping 38s it took to complete on my testing VM.

Phil Sutter (5):
  parser_bison: Fix for chain prio name 'out'
  tests: shell: Fix indenting in 0021prio_0
  tests: shell: Drop one-time use variables in 0021prio_0
  tests: shell: Improve gen_chains() in 0021prio_0
  tests: shell: Improve performance of 0021prio_0

 src/parser_bison.y  | 15 +++-
 tests/shell/testcases/chains/0021prio_0 | 91 +++--
 2 files changed, 51 insertions(+), 55 deletions(-)

-- 
2.19.0

[libnftnl PATCH] expr: xfrm: Fix for unused variable warning

[Phil Sutter]

In nftnl_expr_xfrm_json_parse(), variable 'spnum' is apparently unused
so remove it.

Fixes: f4621a6f87064 ("expr: add xfrm support")
Signed-off-by: Phil Sutter 
---
 src/expr/xfrm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/expr/xfrm.c b/src/expr/xfrm.c
index b159d60d1e3fd..03dd771052d26 100644
--- a/src/expr/xfrm.c
+++ b/src/expr/xfrm.c
@@ -204,7 +204,7 @@ static int nftnl_expr_xfrm_json_parse(struct nftnl_expr *e, 
json_t *root,
 {
 #ifdef JSON_PARSING
const char *key_str, *dir_str;
-   uint32_t reg, key, spnum;
+   uint32_t reg, key;
uint8_t dir;
 
if (nftnl_jansson_parse_reg(root, "dreg", NFTNL_TYPE_U32, , err) == 
0)
-- 
2.19.0

[iptables PATCH 0/6] Follow-up to covscan fixes

[Phil Sutter]

I reviewed the previously rejected changes in "Sanitize calls to
strcpy()" again and found merely two valid ones:

* Copying from 'real_name' of matches/targets: Length of that field is
  not checked xtables_register_* functions, so it's length may be
  arbitrary. Patch 1 of this series adds the missing check.

* In libiptc, a chain name is copied from a larger array to a shorter
  one without checking. This is fixed by patch 2.

The remaining patches in this series are fall-out from the above.

Phil Sutter (6):
  libxtables: Check extension real_name length
  libiptc: NULL-terminate errorname
  Combine command_match() implementations
  Combine parse_target() and command_jump() implementations
  arptables: Use the shared nft_ipv46_parse_target()
  nft-shared: Use xtables_calloc()

 include/xtables.h  |   1 +
 iptables/ip6tables.c   | 101 ++-
 iptables/iptables.c| 106 ++---
 iptables/nft-arp.c |   9 +---
 iptables/nft-shared.c  |  13 +
 iptables/xshared.c | 101 +++
 iptables/xshared.h |   4 ++
 iptables/xtables-arp.c |  60 +--
 iptables/xtables.c | 104 ++--
 libiptc/libiptc.c  |   3 +-
 libxtables/xtables.c   |  12 +
 11 files changed, 137 insertions(+), 377 deletions(-)

-- 
2.19.0

[iptables PATCH 6/6] nft-shared: Use xtables_calloc()

[Phil Sutter]

This simplifies code a bit since it takes care of checking for
out-of-memory conditions.

Signed-off-by: Phil Sutter 
---
 iptables/nft-shared.c | 13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index fdd4522ce24f4..492e4ec124a79 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -319,11 +319,7 @@ void nft_parse_target(struct nft_xt_ctx *ctx, struct 
nftnl_expr *e)
 
size = XT_ALIGN(sizeof(struct xt_entry_target)) + tg_len;
 
-   t = calloc(1, size);
-   if (t == NULL) {
-   fprintf(stderr, "OOM");
-   exit(EXIT_FAILURE);
-   }
+   t = xtables_calloc(1, size);
memcpy(>data, targinfo, tg_len);
t->u.target_size = size;
t->u.user.revision = nftnl_expr_get_u32(e, NFTNL_EXPR_TG_REV);
@@ -361,12 +357,7 @@ void nft_parse_match(struct nft_xt_ctx *ctx, struct 
nftnl_expr *e)
if (match == NULL)
return;
 
-   m = calloc(1, sizeof(struct xt_entry_match) + mt_len);
-   if (m == NULL) {
-   fprintf(stderr, "OOM");
-   exit(EXIT_FAILURE);
-   }
-
+   m = xtables_calloc(1, sizeof(struct xt_entry_match) + mt_len);
memcpy(>data, mt_info, mt_len);
m->u.match_size = mt_len + XT_ALIGN(sizeof(struct xt_entry_match));
m->u.user.revision = nftnl_expr_get_u32(e, NFTNL_EXPR_TG_REV);
-- 
2.19.0

[iptables PATCH 1/6] libxtables: Check extension real_name length

[Phil Sutter]

Just like with 'name', if given check 'real_name' to not exceed max
length.

Signed-off-by: Phil Sutter 
---
 libxtables/xtables.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 6dd0b152dfecf..34a084f47c290 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -920,6 +920,12 @@ void xtables_register_match(struct xtables_match *me)
exit(1);
}
 
+   if (me->real_name && strlen(me->real_name) >= XT_EXTENSION_MAXNAMELEN) {
+   fprintf(stderr, "%s: match `%s' has invalid real name\n",
+   xt_params->program_name, me->real_name);
+   exit(1);
+   }
+
if (me->family >= NPROTO) {
fprintf(stderr,
"%s: BUG: match %s has invalid protocol family\n",
@@ -1107,6 +1113,12 @@ void xtables_register_target(struct xtables_target *me)
exit(1);
}
 
+   if (me->real_name && strlen(me->real_name) >= XT_EXTENSION_MAXNAMELEN) {
+   fprintf(stderr, "%s: target `%s' has invalid real name\n",
+   xt_params->program_name, me->real_name);
+   exit(1);
+   }
+
if (me->family >= NPROTO) {
fprintf(stderr,
"%s: BUG: target %s has invalid protocol family\n",
-- 
2.19.0

[iptables PATCH 3/6] Combine command_match() implementations

[Phil Sutter]

This merges the basically identical implementations of command_match()
from xtables, iptables and ip6tables into one. The only required
adjustment was to make use of xt_params instead of the different
*_globals objects.

Signed-off-by: Phil Sutter 
---
 iptables/ip6tables.c | 35 ---
 iptables/iptables.c  | 37 -
 iptables/xshared.c   | 38 ++
 iptables/xshared.h   |  2 ++
 iptables/xtables.c   | 36 
 5 files changed, 40 insertions(+), 108 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index f447bc7474c25..1137256a259ca 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -1261,41 +1261,6 @@ static void command_jump(struct iptables_command_state 
*cs)
xtables_error(OTHER_PROBLEM, "can't alloc memory!");
 }
 
-static void command_match(struct iptables_command_state *cs)
-{
-   struct xtables_match *m;
-   size_t size;
-
-   if (cs->invert)
-   xtables_error(PARAMETER_PROBLEM,
-  "unexpected ! flag before --match");
-
-   m = xtables_find_match(optarg, XTF_LOAD_MUST_SUCCEED, >matches);
-   size = XT_ALIGN(sizeof(struct xt_entry_match)) + m->size;
-   m->m = xtables_calloc(1, size);
-   m->m->u.match_size = size;
-   if (m->real_name == NULL) {
-   strcpy(m->m->u.user.name, m->name);
-   } else {
-   strcpy(m->m->u.user.name, m->real_name);
-   if (!(m->ext_flags & XTABLES_EXT_ALIAS))
-   fprintf(stderr, "Notice: The %s match is converted into 
%s match "
-   "in rule listing and saving.\n", m->name, 
m->real_name);
-   }
-   m->m->u.user.revision = m->revision;
-
-   xs_init_match(m);
-   if (m == m->next)
-   return;
-   /* Merge options for non-cloned matches */
-   if (m->x6_options != NULL)
-   opts = xtables_options_xfrm(ip6tables_globals.orig_opts, opts,
-   m->x6_options, >option_offset);
-   else if (m->extra_opts != NULL)
-   opts = xtables_merge_options(ip6tables_globals.orig_opts, opts,
-m->extra_opts, >option_offset);
-}
-
 int do_command6(int argc, char *argv[], char **table,
struct xtc_handle **handle, bool restore)
 {
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 144550fcc92ad..70ba67c9a9701 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -1254,43 +1254,6 @@ static void command_jump(struct iptables_command_state 
*cs)
xtables_error(OTHER_PROBLEM, "can't alloc memory!");
 }
 
-static void command_match(struct iptables_command_state *cs)
-{
-   struct xtables_match *m;
-   size_t size;
-
-   if (cs->invert)
-   xtables_error(PARAMETER_PROBLEM,
-  "unexpected ! flag before --match");
-
-   m = xtables_find_match(optarg, XTF_LOAD_MUST_SUCCEED, >matches);
-   size = XT_ALIGN(sizeof(struct xt_entry_match)) + m->size;
-   m->m = xtables_calloc(1, size);
-   m->m->u.match_size = size;
-   if (m->real_name == NULL) {
-   strcpy(m->m->u.user.name, m->name);
-   } else {
-   strcpy(m->m->u.user.name, m->real_name);
-   if (!(m->ext_flags & XTABLES_EXT_ALIAS))
-   fprintf(stderr, "Notice: the %s match is converted into 
%s match "
-   "in rule listing and saving.\n", m->name, 
m->real_name);
-   }
-   m->m->u.user.revision = m->revision;
-
-   xs_init_match(m);
-   if (m == m->next)
-   return;
-   /* Merge options for non-cloned matches */
-   if (m->x6_options != NULL)
-   opts = xtables_options_xfrm(iptables_globals.orig_opts, opts,
-   m->x6_options, >option_offset);
-   else if (m->extra_opts != NULL)
-   opts = xtables_merge_options(iptables_globals.orig_opts, opts,
-m->extra_opts, >option_offset);
-   if (opts == NULL)
-   xtables_error(OTHER_PROBLEM, "can't alloc memory!");
-}
-
 int do_command4(int argc, char *argv[], char **table,
struct xtc_handle **handle, bool restore)
 {
diff --git a/iptables/xshared.c b/iptables/xshared.c
index a10e425c383a2..860373cb2db84 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -593,3 +593,41 @@ void print_ifaces(const char *iniface, const char 
*outiface, uint8_t invflags,
 
printf(FMT("%-6s &

[iptables PATCH 4/6] Combine parse_target() and command_jump() implementations

[Phil Sutter]

Merge these two functions from xtables, iptables, ip6tables and
arptables. Both functions were basically identical in the first three,
only the last one required a bit more attention.

To eliminate access to 'invflags' in variant-specific location, move the
call to set_option() into callers. This is actually consistent with
parsing of other options in them.

As with command_match(), use xt_params instead of the different
*_globals objects to refer to 'opts' and 'orig_opts'.

It was necessary to rename parse_target() as it otherwise clashes with a
static function of same name in libxt_SET.

In arptables, the maximum allowed target name is a bit larger, so
introduce xtables_globals.target_maxnamelen defining the value. It is
used in the shared xt_parse_target() implementation.

Implementation of command_jump() in arptables diverted from the others
for no obvious reason. The call to parse_target() was done outside of it
and a pointer to cs->arp was passed but not used inside.

Signed-off-by: Phil Sutter 
---
 include/xtables.h  |  1 +
 iptables/ip6tables.c   | 66 +++-
 iptables/iptables.c| 69 +++---
 iptables/xshared.c | 63 ++
 iptables/xshared.h |  2 ++
 iptables/xtables-arp.c | 60 ++--
 iptables/xtables.c | 68 +++--
 7 files changed, 80 insertions(+), 249 deletions(-)

diff --git a/include/xtables.h b/include/xtables.h
index bf169b08186f5..fd9e6c33516bc 100644
--- a/include/xtables.h
+++ b/include/xtables.h
@@ -424,6 +424,7 @@ struct xtables_globals
struct option *opts;
void (*exit_err)(enum xtables_exittype status, const char *msg, ...) 
__attribute__((noreturn, format(printf,2,3)));
int (*compat_rev)(const char *name, uint8_t rev, int opt);
+   size_t target_maxnamelen;
 };
 
 #define XT_GETOPT_TABLEEND {.name = NULL, .has_arg = false}
diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 1137256a259ca..7a9cd643f8e76 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -125,6 +125,7 @@ struct xtables_globals ip6tables_globals = {
.orig_opts = original_opts,
.exit_err = ip6tables_exit_error,
.compat_rev = xtables_compatible_revision,
+   .target_maxnamelen = XT_EXTENSION_MAXNAMELEN,
 };
 
 /* Table of legal combinations of commands and options.  If any of the
@@ -420,27 +421,6 @@ parse_chain(const char *chainname)
   "Invalid chain name `%s'", chainname);
 }
 
-static const char *
-parse_target(const char *targetname)
-{
-   const char *ptr;
-
-   if (strlen(targetname) < 1)
-   xtables_error(PARAMETER_PROBLEM,
-  "Invalid target name (too short)");
-
-   if (strlen(targetname) >= XT_EXTENSION_MAXNAMELEN)
-   xtables_error(PARAMETER_PROBLEM,
-  "Invalid target name `%s' (%u chars max)",
-  targetname, XT_EXTENSION_MAXNAMELEN - 1);
-
-   for (ptr = targetname; *ptr; ptr++)
-   if (isspace(*ptr))
-   xtables_error(PARAMETER_PROBLEM,
-  "Invalid target name `%s'", targetname);
-   return targetname;
-}
-
 static void
 set_option(unsigned int *options, unsigned int option, uint8_t *invflg,
   int invert)
@@ -1221,46 +1201,6 @@ generate_entry(const struct ip6t_entry *fw,
return e;
 }
 
-static void command_jump(struct iptables_command_state *cs)
-{
-   size_t size;
-
-   set_option(>options, OPT_JUMP, >fw6.ipv6.invflags, cs->invert);
-   cs->jumpto = parse_target(optarg);
-   /* TRY_LOAD (may be chain name) */
-   cs->target = xtables_find_target(cs->jumpto, XTF_TRY_LOAD);
-
-   if (cs->target == NULL)
-   return;
-
-   size = XT_ALIGN(sizeof(struct xt_entry_target)) + cs->target->size;
-
-   cs->target->t = xtables_calloc(1, size);
-   cs->target->t->u.target_size = size;
-   if (cs->target->real_name == NULL) {
-   strcpy(cs->target->t->u.user.name, cs->jumpto);
-   } else {
-   strcpy(cs->target->t->u.user.name, cs->target->real_name);
-   if (!(cs->target->ext_flags & XTABLES_EXT_ALIAS))
-   fprintf(stderr, "Notice: The %s target is converted 
into %s target "
-   "in rule listing and saving.\n",
-   cs->jumpto, cs->target->real_name);
-   }
-   cs->target->t->u.user.revision = cs->target->revision;
-
-   xs_init_target(cs->target);
-   if (cs->target->x6_options != NULL)
-   opts = xtables_options_xfrm(ip6tables_globals.

[iptables PATCH 5/6] arptables: Use the shared nft_ipv46_parse_target()

[Phil Sutter]

No point in having a dedicated implementation for 'parse_target'
callback since it is identical with the shared one.

Signed-off-by: Phil Sutter 
---
 iptables/nft-arp.c | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index a2109c608670d..bd78a8669bb9a 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -271,13 +271,6 @@ static void nft_arp_parse_meta(struct nft_xt_ctx *ctx, 
struct nftnl_expr *e,
fw->arp.invflags |= ipt_to_arpt_flags(flags);
 }
 
-static void nft_arp_parse_target(struct xtables_target *target, void *data)
-{
-   struct iptables_command_state *cs = data;
-
-   cs->target = target;
-}
-
 static void nft_arp_parse_immediate(const char *jumpto, bool nft_goto,
void *data)
 {
@@ -690,5 +683,5 @@ struct nft_family_ops nft_family_ops_arp = {
.rule_to_cs = nft_arp_rule_to_cs,
.clear_cs   = nft_clear_iptables_command_state,
.rule_find  = nft_arp_rule_find,
-   .parse_target   = nft_arp_parse_target,
+   .parse_target   = nft_ipv46_parse_target,
 };
-- 
2.19.0

Re: [iptables PATCH 20/28] Sanitize calls to strcpy()

[Phil Sutter]

Hi Florian,

On Mon, Sep 24, 2018 at 11:11:59AM +0200, Florian Westphal wrote:
> Phil Sutter  wrote:
> > Make sure destination buffers are NULL-terminated by replacing strcpy()
> > with strncat() (if destination is guaranteed to be zeroed) or explicitly
> > set last byte in buffer to zero.
> 
> I'm sorry, but i don't like this at all.
> 
> > -   strcpy(cs->target->t->u.user.name, cs->jumpto);
> > +   strncat(cs->target->t->u.user.name, cs->jumpto,
> > +   XT_EXTENSION_MAXNAMELEN - 1);
> 
> This reads "append this to user.name", even though this is
> supposed to copy.

Yes, admittedly this is a bit of strncat() misuse simply because it
always appends the terminating 0.

> I realize user.name is 0-terminated, but this is yet one
> more thing one "has to know".

Well, cs->target->t was allocated using calloc() a few lines above so
the whole buffer is zeroed. Otherwise this might really do the wrong
thing. But yes, it's something one might overlook while refactoring
these match creators (which occur in nearly identical form all over the
code).

> So, this should either be
>  strcpy (no change)
>  strncpy + setting last element to 0,
>  snprintf()
> 
> I think use of *cat() should be limited to cases where
> we want to append, not to work around warnings.

I don't like the first option since it requires to make sure input
really can't be too long. Whenever I do the second one (which I actually
did at first), I'm tempted to write a macro to combine the two actions.
I would like to do one of:

- use snprintf(),
- use strlcpy() from libbsd or
- introduce a poor-man's strlcpy() macro/function.

What would you prefer? Leave everything as-is, one of the above or
something completely different? :)

Thanks, Phil

How to contribute to netfilter.org/documentation?

[Phil Sutter]

Hi,

I have a minor correction for the REDIRECT explanation in[1]. Instead
of:

| is exactly equivalent to doing DNAT to the address of

it should read:

| is exactly equivalent to doing DNAT to the primary address of

Is there a repository I could send a patch for?

Thanks, Phil

[1] https://netfilter.org/documentation/HOWTO/NAT-HOWTO-6.html#ss6.2