netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [iptables PATCH 0/4] nft: Fix and improve base chain handling
@ 2021-09-22 16:06 Phil Sutter
  2021-09-22 16:06 ` [iptables PATCH 1/4] nft: cache: Avoid double free of unrecognized base-chains Phil Sutter
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Phil Sutter @ 2021-09-22 16:06 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

This is a combined series of fixes and improvements:

* Patch 1 fixes a double free happening if the ruleset contains more
 than one base-chains for a given hook.

* Patch 2 improves iptables-nft behaviour in above case, allowing to
  continue even if there is a base chain which doesn't fit. Since
  iptables-nft doesn't fetch the full ruleset from kernel in all cases
  anymore, it is prone to miss offending ruleset parts, anyway.

* Patch 4 tries to avoid the negative side-effects that came with
  Florian's patch allowing to delete base-chains. 

* Patch 3 adds a bit of convenience used by patch 4.

Phil Sutter (4):
  nft: cache: Avoid double free of unrecognized base-chains
  nft: Check base-chain compatibility when adding to cache
  nft-chain: Introduce base_slot field
  nft: Delete builtin chains compatibly

 iptables/nft-cache.c                          |  52 +++++---
 iptables/nft-chain.h                          |   1 +
 iptables/nft-cmd.c                            |   2 +-
 iptables/nft.c                                | 112 +++++++-----------
 iptables/nft.h                                |   2 +
 .../shell/testcases/chain/0004extra-base_0    |  37 ++++++
 .../shell/testcases/chain/0005base-delete_0   |  34 ++++++
 iptables/xtables-save.c                       |   3 +
 8 files changed, 161 insertions(+), 82 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/chain/0004extra-base_0
 create mode 100755 iptables/tests/shell/testcases/chain/0005base-delete_0

-- 
2.33.0


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

* [iptables PATCH 1/4] nft: cache: Avoid double free of unrecognized base-chains
  2021-09-22 16:06 [iptables PATCH 0/4] nft: Fix and improve base chain handling Phil Sutter
@ 2021-09-22 16:06 ` Phil Sutter
  2021-09-22 16:06 ` [iptables PATCH 2/4] nft: Check base-chain compatibility when adding to cache Phil Sutter
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Phil Sutter @ 2021-09-22 16:06 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

On error, nft_cache_add_chain() frees the allocated nft_chain object
along with the nftnl_chain it points at. Fix nftnl_chain_list_cb() to
not free the nftnl_chain again in that case.

Fixes: 176c92c26bfc9 ("nft: Introduce a dedicated base chain array")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c                          |  4 +--
 .../shell/testcases/chain/0004extra-base_0    | 27 +++++++++++++++++++
 2 files changed, 28 insertions(+), 3 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/chain/0004extra-base_0

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 2c88301cc7445..9a03bbfbb32bb 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -314,9 +314,7 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 		goto out;
 	}
 
-	if (nft_cache_add_chain(h, t, c))
-		goto out;
-
+	nft_cache_add_chain(h, t, c);
 	return MNL_CB_OK;
 out:
 	nftnl_chain_free(c);
diff --git a/iptables/tests/shell/testcases/chain/0004extra-base_0 b/iptables/tests/shell/testcases/chain/0004extra-base_0
new file mode 100755
index 0000000000000..1b85b060c1487
--- /dev/null
+++ b/iptables/tests/shell/testcases/chain/0004extra-base_0
@@ -0,0 +1,27 @@
+#!/bin/bash
+
+case $XT_MULTI in
+*xtables-nft-multi)
+	;;
+*)
+	echo skip $XT_MULTI
+	exit 0
+	;;
+esac
+
+set -e
+
+nft -f - <<EOF
+table ip filter {
+        chain INPUT {
+                type filter hook input priority filter
+                counter packets 218 bytes 91375 accept
+        }
+
+        chain x {
+                type filter hook input priority filter
+        }
+}
+EOF
+
+$XT_MULTI iptables -L
-- 
2.33.0


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

* [iptables PATCH 2/4] nft: Check base-chain compatibility when adding to cache
  2021-09-22 16:06 [iptables PATCH 0/4] nft: Fix and improve base chain handling Phil Sutter
  2021-09-22 16:06 ` [iptables PATCH 1/4] nft: cache: Avoid double free of unrecognized base-chains Phil Sutter
