[PATCH nf-next 0/3] netfilter: fix a endless jump loop bug

[PATCH nf-next 0/3] netfilter: fix a endless jump loop bug

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

This patch set mainly fix a endless jump loop bug, for example, user
can add the following nft rules successfully:
  # nft add table filter
  # nft add chain filter test
  # nft add rule filter test tcp dport vmap {1: jump test}

This is because we skip the inactive elements in set, and miss the validate
check. Fix it in patch #2.

And after apply patch#2, I also find that there is a redundant 
nf_tables_set_destroy call when set bind fails, which cause my
mechain enter into deadlock. Fix it in patch #3.

Also fix a typo in patch #1.

Liping Zhang (3):
  netfilter: nf_tables: fix wrong check of NFT_SET_MAP in
    nf_tables_bind_set
  netfilter: nf_tables: fix a endless jump loop when use vmap
  netfilter: nf_tables: fix wrong destroy anonymous sets if binding
    fails

 include/net/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c     | 40 +++++++++++++++++++--------------------
 net/netfilter/nft_hash.c          |  3 ++-
 net/netfilter/nft_rbtree.c        |  3 ++-
 4 files changed, 24 insertions(+), 23 deletions(-)

-- 
2.5.5

[PATCH nf-next 1/3] netfilter: nf_tables: fix wrong check of NFT_SET_MAP in nf_tables_bind_set

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

