[PATCH iptables 0/6] cache rework

[PATCH iptables 0/6] cache rework

[Pablo Neira Ayuso]

This patchset updates the cache logic:

* Update nft_table_list_get() to use a list of tables from the cache,
  instead of listing them from the kernel.

* Ensure cache consistency by checking for generation ID is consistent
  when building up the cache.

Without this, we may end up with an inconsistent cache, hence defeating
the refresh transaction logic.

The other patches are just a few preparation patches to allow to
maintain the original cache and a cache that is refreshed everytime this
hits ERESTART.

My plan is to send another batch to revisit the refresh transaction
logic after this patchset, since 0004-restore-race_0 still does not
work after this.

Pablo Neira Ayuso (6):
  nft: add struct nft_cache
  nft: statify nft_rebuild_cache()
  nft: add __nft_table_builtin_find()
  nft: add flush_cache()
  nft: cache table list
  nft: ensure cache consistency

 iptables/nft.c | 195 ++++++++++++++++++++++++++++++++++-----------------------
 iptables/nft.h |  15 +++--
 2 files changed, 126 insertions(+), 84 deletions(-)

-- 
2.11.0

[PATCH iptables 1/6] nft: add struct nft_cache

[Pablo Neira Ayuso]

Add new structure that encloses the cache and update the code to use it.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 iptables/nft.c | 31 ++++++++++++++++---------------
 iptables/nft.h | 13 +++++++++----
 2 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index dab1db59ec97..167237ab45b1 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -631,7 +631,7 @@ const struct builtin_table xtables_bridge[NFT_TABLE_MAX] = {
 static bool nft_table_initialized(const struct nft_handle *h,
 				  enum nft_table_type type)
 {
-	return h->table[type].initialized;
+	return h->cache->table[type].initialized;
 }
 
 static int nft_table_builtin_add(struct nft_handle *h,
@@ -685,7 +685,7 @@ static void nft_chain_builtin_add(struct nft_handle *h,
 		return;
 
 	batch_chain_add(h, NFT_COMPAT_CHAIN_ADD, c);
-	nftnl_chain_list_add_tail(c, h->table[table->type].chain_cache);
+	nftnl_chain_list_add_tail(c, h->cache->table[table->type].chains);
 }
 
 /* find if built-in table already exists */
@@ -763,7 +763,7 @@ static int nft_xt_builtin_init(struct nft_handle *h, const char *table)
 
 	nft_chain_builtin_init(h, t);
 
-	h->table[t->type].initialized = true;
+	h->cache->table[t->type].initialized = true;
 
 	return 0;
 }
@@ -805,6 +805,7 @@ int nft_init(struct nft_handle *h, const struct builtin_table *t)
 
 	h->portid = mnl_socket_get_portid(h->nl);
 	h->tables = t;
+	h->cache = &h->__cache;
 
 	INIT_LIST_HEAD(&h->obj_list);
 	INIT_LIST_HEAD(&h->err_list);
@@ -840,9 +841,9 @@ static void flush_chain_cache(struct nft_handle *h, const char *tablename)
 
 	if (tablename) {
 		table = nft_table_builtin_find(h, tablename);
-		if (!table || !h->table[table->type].chain_cache)
+		if (!table || !h->cache->table[table->type].chains)
 			return;
-		nftnl_chain_list_foreach(h->table[table->type].chain_cache,
+		nftnl_chain_list_foreach(h->cache->table[table->type].chains,
 					 __flush_chain_cache, NULL);
 		return;
 	}
@@ -851,11 +852,11 @@ static void flush_chain_cache(struct nft_handle *h, const char *tablename)
 		if (h->tables[i].name == NULL)
 			continue;
 
-		if (!h->table[i].chain_cache)
+		if (!h->cache->table[i].chains)
 			continue;
 
-		nftnl_chain_list_free(h->table[i].chain_cache);
-		h->table[i].chain_cache = NULL;
+		nftnl_chain_list_free(h->cache->table[i].chains);
+		h->cache->table[i].chains = NULL;
 	}
 	h->have_cache = false;
 }
