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@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Reported-by: Brad Spengler <spender@xxxxxxxxxxxxxx>
> > Reported-by: Стас Ничипорович <stasn77@xxxxxxxxx>
> > Fixes: fdb8e12cc2cc ("netfilter: ipset: fix performance regression in swap operation")
> > Tested-by: Brad Spengler <spender@xxxxxxxxxxxxxx>
> > Tested-by: Стас Ничипорович <stasn77@xxxxxxxxx>
> > Signed-off-by: Jozsef Kadlecsik <kadlec@xxxxxxxxxxxxx>
> > Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> > ---
> > 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
>
>
>
--
E-mail : kadlec@xxxxxxxxxxxxxxxxx, kadlecsik.jozsef@xxxxxxxxx
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
H-1525 Budapest 114, POB. 49, Hungary
