Re: [PATCH nf] netfilter: nf_tables: mark set as dead when deactivating anonymous set

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Mar 04, 2024 at 07:22:27PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > While the rhashtable set gc runs asynchronously, a race allows it to
> > collect elements from an anonymous set while it is being released from
> > the abort path. This also seems possible from the rule error path.
> > 
> > Mingi Cho originally reported this issue in a different path in 6.1.x
> > with a pipapo set with low timeouts which is not possible upstream since
> > 7395dfacfff6 ("netfilter: nf_tables: use timestamp to check for set
> > element timeout").
> > 
> > Fix this by setting on the dead flag to signal set gc to skip anonymous
> > sets from prepare_error, abort and commit paths.
> 
> This seems to contradict what patch is doing, the flag gets toggled for
> all set types.

Setting set->dead for non-anonymous sets break those that remain in place.
This needs a v2.

> > Cc: stable@vger.kernel.org
> > Fixes: 5f68718b34a5 ("netfilter: nf_tables: GC transaction API to avoid race with control plane")
> > Reported-by: Mingi Cho <mgcho.minic@gmail.com>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  net/netfilter/nf_tables_api.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index ca54d4c23123..26d33ce3b682 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -5513,6 +5513,7 @@ void nf_tables_deactivate_set(const struct nft_ctx *ctx, struct nft_set *set,
> >  			list_del_rcu(&binding->list);
> >  
> >  		nft_use_dec(&set->use);
> > +		set->dead = 1;
> >  		break;
> >  	case NFT_TRANS_PREPARE:
> >  		if (nft_set_is_anonymous(set)) {
> > @@ -5534,6 +5535,7 @@ void nf_tables_deactivate_set(const struct nft_ctx *ctx, struct nft_set *set,
> >  	default:
> >  		nf_tables_unbind_set(ctx, set, binding,
> >  				     phase == NFT_TRANS_COMMIT);
> > +		set->dead = 1;
> 
> Shouldn't that be restricted to nf_tables_unbind_set() anonymous-set
> branch?

Right, thanks for reviewing.

Preparing v2.