@ 2021-09-22 16:06 ` Phil Sutter
  2021-09-22 16:06 ` [iptables PATCH 3/4] nft-chain: Introduce base_slot field Phil Sutter
  2021-09-22 16:06 ` [iptables PATCH 4/4] nft: Delete builtin chains compatibly Phil Sutter
  3 siblings, 0 replies; 6+ messages in thread
From: Phil Sutter @ 2021-09-22 16:06 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

With introduction of dedicated base-chain slots, a selection process was
established as no longer all base-chains ended in the same chain list
for later searching/checking but only the first one found for each hook
matching criteria is kept and the rest discarded.

A side-effect of the above is that table compatibility checking started
to omit consecutive base-chains, making iptables-nft less restrictive as
long as the expected base-chains were returned first from kernel when
populating the cache.

Make behaviour consistent and warn users about the possibly disturbing
chains found by:

* Run all base-chain checks from nft_is_chain_compatible() before
  allowing a base-chain to occupy its slot.
* If an unfit base-chain was found (and discarded), flag the table's
  cache as tainted and warn about it if the remaining ruleset is
  otherwise compatible.

Since base-chains that remain in cache would pass
nft_is_chain_compatible() checking, remove that and reduce it to rule
inspection.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c                          | 47 ++++++++++++++-----
 iptables/nft.c                                | 45 +++++-------------
 iptables/nft.h                                |  2 +
 .../shell/testcases/chain/0004extra-base_0    | 12 ++++-
 iptables/xtables-save.c                       |  3 ++
 5 files changed, 65 insertions(+), 44 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 9a03bbfbb32bb..b7f10ab923bc0 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -202,26 +202,51 @@ nft_chain_find(struct nft_handle *h, const char *table, const char *chain)
 	return NULL;
 }
 
+static int
+nft_cache_add_base_chain(struct nft_handle *h, const struct builtin_table *t,
+			 struct nft_chain *nc)
+{
+	uint32_t hooknum = nftnl_chain_get_u32(nc->nftnl, NFTNL_CHAIN_HOOKNUM);
+	const char *name = nftnl_chain_get_str(nc->nftnl, NFTNL_CHAIN_NAME);
+	const char *type = nftnl_chain_get_str(nc->nftnl, NFTNL_CHAIN_TYPE);
+	uint32_t prio = nftnl_chain_get_u32(nc->nftnl, NFTNL_CHAIN_PRIO);
+	const struct builtin_chain *bc = NULL;
+	int i;
+
+	for (i = 0; i < NF_IP_NUMHOOKS && t->chains[i].name != NULL; i++) {
+		if (hooknum == t->chains[i].hook) {
+			bc = &t->chains[i];
+			break;
+		}
+	}
+
+	if (!bc ||
+	    prio != bc->prio ||
+	    strcmp(name, bc->name) ||
+	    strcmp(type, bc->type))
+		return -EINVAL;
+
+	if (h->cache->table[t->type].base_chains[hooknum])
+		return -EEXIST;
+
+	h->cache->table[t->type].base_chains[hooknum] = nc;
+	return 0;
+}
+
 int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
 			struct nftnl_chain *c)
 {
 	const char *cname = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
 	struct nft_chain *nc = nft_chain_alloc(c);
+	int ret;
 
 	if (nftnl_chain_is_set(c, NFTNL_CHAIN_HOOKNUM)) {
-		uint32_t hooknum = nftnl_chain_get_u32(c, NFTNL_CHAIN_HOOKNUM);
-
-		if (hooknum >= NF_INET_NUMHOOKS) {
+		ret = nft_cache_add_base_chain(h, t, nc);
+		if (ret) {
+			h->cache->table[t->type].tainted = true;
 			nft_chain_free(nc);
-			return -EINVAL;
+			return ret;
 		}
-
-		if (h->cache->table[t->type].base_chains[hooknum]) {
-			nft_chain_free(nc);
-			return -EEXIST;
-		}
-
-		h->cache->table[t->type].base_chains[hooknum] = nc;
 	} else {
 		list_add_tail(&nc->head,
 			      &h->cache->table[t->type].chains->list);
diff --git a/iptables/nft.c b/iptables/nft.c
index 89dde9ecca779..17e735aa694af 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -3513,38 +3513,8 @@ static int nft_is_rule_compatible(struct nftnl_rule *rule, void *data)
 static int nft_is_chain_compatible(struct nft_chain *nc, void *data)
 {
 	struct nftnl_chain *c = nc->nftnl;
-	const struct builtin_table *table;
-	const struct builtin_chain *chain;
-	const char *tname, *cname, *type;
-	struct nft_handle *h = data;
-	enum nf_inet_hooks hook;
-	int prio;
-
-	if (nftnl_rule_foreach(c, nft_is_rule_compatible, NULL))
-		return -1;
-
-	if (!nft_chain_builtin(c))
-		return 0;
-
-	tname = nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);
-	table = nft_table_builtin_find(h, tname);
-	if (!table)
-		return -1;
-
-	cname = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
-	chain = nft_chain_builtin_find(table, cname);
-	if (!chain)
-		return -1;
 
-	type = nftnl_chain_get_str(c, NFTNL_CHAIN_TYPE);
-	prio = nftnl_chain_get_u32(c, NFTNL_CHAIN_PRIO);
-	hook = nftnl_chain_get_u32(c, NFTNL_CHAIN_HOOKNUM);
-	if (strcmp(type, chain->type) ||
-	    prio != chain->prio ||
-	    hook != chain->hook)
-		return -1;
-
-	return 0;
+	return nftnl_rule_foreach(c, nft_is_rule_compatible, NULL);
 }
 
 bool nft_is_table_compatible(struct nft_handle *h,
@@ -3559,13 +3529,24 @@ bool nft_is_table_compatible(struct nft_handle *h,
 	return !nft_chain_foreach(h, table, nft_is_chain_compatible, h);
 }
 
+bool nft_is_table_tainted(struct nft_handle *h, const char *table)
+{
+	const struct builtin_table *t = nft_table_builtin_find(h, table);
+
+	return t ? h->cache->table[t->type].tainted : false;
+}
+
 void nft_assert_table_compatible(struct nft_handle *h,
 				 const char *table, const char *chain)
 {
 	const char *pfx = "", *sfx = "";
 
-	if (nft_is_table_compatible(h, table, chain))
+	if (nft_is_table_compatible(h, table, chain)) {
+		if (nft_is_table_tainted(h, table))
+			printf("# Table `%s' contains incompatible base-chains, use 'nft' tool to list them.\n",
+			       table);
 		return;
