From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Duyck Subject: Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock Date: Thu, 26 Mar 2015 14:47:09 -0700 Message-ID: <55147E5D.2070600@redhat.com> References: <1427403769-31208-1-git-send-email-xiyou.wangcong@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit To: Cong Wang , netdev@vger.kernel.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:42774 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751900AbbCZVrL (ORCPT ); Thu, 26 Mar 2015 17:47:11 -0400 In-Reply-To: <1427403769-31208-1-git-send-email-xiyou.wangcong@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On 03/26/2015 02:02 PM, Cong Wang wrote: > ops->rules_list is protected by rtnl_lock + RCU, > there is no reason to take net->rules_mod_lock here. > Also, ops->delete() needs to be called with rtnl_lock > too. The problem exists before, just it is exposed > recently due to the fib local/main table change. > > This fixes the following warning: > > BUG: sleeping function called from invalid context at mm/slub.c:1268 > in_atomic(): 1, irqs_disabled(): 0, pid: 6, name: kworker/u8:0 > INFO: lockdep is turned off. > CPU: 3 PID: 6 Comm: kworker/u8:0 Tainted: G W 4.0.0-rc5+ #895 > Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 > Workqueue: netns cleanup_net > 0000000000000006 ffff88011953fa68 ffffffff81a203b6 000000002c3a2c39 > ffff88011952a680 ffff88011953fa98 ffffffff8109daf0 ffff8801186c6aa8 > ffffffff81fbc9e5 00000000000004f4 0000000000000000 ffff88011953fac8 > Call Trace: > [] dump_stack+0x4c/0x65 > [] ___might_sleep+0x1c3/0x1cb > [] __might_sleep+0x78/0x80 > [] slab_pre_alloc_hook+0x31/0x8f > [] __kmalloc+0x69/0x14e > [] ? kzalloc.constprop.20+0xe/0x10 > [] kzalloc.constprop.20+0xe/0x10 > [] fib_trie_table+0x27/0x8b > [] fib_trie_unmerge+0x37/0x2a6 > [] ? arch_local_irq_save+0x9/0xc > [] fib_unmerge+0x2d/0xb3 > [] fib4_rule_delete+0x1f/0x52 > [] ? fib_rules_unregister+0x30/0xb2 > [] fib_rules_unregister+0x7c/0xb2 > [] fib4_rules_exit+0x15/0x18 > [] ip_fib_net_exit+0x23/0xf2 > [] fib_net_exit+0x32/0x36 > [] ops_exit_list+0x45/0x57 > [] cleanup_net+0x13c/0x1cd > [] process_one_work+0x255/0x4ad > [] ? process_one_work+0x161/0x4ad > [] worker_thread+0x1cd/0x2ab > [] ? process_scheduled_works+0x2f/0x2f > [] kthread+0xd4/0xdc > [] ? local_clock+0x19/0x22 > [] ? __kthread_parkme+0x83/0x83 > [] ret_from_fork+0x58/0x90 > [] ? __kthread_parkme+0x83/0x83 > > Fixes: 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse") > Cc: Alexander Duyck > Signed-off-by: Cong Wang > --- > net/core/fib_rules.c | 5 ++++- > net/ipv4/fib_frontend.c | 3 +-- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c > index 68ea695..0149977 100644 > --- a/net/core/fib_rules.c > +++ b/net/core/fib_rules.c > @@ -165,9 +165,12 @@ void fib_rules_unregister(struct fib_rules_ops *ops) > > spin_lock(&net->rules_mod_lock); > list_del_rcu(&ops->list); > - fib_rules_cleanup_ops(ops); > spin_unlock(&net->rules_mod_lock); > > + rtnl_lock(); > + fib_rules_cleanup_ops(ops); > + rtnl_unlock(); > + > kfree_rcu(ops, rcu); > } > EXPORT_SYMBOL_GPL(fib_rules_unregister); > diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c > index e5b6b05..3e40b01 100644 > --- a/net/ipv4/fib_frontend.c > +++ b/net/ipv4/fib_frontend.c > @@ -1174,11 +1174,10 @@ static void ip_fib_net_exit(struct net *net) > { > unsigned int i; > > - rtnl_lock(); > - > #ifdef CONFIG_IP_MULTIPLE_TABLES > fib4_rules_exit(net); > #endif > + rtnl_lock(); > > for (i = 0; i < FIB_TABLE_HASHSZ; i++) { > struct hlist_head *head = &net->ipv4.fib_table_hash[i]; I kind of think the patch title is misleading. The code was already under an rtnl_lock, the problem was it was wrapped in the rules_mod_lock and that is what was triggering the BUG you have seen. If anything the only change really needed would probably have been to move fib_rules_cleanup_ops out of the spin locked section. The simpler solution for would be to just reorder ip_fib_net_exit so that we call fib4_rules_exit after we have deleted all of the entries and tables, but before we have released the rtnl lock. That way we don't have to worry about the allocation because the table is freed and it follows the convention of allocation as a, b, c in order and then releases it c, b, a. Right now it is kind of out of order to drop the rules first and then the FIB entries. - Alex