From 757b84e866dc1f288c54ad5ca9868b1765da3948 Mon Sep 17 00:00:00 2001
From: Phil Sutter <phil@nwl.cc>
Date: Thu, 20 Dec 2018 16:09:04 +0100
Subject: [PATCH] xtables-restore: Review chain handling

There is no need to "delete" (actually, remove from cache) a chain if
noflush wasn't given: While handling the corresponding table line,
'table_flush' callback has already taken care of that.

This .chain_del indirection is not required since d1eb4d587297
("iptables-compat: chains are purge out already from table flush").

Streamlining the code further, move syntax checks to the top. If these
concede, there are three cases to distinguish:

A) Given chain name matches a builtin one in current table, so assume it
   exists already and just set policy and counters.

B) Noflush was given and the (custom) chain exists already, flush it.

C) Custom chain was either flushed (noflush not given) or didn't exist
   before, create it.

Signed-off-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
(cherry picked from commit 9523b2e9dee3d9b4439214092c496542ce9f434e)
Signed-off-by: Phil Sutter <psutter@redhat.com>
---
 iptables/nft-shared.h      |  2 --
 iptables/xtables-restore.c | 68 +++++++++++---------------------------
 2 files changed, 19 insertions(+), 51 deletions(-)

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index 9a61d8d2863e3..17fff984ba312 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -253,8 +253,6 @@ 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);
-	void (*chain_del)(struct nftnl_chain_list *clist, const char *curtable,
-			  const char *chain);
 	int (*chain_user_flush)(struct nft_handle *h,
 				struct nftnl_chain_list *clist,
 				const char *table, const char *chain);
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 642876d6c70ac..4e00ed86be06d 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -68,21 +68,6 @@ static struct nftnl_chain_list *get_chain_list(struct nft_handle *h,
 	return chain_list;
 }
 
-static void chain_delete(struct nftnl_chain_list *clist, const char *curtable,
-			 const char *chain)
-{
-	struct nftnl_chain *chain_obj;
-
-	chain_obj = nft_chain_list_find(clist, chain);
-	/* This chain has been found, delete from list. Later
-	 * on, unvisited chains will be purged out.
-	 */
-	if (chain_obj != NULL) {
-		nftnl_chain_list_del(chain_obj);
-		nftnl_chain_free(chain_obj);
-	}
-}
-
 struct nft_xt_restore_cb restore_cb = {
 	.chain_list	= get_chain_list,
 	.commit		= nft_commit,
@@ -90,7 +75,6 @@ struct nft_xt_restore_cb restore_cb = {
 	.table_new	= nft_table_new,
 	.table_flush	= nft_table_flush,
 	.chain_user_flush = nft_chain_user_flush,
-	.chain_del	= chain_delete,
 	.do_command	= do_commandx,
 	.chain_set	= nft_chain_set,
 	.chain_user_add	= nft_chain_user_add,
@@ -183,7 +167,6 @@ void xtables_restore_parse(struct nft_handle *h,
 			/* New chain. */
 			char *policy, *chain = NULL;
 			struct xt_counters count = {};
-			bool chain_exists = false;
 
 			chain = strtok(buffer+1, " \t\n");
 			DEBUGP("line %u, chain '%s'\n", line, chain);
@@ -194,21 +177,6 @@ void xtables_restore_parse(struct nft_handle *h,
 				exit(1);
 			}
 
-			if (noflush == 0) {
-				if (cb->chain_del)
-					cb->chain_del(chain_list, curtable->name,
-						      chain);
-			} else if (nft_chain_list_find(chain_list, chain)) {
-				chain_exists = true;
-				/* 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);
-			}
-
 			if (strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
 				xtables_error(PARAMETER_PROBLEM,
 					   "Invalid chain name `%s' "
@@ -246,24 +214,28 @@ void xtables_restore_parse(struct nft_handle *h,
 				}
 				DEBUGP("Setting policy of chain %s to %s\n",
 				       chain, policy);
-				ret = 1;
 
-			} else {
-				if (!chain_exists &&
-				    cb->chain_user_add &&
-				    cb->chain_user_add(h, chain,
-						       curtable->name) < 0) {
-					if (errno == EEXIST)
-						continue;
+			} 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) {
+				if (errno == EEXIST)
+					continue;
 
-					xtables_error(PARAMETER_PROBLEM,
-						      "cannot create chain "
-						      "'%s' (%s)\n", chain,
-						      strerror(errno));
-				}
-				continue;
+				xtables_error(PARAMETER_PROBLEM,
+					      "cannot create chain "
+					      "'%s' (%s)\n", chain,
+					      strerror(errno));
 			}
-
+			ret = 1;
 		} else if (in_table) {
 			int a;
 			char *pcnt = NULL;
@@ -496,7 +468,6 @@ struct nft_xt_restore_cb ebt_restore_cb = {
 	.table_new	= nft_table_new,
 	.table_flush	= nft_table_flush,
 	.chain_user_flush = nft_chain_user_flush,
-	.chain_del	= chain_delete,
 	.do_command	= do_commandeb,
 	.chain_set	= nft_chain_set,
 	.chain_user_add	= nft_chain_user_add,
@@ -542,7 +513,6 @@ struct nft_xt_restore_cb arp_restore_cb = {
 	.table_new	= nft_table_new,
 	.table_flush	= nft_table_flush,
 	.chain_user_flush = nft_chain_user_flush,
-	.chain_del	= chain_delete,
 	.do_command	= do_commandarp,
 	.chain_set	= nft_chain_set,
 	.chain_user_add	= nft_chain_user_add,
-- 
2.21.0