[PATCH iptables 0/6] ERESTART fixes

[PATCH iptables 0/6] ERESTART fixes

[Pablo Neira Ayuso]

A batch of patches to fix concurrent updates via iptables-restore that
result in ERESTART errors in iptables.

Pablo Neira Ayuso (5):
  nft: keep original cache in case of ERESTART
  nft: don't skip table addition from ERESTART
  nft: don't care about previous state in ERESTART
  nft: do not retry on EINTR
  nft: reset netlink sender buffer size of socket restart

Phil Sutter (1):
  xtables: Fix for explicit rule flushes

 iptables/nft.c | 79 ++++++++++++++++++++++++++++------------------------------
 iptables/nft.h |  3 ++-
 2 files changed, 40 insertions(+), 42 deletions(-)

-- 
2.11.0

[PATCH iptables 1/6] nft: keep original cache 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 around the original cache until we have refreshed the
batch.

Fixes: 862818ac3a0de ("xtables: add and use nft_build_cache")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 iptables/nft.c | 23 ++++++++++++++++++++---
 iptables/nft.h |  3 ++-
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 172beec9ae27..288ada4af3ca 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -811,7 +811,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;
+	h->cache = &h->__cache[0];
 
 	INIT_LIST_HEAD(&h->obj_list);
 	INIT_LIST_HEAD(&h->err_list);
@@ -1618,14 +1618,30 @@ void nft_build_cache(struct nft_handle *h)
 		__nft_build_cache(h);
 }
 
-static void nft_rebuild_cache(struct nft_handle *h)
+static void __nft_flush_cache(struct nft_handle *h)
 {
-	if (!h->have_cache)
+	if (!h->cache_index) {
+		h->cache_index++;
+		h->cache = &h->__cache[h->cache_index];
+	} else {
 		flush_chain_cache(h, NULL);
+	}
+}
+
+static void nft_rebuild_cache(struct nft_handle *h)
+{
+	if (h->have_cache)
+		__nft_flush_cache(h);
 
 	__nft_build_cache(h);
 }
 
+static void nft_release_cache(struct nft_handle *h)
+{
+	if (h->cache_index)
+		flush_cache(&h->__cache[0], h->tables, NULL);
+}
+
 struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
 					    const char *table)
 {
@@ -2957,6 +2973,7 @@ retry:
 		batch_obj_del(h, n);
 	}
 
+	nft_release_cache(h);
 	mnl_batch_reset(h->batch);
 
 	if (i)
diff --git a/iptables/nft.h b/iptables/nft.h
index dc0797d302b8..43eb8a39dd9c 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -48,7 +48,8 @@ struct nft_handle {
 	struct list_head	err_list;
 	struct nft_family_ops	*ops;
 	const struct builtin_table *tables;
-	struct nft_cache	__cache;
+	unsigned int		cache_index;
+	struct nft_cache	__cache[2];
 	struct nft_cache	*cache;
 	bool			have_cache;
 	bool			restore;
-- 
2.11.0

[PATCH iptables 2/6] xtables: Fix for explicit rule flushes

[Pablo Neira Ayuso]

From: Phil Sutter <phil@nwl.cc>

The commit this fixes added a new parameter to __nft_rule_flush() to
mark a rule flush job as implicit or not. Yet the code added to that
function ignores the parameter and instead always sets batch job's
'implicit' flag to 1.

Fixes: 77e6a93d5c9dc ("xtables: add and set "implict" flag on transaction objects")
Signed-off-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 iptables/nft.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 288ada4af3ca..b9268b63c86d 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1778,7 +1778,7 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
 		return;
 	}
 
-	obj->implicit = 1;
+	obj->implicit = implicit;
 }
 
 int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
-- 
2.11.0

[PATCH iptables 3/6] nft: don't skip table addition from ERESTART

[Pablo Neira Ayuso]

I don't find a scenario that trigger this case.

Fixes: 58d7de0181f6 ("xtables: handle concurrent ruleset modifications")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 iptables/nft.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index b9268b63c86d..43b9153c2d58 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2794,15 +2794,6 @@ static void nft_refresh_transaction(struct nft_handle *h)
 			else if (!n->skip && !exists)
 				n->skip = 1;
 			break;
-		case NFT_COMPAT_TABLE_ADD:
-			tablename = nftnl_table_get_str(n->table, NFTNL_TABLE_NAME);
-			if (!tablename)
-				continue;
-
-			exists = nft_table_find(h, tablename);
-			if (n->skip && !exists)
-				n->skip = 0;
-			break;
 		case NFT_COMPAT_CHAIN_USER_ADD:
 			tablename = nftnl_chain_get_str(n->chain, NFTNL_CHAIN_TABLE);
 			if (!tablename)
@@ -2822,6 +2813,7 @@ static void nft_refresh_transaction(struct nft_handle *h)
 				n->skip = 0;
 			}
 			break;
+		case NFT_COMPAT_TABLE_ADD:
 		case NFT_COMPAT_CHAIN_ADD:
 		case NFT_COMPAT_CHAIN_ZERO:
 		case NFT_COMPAT_CHAIN_USER_DEL:
