Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [iptables PATCH 0/3] nft: Fix transaction refreshing
@ 2020-10-05 14:48 Phil Sutter
  2020-10-05 14:48 ` [iptables PATCH 1/3] nft: Make batch_add_chain() return the added batch object Phil Sutter
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Phil Sutter @ 2020-10-05 14:48 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal

With iptables-nft-restore in --noflush mode, the created batch job list
may need to be adjusted to a changing ruleset in kernel. In particular,
an input line like ':FOO - [0:0]' either means "flush chain FOO" or
"create chain FOO" depending on whether it exists already or not. Patch
3 contains a test case provoking this peculiar situation and fixes the
transaction prepare and refresh logic in that case. Patch 1 is a simple
preparation change, patch 2 a somewhat related fix for error reporting
with refreshed transactions.

Phil Sutter (3):
  nft: Make batch_add_chain() return the added batch object
  nft: Fix error reporting for refreshed transactions
  nft: Fix for concurrent noflush restore calls

 iptables/nft.c                                | 96 ++++++++++---------
 .../ipt-restore/0016-concurrent-restores_0    | 53 ++++++++++
 2 files changed, 102 insertions(+), 47 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0016-concurrent-restores_0

-- 
2.28.0


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

* [iptables PATCH 1/3] nft: Make batch_add_chain() return the added batch object
  2020-10-05 14:48 [iptables PATCH 0/3] nft: Fix transaction refreshing Phil Sutter
@ 2020-10-05 14:48 ` Phil Sutter
  2020-10-05 21:07   ` Florian Westphal
  2020-10-05 14:48 ` [iptables PATCH 2/3] nft: Fix error reporting for refreshed transactions Phil Sutter
  2020-10-05 14:48 ` [iptables PATCH 3/3] nft: Fix for concurrent noflush restore calls Phil Sutter
  2 siblings, 1 reply; 11+ messages in thread
From: Phil Sutter @ 2020-10-05 14:48 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal

Do this so in a later patch the 'skip' field can be adjusted.

While being at it, simplify a few callers and eliminate the need for a
'ret' variable.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 775ace385c184..09421cf4eaaec 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -388,10 +388,11 @@ batch_set_add(struct nft_handle *h, enum obj_update_type type,
 	return batch_add(h, type, s);
 }
 
-static int batch_chain_add(struct nft_handle *h, enum obj_update_type type,
+static struct obj_update *
+batch_chain_add(struct nft_handle *h, enum obj_update_type type,
 			   struct nftnl_chain *c)
 {
-	return batch_add(h, type, c) ? 0 : -1;
+	return batch_add(h, type, c);
 }
 
 static struct obj_update *
@@ -905,7 +906,6 @@ int nft_chain_set(struct nft_handle *h, const char *table,
 		  const struct xt_counters *counters)
 {
 	struct nftnl_chain *c = NULL;
-	int ret;
 
 	nft_fn = nft_chain_set;
 
@@ -932,10 +932,11 @@ int nft_chain_set(struct nft_handle *h, const char *table,
 	if (c == NULL)
 		return 0;
 
-	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_UPDATE, c);
+	if (!batch_chain_add(h, NFT_COMPAT_CHAIN_UPDATE, c))
+		return 0;
 
 	/* the core expects 1 for success and 0 for error */
-	return ret == 0 ? 1 : 0;
+	return 1;
 }
 
 static int __add_match(struct nftnl_expr *e, struct xt_entry_match *m)
@@ -1725,7 +1726,6 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
 {
 	struct nftnl_chain_list *list;
 	struct nftnl_chain *c;
-	int ret;
 
 	nft_fn = nft_chain_user_add;
 
@@ -1745,14 +1745,15 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
 	if (h->family == NFPROTO_BRIDGE)
 		nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, NF_ACCEPT);
 
-	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
+	if (!batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c))
+		return 0;
 
 	list = nft_chain_list_get(h, table, chain);
 	if (list)
 		nftnl_chain_list_add(c, list);
 
 	/* the core expects 1 for success and 0 for error */
-	return ret == 0 ? 1 : 0;
+	return 1;
 }
 
 int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table)
@@ -1760,7 +1761,6 @@ 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;
 
 	nft_xt_builtin_init(h, table);
 
@@ -1787,14 +1787,15 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
 	if (!created)
 		return 1;
 
-	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
+	if (!batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c))
+		return 0;
 
 	list = nft_chain_list_get(h, table, chain);
 	if (list)
 		nftnl_chain_list_add(c, list);
 
 	/* the core expects 1 for success and 0 for error */
-	return ret == 0 ? 1 : 0;
+	return 1;
 }
 
 /* From linux/netlink.h */
@@ -1812,7 +1813,6 @@ static int __nft_chain_user_del(struct nftnl_chain *c, void *data)
 {
 	struct chain_user_del_data *d = data;
 	struct nft_handle *h = d->handle;
-	int ret;
 
 	/* don't delete built-in chain */
 	if (nft_chain_builtin(c))
@@ -1824,8 +1824,7 @@ static int __nft_chain_user_del(struct nftnl_chain *c, void *data)
 
 	/* XXX This triggers a fast lookup from the kernel. */
 	nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE);
-	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_DEL, c);
-	if (ret)
+	if (!batch_chain_add(h, NFT_COMPAT_CHAIN_USER_DEL, c))
 		return -1;
 
 	nftnl_chain_list_del(c);
@@ -1900,7 +1899,6 @@ int nft_chain_user_rename(struct nft_handle *h,const char *chain,
 {
 	struct nftnl_chain *c;
 	uint64_t handle;
-	int ret;
 
 	nft_fn = nft_chain_user_rename;
 
@@ -1929,10 +1927,11 @@ int nft_chain_user_rename(struct nft_handle *h,const char *chain,
 	nftnl_chain_set_str(c, NFTNL_CHAIN_NAME, newname);
 	nftnl_chain_set_u64(c, NFTNL_CHAIN_HANDLE, handle);
 
-	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_RENAME, c);
+	if (!batch_chain_add(h, NFT_COMPAT_CHAIN_RENAME, c))
+		return 0;
 
 	/* the core expects 1 for success and 0 for error */
-	return ret == 0 ? 1 : 0;
+	return 1;
 }
 
 bool nft_table_find(struct nft_handle *h, const char *tablename)
@@ -3296,7 +3295,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);
-		if (batch_chain_add(h, NFT_COMPAT_CHAIN_ZERO, c))
+		if (!batch_chain_add(h, NFT_COMPAT_CHAIN_ZERO, c))
 			return -1;
 	}
 
-- 
2.28.0


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

* [iptables PATCH 2/3] nft: Fix error reporting for refreshed transactions
  2020-10-05 14:48 [iptables PATCH 0/3] nft: Fix transaction refreshing Phil Sutter
  2020-10-05 14:48 ` [iptables PATCH 1/3] nft: Make batch_add_chain() return the added batch object Phil Sutter
