| Message ID | 20180619072722.4638-1-rvarsha016@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf-next,v5] net: netfilter: nf_tables_api: Use id allocation | expand |
Hi Varsha, On Tue, Jun 19, 2018 at 12:57:22PM +0530, Varsha Rao wrote: [...] > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index ca4c4d994ddb..999255717398 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c > @@ -2924,18 +2924,14 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set, > { > const struct nft_set *i; > const char *p; > - unsigned long *inuse; > - unsigned int n = 0, min = 0; > + unsigned int n = 0, id = 0; > + DEFINE_IDA(inuse); > > p = strchr(name, '%'); > if (p != NULL) { > if (p[1] != 'd' || strchr(p + 2, '%')) > return -EINVAL; > > - inuse = (unsigned long *)get_zeroed_page(GFP_KERNEL); > - if (inuse == NULL) > - return -ENOMEM; > -cont: > list_for_each_entry(i, &ctx->table->sets, list) { > int tmp; > > @@ -2943,22 +2939,30 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set, > continue; > if (!sscanf(i->name, name, &tmp)) > continue; > - if (tmp < min || tmp >= min + BITS_PER_BYTE * PAGE_SIZE) > + if (tmp < 0) I think this branch go away. tmp takes the values from the existing set name in the list, and I don't think we allow set with negative numbers. So this should not ever happen. This check was there in case the page filled up with 1s, so we can continue from where we were in the iteration over the set list. As we discussed during the NFWS, I wonder if the original code really works at all, since you told me you never exercised the "cont:" path. So it would be great to really know if this code is buggy. BTW, I remember you mentioned about "Too many open files errors". I found the reason for this. It seems this nf_tables_set_alloc_name() function returns ENFILE at a later stage. Basically ENFILE is returned when we select a set name that is already in used. So if your patch is hitting this, then maybe there's a bug that makes us hit the ENFILE error. Thanks for not giving up on this! -- 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 ca4c4d994ddb..999255717398 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -2924,18 +2924,14 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set, { const struct nft_set *i; const char *p; - unsigned long *inuse; - unsigned int n = 0, min = 0; + unsigned int n = 0, id = 0; + DEFINE_IDA(inuse); p = strchr(name, '%'); if (p != NULL) { if (p[1] != 'd' || strchr(p + 2, '%')) return -EINVAL; - inuse = (unsigned long *)get_zeroed_page(GFP_KERNEL); - if (inuse == NULL) - return -ENOMEM; -cont: list_for_each_entry(i, &ctx->table->sets, list) { int tmp; @@ -2943,22 +2939,30 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set, continue; if (!sscanf(i->name, name, &tmp)) continue; - if (tmp < min || tmp >= min + BITS_PER_BYTE * PAGE_SIZE) + if (tmp < 0) continue; - set_bit(tmp - min, inuse); + n = ida_get_new_above(&inuse, tmp, &id); + if (n < 0) { + if (n == -EAGAIN) + return -ENOMEM; + + return n; + } } - n = find_first_zero_bit(inuse, BITS_PER_BYTE * PAGE_SIZE); - if (n >= BITS_PER_BYTE * PAGE_SIZE) { - min += BITS_PER_BYTE * PAGE_SIZE; - memset(inuse, 0, PAGE_SIZE); - goto cont; + n = ida_get_new_above(&inuse, 0, &id); + ida_destroy(&inuse); + + if (n < 0) { + if (n == -EAGAIN) + return -ENOMEM; + return n; } - free_page((unsigned long)inuse); + } - set->name = kasprintf(GFP_KERNEL, name, min + n); + set->name = kasprintf(GFP_KERNEL, name, id); if (!set->name) return -ENOMEM;