| Message ID | 20240207233726.331592-6-pablo@netfilter.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,01/13] netfilter: nft_compat: narrow down revision to unsigned 8-bits | expand |
On 08.02.24 00:37, Pablo Neira Ayuso wrote: > From: Jozsef Kadlecsik <kadlec@netfilter.org> > > The patch fdb8e12cc2cc ("netfilter: ipset: fix performance regression > in swap operation") missed to add the calls to gc cancellations > at the error path of create operations and at module unload. Also, > because the half of the destroy operations now executed by a > function registered by call_rcu(), neither NFNL_SUBSYS_IPSET mutex > or rcu read lock is held and therefore the checking of them results > false warnings. > > Reported-by: syzbot+52bbc0ad036f6f0d4a25@syzkaller.appspotmail.com > Reported-by: Brad Spengler <spender@grsecurity.net> > Reported-by: Стас Ничипорович <stasn77@gmail.com> > Fixes: fdb8e12cc2cc ("netfilter: ipset: fix performance regression in swap operation") FWIW, in case anyone cares: that afaics should be Fixes: 97f7cf1cd80e ("netfilter: ipset: fix performance regression in swap operation") instead, as noted yesterday elsewhere[1]. Ciao, Thorsten [1] https://lore.kernel.org/all/07cf1cf8-825e-47b9-9837-f91ae958dd6b@leemhuis.info/
Hi, On Thu, 2024-02-08 at 06:48 +0100, Thorsten Leemhuis wrote: > On 08.02.24 00:37, Pablo Neira Ayuso wrote: > > From: Jozsef Kadlecsik <kadlec@netfilter.org> > > > > The patch fdb8e12cc2cc ("netfilter: ipset: fix performance regression > > in swap operation") missed to add the calls to gc cancellations > > at the error path of create operations and at module unload. Also, > > because the half of the destroy operations now executed by a > > function registered by call_rcu(), neither NFNL_SUBSYS_IPSET mutex > > or rcu read lock is held and therefore the checking of them results > > false warnings. > > > > Reported-by: syzbot+52bbc0ad036f6f0d4a25@syzkaller.appspotmail.com > > Reported-by: Brad Spengler <spender@grsecurity.net> > > Reported-by: Стас Ничипорович <stasn77@gmail.com> > > Fixes: fdb8e12cc2cc ("netfilter: ipset: fix performance regression in swap operation") > > FWIW, in case anyone cares: that afaics should be > > Fixes: 97f7cf1cd80e ("netfilter: ipset: fix performance regression in swap operation") > > instead, as noted yesterday elsewhere[1]. > > Ciao, Thorsten > > [1] https://lore.kernel.org/all/07cf1cf8-825e-47b9-9837-f91ae958dd6b@leemhuis.info/ I think it would be better to update the commit message, to help stable teams. Unless you absolutely need series in today PR, could you please send out a v2? Note that if v2 comes soon enough it can still land into the mentioned PR. Thanks, Paolo
On Thu, 2024-02-08 at 00:37 +0100, Pablo Neira Ayuso wrote: > From: Jozsef Kadlecsik <kadlec@netfilter.org> > > The patch fdb8e12cc2cc ("netfilter: ipset: fix performance regression > in swap operation") missed to add the calls to gc cancellations > at the error path of create operations and at module unload. Also, > because the half of the destroy operations now executed by a > function registered by call_rcu(), neither NFNL_SUBSYS_IPSET mutex > or rcu read lock is held and therefore the checking of them results > false warnings. > > Reported-by: syzbot+52bbc0ad036f6f0d4a25@syzkaller.appspotmail.com > Reported-by: Brad Spengler <spender@grsecurity.net> > Reported-by: Стас Ничипорович <stasn77@gmail.com> > Fixes: fdb8e12cc2cc ("netfilter: ipset: fix performance regression in swap operation") > Tested-by: Brad Spengler <spender@grsecurity.net> > Tested-by: Стас Ничипорович <stasn77@gmail.com> > Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > net/netfilter/ipset/ip_set_core.c | 2 ++ > net/netfilter/ipset/ip_set_hash_gen.h | 4 ++-- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c > index bcaad9c009fe..3184cc6be4c9 100644 > --- a/net/netfilter/ipset/ip_set_core.c > +++ b/net/netfilter/ipset/ip_set_core.c > @@ -1154,6 +1154,7 @@ static int ip_set_create(struct sk_buff *skb, const struct nfnl_info *info, > return ret; > > cleanup: > + set->variant->cancel_gc(set); > set->variant->destroy(set); > put_out: > module_put(set->type->me); > @@ -2378,6 +2379,7 @@ ip_set_net_exit(struct net *net) > set = ip_set(inst, i); > if (set) { > ip_set(inst, i) = NULL; > + set->variant->cancel_gc(set); > ip_set_destroy_set(set); > } > } > diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h > index 1136510521a8..cfa5eecbe393 100644 > --- a/net/netfilter/ipset/ip_set_hash_gen.h > +++ b/net/netfilter/ipset/ip_set_hash_gen.h > @@ -432,7 +432,7 @@ mtype_ahash_destroy(struct ip_set *set, struct htable *t, bool ext_destroy) > u32 i; > > for (i = 0; i < jhash_size(t->htable_bits); i++) { > - n = __ipset_dereference(hbucket(t, i)); AFAICS __ipset_dereference() should not trigger any warning, as it boils down to rcu_dereference_protected() > + n = hbucket(t, i); This statement instead triggers a sparse warning. > if (!n) > continue; > if (set->extensions & IPSET_EXT_DESTROY && ext_destroy) > @@ -452,7 +452,7 @@ mtype_destroy(struct ip_set *set) > struct htype *h = set->data; > struct list_head *l, *lt; > > - mtype_ahash_destroy(set, ipset_dereference_nfnl(h->table), true); > + mtype_ahash_destroy(set, h->table, true); The same here. What about using always __ipset_dereference()? Cheers, Paolo
Hi Paolo, Working on v2 series, it should be ready in before noon. On Thu, Feb 08, 2024 at 09:50:55AM +0100, Paolo Abeni wrote: > Hi, > > On Thu, 2024-02-08 at 06:48 +0100, Thorsten Leemhuis wrote: > > On 08.02.24 00:37, Pablo Neira Ayuso wrote: > > > From: Jozsef Kadlecsik <kadlec@netfilter.org> > > > > > > The patch fdb8e12cc2cc ("netfilter: ipset: fix performance regression > > > in swap operation") missed to add the calls to gc cancellations > > > at the error path of create operations and at module unload. Also, > > > because the half of the destroy operations now executed by a > > > function registered by call_rcu(), neither NFNL_SUBSYS_IPSET mutex > > > or rcu read lock is held and therefore the checking of them results > > > false warnings. > > > > > > Reported-by: syzbot+52bbc0ad036f6f0d4a25@syzkaller.appspotmail.com > > > Reported-by: Brad Spengler <spender@grsecurity.net> > > > Reported-by: Стас Ничипорович <stasn77@gmail.com> > > > Fixes: fdb8e12cc2cc ("netfilter: ipset: fix performance regression in swap operation") > > > > FWIW, in case anyone cares: that afaics should be > > > > Fixes: 97f7cf1cd80e ("netfilter: ipset: fix performance regression in swap operation") > > > > instead, as noted yesterday elsewhere[1]. > > > > Ciao, Thorsten > > > > [1] https://lore.kernel.org/all/07cf1cf8-825e-47b9-9837-f91ae958dd6b@leemhuis.info/ > > I think it would be better to update the commit message, to help stable > teams. > > Unless you absolutely need series in today PR, could you please send > out a v2? Note that if v2 comes soon enough it can still land into the > mentioned PR. > > Thanks, > > Paolo >
On Thu, 8 Feb 2024, Paolo Abeni wrote: > On Thu, 2024-02-08 at 00:37 +0100, Pablo Neira Ayuso wrote: > > From: Jozsef Kadlecsik <kadlec@netfilter.org> > > > > The patch fdb8e12cc2cc ("netfilter: ipset: fix performance regression > > in swap operation") missed to add the calls to gc cancellations > > at the error path of create operations and at module unload. Also, > > because the half of the destroy operations now executed by a > > function registered by call_rcu(), neither NFNL_SUBSYS_IPSET mutex > > or rcu read lock is held and therefore the checking of them results > > false warnings. > > > > Reported-by: syzbot+52bbc0ad036f6f0d4a25@syzkaller.appspotmail.com > > Reported-by: Brad Spengler <spender@grsecurity.net> > > Reported-by: Стас Ничипорович <stasn77@gmail.com> > > Fixes: fdb8e12cc2cc ("netfilter: ipset: fix performance regression in swap operation") > > Tested-by: Brad Spengler <spender@grsecurity.net> > > Tested-by: Стас Ничипорович <stasn77@gmail.com> > > Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > --- > > net/netfilter/ipset/ip_set_core.c | 2 ++ > > net/netfilter/ipset/ip_set_hash_gen.h | 4 ++-- > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c > > index bcaad9c009fe..3184cc6be4c9 100644 > > --- a/net/netfilter/ipset/ip_set_core.c > > +++ b/net/netfilter/ipset/ip_set_core.c > > @@ -1154,6 +1154,7 @@ static int ip_set_create(struct sk_buff *skb, const struct nfnl_info *info, > > return ret; > > > > cleanup: > > + set->variant->cancel_gc(set); > > set->variant->destroy(set); > > put_out: > > module_put(set->type->me); > > @@ -2378,6 +2379,7 @@ ip_set_net_exit(struct net *net) > > set = ip_set(inst, i); > > if (set) { > > ip_set(inst, i) = NULL; > > + set->variant->cancel_gc(set); > > ip_set_destroy_set(set); > > } > > } > > diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h > > index 1136510521a8..cfa5eecbe393 100644 > > --- a/net/netfilter/ipset/ip_set_hash_gen.h > > +++ b/net/netfilter/ipset/ip_set_hash_gen.h > > @@ -432,7 +432,7 @@ mtype_ahash_destroy(struct ip_set *set, struct htable *t, bool ext_destroy) > > u32 i; > > > > for (i = 0; i < jhash_size(t->htable_bits); i++) { > > - n = __ipset_dereference(hbucket(t, i)); > > AFAICS __ipset_dereference() should not trigger any warning, as it > boils down to rcu_dereference_protected() The destroy operation is split into two parts. The second one is called now via call_rcu() when neither NFNL_SUBSYS_IPSET is held nor it is an rcu protected area, which conditions are checked by __ipset_dereference(). So the original lines above and below would generate suspicious RCU usage warnings. That's why I removed these two __ipset_dereference() calls. > > + n = hbucket(t, i); > > This statement instead triggers a sparse warning. Yeah, that's due to 'struct hbucket *' != 'struct hbucket __rcu *'. I'll send a patch to fix it. Thanks! Best regards, Jozsef > > if (!n) > > continue; > > if (set->extensions & IPSET_EXT_DESTROY && ext_destroy) > > @@ -452,7 +452,7 @@ mtype_destroy(struct ip_set *set) > > struct htype *h = set->data; > > struct list_head *l, *lt; > > > > - mtype_ahash_destroy(set, ipset_dereference_nfnl(h->table), true); > > + mtype_ahash_destroy(set, h->table, true); > > The same here. What about using always __ipset_dereference()? > > Cheers, > > Paolo > > >
diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c index bcaad9c009fe..3184cc6be4c9 100644 --- a/net/netfilter/ipset/ip_set_core.c +++ b/net/netfilter/ipset/ip_set_core.c @@ -1154,6 +1154,7 @@ static int ip_set_create(struct sk_buff *skb, const struct nfnl_info *info, return ret; cleanup: + set->variant->cancel_gc(set); set->variant->destroy(set); put_out: module_put(set->type->me); @@ -2378,6 +2379,7 @@ ip_set_net_exit(struct net *net) set = ip_set(inst, i); if (set) { ip_set(inst, i) = NULL; + set->variant->cancel_gc(set); ip_set_destroy_set(set); } } diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h index 1136510521a8..cfa5eecbe393 100644 --- a/net/netfilter/ipset/ip_set_hash_gen.h +++ b/net/netfilter/ipset/ip_set_hash_gen.h @@ -432,7 +432,7 @@ mtype_ahash_destroy(struct ip_set *set, struct htable *t, bool ext_destroy) u32 i; for (i = 0; i < jhash_size(t->htable_bits); i++) { - n = __ipset_dereference(hbucket(t, i)); + n = hbucket(t, i); if (!n) continue; if (set->extensions & IPSET_EXT_DESTROY && ext_destroy) @@ -452,7 +452,7 @@ mtype_destroy(struct ip_set *set) struct htype *h = set->data; struct list_head *l, *lt; - mtype_ahash_destroy(set, ipset_dereference_nfnl(h->table), true); + mtype_ahash_destroy(set, h->table, true); list_for_each_safe(l, lt, &h->ad) { list_del(l); kfree(l);