@ 2020-10-05 14:48 ` Phil Sutter
  2020-10-05 21:13   ` Florian Westphal
  2020-10-05 14:48 ` [iptables PATCH 3/3] nft: Fix for concurrent noflush restore calls Phil Sutter
  2 siblings, 1 reply; 11+ messages in thread
From: Phil Sutter @ 2020-10-05 14:48 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal

When preparing a batch from the list of batch objects in nft_action(),
the sequence number used for each object is stored within that object
for later matching against returned error messages. Though if the
transaction has to be refreshed, some of those objects may be skipped,
other objects take over their sequence number and errors are matched to
skipped objects. Avoid this by resetting the skipped object's sequence
number to zero.

Fixes: 58d7de0181f61 ("xtables: handle concurrent ruleset modifications")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 09421cf4eaaec..70be9ba908edc 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2729,9 +2729,10 @@ retry:
 	h->nft_genid++;
 
 	list_for_each_entry(n, &h->obj_list, head) {
-
-		if (n->skip)
+		if (n->skip) {
+			n->seq = 0;
 			continue;
+		}
 
 		n->seq = seq++;
 		switch (n->type) {
-- 
2.28.0


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

* [iptables PATCH 3/3] nft: Fix for concurrent noflush restore calls
  2020-10-05 14:48 [iptables PATCH 0/3] nft: Fix transaction refreshing Phil Sutter
  2020-10-05 14:48 ` [iptables PATCH 1/3] nft: Make batch_add_chain() return the added batch object Phil Sutter
  2020-10-05 14:48 ` [iptables PATCH 2/3] nft: Fix error reporting for refreshed transactions Phil Sutter
@ 2020-10-05 14:48 ` Phil Sutter
  2020-10-12 12:54   ` Pablo Neira Ayuso
  2 siblings, 1 reply; 11+ messages in thread
From: Phil Sutter @ 2020-10-05 14:48 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal

Transaction refresh was broken with regards to nft_chain_restore(): It
created a rule flush batch object only if the chain was found in cache
and a chain add object only if the chain was not found. Yet with
concurrent ruleset updates, one has to expect both situations:

* If a chain vanishes, the rule flush job must be skipped and instead
  the chain add job become active.

* If a chain appears, the chain add job must be skipped and instead
  rules flushed.

Change the code accordingly: Create both batch objects and set their
'skip' field depending on the situation in cache and adjust both in
nft_refresh_transaction().

As a side-effect, the implicit rule flush becomes explicit and all
handling of implicit batch jobs is dropped along with the related field
indicating such.

Reuse the 'implicit' parameter of __nft_rule_flush() to control the
initial 'skip' field value instead.

A subtle caveat is vanishing of existing chains: Creating the chain add
job based on the chain in cache causes a netlink message containing that
chain's handle which the kernel dislikes. Therefore unset the chain's
handle in that case.

Fixes: 58d7de0181f61 ("xtables: handle concurrent ruleset modifications")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c                                | 58 ++++++++++---------
 .../ipt-restore/0016-concurrent-restores_0    | 53 +++++++++++++++++
 2 files changed, 83 insertions(+), 28 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0016-concurrent-restores_0

diff --git a/iptables/nft.c b/iptables/nft.c
index 70be9ba908edc..9424c181d17cc 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -265,7 +265,6 @@ 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;
@@ -1634,7 +1633,7 @@ struct nftnl_set *nft_set_batch_lookup_byid(struct nft_handle *h,
 
 static void
 __nft_rule_flush(struct nft_handle *h, const char *table,
-		 const char *chain, bool verbose, bool implicit)
+		 const char *chain, bool verbose, bool skip)
 {
 	struct obj_update *obj;
 	struct nftnl_rule *r;
@@ -1656,7 +1655,7 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
 		return;
 	}
 
-	obj->implicit = implicit;
+	obj->skip = skip;
 }
 
 struct nft_rule_flush_data {
@@ -1759,19 +1758,14 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
 int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table)
 {
 	struct nftnl_chain_list *list;
+	struct obj_update *obj;
 	struct nftnl_chain *c;
 	bool created = false;
 
 	nft_xt_builtin_init(h, table);
 
 	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, true);
-	} else {
+	if (!c) {
 		c = nftnl_chain_alloc();
 		if (!c)
 			return 0;
@@ -1779,20 +1773,26 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
 		nftnl_chain_set_str(c, NFTNL_CHAIN_TABLE, table);
 		nftnl_chain_set_str(c, NFTNL_CHAIN_NAME, chain);
 		created = true;
-	}
 
-	if (h->family == NFPROTO_BRIDGE)
-		nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, NF_ACCEPT);
+		list = nft_chain_list_get(h, table, chain);
+		if (list)
+			nftnl_chain_list_add(c, list);
+	} else {
+		/* If the chain should vanish meanwhile, kernel genid changes
+		 * and the transaction is refreshed enabling the chain add
+		 * object. With the handle still set, kernel interprets it as a
+		 * chain replace job and errors since it is not found anymore.
+		 */
+		nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE);
+	}
 
-	if (!created)
-		return 1;
+	__nft_rule_flush(h, table, chain, false, created);
 
-	if (!batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c))
+	obj = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
+	if (!obj)
 		return 0;
 
-	list = nft_chain_list_get(h, table, chain);
-	if (list)
-		nftnl_chain_list_add(c, list);
+	obj->skip = !created;
 
 	/* the core expects 1 for success and 0 for error */
 	return 1;
@@ -2649,11 +2649,6 @@ static void nft_refresh_transaction(struct nft_handle *h)
 	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);
@@ -2679,14 +2674,22 @@ static void nft_refresh_transaction(struct nft_handle *h)
 
 			c = nft_chain_find(h, tablename, chainname);
 			if (c) {
-				/* -restore -n flushes existing rules from redefined user-chain */
-				__nft_rule_flush(h, tablename,
-						 chainname, false, true);
 				n->skip = 1;
 			} else if (!c) {
 				n->skip = 0;
 			}
 			break;
+		case NFT_COMPAT_RULE_FLUSH:
+			tablename = nftnl_rule_get_str(n->rule, NFTNL_RULE_TABLE);
+			if (!tablename)
+				continue;
+
+			chainname = nftnl_rule_get_str(n->rule, NFTNL_RULE_CHAIN);
+			if (!chainname)
+				continue;
+
+			n->skip = !nft_chain_find(h, tablename, chainname);
+			break;
 		case NFT_COMPAT_TABLE_ADD:
 		case NFT_COMPAT_CHAIN_ADD:
 		case NFT_COMPAT_CHAIN_ZERO:
@@ -2698,7 +2701,6 @@ static void nft_refresh_transaction(struct nft_handle *h)
 		case NFT_COMPAT_RULE_INSERT:
 		case NFT_COMPAT_RULE_REPLACE:
 		case NFT_COMPAT_RULE_DELETE:
-		case NFT_COMPAT_RULE_FLUSH:
 		case NFT_COMPAT_SET_ADD:
 		case NFT_COMPAT_RULE_LIST:
 		case NFT_COMPAT_RULE_CHECK:
diff --git a/iptables/tests/shell/testcases/ipt-restore/0016-concurrent-restores_0 b/iptables/tests/shell/testcases/ipt-restore/0016-concurrent-restores_0
new file mode 100755
index 0000000000000..53ec12fa368af
--- /dev/null
+++ b/iptables/tests/shell/testcases/ipt-restore/0016-concurrent-restores_0
@@ -0,0 +1,53 @@
+#!/bin/bash
+
+set -e
+
+RS="*filter
+:INPUT ACCEPT [12024:3123388]
+:FORWARD ACCEPT [0:0]
+:OUTPUT ACCEPT [12840:2144421]
+:FOO - [0:0]
+:BAR0 - [0:0]
+:BAR1 - [0:0]
+:BAR2 - [0:0]
+:BAR3 - [0:0]
+:BAR4 - [0:0]
+:BAR5 - [0:0]
+:BAR6 - [0:0]
+:BAR7 - [0:0]
+:BAR8 - [0:0]
+:BAR9 - [0:0]
+"
+
+RS1="$RS
+-X BAR3
+-X BAR6
+-X BAR9
+-A FOO -s 9.9.0.1/32 -j BAR1
+-A FOO -s 9.9.0.2/32 -j BAR2
+-A FOO -s 9.9.0.4/32 -j BAR4
+-A FOO -s 9.9.0.5/32 -j BAR5
+-A FOO -s 9.9.0.7/32 -j BAR7
+-A FOO -s 9.9.0.8/32 -j BAR8
+COMMIT
+"
+
+RS2="$RS
+-X BAR2
+-X BAR5
+-X BAR7
+-A FOO -s 9.9.0.1/32 -j BAR1
+-A FOO -s 9.9.0.3/32 -j BAR3
+-A FOO -s 9.9.0.4/32 -j BAR4
+-A FOO -s 9.9.0.6/32 -j BAR6
+-A FOO -s 9.9.0.8/32 -j BAR8
+-A FOO -s 9.9.0.9/32 -j BAR9
+COMMIT
+"
+
+for n in $(seq 1 10); do
+	$XT_MULTI iptables-restore --noflush -w <<< "$RS1" &
+	$XT_MULTI iptables-restore --noflush -w <<< "$RS2" &
+	wait -n
+	wait -n
+done
-- 
2.28.0


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

* Re: [iptables PATCH 1/3] nft: Make batch_add_chain() return the added batch object
  2020-10-05 14:48 ` [iptables PATCH 1/3] nft: Make batch_add_chain() return the added batch object Phil Sutter
