netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch nf 0/3] netfilter: xt_hashlimit: a few improvements
@ 2020-01-31 20:52 Cong Wang
  2020-01-31 20:52 ` [Patch nf 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc Cong Wang
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Cong Wang @ 2020-01-31 20:52 UTC (permalink / raw)
  To: netdev; +Cc: netfilter-devel, Cong Wang

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

---

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 | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

-- 
2.21.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Patch nf 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc
  2020-01-31 20:52 [Patch nf 0/3] netfilter: xt_hashlimit: a few improvements Cong Wang
@ 2020-01-31 20:52 ` Cong Wang
  2020-01-31 22:08   ` Florian Westphal
  2020-01-31 20:52 ` [Patch nf 2/3] xt_hashlimit: reduce hashlimit_mutex scope for htable_put() Cong Wang
  2020-01-31 20:52 ` [Patch nf 3/3] xt_hashlimit: limit the max size of hashtable Cong Wang
  2 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2020-01-31 20:52 UTC (permalink / raw)
  To: netdev
  Cc: netfilter-devel, Cong Wang, syzbot+adf6c6c2be1c3a718121,
	Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index bccd47cd7190..885a266d8e57 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -293,8 +293,9 @@ 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));
+	/* FIXME: don't use __vmalloc() here or anywhere else -HW */
+	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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch nf 2/3] xt_hashlimit: reduce hashlimit_mutex scope for htable_put()
  2020-01-31 20:52 [Patch nf 0/3] netfilter: xt_hashlimit: a few improvements Cong Wang
  2020-01-31 20:52 ` [Patch nf 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc Cong Wang
@ 2020-01-31 20:52 ` Cong Wang
  2020-01-31 22:09   ` Florian Westphal
  2020-01-31 20:52 ` [Patch nf 3/3] xt_hashlimit: limit the max size of hashtable Cong Wang
  2 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2020-01-31 20:52 UTC (permalink / raw)
  To: netdev
  Cc: netfilter-devel, Cong Wang, syzbot+adf6c6c2be1c3a718121,
	Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal

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>
Cc: 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 885a266d8e57..57a2639bcc22 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;
 
@@ -316,7 +317,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;
@@ -421,7 +422,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;
 		}
 	}
@@ -430,12 +431,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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch nf 3/3] xt_hashlimit: limit the max size of hashtable
  2020-01-31 20:52 [Patch nf 0/3] netfilter: xt_hashlimit: a few improvements Cong Wang
  2020-01-31 20:52 ` [Patch nf 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc Cong Wang
  2020-01-31 20:52 ` [Patch nf 2/3] xt_hashlimit: reduce hashlimit_mutex scope for htable_put() Cong Wang
@ 2020-01-31 20:52 ` Cong Wang
  2020-01-31 22:08   ` Florian Westphal
  2 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2020-01-31 20:52 UTC (permalink / raw)
  To: netdev
  Cc: netfilter-devel, Cong Wang, syzbot+adf6c6c2be1c3a718121,
	Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal

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.

The max value is derived from the max value when chosen by
the kernel.

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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 57a2639bcc22..6327134c5886 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -272,6 +272,8 @@ dsthash_free(struct xt_hashlimit_htable *ht, struct dsthash_ent *ent)
 }
 static void htable_gc(struct work_struct *work);
 
+#define HASHLIMIT_MAX_SIZE 8192
+
 static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
 			 const char *name, u_int8_t family,
 			 struct xt_hashlimit_htable **out_hinfo,
@@ -290,7 +292,7 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
 		size = (nr_pages << PAGE_SHIFT) / 16384 /
 		       sizeof(struct hlist_head);
 		if (nr_pages > 1024 * 1024 * 1024 / PAGE_SIZE)
-			size = 8192;
+			size = HASHLIMIT_MAX_SIZE;
 		if (size < 16)
 			size = 16;
 	}
@@ -848,6 +850,8 @@ 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)
+		return -ENOMEM;
 	if (par->family == NFPROTO_IPV4) {
 		if (cfg->srcmask > 32 || cfg->dstmask > 32)
 			return -EINVAL;
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Patch nf 3/3] xt_hashlimit: limit the max size of hashtable
  2020-01-31 20:52 ` [Patch nf 3/3] xt_hashlimit: limit the max size of hashtable Cong Wang
@ 2020-01-31 22:08   ` Florian Westphal
  2020-01-31 23:16     ` Cong Wang
  0 siblings, 1 reply; 14+ messages in thread
From: Florian Westphal @ 2020-01-31 22:08 UTC (permalink / raw)
  To: Cong Wang
  Cc: netdev, netfilter-devel, syzbot+adf6c6c2be1c3a718121,
	Pablo Neira Ayuso, Jozsef Kadlecsik, 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.
> 
> The max value is derived from the max value when chosen by
> the kernel.
> 
> 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 | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> index 57a2639bcc22..6327134c5886 100644
> --- a/net/netfilter/xt_hashlimit.c
> +++ b/net/netfilter/xt_hashlimit.c
> @@ -272,6 +272,8 @@ dsthash_free(struct xt_hashlimit_htable *ht, struct dsthash_ent *ent)
>  }
>  static void htable_gc(struct work_struct *work);
>  
> +#define HASHLIMIT_MAX_SIZE 8192
> +
>  static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
>  			 const char *name, u_int8_t family,
>  			 struct xt_hashlimit_htable **out_hinfo,
> @@ -290,7 +292,7 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
>  		size = (nr_pages << PAGE_SHIFT) / 16384 /
>  		       sizeof(struct hlist_head);
>  		if (nr_pages > 1024 * 1024 * 1024 / PAGE_SIZE)
> -			size = 8192;
> +			size = HASHLIMIT_MAX_SIZE;
>  		if (size < 16)
>  			size = 16;
>  	}
> @@ -848,6 +850,8 @@ 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)
> +		return -ENOMEM;

