[PATCH nf 0/3] netfilter: nf_tables: fix some bugs related to dynset

[PATCH nf 0/3] netfilter: nf_tables: fix some bugs related to dynset

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

Suppose that the user input the following nft rules, then a dynset expr is
created:
  # nft add rule filter output flow table test { ip daddr counter }

But actually, there are some bugs exist in kernel:
1. If CONFIG_NFT_SET_HASH is not enabled, kernel panic will happen
2. In extreme case, i.e. memory is exhausted, then expr clone will
   fail, this will cause module refcnt leak, memory leak and incorrect
   set's nelems
3. Packets may race when create the new element, and these *racing*
   packets will not be handled properly.

This patch set is aimed to fix these problems.

Liping Zhang (3):
  netfilter: nft_dynset: fix panic if NFT_SET_HASH is not enabled
  netfilter: nf_tables: fix *leak* when expr clone fail
  netfilter: nf_tables: fix race when create new element in dynset

 include/net/netfilter/nf_tables.h |  6 ++++--
 net/netfilter/nf_tables_api.c     | 11 ++++++-----
 net/netfilter/nft_dynset.c        | 19 +++++++++++++------
 net/netfilter/nft_set_hash.c      | 19 ++++++++++++++-----
 net/netfilter/nft_set_rbtree.c    |  2 +-
 5 files changed, 38 insertions(+), 19 deletions(-)

-- 
2.5.5

[PATCH nf 1/3] netfilter: nft_dynset: fix panic if NFT_SET_HASH is not enabled

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

When CONFIG_NFT_SET_HASH is not enabled and I input the following rule:
"nft add rule filter output flow table test {ip daddr counter }", kernel
panic happened on my system:
 BUG: unable to handle kernel NULL pointer dereference at (null)
 IP: [<          (null)>]           (null)
 [...]
 Call Trace:
 [<ffffffffa0590466>] ? nft_dynset_eval+0x56/0x100 [nf_tables]
 [<ffffffffa05851bb>] nft_do_chain+0xfb/0x4e0 [nf_tables]
 [<ffffffffa0432f01>] ? nf_conntrack_tuple_taken+0x61/0x210 [nf_conntrack]
 [<ffffffffa0459ea6>] ? get_unique_tuple+0x136/0x560 [nf_nat]
 [<ffffffffa043bca1>] ? __nf_ct_ext_add_length+0x111/0x130 [nf_conntrack]
 [<ffffffffa045a357>] ? nf_nat_setup_info+0x87/0x3b0 [nf_nat]
 [<ffffffff81761e27>] ? ipt_do_table+0x327/0x610
 [<ffffffffa045a6d7>] ? __nf_nat_alloc_null_binding+0x57/0x80 [nf_nat]
 [<ffffffffa059f21f>] nft_ipv4_output+0xaf/0xd0 [nf_tables_ipv4]
 [<ffffffff81702515>] nf_iterate+0x55/0x60
 [<ffffffff81702593>] nf_hook_slow+0x73/0xd0

Because in rbtree type set, ops->update is not implemented. So just keep
it simple, in such case, report -EOPNOTSUPP to the user space.

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 net/netfilter/nft_dynset.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index e3b83c3..6a631cb 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -139,6 +139,9 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
 			return PTR_ERR(set);
 	}
 
+	if (set->ops->update == NULL)
+		return -EOPNOTSUPP;
+
 	if (set->flags & NFT_SET_CONSTANT)
 		return -EBUSY;
 
-- 
2.5.5

Re: [PATCH nf 1/3] netfilter: nft_dynset: fix panic if NFT_SET_HASH is not enabled

[Liping Zhang]

2016-10-22 18:51 GMT+08:00 Liping Zhang <zlpnobody@163.com>:
> From: Liping Zhang <zlpnobody@gmail.com>
>
> When CONFIG_NFT_SET_HASH is not enabled and I input the following rule:
> "nft add rule filter output flow table test {ip daddr counter }", kernel
> panic happened on my system:
>  BUG: unable to handle kernel NULL pointer dereference at (null)
> ---
>  net/netfilter/nft_dynset.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
> index e3b83c3..6a631cb 100644
> --- a/net/netfilter/nft_dynset.c
> +++ b/net/netfilter/nft_dynset.c
> @@ -139,6 +139,9 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
>                         return PTR_ERR(set);
>         }
>
> +       if (set->ops->update == NULL)
> +               return -EOPNOTSUPP;
> +