@ 2020-10-05 21:07   ` Florian Westphal
  0 siblings, 0 replies; 11+ messages in thread
From: Florian Westphal @ 2020-10-05 21:07 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel, Florian Westphal

Phil Sutter <phil@nwl.cc> wrote:
> Do this so in a later patch the 'skip' field can be adjusted.
> 
> While being at it, simplify a few callers and eliminate the need for a
> 'ret' variable.

Reviewed-by: Florian Westphal <fw@strlen.de>

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

* Re: [iptables PATCH 2/3] nft: Fix error reporting for refreshed transactions
  2020-10-05 14:48 ` [iptables PATCH 2/3] nft: Fix error reporting for refreshed transactions Phil Sutter
@ 2020-10-05 21:13   ` Florian Westphal
  0 siblings, 0 replies; 11+ messages in thread
From: Florian Westphal @ 2020-10-05 21:13 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel, Florian Westphal

Phil Sutter <phil@nwl.cc> wrote:
> When preparing a batch from the list of batch objects in nft_action(),
> the sequence number used for each object is stored within that object
> for later matching against returned error messages. Though if the
> transaction has to be refreshed, some of those objects may be skipped,
> other objects take over their sequence number and errors are matched to
> skipped objects. Avoid this by resetting the skipped object's sequence
> number to zero.

Reviewed-by: Florian Westphal <fw@strlen.de>

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

* Re: [iptables PATCH 3/3] nft: Fix for concurrent noflush restore calls
  2020-10-05 14:48 ` [iptables PATCH 3/3] nft: Fix for concurrent noflush restore calls Phil Sutter
