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

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

[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

[Patch nf 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 | 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

Re: [Patch nf 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 | 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.

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

[Cong Wang]

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.

[Patch nf 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>
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

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

[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>

[Patch nf 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.

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

Re: [Patch nf 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.
> 
> 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.

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

[Cong Wang]

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.

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

[Florian Westphal]

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?

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

[Cong Wang]

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.

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

[Florian Westphal]

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.

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

[Cong Wang]

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!

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

[Florian Westphal]

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.