On Mon, Mar 04, 2024 at 07:22:27PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Fixes: 5f68718b34a5 ("netfilter: nf_tables: GC transaction API to avoid race with control plane")
> > Reported-by: Mingi Cho <mgcho.minic@xxxxxxxxx>
> > Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> > ---
> > 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.
