[PATCH 0/5] netfilter: nf_tables: nf_tables update

[PATCH 0/5] netfilter: nf_tables: nf_tables update

[kaber]

The following patches for nf_tables fix a few bugs and introduct
loop detection:

- anonymous sets are currently broken since we're not reporting the
  chosen set name to userspace anymore

- save 48 bits per rule by reducing the handle size and plugging a whole

- destroy anonymous sets immediately when binding fails since there
  will be no unbinding operation

- add loop detection for new jump verdicts

Please apply, thanks!

[PATCH 1/5] netfilter: nf_tables: fix anonymous sets

[kaber]

From: Patrick McHardy <kaber@trash.net>

Commit a109bd34 (netfilter: nf_tables: don't report anonymous sets) broke
use of anonymous sets by not reporting the created set name to userspace
anymore. Anonymous sets are created using "set%d", the notification is
needed so elements can be added to the finally chosen name.

set set%d@filter/inet <anonymous,constant>
	key: type 12 len 4
set set%d@filter/inet
element 0x00000002
element 0x00000001
internal:0:0-0: Error: Could not add set elements: Object not found

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/netfilter/nf_tables_api.c | 3 +--
 1 Datei geändert, 1 Zeile hinzugefügt(+), 2 Zeilen entfernt(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index f701dc0..570b877 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1915,8 +1915,7 @@ static int nf_tables_newset(struct sock *nlsk, struct sk_buff *skb,
 		goto err2;
 
 	list_add_tail(&set->list, &table->sets);
-	if (!(set->flags & NFT_SET_ANONYMOUS))
-		nf_tables_set_notify(&ctx, set, NFT_MSG_NEWSET);
+	nf_tables_set_notify(&ctx, set, NFT_MSG_NEWSET);
 	return 0;
 
 err2:
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] netfilter: nf_tables: save 48 bits per rule

[kaber]

From: Patrick McHardy <kaber@trash.net>

We currently have a hole of 48 bits in the layout of struct nft_rule
because the increased handle size. Using 48 bits for the handle should
be enough to avoid overflow and allows to plug that hole.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 include/net/netfilter/nf_tables.h | 4 ++--
 1 Datei geändert, 2 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 2301b74..a3defd8 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -303,8 +303,8 @@ static inline void *nft_expr_priv(const struct nft_expr *expr)
 struct nft_rule {
 	struct list_head		list;
 	struct rcu_head			rcu_head;
-	u64				handle;
-	u16				dlen;
+	u64				handle:48,
+					dlen:16;
 	unsigned char			data[]
 		__attribute__((aligned(__alignof__(struct nft_expr))));
 };
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] netfilter: nf_tables: destroy anonymous sets immediately if binding fails

[kaber]

From: Patrick McHardy <kaber@trash.net>

Treat a failed binding similar to binding+unbinding and destroy the
set immediately to avoid leaving stray sets in the table.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/netfilter/nf_tables_api.c | 10 +++++++++-
 1 Datei geändert, 9 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 570b877..c0f0cf06e 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1980,6 +1980,9 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
 {
 	struct nft_set_bind_check_args args;
 
+	if (!list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS)
+		return -EBUSY;
+
 	if (set->flags & NFT_SET_MAP) {
 		args.iter.skip 	= 0;
 		args.iter.count	= 0;
@@ -1988,8 +1991,13 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
 		args.ctx	= ctx;
 
 		set->ops->walk(set, &args.iter);
-		if (args.iter.err < 0)
+		if (args.iter.err < 0) {
+			/* Destroy anonymous sets if binding fails */
+			if (set->flags & NFT_SET_ANONYMOUS)
+				nf_tables_set_destroy(ctx, set);
+
 			return args.iter.err;
+		}
 	}
 
 	binding->chain = ctx->chain;
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] netfilter: nf_tables: propagate context to set iter callback

[kaber]

From: Patrick McHardy <kaber@trash.net>

Needed when adding new elements and for performing loop detection.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 include/net/netfilter/nf_tables.h |  6 ++++--
 net/netfilter/nf_tables_api.c     | 36 +++++++++++++++---------------------
 net/netfilter/nft_hash.c          |  5 +++--
 net/netfilter/nft_rbtree.c        |  6 ++++--
 4 Dateien geändert, 26 Zeilen hinzugefügt(+), 27 Zeilen entfernt(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index a3defd8..5e216de 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -122,7 +122,8 @@ struct nft_set_iter {
 	unsigned int	count;
 	unsigned int	skip;
 	int		err;
-	int		(*fn)(const struct nft_set *set,
+	int		(*fn)(const struct nft_ctx *ctx,
+			      const struct nft_set *set,
 			      const struct nft_set_iter *iter,
 			      const struct nft_set_elem *elem);
 };
@@ -151,7 +152,8 @@ struct nft_set_ops {
 						  const struct nft_set_elem *elem);
 	void				(*remove)(const struct nft_set *set,
 						  const struct nft_set_elem *elem);
-	void				(*walk)(const struct nft_set *set,
+	void				(*walk)(const struct nft_ctx *ctx,
+						const struct nft_set *set,
 						struct nft_set_iter *iter);
 
 	unsigned int			(*privsize)(const struct nlattr * const nla[]);
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index c0f0cf06e..22b14a5 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1958,45 +1958,38 @@ static int nf_tables_delset(struct sock *nlsk, struct sk_buff *skb,
 	return 0;
 }
 
-struct nft_set_bind_check_args {
-	struct nft_set_iter		iter;
-	const struct nft_ctx		*ctx;
-};
-
-static int nf_tables_bind_check_setelem(const struct nft_set *set,
+static int nf_tables_bind_check_setelem(const struct nft_ctx *ctx,
+					const struct nft_set *set,
 					const struct nft_set_iter *iter,
 					const struct nft_set_elem *elem)
 {
-	struct nft_set_bind_check_args *args;
 	enum nft_registers dreg;
 
-	args = container_of(iter, struct nft_set_bind_check_args, iter);
 	dreg = nft_type_to_reg(set->dtype);
-	return nft_validate_data_load(args->ctx, dreg, &elem->data, set->dtype);
+	return nft_validate_data_load(ctx, dreg, &elem->data, set->dtype);
 }
 
 int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
 		       struct nft_set_binding *binding)
 {
-	struct nft_set_bind_check_args args;
+	struct nft_set_iter iter;
 
 	if (!list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS)
 		return -EBUSY;
 
 	if (set->flags & NFT_SET_MAP) {
-		args.iter.skip 	= 0;
-		args.iter.count	= 0;
-		args.iter.err   = 0;
-		args.iter.fn	= nf_tables_bind_check_setelem;
-		args.ctx	= ctx;
-
-		set->ops->walk(set, &args.iter);
-		if (args.iter.err < 0) {
+		iter.skip 	= 0;
+		iter.count	= 0;
+		iter.err	= 0;
+		iter.fn		= nf_tables_bind_check_setelem;
+
+		set->ops->walk(ctx, set, &iter);
+		if (iter.err < 0) {
 			/* Destroy anonymous sets if binding fails */
 			if (set->flags & NFT_SET_ANONYMOUS)
 				nf_tables_set_destroy(ctx, set);
 
-			return args.iter.err;
+			return iter.err;
 		}
 	}
 
@@ -2091,7 +2084,8 @@ struct nft_set_dump_args {
 	struct sk_buff			*skb;
 };
 
-static int nf_tables_dump_setelem(const struct nft_set *set,
+static int nf_tables_dump_setelem(const struct nft_ctx *ctx,
+				  const struct nft_set *set,
 				  const struct nft_set_iter *iter,
 				  const struct nft_set_elem *elem)
 {
@@ -2157,7 +2151,7 @@ static int nf_tables_dump_set(struct sk_buff *skb, struct netlink_callback *cb)
 	args.iter.count	= 0;
 	args.iter.err   = 0;
 	args.iter.fn	= nf_tables_dump_setelem;
-	set->ops->walk(set, &args.iter);
+	set->ops->walk(&ctx, set, &args.iter);
 
 	nla_nest_end(skb, nest);
 	nlmsg_end(skb, nlh);
diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
index 6c6addb..e50e798 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -127,7 +127,8 @@ static int nft_hash_get(const struct nft_set *set, struct nft_set_elem *elem)
 	return -ENOENT;
 }
 
-static void nft_hash_walk(const struct nft_set *set, struct nft_set_iter *iter)
+static void nft_hash_walk(const struct nft_ctx *ctx, const struct nft_set *set,
+			  struct nft_set_iter *iter)
 {
 	const struct nft_hash *priv = nft_set_priv(set);
 	const struct nft_hash_elem *he;
@@ -145,7 +146,7 @@ static void nft_hash_walk(const struct nft_set *set, struct nft_set_iter *iter)
 				memcpy(&elem.data, he->data, sizeof(elem.data));
 			elem.flags = 0;
 
-			iter->err = iter->fn(set, iter, &elem);
+			iter->err = iter->fn(ctx, set, iter, &elem);
 			if (iter->err < 0)
 				return;
 cont:
diff --git a/net/netfilter/nft_rbtree.c b/net/netfilter/nft_rbtree.c
index c51e779..ca0c1b2 100644
--- a/net/netfilter/nft_rbtree.c
+++ b/net/netfilter/nft_rbtree.c
@@ -162,7 +162,9 @@ static int nft_rbtree_get(const struct nft_set *set, struct nft_set_elem *elem)
 	return -ENOENT;
 }
 
-static void nft_rbtree_walk(const struct nft_set *set, struct nft_set_iter *iter)
+static void nft_rbtree_walk(const struct nft_ctx *ctx,
+			    const struct nft_set *set,
+			    struct nft_set_iter *iter)
 {
 	const struct nft_rbtree *priv = nft_set_priv(set);
 	const struct nft_rbtree_elem *rbe;
@@ -179,7 +181,7 @@ static void nft_rbtree_walk(const struct nft_set *set, struct nft_set_iter *iter
 			nft_data_copy(&elem.data, rbe->data);
 		elem.flags = rbe->flags;
 
-		iter->err = iter->fn(set, iter, &elem);
+		iter->err = iter->fn(ctx, set, iter, &elem);
 		if (iter->err < 0)
 			return;
 cont:
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] netfilter: nf_tables: add loop detection

[kaber]

From: Patrick McHardy <kaber@trash.net>

Perform loop detection when adding new jump rules, new jump verdicts to
a verdict map or when binding a verdict map to a new chain.

The approach is pretty inefficient and probably can be improved by using
some caching. For now just the simple approach is used to perform loop
detection at all.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 include/net/netfilter/nf_tables.h |   1 +
 net/netfilter/nf_tables_api.c     | 113 +++++++++++++++++++++++++++++++++++---
 net/netfilter/nft_immediate.c     |  11 ++++
 3 Dateien geändert, 118 Zeilen hinzugefügt(+), 7 Zeilen entfernt(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 5e216de..99c500f 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -270,6 +270,7 @@ struct nft_expr_ops {
 	void				(*destroy)(const struct nft_expr *expr);
 	int				(*dump)(struct sk_buff *skb,
 						const struct nft_expr *expr);
+	const struct nft_data *		(*get_verdict)(const struct nft_expr *expr);
 	const struct nft_expr_type	*type;
 };
 
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 22b14a5..2253593 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1972,12 +1972,21 @@ static int nf_tables_bind_check_setelem(const struct nft_ctx *ctx,
 int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
 		       struct nft_set_binding *binding)
 {
+	struct nft_set_binding *i;
 	struct nft_set_iter iter;
 
 	if (!list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS)
 		return -EBUSY;
 
 	if (set->flags & NFT_SET_MAP) {
+		/* If the set is already bound to the same chain all
+		 * jumps are already validated for that chain.
+		 */
+		list_for_each_entry(i, &set->bindings, list) {
+			if (i->chain == binding->chain)
+				goto bind;
+		}
+
 		iter.skip 	= 0;
 		iter.count	= 0;
 		iter.err	= 0;
@@ -1992,7 +2001,7 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
 			return iter.err;
 		}
 	}
-
+bind:
 	binding->chain = ctx->chain;
 	list_add_tail(&binding->list, &set->bindings);
 	return 0;
@@ -2457,6 +2466,90 @@ static const struct nfnetlink_subsystem nf_tables_subsys = {
 	.cb		= nf_tables_cb,
 };
 
+/*
+ * Loop detection - walk through the ruleset beginning at the destination chain
+ * of a new jump until either the source chain is reached (loop) or all
+ * reachable chains have been traversed.
+ *
+ * The loop check is performed whenever a new jump verdict is added to an
+ * expression or verdict map or a verdict map is bound to a new chain.
+ */
+
+static int nf_tables_check_loops(const struct nft_ctx *ctx,
+				 const struct nft_chain *chain);
+
+static int nf_tables_loop_check_setelem(const struct nft_ctx *ctx,
+					const struct nft_set *set,
+					const struct nft_set_iter *iter,
+					const struct nft_set_elem *elem)
+{
+	switch (elem->data.verdict) {
+	case NFT_JUMP:
+	case NFT_GOTO:
+		return nf_tables_check_loops(ctx, elem->data.chain);
+	default:
+		return 0;
+	}
+}
+
+static int nf_tables_check_loops(const struct nft_ctx *ctx,
+				 const struct nft_chain *chain)
+{
+	const struct nft_rule *rule;
+	const struct nft_expr *expr, *last;
+	const struct nft_data *data;
+	const struct nft_set *set;
+	struct nft_set_binding *binding;
+	struct nft_set_iter iter;
+	int err;
+
+	if (ctx->chain == chain)
+		return -ELOOP;
+
+	list_for_each_entry(rule, &chain->rules, list) {
+		nft_rule_for_each_expr(expr, last, rule) {
+			if (!expr->ops->get_verdict)
+				continue;
+
+			data = expr->ops->get_verdict(expr);
+			if (data == NULL)
+				break;
+
+			switch (data->verdict) {
+			case NFT_JUMP:
+			case NFT_GOTO:
+				err = nf_tables_check_loops(ctx, data->chain);
+				if (err < 0)
+					return err;
+			default:
+				break;
+			}
+		}
+	}
+
+	list_for_each_entry(set, &ctx->table->sets, list) {
+		if (!(set->flags & NFT_SET_MAP) ||
+		    set->dtype != NFT_DATA_VERDICT)
+			continue;
+
+		list_for_each_entry(binding, &set->bindings, list) {
+			if (binding->chain != chain)
+				continue;
+
+			iter.skip 	= 0;
+			iter.count	= 0;
+			iter.err	= 0;
+			iter.fn		= nf_tables_loop_check_setelem;
+
+			set->ops->walk(ctx, set, &iter);
+			if (iter.err < 0)
+				return iter.err;
+		}
+	}
+
+	return 0;
+}
+
 /**
  *	nft_validate_input_register - validate an expressions' input register
  *
@@ -2510,19 +2603,25 @@ int nft_validate_data_load(const struct nft_ctx *ctx, enum nft_registers reg,
 			   const struct nft_data *data,
 			   enum nft_data_types type)
 {
+	int err;
+
 	switch (reg) {
 	case NFT_REG_VERDICT:
 		if (data == NULL || type != NFT_DATA_VERDICT)
 			return -EINVAL;
 
-		if ((data->verdict == NFT_GOTO || data->verdict == NFT_JUMP) &&
-		    ctx->chain->level + 1 > data->chain->level) {
-			if (ctx->chain->level + 1 == NFT_JUMP_STACK_SIZE)
-				return -EMLINK;
-			data->chain->level = ctx->chain->level + 1;
+		if (data->verdict == NFT_GOTO || data->verdict == NFT_JUMP) {
+			err = nf_tables_check_loops(ctx, data->chain);
+			if (err < 0)
+				return err;
+
+			if (ctx->chain->level + 1 > data->chain->level) {
+				if (ctx->chain->level + 1 == NFT_JUMP_STACK_SIZE)
+					return -EMLINK;
+				data->chain->level = ctx->chain->level + 1;
+			}
 		}
 
-		// FIXME: do loop detection
 		return 0;
 	default:
 		if (data != NULL && type != NFT_DATA_VALUE)
diff --git a/net/netfilter/nft_immediate.c b/net/netfilter/nft_immediate.c
index d1e901e..1bfeeaf 100644
--- a/net/netfilter/nft_immediate.c
+++ b/net/netfilter/nft_immediate.c
@@ -90,6 +90,16 @@ nla_put_failure:
 	return -1;
 }
 
+static const struct nft_data *nft_immediate_get_verdict(const struct nft_expr *expr)
+{
+	const struct nft_immediate_expr *priv = nft_expr_priv(expr);
+
+	if (priv->dreg == NFT_REG_VERDICT)
+		return &priv->data;
+	else
+		return NULL;
+}
+
 static struct nft_expr_type nft_imm_type;
 static const struct nft_expr_ops nft_imm_ops = {
 	.type		= &nft_imm_type,
@@ -98,6 +108,7 @@ static const struct nft_expr_ops nft_imm_ops = {
 	.init		= nft_immediate_init,
 	.destroy	= nft_immediate_destroy,
 	.dump		= nft_immediate_dump,
+	.get_verdict	= nft_immediate_get_verdict,
 };
 
 static struct nft_expr_type nft_imm_type __read_mostly = {
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] netfilter: nf_tables: nf_tables update

[Pablo Neira Ayuso]

On Mon, Dec 10, 2012 at 06:20:07PM +0100, kaber@trash.net wrote:
> The following patches for nf_tables fix a few bugs and introduct
> loop detection:
> 
> - anonymous sets are currently broken since we're not reporting the
>   chosen set name to userspace anymore
> 
> - save 48 bits per rule by reducing the handle size and plugging a whole
> 
> - destroy anonymous sets immediately when binding fails since there
>   will be no unbinding operation
> 
> - add loop detection for new jump verdicts
> 
> Please apply, thanks!

All applied, thanks a lot Patrick!