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

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

[Florian Westphal]

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 <fw@strlen.de>
---
 net/netfilter/ipset/ip_set_list_set.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/ipset/ip_set_list_set.c b/net/netfilter/ipset/ip_set_list_set.c
index ca3ef9479e83..56626f4943a9 100644
--- a/net/netfilter/ipset/ip_set_list_set.c
+++ b/net/netfilter/ipset/ip_set_list_set.c
@@ -301,8 +301,11 @@ list_set_uadd(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 	e->set = set;
 	INIT_LIST_HEAD(&e->list);
 	list_set_init_extensions(set, ext, e);
-	if (n)
+	if (n) {
 		list_set_replace(set, e, n);
+		return 0;
+	}
+
 	else if (next)
 		list_add_tail_rcu(&e->list, &next->list);
 	else if (prev)
-- 
2.54.0

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

[Florian Westphal]

Hi Jozsef, hi Pablo

TL;DR, more unrelated drive-by findings, probably even wrong.

Florian Westphal <fw@strlen.de> 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 <fw@strlen.de>

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.

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

[Jozsef Kadlecsik]

Hi Florian,

On Thu, 6 Aug 2026, Florian Westphal wrote:

> TL;DR, more unrelated drive-by findings, probably even wrong.
> 
> Florian Westphal <fw@strlen.de> 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 <fw@strlen.de>
> 
> 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 : kadlec@netfilter.org, kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
Address: Wigner Research Centre for Physics
         H-1525 Budapest 114, POB. 49, Hungary

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

[Jozsef Kadlecsik]

On Thu, 6 Aug 2026, Florian Westphal 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, well-spotted!

Acked-by: Jozsef Kadlecsik <kadlec@netfilter.org>

> 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 <fw@strlen.de>
> ---
>  net/netfilter/ipset/ip_set_list_set.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/ipset/ip_set_list_set.c b/net/netfilter/ipset/ip_set_list_set.c
> index ca3ef9479e83..56626f4943a9 100644
> --- a/net/netfilter/ipset/ip_set_list_set.c
> +++ b/net/netfilter/ipset/ip_set_list_set.c
> @@ -301,8 +301,11 @@ list_set_uadd(struct ip_set *set, void *value, const struct ip_set_ext *ext,
>  	e->set = set;
>  	INIT_LIST_HEAD(&e->list);
>  	list_set_init_extensions(set, ext, e);
> -	if (n)
> +	if (n) {
>  		list_set_replace(set, e, n);
> +		return 0;
> +	}
> +
>  	else if (next)
>  		list_add_tail_rcu(&e->list, &next->list);
>  	else if (prev)
> -- 
> 2.54.0
> 
> 

-- 
E-mail : kadlec@netfilter.org, kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
Address: Wigner Research Centre for Physics
         H-1525 Budapest 114, POB. 49, Hungary

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

[Florian Westphal]

Jozsef Kadlecsik <kadlec@netfilter.org> wrote:
> Acked-by: Jozsef Kadlecsik <kadlec@netfilter.org>

Thanks for reviewing!

> > +	if (n) {
> >  		list_set_replace(set, e, n);
> > +		return 0;
> > +	}
> > +
> >  	else if (next)
> >  		list_add_tail_rcu(&e->list, &next->list);

While it makes no difference from correctness, the
above looks very weird, so I propose to turn the else if into plain if (next).

(Did not spot it before sending...).

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

[Pablo Neira Ayuso]

On Sun, Aug 09, 2026 at 04:16:32PM +0200, Florian Westphal wrote:
> Jozsef Kadlecsik <kadlec@netfilter.org> wrote:
> > Acked-by: Jozsef Kadlecsik <kadlec@netfilter.org>
> 
> Thanks for reviewing!
> 
> > > +	if (n) {
> > >  		list_set_replace(set, e, n);
> > > +		return 0;
> > > +	}
> > > +
> > >  	else if (next)
> > >  		list_add_tail_rcu(&e->list, &next->list);
> 
> While it makes no difference from correctness, the
> above looks very weird, so I propose to turn the else if into plain if (next).
> 
> (Did not spot it before sending...).

For the record: I mangled the patch to remove this else when applying
this. No need to resend.