[Patch nf v2 0/3] netfilter: xt_hashlimit: a few improvements

[Patch nf v2 0/3] netfilter: xt_hashlimit: a few improvements

[Cong Wang]

This patchset improves vmalloc allocation handling and
hashlimit_mutex usage for xt_hashlimit.

---
v2: address review comments

Cong Wang (3):
  xt_hashlimit: avoid OOM for user-controlled vmalloc
  xt_hashlimit: reduce hashlimit_mutex scope for htable_put()
  xt_hashlimit: limit the max size of hashtable

 net/netfilter/xt_hashlimit.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

-- 
2.21.1

[Patch nf v2 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc

[Cong Wang]

The hashtable size could be controlled by user, so use flags
GFP_USER | __GFP_NOWARN to avoid OOM warning triggered by user-space.

Also add __GFP_NORETRY to avoid retrying, as this is just a
best effort and the failure is already handled gracefully.

Reported-and-tested-by: syzbot+adf6c6c2be1c3a718121@syzkaller.appspotmail.com
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
Cc: Florian Westphal <fw@strlen.de>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/netfilter/xt_hashlimit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index bccd47cd7190..5d9943b37c42 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -293,8 +293,8 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
 		if (size < 16)
 			size = 16;
 	}
-	/* FIXME: don't use vmalloc() here or anywhere else -HW */
-	hinfo = vmalloc(struct_size(hinfo, hash, size));
+	hinfo = __vmalloc(struct_size(hinfo, hash, size),
+			  GFP_USER | __GFP_NOWARN | __GFP_NORETRY, PAGE_KERNEL);
 	if (hinfo == NULL)
 		return -ENOMEM;
 	*out_hinfo = hinfo;
-- 
2.21.1

[Patch nf v2 2/3] xt_hashlimit: reduce hashlimit_mutex scope for htable_put()

[Cong Wang]

It is unnecessary to hold hashlimit_mutex for htable_destroy()
as it is already removed from the global hashtable and its
refcount is already zero.

Also, switch hinfo->use to refcount_t so that we don't have
to hold the mutex until it reaches zero in htable_put().

Reported-and-tested-by: syzbot+adf6c6c2be1c3a718121@syzkaller.appspotmail.com
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
Acked-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/netfilter/xt_hashlimit.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 5d9943b37c42..06e59f9d9f62 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -36,6 +36,7 @@
 #include <linux/netfilter_ipv6/ip6_tables.h>
 #include <linux/mutex.h>
 #include <linux/kernel.h>
+#include <linux/refcount.h>
 #include <uapi/linux/netfilter/xt_hashlimit.h>
 
 #define XT_HASHLIMIT_ALL (XT_HASHLIMIT_HASH_DIP | XT_HASHLIMIT_HASH_DPT | \
