[PATCH xtables-nft 0/6] handle parallel restore case

[PATCH xtables-nft 0/6] handle parallel restore case

[Florian Westphal]

This series handles the case of parallel iptables-nft-restore
invocations.

As xtables-nft ignores -w option (userspace-based serialization
lock), several nft instances can race.

In worse case, we can restore a ruleset twice, i.e. we get
duplicated rules:

1. "iptables-nft-restore < foo" is called for first time.
   No tables exist, so we create a transaction that creates
   the table/chains/rules.
2. "iptables-nft-restore < foo" is called in parallel.
   If the timing is right, it will also find no existing table,
   and will create a 2nd batch that adds rules.

   If the 2nd version doesn't run in parallel, it will detect
   that the table exists and will add a flush operation to the
   batch.

The first patches are preparation work, the fix is coming in patch 5,
it uses the generation counter to detect when the transaction batch
was based off an old state.

In case kernel indicates the generation id doesn't match anymore,
we fetch the current state and will refresh the pending transaction
before re-try.

The refresh currently includes adding/removing flush and chain add
operations.

Last patch adds a test script for this.

Florian Westphal (6):
      xtables: unify user chain add/flush for restore case
      xtables: add skip flag to objects
      xtables: add and use nft_build_cache
      xtables: add and set "implict" flag on transaction objects
      xtables: handle concurrent ruleset modifications
      tests: add test script for race-free restore

 nft-shared.h                                          |    7 
 nft.c                                                 |  325 +++++++++++++-----
 nft.h                                                 |    7 
 tests/shell/testcases/ipt-restore/0004-restore-race_0 |  119 ++++++
 xtables-restore.c                                     |   35 -
 xtables-translate.c                                   |    6 
 6 files changed, 379 insertions(+), 120 deletions(-)

[PATCH xtables-nft 1/6] xtables: unify user chain add/flush for restore case

[Florian Westphal]

The idea here is to move the 'flush' decision into the core, rather than
have the decision in the frontend.

This will be required later when "generation id" is passed to kernel.
In this case, we might have to add the flush when re-trying the
transaction.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 iptables/nft-shared.h        |  7 +--
 iptables/nft.c               | 82 +++++++++++++++++-------------------
 iptables/nft.h               |  4 +-
 iptables/xtables-restore.c   | 38 ++++++-----------
 iptables/xtables-translate.c |  6 +--
 5 files changed, 59 insertions(+), 78 deletions(-)

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index 019c1f20e293..de889ead7b60 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -245,14 +245,11 @@ struct nft_xt_restore_cb {
 	void (*table_new)(struct nft_handle *h, const char *table);
 	struct nftnl_chain_list *(*chain_list)(struct nft_handle *h,
 					       const char *table);
-	int (*chain_user_flush)(struct nft_handle *h,
-				struct nftnl_chain_list *clist,
-				const char *table, const char *chain);
 	int (*chain_set)(struct nft_handle *h, const char *table,
 			 const char *chain, const char *policy,
 			 const struct xt_counters *counters);
-	int (*chain_user_add)(struct nft_handle *h, const char *chain,
-			      const char *table);
+	int (*chain_restore)(struct nft_handle *h, const char *chain,
+			     const char *table);
 
 	int (*table_flush)(struct nft_handle *h, const char *table);
 
diff --git a/iptables/nft.c b/iptables/nft.c
index a297d9856001..16d7b7951969 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1621,49 +1621,6 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
 		nftnl_rule_free(r);
 }
 
-struct chain_user_flush_data {
-	struct nft_handle	*handle;
-	const char		*table;
-	const char		*chain;
-};
-
-static int __nft_chain_user_flush(struct nftnl_chain *c, void *data)
-{
-	const char *table_name = nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);
-	const char *chain_name = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
-	struct chain_user_flush_data *d = data;
-	struct nft_handle *h = d->handle;
-	const char *table = d->table;
-	const char *chain = d->chain;
-
-	if (strcmp(table, table_name) != 0)
-		return 0;
-
-	if (strcmp(chain, chain_name) != 0)
-		return 0;
-
-	if (!nftnl_chain_is_set(c, NFTNL_CHAIN_HOOKNUM))
-		__nft_rule_flush(h, table, chain, false);
-
-	return 0;
-}
-
-int nft_chain_user_flush(struct nft_handle *h, struct nftnl_chain_list *list,
-			 const char *table, const char *chain)
-{
-	struct chain_user_flush_data d = {
-		.handle = h,
-		.table	= table,
-		.chain  = chain,
-	};
-
-	nft_fn = nft_chain_user_flush;
-
-	nftnl_chain_list_foreach(list, __nft_chain_user_flush, &d);
-
-	return 1;
-}
-
 int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 		   bool verbose)
 {
@@ -1750,6 +1707,45 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
 	return ret == 0 ? 1 : 0;
 }
 