Hmm, won't that break restore of rulesets that have something like

--hashlimit-size 10000?

AFAIU this limits the module to vmalloc requests of only 64kbyte.
I'm not opposed to a limit (or a cap), but 64k seems a bit low to me.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch nf 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc
  2020-01-31 20:52 ` [Patch nf 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc Cong Wang
@ 2020-01-31 22:08   ` Florian Westphal
  2020-01-31 23:17     ` Cong Wang
  0 siblings, 1 reply; 14+ messages in thread
From: Florian Westphal @ 2020-01-31 22:08 UTC (permalink / raw)
  To: Cong Wang
  Cc: netdev, netfilter-devel, syzbot+adf6c6c2be1c3a718121,
	Pablo Neira Ayuso, Jozsef Kadlecsik, 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 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> index bccd47cd7190..885a266d8e57 100644
> --- a/net/netfilter/xt_hashlimit.c
> +++ b/net/netfilter/xt_hashlimit.c
> @@ -293,8 +293,9 @@ 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));
> +	/* FIXME: don't use __vmalloc() here or anywhere else -HW */
> +	hinfo = __vmalloc(struct_size(hinfo, hash, size),
> +			  GFP_USER | __GFP_NOWARN | __GFP_NORETRY, PAGE_KERNEL);

Rationale looks sane, wonder if it makes sense to drop Haralds comment
though, I don't see what other solution other than vmalloc could be used
here.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch nf 2/3] xt_hashlimit: reduce hashlimit_mutex scope for htable_put()
  2020-01-31 20:52 ` [Patch nf 2/3] xt_hashlimit: reduce hashlimit_mutex scope for htable_put() Cong Wang
@ 2020-01-31 22:09   ` Florian Westphal
  0 siblings, 0 replies; 14+ messages in thread
From: Florian Westphal @ 2020-01-31 22:09 UTC (permalink / raw)
  To: Cong Wang
  Cc: netdev, netfilter-devel, syzbot+adf6c6c2be1c3a718121,
	Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal

Cong Wang <xiyou.wangcong@gmail.com> 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().

LGTM, thanks!

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch nf 3/3] xt_hashlimit: limit the max size of hashtable
  2020-01-31 22:08   ` Florian Westphal