We should check "i" is used as a dictionary or not, "binding" is already
checked before.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/nf_tables_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 4d292b9..e4a6d7e 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2944,7 +2944,7 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
 		 * jumps are already validated for that chain.
 		 */
 		list_for_each_entry(i, &set->bindings, list) {
-			if (binding->flags & NFT_SET_MAP &&
+			if (i->flags & NFT_SET_MAP &&
 			    i->chain == binding->chain)
 				goto bind;
 		}
-- 
2.5.5

Re: [PATCH nf-next 1/3] netfilter: nf_tables: fix wrong check of NFT_SET_MAP in nf_tables_bind_set

[Pablo Neira Ayuso]

On Sat, Jun 11, 2016 at 12:20:26PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> We should check "i" is used as a dictionary or not, "binding" is already
> checked before.

I've applied this to nf, I qualify this as a fix, thanks.

[PATCH nf-next 2/3] netfilter: nf_tables: fix a endless jump loop when use vmap

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

Currently, user can add such a wrong nft rules successfully, which
will cause an endless jump loop:
  # nft add rule filter test tcp dport vmap {1: jump test}

This is because before we commit, the element in the current anonymous
set is inactive, so osp->walk will skip this element and miss the
validate check.

So we should also check the inactive element when we do loops check,
and ignore the inactive element when we do dump.

This patch also fix the testcase fail in
nftables/tests/shell/testcases/chains/0009masquerade_jump_1.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 include/net/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c     | 31 +++++++++++++++++--------------
 net/netfilter/nft_hash.c          |  3 ++-
 net/netfilter/nft_rbtree.c        |  3 ++-
 4 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 0922354..e35194a 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -167,6 +167,7 @@ struct nft_set_elem {
 
 struct nft_set;
 struct nft_set_iter {
+	bool		ignore_inactive;
 	unsigned int	count;
 	unsigned int	skip;
 	int		err;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index e4a6d7e..d5f3602 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2949,10 +2949,11 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
 				goto bind;
 		}
 
-		iter.skip 	= 0;
-		iter.count	= 0;
-		iter.err	= 0;
-		iter.fn		= nf_tables_bind_check_setelem;
+		iter.ignore_inactive	= false;
+		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) {
@@ -3190,12 +3191,13 @@ static int nf_tables_dump_set(struct sk_buff *skb, struct netlink_callback *cb)
 	if (nest == NULL)
 		goto nla_put_failure;
 
-	args.cb		= cb;
-	args.skb	= skb;
-	args.iter.skip	= cb->args[0];
-	args.iter.count	= 0;
-	args.iter.err   = 0;
-	args.iter.fn	= nf_tables_dump_setelem;
+	args.cb				= cb;
+	args.skb			= skb;
+	args.iter.ignore_inactive	= true;
+	args.iter.skip			= cb->args[0];
+	args.iter.count			= 0;
+	args.iter.err			= 0;
+	args.iter.fn			= nf_tables_dump_setelem;
 	set->ops->walk(&ctx, set, &args.iter);
 
 	nla_nest_end(skb, nest);
@@ -4282,10 +4284,11 @@ static int nf_tables_check_loops(const struct nft_ctx *ctx,
 			    binding->chain != chain)
 				continue;
 
-			iter.skip 	= 0;
-			iter.count	= 0;
-			iter.err	= 0;
-			iter.fn		= nf_tables_loop_check_setelem;
+			iter.ignore_inactive	= false;
+			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)
diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
index 6fa0165..7337580 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -218,7 +218,8 @@ static void nft_hash_walk(const struct nft_ctx *ctx, const struct nft_set *set,
 			goto cont;
 		if (nft_set_elem_expired(&he->ext))
 			goto cont;
-		if (!nft_set_elem_active(&he->ext, genmask))
+		if (iter->ignore_inactive &&
+		    !nft_set_elem_active(&he->ext, genmask))
 			goto cont;
 
 		elem.priv = he;
diff --git a/net/netfilter/nft_rbtree.c b/net/netfilter/nft_rbtree.c
index f762094..b948b72 100644
--- a/net/netfilter/nft_rbtree.c
+++ b/net/netfilter/nft_rbtree.c
@@ -219,7 +219,8 @@ static void nft_rbtree_walk(const struct nft_ctx *ctx,
 
 		if (iter->count < iter->skip)
 			goto cont;
-		if (!nft_set_elem_active(&rbe->ext, genmask))
+		if (iter->ignore_inactive &&
+		    !nft_set_elem_active(&rbe->ext, genmask))
 			goto cont;
 
 		elem.priv = rbe;
-- 
2.5.5

Re: [PATCH nf-next 2/3] netfilter: nf_tables: fix a endless jump loop when use vmap

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

On Sat, Jun 11, 2016 at 12:20:27PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> Currently, user can add such a wrong nft rules successfully, which
> will cause an endless jump loop:
>   # nft add rule filter test tcp dport vmap {1: jump test}
> 
> This is because before we commit, the element in the current anonymous
> set is inactive, so osp->walk will skip this element and miss the
> validate check.
> 
> So we should also check the inactive element when we do loops check,
> and ignore the inactive element when we do dump.
> 
> This patch also fix the testcase fail in
> nftables/tests/shell/testcases/chains/0009masquerade_jump_1.

Thanks for tracking down and fixing this one.

I've made a new version based on your original patch, find it
attached.

Basically, the idea is to pass the genmask through iter, so we can
perform the conditional check based on whether 1) we're dumping or
2) checking for loops (ie. in the middle of a transaction).

Let me know if you see any problem with this different approach,
thanks.

[-- Attachment #2: 0001-netfilter-nf_tables-fix-a-endless-jump-loop-when-use.patch --]
[-- Type: text/x-diff, Size: 4547 bytes --]

>From 0e1a07d8a5315d06531beb311a30464d1b207023 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Sat, 11 Jun 2016 12:20:27 +0800
Subject: [PATCH] netfilter: nf_tables: fix a endless jump loop when use vmap

Liping Zhang says:

"Users may add such a wrong nft rules successfully, which will cause an
endless jump loop:

  # nft add rule filter test tcp dport vmap {1: jump test}

This is because before we commit, the element in the current anonymous
set is inactive, so osp->walk will skip this element and miss the
validate check."

This patch passes the generation mask to the walk function through the
iter container structure depending on the code path:

1) If we're dumping the elements, then we have to check if the element
   is active in the current generation. Thus, we check for the current
   bit in the genmask.

2) If we're checking for loops, then we have to check if the element is
   active in the next generation, as we're in the middle of a
   transaction. Thus, we check for the next bit in the genmask.

