| Message ID | 20181001171714.25327-1-ap420073@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf-next] netfilter: nf_flow_table: remove flowtable hook flush routine in netns exit routine | expand |
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 >
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 > >
On Tue, Oct 02, 2018 at 02:17:14AM +0900, Taehee Yoo wrote: > 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 Applied, thanks for explaining.
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) {
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(-)