@ 2020-01-31 23:16     ` Cong Wang
  2020-01-31 23:36       ` Florian Westphal
  0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2020-01-31 23:16 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Linux Kernel Network Developers, NetFilter, syzbot,
	Pablo Neira Ayuso, Jozsef Kadlecsik

On Fri, Jan 31, 2020 at 2:08 PM Florian Westphal <fw@strlen.de> wrote:
>
> 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.
> >
> > The max value is derived from the max value when chosen by
> > the kernel.
> >
> > 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 | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> > index 57a2639bcc22..6327134c5886 100644
> > --- a/net/netfilter/xt_hashlimit.c
> > +++ b/net/netfilter/xt_hashlimit.c
> > @@ -272,6 +272,8 @@ dsthash_free(struct xt_hashlimit_htable *ht, struct dsthash_ent *ent)
> >  }
> >  static void htable_gc(struct work_struct *work);
> >
> > +#define HASHLIMIT_MAX_SIZE 8192
> > +
> >  static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
> >                        const char *name, u_int8_t family,
> >                        struct xt_hashlimit_htable **out_hinfo,
> > @@ -290,7 +292,7 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
> >               size = (nr_pages << PAGE_SHIFT) / 16384 /
> >                      sizeof(struct hlist_head);
> >               if (nr_pages > 1024 * 1024 * 1024 / PAGE_SIZE)
> > -                     size = 8192;
> > +                     size = HASHLIMIT_MAX_SIZE;
> >               if (size < 16)
> >                       size = 16;
> >       }
> > @@ -848,6 +850,8 @@ 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)
> > +             return -ENOMEM;
>
> Hmm, won't that break restore of rulesets that have something like
>
> --hashlimit-size 10000?
>
> AFAIU this limits the module to vmalloc requests of only 64kbyte.
> I'm not opposed to a limit (or a cap), but 64k seems a bit low to me.

8192 is from the current code which handles kernel-chosen size
(that is cfg->size==0), I personally have no idea what the max
should be. :)

Please suggest a number.

Thanks.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch nf 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc
  2020-01-31 22:08   ` Florian Westphal
@ 2020-01-31 23:17     ` Cong Wang
  0 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2020-01-31 23:17 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Linux Kernel Network Developers, NetFilter, syzbot,
	Pablo Neira Ayuso, Jozsef Kadlecsik

On Fri, Jan 31, 2020 at 2:08 PM 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 | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> > index bccd47cd7190..885a266d8e57 100644
> > --- a/net/netfilter/xt_hashlimit.c
> > +++ b/net/netfilter/xt_hashlimit.c
> > @@ -293,8 +293,9 @@ 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));
> > +     /* FIXME: don't use __vmalloc() here or anywhere else -HW */
> > +     hinfo = __vmalloc(struct_size(hinfo, hash, size),
> > +                       GFP_USER | __GFP_NOWARN | __GFP_NORETRY, PAGE_KERNEL);
>
> Rationale looks sane, wonder if it makes sense to drop Haralds comment
> though, I don't see what other solution other than vmalloc could be used
> here.

I will remove it.

