[V2,nf,3/3] netfilter: nf_tables: add default set size

Message ID 20180710142236.18744-1-ap420073@gmail.com
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series
  • netfilter: nf_tables: fix set destroying bugs
Related show

Commit Message

Taehee Yoo July 10, 2018, 2:22 p.m.
In order to restrict element number of each set, member ->size is used.
that used to be given by user-space. if user-space don't specify ->size,
number of element is unlimited. so that overflow can occurred.

After this patch,
If user-space don't specify ->size, 65535 is set.
all types of set have same default size.

test commands:
   %nft add table ip aa
   %nft add map ip aa map1 { type ipv4_add : verdict\; }
   %nft list ruleset

Before this patch:
   table ip aa {
	   map map1 {
		   type ipv4_addr : verdict
	   }
   }

After this patch:
   table ip aa {
	   map map1 {
		   type ipv4_addr : verdict
		   size 65535
	   }
   }

V2:
 - Add default set->size value instead add check set->size routine.
  - Requested by Florian Westphal

Suggested-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 net/netfilter/nf_tables_api.c | 13 ++++++++-----
 net/netfilter/nft_dynset.c    |  6 +-----
 2 files changed, 9 insertions(+), 10 deletions(-)

Comments

Pablo Neira Ayuso July 18, 2018, 4:44 p.m. | #1
Hi,

On Tue, Jul 10, 2018 at 11:22:36PM +0900, Taehee Yoo wrote:
> In order to restrict element number of each set, member ->size is used.
> that used to be given by user-space. if user-space don't specify ->size,
> number of element is unlimited. so that overflow can occurred.
> 
> After this patch,
> If user-space don't specify ->size, 65535 is set.
> all types of set have same default size.
> 
> test commands:
>    %nft add table ip aa
>    %nft add map ip aa map1 { type ipv4_add : verdict\; }
>    %nft list ruleset
> 
> Before this patch:
>    table ip aa {
> 	   map map1 {
> 		   type ipv4_addr : verdict
> 	   }
>    }
> 
> After this patch:
>    table ip aa {
> 	   map map1 {
> 		   type ipv4_addr : verdict
> 		   size 65535
> 	   }
>    }
> 
> V2:
>  - Add default set->size value instead add check set->size routine.
>   - Requested by Florian Westphal

I agree with Florian in that we can simplify the code by always doing
size accounting all over the place (I mean remove branches to do
inconditional size accounting). So we do size accounting even if we
don't need it.

Then, moving forward, if we go for default size for sets, we may need
a way to signal the kernel that the hashtable is resizable, in case
the user wants to dynamically update the maximum size (in such case,
the rhashtable implementation would be still useful I think).

Another possibility is that we can get rid of the rhashtable, and
implement a more simple way to resize the existing fixed size
hashtable, given that this will only happen from control plane and it
should be a rare operation (just like conntrack resizing), but
probably we may be re-inventing the wheel.

Thoughts?

In any case, this patch should go nf-next, so we have time to discuss
things, so this series are applied, except this one ;-).

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal July 18, 2018, 6:21 p.m. | #2
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Then, moving forward, if we go for default size for sets, we may need
> a way to signal the kernel that the hashtable is resizable, in case
> the user wants to dynamically update the maximum size (in such case,
> the rhashtable implementation would be still useful I think).

Isn't the 'dynamic' flag enough for that?

I think rhashtable backend is still useful, e.g. for meters we might
expect a very low size in practice, even if the upperlimit is large.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Taehee Yoo July 19, 2018, 4:57 p.m. | #3
2018-07-19 1:44 GMT+09:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> Hi,
>
> On Tue, Jul 10, 2018 at 11:22:36PM +0900, Taehee Yoo wrote:
>> In order to restrict element number of each set, member ->size is used.
>> that used to be given by user-space. if user-space don't specify ->size,
>> number of element is unlimited. so that overflow can occurred.
>>
>> After this patch,
>> If user-space don't specify ->size, 65535 is set.
>> all types of set have same default size.
>>
>> test commands:
>>    %nft add table ip aa
>>    %nft add map ip aa map1 { type ipv4_add : verdict\; }
>>    %nft list ruleset
>>
>> Before this patch:
>>    table ip aa {
>>          map map1 {
>>                  type ipv4_addr : verdict
>>          }
>>    }
>>
>> After this patch:
>>    table ip aa {
>>          map map1 {
>>                  type ipv4_addr : verdict
>>                  size 65535
>>          }
>>    }
>>
>> V2:
>>  - Add default set->size value instead add check set->size routine.
>>   - Requested by Florian Westphal
>
> I agree with Florian in that we can simplify the code by always doing
> size accounting all over the place (I mean remove branches to do
> inconditional size accounting). So we do size accounting even if we
> don't need it.
>

