* [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).