@ 2020-10-12 12:54   ` Pablo Neira Ayuso
  2020-10-13 10:08     ` Phil Sutter
  0 siblings, 1 reply; 11+ messages in thread
From: Pablo Neira Ayuso @ 2020-10-12 12:54 UTC (permalink / raw)
  To: Phil Sutter; +Cc: netfilter-devel, Florian Westphal

On Mon, Oct 05, 2020 at 04:48:58PM +0200, Phil Sutter wrote:
> Transaction refresh was broken with regards to nft_chain_restore(): It
> created a rule flush batch object only if the chain was found in cache
> and a chain add object only if the chain was not found. Yet with
> concurrent ruleset updates, one has to expect both situations:
> 
> * If a chain vanishes, the rule flush job must be skipped and instead
>   the chain add job become active.
> 
> * If a chain appears, the chain add job must be skipped and instead
>   rules flushed.
> 
> Change the code accordingly: Create both batch objects and set their
> 'skip' field depending on the situation in cache and adjust both in
> nft_refresh_transaction().
> 
> As a side-effect, the implicit rule flush becomes explicit and all
> handling of implicit batch jobs is dropped along with the related field
> indicating such.
> 
> Reuse the 'implicit' parameter of __nft_rule_flush() to control the
> initial 'skip' field value instead.
> 
> A subtle caveat is vanishing of existing chains: Creating the chain add
> job based on the chain in cache causes a netlink message containing that
> chain's handle which the kernel dislikes. Therefore unset the chain's
> handle in that case.
> 
> Fixes: 58d7de0181f61 ("xtables: handle concurrent ruleset modifications")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  iptables/nft.c                                | 58 ++++++++++---------
>  .../ipt-restore/0016-concurrent-restores_0    | 53 +++++++++++++++++
>  2 files changed, 83 insertions(+), 28 deletions(-)
>  create mode 100755 iptables/tests/shell/testcases/ipt-restore/0016-concurrent-restores_0
> 
> diff --git a/iptables/nft.c b/iptables/nft.c
> index 70be9ba908edc..9424c181d17cc 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -265,7 +265,6 @@ 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;
> @@ -1634,7 +1633,7 @@ struct nftnl_set *nft_set_batch_lookup_byid(struct nft_handle *h,
>  
>  static void
>  __nft_rule_flush(struct nft_handle *h, const char *table,
> -		 const char *chain, bool verbose, bool implicit)
> +		 const char *chain, bool verbose, bool skip)
>  {
>  	struct obj_update *obj;
>  	struct nftnl_rule *r;
> @@ -1656,7 +1655,7 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
>  		return;
>  	}
>  
> -	obj->implicit = implicit;
> +	obj->skip = skip;
>  }
>  
>  struct nft_rule_flush_data {
> @@ -1759,19 +1758,14 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
>  int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table)
>  {
>  	struct nftnl_chain_list *list;
> +	struct obj_update *obj;
>  	struct nftnl_chain *c;
>  	bool created = false;
>  
>  	nft_xt_builtin_init(h, table);
>  
>  	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, true);
> -	} else {
> +	if (!c) {
>  		c = nftnl_chain_alloc();
>  		if (!c)
>  			return 0;
> @@ -1779,20 +1773,26 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
>  		nftnl_chain_set_str(c, NFTNL_CHAIN_TABLE, table);
>  		nftnl_chain_set_str(c, NFTNL_CHAIN_NAME, chain);
>  		created = true;
> -	}
>  
> -	if (h->family == NFPROTO_BRIDGE)
> -		nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, NF_ACCEPT);
> +		list = nft_chain_list_get(h, table, chain);
> +		if (list)
> +			nftnl_chain_list_add(c, list);
> +	} else {
> +		/* If the chain should vanish meanwhile, kernel genid changes
> +		 * and the transaction is refreshed enabling the chain add
> +		 * object. With the handle still set, kernel interprets it as a
> +		 * chain replace job and errors since it is not found anymore.
> +		 */
> +		nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE);
> +	}
>  
> -	if (!created)
> -		return 1;
> +	__nft_rule_flush(h, table, chain, false, created);

