[iptables PATCH v4 0/8] Improve iptables-nft performance with large rulesets

[iptables PATCH v4 0/8] Improve iptables-nft performance with large rulesets

[Phil Sutter]

Fourth try at caching optimizations implementation.

Changes since v3:

* Rebase onto current master after pushing the accepted initial three
  patches.
* Avoid cache inconsistency in __nft_build_cache() if kernel ruleset
  changed since last call.

Phil Sutter (8):
  nft-cache: Introduce cache levels
  nft-cache: Fetch only chains in nft_chain_list_get()
  nft-cache: Cover for multiple fetcher invocation
  nft-cache: Support partial cache per table
  nft-cache: Support partial rule cache per chain
  nft: Reduce cache overhead of nft_chain_builtin_init()
  nft: Support nft_is_table_compatible() per chain
  nft: Optimize flushing all chains of a table

 iptables/nft-cache.c       | 203 ++++++++++++++++++++++++++++++-------
 iptables/nft-cache.h       |   9 +-
 iptables/nft.c             | 108 +++++++++++++-------
 iptables/nft.h             |  14 ++-
 iptables/xtables-restore.c |   4 +-
 iptables/xtables-save.c    |   4 +-
 6 files changed, 259 insertions(+), 83 deletions(-)

-- 
2.23.0

[iptables PATCH v4 1/8] nft-cache: Introduce cache levels

[Phil Sutter]

Replace the simple have_cache boolean by a cache level indicator
defining how complete the cache is. Since have_cache indicated full
cache (including rules), make code depending on it check for cache level
NFT_CL_RULES.

Core cache fetching routine __nft_build_cache() accepts a new level via
parameter and raises cache completeness to that level.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v3:
- Make sure cache stays consistent in __nft_build_cache() by extending
  cache only if kernel ruleset did not change since last call.
---
 iptables/nft-cache.c | 54 ++++++++++++++++++++++++++++++++------------
 iptables/nft.h       |  9 +++++++-
 2 files changed, 47 insertions(+), 16 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 5444419a5cc3b..04f42e0f8ad35 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -224,30 +224,52 @@ static int fetch_rule_cache(struct nft_handle *h)
 	return 0;
 }
 
-static void __nft_build_cache(struct nft_handle *h)
+static void __nft_build_cache(struct nft_handle *h, enum nft_cache_level level)
 {
 	uint32_t genid_start, genid_stop;
 
+	if (level <= h->cache_level)
+		return;
 retry:
 	mnl_genid_get(h, &genid_start);
-	fetch_table_cache(h);
-	fetch_chain_cache(h);
-	fetch_rule_cache(h);
-	h->have_cache = true;
-	mnl_genid_get(h, &genid_stop);
 
+	if (h->cache_level && genid_start != h->nft_genid)
+		flush_chain_cache(h, NULL);
+
+	switch (h->cache_level) {
+	case NFT_CL_NONE:
+		fetch_table_cache(h);
+		if (level == NFT_CL_TABLES)
+			break;
+		/* fall through */
+	case NFT_CL_TABLES:
+		fetch_chain_cache(h);
+		if (level == NFT_CL_CHAINS)
+			break;
+		/* fall through */
+	case NFT_CL_CHAINS:
+		fetch_rule_cache(h);
+		if (level == NFT_CL_RULES)
+			break;
+		/* fall through */
+	case NFT_CL_RULES:
+		break;
+	}
+
+	mnl_genid_get(h, &genid_stop);
 	if (genid_start != genid_stop) {
 		flush_chain_cache(h, NULL);
 		goto retry;
 	}
 
+	h->cache_level = level;
 	h->nft_genid = genid_start;
 }
 
 void nft_build_cache(struct nft_handle *h)
 {
-	if (!h->have_cache)
-		__nft_build_cache(h);
+	if (h->cache_level < NFT_CL_RULES)
+		__nft_build_cache(h, NFT_CL_RULES);
 }
 
 void nft_fake_cache(struct nft_handle *h)
@@ -263,7 +285,7 @@ void nft_fake_cache(struct nft_handle *h)
 
 		h->cache->table[type].chains = nftnl_chain_list_alloc();
 	}
-	h->have_cache = true;
+	h->cache_level = NFT_CL_RULES;
 	mnl_genid_get(h, &h->nft_genid);
 }
 
