[nf-next] netfilter: nf_flow_table: remove flowtable hook flush routine in netns exit routine

Message ID 20181001171714.25327-1-ap420073@gmail.com
State Under Review
Delegated to: Pablo Neira
Headers show
Series
  • [nf-next] netfilter: nf_flow_table: remove flowtable hook flush routine in netns exit routine
Related show

Commit Message

Taehee Yoo Oct. 1, 2018, 5:17 p.m.
When device is unregistered, flowtable flush routine is called
by notifier_call(nf_tables_flowtable_event). and exit callback of
nftables pernet_operation(nf_tables_exit_net) also has flowtable flush
routine. but when network namespace is destroyed, both notifier_call
and pernet_operation are called. hence flowtable flush routine in
pernet_operation is unnecessary.

test commands:
   %ip netns add vm1
   %ip netns exec vm1 nft add table ip filter
   %ip netns exec vm1 nft add flowtable ip filter w \
	{ hook ingress priority 0\; devices = { lo }\; }
   %ip netns del vm1

splat looks like:
[  265.187019] WARNING: CPU: 0 PID: 87 at net/netfilter/core.c:309 nf_hook_entry_head+0xc7/0xf0
[  265.187112] Modules linked in: nf_flow_table_ipv4 nf_flow_table nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nf_tables nfnetlink ip_tables x_tables
[  265.187390] CPU: 0 PID: 87 Comm: kworker/u4:2 Not tainted 4.19.0-rc3+ #5
[  265.187453] Workqueue: netns cleanup_net
[  265.187514] RIP: 0010:nf_hook_entry_head+0xc7/0xf0
[  265.187546] Code: 8d 81 68 03 00 00 5b c3 89 d0 83 fa 04 48 8d 84 c7 e8 11 00 00 76 81 0f 0b 31 c0 e9 78 ff ff ff 0f 0b 48 83 c4 08 31 c0 5b c3 <0f> 0b 31 c0 e9 65 ff ff ff 0f 0b 31 c0 e9 5c ff ff ff 48 89 0c 24
[  265.187573] RSP: 0018:ffff88011546f098 EFLAGS: 00010246
[  265.187624] RAX: ffffffff8d90e135 RBX: 1ffff10022a8de1c RCX: 0000000000000000
[  265.187645] RDX: 0000000000000000 RSI: 0000000000000005 RDI: ffff880116298040
[  265.187645] RBP: ffff88010ea4c1a8 R08: 0000000000000000 R09: 0000000000000000
[  265.187645] R10: ffff88011546f1d8 R11: ffffed0022c532c1 R12: ffff88010ea4c1d0
[  265.187645] R13: 0000000000000005 R14: dffffc0000000000 R15: ffff88010ea4c1c4
[  265.187645] FS:  0000000000000000(0000) GS:ffff88011b200000(0000) knlGS:0000000000000000
[  265.187645] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  265.187645] CR2: 00007fdfb8d00000 CR3: 0000000057a16000 CR4: 00000000001006f0
[  265.187645] Call Trace:
[  265.187645]  __nf_unregister_net_hook+0xca/0x5d0
[  265.187645]  ? nf_hook_entries_free.part.3+0x80/0x80
[  265.187645]  ? save_trace+0x300/0x300
[  265.187645]  nf_unregister_net_hooks+0x2e/0x40
[  265.187645]  nf_tables_exit_net+0x479/0x1340 [nf_tables]
[  265.187645]  ? find_held_lock+0x39/0x1c0
[  265.187645]  ? nf_tables_abort+0x30/0x30 [nf_tables]
[  265.187645]  ? inet_frag_destroy_rcu+0xd0/0xd0
[  265.187645]  ? trace_hardirqs_on+0x93/0x210
[  265.187645]  ? __bpf_trace_preemptirq_template+0x10/0x10
[  265.187645]  ? inet_frag_destroy_rcu+0xd0/0xd0
[  265.187645]  ? inet_frag_destroy_rcu+0xd0/0xd0
[  265.187645]  ? __mutex_unlock_slowpath+0x17f/0x740
[  265.187645]  ? wait_for_completion+0x710/0x710
[  265.187645]  ? bucket_table_free+0xb2/0x1f0
[  265.187645]  ? nested_table_free+0x130/0x130
[  265.187645]  ? __lock_is_held+0xb4/0x140
[  265.187645]  ops_exit_list.isra.10+0x94/0x140
[  265.187645]  cleanup_net+0x45b/0x900
[ ... ]