I like this trick.

If I understood correct, you always place an "add chain" command in
first place before the flush, whether the chain exists or not, so the
flush always succeeds.

> -	if (!batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c))
> +	obj = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
> +	if (!obj)
>  		return 0;
>  
> -	list = nft_chain_list_get(h, table, chain);
> -	if (list)
> -		nftnl_chain_list_add(c, list);
> +	obj->skip = !created;
>  
>  	/* the core expects 1 for success and 0 for error */
>  	return 1;
> @@ -2649,11 +2649,6 @@ static void nft_refresh_transaction(struct nft_handle *h)
>  	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);
> @@ -2679,14 +2674,22 @@ static void nft_refresh_transaction(struct nft_handle *h)
>  
>  			c = nft_chain_find(h, tablename, chainname);
>  			if (c) {
> -				/* -restore -n flushes existing rules from redefined user-chain */
> -				__nft_rule_flush(h, tablename,
> -						 chainname, false, true);
>  				n->skip = 1;
>  			} else if (!c) {
>  				n->skip = 0;
>  			}
>  			break;
> +		case NFT_COMPAT_RULE_FLUSH:
> +			tablename = nftnl_rule_get_str(n->rule, NFTNL_RULE_TABLE);
> +			if (!tablename)
> +				continue;
> +
> +			chainname = nftnl_rule_get_str(n->rule, NFTNL_RULE_CHAIN);
> +			if (!chainname)
> +				continue;
> +
> +			n->skip = !nft_chain_find(h, tablename, chainname);

