[iptables PATCH 2/4] nft: Check base-chain compatibility when adding to cache

[Phil Sutter <phil@nwl.cc>]

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