+int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table)
+{
+	struct nftnl_chain_list *list;
+	struct nftnl_chain *c;
+	bool created = false;
+	int ret;
+
+	c = nft_chain_find(h, table, chain);
+	if (c) {
+		/* Apparently -n still flushes existing user defined
+		 * chains that are redefined.
+		 */
+		if (h->noflush)
+			__nft_rule_flush(h, table, chain, false);
+	} else {
+		c = nftnl_chain_alloc();
+		if (!c)
+			return -1;
+
+		nftnl_chain_set(c, NFTNL_CHAIN_TABLE, (char *)table);
+		nftnl_chain_set(c, NFTNL_CHAIN_NAME, (char *)chain);
+		created = true;
+	}
+
+	if (h->family == NFPROTO_BRIDGE)
+		nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, NF_ACCEPT);
+
+	if (!created)
+		return 0;
+
+	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
+
+	list = nft_chain_list_get(h, table);
+	if (list)
+		nftnl_chain_list_add(c, list);
+
+	return ret;
+}
+
 /* From linux/netlink.h */
 #ifndef NLM_F_NONREC
 #define NLM_F_NONREC	0x100	/* Do not delete recursively    */
diff --git a/iptables/nft.h b/iptables/nft.h
index 56dc20760885..d428287b46ff 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -45,6 +45,7 @@ struct nft_handle {
 	} table[NFT_TABLE_MAX];
 	bool			have_cache;
 	bool			restore;
+	bool			noflush;
 	int8_t			config_done;
 
 	/* meta data, for error reporting */
@@ -87,8 +88,7 @@ struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
 int nft_chain_save(struct nft_handle *h, struct nftnl_chain_list *list);
 int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *table);
 int nft_chain_user_del(struct nft_handle *h, const char *chain, const char *table, bool verbose);
-int nft_chain_user_flush(struct nft_handle *h, struct nftnl_chain_list *list,
-			 const char *chain, const char *table);
+int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table);
 int nft_chain_user_rename(struct nft_handle *h, const char *chain, const char *table, const char *newname);
 int nft_chain_zero_counters(struct nft_handle *h, const char *chain, const char *table, bool verbose);
 const struct builtin_chain *nft_chain_builtin_find(const struct builtin_table *t, const char *chain);
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 6e6daffc9a1d..b12ab6a60b3a 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -19,7 +19,7 @@
 #include "nft-bridge.h"
 #include <libnftnl/chain.h>
 