So n->skip equals true if the chain does not exists in the cache, so
this flush does not fail (after this chain is gone because someone
else have remove it).

Patch LGTM, thanks Phil.

What I don't clearly see yet is what scenario is triggering the bug in
the existing code, if you don't mind to explain.

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

* Re: [iptables PATCH 3/3] nft: Fix for concurrent noflush restore calls
  2020-10-12 12:54   ` Pablo Neira Ayuso
@ 2020-10-13 10:08     ` Phil Sutter
  2020-10-13 10:15       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 11+ messages in thread
From: Phil Sutter @ 2020-10-13 10:08 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal

On Mon, Oct 12, 2020 at 02:54:50PM +0200, Pablo Neira Ayuso wrote:
[...]
> > -	if (h->family == NFPROTO_BRIDGE)
> > -		nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, NF_ACCEPT);
> > +		list = nft_chain_list_get(h, table, chain);
> > +		if (list)
> > +			nftnl_chain_list_add(c, list);
> > +	} else {
> > +		/* If the chain should vanish meanwhile, kernel genid changes
> > +		 * and the transaction is refreshed enabling the chain add
> > +		 * object. With the handle still set, kernel interprets it as a
> > +		 * chain replace job and errors since it is not found anymore.
> > +		 */
> > +		nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE);
> > +	}
> >  
> > -	if (!created)
> > -		return 1;
> > +	__nft_rule_flush(h, table, chain, false, created);
> 
> I like this trick.
> 
> If I understood correct, you always place an "add chain" command in
> first place before the flush, whether the chain exists or not, so the
> flush always succeeds.

Yes, this was already done for "add table". So I'm "merely" extending
Florian's refresh transaction logic with regards to 'skip' flag.

> > -	if (!batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c))
> > +	obj = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
> > +	if (!obj)
> >  		return 0;
> >  
> > -	list = nft_chain_list_get(h, table, chain);
> > -	if (list)
> > -		nftnl_chain_list_add(c, list);
> > +	obj->skip = !created;
> >  
> >  	/* the core expects 1 for success and 0 for error */
> >  	return 1;
> > @@ -2649,11 +2649,6 @@ static void nft_refresh_transaction(struct nft_handle *h)
> >  	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);
> > @@ -2679,14 +2674,22 @@ static void nft_refresh_transaction(struct nft_handle *h)
> >  
> >  			c = nft_chain_find(h, tablename, chainname);
> >  			if (c) {
> > -				/* -restore -n flushes existing rules from redefined user-chain */
> > -				__nft_rule_flush(h, tablename,
> > -						 chainname, false, true);
> >  				n->skip = 1;
> >  			} else if (!c) {
> >  				n->skip = 0;
> >  			}
> >  			break;
> > +		case NFT_COMPAT_RULE_FLUSH:
> > +			tablename = nftnl_rule_get_str(n->rule, NFTNL_RULE_TABLE);
> > +			if (!tablename)
> > +				continue;
> > +
> > +			chainname = nftnl_rule_get_str(n->rule, NFTNL_RULE_CHAIN);
> > +			if (!chainname)
> > +				continue;
> > +
> > +			n->skip = !nft_chain_find(h, tablename, chainname);
> 
> So n->skip equals true if the chain does not exists in the cache, so
> this flush does not fail (after this chain is gone because someone
> else have remove it).