+	}
 
 	if (chain) {
 		pfx = "chain `";
diff --git a/iptables/nft.h b/iptables/nft.h
index a7b652ff62a45..ef79b018f7836 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -45,6 +45,7 @@ struct nft_cache {
 		struct nftnl_set_list	*sets;
 		bool			exists;
 		bool			sorted;
+		bool			tainted;
 	} table[NFT_TABLE_MAX];
 };
 
@@ -262,6 +263,7 @@ void nft_rule_to_arpt_entry(struct nftnl_rule *r, struct arpt_entry *fw);
 
 bool nft_is_table_compatible(struct nft_handle *h,
 			     const char *table, const char *chain);
+bool nft_is_table_tainted(struct nft_handle *h, const char *table);
 void nft_assert_table_compatible(struct nft_handle *h,
 				 const char *table, const char *chain);
 
diff --git a/iptables/tests/shell/testcases/chain/0004extra-base_0 b/iptables/tests/shell/testcases/chain/0004extra-base_0
index 1b85b060c1487..cc07e4be31177 100755
--- a/iptables/tests/shell/testcases/chain/0004extra-base_0
+++ b/iptables/tests/shell/testcases/chain/0004extra-base_0
@@ -13,6 +13,10 @@ set -e
 
 nft -f - <<EOF
 table ip filter {
+	chain a {
+		type filter hook input priority filter
+	}
+
         chain INPUT {
                 type filter hook input priority filter
                 counter packets 218 bytes 91375 accept
@@ -24,4 +28,10 @@ table ip filter {
 }
 EOF
 
-$XT_MULTI iptables -L
+EXPECT="# Table \`filter' contains incompatible base-chains, use 'nft' tool to list them.
+-P INPUT ACCEPT
+-P FORWARD ACCEPT
+-P OUTPUT ACCEPT
+-A INPUT -j ACCEPT"
+
+diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI iptables -S)
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 98cd0ed3f4716..f794e3ff1e318 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -78,6 +78,9 @@ __do_output(struct nft_handle *h, const char *tablename, void *data)
 		printf("# Table `%s' is incompatible, use 'nft' tool.\n",
 		       tablename);
 		return 0;
+	} else if (nft_is_table_tainted(h, tablename)) {
+		printf("# Table `%s' contains incompatible base-chains, use 'nft' tool to list them.\n",
+		       tablename);
 	}
 
 	now = time(NULL);
