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

Florian Westphal <[email protected]> wrote:
> This series contains a few fixes and prepation work for the
> ipset rhashtable conversion.  The rhashtable conversion patch set is
> too large to post in one pile and has one external dependency that is not
> yet in nf (queued in crypto tree).
> 
> I discussed this with Jozsef and we think that targeting the rhashtable
> conversion to nf tree is preferrable to nf-next due to the volume of LLM
> reports.
> 
> 1) Rework ipset CIDR bookkeeping with an RCU-based method to ensure
> safety on weakly-ordered architectures. Replace in-place updates with
> structure replacement. From Jozsef Kadlecsik.

1 & 2 two trigger sashiko reports, but I ask that those are ignored:
https://sashiko.dev/#/patchset/20260730183853.21868-1-fw%40strlen.de

'Is it safe to modify h->rnets[i] and call kfree_rcu() here without holding
set->lock?'

No.  This is resolved in the rhashtable conversion, which introduces
synchronization between delete and flush: only one thread can detach the
element from the rhashtable, those observing the -ENOENT will skip the
element.

>  #ifdef IP_SET_HASH_WITH_NETS
>  	for (i = 0; i < IPSET_NET_COUNT; i++)
> -		mtype_add_cidr(set, h, NCIDR_PUT(DCIDR_GET(d->cidr, i)), i);
> +		mtype_add_cidr(set, h, DCIDR_GET(d->cidr, i), i);

'Does this code correctly handle memory allocation failures?'

No.  I added this handling in the rhashtable conversion, as that has to
rewrite most of that function anyway.

> 2) Switch ext_size to atomic64_t in ipset to prevent races during parallel
> updates.  Also from Jozsef.

'This isn't a bug introduced by this patch, but can a race condition during hash
table resizing and concurrent element updates lead to a double free of comment
extensions here?'

Yes, this is also resolved when converting to rhashtable, the resize
callback is removed.

'This is a pre-existing issue, but does asynchronous element destruction in
list_set_flush() race with this synchronous zeroing of set->ext_size, causing
an integer underflow?

If a user flushes a list:set ipset, list_set_flush() forces the counter to 0
here. The elements are queued for asynchronous deletion via call_rcu() inside
list_set_del().'

I will have another look at this to see if thats correct, in any case
this should be handled in a different change.