-static int counters, verbose, noflush;
+static int counters, verbose;
 
 /* Keeping track of external matches and targets.  */
 static const struct option options[] = {
@@ -74,10 +74,9 @@ struct nft_xt_restore_cb restore_cb = {
 	.abort		= nft_abort,
 	.table_new	= nft_table_new,
 	.table_flush	= nft_table_flush,
-	.chain_user_flush = nft_chain_user_flush,
 	.do_command	= do_commandx,
 	.chain_set	= nft_chain_set,
-	.chain_user_add	= nft_chain_user_add,
+	.chain_restore  = nft_chain_restore,
 };
 
 static const struct xtc_ops xtc_ops = {
@@ -93,7 +92,6 @@ void xtables_restore_parse(struct nft_handle *h,
 	char buffer[10240];
 	int in_table = 0;
 	const struct xtc_ops *ops = &xtc_ops;
-	struct nftnl_chain_list *chain_list = NULL;
 
 	line = 0;
 
@@ -147,10 +145,12 @@ void xtables_restore_parse(struct nft_handle *h,
 			if (p->tablename && (strcmp(p->tablename, table) != 0))
 				continue;
 
-			if (cb->chain_list)
-				chain_list = cb->chain_list(h, table);
+			/* Fixme: Needed to init chain cache.
+			 * Should create explicit function to do this.
+			 */
+			nft_chain_list_get(h, table);
 
-			if (noflush == 0) {
+			if (h->noflush == 0) {
 				DEBUGP("Cleaning all chains of table '%s'\n",
 					table);
 				if (cb->table_flush)
@@ -214,19 +214,7 @@ void xtables_restore_parse(struct nft_handle *h,
 				}
 				DEBUGP("Setting policy of chain %s to %s\n",
 				       chain, policy);
-
-			} else if (noflush &&
-				   nftnl_chain_list_lookup_byname(chain_list, chain)) {
-				/* Apparently -n still flushes existing user
-				 * defined chains that are redefined. Otherwise,
-				 * leave them as is.
-				 */
-				if (cb->chain_user_flush)
-					cb->chain_user_flush(h, chain_list,
-							     curtable->name, chain);
-			} else if (cb->chain_user_add &&
-				   cb->chain_user_add(h, chain,
-						      curtable->name) < 0 &&
+			} else if (cb->chain_restore(h, chain, curtable->name) < 0 &&
 				   errno != EEXIST) {
 				xtables_error(PARAMETER_PROBLEM,
 					      "cannot create chain "
@@ -380,7 +368,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
 					    IPTABLES_VERSION);
 				exit(0);
 			case 'n':
-				noflush = 1;
+				h.noflush = 1;
 				break;
 			case 'M':
 				xtables_modprobe_program = optarg;
@@ -480,10 +468,9 @@ struct nft_xt_restore_cb ebt_restore_cb = {
 	.commit		= nft_commit,
 	.table_new	= nft_table_new,
 	.table_flush	= ebt_table_flush,
-	.chain_user_flush = nft_chain_user_flush,
 	.do_command	= do_commandeb,
 	.chain_set	= nft_chain_set,
-	.chain_user_add	= nft_chain_user_add,
+	.chain_restore  = nft_chain_restore,
 };
 
 static const struct option ebt_restore_options[] = {
@@ -496,6 +483,7 @@ int xtables_eb_restore_main(int argc, char *argv[])
 	struct nft_xt_restore_parse p = {
 		.in = stdin,
 	};
+	bool noflush = false;
 	struct nft_handle h;
 	int c;
 
@@ -514,6 +502,7 @@ int xtables_eb_restore_main(int argc, char *argv[])
 	}
 
 	nft_init_eb(&h, "ebtables-restore");
+	h.noflush = noflush;
 	xtables_restore_parse(&h, &p, &ebt_restore_cb, argc, argv);
 	nft_fini(&h);
 
@@ -525,10 +514,9 @@ struct nft_xt_restore_cb arp_restore_cb = {
 	.commit		= nft_commit,
 	.table_new	= nft_table_new,
 	.table_flush	= nft_table_flush,
-	.chain_user_flush = nft_chain_user_flush,
 	.do_command	= do_commandarp,
 	.chain_set	= nft_chain_set,
-	.chain_user_add	= nft_chain_user_add,
+	.chain_restore  = nft_chain_restore,
 };
 
 int xtables_arp_restore_main(int argc, char *argv[])
diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
index e1d2a7d6cce8..eb35890afa78 100644
--- a/iptables/xtables-translate.c
+++ b/iptables/xtables-translate.c
@@ -329,8 +329,8 @@ static const struct option options[] = {
 	{ NULL },
 };
 
-static int xlate_chain_user_add(struct nft_handle *h, const char *chain,
-				const char *table)
+static int xlate_chain_user_restore(struct nft_handle *h, const char *chain,
+				    const char *table)
 {
 	printf("add chain %s %s %s\n", family2str[h->family], table, chain);
 	return 0;
@@ -416,7 +416,7 @@ static int dummy_compat_rev(const char *name, uint8_t rev, int opt)
 static struct nft_xt_restore_cb cb_xlate = {
 	.table_new	= xlate_table_new,
 	.chain_set	= xlate_chain_set,
-	.chain_user_add	= xlate_chain_user_add,
+	.chain_restore	= xlate_chain_user_restore,
 	.do_command	= do_command_xlate,
 	.commit		= commit,
 	.abort		= commit,
-- 
2.21.0

[PATCH xtables-nft 2/6] xtables: add skip flag to objects

[Florian Westphal]

This will be used to skip transaction objects when committing to
kernel.  This is needed for example when we restore a table that
doesn't exist yet.  In such a case we would already build a flush
operation so we can just enable it when we hit problem with the
generation id and we find that the table/chain was already created
in the mean time.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 iptables/nft.c | 49 +++++++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 16d7b7951969..1cef7a13c90d 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -264,7 +264,8 @@ enum obj_action {
 
 struct obj_update {
 	struct list_head	head;
-	enum obj_update_type	type;
+	enum obj_update_type	type:8;
+	uint8_t			skip:1;
 	unsigned int		seq;
 	union {
 		struct nftnl_table	*table;
@@ -342,13 +343,13 @@ static int mnl_append_error(const struct nft_handle *h,
 	return snprintf(buf, len, "%s: %s", errmsg, tcr);
 }
 
-static int batch_add(struct nft_handle *h, enum obj_update_type type, void *ptr)
+static struct obj_update *batch_add(struct nft_handle *h, enum obj_update_type type, void *ptr)
 {
 	struct obj_update *obj;
 
 	obj = calloc(1, sizeof(struct obj_update));
 	if (obj == NULL)
-		return -1;
+		return NULL;
 
 	obj->ptr = ptr;
 	obj->error.lineno = h->error.lineno;
@@ -356,11 +357,12 @@ static int batch_add(struct nft_handle *h, enum obj_update_type type, void *ptr)
 	list_add_tail(&obj->head, &h->obj_list);
 	h->obj_list_num++;
 
-	return 0;
+	return obj;
 }
 
-static int batch_table_add(struct nft_handle *h, enum obj_update_type type,
-			   struct nftnl_table *t)
+static struct obj_update *
+batch_table_add(struct nft_handle *h, enum obj_update_type type,
+		struct nftnl_table *t)
 {
 	return batch_add(h, type, t);
 }
@@ -368,13 +370,13 @@ static int batch_table_add(struct nft_handle *h, enum obj_update_type type,
 static int batch_chain_add(struct nft_handle *h, enum obj_update_type type,
 			   struct nftnl_chain *c)
 {
-	return batch_add(h, type, c);
+	return batch_add(h, type, c) ? 0 : -1;
 }
 
 static int batch_rule_add(struct nft_handle *h, enum obj_update_type type,
 			  struct nftnl_rule *r)
 {
-	return batch_add(h, type, r);
+	return batch_add(h, type, r) ? 0 : -1;
 }
 
 const struct builtin_table xtables_ipv4[NFT_TABLE_MAX] = {
@@ -609,7 +611,7 @@ static int nft_table_builtin_add(struct nft_handle *h,
 
 	nftnl_table_set(t, NFTNL_TABLE_NAME, (char *)_t->name);
 
-	ret = batch_table_add(h, NFT_COMPAT_TABLE_ADD, t);
+	ret = batch_table_add(h, NFT_COMPAT_TABLE_ADD, t) ? 0 : - 1;
 
 	return ret;
 }
@@ -2000,9 +2002,10 @@ int nft_for_each_table(struct nft_handle *h,
 	return 0;
 }
 
-static int __nft_table_flush(struct nft_handle *h, const char *table)
+static int __nft_table_flush(struct nft_handle *h, const char *table, bool exists)
 {
 	const struct builtin_table *_t;
+	struct obj_update *obj;
 	struct nftnl_table *t;
 
 	t = nftnl_table_alloc();
@@ -2011,7 +2014,14 @@ static int __nft_table_flush(struct nft_handle *h, const char *table)
 
 	nftnl_table_set_str(t, NFTNL_TABLE_NAME, table);
 
-	batch_table_add(h, NFT_COMPAT_TABLE_FLUSH, t);
+	obj = batch_table_add(h, NFT_COMPAT_TABLE_FLUSH, t);
+	if (!obj) {
+		nftnl_table_free(t);
+		return -1;
+	}
+
+	if (!exists)
+		obj->skip = 1;
 
 	_t = nft_table_builtin_find(h, table);
 	assert(_t);
@@ -2027,6 +2037,7 @@ int nft_table_flush(struct nft_handle *h, const char *table)
 	struct nftnl_table_list_iter *iter;
 	struct nftnl_table_list *list;
 	struct nftnl_table *t;
+	bool exists = false;
 	int ret = 0;
 
 	nft_fn = nft_table_flush;
@@ -2048,17 +2059,15 @@ int nft_table_flush(struct nft_handle *h, const char *table)
 		const char *table_name =
 			nftnl_table_get_str(t, NFTNL_TABLE_NAME);
 
-		if (strcmp(table_name, table) != 0)
-			goto next;
+		if (strcmp(table_name, table) == 0) {
+			exists = true;
+			break;
+		}
 
-		ret = __nft_table_flush(h, table);
-		if (ret < 0)
-			goto err_table_iter;
-next:
 		t = nftnl_table_list_iter_next(iter);
 	}
 
-err_table_iter:
+	ret = __nft_table_flush(h, table, exists);
 	nftnl_table_list_iter_destroy(iter);
 err_table_list:
 	nftnl_table_list_free(list);
@@ -2658,6 +2667,10 @@ static int nft_action(struct nft_handle *h, int action)
 	mnl_batch_begin(h->batch, seq++);
 
 	list_for_each_entry(n, &h->obj_list, head) {
+
+		if (n->skip)
+			continue;
+
 		n->seq = seq++;
 		switch (n->type) {
 		case NFT_COMPAT_TABLE_ADD:
-- 
2.21.0

[PATCH xtables-nft 3/6] xtables: add and use nft_build_cache

[Florian Westphal]

Will be used with the "generation id" infrastructure.
When we're told that the commit failed because someone else made
changes, we can use this to re-initialize the cache and then
revalidate the transaction list (e.g. to detect that we now have
to flush the user-defined chain 'foo' that we wanted to create, but
was added just now by someone else).

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 iptables/nft.c             | 28 +++++++++++++++++++++++-----
 iptables/nft.h             |  2 ++
 iptables/xtables-restore.c |  5 +----
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 1cef7a13c90d..4c9ce1a29383 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1486,6 +1486,28 @@ static int fetch_rule_cache(struct nft_handle *h)
 	return 0;
 }
 
+static void __nft_build_cache(struct nft_handle *h)
+{
+	fetch_chain_cache(h);
+	fetch_rule_cache(h);
+	h->have_cache = true;
+}
+
+
+void nft_build_cache(struct nft_handle *h)
+{
+	if (!h->have_cache)
+		__nft_build_cache(h);
+}
+
+void nft_rebuild_cache(struct nft_handle *h)
+{
+	if (!h->have_cache)
+		flush_chain_cache(h, NULL);
+
+	__nft_build_cache(h);
+}
+
 struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
 					    const char *table)
 {
@@ -1495,11 +1517,7 @@ struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
 	if (!t)
 		return NULL;
 
-	if (!h->have_cache) {
-		fetch_chain_cache(h);
-		fetch_rule_cache(h);
-		h->have_cache = true;
-	}
+	nft_build_cache(h);
 
 	return h->table[t->type].chain_cache;
 }
diff --git a/iptables/nft.h b/iptables/nft.h
index d428287b46ff..97c28b356a46 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -63,6 +63,8 @@ int mnl_talk(struct nft_handle *h, struct nlmsghdr *nlh,
 	     void *data);
 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.
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index b12ab6a60b3a..a6a331d39868 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -145,10 +145,7 @@ void xtables_restore_parse(struct nft_handle *h,
 			if (p->tablename && (strcmp(p->tablename, table) != 0))
 				continue;
 
-			/* Fixme: Needed to init chain cache.
-			 * Should create explicit function to do this.
-			 */
-			nft_chain_list_get(h, table);
+			nft_build_cache(h);
 
 			if (h->noflush == 0) {
 				DEBUGP("Cleaning all chains of table '%s'\n",
-- 
2.21.0

[PATCH xtables-nft 4/6] xtables: add and set "implict" flag on transaction objects

[Florian Westphal]

Its used to flag the rule flushes that get added in user-defined-chains
that get redefined with --noflush.

IOW, those objects that are added not by explicit instruction but
to keep semantics.

With --noflush, iptables-legacy-restore will behave as if
-F USERCHAIN was given, in case USERCHAIN exists and USERCHAIN gets
redefined, i.e.:

 iptables-save v1.8.2 on Thu Apr 18 17:11:05 2019
*filter
:USERCHAIN - [0:0]
COMMIT

... will remove all existing rules from USERCHAIN.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 iptables/nft.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 4c9ce1a29383..da73e87011db 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -266,6 +266,7 @@ struct obj_update {
 	struct list_head	head;
 	enum obj_update_type	type:8;
 	uint8_t			skip:1;
+	uint8_t			implicit:1;
 	unsigned int		seq;
 	union {
 		struct nftnl_table	*table;
@@ -373,10 +374,11 @@ static int batch_chain_add(struct nft_handle *h, enum obj_update_type type,
 	return batch_add(h, type, c) ? 0 : -1;
 }
 
-static int batch_rule_add(struct nft_handle *h, enum obj_update_type type,
+static struct obj_update *
+batch_rule_add(struct nft_handle *h, enum obj_update_type type,
 			  struct nftnl_rule *r)
 {
-	return batch_add(h, type, r) ? 0 : -1;
+	return batch_add(h, type, r);
 }
 
 const struct builtin_table xtables_ipv4[NFT_TABLE_MAX] = {
@@ -1215,7 +1217,7 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
 	} else
 		type = NFT_COMPAT_RULE_APPEND;
 
-	if (batch_rule_add(h, type, r) < 0) {
+	if (batch_rule_add(h, type, r) == NULL) {
 		nftnl_rule_free(r);
 		return 0;
 	}
@@ -1402,7 +1404,7 @@ static void nft_bridge_chain_postprocess(struct nft_handle *h,
 	}
 
 	nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, verdict);
-	if (batch_rule_add(h, NFT_COMPAT_RULE_DELETE, last) < 0)
+	if (batch_rule_add(h, NFT_COMPAT_RULE_DELETE, last) == NULL)
 		fprintf(stderr, "Failed to delete old policy rule\n");
 	nftnl_chain_rule_del(last);
 out_iter:
@@ -1623,8 +1625,9 @@ int nft_rule_save(struct nft_handle *h, const char *table, unsigned int format)
 
 static void
 __nft_rule_flush(struct nft_handle *h, const char *table,
-		 const char *chain, bool verbose)
+		 const char *chain, bool verbose, bool implicit)
 {
+	struct obj_update *obj;
 	struct nftnl_rule *r;
 
 	if (verbose)
@@ -1637,8 +1640,13 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
 	nftnl_rule_set(r, NFTNL_RULE_TABLE, (char *)table);
 	nftnl_rule_set(r, NFTNL_RULE_CHAIN, (char *)chain);
 
-	if (batch_rule_add(h, NFT_COMPAT_RULE_FLUSH, r) < 0)
+	obj = batch_rule_add(h, NFT_COMPAT_RULE_FLUSH, r);
+	if (!obj) {
 		nftnl_rule_free(r);
+		return;
+	}
+
+	obj->implicit = 1;
 }
 
 int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
@@ -1665,7 +1673,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 		if (!c)
 			return 0;
 
-		__nft_rule_flush(h, table, chain, verbose);
+		__nft_rule_flush(h, table, chain, verbose, false);
 		flush_rule_cache(c);
 		return 1;
 	}
@@ -1681,7 +1689,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 		const char *chain_name =
 			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
 
-		__nft_rule_flush(h, table, chain_name, verbose);
+		__nft_rule_flush(h, table, chain_name, verbose, false);
 		flush_rule_cache(c);
 		c = nftnl_chain_list_iter_next(iter);
 	}
@@ -1740,7 +1748,7 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
 		 * chains that are redefined.
 		 */
 		if (h->noflush)
-			__nft_rule_flush(h, table, chain, false);
+			__nft_rule_flush(h, table, chain, false, true);
 	} else {
 		c = nftnl_chain_alloc();
 		if (!c)
@@ -2102,12 +2110,12 @@ void nft_table_new(struct nft_handle *h, const char *table)
 
 static int __nft_rule_del(struct nft_handle *h, struct nftnl_rule *r)
 {
-	int ret;
+	struct obj_update *obj;
 
 	nftnl_rule_list_del(r);
 
-	ret = batch_rule_add(h, NFT_COMPAT_RULE_DELETE, r);
-	if (ret < 0) {
+	obj = batch_rule_add(h, NFT_COMPAT_RULE_DELETE, r);
+	if (!obj) {
 		nftnl_rule_free(r);
 		return -1;
 	}
@@ -2223,7 +2231,7 @@ nft_rule_add(struct nft_handle *h, const char *chain,
 		}
 	}
 
-	if (batch_rule_add(h, NFT_COMPAT_RULE_INSERT, r) < 0) {
+	if (!batch_rule_add(h, NFT_COMPAT_RULE_INSERT, r)) {
 		nftnl_rule_free(r);
 		return NULL;
 	}
@@ -2848,7 +2856,7 @@ static int ebt_add_policy_rule(struct nftnl_chain *c, void *data)
 			    nftnl_udata_buf_len(udata));
 	nftnl_udata_buf_free(udata);
 
-	if (batch_rule_add(h, NFT_COMPAT_RULE_APPEND, r) < 0) {
+	if (!batch_rule_add(h, NFT_COMPAT_RULE_APPEND, r)) {
 		nftnl_rule_free(r);
 		return -1;
 	}
@@ -3191,7 +3199,6 @@ static int __nft_chain_zero_counters(struct nftnl_chain *c, void *data)
 	struct nft_handle *h = d->handle;
 	struct nftnl_rule_iter *iter;
 	struct nftnl_rule *r;
-	int ret = 0;
 
 	if (d->verbose)
 		fprintf(stdout, "Zeroing chain `%s'\n",
@@ -3202,8 +3209,7 @@ static int __nft_chain_zero_counters(struct nftnl_chain *c, void *data)
 		nftnl_chain_set_u64(c, NFTNL_CHAIN_PACKETS, 0);
 		nftnl_chain_set_u64(c, NFTNL_CHAIN_BYTES, 0);
 		nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE);
-		ret = batch_chain_add(h, NFT_COMPAT_CHAIN_ZERO, c);
-		if (ret)
+		if (batch_chain_add(h, NFT_COMPAT_CHAIN_ZERO, c))
 			return -1;
 	}
 
@@ -3245,8 +3251,7 @@ static int __nft_chain_zero_counters(struct nftnl_chain *c, void *data)
 			 * rule based on its handle only.
 			 */
 			nftnl_rule_unset(r, NFTNL_RULE_POSITION);
-			ret = batch_rule_add(h, NFT_COMPAT_RULE_REPLACE, r);
-			if (ret)
+			if (!batch_rule_add(h, NFT_COMPAT_RULE_REPLACE, r))
 				return -1;
 		}
 		r = nftnl_rule_iter_next(iter);
-- 
2.21.0

[PATCH xtables-nft 5/6] xtables: handle concurrent ruleset modifications

[Florian Westphal]

We currently race when several xtables-nft-restore processes attempt to
handle rules in parallel.  For instance, when no rules are present at
all, then

iptables-nft-restore < X & iptables-nft-restore < X

... can cause rules to be restored twice.

Reason is that both processes might detect 'no rules exist', so
neither issues a flush operation.

We can't unconditionally issue the flush, because it would
cause kernel to fail with -ENOENT unless the to-be-flushed table
exists.

This change passes the generation id that was used to build
the transaction to the kernel.

In case another process changed *any* rule somewhere, the transaction
will now fail with -ERESTART.

We then flush the cache, re-fetch the ruleset and refresh
our transaction.

For example, in the above 'parallel restore' case, the iptables-restore
instance that lost the race would detect that the table has been created
already, and would add the needed flush.

In a similar vein, in case --noflush is used, we will add the flush
op for user-defined chains that were created in the mean-time.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 iptables/nft.c | 131 +++++++++++++++++++++++++++++++++++++++++++++++--
 iptables/nft.h |   1 +
 2 files changed, 128 insertions(+), 4 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index da73e87011db..6354b7e8e72f 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -41,6 +41,7 @@
 #include <linux/netfilter/xt_limit.h>
 
 #include <libmnl/libmnl.h>
+#include <libnftnl/gen.h>
 #include <libnftnl/table.h>
 #include <libnftnl/chain.h>
 #include <libnftnl/rule.h>
@@ -60,6 +61,36 @@
 
 static void *nft_fn;
 
+static int genid_cb(const struct nlmsghdr *nlh, void *data)
+{
+	struct nft_handle *h = data;
+	struct nftnl_gen *gen;
+
+	gen = nftnl_gen_alloc();
+	if (!gen)
+		return MNL_CB_ERROR;
+
+	if (nftnl_gen_nlmsg_parse(nlh, gen) < 0)
+		goto out;
+
+	h->nft_genid = nftnl_gen_get_u32(gen, NFTNL_GEN_ID);
+
+	nftnl_gen_free(gen);
+	return MNL_CB_STOP;
+out:
+	nftnl_gen_free(gen);
+	return MNL_CB_ERROR;
+}
+
+static int mnl_genid_get(struct nft_handle *h)
+{
+	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);
+}
+
 int mnl_talk(struct nft_handle *h, struct nlmsghdr *nlh,
 	     int (*cb)(const struct nlmsghdr *nlh, void *data),
 	     void *data)
@@ -109,9 +140,14 @@ static void mnl_nft_batch_continue(struct nftnl_batch *batch)
 	assert(nftnl_batch_update(batch) >= 0);
 }
 
-static uint32_t mnl_batch_begin(struct nftnl_batch *batch, uint32_t seqnum)
+static uint32_t mnl_batch_begin(struct nftnl_batch *batch, uint32_t genid, uint32_t seqnum)
 {
-	nftnl_batch_begin(nftnl_batch_buffer(batch), seqnum);
+	struct nlmsghdr *nlh;
+
+	nlh = nftnl_batch_begin(nftnl_batch_buffer(batch), seqnum);
+
+	mnl_attr_put_u32(nlh, NFTA_GEN_ID, htonl(genid));
+
 	mnl_nft_batch_continue(batch);
 
 	return seqnum;
@@ -1490,6 +1526,7 @@ static int fetch_rule_cache(struct nft_handle *h)
 
 static void __nft_build_cache(struct nft_handle *h)
 {
+	mnl_genid_get(h);
 	fetch_chain_cache(h);
 	fetch_rule_cache(h);
 	h->have_cache = true;
@@ -2678,6 +2715,76 @@ static void batch_obj_del(struct nft_handle *h, struct obj_update *o)
 	free(o);
 }
 
+static void nft_refresh_transaction(struct nft_handle *h)
+{
+	const char *tablename, *chainname;
+	const struct nftnl_chain *c;
+	struct obj_update *n, *tmp;
+	bool exists;
+
+	h->error.lineno = 0;
+
+	list_for_each_entry_safe(n, tmp, &h->obj_list, head) {
+		if (n->implicit) {
+			batch_obj_del(h, n);
+			continue;
+		}
+
+		switch (n->type) {
+		case NFT_COMPAT_TABLE_FLUSH:
+			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;
+			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)
+				continue;
+
+			chainname = nftnl_chain_get_str(n->chain, NFTNL_CHAIN_NAME);
+			if (!chainname)
+				continue;
+
+			c = nft_chain_find(h, tablename, chainname);
+			if (c && !n->skip) {
+				/* -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) {
+				n->skip = 0;
+			}
+			break;
+		case NFT_COMPAT_CHAIN_ADD:
+		case NFT_COMPAT_CHAIN_ZERO:
+		case NFT_COMPAT_CHAIN_USER_DEL:
+		case NFT_COMPAT_CHAIN_USER_FLUSH:
+		case NFT_COMPAT_CHAIN_UPDATE:
+		case NFT_COMPAT_CHAIN_RENAME:
+		case NFT_COMPAT_RULE_APPEND:
+		case NFT_COMPAT_RULE_INSERT:
+		case NFT_COMPAT_RULE_REPLACE:
+		case NFT_COMPAT_RULE_DELETE:
+		case NFT_COMPAT_RULE_FLUSH:
+			break;
+		}
+	}
+}
+
 static int nft_action(struct nft_handle *h, int action)
 {
 	struct obj_update *n, *tmp;
@@ -2685,12 +2792,15 @@ static int nft_action(struct nft_handle *h, int action)
 	unsigned int buflen, i, len;
 	bool show_errors = true;
 	char errmsg[1024];
-	uint32_t seq = 1;
+	uint32_t seq;
 	int ret = 0;
 
+retry:
+	seq = 1;
 	h->batch = mnl_batch_init();
 
-	mnl_batch_begin(h->batch, seq++);
+	mnl_batch_begin(h->batch, h->nft_genid, seq++);
+	h->nft_genid++;
 
 	list_for_each_entry(n, &h->obj_list, head) {
 
@@ -2773,7 +2883,20 @@ static int nft_action(struct nft_handle *h, int action)
 		break;
 	}
 
+	errno = 0;
 	ret = mnl_batch_talk(h->nl, h->batch, &h->err_list);
+	if (ret && errno == ERESTART) {
+		nft_rebuild_cache(h);
+
+		nft_refresh_transaction(h);
+
+		i=0;
+		list_for_each_entry_safe(err, ne, &h->err_list, head)
+			mnl_err_list_free(err);
+
+		mnl_batch_reset(h->batch);
+		goto retry;
+	}
 
 	i = 0;
 	buflen = sizeof(errmsg);
diff --git a/iptables/nft.h b/iptables/nft.h
index 97c28b356a46..23bd2b79884c 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -32,6 +32,7 @@ struct nft_handle {
 	struct mnl_socket	*nl;
 	uint32_t		portid;
 	uint32_t		seq;
+	uint32_t		nft_genid;
 	uint32_t		rule_id;
 	struct list_head	obj_list;
 	int			obj_list_num;
-- 
2.21.0

[PATCH xtables-nft 6/6] tests: add test script for race-free restore

[Florian Westphal]

xtables-nft-restore ignores -w, check that we don't add
duplicate rules when parallel restores happen.

With a slightly older iptables-nft version this ususally fails with:
I: [EXECUTING] iptables/tests/shell/testcases/ipt-restore/0004-restore-race_0
iptables-restore v1.8.2 (nf_tables):
line 5: CHAIN_USER_ADD failed (File exists): chain UC-0
line 6: CHAIN_USER_ADD failed (File exists): chain UC-1
W: [FAILED] ipt-restore/0004-restore-race_0: expected 0 but got 4

or
I: [EXECUTING]   iptables/tests/shell/testcases/ipt-restore/0004-restore-race_0
iptables-restore v1.8.2 (nf_tables):
line 1: TABLE_FLUSH failed (No such file or directory): table filter

or
/tmp/tmp.SItN4URxxF /tmp/tmp.P1y4LIxhTl differ: byte 7159, line 137

As the legacy version should not have such race (due to nature
of full-table-replace), only do one iteration for legacy case.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 .../testcases/ipt-restore/0004-restore-race_0 | 119 ++++++++++++++++++
 1 file changed, 119 insertions(+)
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0004-restore-race_0

diff --git a/iptables/tests/shell/testcases/ipt-restore/0004-restore-race_0 b/iptables/tests/shell/testcases/ipt-restore/0004-restore-race_0
new file mode 100755
index 000000000000..14b910eb373b
--- /dev/null
+++ b/iptables/tests/shell/testcases/ipt-restore/0004-restore-race_0
@@ -0,0 +1,119 @@
+#!/bin/bash
+
+export XTABLES_LIBDIR=$(pwd)/extensions
+have_nft=false
+nft -v > /dev/null && have_nft=true
+
+dumpfile=""
+tmpfile=""
+
+set -e
+
+clean()
+{
+	$XT_MULTI iptables -t filter -F
+	$XT_MULTI iptables -t filter -X
+	$have_nft && nft flush ruleset
+}
+
+clean_tempfile()
+{
+	[ -n "${tmpfile}" ] && rm -f "${tmpfile}"
+	[ -n "${dumpfile}" ] && rm -f "${dumpfile}"
+	clean
+}
+
+trap clean_tempfile EXIT
+
+ENTRY_NUM=$((RANDOM%100))
+UCHAIN_NUM=$((RANDOM%10))
+
+get_target()
+{
+	if [ $UCHAIN_NUM -eq 0 ]; then
+		echo -n "ACCEPT"
+		return
+	fi
+
+
+	x=$((RANDOM%2))
+	if [ $x -eq 0 ];then
+		echo -n "ACCEPT"
+	else
+		printf -- "UC-%x" $((RANDOM%UCHAIN_NUM))
+	fi
+}
+
+make_dummy_rules()
+{
+
+	echo "*filter"
+	echo ":INPUT ACCEPT [0:0]"
+	echo ":FORWARD ACCEPT [0:0]"
+	echo ":OUTPUT ACCEPT [0:0]"
+
+	if [ $UCHAIN_NUM -gt 0 ]; then
+		for i in $(seq 0 $UCHAIN_NUM); do
+			printf -- ":UC-%x - [0:0]\n" $i
+		done
+	fi
+
+	for proto in tcp udp sctp; do
+		for i in $(seq 0 $ENTRY_NUM); do
+			t=$(get_target)
+			printf -- "-A INPUT -i lo -p $proto --dport %d -j %s\n" $((61000-i)) $t
+			t=$(get_target)
+			printf -- "-A FORWARD -i lo -o lo -p $proto --dport %d -j %s\n" $((61000-i)) $t
+			t=$(get_target)
+			printf -- "-A OUTPUT -o lo -p $proto --dport %d -j %s\n" $((61000-i)) $t
+			[ $UCHAIN_NUM -gt 0 ] && printf -- "-A UC-%x -j ACCEPT\n" $((RANDOM%UCHAIN_NUM))
+		done
+	done
+	echo COMMIT
+}
+
+tmpfile=$(mktemp) || exit 1
+dumpfile=$(mktemp) || exit 1
+
+make_dummy_rules > $dumpfile
+$XT_MULTI iptables-restore -w < $dumpfile
+LINES=$(wc -l < $dumpfile)
+$XT_MULTI iptables-save | grep -v '^#' > $dumpfile
+LINES2=$(wc -l < $dumpfile)
+
+if [ $LINES -ne $LINES2 ]; then
+	echo "Original dump has $LINES, not $LINES2" 1>&2
+	exit 111
+fi
+
+case "$XT_MULTI" in
+*/xtables-nft-multi)
+	attempts=$((RANDOM%200))
+	attempts=$((attempts+1))
+	;;
+*)
+	attempts=1
+	;;
+esac
+
+while [ $attempts -gt 0 ]; do
+	attempts=$((attempts-1))
+
+	clean
+
+	for i in $(seq 1 10); do
+		$XT_MULTI iptables-restore -w 15 < $dumpfile &
+	done
+
+	for i in $(seq 1 10); do
+		# causes exit in case ipt-restore failed (runs with set -e)
+		wait %$i
+	done
+
+	$XT_MULTI iptables-save | grep -v '^#' > $tmpfile
+
+	clean
+	cmp $tmpfile $dumpfile
+done
+
+exit 0
-- 
2.21.0

Re: [PATCH xtables-nft 0/6] handle parallel restore case

[Pablo Neira Ayuso]

On Tue, Apr 23, 2019 at 03:16:19PM +0200, Florian Westphal wrote:
> This series handles the case of parallel iptables-nft-restore
> invocations.
> 
> As xtables-nft ignores -w option (userspace-based serialization
> lock), several nft instances can race.
> 
> In worse case, we can restore a ruleset twice, i.e. we get
> duplicated rules:
> 
> 1. "iptables-nft-restore < foo" is called for first time.
>    No tables exist, so we create a transaction that creates
>    the table/chains/rules.
> 2. "iptables-nft-restore < foo" is called in parallel.
>    If the timing is right, it will also find no existing table,
>    and will create a 2nd batch that adds rules.
> 
>    If the 2nd version doesn't run in parallel, it will detect
>    that the table exists and will add a flush operation to the
>    batch.
> 
> The first patches are preparation work, the fix is coming in patch 5,
> it uses the generation counter to detect when the transaction batch
> was based off an old state.
> 
> In case kernel indicates the generation id doesn't match anymore,
> we fetch the current state and will refresh the pending transaction
> before re-try.
> 
> The refresh currently includes adding/removing flush and chain add
> operations.
> 
> Last patch adds a test script for this.

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