[iptables-nftables - PATCH 0/9] Various fixes

[iptables-nftables - PATCH 0/9] Various fixes

[Tomasz Bursztyka]

Hi,

Here are some fixes, mostly on output issues.
Afaik, nftables does not handle any chain ordering, builtin ones or not, but in iptables user is used to see builtins ones in a certains order and the rest.
So patch 6, 7 and 8 ensures this.

Other patches are self explained.

Tomasz Bursztyka (9):
  nft: Set the rule family when creating a new one
  nft: Handle error on adding rule expressions
  nft: Refactor and optimize nft_rule_list
  xtables: Remove useless parameter to nft_chain_list_find
  nft: Une one unique function to test for a builtin chain
  nft: Print chains in right order when listing rules
  nft: Print chains in right order when saving rules
  xtables-save: Print chains in right order
  nft: Fix small memory leaks

 iptables/nft.c             | 250 ++++++++++++++++++++++++++++++---------------
 iptables/nft.h             |   2 +-
 iptables/xtables-restore.c |   2 +-
 3 files changed, 171 insertions(+), 83 deletions(-)

-- 
1.8.2.1

[iptables-nftables - PATCH 1/9] nft: Set the rule family when creating a new one

[Tomasz Bursztyka]

Fixes the debug output from (in case of ipv4 rule):
	DEBUG: rule: arp filter INPUT 0
to:
	DEBUG: rule: ip filter INPUT 0

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/iptables/nft.c b/iptables/nft.c
index 2dba7ff..f475d28 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -690,6 +690,7 @@ nft_rule_add(struct nft_handle *h, const char *chain, const char *table,
 		goto err;
 	}
 
+	nft_rule_attr_set_u32(r, NFT_RULE_ATTR_FAMILY, h->family);
 	nft_rule_attr_set(r, NFT_RULE_ATTR_TABLE, (char *)table);
 	nft_rule_attr_set(r, NFT_RULE_ATTR_CHAIN, (char *)chain);
 
-- 
1.8.2.1

[iptables-nftables - PATCH 2/9] nft: Handle error on adding rule expressions

[Tomasz Bursztyka]

If adding one of match/target/jumpto/verdit/counters fails, adding a rule will
return an error.

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 78 +++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 25 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index f475d28..e62885b 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -548,7 +548,7 @@ int nft_chain_set(struct nft_handle *h, const char *table,
 	return ret == 0 ? 1 : 0;
 }
 
-static void __add_match(struct nft_rule_expr *e, struct xt_entry_match *m)
+static int __add_match(struct nft_rule_expr *e, struct xt_entry_match *m)
 {
 	void *info;
 
@@ -557,25 +557,30 @@ static void __add_match(struct nft_rule_expr *e, struct xt_entry_match *m)
 
 	info = calloc(1, m->u.match_size);
 	if (info == NULL)
-		return;
+		return -ENOMEM;
 
 	memcpy(info, m->data, m->u.match_size);
 	nft_rule_expr_set(e, NFT_EXPR_MT_INFO, info, m->u.match_size - sizeof(*m));
+
+	return 0;
 }
 
-static void add_match(struct nft_rule *r, struct xt_entry_match *m)
+static int add_match(struct nft_rule *r, struct xt_entry_match *m)
 {
 	struct nft_rule_expr *expr;
+	int ret;
 
 	expr = nft_rule_expr_alloc("match");
 	if (expr == NULL)
-		return;
+		return -ENOMEM;
 
-	__add_match(expr, m);
+	ret = __add_match(expr, m);
 	nft_rule_add_expr(r, expr);
+
+	return ret;
 }
 