-- 
2.33.0


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

* [iptables PATCH 3/4] nft-chain: Introduce base_slot field
  2021-09-22 16:06 [iptables PATCH 0/4] nft: Fix and improve base chain handling Phil Sutter
  2021-09-22 16:06 ` [iptables PATCH 1/4] nft: cache: Avoid double free of unrecognized base-chains Phil Sutter
  2021-09-22 16:06 ` [iptables PATCH 2/4] nft: Check base-chain compatibility when adding to cache Phil Sutter
@ 2021-09-22 16:06 ` Phil Sutter
  2021-09-22 16:06 ` [iptables PATCH 4/4] nft: Delete builtin chains compatibly Phil Sutter
  3 siblings, 0 replies; 6+ messages in thread
From: Phil Sutter @ 2021-09-22 16:06 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

For builtin chains, record the base_chains array slot they are assigned
to. This simplifies removing that reference if they are being deleted
later.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c |  5 +++--
 iptables/nft-chain.h |  1 +
 iptables/nft.c       | 28 +---------------------------
 3 files changed, 5 insertions(+), 29 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index b7f10ab923bc0..43ac291ec84b2 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -226,10 +226,11 @@ nft_cache_add_base_chain(struct nft_handle *h, const struct builtin_table *t,
 	    strcmp(type, bc->type))
 		return -EINVAL;
 
-	if (h->cache->table[t->type].base_chains[hooknum])
+	nc->base_slot = &h->cache->table[t->type].base_chains[hooknum];
+	if (*nc->base_slot)
 		return -EEXIST;
 
-	h->cache->table[t->type].base_chains[hooknum] = nc;
+	*nc->base_slot = nc;
 	return 0;
 }
 
diff --git a/iptables/nft-chain.h b/iptables/nft-chain.h
index 137f4b7f90085..9adf173857420 100644
--- a/iptables/nft-chain.h
+++ b/iptables/nft-chain.h
@@ -9,6 +9,7 @@ struct nft_handle;
 struct nft_chain {
 	struct list_head	head;
 	struct hlist_node	hnode;
+	struct nft_chain	**base_slot;
 	struct nftnl_chain	*nftnl;
 };
 