@@ -331,19 +353,22 @@ static int flush_cache(struct nft_handle *h, struct nft_cache *c,
 
 void flush_chain_cache(struct nft_handle *h, const char *tablename)
 {
-	if (!h->have_cache)
+	if (!h->cache_level)
 		return;
 
 	if (flush_cache(h, h->cache, tablename))
-		h->have_cache = false;
+		h->cache_level = NFT_CL_NONE;
 }
 
 void nft_rebuild_cache(struct nft_handle *h)
 {
-	if (h->have_cache)
+	enum nft_cache_level level = h->cache_level;
+
+	if (h->cache_level)
 		__nft_flush_cache(h);
 
-	__nft_build_cache(h);
+	h->cache_level = NFT_CL_NONE;
+	__nft_build_cache(h, level);
 }
 
 void nft_release_cache(struct nft_handle *h)
@@ -354,8 +379,7 @@ void nft_release_cache(struct nft_handle *h)
 
 struct nftnl_table_list *nftnl_table_list_get(struct nft_handle *h)
 {
-	if (!h->cache->tables)
-		fetch_table_cache(h);
+	__nft_build_cache(h, NFT_CL_TABLES);
 
 	return h->cache->tables;
 }
diff --git a/iptables/nft.h b/iptables/nft.h
index 451c266016d8d..9ae3122a1c515 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -27,6 +27,13 @@ struct builtin_table {
 	struct builtin_chain chains[NF_INET_NUMHOOKS];
 };
 
+enum nft_cache_level {
+	NFT_CL_NONE,
+	NFT_CL_TABLES,
+	NFT_CL_CHAINS,
+	NFT_CL_RULES
+};
+
 struct nft_cache {
 	struct nftnl_table_list		*tables;
 	struct {
@@ -53,7 +60,7 @@ struct nft_handle {
 	unsigned int		cache_index;
 	struct nft_cache	__cache[2];
 	struct nft_cache	*cache;
-	bool			have_cache;
+	enum nft_cache_level	cache_level;
 	bool			restore;
 	bool			noflush;
 	int8_t			config_done;
-- 
2.23.0

Re: [iptables PATCH v4 1/8] nft-cache: Introduce cache levels

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 01:41:45PM +0200, Phil Sutter wrote:
> Replace the simple have_cache boolean by a cache level indicator
> defining how complete the cache is. Since have_cache indicated full
> cache (including rules), make code depending on it check for cache level
> NFT_CL_RULES.
> 
> Core cache fetching routine __nft_build_cache() accepts a new level via
> parameter and raises cache completeness to that level.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

[iptables PATCH v4 2/8] nft-cache: Fetch only chains in nft_chain_list_get()

[Phil Sutter]

The function is used to return the given table's chains, so fetching
chain cache is enough.

Add calls to nft_build_cache() in places where a rule cache is required.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c |  2 +-
 iptables/nft.c       | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 04f42e0f8ad35..22468d70fec57 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -393,7 +393,7 @@ struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
 	if (!t)
 		return NULL;
 
-	nft_build_cache(h);
+	__nft_build_cache(h, NFT_CL_CHAINS);
 
 	return h->cache->table[t->type].chains;
 }
diff --git a/iptables/nft.c b/iptables/nft.c
index 81de10d8a0892..94fabd78e527e 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1173,6 +1173,14 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
 
 	nft_xt_builtin_init(h, table);
 
+	/* Since ebtables user-defined chain policies are implemented as last
+	 * rule in nftables, rule cache is required here to treat them right. */
+	if (h->family == NFPROTO_BRIDGE) {
+		c = nft_chain_find(h, table, chain);
+		if (c && !nft_chain_builtin(c))
+			nft_build_cache(h);
+	}
+
 	nft_fn = nft_rule_append;
 
 	r = nft_rule_new(h, chain, table, data);
@@ -1397,6 +1405,8 @@ int nft_rule_save(struct nft_handle *h, const char *table, unsigned int format)
 	struct nftnl_chain *c;
 	int ret = 0;
 
+	nft_build_cache(h);
+
 	list = nft_chain_list_get(h, table);
 	if (!list)
 		return 0;
@@ -1595,6 +1605,10 @@ static int __nft_chain_user_del(struct nftnl_chain *c, void *data)
 		fprintf(stdout, "Deleting chain `%s'\n",
 			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME));
 
+	/* This triggers required policy rule deletion. */
+	if (h->family == NFPROTO_BRIDGE)
+		nft_build_cache(h);
+
 	/* XXX This triggers a fast lookup from the kernel. */
 	nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE);
 	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_DEL, c);
@@ -1876,6 +1890,8 @@ nft_rule_find(struct nft_handle *h, struct nftnl_chain *c, void *data, int rulen
 	struct nftnl_rule_iter *iter;
 	bool found = false;
 
+	nft_build_cache(h);
+
 	if (rulenum >= 0)
 		/* Delete by rule number case */
 		return nftnl_rule_lookup_byindex(c, rulenum);
@@ -2701,6 +2717,8 @@ int ebt_set_user_chain_policy(struct nft_handle *h, const char *table,
 	else
 		return 0;
 
+	nft_build_cache(h);
+
 	nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, pval);
 	return 1;
 }
@@ -3038,6 +3056,8 @@ static int nft_is_chain_compatible(struct nftnl_chain *c, void *data)
 	enum nf_inet_hooks hook;
 	int prio;
 
+	nft_build_cache(h);
+
 	if (nftnl_rule_foreach(c, nft_is_rule_compatible, NULL))
 		return -1;
 
-- 
2.23.0

[iptables PATCH v4 3/8] nft-cache: Cover for multiple fetcher invocation

[Phil Sutter]

Preparing for partial caches, it is necessary to make sure these
functions don't cause harm if called repeatedly.

* Use h->cache->tables pointer as indicator for existing table cache,
  return immediately from fetch_table_cache() if non-NULL.

* Initialize table's chain list only if non-NULL.

* Search for chain in table's chain list before adding it.

* Don't fetch rules for a chain if it has any rules already. With rule
  list being embedded in struct nftnl_chain, this is the best way left
  to check if rules have been fetched already or not. It will fail for
  empty chains, but causes no harm in that case, either.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 22468d70fec57..afb2126b51495 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -86,6 +86,9 @@ static int fetch_table_cache(struct nft_handle *h)
 	struct nftnl_table_list *list;
 	int ret;
 
+	if (h->cache->tables)
+		return 0;
+
 	list = nftnl_table_list_alloc();
 	if (list == NULL)
 		return 0;
@@ -106,7 +109,9 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 {
 	struct nft_handle *h = data;
 	const struct builtin_table *t;
+	struct nftnl_chain_list *list;
 	struct nftnl_chain *c;
+	const char *cname;
 
 	c = nftnl_chain_alloc();
 	if (c == NULL)
@@ -120,7 +125,13 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 	if (!t)
 		goto out;
 
-	nftnl_chain_list_add_tail(c, h->cache->table[t->type].chains);
+	list = h->cache->table[t->type].chains;
+	cname = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
+
+	if (nftnl_chain_list_lookup_byname(list, cname))
+		goto out;
+
+	nftnl_chain_list_add_tail(c, list);
 
 	return MNL_CB_OK;
 out:
@@ -141,6 +152,9 @@ static int fetch_chain_cache(struct nft_handle *h)
 		if (!h->tables[i].name)
 			continue;
 
+		if (h->cache->table[type].chains)
+			continue;
+
 		h->cache->table[type].chains = nftnl_chain_list_alloc();
 		if (!h->cache->table[type].chains)
 			return -1;
@@ -182,6 +196,9 @@ static int nft_rule_list_update(struct nftnl_chain *c, void *data)
 	struct nftnl_rule *rule;
 	int ret;
 
+	if (nftnl_rule_lookup_byindex(c, 0))
+		return 0;
+
 	rule = nftnl_rule_alloc();
 	if (!rule)
 		return -1;
-- 
2.23.0

[iptables PATCH v4 4/8] nft-cache: Support partial cache per table

[Phil Sutter]

Accept a builtin_table pointer in __nft_build_cache() and pass it along
when fetching chains and rules to operate on that table only (unless the
pointer is NULL).

Make use of it in nft_chain_list_get() since that accepts a table name
and performs a builtin table lookup internally already.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c | 82 ++++++++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 25 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index afb2126b51495..822d6f20cf51c 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -11,6 +11,7 @@
 
 #include <assert.h>
 #include <errno.h>
+#include <string.h>
 #include <xtables.h>
 
 #include <linux/netfilter/nf_tables.h>
@@ -105,13 +106,19 @@ static int fetch_table_cache(struct nft_handle *h)
 	return 1;
 }
 
