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

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

[Pablo Neira Ayuso]

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.

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;
 	}
 }
 EXPORT_SYMBOL_GPL(nf_tables_deactivate_set);
-- 
2.30.2

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

[Florian Westphal]

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.

> 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?

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

[Pablo Neira Ayuso]

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.