Hi Vishwanath,
On Wed, 29 Jun 2022, Vishwanath Pai wrote:
> We fixed a similar problem before in commit 7f4f7dd4417d ("netfilter:
> ipset: ipset list may return wrong member count for set with timeout").
> The same issue exists in ip_set_bitmap_gen.h as well.
Could you rework the patch to solve the issue the same way as for the hash
types (i.e. scanning the set without locking) like in the commit
33f08da28324 (netfilter: ipset: Fix "INFO: rcu detected stall in hash_xxx"
reports)? I know the bitmap types have got a limited size but it'd be
great if the general method would be the same across the different types.
Best regards,
Jozsef
> Test case:
>
> $ ipset create test bitmap:ip range 10.0.0.0/8 netmask 24 timeout 5
> $ ipset add test 10.0.0.0
> $ ipset add test 10.255.255.255
> $ sleep 5s
>
> $ ipset list test
> Name: test
> Type: bitmap:ip
> Revision: 3
> Header: range 10.0.0.0-10.255.255.255 netmask 24 timeout 5
> Size in memory: 532568
> References: 0
> Number of entries: 2
> Members:
>
> We return "Number of entries: 2" but no members are listed. That is
> because when we run mtype_head the garbage collector hasn't run yet, but
> mtype_list checks and cleans up members with expired timeout. To avoid
> this we can run mtype_expire before printing the number of elements in
> mytype_head().
>
> Reviewed-by: Joshua Hunt <johunt@xxxxxxxxxx>
> Signed-off-by: Vishwanath Pai <vpai@xxxxxxxxxx>
> ---
> net/netfilter/ipset/ip_set_bitmap_gen.h | 46 ++++++++++++++++++-------
> 1 file changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/net/netfilter/ipset/ip_set_bitmap_gen.h b/net/netfilter/ipset/ip_set_bitmap_gen.h
> index 26ab0e9612d8..dd871305bd6e 100644
> --- a/net/netfilter/ipset/ip_set_bitmap_gen.h
> +++ b/net/netfilter/ipset/ip_set_bitmap_gen.h
> @@ -28,6 +28,7 @@
> #define mtype_del IPSET_TOKEN(MTYPE, _del)
> #define mtype_list IPSET_TOKEN(MTYPE, _list)
> #define mtype_gc IPSET_TOKEN(MTYPE, _gc)
> +#define mtype_expire IPSET_TOKEN(MTYPE, _expire)
> #define mtype MTYPE
>
> #define get_ext(set, map, id) ((map)->extensions + ((set)->dsize * (id)))
> @@ -88,13 +89,44 @@ mtype_memsize(const struct mtype *map, size_t dsize)
> map->elements * dsize;
> }
>
> +/* We should grab set->lock before calling this function */
> +static void
> +mtype_expire(struct ip_set *set)
> +{
> + struct mtype *map = set->data;
> + void *x;
> + u32 id;
> +
> + for (id = 0; id < map->elements; id++)
> + if (mtype_gc_test(id, map, set->dsize)) {
> + x = get_ext(set, map, id);
> + if (ip_set_timeout_expired(ext_timeout(x, set))) {
> + clear_bit(id, map->members);
> + ip_set_ext_destroy(set, x);
> + set->elements--;
> + }
> + }
> +}
> +
> static int
> mtype_head(struct ip_set *set, struct sk_buff *skb)
> {
> const struct mtype *map = set->data;
> struct nlattr *nested;
> - size_t memsize = mtype_memsize(map, set->dsize) + set->ext_size;
> + size_t memsize;
> +
> + /* If any members have expired, set->elements will be wrong,
> + * mytype_expire function will update it with the right count.
> + * set->elements can still be incorrect in the case of a huge set
> + * because elements can timeout during set->list().
> + */
> + if (SET_WITH_TIMEOUT(set)) {
> + spin_lock_bh(&set->lock);
> + mtype_expire(set);
> + spin_unlock_bh(&set->lock);
> + }
>
> + memsize = mtype_memsize(map, set->dsize) + set->ext_size;
> nested = nla_nest_start(skb, IPSET_ATTR_DATA);
> if (!nested)
> goto nla_put_failure;
> @@ -266,22 +298,12 @@ mtype_gc(struct timer_list *t)
> {
> struct mtype *map = from_timer(map, t, gc);
> struct ip_set *set = map->set;
> - void *x;
> - u32 id;
>
> /* We run parallel with other readers (test element)
> * but adding/deleting new entries is locked out
> */
> spin_lock_bh(&set->lock);
> - for (id = 0; id < map->elements; id++)
> - if (mtype_gc_test(id, map, set->dsize)) {
> - x = get_ext(set, map, id);
> - if (ip_set_timeout_expired(ext_timeout(x, set))) {
> - clear_bit(id, map->members);
> - ip_set_ext_destroy(set, x);
> - set->elements--;
> - }
> - }
> + mtype_expire(set);
> spin_unlock_bh(&set->lock);
>
> map->gc.expires = jiffies + IPSET_GC_PERIOD(set->timeout) * HZ;
> --
> 2.25.1
>
>
-
E-mail : kadlec@xxxxxxxxxxxxxxxxx, kadlecsik.jozsef@xxxxxxxxx
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
H-1525 Budapest 114, POB. 49, Hungary
