| Message ID | 20260820010617.46851-1-blbllhy@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net] netfilter: ipset: list:set: defer ip_set_put_byindex to RCU callback | expand |
在 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.
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
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); }
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(-)