Yes, the tricky bit is being able to adjust the batch object list no
matter how kernel ruleset changes. The only way is to assume the most
overhead and temporary disable individual jobs if not needed. This way
we can enable/disable while refreshing the transaction.

> Patch LGTM, thanks Phil.
> 
> What I don't clearly see yet is what scenario is triggering the bug in
> the existing code, if you don't mind to explain.

See the test case attached to the patch: An other iptables-restore
process may add references (i.e., jumps) to a chain the own
iptables-restore process wants to delete. This should not be a problem
because these references are added to a chain that is being flushed by
the own process as well. But if that chain doesn't exist while the own
process fetches kernel's ruleset, this flush job is not created.

Cheers, Phil

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

* Re: [iptables PATCH 3/3] nft: Fix for concurrent noflush restore calls
  2020-10-13 10:08     ` Phil Sutter
@ 2020-10-13 10:15       ` Pablo Neira Ayuso
  2020-10-14  9:46         ` Phil Sutter
  0 siblings, 1 reply; 11+ messages in thread
From: Pablo Neira Ayuso @ 2020-10-13 10:15 UTC (permalink / raw)
  To: Phil Sutter, netfilter-devel, Florian Westphal

On Tue, Oct 13, 2020 at 12:08:03PM +0200, Phil Sutter wrote:
[...]
> On Mon, Oct 12, 2020 at 02:54:50PM +0200, Pablo Neira Ayuso wrote:
> [...]
> > Patch LGTM, thanks Phil.
> > 
> > What I don't clearly see yet is what scenario is triggering the bug in
> > the existing code, if you don't mind to explain.
> 
> See the test case attached to the patch: An other iptables-restore
> process may add references (i.e., jumps) to a chain the own
> iptables-restore process wants to delete. This should not be a problem
> because these references are added to a chain that is being flushed by
> the own process as well. But if that chain doesn't exist while the own
> process fetches kernel's ruleset, this flush job is not created.

Let me rephrase this:

1) process A fetches the ruleset, finds no chain C (no flush job then)
2) process B adds new chain C, flush job is present
3) process B adds the ruleset
4) process A appends rules to the existing chain C (because there is
   no flush job)

Is this the scenario? If so, I wonder why the generation ID is not
helping to refresh and retry.

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

* Re: [iptables PATCH 3/3] nft: Fix for concurrent noflush restore calls
  2020-10-13 10:15       ` Pablo Neira Ayuso
@ 2020-10-14  9:46         ` Phil Sutter
  2020-10-16 15:28           ` Pablo Neira Ayuso
  0 siblings, 1 reply; 11+ messages in thread
From: Phil Sutter @ 2020-10-14  9:46 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal

On Tue, Oct 13, 2020 at 12:15:02PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 13, 2020 at 12:08:03PM +0200, Phil Sutter wrote:
> [...]
> > On Mon, Oct 12, 2020 at 02:54:50PM +0200, Pablo Neira Ayuso wrote:
> > [...]
> > > Patch LGTM, thanks Phil.
> > > 
> > > What I don't clearly see yet is what scenario is triggering the bug in
> > > the existing code, if you don't mind to explain.
> > 
> > See the test case attached to the patch: An other iptables-restore
> > process may add references (i.e., jumps) to a chain the own
> > iptables-restore process wants to delete. This should not be a problem
> > because these references are added to a chain that is being flushed by
> > the own process as well. But if that chain doesn't exist while the own
> > process fetches kernel's ruleset, this flush job is not created.
> 
> Let me rephrase this:
> 
> 1) process A fetches the ruleset, finds no chain C (no flush job then)
> 2) process B adds new chain C, flush job is present
> 3) process B adds the ruleset
> 4) process A appends rules to the existing chain C (because there is
>    no flush job)
> 
> Is this the scenario? If so, I wonder why the generation ID is not
> helping to refresh and retry.

Not quite, let me try to put this more clearly:

* Dump A:
  | *filter
  | :FOO - [0:0] # flush chain FOO
  | -X BAR       # remove chain BAR
  | COMMIT

* Dump B:
  | *filter
  | -A FOO -j BAR # reference BAR from a rule in FOO
  | COMMIT