Thanks.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch nf 3/3] xt_hashlimit: limit the max size of hashtable
  2020-01-31 23:16     ` Cong Wang
@ 2020-01-31 23:36       ` Florian Westphal
  2020-02-01  2:53         ` Cong Wang
  0 siblings, 1 reply; 14+ messages in thread
From: Florian Westphal @ 2020-01-31 23:36 UTC (permalink / raw)
  To: Cong Wang
  Cc: Florian Westphal, Linux Kernel Network Developers, NetFilter,
	syzbot, Pablo Neira Ayuso, Jozsef Kadlecsik

Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Jan 31, 2020 at 2:08 PM Florian Westphal <fw@strlen.de> wrote:
> >
> > 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.
> > >
> > > The max value is derived from the max value when chosen by
> > > the kernel.
> > >
> > > 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 | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> > > index 57a2639bcc22..6327134c5886 100644
> > > --- a/net/netfilter/xt_hashlimit.c
> > > +++ b/net/netfilter/xt_hashlimit.c
> > > @@ -272,6 +272,8 @@ dsthash_free(struct xt_hashlimit_htable *ht, struct dsthash_ent *ent)
> > >  }
> > >  static void htable_gc(struct work_struct *work);
> > >
> > > +#define HASHLIMIT_MAX_SIZE 8192
> > > +
> > >  static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
> > >                        const char *name, u_int8_t family,
> > >                        struct xt_hashlimit_htable **out_hinfo,
> > > @@ -290,7 +292,7 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
> > >               size = (nr_pages << PAGE_SHIFT) / 16384 /
> > >                      sizeof(struct hlist_head);
> > >               if (nr_pages > 1024 * 1024 * 1024 / PAGE_SIZE)
> > > -                     size = 8192;
> > > +                     size = HASHLIMIT_MAX_SIZE;
> > >               if (size < 16)
> > >                       size = 16;
> > >       }
> > > @@ -848,6 +850,8 @@ 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)
> > > +             return -ENOMEM;
> >
> > Hmm, won't that break restore of rulesets that have something like
> >
> > --hashlimit-size 10000?
> >
> > AFAIU this limits the module to vmalloc requests of only 64kbyte.
> > I'm not opposed to a limit (or a cap), but 64k seems a bit low to me.
> 
> 8192 is from the current code which handles kernel-chosen size
> (that is cfg->size==0), I personally have no idea what the max
> should be. :)

Me neither :-/

> Please suggest a number.

O would propose a max alloc size (hard limit) of ~8 MByte of vmalloc
space, or maybe 16 at most.

1048576 max upperlimit -> ~8mbyte vmalloc request -> allows to store
up to 2**23 entries.

In order to prevent breaking userspace, perhaps make it so that the
kernel caps cfg.max at twice that value?  Would allow storing up to
16777216 addresses with an average chain depth of 16 (which is quite
large).  We could increase the max limit in case someone presents a use
case.

What do you think?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch nf 3/3] xt_hashlimit: limit the max size of hashtable
  2020-01-31 23:36       ` Florian Westphal
@ 2020-02-01  2:53         ` Cong Wang
  2020-02-02  6:16           ` Florian Westphal
  0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2020-02-01  2:53 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Linux Kernel Network Developers, NetFilter, syzbot,
	Pablo Neira Ayuso, Jozsef Kadlecsik

On Fri, Jan 31, 2020 at 3:37 PM Florian Westphal <fw@strlen.de> wrote:
> O would propose a max alloc size (hard limit) of ~8 MByte of vmalloc
> space, or maybe 16 at most.
>
> 1048576 max upperlimit -> ~8mbyte vmalloc request -> allows to store
> up to 2**23 entries.

Changing HASHLIMIT_MAX_SIZE to 1048576 seems fine.

>
> In order to prevent breaking userspace, perhaps make it so that the
> kernel caps cfg.max at twice that value?  Would allow storing up to
> 16777216 addresses with an average chain depth of 16 (which is quite
> large).  We could increase the max limit in case someone presents a use
> case.
>

Not sure if I understand this, I don't see how cap'ing cfg->max could
help prevent breaking user-space? Are you suggesting to cap it with
HASHLIMIT_MAX_SIZE too? Something like below?

+       if (cfg->max > 2 * HASHLIMIT_MAX_SIZE)
+               cfg->max = 2 * HASHLIMIT_MAX_SIZE;

