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

[PATCH iptables 1/4] 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 | 33 ++++++++++++++++++---------------
 iptables/nft.h | 12 ++++++++----
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index dab1db59ec97..d78c431703ca 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;
 }
@@ -792,6 +792,8 @@ static int nft_restart(struct nft_handle *h)
 	return 0;
 }
 
+static struct nft_cache cache;
+
 int nft_init(struct nft_handle *h, const struct builtin_table *t)
 {
 	h->nl = mnl_socket_open(NETLINK_NETFILTER);
@@ -805,6 +807,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 = &cache;
 
 	INIT_LIST_HEAD(&h->obj_list);
 	INIT_LIST_HEAD(&h->err_list);
@@ -840,9 +843,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 +854,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 +1329,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 +1351,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 +1520,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 +1561,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 +2091,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 +3024,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..4c207a433820 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,7 @@ 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;
 	bool			have_cache;
 	bool			restore;
 	bool			noflush;
-- 
2.11.0

[PATCH iptables 2/4] 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 d78c431703ca..c5ddde5f0064 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1542,7 +1542,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 4c207a433820..40da8064b742 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -69,7 +69,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/4] 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 | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index c5ddde5f0064..14141bb7dbcf 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 */
@@ -836,30 +842,37 @@ 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 void 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)
+		table = __nft_table_builtin_find(tables, tablename);
+		if (!table || !c->table[table->type].chains)
 			return;
-		nftnl_chain_list_foreach(h->cache->table[table->type].chains,
+		nftnl_chain_list_foreach(c->table[table->type].chains,
 					 __flush_chain_cache, NULL);
 		return;
 	}
 
 	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;
 	}
+}
+
+static void flush_chain_cache(struct nft_handle *h, const char *tablename)
+{
+	flush_cache(h->cache, h->tables, tablename);
 	h->have_cache = false;
 }
 
-- 
2.11.0

[PATCH iptables 4/4] nft: keep old cache around until batch is refreshed in case of ERESTART

[Pablo Neira Ayuso]

Phil Sutter says:

"The problem is that data in h->obj_list potentially sits in cache, too.
At least rules have to be there so insert with index works correctly. If
the cache is flushed before regenerating the batch, use-after-free
occurs which crashes the program."

This patch keeps the old cache around until we have refreshed the batch.

Fixes: 862818ac3a0de ("xtables: add and use nft_build_cache")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
@Phil: I'd like to avoid the refcount infrastructure in libnftnl.

Compile tested-only.

 iptables/nft.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 14141bb7dbcf..51f05b6a6267 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -798,7 +798,10 @@ static int nft_restart(struct nft_handle *h)
 	return 0;
 }
 
-static struct nft_cache cache;
+struct {
+	struct nft_cache 	cache[2];
+	unsigned int		index;
+} pool;
 
 int nft_init(struct nft_handle *h, const struct builtin_table *t)
 {
@@ -813,7 +816,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 = &cache;
+	h->cache = &pool.cache[0];
 
 	INIT_LIST_HEAD(&h->obj_list);
 	INIT_LIST_HEAD(&h->err_list);
@@ -1555,12 +1558,25 @@ void nft_build_cache(struct nft_handle *h)
 		__nft_build_cache(h);
 }
 
-static void nft_rebuild_cache(struct nft_handle *h)
+static struct nft_cache *nft_get_cache_next(void)
 {
-	if (!h->have_cache)
-		flush_chain_cache(h, NULL);
+	if (++pool.index > 1)
+		pool.index = 0;
 
+	return &pool.cache[pool.index];
+}
+
+static struct nft_cache *nft_rebuild_cache(struct nft_handle *h)
+{
+	struct nft_cache *cache = NULL;
+
+	if (h->have_cache) {
+		cache = h->cache;
+		h->cache = nft_get_cache_next();
+	}
 	__nft_build_cache(h);
+
+	return cache;
 }
 
 struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
