netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH nf 0/3] netfilter: nf_tables: fix use counter for rule
@ 2019-12-18 14:59 wenxu
  2019-12-18 14:59 ` [PATCH nf 1/3] netfilter: nf_tables: fix rule release in err path wenxu
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: wenxu @ 2019-12-18 14:59 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH nf 1/3] netfilter: nf_tables: fix rule release in err path
  2019-12-18 14:59 [PATCH nf 0/3] netfilter: nf_tables: fix use counter for rule wenxu
@ 2019-12-18 14:59 ` wenxu
  2019-12-19 23:43   ` Pablo Neira Ayuso
  2019-12-18 14:59 ` [PATCH nf 2/3] netfilter: nf_tables: fix miss activate operation in the wenxu
  2019-12-18 14:59 ` [PATCH nf 3/3] netfilter: nf_tables: fix miss dec set use counter in the nf_tables_destroy_set wenxu
  2 siblings, 1 reply; 10+ messages in thread
From: wenxu @ 2019-12-18 14:59 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH nf 2/3] netfilter: nf_tables: fix miss activate operation in the
  2019-12-18 14:59 [PATCH nf 0/3] netfilter: nf_tables: fix use counter for rule wenxu
  2019-12-18 14:59 ` [PATCH nf 1/3] netfilter: nf_tables: fix rule release in err path wenxu
@ 2019-12-18 14:59 ` wenxu
  2019-12-19 23:55   ` Pablo Neira Ayuso
  2019-12-18 14:59 ` [PATCH nf 3/3] netfilter: nf_tables: fix miss dec set use counter in the nf_tables_destroy_set wenxu
  2 siblings, 1 reply; 10+ messages in thread
From: wenxu @ 2019-12-18 14:59 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH nf 3/3] netfilter: nf_tables: fix miss dec set use counter in the nf_tables_destroy_set
  2019-12-18 14:59 [PATCH nf 0/3] netfilter: nf_tables: fix use counter for rule wenxu
  2019-12-18 14:59 ` [PATCH nf 1/3] netfilter: nf_tables: fix rule release in err path wenxu
  2019-12-18 14:59 ` [PATCH nf 2/3] netfilter: nf_tables: fix miss activate operation in the wenxu
@ 2019-12-18 14:59 ` wenxu
  2019-12-19 23:56   ` Pablo Neira Ayuso
  2 siblings, 1 reply; 10+ messages in thread
From: wenxu @ 2019-12-18 14:59 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 1/3] netfilter: nf_tables: fix rule release in err path
  2019-12-18 14:59 ` [PATCH nf 1/3] netfilter: nf_tables: fix rule release in err path wenxu
@ 2019-12-19 23:43   ` Pablo Neira Ayuso
  0 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2019-12-19 23:43 UTC (permalink / raw)
  To: wenxu; +Cc: netfilter-devel

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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 2/3] netfilter: nf_tables: fix miss activate operation in the
  2019-12-18 14:59 ` [PATCH nf 2/3] netfilter: nf_tables: fix miss activate operation in the wenxu
@ 2019-12-19 23:55   ` Pablo Neira Ayuso
  2019-12-20  3:42     ` wenxu
  0 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2019-12-19 23:55 UTC (permalink / raw)
  To: wenxu; +Cc: netfilter-devel

[-- 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);
 }
 

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 3/3] netfilter: nf_tables: fix miss dec set use counter in the nf_tables_destroy_set
  2019-12-18 14:59 ` [PATCH nf 3/3] netfilter: nf_tables: fix miss dec set use counter in the nf_tables_destroy_set wenxu
@ 2019-12-19 23:56   ` Pablo Neira Ayuso
  2019-12-20  9:12     ` wenxu
  0 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2019-12-19 23:56 UTC (permalink / raw)
  To: wenxu; +Cc: netfilter-devel

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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 2/3] netfilter: nf_tables: fix miss activate operation in the
  2019-12-19 23:55   ` Pablo Neira Ayuso
@ 2019-12-20  3:42     ` wenxu
  2019-12-20  9:04       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 10+ messages in thread
From: wenxu @ 2019-12-20  3:42 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel


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





^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 2/3] netfilter: nf_tables: fix miss activate operation in the
  2019-12-20  3:42     ` wenxu
@ 2019-12-20  9:04       ` Pablo Neira Ayuso
  0 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2019-12-20  9:04 UTC (permalink / raw)
  To: wenxu; +Cc: netfilter-devel

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 3/3] netfilter: nf_tables: fix miss dec set use counter in the nf_tables_destroy_set
  2019-12-19 23:56   ` Pablo Neira Ayuso
@ 2019-12-20  9:12     ` wenxu
  0 siblings, 0 replies; 10+ messages in thread
From: wenxu @ 2019-12-20  9:12 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-12-20  9:12 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-18 14:59 [PATCH nf 0/3] netfilter: nf_tables: fix use counter for rule wenxu
2019-12-18 14:59 ` [PATCH nf 1/3] netfilter: nf_tables: fix rule release in err path wenxu
2019-12-19 23:43   ` Pablo Neira Ayuso
2019-12-18 14:59 ` [PATCH nf 2/3] netfilter: nf_tables: fix miss activate operation in the wenxu
2019-12-19 23:55   ` Pablo Neira Ayuso
2019-12-20  3:42     ` wenxu
2019-12-20  9:04       ` Pablo Neira Ayuso
2019-12-18 14:59 ` [PATCH nf 3/3] netfilter: nf_tables: fix miss dec set use counter in the nf_tables_destroy_set wenxu
2019-12-19 23:56   ` Pablo Neira Ayuso
2019-12-20  9:12     ` wenxu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).