Reported-by: Liping Zhang <liping.zhang@spreadtrum.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c     | 15 +++++++++------
 net/netfilter/nft_hash.c          |  3 +--
 net/netfilter/nft_rbtree.c        |  3 +--
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 7b3d97c..065fcf3 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -167,6 +167,7 @@ struct nft_set_elem {
 
 struct nft_set;
 struct nft_set_iter {
+	u8		genmask;
 	unsigned int	count;
 	unsigned int	skip;
 	int		err;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 44de9b9..ccce337 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2976,6 +2976,7 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
 				goto bind;
 		}
 
+		iter.genmask	= nft_genmask_next(read_pnet(&set->pnet));
 		iter.skip 	= 0;
 		iter.count	= 0;
 		iter.err	= 0;
@@ -3217,12 +3218,13 @@ static int nf_tables_dump_set(struct sk_buff *skb, struct netlink_callback *cb)
 	if (nest == NULL)
 		goto nla_put_failure;
 
-	args.cb		= cb;
-	args.skb	= skb;
-	args.iter.skip	= cb->args[0];
-	args.iter.count	= 0;
-	args.iter.err   = 0;
-	args.iter.fn	= nf_tables_dump_setelem;
+	args.cb			= cb;
+	args.skb		= skb;
+	args.iter.genmask	= nft_genmask_cur(read_pnet(&set->pnet));
+	args.iter.skip		= cb->args[0];
+	args.iter.count		= 0;
+	args.iter.err		= 0;
+	args.iter.fn		= nf_tables_dump_setelem;
 	set->ops->walk(&ctx, set, &args.iter);
 
 	nla_nest_end(skb, nest);
@@ -4321,6 +4323,7 @@ static int nf_tables_check_loops(const struct nft_ctx *ctx,
 			    binding->chain != chain)
 				continue;
 
+			iter.genmask	= nft_genmask_next(read_pnet(&set->pnet));
 			iter.skip 	= 0;
 			iter.count	= 0;
 			iter.err	= 0;
diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
index 6fa0165..f39c53a 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -189,7 +189,6 @@ static void nft_hash_walk(const struct nft_ctx *ctx, const struct nft_set *set,
 	struct nft_hash_elem *he;
 	struct rhashtable_iter hti;
 	struct nft_set_elem elem;
-	u8 genmask = nft_genmask_cur(read_pnet(&set->pnet));
 	int err;
 
 	err = rhashtable_walk_init(&priv->ht, &hti, GFP_KERNEL);
@@ -218,7 +217,7 @@ static void nft_hash_walk(const struct nft_ctx *ctx, const struct nft_set *set,
 			goto cont;
 		if (nft_set_elem_expired(&he->ext))
 			goto cont;
-		if (!nft_set_elem_active(&he->ext, genmask))
+		if (!nft_set_elem_active(&he->ext, iter->genmask))
 			goto cont;
 
 		elem.priv = he;
diff --git a/net/netfilter/nft_rbtree.c b/net/netfilter/nft_rbtree.c
index f762094..7201d57 100644
--- a/net/netfilter/nft_rbtree.c
+++ b/net/netfilter/nft_rbtree.c
@@ -211,7 +211,6 @@ static void nft_rbtree_walk(const struct nft_ctx *ctx,
 	struct nft_rbtree_elem *rbe;
 	struct nft_set_elem elem;
 	struct rb_node *node;
-	u8 genmask = nft_genmask_cur(read_pnet(&set->pnet));
 
 	spin_lock_bh(&nft_rbtree_lock);
 	for (node = rb_first(&priv->root); node != NULL; node = rb_next(node)) {
@@ -219,7 +218,7 @@ static void nft_rbtree_walk(const struct nft_ctx *ctx,
 
 		if (iter->count < iter->skip)
 			goto cont;
-		if (!nft_set_elem_active(&rbe->ext, genmask))
+		if (!nft_set_elem_active(&rbe->ext, iter->genmask))
 			goto cont;
 
 		elem.priv = rbe;
-- 
2.1.4

Re:Re: [PATCH nf-next 2/3] netfilter: nf_tables: fix a endless jump loop when use vmap

[Liping Zhang]

Hi pablo,

At 2016-06-14 02:19:02, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:
>On Sat, Jun 11, 2016 at 12:20:27PM +0800, Liping Zhang wrote:
>
>Thanks for tracking down and fixing this one.
>
>I've made a new version based on your original patch, find it
>attached.
>
>Basically, the idea is to pass the genmask through iter, so we can
>perform the conditional check based on whether 1) we're dumping or
>2) checking for loops (ie. in the middle of a transaction).
>
>Let me know if you see any problem with this different approach,
>thanks.

I think your patch is better than mine. I also test it, it works well.

Tested-by: Liping Zhang <liping.zhang@spreadtrum.com>

Thanks

Re: Re: [PATCH nf-next 2/3] netfilter: nf_tables: fix a endless jump loop when use vmap

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

On Tue, Jun 14, 2016 at 08:07:41PM +0800, Liping Zhang wrote:
> Hi pablo,
> 
> At 2016-06-14 02:19:02, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:
> >On Sat, Jun 11, 2016 at 12:20:27PM +0800, Liping Zhang wrote:
> >
> >Thanks for tracking down and fixing this one.
> >
> >I've made a new version based on your original patch, find it
> >attached.
> >
> >Basically, the idea is to pass the genmask through iter, so we can
> >perform the conditional check based on whether 1) we're dumping or
> >2) checking for loops (ie. in the middle of a transaction).
> >
> >Let me know if you see any problem with this different approach,
> >thanks.
> 
> I think your patch is better than mine. I also test it, it works well.
> 
> Tested-by: Liping Zhang <liping.zhang@spreadtrum.com>

Thanks, I'm attaching a v2. I noticed I can simplify the previous
patch by using ctx->net. Will push this (and your patches) upstream
asap.

[-- Attachment #2: 0001-netfilter-nf_tables-reject-loops-from-set-element-ju.patch --]
[-- Type: text/x-diff, Size: 4712 bytes --]

>From e067bde1535ca78d9c8fea9f49f86c0731274732 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Sat, 11 Jun 2016 12:20:27 +0800
Subject: [PATCH] netfilter: nf_tables: reject loops from set element jump to
 chain

Liping Zhang says:

"Users may add such a wrong nft rules successfully, which will cause an
endless jump loop:

  # nft add rule filter test tcp dport vmap {1: jump test}

This is because before we commit, the element in the current anonymous
set is inactive, so osp->walk will skip this element and miss the
validate check."

To resolve this problem, this patch passes the generation mask to the
walk function through the iter container structure depending on the code
path:

1) If we're dumping the elements, then we have to check if the element
   is active in the current generation. Thus, we check for the current
   bit in the genmask.

2) If we're checking for loops, then we have to check if the element is
   active in the next generation, as we're in the middle of a
   transaction. Thus, we check for the next bit in the genmask.

Based on original patch from Liping Zhang.

Reported-by: Liping Zhang <liping.zhang@spreadtrum.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Tested-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
v2: Simplify previous patch through using ctx->net instead of set_pnet().

 include/net/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c     | 15 +++++++++------
 net/netfilter/nft_hash.c          |  3 +--
 net/netfilter/nft_rbtree.c        |  3 +--
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 0922354..f7c291f 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -167,6 +167,7 @@ struct nft_set_elem {
 
 struct nft_set;
 struct nft_set_iter {
+	u8		genmask;
 	unsigned int	count;
 	unsigned int	skip;
 	int		err;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 492f6f8..0fd6998 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2951,6 +2951,7 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
 				goto bind;
 		}
 
+		iter.genmask	= nft_genmask_next(ctx->net);
 		iter.skip 	= 0;
 		iter.count	= 0;
 		iter.err	= 0;
@@ -3192,12 +3193,13 @@ static int nf_tables_dump_set(struct sk_buff *skb, struct netlink_callback *cb)
 	if (nest == NULL)
 		goto nla_put_failure;
 
-	args.cb		= cb;
-	args.skb	= skb;
-	args.iter.skip	= cb->args[0];
-	args.iter.count	= 0;
-	args.iter.err   = 0;
-	args.iter.fn	= nf_tables_dump_setelem;
+	args.cb			= cb;
+	args.skb		= skb;
+	args.iter.genmask	= nft_genmask_cur(ctx.net);
+	args.iter.skip		= cb->args[0];
+	args.iter.count		= 0;
+	args.iter.err		= 0;
+	args.iter.fn		= nf_tables_dump_setelem;
 	set->ops->walk(&ctx, set, &args.iter);
 
 	nla_nest_end(skb, nest);
@@ -4284,6 +4286,7 @@ static int nf_tables_check_loops(const struct nft_ctx *ctx,
 			    binding->chain != chain)
 				continue;
 
+			iter.genmask	= nft_genmask_next(ctx->net);
 			iter.skip 	= 0;
 			iter.count	= 0;
 			iter.err	= 0;
diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
index 6fa0165..f39c53a 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -189,7 +189,6 @@ static void nft_hash_walk(const struct nft_ctx *ctx, const struct nft_set *set,
 	struct nft_hash_elem *he;
 	struct rhashtable_iter hti;
 	struct nft_set_elem elem;
-	u8 genmask = nft_genmask_cur(read_pnet(&set->pnet));
 	int err;
 
 	err = rhashtable_walk_init(&priv->ht, &hti, GFP_KERNEL);
@@ -218,7 +217,7 @@ static void nft_hash_walk(const struct nft_ctx *ctx, const struct nft_set *set,
 			goto cont;
 		if (nft_set_elem_expired(&he->ext))
 			goto cont;
-		if (!nft_set_elem_active(&he->ext, genmask))
+		if (!nft_set_elem_active(&he->ext, iter->genmask))
 			goto cont;
 
 		elem.priv = he;
diff --git a/net/netfilter/nft_rbtree.c b/net/netfilter/nft_rbtree.c
index f762094..7201d57 100644
--- a/net/netfilter/nft_rbtree.c
+++ b/net/netfilter/nft_rbtree.c
@@ -211,7 +211,6 @@ static void nft_rbtree_walk(const struct nft_ctx *ctx,
 	struct nft_rbtree_elem *rbe;
 	struct nft_set_elem elem;
 	struct rb_node *node;
-	u8 genmask = nft_genmask_cur(read_pnet(&set->pnet));
 
 	spin_lock_bh(&nft_rbtree_lock);
 	for (node = rb_first(&priv->root); node != NULL; node = rb_next(node)) {
@@ -219,7 +218,7 @@ static void nft_rbtree_walk(const struct nft_ctx *ctx,
 
 		if (iter->count < iter->skip)
 			goto cont;
-		if (!nft_set_elem_active(&rbe->ext, genmask))
+		if (!nft_set_elem_active(&rbe->ext, iter->genmask))
 			goto cont;
 
 		elem.priv = rbe;
-- 
2.1.4

Re: Re: [PATCH nf-next 2/3] netfilter: nf_tables: fix a endless jump loop when use vmap

[Pablo Neira Ayuso]

On Tue, Jun 14, 2016 at 05:38:51PM +0200, Pablo Neira Ayuso wrote:
> From e067bde1535ca78d9c8fea9f49f86c0731274732 Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Sat, 11 Jun 2016 12:20:27 +0800
> Subject: [PATCH] netfilter: nf_tables: reject loops from set element jump to
>  chain
> 
> Liping Zhang says:
> 
> "Users may add such a wrong nft rules successfully, which will cause an
> endless jump loop:
> 
>   # nft add rule filter test tcp dport vmap {1: jump test}
> 
> This is because before we commit, the element in the current anonymous
> set is inactive, so osp->walk will skip this element and miss the
> validate check."
> 
> To resolve this problem, this patch passes the generation mask to the
> walk function through the iter container structure depending on the code
> path:
> 
> 1) If we're dumping the elements, then we have to check if the element
>    is active in the current generation. Thus, we check for the current
>    bit in the genmask.
> 
> 2) If we're checking for loops, then we have to check if the element is
>    active in the next generation, as we're in the middle of a
>    transaction. Thus, we check for the next bit in the genmask.
> 
> Based on original patch from Liping Zhang.
> 
> Reported-by: Liping Zhang <liping.zhang@spreadtrum.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Tested-by: Liping Zhang <liping.zhang@spreadtrum.com>
> ---
> v2: Simplify previous patch through using ctx->net instead of set_pnet().

I have applied this to nf.

[PATCH nf-next 3/3] netfilter: nf_tables: fix wrong destroy anonymous sets if binding fails

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

When we add a nft rule like follows:
  # nft add rule filter test tcp dport vmap {1: jump test}
-ELOOP error will be returned, and the anonymous set will be
destroyed.

But after that, nf_tables_abort will also try to remove the
element and destroy the set, which was already destroyed and
freed.

If we add a nft wrong rule, nft_tables_abort will do the cleanup
work rightly, so nf_tables_set_destroy call here is redundant and
wrong, remove it.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/nf_tables_api.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index d5f3602..ba2dad4 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2956,13 +2956,8 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
 		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);
-
+		if (iter.err < 0)
 			return iter.err;
-		}
 	}
 bind:
 	binding->chain = ctx->chain;
-- 
2.5.5

Re: [PATCH nf-next 3/3] netfilter: nf_tables: fix wrong destroy anonymous sets if binding fails

[Pablo Neira Ayuso]

On Sat, Jun 11, 2016 at 12:20:28PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> When we add a nft rule like follows:
>   # nft add rule filter test tcp dport vmap {1: jump test}
> -ELOOP error will be returned, and the anonymous set will be
> destroyed.
> 
> But after that, nf_tables_abort will also try to remove the
> element and destroy the set, which was already destroyed and
> freed.
> 
> If we add a nft wrong rule, nft_tables_abort will do the cleanup
> work rightly, so nf_tables_set_destroy call here is redundant and
> wrong, remove it.

Also applied, thanks.