| 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 | expand |
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
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
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
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;
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(-)