diff --git a/iptables/nft.c b/iptables/nft.c
index 17e735aa694af..381061473047f 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1838,8 +1838,6 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
 
 struct chain_del_data {
 	struct nft_handle	*handle;
-	struct nft_cache	*cache;
-	enum nft_table_type	type;
 	bool			verbose;
 };
 
@@ -1860,10 +1858,7 @@ static int __nft_chain_del(struct nft_chain *nc, void *data)
 		return -1;
 
 	if (nft_chain_builtin(c)) {
-		uint32_t num = nftnl_chain_get_u32(c, NFTNL_CHAIN_HOOKNUM);
-
-		if (nc == d->cache->table[d->type].base_chains[num])
-			d->cache->table[d->type].base_chains[num] = NULL;
+		*nc->base_slot = NULL;
 	}
 
 	/* nftnl_chain is freed when deleting the batch object */
@@ -1877,7 +1872,6 @@ static int __nft_chain_del(struct nft_chain *nc, void *data)
 int nft_chain_del(struct nft_handle *h, const char *chain,
 		       const char *table, bool verbose)
 {
-	const struct builtin_table *t;
 	struct chain_del_data d = {
 		.handle = h,
 		.verbose = verbose,
@@ -1894,32 +1888,12 @@ int nft_chain_del(struct nft_handle *h, const char *chain,
 			return 0;
 		}
 
-		if (nft_chain_builtin(c->nftnl)) {
-			t = nft_table_builtin_find(h, table);
-			if (!t) {
-				errno = EINVAL;
-				return 0;
-			}
-
-			d.type = t->type;
-			d.cache = h->cache;
-		}
-
 		ret = __nft_chain_del(c, &d);
 		if (ret == -2)
 			errno = EINVAL;
 		goto out;
 	}
 
-	t = nft_table_builtin_find(h, table);
-	if (!t) {
-		errno = EINVAL;
-		return 0;
-	}
-
-	d.type = t->type;
-	d.cache = h->cache;
-
 	if (verbose)
 		nft_cache_sort_chains(h, table);
 
-- 
2.33.0


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

* [iptables PATCH 4/4] nft: Delete builtin chains compatibly
  2021-09-22 16:06 [iptables PATCH 0/4] nft: Fix and improve base chain handling Phil Sutter
                   ` (2 preceding siblings ...)
  2021-09-22 16:06 ` [iptables PATCH 3/4] nft-chain: Introduce base_slot field Phil Sutter
@ 2021-09-22 16:06 ` Phil Sutter
  2021-09-27  7:32   ` Florian Westphal
  3 siblings, 1 reply; 6+ messages in thread
From: Phil Sutter @ 2021-09-22 16:06 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Attempting to delete all chains if --delete-chain is called without
argument has unwanted side-effects especially legacy iptables users are
not aware of and won't expect:

* Non-default policies are ignored, a previously dropping firewall may
  start accepting traffic.

* The kernel refuses to remove non-empty chains, causing program abort
  even if no user-defined chain exists.

Fix this by requiring a rule cache in that situation and make builtin
chain deletion depend on its policy and number of rules. Since this may
change concurrently, check again when having to refresh the transaction.

Also, hide builtin chains from verbose output - their creation is
implicit, so treat their removal as implicit, too.

When deleting a specific chain, do not allow to skip the job though.
Otherwise deleting a builtin chain which is still in use will succeed
although not executed.

Fixes: 61e85e3192dea ("iptables-nft: allow removal of empty builtin chains")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cmd.c                            |  2 +-
 iptables/nft.c                                | 39 +++++++++++++++----
 .../shell/testcases/chain/0005base-delete_0   | 34 ++++++++++++++++
 3 files changed, 66 insertions(+), 9 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/chain/0005base-delete_0

diff --git a/iptables/nft-cmd.c b/iptables/nft-cmd.c
index 2d874bd41e8f6..fcd01bd02831c 100644
--- a/iptables/nft-cmd.c
+++ b/iptables/nft-cmd.c
@@ -220,7 +220,7 @@ int nft_cmd_chain_del(struct nft_handle *h, const char *chain,
 	/* This triggers nft_bridge_chain_postprocess() when fetching the
 	 * rule cache.
 	 */
-	if (h->family == NFPROTO_BRIDGE)
+	if (h->family == NFPROTO_BRIDGE || !chain)
 		nft_cache_level_set(h, NFT_CL_RULES, cmd);
 	else
 		nft_cache_level_set(h, NFT_CL_CHAINS, cmd);
diff --git a/iptables/nft.c b/iptables/nft.c
index 381061473047f..dc1f5160eb983 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1838,26 +1838,46 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
 
 struct chain_del_data {
 	struct nft_handle	*handle;
+	const char		*chain;
 	bool			verbose;
 };
 
+static bool nft_may_delete_chain(struct nftnl_chain *c)
+{
+	if (nftnl_chain_is_set(c, NFTNL_CHAIN_POLICY) &&
+	    nftnl_chain_get_u32(c, NFTNL_CHAIN_POLICY) != NF_ACCEPT)
+		return false;
+
+	return nftnl_rule_lookup_byindex(c, 0) == NULL;
+}
+
 static int __nft_chain_del(struct nft_chain *nc, void *data)
 {
 	struct chain_del_data *d = data;
 	struct nftnl_chain *c = nc->nftnl;
 	struct nft_handle *h = d->handle;
+	bool builtin = nft_chain_builtin(c);
+	struct obj_update *obj;
+	int ret = 0;
 
-	if (d->verbose)
+	if (d->verbose && !builtin)
 		fprintf(stdout, "Deleting chain `%s'\n",
 			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME));
 
 
 	/* XXX This triggers a fast lookup from the kernel. */
 	nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE);
-	if (!batch_chain_add(h, NFT_COMPAT_CHAIN_DEL, c))
+	obj = batch_chain_add(h, NFT_COMPAT_CHAIN_DEL, c);
+	if (!obj)
 		return -1;
 
-	if (nft_chain_builtin(c)) {
+	if (builtin) {
+		obj->skip = !nft_may_delete_chain(c);
+		if (obj->skip && d->chain) {
+			/* complain if explicitly requested */
+			errno = EBUSY;
+			ret = -1;
+		}
 		*nc->base_slot = NULL;
 	}
 
@@ -1866,14 +1886,15 @@ static int __nft_chain_del(struct nft_chain *nc, void *data)
 
 	nft_chain_list_del(nc);
 	nft_chain_free(nc);
-	return 0;
+	return ret;
 }
 
 int nft_chain_del(struct nft_handle *h, const char *chain,
-		       const char *table, bool verbose)
+		  const char *table, bool verbose)
 {
 	struct chain_del_data d = {
 		.handle = h,
+		.chain = chain,
 		.verbose = verbose,
 	};
 	struct nft_chain *c;
@@ -1889,8 +1910,6 @@ int nft_chain_del(struct nft_handle *h, const char *chain,
 		}
 
 		ret = __nft_chain_del(c, &d);
-		if (ret == -2)
-			errno = EINVAL;
 		goto out;
 	}
 