@@ -114,7 +115,7 @@ struct dsthash_ent {
 
 struct xt_hashlimit_htable {
 	struct hlist_node node;		/* global list of all htables */
-	int use;
+	refcount_t use;
 	u_int8_t family;
 	bool rnd_initialized;
 
@@ -315,7 +316,7 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
 	for (i = 0; i < hinfo->cfg.size; i++)
 		INIT_HLIST_HEAD(&hinfo->hash[i]);
 
-	hinfo->use = 1;
+	refcount_set(&hinfo->use, 1);
 	hinfo->count = 0;
 	hinfo->family = family;
 	hinfo->rnd_initialized = false;
@@ -420,7 +421,7 @@ static struct xt_hashlimit_htable *htable_find_get(struct net *net,
 	hlist_for_each_entry(hinfo, &hashlimit_net->htables, node) {
 		if (!strcmp(name, hinfo->name) &&
 		    hinfo->family == family) {
-			hinfo->use++;
+			refcount_inc(&hinfo->use);
 			return hinfo;
 		}
 	}
@@ -429,12 +430,11 @@ static struct xt_hashlimit_htable *htable_find_get(struct net *net,
 
 static void htable_put(struct xt_hashlimit_htable *hinfo)
 {
-	mutex_lock(&hashlimit_mutex);
-	if (--hinfo->use == 0) {
+	if (refcount_dec_and_mutex_lock(&hinfo->use, &hashlimit_mutex)) {
 		hlist_del(&hinfo->node);
+		mutex_unlock(&hashlimit_mutex);
 		htable_destroy(hinfo);
 	}
-	mutex_unlock(&hashlimit_mutex);
 }
 
 /* The algorithm used is the Simple Token Bucket Filter (TBF)
-- 
2.21.1

[Patch nf v2 3/3] xt_hashlimit: limit the max size of hashtable

[Cong Wang]

The user-specified hashtable size is unbound, this could
easily lead to an OOM or a hung task as we hold the global
mutex while allocating and initializing the new hashtable.

Add a max value to cap both cfg->size and cfg->max, as
suggested by Florian.

Reported-and-tested-by: syzbot+adf6c6c2be1c3a718121@syzkaller.appspotmail.com
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
Cc: Florian Westphal <fw@strlen.de>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/netfilter/xt_hashlimit.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 06e59f9d9f62..60c023630d14 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -837,6 +837,8 @@ hashlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	return hashlimit_mt_common(skb, par, hinfo, &info->cfg, 3);
 }
 
+#define HASHLIMIT_MAX_SIZE 1048576
+
 static int hashlimit_mt_check_common(const struct xt_mtchk_param *par,
 				     struct xt_hashlimit_htable **hinfo,
 				     struct hashlimit_cfg3 *cfg,
@@ -847,6 +849,14 @@ static int hashlimit_mt_check_common(const struct xt_mtchk_param *par,
 
 	if (cfg->gc_interval == 0 || cfg->expire == 0)
 		return -EINVAL;
+	if (cfg->size > HASHLIMIT_MAX_SIZE) {
+		cfg->size = HASHLIMIT_MAX_SIZE;
+		pr_info_ratelimited("size too large, truncated to %u\n", cfg->size);
+	}
+	if (cfg->max > HASHLIMIT_MAX_SIZE) {
+		cfg->max = HASHLIMIT_MAX_SIZE;
+		pr_info_ratelimited("max too large, truncated to %u\n", cfg->max);
+	}
 	if (par->family == NFPROTO_IPV4) {
 		if (cfg->srcmask > 32 || cfg->dstmask > 32)
 			return -EINVAL;
-- 
2.21.1

Re: [Patch nf v2 3/3] xt_hashlimit: limit the max size of hashtable

[Florian Westphal]

Cong Wang <xiyou.wangcong@gmail.com> wrote:
> The user-specified hashtable size is unbound, this could
> easily lead to an OOM or a hung task as we hold the global
> mutex while allocating and initializing the new hashtable.

Reviewed-by: Florian Westphal <fw@strlen.de>

Re: [Patch nf v2 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc

[Florian Westphal]

Cong Wang <xiyou.wangcong@gmail.com> wrote:
> The hashtable size could be controlled by user, so use flags
> GFP_USER | __GFP_NOWARN to avoid OOM warning triggered by user-space.
> 
> Also add __GFP_NORETRY to avoid retrying, as this is just a
> best effort and the failure is already handled gracefully.
> 
> Reported-and-tested-by: syzbot+adf6c6c2be1c3a718121@syzkaller.appspotmail.com
> Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
> Cc: Florian Westphal <fw@strlen.de>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/netfilter/xt_hashlimit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> index bccd47cd7190..5d9943b37c42 100644
> --- a/net/netfilter/xt_hashlimit.c
> +++ b/net/netfilter/xt_hashlimit.c
> @@ -293,8 +293,8 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
>  		if (size < 16)
>  			size = 16;
>  	}
> -	/* FIXME: don't use vmalloc() here or anywhere else -HW */
> -	hinfo = vmalloc(struct_size(hinfo, hash, size));
> +	hinfo = __vmalloc(struct_size(hinfo, hash, size),
> +			  GFP_USER | __GFP_NOWARN | __GFP_NORETRY, PAGE_KERNEL);

Sorry for not noticing this earlier: should that be GFP_KERNEL_ACCOUNT
instead of GFP_USER?

Re: [Patch nf v2 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc

[Cong Wang]

On Mon, Feb 3, 2020 at 4:16 AM Florian Westphal <fw@strlen.de> wrote:
>
> Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > The hashtable size could be controlled by user, so use flags
> > GFP_USER | __GFP_NOWARN to avoid OOM warning triggered by user-space.
> >
> > Also add __GFP_NORETRY to avoid retrying, as this is just a
> > best effort and the failure is already handled gracefully.
> >
> > Reported-and-tested-by: syzbot+adf6c6c2be1c3a718121@syzkaller.appspotmail.com
> > Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> > Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
> > Cc: Florian Westphal <fw@strlen.de>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > ---
> >  net/netfilter/xt_hashlimit.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> > index bccd47cd7190..5d9943b37c42 100644
> > --- a/net/netfilter/xt_hashlimit.c
> > +++ b/net/netfilter/xt_hashlimit.c
> > @@ -293,8 +293,8 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
> >               if (size < 16)
> >                       size = 16;
> >       }
> > -     /* FIXME: don't use vmalloc() here or anywhere else -HW */
> > -     hinfo = vmalloc(struct_size(hinfo, hash, size));
> > +     hinfo = __vmalloc(struct_size(hinfo, hash, size),
> > +                       GFP_USER | __GFP_NOWARN | __GFP_NORETRY, PAGE_KERNEL);
>
> Sorry for not noticing this earlier: should that be GFP_KERNEL_ACCOUNT
> instead of GFP_USER?

Why do you think it should be accounted in kmemcg?

I think this one is controlled by user, so I pick GFP_USER,
like many other cases, for example,
proc_allowed_congestion_control().

GFP_KERNEL_ACCOUNT (or SLAB_ACCOUNT) is not
common in networking, it is typically for socket allocations.
GFP_USER is common.

Thanks.

Re: [Patch nf v2 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc

[Florian Westphal]

Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Feb 3, 2020 at 4:16 AM Florian Westphal <fw@strlen.de> wrote:
> >
> > Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > The hashtable size could be controlled by user, so use flags
> > > GFP_USER | __GFP_NOWARN to avoid OOM warning triggered by user-space.
> > >
> > > Also add __GFP_NORETRY to avoid retrying, as this is just a
> > > best effort and the failure is already handled gracefully.
> > >
> > > Reported-and-tested-by: syzbot+adf6c6c2be1c3a718121@syzkaller.appspotmail.com
> > > Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> > > Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
> > > Cc: Florian Westphal <fw@strlen.de>
> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > > ---
> > >  net/netfilter/xt_hashlimit.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> > > index bccd47cd7190..5d9943b37c42 100644
> > > --- a/net/netfilter/xt_hashlimit.c
> > > +++ b/net/netfilter/xt_hashlimit.c
> > > @@ -293,8 +293,8 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
> > >               if (size < 16)
> > >                       size = 16;
> > >       }
> > > -     /* FIXME: don't use vmalloc() here or anywhere else -HW */
> > > -     hinfo = vmalloc(struct_size(hinfo, hash, size));
> > > +     hinfo = __vmalloc(struct_size(hinfo, hash, size),
> > > +                       GFP_USER | __GFP_NOWARN | __GFP_NORETRY, PAGE_KERNEL);
> >
> > Sorry for not noticing this earlier: should that be GFP_KERNEL_ACCOUNT
> > instead of GFP_USER?
> 
> Why do you think it should be accounted in kmemcg?

We do that for xtables blob allocation, see xt_alloc_table_info().

> I think this one is controlled by user, so I pick GFP_USER,
> like many other cases, for example,
> proc_allowed_congestion_control().

Ok, I see, fair enough -- no objections from me.

Re: [Patch nf v2 2/3] xt_hashlimit: reduce hashlimit_mutex scope for htable_put()

[Pablo Neira Ayuso]

On Sun, Feb 02, 2020 at 08:30:52PM -0800, Cong Wang wrote:
> It is unnecessary to hold hashlimit_mutex for htable_destroy()
> as it is already removed from the global hashtable and its
> refcount is already zero.
> 
> Also, switch hinfo->use to refcount_t so that we don't have
> to hold the mutex until it reaches zero in htable_put().

Applied, thanks.

Re: [Patch nf v2 3/3] xt_hashlimit: limit the max size of hashtable

[Pablo Neira Ayuso]

On Sun, Feb 02, 2020 at 08:30:53PM -0800, Cong Wang wrote:
> The user-specified hashtable size is unbound, this could
> easily lead to an OOM or a hung task as we hold the global
> mutex while allocating and initializing the new hashtable.
> 
> Add a max value to cap both cfg->size and cfg->max, as
> suggested by Florian.

Also applied, thanks.

Re: [Patch nf v2 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc

[Pablo Neira Ayuso]

On Sun, Feb 02, 2020 at 08:30:51PM -0800, Cong Wang wrote:
> The hashtable size could be controlled by user, so use flags
> GFP_USER | __GFP_NOWARN to avoid OOM warning triggered by user-space.
> 
> Also add __GFP_NORETRY to avoid retrying, as this is just a
> best effort and the failure is already handled gracefully.

I think OOM is unlikely to happen now that cfg->size is capped in
your patch 3/3.