+struct nftnl_chain_list_cb_data {
+	struct nft_handle *h;
+	const struct builtin_table *t;
+};
+
 static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 {
-	struct nft_handle *h = data;
-	const struct builtin_table *t;
+	struct nftnl_chain_list_cb_data *d = data;
+	const struct builtin_table *t = d->t;
 	struct nftnl_chain_list *list;
+	struct nft_handle *h = d->h;
+	const char *tname, *cname;
 	struct nftnl_chain *c;
-	const char *cname;
 
 	c = nftnl_chain_alloc();
 	if (c == NULL)
@@ -120,10 +127,15 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 	if (nftnl_chain_nlmsg_parse(nlh, c) < 0)
 		goto out;
 
-	t = nft_table_builtin_find(h,
-			nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE));
-	if (!t)
+	tname = nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);
+
+	if (!t) {
+		t = nft_table_builtin_find(h, tname);
+		if (!t)
+			goto out;
+	} else if (strcmp(t->name, tname)) {
 		goto out;
+	}
 
 	list = h->cache->table[t->type].chains;
 	cname = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
@@ -140,30 +152,41 @@ err:
 	return MNL_CB_OK;
 }
 
-static int fetch_chain_cache(struct nft_handle *h)
+static int fetch_chain_cache(struct nft_handle *h,
+			     const struct builtin_table *t)
 {
+	struct nftnl_chain_list_cb_data d = {
+		.h = h,
+		.t = t,
+	};
 	char buf[16536];
 	struct nlmsghdr *nlh;
 	int i, ret;
 
-	for (i = 0; i < NFT_TABLE_MAX; i++) {
-		enum nft_table_type type = h->tables[i].type;
+	if (!t) {
+		for (i = 0; i < NFT_TABLE_MAX; i++) {
+			enum nft_table_type type = h->tables[i].type;
 
-		if (!h->tables[i].name)
-			continue;
+			if (!h->tables[i].name)
+				continue;
 
-		if (h->cache->table[type].chains)
-			continue;
+			if (h->cache->table[type].chains)
+				continue;
 
-		h->cache->table[type].chains = nftnl_chain_list_alloc();
-		if (!h->cache->table[type].chains)
+			h->cache->table[type].chains = nftnl_chain_list_alloc();
+			if (!h->cache->table[type].chains)
+				return -1;
+		}
+	} else if (!h->cache->table[t->type].chains) {
+		h->cache->table[t->type].chains = nftnl_chain_list_alloc();
+		if (!h->cache->table[t->type].chains)
 			return -1;
 	}
 
 	nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETCHAIN, h->family,
 					NLM_F_DUMP, h->seq);
 
-	ret = mnl_talk(h, nlh, nftnl_chain_list_cb, h);
+	ret = mnl_talk(h, nlh, nftnl_chain_list_cb, &d);
 	if (ret < 0 && errno == EINTR)
 		assert(nft_restart(h) >= 0);
 
@@ -224,10 +247,14 @@ static int nft_rule_list_update(struct nftnl_chain *c, void *data)
 	return 0;
 }
 
-static int fetch_rule_cache(struct nft_handle *h)
+static int fetch_rule_cache(struct nft_handle *h, const struct builtin_table *t)
 {
 	int i;
 
+	if (t)
+		return nftnl_chain_list_foreach(h->cache->table[t->type].chains,
+						nft_rule_list_update, h);
+
 	for (i = 0; i < NFT_TABLE_MAX; i++) {
 		enum nft_table_type type = h->tables[i].type;
 
@@ -241,7 +268,8 @@ static int fetch_rule_cache(struct nft_handle *h)
 	return 0;
 }
 
-static void __nft_build_cache(struct nft_handle *h, enum nft_cache_level level)
+static void __nft_build_cache(struct nft_handle *h, enum nft_cache_level level,
+			      const struct builtin_table *t)
 {
 	uint32_t genid_start, genid_stop;
 
@@ -260,12 +288,12 @@ retry:
 			break;
 		/* fall through */
 	case NFT_CL_TABLES:
-		fetch_chain_cache(h);
+		fetch_chain_cache(h, t);
 		if (level == NFT_CL_CHAINS)
 			break;
 		/* fall through */
 	case NFT_CL_CHAINS:
-		fetch_rule_cache(h);
+		fetch_rule_cache(h, t);
 		if (level == NFT_CL_RULES)
 			break;
 		/* fall through */
@@ -279,14 +307,18 @@ retry:
 		goto retry;
 	}
 
-	h->cache_level = level;
+	if (!t)
+		h->cache_level = level;
+	else if (h->cache_level < NFT_CL_TABLES)
+		h->cache_level = NFT_CL_TABLES;
+
 	h->nft_genid = genid_start;
 }
 
 void nft_build_cache(struct nft_handle *h)
 {
 	if (h->cache_level < NFT_CL_RULES)
-		__nft_build_cache(h, NFT_CL_RULES);
+		__nft_build_cache(h, NFT_CL_RULES, NULL);
 }
 
 void nft_fake_cache(struct nft_handle *h)
@@ -385,7 +417,7 @@ void nft_rebuild_cache(struct nft_handle *h)
 		__nft_flush_cache(h);
 
 	h->cache_level = NFT_CL_NONE;
-	__nft_build_cache(h, level);
+	__nft_build_cache(h, level, NULL);
 }
 
 void nft_release_cache(struct nft_handle *h)
@@ -396,7 +428,7 @@ void nft_release_cache(struct nft_handle *h)
 
 struct nftnl_table_list *nftnl_table_list_get(struct nft_handle *h)
 {
-	__nft_build_cache(h, NFT_CL_TABLES);
+	__nft_build_cache(h, NFT_CL_TABLES, NULL);
 
 	return h->cache->tables;
 }
@@ -410,7 +442,7 @@ struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
 	if (!t)
 		return NULL;
 