@@ -2744,10 +2763,14 @@ static void nft_refresh_transaction(struct nft_handle *h)
 
 			n->skip = !nft_chain_find(h, tablename, chainname);
 			break;
+		case NFT_COMPAT_CHAIN_DEL:
+			if (!nftnl_chain_get(n->chain, NFTNL_CHAIN_HOOKNUM))
+				break;
+			n->skip = !nft_may_delete_chain(n->chain);
+			break;
 		case NFT_COMPAT_TABLE_ADD:
 		case NFT_COMPAT_CHAIN_ADD:
 		case NFT_COMPAT_CHAIN_ZERO:
-		case NFT_COMPAT_CHAIN_DEL:
 		case NFT_COMPAT_CHAIN_USER_FLUSH:
 		case NFT_COMPAT_CHAIN_UPDATE:
 		case NFT_COMPAT_CHAIN_RENAME:
diff --git a/iptables/tests/shell/testcases/chain/0005base-delete_0 b/iptables/tests/shell/testcases/chain/0005base-delete_0
new file mode 100755
index 0000000000000..033a28191d115
--- /dev/null
+++ b/iptables/tests/shell/testcases/chain/0005base-delete_0
@@ -0,0 +1,34 @@
+#!/bin/bash -x
+
+$XT_MULTI iptables -N foo || exit 1
+$XT_MULTI iptables -P FORWARD DROP || exit 1
+$XT_MULTI iptables -X || exit 1
+$XT_MULTI iptables -X foo && exit 1
+
+# indefinite -X fails if a non-empty user-defined chain exists
+$XT_MULTI iptables -N foo
+$XT_MULTI iptables -N bar
+$XT_MULTI iptables -A bar -j ACCEPT
+$XT_MULTI iptables -X && exit 1
+$XT_MULTI iptables -D bar -j ACCEPT
+$XT_MULTI iptables -X || exit 1
+
+# make sure OUTPUT chain is created by iptables-nft
+$XT_MULTI iptables -A OUTPUT -j ACCEPT || exit 1
+$XT_MULTI iptables -D OUTPUT -j ACCEPT || exit 1
+
+case $XT_MULTI in
+*xtables-nft-multi)
+	# must not delete chain FORWARD, its policy is not ACCEPT
+	$XT_MULTI iptables -X FORWARD && exit 1
+	nft list chain ip filter FORWARD || exit 1
+	# this should evict chain OUTPUT
+	$XT_MULTI iptables -X OUTPUT || exit 1
+	nft list chain ip filter OUTPUT && exit 1
+	;;
+*)
+	$XT_MULTI iptables -X FORWARD && exit 1
+	$XT_MULTI iptables -X OUTPUT && exit 1
+	;;
+esac
+exit 0
-- 
2.33.0


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

