[PATCH nf] netfilter: nft_flow_offload: fix unnecessary use counter decrease in destory

[PATCH nf] netfilter: nft_flow_offload: fix unnecessary use counter decrease in destory

[wenxu]

From: wenxu <wenxu@ucloud.cn>

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 and overflow for unnecessary use counter
decrease..

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 net/netfilter/nft_flow_offload.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
index dd82ff2..b70b489 100644
--- a/net/netfilter/nft_flow_offload.c
+++ b/net/netfilter/nft_flow_offload.c
@@ -200,9 +200,6 @@ static void nft_flow_offload_activate(const struct nft_ctx *ctx,
 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);
 }
 
-- 
1.8.3.1

Re: [PATCH nf] netfilter: nft_flow_offload: fix unnecessary use counter decrease in destory

[wenxu]

Hi pablo,


How about this patch?


BR

wenxu

在 2019/12/20 17:08, wenxu@ucloud.cn 写道:
> From: wenxu <wenxu@ucloud.cn>
>
> 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 and overflow for unnecessary use counter
> decrease..
>
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
>   net/netfilter/nft_flow_offload.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
> index dd82ff2..b70b489 100644
> --- a/net/netfilter/nft_flow_offload.c
> +++ b/net/netfilter/nft_flow_offload.c
> @@ -200,9 +200,6 @@ static void nft_flow_offload_activate(const struct nft_ctx *ctx,
>   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);
>   }
>   

Re: [PATCH nf] netfilter: nft_flow_offload: fix unnecessary use counter decrease in destory

[Pablo Neira Ayuso]

On Mon, Dec 30, 2019 at 09:25:36PM +0800, wenxu wrote:
> Hi pablo,
> 
> How about this patch?

This test still fails after a second run with this patch:

./run-tests.sh testcases/flowtable/0009deleteafterflush_0
I: using nft binary ./../../src/nft

W: [FAILED]     testcases/flowtable/0009deleteafterflush_0: got 1
Error: Could not process rule: Device or resource busy
delete flowtable x f

Re: [PATCH nf] netfilter: nft_flow_offload: fix unnecessary use counter decrease in destory

[wenxu]

在 2019/12/31 4:02, Pablo Neira Ayuso 写道:
> On Mon, Dec 30, 2019 at 09:25:36PM +0800, wenxu wrote:
>> Hi pablo,
>>
>> How about this patch?
> This test still fails after a second run with this patch:
>
> ./run-tests.sh testcases/flowtable/0009deleteafterflush_0
> I: using nft binary ./../../src/nft
>
> W: [FAILED]     testcases/flowtable/0009deleteafterflush_0: got 1
> Error: Could not process rule: Device or resource busy
> delete flowtable x f

Hi pablo,


I did the same test for testcase 0009deleteafterflush_0, It is okay even 
there is no this patch in my tree.

++ which nft
+ NFT=/usr/sbin/nft
+ /usr/sbin/nft add table x
+ /usr/sbin/nft add chain x y
+ /usr/sbin/nft add flowtable x f '{' hook ingress priority '0;' devices 
= '{' lo '};}'
+ /usr/sbin/nft add rule x y flow add @f
+ /usr/sbin/nft flush chain x y

+ /usr/sbin/nft delete flowtable x f


This patch fix the problem that there are nft_flow_offload rules,  when 
flush the rules or chain which will lead the use counter double decrease 
and overflow.

nft add rule firewall ftb-all ct zone 1 ip protocol tcp flow offload @f
nft add rule firewall ftb-all ct zone 1 ip protocol udp flow offload @f

This testcase does not have any nft_flow_offload rules. So this testcase 
don't cover the problem I want to fixes.


Ps:

  I test the nf-next tree, this testcase have the problem, I think it 
should be another new problem. I will check it.

Re: [PATCH nf] netfilter: nft_flow_offload: fix unnecessary use counter decrease in destory

[wenxu]

Hi Pablo,


I found this testcase already contain the rules.


I can reproduce it with your nf branch. 

# uname -r
5.5.0-rc2+


# cat testcases/flowtable/0009deleteafterflush_0:

#!/bin/bash

set -e

$NFT add table x
$NFT add chain x y
$NFT add flowtable x f { hook ingress priority 0\; devices = { lo }\;}
$NFT add rule x y flow add @f
$NFT flush chain x y
sleep 1
$NFT delete flowtable x f


It is not easy to reproduce it without the sleep 1 between flush chain and delete the flowtable.

The flowtable delete maybe early than rule destroy because the operation is the flush chain

but not delete rule directly .


Without my patch:

# ./run-tests.sh testcases/flowtable/0009deleteafterflush_0
I: using nft binary ./../../src/nft

W: [FAILED]    testcases/flowtable/0009deleteafterflush_0: got 1
Error: Could not process rule: Device or resource busy
delete flowtable x f
^^^^^^^^^^^^^^^^^^^^^

I: results: [OK] 0 [FAILED] 1 [TOTAL] 1


Add with my fixes patch: I run this 50 times all result is OK

# ./run-tests.sh testcases/flowtable/0009deleteafterflush_0
I: using nft binary ./../../src/nft

I: [OK]        testcases/flowtable/0009deleteafterflush_0

I: results: [OK] 1 [FAILED] 0 [TOTAL] 1