-	__nft_build_cache(h, NFT_CL_CHAINS);
+	__nft_build_cache(h, NFT_CL_CHAINS, t);
 
 	return h->cache->table[t->type].chains;
 }
-- 
2.23.0

[iptables PATCH v4 5/8] nft-cache: Support partial rule cache per chain

[Phil Sutter]

Accept an additional chain name pointer in __nft_build_cache() and pass
it along to fetch only that specific chain and its rules.

Enhance nft_build_cache() to take an optional nftnl_chain pointer to
fetch rules for.

Enhance nft_chain_list_get() to take an optional chain name. If cache
level doesn't include chains already, it will fetch only the specified
chain from kernel (if existing) and add that to table's chain list which
is returned. This keeps operations for all chains of a table or a
specific one within the same code path in nft.c.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c       | 76 ++++++++++++++++++++++++++++----------
 iptables/nft-cache.h       |  6 +--
 iptables/nft.c             | 35 +++++++++---------
 iptables/xtables-restore.c |  4 +-
 iptables/xtables-save.c    |  2 +-
 5 files changed, 79 insertions(+), 44 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 822d6f20cf51c..64dcda578a163 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -153,7 +153,8 @@ err:
 }
 
 static int fetch_chain_cache(struct nft_handle *h,
-			     const struct builtin_table *t)
+			     const struct builtin_table *t,
+			     const char *chain)
 {
 	struct nftnl_chain_list_cb_data d = {
 		.h = h,
@@ -183,8 +184,24 @@ static int fetch_chain_cache(struct nft_handle *h,
 			return -1;
 	}
 
-	nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETCHAIN, h->family,
-					NLM_F_DUMP, h->seq);
+	if (t && chain) {
+		struct nftnl_chain *c = nftnl_chain_alloc();
+
+		if (!c)
+			return -1;
+
+		nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETCHAIN,
+						  h->family, NLM_F_ACK,
+						  h->seq);
+		nftnl_chain_set_str(c, NFTNL_CHAIN_TABLE, t->name);
+		nftnl_chain_set_str(c, NFTNL_CHAIN_NAME, chain);
+		nftnl_chain_nlmsg_build_payload(nlh, c);
+		nftnl_chain_free(c);
+	} else {
+		nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETCHAIN,
+						  h->family, NLM_F_DUMP,
+						  h->seq);
+	}
 
 	ret = mnl_talk(h, nlh, nftnl_chain_list_cb, &d);
 	if (ret < 0 && errno == EINTR)
@@ -247,13 +264,23 @@ static int nft_rule_list_update(struct nftnl_chain *c, void *data)
 	return 0;
 }
 
-static int fetch_rule_cache(struct nft_handle *h, const struct builtin_table *t)
+static int fetch_rule_cache(struct nft_handle *h,
+			    const struct builtin_table *t, const char *chain)
 {
 	int i;
 
-	if (t)
-		return nftnl_chain_list_foreach(h->cache->table[t->type].chains,
-						nft_rule_list_update, h);
+	if (t) {
+		struct nftnl_chain_list *list;
+		struct nftnl_chain *c;
+
+		list = h->cache->table[t->type].chains;
+
+		if (chain) {
+			c = nftnl_chain_list_lookup_byname(list, chain);
+			return nft_rule_list_update(c, h);
+		}
+		return nftnl_chain_list_foreach(list, nft_rule_list_update, h);
+	}
 
 	for (i = 0; i < NFT_TABLE_MAX; i++) {
 		enum nft_table_type type = h->tables[i].type;
@@ -268,8 +295,9 @@ static int fetch_rule_cache(struct nft_handle *h, const struct builtin_table *t)
 	return 0;
 }
 
-static void __nft_build_cache(struct nft_handle *h, enum nft_cache_level level,
-			      const struct builtin_table *t)
+static void
+__nft_build_cache(struct nft_handle *h, enum nft_cache_level level,
+		  const struct builtin_table *t, const char *chain)
 {
 	uint32_t genid_start, genid_stop;
 
@@ -288,12 +316,12 @@ retry:
 			break;
 		/* fall through */
 	case NFT_CL_TABLES:
-		fetch_chain_cache(h, t);
+		fetch_chain_cache(h, t, chain);
 		if (level == NFT_CL_CHAINS)
 			break;
 		/* fall through */
 	case NFT_CL_CHAINS:
-		fetch_rule_cache(h, t);
+		fetch_rule_cache(h, t, chain);
 		if (level == NFT_CL_RULES)
 			break;
 		/* fall through */
@@ -307,7 +335,7 @@ retry:
 		goto retry;
 	}
 
-	if (!t)
+	if (!t && !chain)
 		h->cache_level = level;
 	else if (h->cache_level < NFT_CL_TABLES)
 		h->cache_level = NFT_CL_TABLES;
@@ -315,10 +343,18 @@ retry:
 	h->nft_genid = genid_start;
 }
 
-void nft_build_cache(struct nft_handle *h)
+void nft_build_cache(struct nft_handle *h, struct nftnl_chain *c)
 {
-	if (h->cache_level < NFT_CL_RULES)
-		__nft_build_cache(h, NFT_CL_RULES, NULL);
+	const struct builtin_table *t;
+	const char *table, *chain;
+
+	if (!c)
+		return __nft_build_cache(h, NFT_CL_RULES, NULL, NULL);
+
+	table = nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);
+	chain = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
+	t = nft_table_builtin_find(h, table);
+	__nft_build_cache(h, NFT_CL_RULES, t, chain);
 }
 
 void nft_fake_cache(struct nft_handle *h)
@@ -417,7 +453,7 @@ void nft_rebuild_cache(struct nft_handle *h)
 		__nft_flush_cache(h);
 
 	h->cache_level = NFT_CL_NONE;
-	__nft_build_cache(h, level, NULL);
+	__nft_build_cache(h, level, NULL, NULL);
 }
 
 void nft_release_cache(struct nft_handle *h)
@@ -428,13 +464,13 @@ void nft_release_cache(struct nft_handle *h)
 
 struct nftnl_table_list *nftnl_table_list_get(struct nft_handle *h)
 {
-	__nft_build_cache(h, NFT_CL_TABLES, NULL);
+	__nft_build_cache(h, NFT_CL_TABLES, NULL, NULL);
 
 	return h->cache->tables;
 }
 
-struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
-					    const char *table)
+struct nftnl_chain_list *
+nft_chain_list_get(struct nft_handle *h, const char *table, const char *chain)
 {
 	const struct builtin_table *t;
 
@@ -442,7 +478,7 @@ struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
 	if (!t)
 		return NULL;
 
-	__nft_build_cache(h, NFT_CL_CHAINS, t);
+	__nft_build_cache(h, NFT_CL_CHAINS, t, chain);
 
 	return h->cache->table[t->type].chains;
 }
diff --git a/iptables/nft-cache.h b/iptables/nft-cache.h
index 423c6516de9bb..793a85f453ffc 100644
--- a/iptables/nft-cache.h
+++ b/iptables/nft-cache.h
@@ -4,14 +4,14 @@
 struct nft_handle;
 
 void nft_fake_cache(struct nft_handle *h);
-void nft_build_cache(struct nft_handle *h);
+void nft_build_cache(struct nft_handle *h, struct nftnl_chain *c);
 void nft_rebuild_cache(struct nft_handle *h);
 void nft_release_cache(struct nft_handle *h);
 void flush_chain_cache(struct nft_handle *h, const char *tablename);
 void flush_rule_cache(struct nftnl_chain *c);
 
-struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
-					    const char *table);
+struct nftnl_chain_list *
+nft_chain_list_get(struct nft_handle *h, const char *table, const char *chain);
 struct nftnl_table_list *nftnl_table_list_get(struct nft_handle *h);
 
 #endif /* _NFT_CACHE_H_ */
diff --git a/iptables/nft.c b/iptables/nft.c
index 94fabd78e527e..775582aab7955 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -709,7 +709,7 @@ nft_chain_builtin_find(const struct builtin_table *t, const char *chain)
 static void nft_chain_builtin_init(struct nft_handle *h,
 				   const struct builtin_table *table)
 {
-	struct nftnl_chain_list *list = nft_chain_list_get(h, table->name);
+	struct nftnl_chain_list *list = nft_chain_list_get(h, table->name, NULL);
 	struct nftnl_chain *c;
 	int i;
 
@@ -1178,7 +1178,7 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
 	if (h->family == NFPROTO_BRIDGE) {
 		c = nft_chain_find(h, table, chain);
 		if (c && !nft_chain_builtin(c))
-			nft_build_cache(h);
+			nft_build_cache(h, c);
 	}
 
 	nft_fn = nft_rule_append;
@@ -1405,9 +1405,7 @@ int nft_rule_save(struct nft_handle *h, const char *table, unsigned int format)
 	struct nftnl_chain *c;
 	int ret = 0;
 
-	nft_build_cache(h);
-
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, NULL);
 	if (!list)
 		return 0;
 