Thank you for reviewing!

> Then, moving forward, if we go for default size for sets, we may need
> a way to signal the kernel that the hashtable is resizable, in case
> the user wants to dynamically update the maximum size (in such case,
> the rhashtable implementation would be still useful I think).
>

In my opinion, we can use estimate callback.
If user sets 'performance', hashtable will be selected
or 'memory' is set, rhashtable will be selected.

As far as I know, updating flags and size value is not support.
If we are going to add to support updating maximum size
manually or dynamically, new flags should be added.

> Another possibility is that we can get rid of the rhashtable, and
> implement a more simple way to resize the existing fixed size
> hashtable, given that this will only happen from control plane and it
> should be a rare operation (just like conntrack resizing), but
> probably we may be re-inventing the wheel.
>
> Thoughts?
>

I would like to keep using rhashtable unless the rhashtable
reduces lookup performance much. because I think rhashtable is
a little bit easy to use and it makes readable code.

> In any case, this patch should go nf-next, so we have time to discuss
> things, so this series are applied, except this one ;-).
>
> Thanks!

I agree with it, I checked!

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 896d4a3..eb069b0 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -23,6 +23,8 @@ 
 #include <net/net_namespace.h>
 #include <net/sock.h>
 
+#define NFT_DEFAULT_SET_SIZE	0xffff
+
 static LIST_HEAD(nf_tables_expressions);
 static LIST_HEAD(nf_tables_objects);
 static LIST_HEAD(nf_tables_flowtables);
@@ -3060,8 +3062,7 @@  static int nf_tables_fill_set(struct sk_buff *skb, const struct nft_ctx *ctx,
 	desc = nla_nest_start(skb, NFTA_SET_DESC);
 	if (desc == NULL)
 		goto nla_put_failure;
-	if (set->size &&
-	    nla_put_be32(skb, NFTA_SET_DESC_SIZE, htonl(set->size)))
+	if (nla_put_be32(skb, NFTA_SET_DESC_SIZE, htonl(set->size)))
 		goto nla_put_failure;
 	nla_nest_end(skb, desc);
 
@@ -3437,7 +3438,10 @@  static int nf_tables_newset(struct net *net, struct sock *nlsk,
 	set->objtype = objtype;
 	set->dlen  = desc.dlen;
 	set->flags = flags;
-	set->size  = desc.size;
+	if (desc.size)
+		set->size  = desc.size;
+	else
+		set->size  = NFT_DEFAULT_SET_SIZE;
 	set->policy = policy;
 	set->udlen  = udlen;
 	set->udata  = udata;
@@ -4331,8 +4335,7 @@  static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 		goto err5;
 	}
 
-	if (set->size &&
-	    !atomic_add_unless(&set->nelems, 1, set->size + set->ndeact)) {
+	if (!atomic_add_unless(&set->nelems, 1, set->size + set->ndeact)) {
 		err = -ENFILE;
 		goto err6;
 	}
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index 27d7e459..c26970f 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -57,8 +57,7 @@  static void *nft_dynset_new(struct nft_set *set, const struct nft_expr *expr,
 err2:
 	nft_set_elem_destroy(set, elem, false);
 err1:
-	if (set->size)
-		atomic_dec(&set->nelems);
+	atomic_dec(&set->nelems);
 	return NULL;
 }
 
@@ -223,9 +222,6 @@  static int nft_dynset_init(const struct nft_ctx *ctx,
 	if (err < 0)
 		goto err1;
 
-	if (set->size == 0)
-		set->size = 0xffff;
-
 	priv->set = set;
 	return 0;