Thanks.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch nf 3/3] xt_hashlimit: limit the max size of hashtable
  2020-02-01  2:53         ` Cong Wang
@ 2020-02-02  6:16           ` Florian Westphal
  2020-02-02 18:27             ` Cong Wang
  0 siblings, 1 reply; 14+ messages in thread
From: Florian Westphal @ 2020-02-02  6:16 UTC (permalink / raw)
  To: Cong Wang
  Cc: Florian Westphal, Linux Kernel Network Developers, NetFilter,
	syzbot, Pablo Neira Ayuso, Jozsef Kadlecsik

Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > In order to prevent breaking userspace, perhaps make it so that the
> > kernel caps cfg.max at twice that value?  Would allow storing up to
> > 16777216 addresses with an average chain depth of 16 (which is quite
> > large).  We could increase the max limit in case someone presents a use
> > case.
> >
> 
> Not sure if I understand this, I don't see how cap'ing cfg->max could
> help prevent breaking user-space? Are you suggesting to cap it with
> HASHLIMIT_MAX_SIZE too? Something like below?
> 
> +       if (cfg->max > 2 * HASHLIMIT_MAX_SIZE)
> +               cfg->max = 2 * HASHLIMIT_MAX_SIZE;
> 

Yes, thats what I meant, cap the user-provided value to something thats
going to be less of a problem.

But now that I read it, the "2 *" part looks really silly, so I suggst
to go with " > FOO_MAX", else its not a maximum value after all.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch nf 3/3] xt_hashlimit: limit the max size of hashtable
  2020-02-02  6:16           ` Florian Westphal
@ 2020-02-02 18:27             ` Cong Wang
  2020-02-02 22:37               ` Florian Westphal
  0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2020-02-02 18:27 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Linux Kernel Network Developers, NetFilter, syzbot,
	Pablo Neira Ayuso, Jozsef Kadlecsik

On Sat, Feb 1, 2020 at 10:16 PM Florian Westphal <fw@strlen.de> wrote:
>
> Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > In order to prevent breaking userspace, perhaps make it so that the
> > > kernel caps cfg.max at twice that value?  Would allow storing up to
> > > 16777216 addresses with an average chain depth of 16 (which is quite
> > > large).  We could increase the max limit in case someone presents a use
> > > case.
> > >
> >
> > Not sure if I understand this, I don't see how cap'ing cfg->max could
> > help prevent breaking user-space? Are you suggesting to cap it with
> > HASHLIMIT_MAX_SIZE too? Something like below?
> >
> > +       if (cfg->max > 2 * HASHLIMIT_MAX_SIZE)
> > +               cfg->max = 2 * HASHLIMIT_MAX_SIZE;
> >
>
> Yes, thats what I meant, cap the user-provided value to something thats
> going to be less of a problem.
>
> But now that I read it, the "2 *" part looks really silly, so I suggst
> to go with " > FOO_MAX", else its not a maximum value after all.

Ok, so here is what I have now:


+#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);
+       }

Please let me know if it is still different with your suggestion.

Thanks!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch nf 3/3] xt_hashlimit: limit the max size of hashtable
  2020-02-02 18:27             ` Cong Wang
@ 2020-02-02 22:37               ` Florian Westphal
  0 siblings, 0 replies; 14+ messages in thread
From: Florian Westphal @ 2020-02-02 22:37 UTC (permalink / raw)
  To: Cong Wang
  Cc: Florian Westphal, Linux Kernel Network Developers, NetFilter,
	syzbot, Pablo Neira Ayuso, Jozsef Kadlecsik

Cong Wang <xiyou.wangcong@gmail.com> wrote:
> Ok, so here is what I have now:
> 
> 
> +#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);
> +       }
> 
> Please let me know if it is still different with your suggestion.

I am fine with this.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-02-02 22:37 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-31 20:52 [Patch nf 0/3] netfilter: xt_hashlimit: a few improvements Cong Wang
2020-01-31 20:52 ` [Patch nf 1/3] xt_hashlimit: avoid OOM for user-controlled vmalloc Cong Wang
2020-01-31 22:08   ` Florian Westphal
2020-01-31 23:17     ` Cong Wang
2020-01-31 20:52 ` [Patch nf 2/3] xt_hashlimit: reduce hashlimit_mutex scope for htable_put() Cong Wang
2020-01-31 22:09   ` Florian Westphal
2020-01-31 20:52 ` [Patch nf 3/3] xt_hashlimit: limit the max size of hashtable Cong Wang
2020-01-31 22:08   ` Florian Westphal
2020-01-31 23:16     ` Cong Wang
2020-01-31 23:36       ` Florian Westphal
2020-02-01  2:53         ` Cong Wang
2020-02-02  6:16           ` Florian Westphal
2020-02-02 18:27             ` Cong Wang
2020-02-02 22:37               ` Florian Westphal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).