@@ -1326,7 +1327,7 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 	if (!t)
 		goto out;
 
-	nftnl_chain_list_add_tail(c, h->table[t->type].chain_cache);
+	nftnl_chain_list_add_tail(c, h->cache->table[t->type].chains);
 
 	return MNL_CB_OK;
 out:
@@ -1348,8 +1349,8 @@ retry:
 		if (!h->tables[i].name)
 			continue;
 
-		h->table[type].chain_cache = nftnl_chain_list_alloc();
-		if (!h->table[type].chain_cache)
+		h->cache->table[type].chains = nftnl_chain_list_alloc();
+		if (!h->cache->table[type].chains)
 			return -1;
 	}
 
@@ -1517,7 +1518,7 @@ static int fetch_rule_cache(struct nft_handle *h)
 		if (!h->tables[i].name)
 			continue;
 
-		if (nftnl_chain_list_foreach(h->table[type].chain_cache,
+		if (nftnl_chain_list_foreach(h->cache->table[type].chains,
 					     nft_rule_list_update, h))
 			return -1;
 	}
@@ -1558,7 +1559,7 @@ struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
 
 	nft_build_cache(h);
 
-	return h->table[t->type].chain_cache;
+	return h->cache->table[t->type].chains;
 }
 
 static const char *policy_name[NF_ACCEPT+1] = {
@@ -2088,7 +2089,7 @@ static int __nft_table_flush(struct nft_handle *h, const char *table, bool exist
 
 	_t = nft_table_builtin_find(h, table);
 	assert(_t);
-	h->table[_t->type].initialized = false;
+	h->cache->table[_t->type].initialized = false;
 
 	flush_chain_cache(h, table);
 
@@ -3021,7 +3022,7 @@ static void nft_bridge_commit_prepare(struct nft_handle *h)
 		if (!t->name)
 			continue;
 
-		list = h->table[t->type].chain_cache;
+		list = h->cache->table[t->type].chains;
 		if (!list)
 			continue;
 
diff --git a/iptables/nft.h b/iptables/nft.h
index 23bd2b79884c..8292a2922d6a 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -27,6 +27,13 @@ struct builtin_table {
 	struct builtin_chain chains[NF_INET_NUMHOOKS];
 };
 
+struct nft_cache {
+	struct {
+		struct nftnl_chain_list *chains;
+		bool			initialized;
+	} table[NFT_TABLE_MAX];
+};
+
 struct nft_handle {
 	int			family;
 	struct mnl_socket	*nl;
@@ -40,10 +47,8 @@ struct nft_handle {
 	struct list_head	err_list;
 	struct nft_family_ops	*ops;
 	const struct builtin_table *tables;
-	struct {
-		struct nftnl_chain_list *chain_cache;
-		bool			initialized;
-	} table[NFT_TABLE_MAX];
+	struct nft_cache	__cache;
+	struct nft_cache	*cache;
 	bool			have_cache;
 	bool			restore;
 	bool			noflush;
-- 
2.11.0

[PATCH iptables 2/6] nft: statify nft_rebuild_cache()

[Pablo Neira Ayuso]

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 iptables/nft.c | 2 +-
 iptables/nft.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 167237ab45b1..6c6cd787c06f 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1540,7 +1540,7 @@ void nft_build_cache(struct nft_handle *h)
 		__nft_build_cache(h);
 }
 
-void nft_rebuild_cache(struct nft_handle *h)
+static void nft_rebuild_cache(struct nft_handle *h)
 {
 	if (!h->have_cache)
 		flush_chain_cache(h, NULL);
diff --git a/iptables/nft.h b/iptables/nft.h
index 8292a2922d6a..c137c5c6708d 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -70,7 +70,6 @@ int mnl_talk(struct nft_handle *h, struct nlmsghdr *nlh,
 int nft_init(struct nft_handle *h, const struct builtin_table *t);
 void nft_fini(struct nft_handle *h);
 void nft_build_cache(struct nft_handle *h);
-void nft_rebuild_cache(struct nft_handle *h);
 
 /*
  * Operations with tables.
-- 
2.11.0

[PATCH iptables 3/6] nft: add __nft_table_builtin_find()

[Pablo Neira Ayuso]

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 iptables/nft.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 6c6cd787c06f..b0a15e9b3f7c 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -688,25 +688,31 @@ static void nft_chain_builtin_add(struct nft_handle *h,
 	nftnl_chain_list_add_tail(c, h->cache->table[table->type].chains);
 }
 
-/* find if built-in table already exists */
-const struct builtin_table *
-nft_table_builtin_find(struct nft_handle *h, const char *table)
+static const struct builtin_table *
+__nft_table_builtin_find(const struct builtin_table *tables, const char *table)
 {
 	int i;
 	bool found = false;
 
 	for (i = 0; i < NFT_TABLE_MAX; i++) {
-		if (h->tables[i].name == NULL)
+		if (tables[i].name == NULL)
 			continue;
 
-		if (strcmp(h->tables[i].name, table) != 0)
+		if (strcmp(tables[i].name, table) != 0)
 			continue;
 
 		found = true;
 		break;
 	}
 
-	return found ? &h->tables[i] : NULL;
+	return found ? &tables[i] : NULL;
+}
+
+/* find if built-in table already exists */
+const struct builtin_table *
+nft_table_builtin_find(struct nft_handle *h, const char *table)
+{
+	return __nft_table_builtin_find(h->tables, table);
 }
 
 /* find if built-in chain already exists */
-- 
2.11.0

[PATCH iptables 4/6] nft: add flush_cache()

[Pablo Neira Ayuso]

This new function takes a struct nft_cache as parameter.

This patch also introduces __nft_table_builtin_find() which is required
to look up for built-in tables without the nft_handle structure.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 iptables/nft.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index b0a15e9b3f7c..8f6f7a41ff33 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -840,31 +840,39 @@ static int __flush_chain_cache(struct nftnl_chain *c, void *data)
 	return 0;
 }
 
-static void flush_chain_cache(struct nft_handle *h, const char *tablename)
+static int flush_cache(struct nft_cache *c, const struct builtin_table *tables,
+		       const char *tablename)
 {
 	const struct builtin_table *table;
 	int i;
 
 	if (tablename) {
-		table = nft_table_builtin_find(h, tablename);
-		if (!table || !h->cache->table[table->type].chains)
-			return;
-		nftnl_chain_list_foreach(h->cache->table[table->type].chains,
+		table = __nft_table_builtin_find(tables, tablename);
+		if (!table || !c->table[table->type].chains)
+			return 0;
+		nftnl_chain_list_foreach(c->table[table->type].chains,
 					 __flush_chain_cache, NULL);
-		return;
+		return 0;
 	}
 
 	for (i = 0; i < NFT_TABLE_MAX; i++) {
-		if (h->tables[i].name == NULL)
+		if (tables[i].name == NULL)
 			continue;
 
-		if (!h->cache->table[i].chains)
+		if (!c->table[i].chains)
 			continue;
 
-		nftnl_chain_list_free(h->cache->table[i].chains);
-		h->cache->table[i].chains = NULL;
+		nftnl_chain_list_free(c->table[i].chains);
+		c->table[i].chains = NULL;
 	}
-	h->have_cache = false;
+
+	return 1;
+}
+
+static void flush_chain_cache(struct nft_handle *h, const char *tablename)
+{
+	if (flush_cache(h->cache, h->tables, tablename))
+		h->have_cache = false;
 }
 
 void nft_fini(struct nft_handle *h)
-- 
2.11.0

[PATCH iptables 5/6] nft: cache table list

[Pablo Neira Ayuso]

nft_table_find() uses the table list cache to look up for existing
tables.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 iptables/nft.c | 102 ++++++++++++++++++++++++++++++++-------------------------
 iptables/nft.h |   1 +
 2 files changed, 58 insertions(+), 45 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 8f6f7a41ff33..063637e22fb8 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -865,12 +865,17 @@ static int flush_cache(struct nft_cache *c, const struct builtin_table *tables,
 		nftnl_chain_list_free(c->table[i].chains);
 		c->table[i].chains = NULL;
 	}
+	nftnl_table_list_free(c->tables);
+	c->tables = NULL;
 
 	return 1;
 }
 
 static void flush_chain_cache(struct nft_handle *h, const char *tablename)
 {
+	if (!h->have_cache)
+		return;
+
 	if (flush_cache(h->cache, h->tables, tablename))
 		h->have_cache = false;
 }
@@ -1350,6 +1355,53 @@ err:
 	return MNL_CB_OK;
 }
 
+static int nftnl_table_list_cb(const struct nlmsghdr *nlh, void *data)
+{
+	struct nftnl_table *t;
+	struct nftnl_table_list *list = data;
+
+	t = nftnl_table_alloc();
+	if (t == NULL)
+		goto err;
+
+	if (nftnl_table_nlmsg_parse(nlh, t) < 0)
+		goto out;
+
+	nftnl_table_list_add_tail(t, list);
+
+	return MNL_CB_OK;
+out:
+	nftnl_table_free(t);
+err:
+	return MNL_CB_OK;
+}
+
+static int fetch_table_cache(struct nft_handle *h)
+{
+	char buf[16536];
+	struct nlmsghdr *nlh;
+	struct nftnl_table_list *list;
+	int ret;
+
+retry:
+	list = nftnl_table_list_alloc();
+	if (list == NULL)
+		return 0;
+
+	nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_GETTABLE, h->family,
+					NLM_F_DUMP, h->seq);
+
+	ret = mnl_talk(h, nlh, nftnl_table_list_cb, list);
+	if (ret < 0 && errno == EINTR) {
+		assert(nft_restart(h) >= 0);
+		nftnl_table_list_free(list);
+		goto retry;
+	}
+	h->cache->tables = list;
+
+	return 1;
+}
+
 static int fetch_chain_cache(struct nft_handle *h)
 {
 	char buf[16536];
@@ -1357,6 +1409,8 @@ static int fetch_chain_cache(struct nft_handle *h)
 	int i, ret;
 
 retry:
+	fetch_table_cache(h);
+
 	for (i = 0; i < NFT_TABLE_MAX; i++) {
 		enum nft_table_type type = h->tables[i].type;
 
@@ -1968,56 +2022,17 @@ int nft_chain_user_rename(struct nft_handle *h,const char *chain,
 	return ret == 0 ? 1 : 0;
 }
 
-static int nftnl_table_list_cb(const struct nlmsghdr *nlh, void *data)
-{
-	struct nftnl_table *t;
-	struct nftnl_table_list *list = data;
-
-	t = nftnl_table_alloc();
-	if (t == NULL)
-		goto err;
-
-	if (nftnl_table_nlmsg_parse(nlh, t) < 0)
-		goto out;
-
-	nftnl_table_list_add_tail(t, list);
-
-	return MNL_CB_OK;
-out:
-	nftnl_table_free(t);
-err:
-	return MNL_CB_OK;
-}
-
 static struct nftnl_table_list *nftnl_table_list_get(struct nft_handle *h)
 {
-	char buf[16536];
-	struct nlmsghdr *nlh;
-	struct nftnl_table_list *list;
-	int ret;
-
-retry:
-	list = nftnl_table_list_alloc();
-	if (list == NULL)
-		return 0;
-
-	nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_GETTABLE, h->family,
-					NLM_F_DUMP, h->seq);
-
-	ret = mnl_talk(h, nlh, nftnl_table_list_cb, list);
-	if (ret < 0 && errno == EINTR) {
-		assert(nft_restart(h) >= 0);
-		nftnl_table_list_free(list);
-		goto retry;
-	}
+	nft_build_cache(h);
 
-	return list;
+	return h->cache->tables;
 }
 
 bool nft_table_find(struct nft_handle *h, const char *tablename)
 {
-	struct nftnl_table_list *list;
 	struct nftnl_table_list_iter *iter;
+	struct nftnl_table_list *list;
 	struct nftnl_table *t;
 	bool ret = false;
 
@@ -2043,7 +2058,6 @@ bool nft_table_find(struct nft_handle *h, const char *tablename)
 	}
 
 	nftnl_table_list_iter_destroy(iter);
-	nftnl_table_list_free(list);
 
 err:
 	return ret;
@@ -2076,7 +2090,6 @@ int nft_for_each_table(struct nft_handle *h,
 	}
 
 	nftnl_table_list_iter_destroy(iter);
-	nftnl_table_list_free(list);
 	return 0;
 }
 
@@ -2148,7 +2161,6 @@ int nft_table_flush(struct nft_handle *h, const char *table)
 	ret = __nft_table_flush(h, table, exists);
 	nftnl_table_list_iter_destroy(iter);
 err_table_list:
-	nftnl_table_list_free(list);
 err_out:
 	/* the core expects 1 for success and 0 for error */
 	return ret == 0 ? 1 : 0;
diff --git a/iptables/nft.h b/iptables/nft.h
index c137c5c6708d..dc0797d302b8 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -28,6 +28,7 @@ struct builtin_table {
 };
 
 struct nft_cache {
+	struct nftnl_table_list		*tables;
 	struct {
 		struct nftnl_chain_list *chains;
 		bool			initialized;
-- 
2.11.0

[PATCH iptables 6/6] nft: ensure cache consistency

[Pablo Neira Ayuso]

Check for generation ID before and after fetching the cache to ensure
consistency.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 iptables/nft.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 063637e22fb8..172beec9ae27 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -63,7 +63,7 @@ static void *nft_fn;
 
 static int genid_cb(const struct nlmsghdr *nlh, void *data)
 {
-	struct nft_handle *h = data;
+	uint32_t *genid = data;
 	struct nftnl_gen *gen;
 
 	gen = nftnl_gen_alloc();
@@ -73,7 +73,7 @@ static int genid_cb(const struct nlmsghdr *nlh, void *data)
 	if (nftnl_gen_nlmsg_parse(nlh, gen) < 0)
 		goto out;
 
-	h->nft_genid = nftnl_gen_get_u32(gen, NFTNL_GEN_ID);
+	*genid = nftnl_gen_get_u32(gen, NFTNL_GEN_ID);
 
 	nftnl_gen_free(gen);
 	return MNL_CB_STOP;
@@ -82,13 +82,13 @@ out:
 	return MNL_CB_ERROR;
 }
 
-static int mnl_genid_get(struct nft_handle *h)
+static int mnl_genid_get(struct nft_handle *h, uint32_t *genid)
 {
 	char buf[MNL_SOCKET_BUFFER_SIZE];
 	struct nlmsghdr *nlh;
 
 	nlh = nftnl_nlmsg_build_hdr(buf, NFT_MSG_GETGEN, 0, 0, h->seq);
-	return mnl_talk(h, nlh, genid_cb, h);
+	return mnl_talk(h, nlh, genid_cb, genid);
 }
 
 int mnl_talk(struct nft_handle *h, struct nlmsghdr *nlh,
@@ -1595,12 +1595,22 @@ static int fetch_rule_cache(struct nft_handle *h)
 
 static void __nft_build_cache(struct nft_handle *h)
 {
-	mnl_genid_get(h);
+	uint32_t genid_start, genid_stop;
+
+retry:
+	mnl_genid_get(h, &genid_start);
 	fetch_chain_cache(h);
 	fetch_rule_cache(h);
 	h->have_cache = true;
-}
+	mnl_genid_get(h, &genid_stop);
 
+	if (genid_start != genid_stop) {
+		flush_chain_cache(h, NULL);
+		goto retry;
+	}
+
+	h->nft_genid = genid_start;
+}
 
 void nft_build_cache(struct nft_handle *h)
 {
-- 
2.11.0