-static void __add_target(struct nft_rule_expr *e, struct xt_entry_target *t)
+static int __add_target(struct nft_rule_expr *e, struct xt_entry_target *t)
 {
 	void *info = NULL;
 
@@ -586,51 +591,60 @@ static void __add_target(struct nft_rule_expr *e, struct xt_entry_target *t)
 	if (info == NULL) {
 		info = calloc(1, t->u.target_size);
 		if (info == NULL)
-			return;
+			return -ENOMEM;
 
 		memcpy(info, t->data, t->u.target_size);
 	}
 
 	nft_rule_expr_set(e, NFT_EXPR_TG_INFO, info, t->u.target_size - sizeof(*t));
+
+	return 0;
 }
 
-static void add_target(struct nft_rule *r, struct xt_entry_target *t)
+static int add_target(struct nft_rule *r, struct xt_entry_target *t)
 {
 	struct nft_rule_expr *expr;
+	int ret;
 
 	expr = nft_rule_expr_alloc("target");
 	if (expr == NULL)
-		return;
+		return -ENOMEM;
 
-	__add_target(expr, t);
+	ret = __add_target(expr, t);
 	nft_rule_add_expr(r, expr);
+
+	return ret;
 }
 
-static void add_jumpto(struct nft_rule *r, const char *name, int verdict)
+static int add_jumpto(struct nft_rule *r, const char *name, int verdict)
 {
 	struct nft_rule_expr *expr;
 
 	expr = nft_rule_expr_alloc("immediate");
 	if (expr == NULL)
-		return;
+		return -ENOMEM;
 
 	nft_rule_expr_set_u32(expr, NFT_EXPR_IMM_DREG, NFT_REG_VERDICT);
 	nft_rule_expr_set_u32(expr, NFT_EXPR_IMM_VERDICT, verdict);
 	nft_rule_expr_set_str(expr, NFT_EXPR_IMM_CHAIN, (char *)name);
 	nft_rule_add_expr(r, expr);
+
+	return 0;
 }
 
-static void add_verdict(struct nft_rule *r, int verdict)
+static int add_verdict(struct nft_rule *r, int verdict)
 {
 	struct nft_rule_expr *expr;
 
 	expr = nft_rule_expr_alloc("immediate");
 	if (expr == NULL)
-		return;
+		return -ENOMEM;
 
 	nft_rule_expr_set_u32(expr, NFT_EXPR_IMM_DREG, NFT_REG_VERDICT);
 	nft_rule_expr_set_u32(expr, NFT_EXPR_IMM_VERDICT, verdict);
 	nft_rule_add_expr(r, expr);
+
+	return 0;
 }
 
 static void nft_rule_print_debug(struct nft_rule *r, struct nlmsghdr *nlh)
@@ -644,18 +658,20 @@ static void nft_rule_print_debug(struct nft_rule *r, struct nlmsghdr *nlh)
 #endif
 }
 
-static void add_counters(struct nft_rule *r, uint64_t packets, uint64_t bytes)
+static int add_counters(struct nft_rule *r, uint64_t packets, uint64_t bytes)
 {
 	struct nft_rule_expr *expr;
 
 	expr = nft_rule_expr_alloc("counter");
 	if (expr == NULL)
-		return;
+		return -ENOMEM;
 
 	nft_rule_expr_set_u64(expr, NFT_EXPR_CTR_BYTES, packets);
 	nft_rule_expr_set_u64(expr, NFT_EXPR_CTR_PACKETS, bytes);
 
 	nft_rule_add_expr(r, expr);
+
+	return 0;
 }
 
 void add_compat(struct nft_rule *r, uint32_t proto, bool inv)