@@ -2902,10 +2918,13 @@ retry:
 	errno = 0;
 	ret = mnl_batch_talk(h->nl, h->batch, &h->err_list);
 	if (ret && errno == ERESTART) {
-		nft_rebuild_cache(h);
+		struct nft_cache *old_cache = nft_rebuild_cache(h);
 
 		nft_refresh_transaction(h);
 
+		if (old_cache)
+			flush_cache(old_cache, h->tables, NULL);
+
 		i=0;
 		list_for_each_entry_safe(err, ne, &h->err_list, head)
 			mnl_err_list_free(err);
-- 
2.11.0

Re: [PATCH iptables 4/4] nft: keep old cache around until batch is refreshed in case of ERESTART

[Phil Sutter]

Hi Pablo,

On Sun, May 19, 2019 at 01:51:21PM +0200, Pablo Neira Ayuso wrote:
> Phil Sutter says:
> 
> "The problem is that data in h->obj_list potentially sits in cache, too.
> At least rules have to be there so insert with index works correctly. If
> the cache is flushed before regenerating the batch, use-after-free
> occurs which crashes the program."
> 
> This patch keeps the old cache around until we have refreshed the batch.
> 
> Fixes: 862818ac3a0de ("xtables: add and use nft_build_cache")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> @Phil: I'd like to avoid the refcount infrastructure in libnftnl.

OK, then see below:

> Compile tested-only.

Please test it at least against
iptables/tests/shell/testcases/ipt-restore/0004-restore-race_0. You are
reworking a code-path which is hit only in rare cases, Florian did a
great job in creating something that triggers it.

My point is, I don't trust this code:

> diff --git a/iptables/nft.c b/iptables/nft.c
> index 14141bb7dbcf..51f05b6a6267 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
[...]
> @@ -2902,10 +2918,13 @@ retry:
>  	errno = 0;
>  	ret = mnl_batch_talk(h->nl, h->batch, &h->err_list);
>  	if (ret && errno == ERESTART) {
> -		nft_rebuild_cache(h);
> +		struct nft_cache *old_cache = nft_rebuild_cache(h);
>  
>  		nft_refresh_transaction(h);
>  
> +		if (old_cache)
> +			flush_cache(old_cache, h->tables, NULL);
> +

The call to flush_cache() should free objects referenced in batch jobs.
Note that nft_refresh_transaction() does not care about batch jobs'
payloads, it just toggles them on/off via 'skip' bit.

The only way to make the above work is by keeping the original cache
copy around until mnl_batch_talk has finally succeeded or failed with
something else than ERESTART.

Cheers, Phil

Re: [PATCH iptables 4/4] nft: keep old cache around until batch is refreshed in case of ERESTART

[Phil Sutter]

Hi,

On Sun, May 19, 2019 at 06:45:08PM +0200, Phil Sutter wrote:
[...]
> The only way to make the above work is by keeping the original cache
> copy around until mnl_batch_talk has finally succeeded or failed with
> something else than ERESTART.

How about a completely different approach:

If memory serves right (and from reading the related Red Hat ticket),
the actual problem we're trying to solve is that iptables-nft-restore
creates NFT_MSG_DELTABLE only if that table exists already at the point
of parsing but another client might create it in the mean time before
committing.

My idea for solving this was to unconditionally create NFT_MSG_NEWTABLE
followed by NFT_MSG_DELTABLE - in case the table exists, the first one
is a noop; in case the table doesn't exist, the second one won't provoke
an error message from kernel space.

Since NFT_MSG_DELTABLE removes the table recursively, we don't need to
care about any content added by the other client.

Or is this about a generic solution for all commands not just
iptables-nft-restore (without --noflush)?

Cheers, Phil

Re: [PATCH iptables 4/4] nft: keep old cache around until batch is refreshed in case of ERESTART

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Sun, May 19, 2019 at 06:45:08PM +0200, Phil Sutter wrote:
> [...]
> > The only way to make the above work is by keeping the original cache
> > copy around until mnl_batch_talk has finally succeeded or failed with
> > something else than ERESTART.
> 
> How about a completely different approach:
> 
> If memory serves right (and from reading the related Red Hat ticket),
> the actual problem we're trying to solve is that iptables-nft-restore
> creates NFT_MSG_DELTABLE only if that table exists already at the point
> of parsing but another client might create it in the mean time before
> committing.
> 
> My idea for solving this was to unconditionally create NFT_MSG_NEWTABLE
> followed by NFT_MSG_DELTABLE - in case the table exists, the first one
> is a noop; in case the table doesn't exist, the second one won't provoke
> an error message from kernel space.

Does that work even work?
new table x
del table x
add rule to x // table was deleted?

Or are you talking about a new/del/new sequence?

If it works, ok/fine, but it seems ugly.

> Since NFT_MSG_DELTABLE removes the table recursively, we don't need to
> care about any content added by the other client.

Yes, we can't do this in the --noflush case though.

> Or is this about a generic solution for all commands not just
> iptables-nft-restore (without --noflush)?

Its for ipt-nft-restore, including --noflush.

It would be good though to also speed up 'iptables-nft -A' later on
by eliding cache completely.

Re: [PATCH iptables 4/4] nft: keep old cache around until batch is refreshed in case of ERESTART

[Phil Sutter]

On Mon, May 20, 2019 at 02:06:20PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > On Sun, May 19, 2019 at 06:45:08PM +0200, Phil Sutter wrote:
> > [...]
> > > The only way to make the above work is by keeping the original cache
> > > copy around until mnl_batch_talk has finally succeeded or failed with
> > > something else than ERESTART.
> > 
> > How about a completely different approach:
> > 
> > If memory serves right (and from reading the related Red Hat ticket),
> > the actual problem we're trying to solve is that iptables-nft-restore
> > creates NFT_MSG_DELTABLE only if that table exists already at the point
> > of parsing but another client might create it in the mean time before
> > committing.
> > 
> > My idea for solving this was to unconditionally create NFT_MSG_NEWTABLE
> > followed by NFT_MSG_DELTABLE - in case the table exists, the first one
> > is a noop; in case the table doesn't exist, the second one won't provoke
> > an error message from kernel space.
> 
> Does that work even work?
> new table x
> del table x
> add rule to x // table was deleted?
> 
> Or are you talking about a new/del/new sequence?

Oh, yes of course. Existing iptables-nft-restore does:

1) delete table x if exists
2) add table x
3) add table x content

My idea is to:

+ 0) add table x
- 1) delete table x if exists
+ 1) delete table x
  2) add table x
  3) add table x content

> If it works, ok/fine, but it seems ugly.

Did you consider rule insert with index in your batch replay logic? I
did when duplicating it for nft, but decided it's not worth it and
people using 'add rule ... index IDX' have been warned already anyway.

> > Since NFT_MSG_DELTABLE removes the table recursively, we don't need to
> > care about any content added by the other client.
> 
> Yes, we can't do this in the --noflush case though.

My state from our last talk about it was "--noflush users are screwed
anyway". :)

> > Or is this about a generic solution for all commands not just
> > iptables-nft-restore (without --noflush)?
> 
> Its for ipt-nft-restore, including --noflush.
> 
> It would be good though to also speed up 'iptables-nft -A' later on
> by eliding cache completely.

The problem is that we can't unconditionally create NFT_MSG_NEWCHAIN
because that will reset the chain policy if non-default. So we need at
least a cache containing tables and chains, even for that simple rule
append command.

Cheers, Phil