* Re: [iptables PATCH 4/4] nft: Delete builtin chains compatibly
  2021-09-22 16:06 ` [iptables PATCH 4/4] nft: Delete builtin chains compatibly Phil Sutter
@ 2021-09-27  7:32   ` Florian Westphal
  0 siblings, 0 replies; 6+ messages in thread
From: Florian Westphal @ 2021-09-27  7:32 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel

Phil Sutter <phil@nwl.cc> wrote:
> Attempting to delete all chains if --delete-chain is called without
> argument has unwanted side-effects especially legacy iptables users are
> not aware of and won't expect:
> 
> * Non-default policies are ignored, a previously dropping firewall may
>   start accepting traffic.
> 
> * The kernel refuses to remove non-empty chains, causing program abort
>   even if no user-defined chain exists.
> 
> Fix this by requiring a rule cache in that situation and make builtin
> chain deletion depend on its policy and number of rules. Since this may
> change concurrently, check again when having to refresh the transaction.
> 
> Also, hide builtin chains from verbose output - their creation is
> implicit, so treat their removal as implicit, too.
> 
> When deleting a specific chain, do not allow to skip the job though.
> Otherwise deleting a builtin chain which is still in use will succeed
> although not executed.

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

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

end of thread, other threads:[~2021-09-27  7:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 16:06 [iptables PATCH 0/4] nft: Fix and improve base chain handling Phil Sutter
2021-09-22 16:06 ` [iptables PATCH 1/4] nft: cache: Avoid double free of unrecognized base-chains Phil Sutter
2021-09-22 16:06 ` [iptables PATCH 2/4] nft: Check base-chain compatibility when adding to cache Phil Sutter
2021-09-22 16:06 ` [iptables PATCH 3/4] nft-chain: Introduce base_slot field Phil Sutter
2021-09-22 16:06 ` [iptables PATCH 4/4] nft: Delete builtin chains compatibly Phil Sutter
2021-09-27  7:32   ` Florian Westphal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).