@@ -696,31 +712,43 @@ nft_rule_add(struct nft_handle *h, const char *chain, const char *table,
 
 	ip_flags = h->ops->add(r, cs);
 
-	for (matchp = cs->matches; matchp; matchp = matchp->next)
-		add_match(r, matchp->match->m);
+	for (matchp = cs->matches; matchp; matchp = matchp->next) {
+		if (add_match(r, matchp->match->m) < 0) {
+			ret = 0;
+			goto err;
+		}
+	}
 
 	/* Counters need to me added before the target, otherwise they are
 	 * increased for each rule because of the way nf_tables works.
 	 */
-	add_counters(r, cs->counters.pcnt, cs->counters.bcnt);
+	if (add_counters(r, cs->counters.pcnt, cs->counters.bcnt) < 0) {
+		ret = 0;
+		goto err;
+	}
 
 	/* If no target at all, add nothing (default to continue) */
 	if (cs->target != NULL) {
 		/* Standard target? */
 		if (strcmp(cs->jumpto, XTC_LABEL_ACCEPT) == 0)
-			add_verdict(r, NF_ACCEPT);
+			ret = add_verdict(r, NF_ACCEPT);
 		else if (strcmp(cs->jumpto, XTC_LABEL_DROP) == 0)
-			add_verdict(r, NF_DROP);
+			ret = add_verdict(r, NF_DROP);
 		else if (strcmp(cs->jumpto, XTC_LABEL_RETURN) == 0)
-			add_verdict(r, NFT_RETURN);
+			ret = add_verdict(r, NFT_RETURN);
 		else
-			add_target(r, cs->target->t);
+			ret = add_target(r, cs->target->t);
 	} else if (strlen(cs->jumpto) > 0) {
 		/* Not standard, then it's a go / jump to chain */
 		if (ip_flags & IPT_F_GOTO)
-			add_jumpto(r, cs->jumpto, NFT_GOTO);
+			ret = add_jumpto(r, cs->jumpto, NFT_GOTO);
 		else
-			add_jumpto(r, cs->jumpto, NFT_JUMP);
+			ret = add_jumpto(r, cs->jumpto, NFT_JUMP);
+	}
+
+	if (ret < 0) {
+		ret = 0;
+		goto err;
 	}
 
 	/* NLM_F_CREATE autoloads the built-in table if it does not exists */
-- 
1.8.2.1

[iptables-nftables - PATCH 3/9] nft: Refactor and optimize nft_rule_list

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 61 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index e62885b..09a4e95 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2444,13 +2444,35 @@ err:
 	return ret;
 }
 
+static void __nft_chain_rule_list(struct nft_handle *h, struct nft_chain *c,
+				  const char *table, int rulenum,
+				  unsigned int format)
+{
+	const char *chain_name = nft_chain_attr_get_str(c,
+							NFT_CHAIN_ATTR_NAME);
+	uint32_t policy = nft_chain_attr_get_u32(c, NFT_CHAIN_ATTR_POLICY);
+	int32_t refs = nft_chain_attr_get_u32(c, NFT_CHAIN_ATTR_USE);
+	struct xt_counters ctrs = {
+		.pcnt = nft_chain_attr_get_u64(c, NFT_CHAIN_ATTR_PACKETS),
+		.bcnt = nft_chain_attr_get_u64(c, NFT_CHAIN_ATTR_BYTES),
+	};
+	bool basechain = false;
+
+	if (nft_chain_attr_get(c, NFT_CHAIN_ATTR_HOOKNUM))
+		basechain = true;
+
+	print_header(format, chain_name, policy_name[policy],
+						&ctrs, basechain, refs);
+	__nft_rule_list(h, c, table, rulenum, format, print_firewall);
+}
+
 int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 		  int rulenum, unsigned int format)
 {
 	struct nft_chain_list *list;
 	struct nft_chain_list_iter *iter;
 	struct nft_chain *c;
-	bool found = false;
+	bool round = false;
 
 	/* If built-in chains don't exist for this table, create them */
 	if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
@@ -2458,50 +2480,37 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 
 	list = nft_chain_dump(h);
 
+	if (chain != NULL) {
+		c = nft_chain_list_find(h, list, table, chain);
+		if (c != NULL)
+			__nft_chain_rule_list(h, c, table, rulenum, format);
+		goto out;
+	};
+
 	iter = nft_chain_list_iter_create(list);
 	if (iter == NULL)
-		goto err;
+		goto out;
 
 	c = nft_chain_list_iter_next(iter);
 	while (c != NULL) {
 		const char *chain_table =
 			nft_chain_attr_get_str(c, NFT_CHAIN_ATTR_TABLE);
-		const char *chain_name =
-			nft_chain_attr_get_str(c, NFT_CHAIN_ATTR_NAME);
-		uint32_t policy =
-			nft_chain_attr_get_u32(c, NFT_CHAIN_ATTR_POLICY);
-		uint32_t refs =
-			nft_chain_attr_get_u32(c, NFT_CHAIN_ATTR_USE);
-		struct xt_counters ctrs = {
-			.pcnt = nft_chain_attr_get_u64(c, NFT_CHAIN_ATTR_PACKETS),
-			.bcnt = nft_chain_attr_get_u64(c, NFT_CHAIN_ATTR_BYTES),
-		};
-		bool basechain = false;
-
-		if (nft_chain_attr_get(c, NFT_CHAIN_ATTR_HOOKNUM))
-			basechain = true;
 
 		if (strcmp(table, chain_table) != 0)
 			goto next;
-		if (chain && strcmp(chain, chain_name) != 0)
-			goto next;
 
-		if (found)
+		if (round)
 			printf("\n");
 
-		print_header(format, chain_name, policy_name[policy], &ctrs,
-			     basechain, refs);
-
-		__nft_rule_list(h, c, table, rulenum, format, print_firewall);
-
-		found = true;
+		__nft_chain_rule_list(h, c, table, rulenum, format);
 
+		round = true;
 next:
 		c = nft_chain_list_iter_next(iter);
 	}
 
 	nft_chain_list_iter_destroy(iter);
-err:
+out:
 	nft_chain_list_free(list);
 
 	return 1;
-- 
1.8.2.1

[iptables-nftables - PATCH 4/9] xtables: Remove useless parameter to nft_chain_list_find

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c             | 6 +++---
 iptables/nft.h             | 2 +-
 iptables/xtables-restore.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 09a4e95..bcb834e 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1428,7 +1428,7 @@ err:
 }
 
 struct nft_chain *
