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()
> + 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