* Kernel ruleset:
  | *filter
  | :BAR - [0:0]
  | COMMIT

* Process A:
  * read dump A
  * fetch cache

* Process B:
  * read dump B
  * fetch ruleset
  * commit to kernel

* Process A:
  * skip flush chain FOO job: not present
  * add delete chain BAR job: chain exists
  * commit fails (genid outdated)
  * refresh transaction:
    * delete chain BAR job remains active
    * genid updated
  * commit fails: can't remove chain BAR: EBUSY

I realize the test case is not quite effective, ruleset should be
emptied upon each iteration of concurrent restore job startup.

Cheers, Phil

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

* Re: [iptables PATCH 3/3] nft: Fix for concurrent noflush restore calls
  2020-10-14  9:46         ` Phil Sutter
@ 2020-10-16 15:28           ` Pablo Neira Ayuso
  0 siblings, 0 replies; 11+ messages in thread
From: Pablo Neira Ayuso @ 2020-10-16 15:28 UTC (permalink / raw)
  To: Phil Sutter, netfilter-devel, Florian Westphal

On Wed, Oct 14, 2020 at 11:46:40AM +0200, Phil Sutter wrote:
> On Tue, Oct 13, 2020 at 12:15:02PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Oct 13, 2020 at 12:08:03PM +0200, Phil Sutter wrote:
> > [...]
> > > On Mon, Oct 12, 2020 at 02:54:50PM +0200, Pablo Neira Ayuso wrote:
> > > [...]
> > > > Patch LGTM, thanks Phil.
> > > > 
> > > > What I don't clearly see yet is what scenario is triggering the bug in
> > > > the existing code, if you don't mind to explain.
> > > 
> > > See the test case attached to the patch: An other iptables-restore
> > > process may add references (i.e., jumps) to a chain the own
> > > iptables-restore process wants to delete. This should not be a problem
> > > because these references are added to a chain that is being flushed by
> > > the own process as well. But if that chain doesn't exist while the own
> > > process fetches kernel's ruleset, this flush job is not created.
> > 
> > Let me rephrase this:
> > 
> > 1) process A fetches the ruleset, finds no chain C (no flush job then)
> > 2) process B adds new chain C, flush job is present
> > 3) process B adds the ruleset
> > 4) process A appends rules to the existing chain C (because there is
> >    no flush job)
> > 
> > Is this the scenario? If so, I wonder why the generation ID is not
> > helping to refresh and retry.
> 
> Not quite, let me try to put this more clearly:
> 
> * Dump A:
>   | *filter
>   | :FOO - [0:0] # flush chain FOO
>   | -X BAR       # remove chain BAR
>   | COMMIT
> 
> * Dump B:
>   | *filter
>   | -A FOO -j BAR # reference BAR from a rule in FOO
>   | COMMIT
> 
> * Kernel ruleset:
>   | *filter
>   | :BAR - [0:0]
>   | COMMIT
> 
> * Process A:
>   * read dump A
>   * fetch cache
> 
> * Process B:
>   * read dump B
>   * fetch ruleset
>   * commit to kernel
> 
> * Process A:
>   * skip flush chain FOO job: not present
>   * add delete chain BAR job: chain exists
>   * commit fails (genid outdated)
>   * refresh transaction:
>     * delete chain BAR job remains active
>     * genid updated
>   * commit fails: can't remove chain BAR: EBUSY

Makes sense. Thanks a lot for explaining. Probably you can include
this in the commit description for the record.

> I realize the test case is not quite effective, ruleset should be
> emptied upon each iteration of concurrent restore job startup.

Please, update the test and revamp.

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

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 14:48 [iptables PATCH 0/3] nft: Fix transaction refreshing Phil Sutter
2020-10-05 14:48 ` [iptables PATCH 1/3] nft: Make batch_add_chain() return the added batch object Phil Sutter
2020-10-05 21:07   ` Florian Westphal
2020-10-05 14:48 ` [iptables PATCH 2/3] nft: Fix error reporting for refreshed transactions Phil Sutter
2020-10-05 21:13   ` Florian Westphal
2020-10-05 14:48 ` [iptables PATCH 3/3] nft: Fix for concurrent noflush restore calls Phil Sutter
2020-10-12 12:54   ` Pablo Neira Ayuso
2020-10-13 10:08     ` Phil Sutter
2020-10-13 10:15       ` Pablo Neira Ayuso
2020-10-14  9:46         ` Phil Sutter
2020-10-16 15:28           ` Pablo Neira Ayuso

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git