This WARNING means that hook unregisteration is failed because
all flowtables hooks are already unregistered by notifier_call.

Network namespace exit routine guarantees that all devices will be
unregistered first. then, other exit callbacks of pernet_operations
are called. so that removing flowtable flush routine in exit callback of
pernet_operation(nf_tables_exit_net) doesn't make flowtable leak.

Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 net/netfilter/nf_tables_api.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Pablo Neira Ayuso Oct. 8, 2018, 11:19 p.m. | #1
Hi Taehee,

I can reproduce it, so this is a bug :-). Still one question below:

On Tue, Oct 02, 2018 at 02:17:14AM +0900, Taehee Yoo wrote:
[...]
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index f0159eea2978..42487d01a3ed 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -7280,9 +7280,6 @@ static void __nft_release_tables(struct net *net)
>  
>  		list_for_each_entry(chain, &table->chains, list)
>  			nf_tables_unregister_hook(net, table, chain);
> -		list_for_each_entry(flowtable, &table->flowtables, list)
> -			nf_unregister_net_hooks(net, flowtable->ops,
> -						flowtable->ops_len);

Hm, why do we still need for basechains with device, ie. from ingress?
I might be missing something...

>  		/* No packets are walking on these chains anymore. */
>  		ctx.table = table;
>  		list_for_each_entry(chain, &table->chains, list) {
> -- 
> 2.17.1
>
Taehee Yoo Oct. 9, 2018, 1:06 p.m. | #2
On Tue, 9 Oct 2018 at 08:19, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi Taehee,
>

Hi Pablo,

Thank you for your review!

> I can reproduce it, so this is a bug :-). Still one question below:
>
> On Tue, Oct 02, 2018 at 02:17:14AM +0900, Taehee Yoo wrote:
> [...]
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index f0159eea2978..42487d01a3ed 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -7280,9 +7280,6 @@ static void __nft_release_tables(struct net *net)
> >
> >               list_for_each_entry(chain, &table->chains, list)
> >                       nf_tables_unregister_hook(net, table, chain);
> > -             list_for_each_entry(flowtable, &table->flowtables, list)
> > -                     nf_unregister_net_hooks(net, flowtable->ops,
> > -                                             flowtable->ops_len);
>
> Hm, why do we still need for basechains with device, ie. from ingress?
> I might be missing something...
>

As far as I know, at this point, all types of basechains(arp, ipv4, ipv6, ...)
are unregistered. ingress basechains are already unregistered by
notifier_call(nf_tables_netdev_event) but other types of basechains
still exist in chain list. so that this code is still needed.
But I might have misunderstood about your mention.
If so, please let me know about that.

Thanks!

> >               /* No packets are walking on these chains anymore. */
> >               ctx.table = table;
> >               list_for_each_entry(chain, &table->chains, list) {
> > --
> > 2.17.1
> >

Patch

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index f0159eea2978..42487d01a3ed 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -7280,9 +7280,6 @@  static void __nft_release_tables(struct net *net)
 
 		list_for_each_entry(chain, &table->chains, list)
 			nf_tables_unregister_hook(net, table, chain);
-		list_for_each_entry(flowtable, &table->flowtables, list)
-			nf_unregister_net_hooks(net, flowtable->ops,
-						flowtable->ops_len);
 		/* No packets are walking on these chains anymore. */
 		ctx.table = table;
 		list_for_each_entry(chain, &table->chains, list) {