Re: [PATCH nf 0/5] netfilter: ipset fixes and rhastable prep work

Jozsef Kadlecsik <[email protected]> wrote:
> > I will have another look at this to see if thats correct, in any case
> > this should be handled in a different change.
> 
> I should have left out the call to atomic64_set(&set->ext_size, 0) in 
> list_set_flush() and let the list_set_del() calls adjust the ext_size 
> value. What do you thing, would it solve the issue?

Agree wrt. list_set_flush(), removing the explicit set should
work.  What about:

mtype_flush(struct ip_set *set)
{
        struct mtype *map = set->data;

        if (set->extensions & IPSET_EXT_DESTROY)
                mtype_ext_cleanup(set);
        bitmap_zero(map->members, map->elements);
        set->elements = 0;
        atomic64_set(&set->ext_size, 0);
}

in ip_set_bitmap_gen.h ?

Should this be changed as well, i.e. remove atomic64_set()?

AFAICS this is harmless because its synchronous and runs with
set->lock held, so this should set ext_size to 0 again.

Maybe this should be changed to

NET_DEBUG_WARN_ON_ONCE(atomic64_read() != 0) ?

There is another new report:

#define INIT_CIDR(n, host_mask) ({                              \
        const struct net_prefixes *__n = rcu_dereference(n);            \
        DCIDR_PUT((__n)->len ? (__n)->nets[0].cidr : host_mask);\
})

If we fail to allocate replacement net_prefix, then nets[0].count can
be 0.  This means we either need to walk ->nets[] until we find a slot
where count is > 0, or we need to resort to something like this:

[ not even compile tested! ]

 #define INIT_CIDR(n, host_mask)        \
-       DCIDR_PUT((n)->len ? (n)->nets[0].cidr : host_mask)
+       DCIDR_PUT((n)->len && (n)->nets[0].count ? (n)->nets[0].cidr : host_mask)

 #endif /* IP_SET_HASH_WITH_NETS */

@@ -374,9 +374,20 @@ mtype_del_cidr(struct ip_set *set, struct htype *h, u8 cidr, u8 n)
        len--;
        tmp = kzalloc(sizeof(struct net_prefixes) +
                      len * sizeof(struct net_prefix), GFP_ATOMIC);
-       if (!tmp)
-               /* Leave a hole */
+       if (!tmp) { /* handle in-place */
+               for (i = 0, j = 0; i < nets->len; i++) {
+                       if (i == found)
+                               continue;
+                       if (i != j) {
+                               WRITE_ONCE(nets->nets[j].cidr, nets->nets[i].cidr);
+                               WRITE_ONCE(nets->nets[j].count, nets->nets[i].count);
+                       }
+                       j++;
+               }
+               while (j < nets->len)
+                       WRITE_ONCE(nets->nets[j++].count, 0);
                goto unlock;
+       }