[PATCH nf 0/3] netfilter: nf_tables: fix use counter for rule

[PATCH nf 0/3] netfilter: nf_tables: fix use counter for rule

[wenxu]

From: wenxu <wenxu@ucloud.cn>

wenxu (3):
  netfilter: nf_tables: fix rule release in err path
  netfilter: nf_tables: fix miss activate operation in the
  netfilter: nf_tables: fix miss dec set use counter in the
    nf_tables_destroy_set

 net/netfilter/nf_tables_api.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[PATCH nf 1/3] netfilter: nf_tables: fix rule release in err path

[wenxu]

From: wenxu <wenxu@ucloud.cn>

The err2 failed path in nf_tables_newrule fail err2  should only destory this new rule
without deactivate it. Because the rule is not been activated.

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 net/netfilter/nf_tables_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index a8caf73..27e6a6f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3130,7 +3130,7 @@ static int nf_tables_newrule(struct net *net, struct sock *nlsk,
 
 	return 0;
 err2:
-	nf_tables_rule_release(&ctx, rule);
+	nf_tables_rule_destroy(&ctx, rule);
 err1:
 	for (i = 0; i < n; i++) {
 		if (info[i].ops) {
-- 
1.8.3.1

Re: [PATCH nf 1/3] netfilter: nf_tables: fix rule release in err path

[Pablo Neira Ayuso]

On Wed, Dec 18, 2019 at 10:59:11PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> The err2 failed path in nf_tables_newrule fail err2  should only destory this new rule
> without deactivate it. Because the rule is not been activated.
> 
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
>  net/netfilter/nf_tables_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index a8caf73..27e6a6f 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -3130,7 +3130,7 @@ static int nf_tables_newrule(struct net *net, struct sock *nlsk,
>  
>  	return 0;
>  err2:
> -	nf_tables_rule_release(&ctx, rule);
> +	nf_tables_rule_destroy(&ctx, rule);

This is not correct, the rule might have a reference to a chain jump,
nft_data_release() needs to be called in that case.

>  err1:
>  	for (i = 0; i < n; i++) {
>  		if (info[i].ops) {
> -- 
> 1.8.3.1
> 

[PATCH nf 2/3] netfilter: nf_tables: fix miss activate operation in the

[wenxu]

From: wenxu <wenxu@ucloud.cn>

nf_tables_commit for NFT_MSG_NEWRULE

The new create rule should be activated in the nf_tables_commit.

create a flowtable:
nft add table firewall
nft add flowtable firewall fb1 { hook ingress priority 2 \; devices = { tun1, mlx_pf0vf0 } \; }
nft add chain firewall ftb-all {type filter hook forward priority 0 \; policy accept \; }
nft add rule firewall ftb-all ct zone 1 ip protocol tcp flow offload @fb1
nft add rule firewall ftb-all ct zone 1 ip protocol udp flow offload @fb1

delete the related rule:
nft delete chain firewall ftb-all

The flowtable can be deleted
nft delete flowtable firewall fb1

But failed with: Device is busy

The nf_flowtable->use is not zero for no activated operation.

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 net/netfilter/nf_tables_api.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 27e6a6f..174b362 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -7101,6 +7101,7 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 			nf_tables_rule_notify(&trans->ctx,
 					      nft_trans_rule(trans),
 					      NFT_MSG_NEWRULE);
+			nft_rule_expr_activate(&trans->ctx, nft_trans_rule(trans));
 			nft_trans_destroy(trans);
 			break;
 		case NFT_MSG_DELRULE:
-- 
1.8.3.1

Re: [PATCH nf 2/3] netfilter: nf_tables: fix miss activate operation in the

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

On Wed, Dec 18, 2019 at 10:59:12PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> nf_tables_commit for NFT_MSG_NEWRULE
> 
> The new create rule should be activated in the nf_tables_commit.
> 
> create a flowtable:
> nft add table firewall
> nft add flowtable firewall fb1 { hook ingress priority 2 \; devices = { tun1, mlx_pf0vf0 } \; }
> nft add chain firewall ftb-all {type filter hook forward priority 0 \; policy accept \; }
> nft add rule firewall ftb-all ct zone 1 ip protocol tcp flow offload @fb1
> nft add rule firewall ftb-all ct zone 1 ip protocol udp flow offload @fb1
> 
> delete the related rule:
> nft delete chain firewall ftb-all
> 
> The flowtable can be deleted
> nft delete flowtable firewall fb1
> 
> But failed with: Device is busy
> 
> The nf_flowtable->use is not zero for no activated operation.

This is correct.

> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
>  net/netfilter/nf_tables_api.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 27e6a6f..174b362 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -7101,6 +7101,7 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
>  			nf_tables_rule_notify(&trans->ctx,
>  					      nft_trans_rule(trans),
>  					      NFT_MSG_NEWRULE);
> +			nft_rule_expr_activate(&trans->ctx, nft_trans_rule(trans));

I don't think this fix is correct, probably this patch?

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 460 bytes --]

diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
index dd82ff2ee19f..f1280321b129 100644
--- a/net/netfilter/nft_flow_offload.c
+++ b/net/netfilter/nft_flow_offload.c
@@ -186,6 +186,9 @@ static void nft_flow_offload_deactivate(const struct nft_ctx *ctx,
 {
 	struct nft_flow_offload *priv = nft_expr_priv(expr);
 
+	if (phase == NFT_TRANS_COMMIT)
+		return;
+
 	nf_tables_deactivate_flowtable(ctx, priv->flowtable, phase);
 }
 

Re: [PATCH nf 2/3] netfilter: nf_tables: fix miss activate operation in the

[wenxu]

On 12/20/2019 7:55 AM, Pablo Neira Ayuso wrote:
> On Wed, Dec 18, 2019 at 10:59:12PM +0800, wenxu@ucloud.cn wrote:
>> From: wenxu <wenxu@ucloud.cn>
>>
>> nf_tables_commit for NFT_MSG_NEWRULE
>>
>> The new create rule should be activated in the nf_tables_commit.
>>
>> create a flowtable:
>> nft add table firewall
>> nft add flowtable firewall fb1 { hook ingress priority 2 \; devices = { tun1, mlx_pf0vf0 } \; }
>> nft add chain firewall ftb-all {type filter hook forward priority 0 \; policy accept \; }
>> nft add rule firewall ftb-all ct zone 1 ip protocol tcp flow offload @fb1
>> nft add rule firewall ftb-all ct zone 1 ip protocol udp flow offload @fb1
>>
>> delete the related rule:
>> nft delete chain firewall ftb-all
>>
>> The flowtable can be deleted
>> nft delete flowtable firewall fb1
>>
>> But failed with: Device is busy
>>
>> The nf_flowtable->use is not zero for no activated operation.
> This is correct.
>
>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>> ---
>>  net/netfilter/nf_tables_api.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> index 27e6a6f..174b362 100644
>> --- a/net/netfilter/nf_tables_api.c
>> +++ b/net/netfilter/nf_tables_api.c
>> @@ -7101,6 +7101,7 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
>>  			nf_tables_rule_notify(&trans->ctx,
>>  					      nft_trans_rule(trans),
>>  					      NFT_MSG_NEWRULE);
>> +			nft_rule_expr_activate(&trans->ctx, nft_trans_rule(trans));
> I don't think this fix is correct, probably this patch?


Maybe your patch is also not correct.    The  nf_tables_deactivate_flowtable already ignore

the NFT_TRANS_COMMIT.

void nf_tables_deactivate_flowtable(const struct nft_ctx *ctx,
                                    struct nft_flowtable *flowtable,
                                    enum nft_trans_phase phase)
{
        switch (phase) {
        case NFT_TRANS_PREPARE:
        case NFT_TRANS_ABORT:
        case NFT_TRANS_RELEASE:
                flowtable->use--;
                /* fall through */
        default:
                return;
        }   
}


Nft_flow_offload  inc the use counter , when delete the rule and dec it in deactivate with phase NFT_TRANS_PREPARE.

So the nft_flow_offload_destroy should not dec the use?

So the patch should be as following.

static void nft_flow_offload_destroy(const struct nft_ctx *ctx,
                                     const struct nft_expr *expr)
{
        struct nft_flow_offload *priv = nft_expr_priv(expr);

-        priv->flowtable->use--;
        nf_ct_netns_put(ctx->net, ctx->family);
}


The rule should be like the following?


Create rule nft_xx_init   inc the use counter,  If the rule create failed just deactivate it

Delete the rule  deactivate dec the use counter, If the rule delete failed just activate it


BR

wenxu

Re: [PATCH nf 2/3] netfilter: nf_tables: fix miss activate operation in the

[Pablo Neira Ayuso]

On Fri, Dec 20, 2019 at 11:42:27AM +0800, wenxu wrote:
> On 12/20/2019 7:55 AM, Pablo Neira Ayuso wrote:
> > On Wed, Dec 18, 2019 at 10:59:12PM +0800, wenxu@ucloud.cn wrote:
> >> From: wenxu <wenxu@ucloud.cn>
> >>
> >> nf_tables_commit for NFT_MSG_NEWRULE
> >>
> >> The new create rule should be activated in the nf_tables_commit.
> >>
> >> create a flowtable:
> >> nft add table firewall
> >> nft add flowtable firewall fb1 { hook ingress priority 2 \; devices = { tun1, mlx_pf0vf0 } \; }
> >> nft add chain firewall ftb-all {type filter hook forward priority 0 \; policy accept \; }
> >> nft add rule firewall ftb-all ct zone 1 ip protocol tcp flow offload @fb1
> >> nft add rule firewall ftb-all ct zone 1 ip protocol udp flow offload @fb1
> >>
> >> delete the related rule:
> >> nft delete chain firewall ftb-all
> >>
> >> The flowtable can be deleted
> >> nft delete flowtable firewall fb1
> >>
> >> But failed with: Device is busy
> >>
> >> The nf_flowtable->use is not zero for no activated operation.
> > This is correct.
> >
> >> Signed-off-by: wenxu <wenxu@ucloud.cn>
[...]
> So the patch should be as following.
> 
> static void nft_flow_offload_destroy(const struct nft_ctx *ctx,
>                                      const struct nft_expr *expr)
> {
>         struct nft_flow_offload *priv = nft_expr_priv(expr);
> 
> -        priv->flowtable->use--;
>         nf_ct_netns_put(ctx->net, ctx->family);
> }
> 
> 
> The rule should be like the following?
> 
> 
> Create rule nft_xx_init   inc the use counter,  If the rule create
> failed just deactivate it
> 
> Delete the rule  deactivate dec the use counter, If the rule delete
> failed just activate it

That looks like the right fix.

Thanks.

[PATCH nf 3/3] netfilter: nf_tables: fix miss dec set use counter in the nf_tables_destroy_set

[wenxu]

From: wenxu <wenxu@ucloud.cn>

In the create rule path nf_tables_bind_set the set->use will inc, and
with the activate operatoion also inc it. In the delete rule patch
deactivate will dec it. So the destroy opertion should also deactivate
it.

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 net/netfilter/nf_tables_api.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 174b362..d71793e 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -4147,8 +4147,10 @@ void nf_tables_deactivate_set(const struct nft_ctx *ctx, struct nft_set *set,
 
 void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set)
 {
-	if (list_empty(&set->bindings) && nft_set_is_anonymous(set))
+	if (list_empty(&set->bindings) && nft_set_is_anonymous(set)) {
+		set->use--;
 		nft_set_destroy(set);
+	}
 }
 EXPORT_SYMBOL_GPL(nf_tables_destroy_set);
 
-- 
1.8.3.1

Re: [PATCH nf 3/3] netfilter: nf_tables: fix miss dec set use counter in the nf_tables_destroy_set

[Pablo Neira Ayuso]

On Wed, Dec 18, 2019 at 10:59:13PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> In the create rule path nf_tables_bind_set the set->use will inc, and
> with the activate operatoion also inc it. In the delete rule patch
> deactivate will dec it. So the destroy opertion should also deactivate
> it.
[...]

Is this a theoretical issue? Thanks.

[...]
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 174b362..d71793e 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -4147,8 +4147,10 @@ void nf_tables_deactivate_set(const struct nft_ctx *ctx, struct nft_set *set,
>  
>  void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set)
>  {
> -	if (list_empty(&set->bindings) && nft_set_is_anonymous(set))
> +	if (list_empty(&set->bindings) && nft_set_is_anonymous(set)) {
> +		set->use--;
>  		nft_set_destroy(set);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(nf_tables_destroy_set);
>  
> -- 
> 1.8.3.1
> 

Re: [PATCH nf 3/3] netfilter: nf_tables: fix miss dec set use counter in the nf_tables_destroy_set

[wenxu]

On 12/20/2019 7:56 AM, Pablo Neira Ayuso wrote:
> On Wed, Dec 18, 2019 at 10:59:13PM +0800, wenxu@ucloud.cn wrote:
>> From: wenxu <wenxu@ucloud.cn>
>>
>> In the create rule path nf_tables_bind_set the set->use will inc, and
>> with the activate operatoion also inc it. In the delete rule patch
>> deactivate will dec it. So the destroy opertion should also deactivate
>> it.
> [...]
>
> Is this a theoretical issue? Thanks.
As we talked in patch2.  Destroy the rule don't need  dec the use counter. So just drop this series. Thx!
> [...]
>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> index 174b362..d71793e 100644
>> --- a/net/netfilter/nf_tables_api.c
>> +++ b/net/netfilter/nf_tables_api.c
>> @@ -4147,8 +4147,10 @@ void nf_tables_deactivate_set(const struct nft_ctx *ctx, struct nft_set *set,
>>  
>>  void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set)
>>  {
>> -	if (list_empty(&set->bindings) && nft_set_is_anonymous(set))
>> +	if (list_empty(&set->bindings) && nft_set_is_anonymous(set)) {
>> +		set->use--;
>>  		nft_set_destroy(set);
>> +	}
>>  }
>>  EXPORT_SYMBOL_GPL(nf_tables_destroy_set);
>>  
>> -- 
>> 1.8.3.1
>>