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

Hi Florian,

[Sorry for the long delay!]

On Mon, 3 Aug 2026, Florian Westphal wrote:

> 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.

Yes, it is synchronous so atomic64_set() just resets the value to zero 
again.

> Maybe this should be changed to
> 
> NET_DEBUG_WARN_ON_ONCE(atomic64_read() != 0) ?

Yes, I agree: it'd add a safety check that mtype_ext_cleanup() did the job 
properly.
 
> 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:

You mean when we leave a hole in the first position? Yes, it's a corner 
case...

> [ 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;
> +       }

... and I agree that your suggestion above is fine. We are talking about 
the case when the SET target adds/deletes an entry to/from a hash:*net* 
type of set and it should figure out the cidr value to use for the network 
address to add. The convention is "use the first, i.e. the most specific 
value from the list of already added cidr values or if (the list) is 
empty, the host_mask cidr. If the count of most specific cidr reached zero 
but the new net_prefix could not be created to remove it, it's fine to 
fall back to host_mask. Especially when the holes are moved to the end of 
the array.

Best regards,
Jozsef
-- 
E-mail : [email protected], [email protected], [email protected]
Address: Wigner Research Centre for Physics
         H-1525 Budapest 114, POB. 49, Hungary