Re: [PATCH nf] netfilter: ipset: fix list type element drift bug

Hi Florian,

On Thu, 6 Aug 2026, Florian Westphal wrote:

> TL;DR, more unrelated drive-by findings, probably even wrong.
> 
> Florian Westphal <[email protected]> wrote:
> > If list_set_uadd() calls list_set_replace() to swap an expired entry,
> > the element count remains the same, therefore the increment must be elided.

Yes, that is correct and your patch "netfilter: ipset: fix list type 
element drift bug" fixes it properly.
 
> > Fixes: 702b71e7c666 ("netfilter: ipset: Add element count to all set types header")
> > Link: https://sashiko.dev/#/patchset/20260806101947.2802-1-fw%40strlen.de
> > Signed-off-by: Florian Westphal <[email protected]>
> 
> Jozsef, could you please have a look at the LLM sauce below?
> https://sashiko.dev/#/patchset/20260806135341.8896-1-fw%40strlen.de
> -- LLM sauce follows --
> When inserting before a reference element, does this mistakenly identify the
> element after the reference as the candidate for replacement?
> Similarly, when inserting after, does it replace the element before the
> reference?
> Could this lead to incorrect rule ordering by placing the new element on the
> opposite side of the reference element than requested?
> -- LLM end --

I can't really make sense of it. There's no replacement here, the code 
tries to identify where to insert the element: before/after the reference 
one, if it was given. The only replacement happens when the same element 
is re-added and the extensions may change, but then in that case there's 
no before-after.
 
> I had a look, even if its correct, this change is super old:
> 00590fdd5be0 ("netfilter: ipset: Introduce RCU locking in list type")
> 
> It is been in RHEL7 for a long time as well.
> So even if its correct, I'm reluctant to make changes.

The testsuite contains tests to verify add/del/test before/after 
functionalities. So I don't know what to fix here.
 
> -- LLM sauce follows --
> This is a pre-existing issue, but does this code fail to enforce the set size
> limit?
> Just below this hunk in list_set_uadd(), the element is added and the counter
> is incremented without validation:
> 	else if (prev)
> 		list_add_rcu(&e->list, &prev->list);
> 	else
> 		list_add_tail_rcu(&e->list, &map->members);
> 	set->elements++;
> Since the list:set type accepts a size parameter during creation, should this
> code verify that set->elements is strictly less than map->size before adding
> the new element to prevent unbounded memory allocation?
> -- LLM end --
> 
> ipset man page says size parameter is ignored nowadays, so this
> looks intentional.

Yes, exactly. The parameter is kept for backward compatibility reason 
only.
 
> So the only complaint about this patch that is valid is this:
> 
> +		return 0;
> +       }
> +
>         else if (next)
> 
> Should better be:
> +		return 0;
> +       }
> +
> +       if (next)
> -       else if (next)
> 
> .. as its more readable.
> 

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