So which branch did you test with this patch?


BR

wenxu

On 12/31/2019 8:45 AM, wenxu wrote:
>
> 在 2019/12/31 4:02, Pablo Neira Ayuso 写道:
>> On Mon, Dec 30, 2019 at 09:25:36PM +0800, wenxu wrote:
>>> Hi pablo,
>>>
>>> How about this patch?
>> This test still fails after a second run with this patch:
>>
>> ./run-tests.sh testcases/flowtable/0009deleteafterflush_0
>> I: using nft binary ./../../src/nft
>>
>> W: [FAILED]     testcases/flowtable/0009deleteafterflush_0: got 1
>> Error: Could not process rule: Device or resource busy
>> delete flowtable x f
>
> Hi pablo,
>
>
> I did the same test for testcase 0009deleteafterflush_0, It is okay even there is no this patch in my tree.
>
> ++ which nft
> + NFT=/usr/sbin/nft
> + /usr/sbin/nft add table x
> + /usr/sbin/nft add chain x y
> + /usr/sbin/nft add flowtable x f '{' hook ingress priority '0;' devices = '{' lo '};}'
> + /usr/sbin/nft add rule x y flow add @f
> + /usr/sbin/nft flush chain x y
>
> + /usr/sbin/nft delete flowtable x f
>
>
> This patch fix the problem that there are nft_flow_offload rules,  when flush the rules or chain which will lead the use counter double decrease and overflow.
>
> nft add rule firewall ftb-all ct zone 1 ip protocol tcp flow offload @f
> nft add rule firewall ftb-all ct zone 1 ip protocol udp flow offload @f
>
> This testcase does not have any nft_flow_offload rules. So this testcase don't cover the problem I want to fixes.
>
>
> Ps:
>
>  I test the nf-next tree, this testcase have the problem, I think it should be another new problem. I will check it.
>
>

Re: [PATCH nf] netfilter: nft_flow_offload: fix unnecessary use counter decrease in destory

[Pablo Neira Ayuso]

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

On Tue, Dec 31, 2019 at 08:45:27AM +0800, wenxu wrote:
> 
> 在 2019/12/31 4:02, Pablo Neira Ayuso 写道:
> > On Mon, Dec 30, 2019 at 09:25:36PM +0800, wenxu wrote:
> > > Hi pablo,
> > > 
> > > How about this patch?
> > This test still fails after a second run with this patch:
> > 
> > ./run-tests.sh testcases/flowtable/0009deleteafterflush_0
> > I: using nft binary ./../../src/nft
> > 
> > W: [FAILED]     testcases/flowtable/0009deleteafterflush_0: got 1
> > Error: Could not process rule: Device or resource busy
> > delete flowtable x f
> 
> Hi pablo,
> 
> I did the same test for testcase 0009deleteafterflush_0, It is okay even
> there is no this patch in my tree.

Thanks, I'm going to apply the patch that I'm attaching to this email.

[-- Attachment #2: 0001-netfilter-nft_flow_offload-fix-underflow-in-flowtabl.patch --]
[-- Type: text/x-diff, Size: 1152 bytes --]

From 7c79a32304ccd7ee33f672a3b543ef62a664ce5d Mon Sep 17 00:00:00 2001
From: wenxu <wenxu@ucloud.cn>
Date: Fri, 20 Dec 2019 17:08:46 +0800
Subject: [PATCH] netfilter: nft_flow_offload: fix underflow in flowtable
 reference counter

The .deactivate and .activate interfaces already deal with the reference
counter. Otherwise, this results in spurious "Device is busy" errors.

Fixes: a3c90f7a2323 ("netfilter: nf_tables: flow offload expression")
Signed-off-by: wenxu <wenxu@ucloud.cn>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_flow_offload.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
index dd82ff2ee19f..b70b48996801 100644
--- a/net/netfilter/nft_flow_offload.c
+++ b/net/netfilter/nft_flow_offload.c
@@ -200,9 +200,6 @@ static void nft_flow_offload_activate(const struct nft_ctx *ctx,
 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);
 }
 
-- 
2.11.0

Re: [PATCH nf] netfilter: nft_flow_offload: fix unnecessary use counter decrease in destory

[wenxu]

在 2020/1/4 1:12, Pablo Neira Ayuso 写道:
> On Tue, Dec 31, 2019 at 08:45:27AM +0800, wenxu wrote:
>> 在 2019/12/31 4:02, Pablo Neira Ayuso 写道:
>>> On Mon, Dec 30, 2019 at 09:25:36PM +0800, wenxu wrote:
>>>> Hi pablo,
>>>>
>>>> How about this patch?
>>> This test still fails after a second run with this patch:
>>>
>>> ./run-tests.sh testcases/flowtable/0009deleteafterflush_0
>>> I: using nft binary ./../../src/nft
>>>
>>> W: [FAILED]     testcases/flowtable/0009deleteafterflush_0: got 1
>>> Error: Could not process rule: Device or resource busy
>>> delete flowtable x f
>> Hi pablo,
>>
>> I did the same test for testcase 0009deleteafterflush_0, It is okay even
>> there is no this patch in my tree.
> Thanks, I'm going to apply the patch that I'm attaching to this email.

Thanks,  It's ok for me.