Maybe it's better to treat the NFT_SET_EVAL as features, I will send V2 latter:

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b70d3ea..8a39b2a 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2350,7 +2350,8 @@ nft_select_set_ops(const struct nlattr * const nla[],
        features = 0;
        if (nla[NFTA_SET_FLAGS] != NULL) {
                features = ntohl(nla_get_be32(nla[NFTA_SET_FLAGS]));
-               features &= NFT_SET_INTERVAL | NFT_SET_MAP | NFT_SET_TIMEOUT;
+               features &= NFT_SET_INTERVAL | NFT_SET_MAP | NFT_SET_TIMEOUT |
+                           NFT_SET_EVAL;
        }

        bops       = NULL;
diff --git a/net/netfilter/nft_set_hash.c b/net/netfilter/nft_set_hash.c
index 3794cb2..328d23c 100644
--- a/net/netfilter/nft_set_hash.c
+++ b/net/netfilter/nft_set_hash.c
@@ -382,7 +382,7 @@ static struct nft_set_ops nft_hash_ops __read_mostly = {
        .lookup         = nft_hash_lookup,
        .update         = nft_hash_update,
        .walk           = nft_hash_walk,
-       .features       = NFT_SET_MAP | NFT_SET_TIMEOUT,
+       .features       = NFT_SET_MAP | NFT_SET_TIMEOUT | NFT_SET_EVAL,
        .owner          = THIS_MODULE,
 };

Re: [PATCH nf 1/3] netfilter: nft_dynset: fix panic if NFT_SET_HASH is not enabled

[Liping Zhang]

2016-10-25 22:25 GMT+08:00 Liping Zhang <zlpnobody@gmail.com>:
> Maybe it's better to treat the NFT_SET_EVAL as features, I will send V2 latter:
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index b70d3ea..8a39b2a 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -2350,7 +2350,8 @@ nft_select_set_ops(const struct nlattr * const nla[],
>         features = 0;
>         if (nla[NFTA_SET_FLAGS] != NULL) {
>                 features = ntohl(nla_get_be32(nla[NFTA_SET_FLAGS]));
> -               features &= NFT_SET_INTERVAL | NFT_SET_MAP | NFT_SET_TIMEOUT;
> +               features &= NFT_SET_INTERVAL | NFT_SET_MAP | NFT_SET_TIMEOUT |
> +                           NFT_SET_EVAL;
>         }
>
>         bops       = NULL;
> diff --git a/net/netfilter/nft_set_hash.c b/net/netfilter/nft_set_hash.c
> index 3794cb2..328d23c 100644
> --- a/net/netfilter/nft_set_hash.c
> +++ b/net/netfilter/nft_set_hash.c
> @@ -382,7 +382,7 @@ static struct nft_set_ops nft_hash_ops __read_mostly = {
>         .lookup         = nft_hash_lookup,
>         .update         = nft_hash_update,
>         .walk           = nft_hash_walk,
> -       .features       = NFT_SET_MAP | NFT_SET_TIMEOUT,
> +       .features       = NFT_SET_MAP | NFT_SET_TIMEOUT | NFT_SET_EVAL,
>         .owner          = THIS_MODULE,
>  };

Sorry for this noise, the original patch should be fine. :(

After I have a careful look at the implementation of the dynset expr,
it's not appropriate to treat the NFT_SET_EVAL as the features.
The NFTA_DYNSET_EXPR attr is optional, and when it is not
specified, we will report -EINVAL if (set->flags & NFT_SET_EVAL)
is true:

static int nft_dynset_init()
{
    ...
    if (tb[NFTA_DYNSET_EXPR] != NULL) {
        if (!(set->flags & NFT_SET_EVAL))
            return -EINVAL;
        ...
    } else if (set->flags & NFT_SET_EVAL)
        return -EINVAL;

So for dynset, NFT_SET_EVAL is not a must option, but set->ops->update is.

[PATCH nf 2/3] netfilter: nf_tables: fix *leak* when expr clone fail

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

When nft_expr_clone failed, a series of problems will happen:

1. module refcnt will leak, we call __module_get at the beginning but
   we forget to put it back if ops->clone returns fail
2. memory will be leaked, if clone fail, we just return NULL and forget
   to free the alloced element
3. set->nelems will become incorrect when set->size is specified. If
   clone fail, we should decrease the set->nelems

Now this patch fixes these problems. And fortunately, clone fail will
only happen on counter expression when memory is exhausted.

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 include/net/netfilter/nf_tables.h |  6 ++++--
 net/netfilter/nf_tables_api.c     | 11 ++++++-----
 net/netfilter/nft_dynset.c        | 16 ++++++++++------
 net/netfilter/nft_set_hash.c      |  4 ++--
 net/netfilter/nft_set_rbtree.c    |  2 +-
 5 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 5031e07..741dcde 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -542,7 +542,8 @@ void *nft_set_elem_init(const struct nft_set *set,
 			const struct nft_set_ext_tmpl *tmpl,
 			const u32 *key, const u32 *data,
 			u64 timeout, gfp_t gfp);
-void nft_set_elem_destroy(const struct nft_set *set, void *elem);
+void nft_set_elem_destroy(const struct nft_set *set, void *elem,
+			  bool destroy_expr);
 
 /**
  *	struct nft_set_gc_batch_head - nf_tables set garbage collection batch
@@ -693,7 +694,6 @@ static inline int nft_expr_clone(struct nft_expr *dst, struct nft_expr *src)
 {
 	int err;
 
-	__module_get(src->ops->type->owner);
 	if (src->ops->clone) {
 		dst->ops = src->ops;
 		err = src->ops->clone(dst, src);
@@ -702,6 +702,8 @@ static inline int nft_expr_clone(struct nft_expr *dst, struct nft_expr *src)
 	} else {
 		memcpy(dst, src, src->ops->size);
 	}
+
+	__module_get(src->ops->type->owner);
 	return 0;
 }
 
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b70d3ea..9de155a 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3452,14 +3452,15 @@ void *nft_set_elem_init(const struct nft_set *set,
 	return elem;
 }
 
-void nft_set_elem_destroy(const struct nft_set *set, void *elem)
+void nft_set_elem_destroy(const struct nft_set *set, void *elem,
+			  bool destroy_expr)
 {
 	struct nft_set_ext *ext = nft_set_elem_ext(set, elem);
 
 	nft_data_uninit(nft_set_ext_key(ext), NFT_DATA_VALUE);
 	if (nft_set_ext_exists(ext, NFT_SET_EXT_DATA))
 		nft_data_uninit(nft_set_ext_data(ext), set->dtype);
-	if (nft_set_ext_exists(ext, NFT_SET_EXT_EXPR))
+	if (destroy_expr && nft_set_ext_exists(ext, NFT_SET_EXT_EXPR))
 		nf_tables_expr_destroy(NULL, nft_set_ext_expr(ext));
 
 	kfree(elem);
@@ -3812,7 +3813,7 @@ void nft_set_gc_batch_release(struct rcu_head *rcu)
 
 	gcb = container_of(rcu, struct nft_set_gc_batch, head.rcu);
 	for (i = 0; i < gcb->head.cnt; i++)
-		nft_set_elem_destroy(gcb->head.set, gcb->elems[i]);
+		nft_set_elem_destroy(gcb->head.set, gcb->elems[i], true);
 	kfree(gcb);
 }
 EXPORT_SYMBOL_GPL(nft_set_gc_batch_release);
@@ -4030,7 +4031,7 @@ static void nf_tables_commit_release(struct nft_trans *trans)
 		break;
 	case NFT_MSG_DELSETELEM:
 		nft_set_elem_destroy(nft_trans_elem_set(trans),
-				     nft_trans_elem(trans).priv);
+				     nft_trans_elem(trans).priv, true);
 		break;
 	}
 	kfree(trans);
@@ -4171,7 +4172,7 @@ static void nf_tables_abort_release(struct nft_trans *trans)
 		break;
 	case NFT_MSG_NEWSETELEM:
 		nft_set_elem_destroy(nft_trans_elem_set(trans),
-				     nft_trans_elem(trans).priv);
+				     nft_trans_elem(trans).priv, true);
 		break;
 	}
 	kfree(trans);
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index 6a631cb..a918482 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -44,18 +44,22 @@ static void *nft_dynset_new(struct nft_set *set, const struct nft_expr *expr,
 				 &regs->data[priv->sreg_key],
 				 &regs->data[priv->sreg_data],
 				 timeout, GFP_ATOMIC);
-	if (elem == NULL) {
-		if (set->size)
-			atomic_dec(&set->nelems);
-		return NULL;
-	}
+	if (elem == NULL)
+		goto err1;
 
 	ext = nft_set_elem_ext(set, elem);
 	if (priv->expr != NULL &&
 	    nft_expr_clone(nft_set_ext_expr(ext), priv->expr) < 0)
-		return NULL;
+		goto err2;
 
 	return elem;
+
+err2:
+	nft_set_elem_destroy(set, elem, false);
+err1:
+	if (set->size)
+		atomic_dec(&set->nelems);
+	return NULL;
 }
 
 static void nft_dynset_eval(const struct nft_expr *expr,
diff --git a/net/netfilter/nft_set_hash.c b/net/netfilter/nft_set_hash.c
index 3794cb2..88d9fc8 100644
--- a/net/netfilter/nft_set_hash.c
+++ b/net/netfilter/nft_set_hash.c
@@ -120,7 +120,7 @@ static bool nft_hash_update(struct nft_set *set, const u32 *key,
 	return true;
 
 err2:
-	nft_set_elem_destroy(set, he);
+	nft_set_elem_destroy(set, he, true);
 err1:
 	return false;
 }
@@ -332,7 +332,7 @@ static int nft_hash_init(const struct nft_set *set,
 
 static void nft_hash_elem_destroy(void *ptr, void *arg)
 {
-	nft_set_elem_destroy((const struct nft_set *)arg, ptr);
+	nft_set_elem_destroy((const struct nft_set *)arg, ptr, true);
 }
 
 static void nft_hash_destroy(const struct nft_set *set)
diff --git a/net/netfilter/nft_set_rbtree.c b/net/netfilter/nft_set_rbtree.c
index 38b5bda..36493a7 100644
--- a/net/netfilter/nft_set_rbtree.c
+++ b/net/netfilter/nft_set_rbtree.c
@@ -266,7 +266,7 @@ static void nft_rbtree_destroy(const struct nft_set *set)
 	while ((node = priv->root.rb_node) != NULL) {
 		rb_erase(node, &priv->root);
 		rbe = rb_entry(node, struct nft_rbtree_elem, node);
-		nft_set_elem_destroy(set, rbe);
+		nft_set_elem_destroy(set, rbe, true);
 	}
 }
 
-- 
2.5.5

[PATCH nf 3/3] netfilter: nf_tables: fix race when create new element in dynset

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

Packets may race when create the new element in nft_hash_update:
       CPU0                 CPU1
  lookup_fast - fail     lookup_fast - fail
       new - ok             new - ok
     insert - ok         insert - fail(EEXIST)

So when race happened, we reuse the existing element. Otherwise,
these *racing* packets will not be handled properly.

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 net/netfilter/nft_set_hash.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nft_set_hash.c b/net/netfilter/nft_set_hash.c
index 88d9fc8..a3dface 100644
--- a/net/netfilter/nft_set_hash.c
+++ b/net/netfilter/nft_set_hash.c
@@ -98,7 +98,7 @@ static bool nft_hash_update(struct nft_set *set, const u32 *key,
 			    const struct nft_set_ext **ext)
 {
 	struct nft_hash *priv = nft_set_priv(set);
-	struct nft_hash_elem *he;
+	struct nft_hash_elem *he, *prev;
 	struct nft_hash_cmp_arg arg = {
 		.genmask = NFT_GENMASK_ANY,
 		.set	 = set,
@@ -112,9 +112,18 @@ static bool nft_hash_update(struct nft_set *set, const u32 *key,
 	he = new(set, expr, regs);
 	if (he == NULL)
 		goto err1;
-	if (rhashtable_lookup_insert_key(&priv->ht, &arg, &he->node,
-					 nft_hash_params))
+
+	prev = rhashtable_lookup_get_insert_key(&priv->ht, &arg, &he->node,
+						nft_hash_params);
+	if (IS_ERR(prev))
 		goto err2;
+
+	/* Another cpu may race to insert the element with the same key */
+	if (prev) {
+		nft_set_elem_destroy(set, he, true);
+		he = prev;
+	}
+
 out:
 	*ext = &he->ext;
 	return true;
-- 
2.5.5

Re: [PATCH nf 0/3] netfilter: nf_tables: fix some bugs related to dynset

[Pablo Neira Ayuso]

On Sat, Oct 22, 2016 at 06:51:23PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> Suppose that the user input the following nft rules, then a dynset expr is
> created:
>   # nft add rule filter output flow table test { ip daddr counter }
> 
> But actually, there are some bugs exist in kernel:
> 1. If CONFIG_NFT_SET_HASH is not enabled, kernel panic will happen
> 2. In extreme case, i.e. memory is exhausted, then expr clone will
>    fail, this will cause module refcnt leak, memory leak and incorrect
>    set's nelems
> 3. Packets may race when create the new element, and these *racing*
>    packets will not be handled properly.
> 
> This patch set is aimed to fix these problems.

Series applied, thanks!