-- 
2.11.0

[PATCH iptables 4/6] nft: don't care about previous state in ERESTART

[Pablo Neira Ayuso]

We need to re-evalute based on the existing cache generation.

Fixes: 58d7de0181f6 ("xtables: handle concurrent ruleset modifications")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 iptables/nft.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 43b9153c2d58..f6d407029892 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2789,9 +2789,9 @@ static void nft_refresh_transaction(struct nft_handle *h)
 			if (!tablename)
 				continue;
 			exists = nft_table_find(h, tablename);
-			if (n->skip && exists)
+			if (exists)
 				n->skip = 0;
-			else if (!n->skip && !exists)
+			else
 				n->skip = 1;
 			break;
 		case NFT_COMPAT_CHAIN_USER_ADD:
@@ -2803,13 +2803,16 @@ static void nft_refresh_transaction(struct nft_handle *h)
 			if (!chainname)
 				continue;
 
+			if (!h->noflush)
+				break;
+
 			c = nft_chain_find(h, tablename, chainname);
-			if (c && !n->skip) {
+			if (c) {
 				/* -restore -n flushes existing rules from redefined user-chain */
-				if (h->noflush)
-					__nft_rule_flush(h, tablename,
-							 chainname, false, true);
-			} else if (!c && n->skip) {
+				__nft_rule_flush(h, tablename,
+						 chainname, false, true);
+				n->skip = 1;
+			} else if (!c) {
 				n->skip = 0;
 			}
 			break;
-- 
2.11.0

[PATCH iptables 5/6] nft: do not retry on EINTR

[Pablo Neira Ayuso]

Patch ab1cd3b510fa ("nft: ensure cache consistency") already handles
consistency via generation ID.

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

diff --git a/iptables/nft.c b/iptables/nft.c
index f6d407029892..9a3e9fdf4f12 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1383,7 +1383,6 @@ static int fetch_table_cache(struct nft_handle *h)
 	struct nftnl_table_list *list;
 	int ret;
 
-retry:
 	list = nftnl_table_list_alloc();
 	if (list == NULL)
 		return 0;
@@ -1392,11 +1391,9 @@ retry:
 					NLM_F_DUMP, h->seq);
 
 	ret = mnl_talk(h, nlh, nftnl_table_list_cb, list);
-	if (ret < 0 && errno == EINTR) {
+	if (ret < 0 && errno == EINTR)
 		assert(nft_restart(h) >= 0);
-		nftnl_table_list_free(list);
-		goto retry;
-	}
+
 	h->cache->tables = list;
 
 	return 1;
@@ -1408,7 +1405,6 @@ static int fetch_chain_cache(struct nft_handle *h)
 	struct nlmsghdr *nlh;
 	int i, ret;
 
-retry:
 	fetch_table_cache(h);
 
 	for (i = 0; i < NFT_TABLE_MAX; i++) {
@@ -1426,11 +1422,8 @@ retry:
 					NLM_F_DUMP, h->seq);
 
 	ret = mnl_talk(h, nlh, nftnl_chain_list_cb, h);
-	if (ret < 0 && errno == EINTR) {
+	if (ret < 0 && errno == EINTR)
 		assert(nft_restart(h) >= 0);
-		flush_chain_cache(h, NULL);
-		goto retry;
-	}
 
 	return ret;
 }
@@ -1551,22 +1544,13 @@ static int nft_rule_list_update(struct nftnl_chain *c, void *data)
 	nftnl_rule_set_str(rule, NFTNL_RULE_CHAIN,
 			   nftnl_chain_get_str(c, NFTNL_CHAIN_NAME));
 
-retry:
 	nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_GETRULE, h->family,
 					NLM_F_DUMP, h->seq);
 	nftnl_rule_nlmsg_build_payload(nlh, rule);
 
 	ret = mnl_talk(h, nlh, nftnl_rule_list_cb, c);
-	if (ret < 0) {
-		flush_rule_cache(c);
-
-		if (errno == EINTR) {
-			assert(nft_restart(h) >= 0);
-			goto retry;
-		}
-		nftnl_rule_free(rule);
-		return -1;
-	}
+	if (ret < 0 && errno == EINTR)
+		assert(nft_restart(h) >= 0);
 
 	nftnl_rule_free(rule);
 
-- 
2.11.0

[PATCH iptables 6/6] nft: reset netlink sender buffer size of socket restart

[Pablo Neira Ayuso]

Otherwise, mnl_set_sndbuffer() skips the buffer update after socket
restart. Then, sendmsg() fails with EMSGSIZE later on when sending the
batch to the kernel.

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

diff --git a/iptables/nft.c b/iptables/nft.c
index 9a3e9fdf4f12..2c61521455de 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -794,6 +794,7 @@ static int nft_restart(struct nft_handle *h)
 		return -1;
 
 	h->portid = mnl_socket_get_portid(h->nl);
+	nlbuffsiz = 0;
 
 	return 0;
 }
-- 
2.11.0