Re: [syzbot] [netfilter?] WARNING in nft_map_deactivate

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> This is an interval set allocating a new array that is allocated with
> GFP_KERNEL (rbtree/pipapo follow a similar approach), I suspect fault
> injection is making this memory allocation fail.
> 
> Then, this WARN_ON_ONCE below triggers:
> 
> static void nft_map_deactivate(const struct nft_ctx *ctx, struct nft_set *set)
> {               
>         struct nft_set_iter iter = {
>                 .genmask        = nft_genmask_next(ctx->net),
>                 .type           = NFT_ITER_UPDATE,
>                 .fn             = nft_mapelem_deactivate,
>         };
>         
>         set->ops->walk(ctx, set, &iter);
>         WARN_ON_ONCE(iter.err);
> 
> For the traceback below, it should be possible to add NFT_ITER_RELEASE
> to skip the allocation.

Agreed.

> But there are other paths where this can happen too, I am looking into
> making these nft_map_activate/nft_map_deactivate function never fail
> in the second stage, this is the idea:
> 
> - For anonymous sets, the allocation (clone) can be skipped since they
>   are immutable.

Right, there is no need to clone anon sets.

> - For non-anonymous sets, add a .clone interface to nft_set_ops so
>   the clone is not done from the 

Yes, that would help.  However, I'm not sure its needed.
For 2nd stage that cannot fail, all inserts/deletes already completed,
i.e. there is already a clone present that is re-used, no new clone
happens.

The only exception that I can see is the "release" case you mention.
In that case, we had no delete or add, so the clone pointer is NULL.

With a new iter type we could detect this and just re-use the live
copy instead of making an unnecessary copy.

Or did I miss something?