-nft_chain_list_find(struct nft_handle *h, struct nft_chain_list *list,
+nft_chain_list_find(struct nft_chain_list *list,
 		    const char *table, const char *chain)
 {
 	struct nft_chain_list_iter *iter;
@@ -1469,7 +1469,7 @@ nft_chain_find(struct nft_handle *h, const char *table, const char *chain)
 	if (list == NULL)
 		return NULL;
 
-	return nft_chain_list_find(h, list, table, chain);
+	return nft_chain_list_find(list, table, chain);
 }
 
 int nft_chain_user_rename(struct nft_handle *h,const char *chain,
@@ -2481,7 +2481,7 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 	list = nft_chain_dump(h);
 
 	if (chain != NULL) {
-		c = nft_chain_list_find(h, list, table, chain);
+		c = nft_chain_list_find(list, table, chain);
 		if (c != NULL)
 			__nft_chain_rule_list(h, c, table, rulenum, format);
 		goto out;
diff --git a/iptables/nft.h b/iptables/nft.h
index 082260e..a647671 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -37,7 +37,7 @@ struct nft_chain;
 int nft_chain_add(struct nft_handle *h, const struct nft_chain *c);
 int nft_chain_set(struct nft_handle *h, const char *table, const char *chain, const char *policy, const struct xt_counters *counters);
 struct nft_chain_list *nft_chain_dump(struct nft_handle *h);
-struct nft_chain *nft_chain_list_find(struct nft_handle *h, struct nft_chain_list *list, const char *table, const char *chain);
+struct nft_chain *nft_chain_list_find(struct nft_chain_list *list, const char *table, const char *chain);
 int nft_chain_save(struct nft_handle *h, struct nft_chain_list *list, const char *table);
 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);
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index e66f10c..8469ba1 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -326,7 +326,7 @@ xtables_restore_main(int argc, char *argv[])
 				exit(1);
 			}
 
-			chain_obj = nft_chain_list_find(&h, chain_list,
+			chain_obj = nft_chain_list_find(chain_list,
 							curtable, chain);
 			/* This chain has been found, delete from list. Later
 			 * on, unvisited chains will be purged out.
-- 
1.8.2.1

[iptables-nftables - PATCH 5/9] nft: Une one unique function to test for a builtin chain

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index bcb834e..230c4f7 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -381,6 +381,14 @@ out:
 	return ret;
 }
 
+static bool nft_chain_builtin(struct nft_chain *c)
+{
+	/* Check if this chain has hook number, in that case is built-in.
+	 * Should we better export the flags to user-space via nf_tables?
+	 */
+	return nft_chain_attr_get(c, NFT_CHAIN_ATTR_HOOKNUM) != NULL;
+}
+
 int nft_init(struct nft_handle *h)
 {
 	h->nl = mnl_socket_open(NETLINK_NETFILTER);
@@ -1132,9 +1140,7 @@ int nft_chain_save(struct nft_handle *h, struct nft_chain_list *list,
 		if (strcmp(table, chain_table) != 0)
 			goto next;
 
-		if (nft_chain_attr_get(c, NFT_CHAIN_ATTR_HOOKNUM))
-			basechain = true;
-
+		basechain = nft_chain_builtin(c);
 		nft_chain_print_save(c, basechain);
 next:
 		c = nft_chain_list_iter_next(iter);
@@ -1362,14 +1368,6 @@ static int __nft_chain_del(struct nft_handle *h, struct nft_chain *c)
 	return ret;
 }
 
-static bool nft_chain_builtin(struct nft_chain *c)
-{
-	/* Check if this chain has hook number, in that case is built-in.
-	 * Should we better export the flags to user-space via nf_tables?
-	 */
-	return nft_chain_attr_get(c, NFT_CHAIN_ATTR_HOOKNUM) != NULL;
-}
-
 int nft_chain_user_del(struct nft_handle *h, const char *chain, const char *table)
 {
 	struct nft_chain_list *list;
@@ -2456,10 +2454,7 @@ static void __nft_chain_rule_list(struct nft_handle *h, struct nft_chain *c,
 		.pcnt = nft_chain_attr_get_u64(c, NFT_CHAIN_ATTR_PACKETS),
 		.bcnt = nft_chain_attr_get_u64(c, NFT_CHAIN_ATTR_BYTES),
 	};
-	bool basechain = false;
-
-	if (nft_chain_attr_get(c, NFT_CHAIN_ATTR_HOOKNUM))
-		basechain = true;
+	bool basechain = nft_chain_builtin(c);
 
 	print_header(format, chain_name, policy_name[policy],
 						&ctrs, basechain, refs);
@@ -2547,7 +2542,7 @@ nft_rule_list_chain_save(struct nft_handle *h, const char *table,
 			goto next;
 
 		/* this is a base chain */
-		if (nft_chain_attr_get(c, NFT_CHAIN_ATTR_HOOKNUM)) {
+		if (nft_chain_builtin(c)) {
 			printf("-P %s %s", chain_name, policy_name[policy]);
 
 			if (counters) {
-- 
1.8.2.1

[iptables-nftables - PATCH 6/9] nft: Print chains in right order when listing rules

[Tomasz Bursztyka]

Fixes an output bug, it was:
Chain OUTPUT (policy ACCEPT)
target     prot opt source               destination

Chain FORWARD (policy ACCEPT)
target     prot opt source               destination

Chain INPUT (policy ACCEPT)
target     prot opt source               destination

where it should be:
Chain INPUT (policy ACCEPT)
target     prot opt source               destination

Chain FORWARD (policy ACCEPT)
target     prot opt source               destination

Chain OUTPUT (policy ACCEPT)
target     prot opt source               destination

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 230c4f7..2f03f63 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2464,10 +2464,12 @@ static void __nft_chain_rule_list(struct nft_handle *h, struct nft_chain *c,
 int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 		  int rulenum, unsigned int format)
 {
+	const struct builtin_table *t;
 	struct nft_chain_list *list;
 	struct nft_chain_list_iter *iter;
 	struct nft_chain *c;
 	bool round = false;
+	int i;
 
 	/* If built-in chains don't exist for this table, create them */
 	if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
@@ -2482,6 +2484,22 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 		goto out;
 	};
 
+	/* Let's print out builtin chains first, in right order */
+	t = nft_table_builtin_find(table);
+	if (t == NULL)
+		goto out;
+
+	for (i = 0; i < NF_IP_NUMHOOKS && t->chains[i].name != NULL; i++) {
+		if (round)
+			printf("\n");
+
+		c = nft_chain_list_find(list, table, t->chains[i].name);
+		if (c != NULL) {
+			__nft_chain_rule_list(h, c, table, rulenum, format);
+			round = true;
+		}
+	}
+
 	iter = nft_chain_list_iter_create(list);
 	if (iter == NULL)
 		goto out;
@@ -2494,12 +2512,12 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 		if (strcmp(table, chain_table) != 0)
 			goto next;
 
-		if (round)
-			printf("\n");
+		/* we skip already listed builtin chains */
+		if (nft_chain_builtin(c))
+			goto next;
 
+		printf("\n");
 		__nft_chain_rule_list(h, c, table, rulenum, format);
-
-		round = true;
 next:
 		c = nft_chain_list_iter_next(iter);
 	}
-- 
1.8.2.1

[iptables-nftables - PATCH 7/9] nft: Print chains in right order when saving rules

[Tomasz Bursztyka]

Fixes the output which was:
-P OUTPUT ACCEPT
-P FORWARD ACCEPT
-P INPUT ACCEPT

Where it should be:
-P INPUT ACCEPT
-P FORWARD ACCEPT
-P OUTPUT ACCEPT

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 46 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 2f03f63..4ca1cec 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2540,8 +2540,36 @@ static int
 nft_rule_list_chain_save(struct nft_handle *h, const char *table,
 			 struct nft_chain_list *list, int counters)
 {
+	const struct builtin_table *t;
 	struct nft_chain_list_iter *iter;
 	struct nft_chain *c;
+	int i;
+
+	/* Let's print out builtin chains first, in right order */
+	t = nft_table_builtin_find(table);
+	if (t == NULL)
+		return 0;
+
+	for (i = 0; i < NF_IP_NUMHOOKS && t->chains[i].name != NULL; i++) {
+		uint32_t policy;
+
+		c = nft_chain_list_find(list, table, t->chains[i].name);
+		if (c == NULL)
+			return 0;
+
+		policy = nft_chain_attr_get_u32(c, NFT_CHAIN_ATTR_POLICY);
+
+		printf("-P %s %s", t->chains[i].name, policy_name[policy]);
+		if (counters) {
+			printf(" -c %"PRIu64" %"PRIu64"\n",
+				nft_chain_attr_get_u64(c,
+						NFT_CHAIN_ATTR_PACKETS),
+				nft_chain_attr_get_u64(c,
+						NFT_CHAIN_ATTR_BYTES));
+		}
+
+		printf("\n");
+	}
 
 	iter = nft_chain_list_iter_create(list);
 	if (iter == NULL)
@@ -2553,25 +2581,15 @@ nft_rule_list_chain_save(struct nft_handle *h, const char *table,
 			nft_chain_attr_get_str(c, NFT_CHAIN_ATTR_TABLE);
 		const char *chain_name =
 			nft_chain_attr_get_str(c, NFT_CHAIN_ATTR_NAME);
-		uint32_t policy =
-			nft_chain_attr_get_u32(c, NFT_CHAIN_ATTR_POLICY);
 
 		if (strcmp(table, chain_table) != 0)
 			goto next;
 
-		/* this is a base chain */
-		if (nft_chain_builtin(c)) {
-			printf("-P %s %s", chain_name, policy_name[policy]);
+		/* we already handled builtin chains */
+		if (nft_chain_builtin(c))
+			goto next;
 
-			if (counters) {
-				printf(" -c %"PRIu64" %"PRIu64"\n",
-					nft_chain_attr_get_u64(c, NFT_CHAIN_ATTR_PACKETS),
-					nft_chain_attr_get_u64(c, NFT_CHAIN_ATTR_BYTES));
-			} else
-				printf("\n");
-		} else {
-			printf("-N %s\n", chain_name);
-		}
+		printf("-N %s\n", chain_name);
 next:
 		c = nft_chain_list_iter_next(iter);
 	}
-- 
1.8.2.1

[iptables-nftables - PATCH 8/9] xtables-save: Print chains in right order

[Tomasz Bursztyka]

Fixes the output which was:
:OUTPUT ACCEPT [4271:670423]
:FORWARD ACCEPT [0:0]
:INPUT ACCEPT [6434:597396]

Where it should be:
:INPUT ACCEPT [6434:597396]
:FORWARD ACCEPT [0:0]
:OUTPUT ACCEPT [4271:670423]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 4ca1cec..2056032 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1124,8 +1124,21 @@ static void nft_chain_print_save(struct nft_chain *c, bool basechain)
 int nft_chain_save(struct nft_handle *h, struct nft_chain_list *list,
 		   const char *table)
 {
+	const struct builtin_table *t;
 	struct nft_chain_list_iter *iter;
 	struct nft_chain *c;
+	int i;
+
+	/* Let's print out builtin chains first, in right order */
+	t = nft_table_builtin_find(table);
+	if (t == NULL)
+		return 0;
+
+	for (i = 0; i < NF_IP_NUMHOOKS && t->chains[i].name != NULL; i++) {
+		c = nft_chain_list_find(list, table, t->chains[i].name);
+		if (c != NULL)
+			nft_chain_print_save(c, true);
+	}
 
 	iter = nft_chain_list_iter_create(list);
 	if (iter == NULL)
@@ -1135,13 +1148,15 @@ int nft_chain_save(struct nft_handle *h, struct nft_chain_list *list,
 	while (c != NULL) {
 		const char *chain_table =
 			nft_chain_attr_get_str(c, NFT_CHAIN_ATTR_TABLE);
-		bool basechain = false;
 
 		if (strcmp(table, chain_table) != 0)
 			goto next;
 
-		basechain = nft_chain_builtin(c);
-		nft_chain_print_save(c, basechain);
+		/* We already handled builtin chain */
+		if (nft_chain_builtin(c))
+			goto next;
+
+		nft_chain_print_save(c, false);
 next:
 		c = nft_chain_list_iter_next(iter);
 	}
-- 
1.8.2.1

[iptables-nftables - PATCH 9/9] nft: Fix small memory leaks

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/iptables/nft.c b/iptables/nft.c
index 2056032..0c0ca60 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2609,6 +2609,8 @@ next:
 		c = nft_chain_list_iter_next(iter);
 	}
 
+	nft_chain_list_iter_destroy(iter);
+
 	return 1;
 }
 
@@ -2646,6 +2648,8 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 next:
 		c = nft_chain_list_iter_next(iter);
 	}
+
+	nft_chain_list_iter_destroy(iter);
 err:
 	nft_chain_list_free(list);
 
-- 
1.8.2.1

Re: [iptables-nftables - PATCH 1/9] nft: Set the rule family when creating a new one

[Pablo Neira Ayuso]

On Tue, Jul 16, 2013 at 03:38:45PM +0300, Tomasz Bursztyka wrote:
> Fixes the debug output from (in case of ipv4 rule):
> 	DEBUG: rule: arp filter INPUT 0
> to:
> 	DEBUG: rule: ip filter INPUT 0

Applied.

Re: [iptables-nftables - PATCH 2/9] nft: Handle error on adding rule expressions

[Pablo Neira Ayuso]

On Tue, Jul 16, 2013 at 03:38:46PM +0300, Tomasz Bursztyka wrote:
> If adding one of match/target/jumpto/verdit/counters fails, adding a rule will
> return an error.

Applied, thanks.

Re: [iptables-nftables - PATCH 3/9] nft: Refactor and optimize nft_rule_list

[Pablo Neira Ayuso]

On Tue, Jul 16, 2013 at 03:38:47PM +0300, Tomasz Bursztyka wrote:
> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>

This does not apply cleanly to current head.

Could you rebase and resend, please?

Thanks.

Re: [iptables-nftables - PATCH 4/9] xtables: Remove useless parameter to nft_chain_list_find

[Pablo Neira Ayuso]

Applied, thanks.

Re: [iptables-nftables - PATCH 5/9] nft: Une one unique function to test for a builtin chain

[Pablo Neira Ayuso]

Applied, thanks.

Re: [iptables-nftables - PATCH 6/9] nft: Print chains in right order when listing rules

[Pablo Neira Ayuso]

On Tue, Jul 16, 2013 at 03:38:50PM +0300, Tomasz Bursztyka wrote:
> Fixes an output bug, it was:
> Chain OUTPUT (policy ACCEPT)
> target     prot opt source               destination
> 
> Chain FORWARD (policy ACCEPT)
> target     prot opt source               destination
> 
> Chain INPUT (policy ACCEPT)
> target     prot opt source               destination
>
> where it should be:
> Chain INPUT (policy ACCEPT)
> target     prot opt source               destination
> 
> Chain FORWARD (policy ACCEPT)
> target     prot opt source               destination
> 
> Chain OUTPUT (policy ACCEPT)
> target     prot opt source               destination

I have just checked this. The order is fine except by the nat table,
that one has been corrected it here:

http://git.netfilter.org/iptables-nftables/commit/?id=990b5aec1df02450545b57b94d3c960d9b7b1188

However, if the xtables.conf file is used, the order was reversed so I
could reproduce exactly the same output that you posted here.

I have fixed that by fixing the semantically of nft_*_list_add in
libnftables to prepend, instead of appending. Now we have
nft_*_list_add_tail, I have adapted iptables-nftables to use add_tail
when needed:

http://git.netfilter.org/iptables-nftables/commit/?id=5e6ed2aae9e4a8ec0a340036f485c2567635eca9

Those should be enough to resolve this issue.

Thanks for the initial patch to address this issue.

Re: [iptables-nftables - PATCH 9/9] nft: Fix small memory leaks

[Pablo Neira Ayuso]

Also applied, thanks.

Re: [iptables-nftables - PATCH 6/9] nft: Print chains in right order when listing rules

[Tomasz Bursztyka]

Hi Pablo,

> I have just checked this. The order is fine except by the nat table,
> that one has been corrected it here:
>
> http://git.netfilter.org/iptables-nftables/commit/?id=990b5aec1df02450545b57b94d3c960d9b7b1188
>
> However, if the xtables.conf file is used, the order was reversed so I
> could reproduce exactly the same output that you posted here.
>
> I have fixed that by fixing the semantically of nft_*_list_add in
> libnftables to prepend, instead of appending. Now we have
> nft_*_list_add_tail, I have adapted iptables-nftables to use add_tail
> when needed:
>
> http://git.netfilter.org/iptables-nftables/commit/?id=5e6ed2aae9e4a8ec0a340036f485c2567635eca9
>
> Those should be enough to resolve this issue.

If you think it's sufficient to ensure right chain ordering then ok, as 
long as users don't mess up with conf/save files.
I did not liked much the for loop on builtin chains anyway.

Tomasz

Re: [iptables-nftables - PATCH 3/9] nft: Refactor and optimize nft_rule_list

[Tomasz Bursztyka]

Hi Pablo,

> On Tue, Jul 16, 2013 at 03:38:47PM +0300, Tomasz Bursztyka wrote:
>> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
> This does not apply cleanly to current head.
>
> Could you rebase and resend, please?

Forget about this one, stuff are superfluous now that patch 6/7/8 are 
useless. I have another one coming then.

Tomasz