All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH iptables 0/6] ERESTART fixes
@ 2019-05-20 19:08 Pablo Neira Ayuso
  2019-05-20 19:08 ` [PATCH iptables 1/6] nft: keep original cache in case of ERESTART Pablo Neira Ayuso
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-20 19:08 UTC (permalink / raw)
  To: netfilter-devel; +Cc: phil, fw

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH iptables 1/6] nft: keep original cache in case of ERESTART
  2019-05-20 19:08 [PATCH iptables 0/6] ERESTART fixes Pablo Neira Ayuso
@ 2019-05-20 19:08 ` Pablo Neira Ayuso
  2019-05-20 19:08 ` [PATCH iptables 2/6] xtables: Fix for explicit rule flushes Pablo Neira Ayuso
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-20 19:08 UTC (permalink / raw)
  To: netfilter-devel; +Cc: phil, fw

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH iptables 2/6] xtables: Fix for explicit rule flushes
  2019-05-20 19:08 [PATCH iptables 0/6] ERESTART fixes Pablo Neira Ayuso
  2019-05-20 19:08 ` [PATCH iptables 1/6] nft: keep original cache in case of ERESTART Pablo Neira Ayuso
@ 2019-05-20 19:08 ` Pablo Neira Ayuso
  2019-05-20 19:08 ` [PATCH iptables 3/6] nft: don't skip table addition from ERESTART Pablo Neira Ayuso
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-20 19:08 UTC (permalink / raw)
  To: netfilter-devel; +Cc: phil, fw

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH iptables 3/6] nft: don't skip table addition from ERESTART
  2019-05-20 19:08 [PATCH iptables 0/6] ERESTART fixes Pablo Neira Ayuso
  2019-05-20 19:08 ` [PATCH iptables 1/6] nft: keep original cache in case of ERESTART Pablo Neira Ayuso
  2019-05-20 19:08 ` [PATCH iptables 2/6] xtables: Fix for explicit rule flushes Pablo Neira Ayuso
@ 2019-05-20 19:08 ` Pablo Neira Ayuso
  2019-05-20 19:08 ` [PATCH iptables 4/6] nft: don't care about previous state in ERESTART Pablo Neira Ayuso
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-20 19:08 UTC (permalink / raw)
  To: netfilter-devel; +Cc: phil, fw

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH iptables 4/6] nft: don't care about previous state in ERESTART
  2019-05-20 19:08 [PATCH iptables 0/6] ERESTART fixes Pablo Neira Ayuso
                   ` (2 preceding siblings ...)
  2019-05-20 19:08 ` [PATCH iptables 3/6] nft: don't skip table addition from ERESTART Pablo Neira Ayuso
@ 2019-05-20 19:08 ` Pablo Neira Ayuso
  2019-05-20 19:08 ` [PATCH iptables 5/6] nft: do not retry on EINTR Pablo Neira Ayuso
  2019-05-20 19:08 ` [PATCH iptables 6/6] nft: reset netlink sender buffer size of socket restart Pablo Neira Ayuso
  5 siblings, 0 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-20 19:08 UTC (permalink / raw)
  To: netfilter-devel; +Cc: phil, fw

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH iptables 5/6] nft: do not retry on EINTR
  2019-05-20 19:08 [PATCH iptables 0/6] ERESTART fixes Pablo Neira Ayuso
                   ` (3 preceding siblings ...)
  2019-05-20 19:08 ` [PATCH iptables 4/6] nft: don't care about previous state in ERESTART Pablo Neira Ayuso
@ 2019-05-20 19:08 ` Pablo Neira Ayuso
  2019-05-20 19:08 ` [PATCH iptables 6/6] nft: reset netlink sender buffer size of socket restart Pablo Neira Ayuso
  5 siblings, 0 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-20 19:08 UTC (permalink / raw)
  To: netfilter-devel; +Cc: phil, fw

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH iptables 6/6] nft: reset netlink sender buffer size of socket restart
  2019-05-20 19:08 [PATCH iptables 0/6] ERESTART fixes Pablo Neira Ayuso
                   ` (4 preceding siblings ...)
  2019-05-20 19:08 ` [PATCH iptables 5/6] nft: do not retry on EINTR Pablo Neira Ayuso
@ 2019-05-20 19:08 ` Pablo Neira Ayuso
  5 siblings, 0 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-20 19:08 UTC (permalink / raw)
  To: netfilter-devel; +Cc: phil, fw

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-05-20 19:08 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-20 19:08 [PATCH iptables 0/6] ERESTART fixes Pablo Neira Ayuso
2019-05-20 19:08 ` [PATCH iptables 1/6] nft: keep original cache in case of ERESTART Pablo Neira Ayuso
2019-05-20 19:08 ` [PATCH iptables 2/6] xtables: Fix for explicit rule flushes Pablo Neira Ayuso
2019-05-20 19:08 ` [PATCH iptables 3/6] nft: don't skip table addition from ERESTART Pablo Neira Ayuso
2019-05-20 19:08 ` [PATCH iptables 4/6] nft: don't care about previous state in ERESTART Pablo Neira Ayuso
2019-05-20 19:08 ` [PATCH iptables 5/6] nft: do not retry on EINTR Pablo Neira Ayuso
2019-05-20 19:08 ` [PATCH iptables 6/6] nft: reset netlink sender buffer size of socket restart Pablo Neira Ayuso

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.