@@ -1417,6 +1415,7 @@ int nft_rule_save(struct nft_handle *h, const char *table, unsigned int format)
 
 	c = nftnl_chain_list_iter_next(iter);
 	while (c) {
+		nft_build_cache(h, c);
 		ret = nft_chain_save_rules(h, c, format);
 		if (ret != 0)
 			break;
@@ -1468,7 +1467,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 
 	nft_fn = nft_rule_flush;
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (list == NULL) {
 		ret = 1;
 		goto err;
@@ -1533,7 +1532,7 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
 
 	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (list)
 		nftnl_chain_list_add(c, list);
 
@@ -1573,7 +1572,7 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
 
 	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (list)
 		nftnl_chain_list_add(c, list);
 
@@ -1607,7 +1606,7 @@ static int __nft_chain_user_del(struct nftnl_chain *c, void *data)
 
 	/* This triggers required policy rule deletion. */
 	if (h->family == NFPROTO_BRIDGE)
-		nft_build_cache(h);
+		nft_build_cache(h, c);
 
 	/* XXX This triggers a fast lookup from the kernel. */
 	nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE);
@@ -1632,7 +1631,7 @@ int nft_chain_user_del(struct nft_handle *h, const char *chain,
 
 	nft_fn = nft_chain_user_del;
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (list == NULL)
 		return 0;
 
@@ -1660,7 +1659,7 @@ nft_chain_find(struct nft_handle *h, const char *table, const char *chain)
 {
 	struct nftnl_chain_list *list;
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (list == NULL)
 		return NULL;
 
@@ -1890,7 +1889,7 @@ nft_rule_find(struct nft_handle *h, struct nftnl_chain *c, void *data, int rulen
 	struct nftnl_rule_iter *iter;
 	bool found = false;
 
-	nft_build_cache(h);
+	nft_build_cache(h, c);
 
 	if (rulenum >= 0)
 		/* Delete by rule number case */
@@ -2198,7 +2197,7 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 	if (!nft_is_table_compatible(h, table))
 		xtables_error(OTHER_PROBLEM, "table `%s' is incompatible, use 'nft' tool.\n", table);
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (!list)
 		return 0;
 
@@ -2299,7 +2298,7 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 	if (!nft_is_table_compatible(h, table))
 		xtables_error(OTHER_PROBLEM, "table `%s' is incompatible, use 'nft' tool.\n", table);
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (!list)
 		return 0;
 
@@ -2717,7 +2716,7 @@ int ebt_set_user_chain_policy(struct nft_handle *h, const char *table,
 	else
 		return 0;
 
-	nft_build_cache(h);
+	nft_build_cache(h, c);
 
 	nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, pval);
 	return 1;
@@ -2983,7 +2982,7 @@ int nft_chain_zero_counters(struct nft_handle *h, const char *chain,
 	struct nftnl_chain *c;
 	int ret = 0;
 
-	list = nft_chain_list_get(h, table);
+	list = nft_chain_list_get(h, table, chain);
 	if (list == NULL)
 		goto err;
 
@@ -3056,7 +3055,7 @@ static int nft_is_chain_compatible(struct nftnl_chain *c, void *data)
 	enum nf_inet_hooks hook;
 	int prio;
 
-	nft_build_cache(h);
+	nft_build_cache(h, c);
 
 	if (nftnl_rule_foreach(c, nft_is_rule_compatible, NULL))
 		return -1;
@@ -3089,7 +3088,7 @@ bool nft_is_table_compatible(struct nft_handle *h, const char *tablename)
 {
 	struct nftnl_chain_list *clist;
 
-	clist = nft_chain_list_get(h, tablename);
+	clist = nft_chain_list_get(h, tablename, NULL);
 	if (clist == NULL)
 		return false;
 
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 7641955d1ce53..4f6d324bdafe9 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -63,7 +63,7 @@ static struct nftnl_chain_list *get_chain_list(struct nft_handle *h,
 {
 	struct nftnl_chain_list *chain_list;
 
-	chain_list = nft_chain_list_get(h, table);
+	chain_list = nft_chain_list_get(h, table, NULL);
 	if (chain_list == NULL)
 		xtables_error(OTHER_PROBLEM, "cannot retrieve chain list\n");
 
@@ -100,7 +100,7 @@ void xtables_restore_parse(struct nft_handle *h,
 	if (!h->noflush)
 		nft_fake_cache(h);
 	else
-		nft_build_cache(h);
+		nft_build_cache(h, NULL);
 
 	/* Grab standard input. */
 	while (fgets(buffer, sizeof(buffer), p->in)) {
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 3741888f9af44..e234425ded293 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -83,7 +83,7 @@ __do_output(struct nft_handle *h, const char *tablename, void *data)
 		return 0;
 	}
 
-	chain_list = nft_chain_list_get(h, tablename);
+	chain_list = nft_chain_list_get(h, tablename, NULL);
 	if (!chain_list)
 		return 0;
 
-- 
2.23.0

[iptables PATCH v4 6/8] nft: Reduce cache overhead of nft_chain_builtin_init()

[Phil Sutter]

There is no need for a full chain cache, fetch only the few builtin
chains that might need to be created.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 775582aab7955..7e019d54ee475 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -709,15 +709,16 @@ nft_chain_builtin_find(const struct builtin_table *t, const char *chain)
 static void nft_chain_builtin_init(struct nft_handle *h,
 				   const struct builtin_table *table)
 {
-	struct nftnl_chain_list *list = nft_chain_list_get(h, table->name, NULL);
+	struct nftnl_chain_list *list;
 	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++) {
+		list = nft_chain_list_get(h, table->name,
+					  table->chains[i].name);
+		if (!list)
+			continue;
 
 		c = nftnl_chain_list_lookup_byname(list, table->chains[i].name);
 		if (c != NULL)
-- 
2.23.0

[iptables PATCH v4 7/8] nft: Support nft_is_table_compatible() per chain

[Phil Sutter]

When operating on a single chain only, compatibility checking causes
unwanted overhead by checking all chains of the current table. Avoid
this by accepting the current chain name as parameter and pass it along
to nft_chain_list_get().

While being at it, introduce nft_assert_table_compatible() which
calls xtables_error() in case compatibility check fails. If a chain name
was given, include that in error message.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c          | 32 ++++++++++++++++++++++++--------
 iptables/nft.h          |  5 ++++-
 iptables/xtables-save.c |  2 +-
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 7e019d54ee475..12cc423c87bbb 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2192,12 +2192,10 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 	bool found = false;
 
 	nft_xt_builtin_init(h, table);
+	nft_assert_table_compatible(h, table, chain);
 
 	ops = nft_family_ops_lookup(h->family);
 
-	if (!nft_is_table_compatible(h, table))
-		xtables_error(OTHER_PROBLEM, "table `%s' is incompatible, use 'nft' tool.\n", table);
-
 	list = nft_chain_list_get(h, table, chain);
 	if (!list)
 		return 0;
@@ -2295,9 +2293,7 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 	int ret = 0;
 
 	nft_xt_builtin_init(h, table);
-
-	if (!nft_is_table_compatible(h, table))
-		xtables_error(OTHER_PROBLEM, "table `%s' is incompatible, use 'nft' tool.\n", table);
+	nft_assert_table_compatible(h, table, chain);
 
 	list = nft_chain_list_get(h, table, chain);
 	if (!list)
@@ -3085,11 +3081,12 @@ static int nft_is_chain_compatible(struct nftnl_chain *c, void *data)
 	return 0;
 }
 
-bool nft_is_table_compatible(struct nft_handle *h, const char *tablename)
+bool nft_is_table_compatible(struct nft_handle *h,
+			     const char *table, const char *chain)
 {
 	struct nftnl_chain_list *clist;
 
-	clist = nft_chain_list_get(h, tablename, NULL);
+	clist = nft_chain_list_get(h, table, chain);
 	if (clist == NULL)
 		return false;
 
@@ -3098,3 +3095,22 @@ bool nft_is_table_compatible(struct nft_handle *h, const char *tablename)
 
 	return true;
 }
+
+void nft_assert_table_compatible(struct nft_handle *h,
+				 const char *table, const char *chain)
+{
+	const char *pfx = "", *sfx = "";
+
+	if (nft_is_table_compatible(h, table, chain))
+		return;
+
+	if (chain) {
+		pfx = "chain `";
+		sfx = "' in ";
+	} else {
+		chain = "";
+	}
+	xtables_error(OTHER_PROBLEM,
+		      "%s%s%stable `%s' is incompatible, use 'nft' tool.\n",
+		      pfx, chain, sfx, table);
+}
diff --git a/iptables/nft.h b/iptables/nft.h
index 9ae3122a1c515..4b8b3033a56c0 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -206,7 +206,10 @@ int nft_arp_rule_insert(struct nft_handle *h, const char *chain,
 
 void nft_rule_to_arpt_entry(struct nftnl_rule *r, struct arpt_entry *fw);
 
-bool nft_is_table_compatible(struct nft_handle *h, const char *name);
+bool nft_is_table_compatible(struct nft_handle *h,
+			     const char *table, const char *chain);
+void nft_assert_table_compatible(struct nft_handle *h,
+				 const char *table, const char *chain);
 
 int ebt_set_user_chain_policy(struct nft_handle *h, const char *table,
 			      const char *chain, const char *policy);
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index e234425ded293..44687f998c91a 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -77,7 +77,7 @@ __do_output(struct nft_handle *h, const char *tablename, void *data)
 	if (!nft_table_builtin_find(h, tablename))
 		return 0;
 
-	if (!nft_is_table_compatible(h, tablename)) {
+	if (!nft_is_table_compatible(h, tablename, NULL)) {
 		printf("# Table `%s' is incompatible, use 'nft' tool.\n",
 		       tablename);
 		return 0;
-- 
2.23.0

[iptables PATCH v4 8/8] nft: Optimize flushing all chains of a table

[Phil Sutter]

Leverage nftables' support for flushing all chains of a table by
omitting NFTNL_RULE_CHAIN attribute in NFT_MSG_DELRULE payload.

The only caveat is with verbose output, as that still requires to have a
list of (existing) chains to iterate over. Apart from that, implementing
this shortcut is pretty straightforward: Don't retrieve a chain list and
just call __nft_rule_flush() directly which doesn't set above attribute
if chain name pointer is NULL.

A bigger deal is keeping rule cache consistent: Instead of just clearing
rule list for each flushed chain, flush_rule_cache() is updated to
iterate over all cached chains of the given table, clearing their rule
lists if not called for a specific chain.

While being at it, sort local variable declarations in nft_rule_flush()
from longest to shortest and drop the loop-local 'chain_name' variable
(but instead use 'chain' function parameter which is not used at that
point).

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c | 22 +++++++++++++++++++---
 iptables/nft-cache.h |  3 ++-
 iptables/nft.c       | 32 ++++++++++++++++++--------------
 3 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 64dcda578a163..c55970d074a76 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -384,7 +384,7 @@ static void __nft_flush_cache(struct nft_handle *h)
 	}
 }
 
-static int __flush_rule_cache(struct nftnl_rule *r, void *data)
+static int ____flush_rule_cache(struct nftnl_rule *r, void *data)
 {
 	nftnl_rule_list_del(r);
 	nftnl_rule_free(r);
@@ -392,9 +392,25 @@ static int __flush_rule_cache(struct nftnl_rule *r, void *data)
 	return 0;
 }
 
-void flush_rule_cache(struct nftnl_chain *c)
+static int __flush_rule_cache(struct nftnl_chain *c, void *data)
 {
-	nftnl_rule_foreach(c, __flush_rule_cache, NULL);
+	return nftnl_rule_foreach(c, ____flush_rule_cache, NULL);
+}
+
+int flush_rule_cache(struct nft_handle *h, const char *table,
+		     struct nftnl_chain *c)
+{
+	const struct builtin_table *t;
+
+	if (c)
+		return __flush_rule_cache(c, NULL);
+
+	t = nft_table_builtin_find(h, table);
+	if (!t || !h->cache->table[t->type].chains)
+		return 0;
+
+	return nftnl_chain_list_foreach(h->cache->table[t->type].chains,
+					__flush_rule_cache, NULL);
 }
 
 static int __flush_chain_cache(struct nftnl_chain *c, void *data)
diff --git a/iptables/nft-cache.h b/iptables/nft-cache.h
index 793a85f453ffc..cb7a7688be37a 100644
--- a/iptables/nft-cache.h
+++ b/iptables/nft-cache.h
@@ -8,7 +8,8 @@ void nft_build_cache(struct nft_handle *h, struct nftnl_chain *c);
 void nft_rebuild_cache(struct nft_handle *h);
 void nft_release_cache(struct nft_handle *h);
 void flush_chain_cache(struct nft_handle *h, const char *tablename);
-void flush_rule_cache(struct nftnl_chain *c);
+int flush_rule_cache(struct nft_handle *h, const char *table,
+		     struct nftnl_chain *c);
 
 struct nftnl_chain_list *
 nft_chain_list_get(struct nft_handle *h, const char *table, const char *chain);
diff --git a/iptables/nft.c b/iptables/nft.c
index 12cc423c87bbb..89b1c7a808f57 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1437,7 +1437,7 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
 	struct obj_update *obj;
 	struct nftnl_rule *r;
 
-	if (verbose)
+	if (verbose && chain)
 		fprintf(stdout, "Flushing chain `%s'\n", chain);
 
 	r = nftnl_rule_alloc();
@@ -1445,7 +1445,8 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
 		return;
 
 	nftnl_rule_set_str(r, NFTNL_RULE_TABLE, table);
-	nftnl_rule_set_str(r, NFTNL_RULE_CHAIN, chain);
+	if (chain)
+		nftnl_rule_set_str(r, NFTNL_RULE_CHAIN, chain);
 
 	obj = batch_rule_add(h, NFT_COMPAT_RULE_FLUSH, r);
 	if (!obj) {
@@ -1459,19 +1460,21 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
 int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 		   bool verbose)
 {
-	int ret = 0;
-	struct nftnl_chain_list *list;
 	struct nftnl_chain_list_iter *iter;
-	struct nftnl_chain *c;
+	struct nftnl_chain_list *list;
+	struct nftnl_chain *c = NULL;
+	int ret = 0;
 
 	nft_xt_builtin_init(h, table);
 
 	nft_fn = nft_rule_flush;
 
-	list = nft_chain_list_get(h, table, chain);
-	if (list == NULL) {
-		ret = 1;
-		goto err;
+	if (chain || verbose) {
+		list = nft_chain_list_get(h, table, chain);
+		if (list == NULL) {
+			ret = 1;
+			goto err;
+		}
 	}
 
 	if (chain) {
@@ -1480,9 +1483,11 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 			errno = ENOENT;
 			return 0;
 		}
+	}
 
+	if (chain || !verbose) {
 		__nft_rule_flush(h, table, chain, verbose, false);
-		flush_rule_cache(c);
+		flush_rule_cache(h, table, c);
 		return 1;
 	}
 
@@ -1494,11 +1499,10 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 
 	c = nftnl_chain_list_iter_next(iter);
 	while (c != NULL) {
-		const char *chain_name =
-			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
+		chain = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
 
-		__nft_rule_flush(h, table, chain_name, verbose, false);
-		flush_rule_cache(c);
+		__nft_rule_flush(h, table, chain, verbose, false);
+		flush_rule_cache(h, table, c);
 		c = nftnl_chain_list_iter_next(iter);
 	}
 	nftnl_chain_list_iter_destroy(iter);
-- 
2.23.0

Re: [iptables PATCH v4 0/8] Improve iptables-nft performance with large rulesets

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 01:41:44PM +0200, Phil Sutter wrote:
> Fourth try at caching optimizations implementation.
> 
> Changes since v3:
> 
> * Rebase onto current master after pushing the accepted initial three
>   patches.
> * Avoid cache inconsistency in __nft_build_cache() if kernel ruleset
>   changed since last call.

I still hesitate with this cache approach.

Can this deal with this scenario? Say you have a ruleset composed on N
rules.

* Rule 1..M starts using generation X for the evaluation, they pass
  OK.

* Generation is bumped.

* Rule M..N is evaluated with a diferent cache.

So the ruleset evaluation is inconsistent itself since it is based on
different caches for each rule in the batch.

Re: [iptables PATCH v4 0/8] Improve iptables-nft performance with large rulesets

[Pablo Neira Ayuso]

On Thu, Oct 17, 2019 at 11:03:32AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 15, 2019 at 01:41:44PM +0200, Phil Sutter wrote:
> > Fourth try at caching optimizations implementation.
> > 
> > Changes since v3:
> > 
> > * Rebase onto current master after pushing the accepted initial three
> >   patches.
> > * Avoid cache inconsistency in __nft_build_cache() if kernel ruleset
> >   changed since last call.
> 
> I still hesitate with this cache approach.
> 
> Can this deal with this scenario? Say you have a ruleset composed on N
> rules.
> 
> * Rule 1..M starts using generation X for the evaluation, they pass
>   OK.
> 
> * Generation is bumped.
> 
> * Rule M..N is evaluated with a diferent cache.
> 
> So the ruleset evaluation is inconsistent itself since it is based on
> different caches for each rule in the batch.

It might be that rule M fails because a user-defined chain is not
found anymore, error reporting will not be consistent on races, and
who knows what else.

Anyway, if you want to go for this approach, merge it upstream and
let's test how it goes... this batch looks much better indeed than v1,
so push it out.

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Re: [iptables PATCH v4 0/8] Improve iptables-nft performance with large rulesets

[Phil Sutter]

On Thu, Oct 17, 2019 at 12:08:16PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 17, 2019 at 11:03:32AM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Oct 15, 2019 at 01:41:44PM +0200, Phil Sutter wrote:
> > > Fourth try at caching optimizations implementation.
> > > 
> > > Changes since v3:
> > > 
> > > * Rebase onto current master after pushing the accepted initial three
> > >   patches.
> > > * Avoid cache inconsistency in __nft_build_cache() if kernel ruleset
> > >   changed since last call.
> > 
> > I still hesitate with this cache approach.
> > 
> > Can this deal with this scenario? Say you have a ruleset composed on N
> > rules.
> > 
> > * Rule 1..M starts using generation X for the evaluation, they pass
> >   OK.
> > 
> > * Generation is bumped.
> > 
> > * Rule M..N is evaluated with a diferent cache.
> > 
> > So the ruleset evaluation is inconsistent itself since it is based on
> > different caches for each rule in the batch.
> 
> It might be that rule M fails because a user-defined chain is not
> found anymore, error reporting will not be consistent on races, and
> who knows what else.
> 
> Anyway, if you want to go for this approach, merge it upstream and
> let's test how it goes... this batch looks much better indeed than v1,
> so push it out.

Yes, let's please give it a try. Fingers crossed, but if it blows up
I'll either fix it or revert it myself. :)

Pushed the whole series with your ACKs added.

Thanks, Phil

Re: [iptables PATCH v4 0/8] Improve iptables-nft performance with large rulesets

[Pablo Neira Ayuso]

On Thu, Oct 17, 2019 at 07:06:28PM +0200, Phil Sutter wrote:
> On Thu, Oct 17, 2019 at 12:08:16PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Oct 17, 2019 at 11:03:32AM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Oct 15, 2019 at 01:41:44PM +0200, Phil Sutter wrote:
> > > > Fourth try at caching optimizations implementation.
> > > > 
> > > > Changes since v3:
> > > > 
> > > > * Rebase onto current master after pushing the accepted initial three
> > > >   patches.
> > > > * Avoid cache inconsistency in __nft_build_cache() if kernel ruleset
> > > >   changed since last call.
> > > 
> > > I still hesitate with this cache approach.
> > > 
> > > Can this deal with this scenario? Say you have a ruleset composed on N
> > > rules.
> > > 
> > > * Rule 1..M starts using generation X for the evaluation, they pass
> > >   OK.
> > > 
> > > * Generation is bumped.
> > > 
> > > * Rule M..N is evaluated with a diferent cache.
> > > 
> > > So the ruleset evaluation is inconsistent itself since it is based on
> > > different caches for each rule in the batch.
> > 
> > It might be that rule M fails because a user-defined chain is not
> > found anymore, error reporting will not be consistent on races, and
> > who knows what else.
> > 
> > Anyway, if you want to go for this approach, merge it upstream and
> > let's test how it goes... this batch looks much better indeed than v1,
> > so push it out.
> 
> Yes, let's please give it a try. Fingers crossed, but if it blows up
> I'll either fix it or revert it myself. :)

Thanks Phil, let's do that!

Re: [iptables PATCH v4 0/8] Improve iptables-nft performance with large rulesets

[Phil Sutter]

Hi,

On Thu, Oct 17, 2019 at 11:03:32AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 15, 2019 at 01:41:44PM +0200, Phil Sutter wrote:
> > Fourth try at caching optimizations implementation.
> > 
> > Changes since v3:
> > 
> > * Rebase onto current master after pushing the accepted initial three
> >   patches.
> > * Avoid cache inconsistency in __nft_build_cache() if kernel ruleset
> >   changed since last call.
> 
> I still hesitate with this cache approach.
> 
> Can this deal with this scenario? Say you have a ruleset composed on N
> rules.
> 
> * Rule 1..M starts using generation X for the evaluation, they pass
>   OK.
> 
> * Generation is bumped.
> 
> * Rule M..N is evaluated with a diferent cache.
> 
> So the ruleset evaluation is inconsistent itself since it is based on
> different caches for each rule in the batch.

Yes, that is possible. In a discussion with Florian back when he fixed
for concurrent xtables-restore calls, consensus was: If you use
--noflush and concurrent ruleset updates happen, you're screwed anyway.
(Meaning, results are not predictable and we can't do anything about
it.)

In comparison with current code which just fetches full cache upon
invocation of 'xtables-restore --noflush', problems might not be
detected during evaluation but only later when kernel rejects the
commands.

Eventually, commands have to apply to the ruleset as it is after opening
the transaction. If you cache everything first, you don't detect
incompatible ruleset changes at all. If you cache multiple times, you
may detect the incompatible changes while evaluating but the result is
the same, just with different error messages. :)

Cheers, Phil