[PATCH net] netfilter: ipset: list:set: defer ip_set_put_byindex to RCU callback

[PATCH net] netfilter: ipset: list:set: defer ip_set_put_byindex to RCU callback

[Cen Zhang (Microsoft)]

list_set_del() and list_set_replace() call ip_set_put_byindex() before
call_rcu(), dropping the child set refcount to zero. Meanwhile,
list_set_ktest() on another CPU may have already read e->id under
rcu_read_lock(). A concurrent ip_set_destroy() sees refcount zero,
NULLs ip_set_list[index], and frees the child set. list_set_ktest()
then passes the now-dangling index to ip_set_test(), ip_set_rcu_get()
returns NULL, and BUG_ON(!set) fires at ip_set_core.c:746.

 kernel BUG at net/netfilter/ipset/ip_set_core.c:746!
   ip_set_test+0x329/0x590
   list_set_kadt+0x2a6/0x810
   ip_set_test+0x24f/0x590
   set_match_v1+0x1a9/0x280
   ipt_do_table+0x83d/0x1360
   nf_hook_slow+0xac/0x1e0

Move ip_set_put_byindex() from list_set_del()/list_set_replace() into
the __list_set_del_rcu() callback so the refcount is not decremented
until all RCU readers have finished.

Fixes: 439cd39ea136 ("netfilter: ipset: list:set: Decrease refcount synchronously on deletion and replace")
Reported-by: AutonomousCodeSecurity@microsoft.com
Reported-by: Xiang Mei (Microsoft) <xmei5@asu.edu>
Reported-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
---
 net/netfilter/ipset/ip_set_list_set.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_list_set.c b/net/netfilter/ipset/ip_set_list_set.c
index f070088742d6..aeee0c6c314d 100644
--- a/net/netfilter/ipset/ip_set_list_set.c
+++ b/net/netfilter/ipset/ip_set_list_set.c
@@ -144,7 +144,9 @@ __list_set_del_rcu(struct rcu_head * rcu)
 {
 	struct set_elem *e = container_of(rcu, struct set_elem, rcu);
 	struct ip_set *set = e->set;
+	struct list_set *map = set->data;
 
+	ip_set_put_byindex(map->net, e->id);
 	ip_set_ext_destroy(set, e);
 	kfree(e);
 }
@@ -152,21 +154,15 @@ __list_set_del_rcu(struct rcu_head * rcu)
 static void
 list_set_del(struct ip_set *set, struct set_elem *e)
 {
-	struct list_set *map = set->data;
-
 	set->elements--;
 	list_del_rcu(&e->list);
-	ip_set_put_byindex(map->net, e->id);
 	call_rcu(&e->rcu, __list_set_del_rcu);
 }
 
 static void
 list_set_replace(struct ip_set *set, struct set_elem *e, struct set_elem *old)
 {
-	struct list_set *map = set->data;
-
 	list_replace_rcu(&old->list, &e->list);
-	ip_set_put_byindex(map->net, old->id);
 	call_rcu(&old->rcu, __list_set_del_rcu);
 }
 
-- 
2.52.0

Re: [PATCH net] netfilter: ipset: list:set: defer ip_set_put_byindex to RCU callback

[luoxuanqiang]

在 2026/8/20 09:06, Cen Zhang (Microsoft) 写道:
> list_set_del() and list_set_replace() call ip_set_put_byindex() before
> call_rcu(), dropping the child set refcount to zero. Meanwhile,
> list_set_ktest() on another CPU may have already read e->id under
> rcu_read_lock(). A concurrent ip_set_destroy() sees refcount zero,
> NULLs ip_set_list[index], and frees the child set. list_set_ktest()
> then passes the now-dangling index to ip_set_test(), ip_set_rcu_get()
> returns NULL, and BUG_ON(!set) fires at ip_set_core.c:746.
>
>   kernel BUG at net/netfilter/ipset/ip_set_core.c:746!
>     ip_set_test+0x329/0x590
>     list_set_kadt+0x2a6/0x810
>     ip_set_test+0x24f/0x590
>     set_match_v1+0x1a9/0x280
>     ipt_do_table+0x83d/0x1360
>     nf_hook_slow+0xac/0x1e0
>
> Move ip_set_put_byindex() from list_set_del()/list_set_replace() into
> the __list_set_del_rcu() callback so the refcount is not decremented
> until all RCU readers have finished.
>
> Fixes: 439cd39ea136 ("netfilter: ipset: list:set: Decrease refcount synchronously on deletion and replace")
> Reported-by: AutonomousCodeSecurity@microsoft.com
> Reported-by: Xiang Mei (Microsoft) <xmei5@asu.edu>
> Reported-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> ---
>   net/netfilter/ipset/ip_set_list_set.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/net/netfilter/ipset/ip_set_list_set.c b/net/netfilter/ipset/ip_set_list_set.c
> index f070088742d6..aeee0c6c314d 100644
> --- a/net/netfilter/ipset/ip_set_list_set.c
> +++ b/net/netfilter/ipset/ip_set_list_set.c
> @@ -144,7 +144,9 @@ __list_set_del_rcu(struct rcu_head * rcu)
>   {
>   	struct set_elem *e = container_of(rcu, struct set_elem, rcu);
>   	struct ip_set *set = e->set;
> +	struct list_set *map = set->data;
>   
> +	ip_set_put_byindex(map->net, e->id);
>   	ip_set_ext_destroy(set, e);
>   	kfree(e);
>   }
> @@ -152,21 +154,15 @@ __list_set_del_rcu(struct rcu_head * rcu)
>   static void
>   list_set_del(struct ip_set *set, struct set_elem *e)
>   {
> -	struct list_set *map = set->data;
> -
>   	set->elements--;
>   	list_del_rcu(&e->list);
> -	ip_set_put_byindex(map->net, e->id);
>   	call_rcu(&e->rcu, __list_set_del_rcu);
>   }
>   
>   static void
>   list_set_replace(struct ip_set *set, struct set_elem *e, struct set_elem *old)
>   {
> -	struct list_set *map = set->data;
> -
>   	list_replace_rcu(&old->list, &e->list);
> -	ip_set_put_byindex(map->net, old->id);
>   	call_rcu(&old->rcu, __list_set_del_rcu);
>   }
>   

Thanks for the patch.

But this is effectively a revert of 439cd39ea136 and brings back the
problem it fixed: within the grace period after the del, userspace still
sees a stale refcount, and destroy/rename are briefly rejected as well.

Please take a look at that commit's message first.

Re: [PATCH net] netfilter: ipset: list:set: defer ip_set_put_byindex to RCU callback

[Cen Zhang (Microsoft)]

Hi Xuanqiang,

Thanks for the review. You're right that this effectively restores the
stale-refcount window fixed by 439cd39ea136. I was aware of that
trade-off, but focused first on preventing the kernel BUG that can be
triggered by an unprivileged user. I agree that the final fix should
address both issues.

I'll rework the fix and send a v2.

Suggestions on the preferred approach are welcome.

Thanks,
Cen