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

Hi Jozsef, hi Pablo

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

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

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.