[Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Cong Wang]

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:
  [<ffffffff81a203b6>] dump_stack+0x4c/0x65
  [<ffffffff8109daf0>] ___might_sleep+0x1c3/0x1cb
  [<ffffffff8109db70>] __might_sleep+0x78/0x80
  [<ffffffff8117a60e>] slab_pre_alloc_hook+0x31/0x8f
  [<ffffffff8117d4f6>] __kmalloc+0x69/0x14e
  [<ffffffff818ed0e1>] ? kzalloc.constprop.20+0xe/0x10
  [<ffffffff818ed0e1>] kzalloc.constprop.20+0xe/0x10
  [<ffffffff818ef622>] fib_trie_table+0x27/0x8b
  [<ffffffff818ef6bd>] fib_trie_unmerge+0x37/0x2a6
  [<ffffffff810b06e1>] ? arch_local_irq_save+0x9/0xc
  [<ffffffff818e9793>] fib_unmerge+0x2d/0xb3
  [<ffffffff818f5f56>] fib4_rule_delete+0x1f/0x52
  [<ffffffff817f1c3f>] ? fib_rules_unregister+0x30/0xb2
  [<ffffffff817f1c8b>] fib_rules_unregister+0x7c/0xb2
  [<ffffffff818f64a1>] fib4_rules_exit+0x15/0x18
  [<ffffffff818e8c0a>] ip_fib_net_exit+0x23/0xf2
  [<ffffffff818e91f8>] fib_net_exit+0x32/0x36
  [<ffffffff817c8352>] ops_exit_list+0x45/0x57
  [<ffffffff817c8d3d>] cleanup_net+0x13c/0x1cd
  [<ffffffff8108b05d>] process_one_work+0x255/0x4ad
  [<ffffffff8108af69>] ? process_one_work+0x161/0x4ad
  [<ffffffff8108b4b1>] worker_thread+0x1cd/0x2ab
  [<ffffffff8108b2e4>] ? process_scheduled_works+0x2f/0x2f
  [<ffffffff81090686>] kthread+0xd4/0xdc
  [<ffffffff8109ec8f>] ? local_clock+0x19/0x22
  [<ffffffff810905b2>] ? __kthread_parkme+0x83/0x83
  [<ffffffff81a2c0c8>] ret_from_fork+0x58/0x90
  [<ffffffff810905b2>] ? __kthread_parkme+0x83/0x83

Fixes: 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse")
Cc: Alexander Duyck <alexander.h.duyck@redhat.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 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];
-- 
1.8.3.1

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Alexander Duyck]

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:
>    [<ffffffff81a203b6>] dump_stack+0x4c/0x65
>    [<ffffffff8109daf0>] ___might_sleep+0x1c3/0x1cb
>    [<ffffffff8109db70>] __might_sleep+0x78/0x80
>    [<ffffffff8117a60e>] slab_pre_alloc_hook+0x31/0x8f
>    [<ffffffff8117d4f6>] __kmalloc+0x69/0x14e
>    [<ffffffff818ed0e1>] ? kzalloc.constprop.20+0xe/0x10
>    [<ffffffff818ed0e1>] kzalloc.constprop.20+0xe/0x10
>    [<ffffffff818ef622>] fib_trie_table+0x27/0x8b
>    [<ffffffff818ef6bd>] fib_trie_unmerge+0x37/0x2a6
>    [<ffffffff810b06e1>] ? arch_local_irq_save+0x9/0xc
>    [<ffffffff818e9793>] fib_unmerge+0x2d/0xb3
>    [<ffffffff818f5f56>] fib4_rule_delete+0x1f/0x52
>    [<ffffffff817f1c3f>] ? fib_rules_unregister+0x30/0xb2
>    [<ffffffff817f1c8b>] fib_rules_unregister+0x7c/0xb2
>    [<ffffffff818f64a1>] fib4_rules_exit+0x15/0x18
>    [<ffffffff818e8c0a>] ip_fib_net_exit+0x23/0xf2
>    [<ffffffff818e91f8>] fib_net_exit+0x32/0x36
>    [<ffffffff817c8352>] ops_exit_list+0x45/0x57
>    [<ffffffff817c8d3d>] cleanup_net+0x13c/0x1cd
>    [<ffffffff8108b05d>] process_one_work+0x255/0x4ad
>    [<ffffffff8108af69>] ? process_one_work+0x161/0x4ad
>    [<ffffffff8108b4b1>] worker_thread+0x1cd/0x2ab
>    [<ffffffff8108b2e4>] ? process_scheduled_works+0x2f/0x2f
>    [<ffffffff81090686>] kthread+0xd4/0xdc
>    [<ffffffff8109ec8f>] ? local_clock+0x19/0x22
>    [<ffffffff810905b2>] ? __kthread_parkme+0x83/0x83
>    [<ffffffff81a2c0c8>] ret_from_fork+0x58/0x90
>    [<ffffffff810905b2>] ? __kthread_parkme+0x83/0x83
>
> Fixes: 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse")
> Cc: Alexander Duyck <alexander.h.duyck@redhat.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>   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

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Cong Wang]

On Thu, Mar 26, 2015 at 2:47 PM, Alexander Duyck
<alexander.h.duyck@redhat.com> wrote:
> 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:
>>    [<ffffffff81a203b6>] dump_stack+0x4c/0x65
>>    [<ffffffff8109daf0>] ___might_sleep+0x1c3/0x1cb
>>    [<ffffffff8109db70>] __might_sleep+0x78/0x80
>>    [<ffffffff8117a60e>] slab_pre_alloc_hook+0x31/0x8f
>>    [<ffffffff8117d4f6>] __kmalloc+0x69/0x14e
>>    [<ffffffff818ed0e1>] ? kzalloc.constprop.20+0xe/0x10
>>    [<ffffffff818ed0e1>] kzalloc.constprop.20+0xe/0x10
>>    [<ffffffff818ef622>] fib_trie_table+0x27/0x8b
>>    [<ffffffff818ef6bd>] fib_trie_unmerge+0x37/0x2a6
>>    [<ffffffff810b06e1>] ? arch_local_irq_save+0x9/0xc
>>    [<ffffffff818e9793>] fib_unmerge+0x2d/0xb3
>>    [<ffffffff818f5f56>] fib4_rule_delete+0x1f/0x52
>>    [<ffffffff817f1c3f>] ? fib_rules_unregister+0x30/0xb2
>>    [<ffffffff817f1c8b>] fib_rules_unregister+0x7c/0xb2
>>    [<ffffffff818f64a1>] fib4_rules_exit+0x15/0x18
>>    [<ffffffff818e8c0a>] ip_fib_net_exit+0x23/0xf2
>>    [<ffffffff818e91f8>] fib_net_exit+0x32/0x36
>>    [<ffffffff817c8352>] ops_exit_list+0x45/0x57
>>    [<ffffffff817c8d3d>] cleanup_net+0x13c/0x1cd
>>    [<ffffffff8108b05d>] process_one_work+0x255/0x4ad
>>    [<ffffffff8108af69>] ? process_one_work+0x161/0x4ad
>>    [<ffffffff8108b4b1>] worker_thread+0x1cd/0x2ab
>>    [<ffffffff8108b2e4>] ? process_scheduled_works+0x2f/0x2f
>>    [<ffffffff81090686>] kthread+0xd4/0xdc
>>    [<ffffffff8109ec8f>] ? local_clock+0x19/0x22
>>    [<ffffffff810905b2>] ? __kthread_parkme+0x83/0x83
>>    [<ffffffff81a2c0c8>] ret_from_fork+0x58/0x90
>>    [<ffffffff810905b2>] ? __kthread_parkme+0x83/0x83
>>
>> Fixes: 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse")
>> Cc: Alexander Duyck <alexander.h.duyck@redhat.com>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>> ---
>>   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


I don't see callers like ipmr_rules_exit() holds a rtnl lock.


> 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.
>

As I said in changelog, the problem exists before your commit,
it is just exposed by it.

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Alexander Duyck]

On 03/26/2015 02:55 PM, Cong Wang wrote:
> On Thu, Mar 26, 2015 at 2:47 PM, Alexander Duyck
> <alexander.h.duyck@redhat.com> wrote:
>> 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:
>>>     [<ffffffff81a203b6>] dump_stack+0x4c/0x65
>>>     [<ffffffff8109daf0>] ___might_sleep+0x1c3/0x1cb
>>>     [<ffffffff8109db70>] __might_sleep+0x78/0x80
>>>     [<ffffffff8117a60e>] slab_pre_alloc_hook+0x31/0x8f
>>>     [<ffffffff8117d4f6>] __kmalloc+0x69/0x14e
>>>     [<ffffffff818ed0e1>] ? kzalloc.constprop.20+0xe/0x10
>>>     [<ffffffff818ed0e1>] kzalloc.constprop.20+0xe/0x10
>>>     [<ffffffff818ef622>] fib_trie_table+0x27/0x8b
>>>     [<ffffffff818ef6bd>] fib_trie_unmerge+0x37/0x2a6
>>>     [<ffffffff810b06e1>] ? arch_local_irq_save+0x9/0xc
>>>     [<ffffffff818e9793>] fib_unmerge+0x2d/0xb3
>>>     [<ffffffff818f5f56>] fib4_rule_delete+0x1f/0x52
>>>     [<ffffffff817f1c3f>] ? fib_rules_unregister+0x30/0xb2
>>>     [<ffffffff817f1c8b>] fib_rules_unregister+0x7c/0xb2
>>>     [<ffffffff818f64a1>] fib4_rules_exit+0x15/0x18
>>>     [<ffffffff818e8c0a>] ip_fib_net_exit+0x23/0xf2
>>>     [<ffffffff818e91f8>] fib_net_exit+0x32/0x36
>>>     [<ffffffff817c8352>] ops_exit_list+0x45/0x57
>>>     [<ffffffff817c8d3d>] cleanup_net+0x13c/0x1cd
>>>     [<ffffffff8108b05d>] process_one_work+0x255/0x4ad
>>>     [<ffffffff8108af69>] ? process_one_work+0x161/0x4ad
>>>     [<ffffffff8108b4b1>] worker_thread+0x1cd/0x2ab
>>>     [<ffffffff8108b2e4>] ? process_scheduled_works+0x2f/0x2f
>>>     [<ffffffff81090686>] kthread+0xd4/0xdc
>>>     [<ffffffff8109ec8f>] ? local_clock+0x19/0x22
>>>     [<ffffffff810905b2>] ? __kthread_parkme+0x83/0x83
>>>     [<ffffffff81a2c0c8>] ret_from_fork+0x58/0x90
>>>     [<ffffffff810905b2>] ? __kthread_parkme+0x83/0x83
>>>
>>> Fixes: 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse")
>>> Cc: Alexander Duyck <alexander.h.duyck@redhat.com>
>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>> ---
>>>    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
>
> I don't see callers like ipmr_rules_exit() holds a rtnl lock.

It doesn't matter since ipmr is using a different set of fib_rules_ops.  
So for example it doesn't appear to implement a delete so all it is 
doing is dropping the rules.  That is why fib_rules_cleanup_ops needs to 
stay within the rules_mod_lock.

>> 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.
>>
> As I said in changelog, the problem exists before your commit,
> it is just exposed by it.

No, the problem didn't.  The code covered the call to it with an 
rtnl_lock.  The problem is the ordering.  If you move the call to 
fib4_rules_exit as I have suggested it solves the problem without 
messing up a number of other protocols since there won't be any local 
table to unmerge so the unmerge call will simply return 0.

- Alex

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Cong Wang]

On Thu, Mar 26, 2015 at 3:17 PM, Alexander Duyck
<alexander.h.duyck@redhat.com> wrote:
>
> On 03/26/2015 02:55 PM, Cong Wang wrote:
>>
>> On Thu, Mar 26, 2015 at 2:47 PM, Alexander Duyck
>> <alexander.h.duyck@redhat.com> wrote:
>>>
>>>
>>> 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
>>
>>
>> I don't see callers like ipmr_rules_exit() holds a rtnl lock.
>
>
> It doesn't matter since ipmr is using a different set of fib_rules_ops.  So
> for example it doesn't appear to implement a delete so all it is doing is
> dropping the rules.  That is why fib_rules_cleanup_ops needs to stay within
> the rules_mod_lock.

Why ops->delete() matters here since ops->rules_list is generic?

Rules are added to or deleted from ops->rules_list with rtnl lock,
so why it is safe to clean up them all without rtnl lock?

On the other hand, the name rules_mod_lock already tells it is
just a protection for ops (module) register.

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Cong Wang]

On Thu, Mar 26, 2015 at 3:32 PM, Cong Wang <cwang@twopensource.com> wrote:
>
> On the other hand, the name rules_mod_lock already tells it is
> just a protection for ops (module) register.

I even doubt we really need rules_mod_lock, it is per netns,
which is newly allocated when registering pernet and upper
layer should guarantee no concurrent unregistering, so we
probably only need to take rtnl lock.

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Alexander Duyck]

On 03/26/2015 04:05 PM, Cong Wang wrote:
> On Thu, Mar 26, 2015 at 3:32 PM, Cong Wang <cwang@twopensource.com> wrote:
>> On the other hand, the name rules_mod_lock already tells it is
>> just a protection for ops (module) register.
> I even doubt we really need rules_mod_lock, it is per netns,
> which is newly allocated when registering pernet and upper
> layer should guarantee no concurrent unregistering, so we
> probably only need to take rtnl lock.

I'm adding Thomas as he was the original author for the code and might 
have a better idea of what needs to be rtnl locked and what doesn't.  
You should probably CC him as well on the v2 patch.

As far as why I am so focused on moving fib4_rules_exit it is because we 
don't want to call that delete function until after the table has been 
cleared.  Otherwise you end up triggering the external_flush and unmerge 
code on a full table instead of an empty one.  The result is you end up 
allocating a bunch of memory before you then turn around and free it.  
So even if you retain the rtnl_lock changes it would still be best to 
move fib4_rules_exit call to the region after you have freed the FIB 
tables, but before you free fib_table_hash.

- Alex

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Thomas Graf]

On 03/26/15 at 04:47pm, Alexander Duyck wrote:
> On 03/26/2015 04:05 PM, Cong Wang wrote:
> >On Thu, Mar 26, 2015 at 3:32 PM, Cong Wang <cwang@twopensource.com> wrote:
> >>On the other hand, the name rules_mod_lock already tells it is
> >>just a protection for ops (module) register.
> >I even doubt we really need rules_mod_lock, it is per netns,
> >which is newly allocated when registering pernet and upper
> >layer should guarantee no concurrent unregistering, so we
> >probably only need to take rtnl lock.
> 
> I'm adding Thomas as he was the original author for the code and might have
> a better idea of what needs to be rtnl locked and what doesn't.  You should
> probably CC him as well on the v2 patch.
> 
> As far as why I am so focused on moving fib4_rules_exit it is because we
> don't want to call that delete function until after the table has been
> cleared.  Otherwise you end up triggering the external_flush and unmerge
> code on a full table instead of an empty one.  The result is you end up
> allocating a bunch of memory before you then turn around and free it.  So
> even if you retain the rtnl_lock changes it would still be best to move
> fib4_rules_exit call to the region after you have freed the FIB tables, but
> before you free fib_table_hash.

I agree with Alex. Reordering fib4_rules_exit() makes sense. Not only
to fix this issue but also for the purpose of correct ordering of
allocation and releasing.

It is definitely also safe to move fib_rules_cleanup_ops() out of
rules_mod_lock. It is the responsibility of whoever registers the
rules that no rule is in use he calls fib_rules_unregister().

I don't see why fib_rules should hold rtnl_lock upon delete for the
caller. If the caller requires this protection it's up to him to
provide it.

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Cong Wang]

On Fri, Mar 27, 2015 at 5:01 AM, Thomas Graf <tgraf@suug.ch> wrote:
> On 03/26/15 at 04:47pm, Alexander Duyck wrote:
>> On 03/26/2015 04:05 PM, Cong Wang wrote:
>> >On Thu, Mar 26, 2015 at 3:32 PM, Cong Wang <cwang@twopensource.com> wrote:
>> >>On the other hand, the name rules_mod_lock already tells it is
>> >>just a protection for ops (module) register.
>> >I even doubt we really need rules_mod_lock, it is per netns,
>> >which is newly allocated when registering pernet and upper
>> >layer should guarantee no concurrent unregistering, so we
>> >probably only need to take rtnl lock.
>>
>> I'm adding Thomas as he was the original author for the code and might have
>> a better idea of what needs to be rtnl locked and what doesn't.  You should
>> probably CC him as well on the v2 patch.
>>
>> As far as why I am so focused on moving fib4_rules_exit it is because we
>> don't want to call that delete function until after the table has been
>> cleared.  Otherwise you end up triggering the external_flush and unmerge
>> code on a full table instead of an empty one.  The result is you end up
>> allocating a bunch of memory before you then turn around and free it.  So
>> even if you retain the rtnl_lock changes it would still be best to move
>> fib4_rules_exit call to the region after you have freed the FIB tables, but
>> before you free fib_table_hash.
>
> I agree with Alex. Reordering fib4_rules_exit() makes sense. Not only
> to fix this issue but also for the purpose of correct ordering of
> allocation and releasing.

I am surprised you guys only care this one issue, there are more
for me.

$subject already says this is net-next, nothing needs to worry
about backport.

>
> It is definitely also safe to move fib_rules_cleanup_ops() out of
> rules_mod_lock. It is the responsibility of whoever registers the
> rules that no rule is in use he calls fib_rules_unregister().
>
> I don't see why fib_rules should hold rtnl_lock upon delete for the
> caller. If the caller requires this protection it's up to him to
> provide it.

If not rtnl lock, then what prevents the race between netns unregistering
with rule add/del via netlink?

I know ops is removed from the list at that point, but ops->rules might be
still being traversed under rtnl lock:

                                         ops = lookup_rules_ops();
list_del_rcu(&ops->list);
                                         list_for_each_entry(ops->rules) {
fib_rules_cleanup_ops(ops);

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Alexander Duyck]

On 03/27/2015 12:25 PM, Cong Wang wrote:
> On Fri, Mar 27, 2015 at 5:01 AM, Thomas Graf <tgraf@suug.ch> wrote:
>> On 03/26/15 at 04:47pm, Alexander Duyck wrote:
>>> On 03/26/2015 04:05 PM, Cong Wang wrote:
>>>> On Thu, Mar 26, 2015 at 3:32 PM, Cong Wang <cwang@twopensource.com> wrote:
>>>>> On the other hand, the name rules_mod_lock already tells it is
>>>>> just a protection for ops (module) register.
>>>> I even doubt we really need rules_mod_lock, it is per netns,
>>>> which is newly allocated when registering pernet and upper
>>>> layer should guarantee no concurrent unregistering, so we
>>>> probably only need to take rtnl lock.
>>> I'm adding Thomas as he was the original author for the code and might have
>>> a better idea of what needs to be rtnl locked and what doesn't.  You should
>>> probably CC him as well on the v2 patch.
>>>
>>> As far as why I am so focused on moving fib4_rules_exit it is because we
>>> don't want to call that delete function until after the table has been
>>> cleared.  Otherwise you end up triggering the external_flush and unmerge
>>> code on a full table instead of an empty one.  The result is you end up
>>> allocating a bunch of memory before you then turn around and free it.  So
>>> even if you retain the rtnl_lock changes it would still be best to move
>>> fib4_rules_exit call to the region after you have freed the FIB tables, but
>>> before you free fib_table_hash.
>> I agree with Alex. Reordering fib4_rules_exit() makes sense. Not only
>> to fix this issue but also for the purpose of correct ordering of
>> allocation and releasing.
> I am surprised you guys only care this one issue, there are more
> for me.
>
> $subject already says this is net-next, nothing needs to worry
> about backport.

Who said anything about backport?  I said my concern was the fact that 
we were allocating in a shutdown path.  We shouldn't be.  That was why I 
wanted the function moved.

>> It is definitely also safe to move fib_rules_cleanup_ops() out of
>> rules_mod_lock. It is the responsibility of whoever registers the
>> rules that no rule is in use he calls fib_rules_unregister().
>>
>> I don't see why fib_rules should hold rtnl_lock upon delete for the
>> caller. If the caller requires this protection it's up to him to
>> provide it.
> If not rtnl lock, then what prevents the race between netns unregistering
> with rule add/del via netlink?
>
> I know ops is removed from the list at that point, but ops->rules might be
> still being traversed under rtnl lock:
>
>                                           ops = lookup_rules_ops();
> list_del_rcu(&ops->list);
>                                           list_for_each_entry(ops->rules) {
> fib_rules_cleanup_ops(ops);

This locking issue, if present, is separate from the original issue you 
reported.  I'm going to submit a patch to fix your original issue and 
you can chase this locking issue down separately if that is what you 
want to do.  This way if someone ever decides to backport it they can 
actually fix the original issue without pulling in speculative fixes for 
the rtnl locking problem since we were already holding the lock for fib4.

- Alex

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Cong Wang]

On Fri, Mar 27, 2015 at 2:08 PM, Alexander Duyck
<alexander.h.duyck@redhat.com> wrote:
>
> This locking issue, if present, is separate from the original issue you
> reported.  I'm going to submit a patch to fix your original issue and you
> can chase this locking issue down separately if that is what you want to do.

Make sure you really read my changelog, in case you don't:

"
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.
"

Sometimes people more easily miss the most obvious thing,
which is the first sentences of my changelog.


> This way if someone ever decides to backport it they can actually fix the
> original issue without pulling in speculative fixes for the rtnl locking
> problem since we were already holding the lock for fib4.
>

Backporting is my guess of Thomas's point, you go too far beyond it.

Also, you have a different definition of original issue.

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Alexander Duyck]

On 03/27/2015 02:17 PM, Cong Wang wrote:
> On Fri, Mar 27, 2015 at 2:08 PM, Alexander Duyck
> <alexander.h.duyck@redhat.com> wrote:
>> This locking issue, if present, is separate from the original issue you
>> reported.  I'm going to submit a patch to fix your original issue and you
>> can chase this locking issue down separately if that is what you want to do.
> Make sure you really read my changelog, in case you don't:
>
> "
> 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.
> "
>
> Sometimes people more easily miss the most obvious thing,
> which is the first sentences of my changelog.

I got that, but you are arguing in circles.  In the case of fib4 we 
already held the rtnl lock when all of this was called.  The delete bit 
only really applies to fib4 since that is the only rules setup that 
seems to implement that function.  As I said your "fix" was obscuring 
the original issue.  The original issue was that we were allocating in a 
cleanup path.  That is the first thing that needs to be fixed.

The rtnl_lock or not is a secondary issue.  It may be a fix but it 
doesn't really address the original problem which was allocating in a 
cleanup path.

>
>> This way if someone ever decides to backport it they can actually fix the
>> original issue without pulling in speculative fixes for the rtnl locking
>> problem since we were already holding the lock for fib4.
>>
> Backporting is my guess of Thomas's point, you go too far beyond it.

Backporting wasn't his issue.  From what I can tell he was okay with 
pulling the fib_rules_cleanup_ops outside of the rules_mode_lock, I am 
as well since I believe that is only there because that used to be in a 
loop that would walk through a list looking for ops in order to delete 
it.  Since the list walk is gone you could just hold the lock for the 
list_del_rcu and you are good.

The point he was trying to get at is that you should not make the 
rtnl_lock a part of fib_rules_unregster.  If someone is calling it in 
clean-up and requires it they should be taking the rtnl_lock like we did 
in fib4.  The issue is fib_rules_unregister is also called in the 
exception path for init and the rtnl_lock isn't necessary in that path.

> Also, you have a different definition of original issue.

Yes.  You reported a sleeping function called from invalid context, and 
you were fixing it by splitting up the rtnl_lock/unlock section in fib4 
unnecessarily which opens us up to other possible races, and left the 
function expensive and bloated as it was performing allocations in a 
clean-up path.

I've submitted patches for the issue I cared about so once those patches 
are applied feel free to try and address the rtnl_lock issue separately, 
however I would prefer it if you didn't split up the locking between the 
table freeing and the unregister as it should really all be done as one 
transaction without having to release and reacquire the RTNL lock in the 
middle of it.

- Alex

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Cong Wang]

On Fri, Mar 27, 2015 at 3:12 PM, Alexander Duyck
<alexander.h.duyck@redhat.com> wrote:
>
> On 03/27/2015 02:17 PM, Cong Wang wrote:
>>
>> On Fri, Mar 27, 2015 at 2:08 PM, Alexander Duyck
>> <alexander.h.duyck@redhat.com> wrote:
>>>
>>> This locking issue, if present, is separate from the original issue you
>>> reported.  I'm going to submit a patch to fix your original issue and you
>>> can chase this locking issue down separately if that is what you want to
>>> do.
>>
>> Make sure you really read my changelog, in case you don't:
>>
>> "
>> 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.
>> "
>>
>> Sometimes people more easily miss the most obvious thing,
>> which is the first sentences of my changelog.
>
>
> I got that, but you are arguing in circles.  In the case of fib4 we already
> held the rtnl lock when all of this was called.  The delete bit only really
> applies to fib4 since that is the only rules setup that seems to implement
> that function.  As I said your "fix" was obscuring the original issue.  The
> original issue was that we were allocating in a cleanup path.  That is the
> first thing that needs to be fixed.

I never said it is a fib4-only issue, ops->rules_list is generic.
I know you don't care about anything beyond fib4, I do. :)


>
> The rtnl_lock or not is a secondary issue.  It may be a fix but it doesn't
> really address the original problem which was allocating in a cleanup path.
>

Unless you understand there are two original problems...


>>
>>> This way if someone ever decides to backport it they can actually fix the
>>> original issue without pulling in speculative fixes for the rtnl locking
>>> problem since we were already holding the lock for fib4.
>>>
>> Backporting is my guess of Thomas's point, you go too far beyond it.
>
>
> Backporting wasn't his issue.  From what I can tell he was okay with pulling
> the fib_rules_cleanup_ops outside of the rules_mode_lock, I am as well since
> I believe that is only there because that used to be in a loop that would
> walk through a list looking for ops in order to delete it.  Since the list
> walk is gone you could just hold the lock for the list_del_rcu and you are
> good.


Quote from my previous reply:
"
I know ops is removed from the list at that point, but ops->rules might be
still being traversed under rtnl lock:

                                         ops = lookup_rules_ops();
list_del_rcu(&ops->list);
                                         list_for_each_entry(ops->rules) {
fib_rules_cleanup_ops(ops);
"

Pulling it out of mod_lock is one step, move it under rtnl lock is the second.

>
> The point he was trying to get at is that you should not make the rtnl_lock
> a part of fib_rules_unregster.  If someone is calling it in clean-up and
> requires it they should be taking the rtnl_lock like we did in fib4.  The
> issue is fib_rules_unregister is also called in the exception path for init
> and the rtnl_lock isn't necessary in that path.

This is trivial to solve, you are free to invent __fib_rules_unregister()
if you want.


>
>> Also, you have a different definition of original issue.
>
>
> Yes.  You reported a sleeping function called from invalid context, and you
> were fixing it by splitting up the rtnl_lock/unlock section in fib4
> unnecessarily which opens us up to other possible races, and left the
> function expensive and bloated as it was performing allocations in a
> clean-up path.

Sounds like it is me who called fib_unmerge(), ouch. ;)


>
> I've submitted patches for the issue I cared about so once those patches are
> applied feel free to try and address the rtnl_lock issue separately, however
> I would prefer it if you didn't split up the locking between the table
> freeing and the unregister as it should really all be done as one
> transaction without having to release and reacquire the RTNL lock in the
> middle of it.

As long as we agree rtnl lock should be taken, you already take my point
here ($subject says so).

It is just API change to move rtnl_lock up to caller or whatever appropriate.

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Alexander Duyck]

On 03/30/2015 04:47 PM, Cong Wang wrote:
> On Fri, Mar 27, 2015 at 3:12 PM, Alexander Duyck
> <alexander.h.duyck@redhat.com> wrote:
>> On 03/27/2015 02:17 PM, Cong Wang wrote:
>>> On Fri, Mar 27, 2015 at 2:08 PM, Alexander Duyck
>>> <alexander.h.duyck@redhat.com> wrote:
>>>> This locking issue, if present, is separate from the original issue you
>>>> reported.  I'm going to submit a patch to fix your original issue and you
>>>> can chase this locking issue down separately if that is what you want to
>>>> do.
>>> Make sure you really read my changelog, in case you don't:
>>>
>>> "
>>> 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.
>>> "
>>>
>>> Sometimes people more easily miss the most obvious thing,
>>> which is the first sentences of my changelog.
>>
>> I got that, but you are arguing in circles.  In the case of fib4 we already
>> held the rtnl lock when all of this was called.  The delete bit only really
>> applies to fib4 since that is the only rules setup that seems to implement
>> that function.  As I said your "fix" was obscuring the original issue.  The
>> original issue was that we were allocating in a cleanup path.  That is the
>> first thing that needs to be fixed.
> I never said it is a fib4-only issue, ops->rules_list is generic.
> I know you don't care about anything beyond fib4, I do. :)
>
>
>> The rtnl_lock or not is a secondary issue.  It may be a fix but it doesn't
>> really address the original problem which was allocating in a cleanup path.
>>
> Unless you understand there are two original problems...
>
>
>>>> This way if someone ever decides to backport it they can actually fix the
>>>> original issue without pulling in speculative fixes for the rtnl locking
>>>> problem since we were already holding the lock for fib4.
>>>>
>>> Backporting is my guess of Thomas's point, you go too far beyond it.
>>
>> Backporting wasn't his issue.  From what I can tell he was okay with pulling
>> the fib_rules_cleanup_ops outside of the rules_mode_lock, I am as well since
>> I believe that is only there because that used to be in a loop that would
>> walk through a list looking for ops in order to delete it.  Since the list
>> walk is gone you could just hold the lock for the list_del_rcu and you are
>> good.
>
> Quote from my previous reply:
> "
> I know ops is removed from the list at that point, but ops->rules might be
> still being traversed under rtnl lock:
>
>                                           ops = lookup_rules_ops();
> list_del_rcu(&ops->list);
>                                           list_for_each_entry(ops->rules) {
> fib_rules_cleanup_ops(ops);
> "
>
> Pulling it out of mod_lock is one step, move it under rtnl lock is the second.
>
>> The point he was trying to get at is that you should not make the rtnl_lock
>> a part of fib_rules_unregster.  If someone is calling it in clean-up and
>> requires it they should be taking the rtnl_lock like we did in fib4.  The
>> issue is fib_rules_unregister is also called in the exception path for init
>> and the rtnl_lock isn't necessary in that path.
> This is trivial to solve, you are free to invent __fib_rules_unregister()
> if you want.
>

It isn't necessary though, and for example in the case of 
ip6mr_rules_exit and ipmr_rules_exit it in general looks much cleaner 
since the init doesn't need the lock when allocating the tables, but the 
cleanup does when freeing them.  So for example in ip6mr_rules_exit you 
only have to swap the rtnl_unlock and call to fib_rules_unregister and 
the problem is solved, and from the sound of it you already had a 
similar patch for ipmr to bring it in line with what is in ip6mr so you 
would only need to modify it slightly.

>>> Also, you have a different definition of original issue.
>>
>> Yes.  You reported a sleeping function called from invalid context, and you
>> were fixing it by splitting up the rtnl_lock/unlock section in fib4
>> unnecessarily which opens us up to other possible races, and left the
>> function expensive and bloated as it was performing allocations in a
>> clean-up path.
> Sounds like it is me who called fib_unmerge(), ouch. ;)
>

No, you just left it there.  Like you said, two issues.  The fix for 
what I considered to be the higher priority was getting fouled up in the 
process of trying to address the second one.  That is why I wanted them 
done as two separate fixes and submitted the fix for the first one now 
as I considered it a higher priority since it was something that you had 
been able to reproduce.

>> I've submitted patches for the issue I cared about so once those patches are
>> applied feel free to try and address the rtnl_lock issue separately, however
>> I would prefer it if you didn't split up the locking between the table
>> freeing and the unregister as it should really all be done as one
>> transaction without having to release and reacquire the RTNL lock in the
>> middle of it.
> As long as we agree rtnl lock should be taken, you already take my point
> here ($subject says so).

Yes, I agree lock can be held.  For fib4 it was already holding the RTNL 
lock when it made that call.  You can update the other users of 
fib_rules_unregister so that they call it with the RTNL lock held as well.

> It is just API change to move rtnl_lock up to caller or whatever appropriate.

Right, so like I said for fib4 this is resolved.  That just leaves ipmr, 
ip6mr, fib6, and dn_rules that need to be updated so that they correctly 
handle the RTNL locking in their exit/cleanup paths. Since you already 
have some related patches out for these I will let you take them 
otherwise I might try to go through and clean them up next week.

- Alex

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Cong Wang]

On Mon, Mar 30, 2015 at 5:02 PM, Alexander Duyck
<alexander.h.duyck@redhat.com> wrote:
>
> On 03/30/2015 04:47 PM, Cong Wang wrote:
>>
>> As long as we agree rtnl lock should be taken, you already take my point
>> here ($subject says so).
>
>
> Yes, I agree lock can be held.  For fib4 it was already holding the RTNL
> lock when it made that call.  You can update the other users of
> fib_rules_unregister so that they call it with the RTNL lock held as well.
>
>> It is just API change to move rtnl_lock up to caller or whatever
>> appropriate.
>
>
> Right, so like I said for fib4 this is resolved.  That just leaves ipmr,
> ip6mr, fib6, and dn_rules that need to be updated so that they correctly
> handle the RTNL locking in their exit/cleanup paths. Since you already have
> some related patches out for these I will let you take them otherwise I
> might try to go through and clean them up next week.
>

Ok, then we are finally on the same page. We need two patches:

1) move unregister under rtnl lock (as what this patch intended to do)
2) remove the unnecessary rules_mod_lock

Thanks.

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Alexander Duyck]

On 03/30/2015 05:12 PM, Cong Wang wrote:
> On Mon, Mar 30, 2015 at 5:02 PM, Alexander Duyck
> <alexander.h.duyck@redhat.com> wrote:
>> On 03/30/2015 04:47 PM, Cong Wang wrote:
>>> As long as we agree rtnl lock should be taken, you already take my point
>>> here ($subject says so).
>>
>> Yes, I agree lock can be held.  For fib4 it was already holding the RTNL
>> lock when it made that call.  You can update the other users of
>> fib_rules_unregister so that they call it with the RTNL lock held as well.
>>
>>> It is just API change to move rtnl_lock up to caller or whatever
>>> appropriate.
>>
>> Right, so like I said for fib4 this is resolved.  That just leaves ipmr,
>> ip6mr, fib6, and dn_rules that need to be updated so that they correctly
>> handle the RTNL locking in their exit/cleanup paths. Since you already have
>> some related patches out for these I will let you take them otherwise I
>> might try to go through and clean them up next week.
>>
> Ok, then we are finally on the same page. We need two patches:
>
> 1) move unregister under rtnl lock (as what this patch intended to do)

Yes I think the only disagreement was on how to do it.  Your original 
patch placed it in fib_rules_cleanup_ops, and the preference of myself 
and Thomas was to hold the lock before you even call 
fib_rules_unregister.  So the IPv4 code is fine with the patch I 
submitted, it is the other callers of fib_rules_unregister which must be 
updated.

> 2) remove the unnecessary rules_mod_lock
>
> Thanks.

Please define "unnecessary" as we have had a bit of back and forth on 
how our views can differ there.  As far as I know it still has to be 
held for the fib_rule_ops list manipulation, specifically the call to 
list_del_rcu.  However, it doesn't need to be held when we call 
fib_rules_cleanup_ops.

- Alex

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Cong Wang]

On Mon, Mar 30, 2015 at 8:10 PM, Alexander Duyck
<alexander.h.duyck@redhat.com> wrote:
>
> On 03/30/2015 05:12 PM, Cong Wang wrote:
>>
>> On Mon, Mar 30, 2015 at 5:02 PM, Alexander Duyck
>> <alexander.h.duyck@redhat.com> wrote:
>>>
>>> On 03/30/2015 04:47 PM, Cong Wang wrote:
>>>>
>>>> As long as we agree rtnl lock should be taken, you already take my point
>>>> here ($subject says so).
>>>
>>>
>>> Yes, I agree lock can be held.  For fib4 it was already holding the RTNL
>>> lock when it made that call.  You can update the other users of
>>> fib_rules_unregister so that they call it with the RTNL lock held as
>>> well.
>>>
>>>> It is just API change to move rtnl_lock up to caller or whatever
>>>> appropriate.
>>>
>>>
>>> Right, so like I said for fib4 this is resolved.  That just leaves ipmr,
>>> ip6mr, fib6, and dn_rules that need to be updated so that they correctly
>>> handle the RTNL locking in their exit/cleanup paths. Since you already
>>> have
>>> some related patches out for these I will let you take them otherwise I
>>> might try to go through and clean them up next week.
>>>
>> Ok, then we are finally on the same page. We need two patches:
>>
>> 1) move unregister under rtnl lock (as what this patch intended to do)
>
>
> Yes I think the only disagreement was on how to do it.  Your original patch
> placed it in fib_rules_cleanup_ops, and the preference of myself and Thomas
> was to hold the lock before you even call fib_rules_unregister.  So the IPv4
> code is fine with the patch I submitted, it is the other callers of
> fib_rules_unregister which must be updated.

I never say your patch is not fine (except on the ops->delete issue which is
not related here) to fix it, as I said your patch only fixes one of the problems
I saw, that is it.

>
>> 2) remove the unnecessary rules_mod_lock
>>
>> Thanks.
>
>
> Please define "unnecessary" as we have had a bit of back and forth on how
> our views can differ there.  As far as I know it still has to be held for
> the fib_rule_ops list manipulation, specifically the call to list_del_rcu.
> However, it doesn't need to be held when we call fib_rules_cleanup_ops.
>

Look at where rules_mod_lock are held: either when the net is initialized
or when unregistering, neither of them really needs this per netns lock:
new netns is not ready to expose;
concurrent unregistering is prevented by upper layer locking,
readers (lookup_rules_ops) hold RCU but we already should hold rtnl lock
(after patch of course).

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Alexander Duyck]

On 03/31/2015 09:47 AM, Cong Wang wrote:
> On Mon, Mar 30, 2015 at 8:10 PM, Alexander Duyck
> <alexander.h.duyck@redhat.com> wrote:
>> On 03/30/2015 05:12 PM, Cong Wang wrote:
>>> 2) remove the unnecessary rules_mod_lock
>>>
>>> Thanks.
>>
>> Please define "unnecessary" as we have had a bit of back and forth on how
>> our views can differ there.  As far as I know it still has to be held for
>> the fib_rule_ops list manipulation, specifically the call to list_del_rcu.
>> However, it doesn't need to be held when we call fib_rules_cleanup_ops.
>>
> Look at where rules_mod_lock are held: either when the net is initialized
> or when unregistering, neither of them really needs this per netns lock:
> new netns is not ready to expose;
> concurrent unregistering is prevented by upper layer locking,
> readers (lookup_rules_ops) hold RCU but we already should hold rtnl lock
> (after patch of course).

I would have almost agreed with you, however I noticed that decnet 
doesn't seem to follow the same rules as the rest of the callers to 
fib_rules_register.  It will simply call into module_init -> decnet_init 
-> dn_fib_init -> dn_fib_rules_injt -> fib_rules_register(&init_net).  
It probably just needs to be rewritten to use 
register_pernet_subsys/unregister_pernet_subsys, or correctly use the 
net_mutex, and then you could probably go head and tear out the 
rules_mod_lock since they would all be covered under the net_mutex.

- Alex

Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

[Cong Wang]

On Tue, Mar 31, 2015 at 10:30 AM, Alexander Duyck
<alexander.h.duyck@redhat.com> wrote:
>
> On 03/31/2015 09:47 AM, Cong Wang wrote:
>>
>> On Mon, Mar 30, 2015 at 8:10 PM, Alexander Duyck
>> <alexander.h.duyck@redhat.com> wrote:
>>>
>>> On 03/30/2015 05:12 PM, Cong Wang wrote:
>>>>
>>>> 2) remove the unnecessary rules_mod_lock
>>>>
>>>> Thanks.
>>>
>>>
>>> Please define "unnecessary" as we have had a bit of back and forth on how
>>> our views can differ there.  As far as I know it still has to be held for
>>> the fib_rule_ops list manipulation, specifically the call to
>>> list_del_rcu.
>>> However, it doesn't need to be held when we call fib_rules_cleanup_ops.
>>>
>> Look at where rules_mod_lock are held: either when the net is initialized
>> or when unregistering, neither of them really needs this per netns lock:
>> new netns is not ready to expose;
>> concurrent unregistering is prevented by upper layer locking,
>> readers (lookup_rules_ops) hold RCU but we already should hold rtnl lock
>> (after patch of course).
>
>
> I would have almost agreed with you, however I noticed that decnet doesn't
> seem to follow the same rules as the rest of the callers to
> fib_rules_register.  It will simply call into module_init -> decnet_init ->
> dn_fib_init -> dn_fib_rules_injt -> fib_rules_register(&init_net).  It
> probably just needs to be rewritten to use
> register_pernet_subsys/unregister_pernet_subsys, or correctly use the
> net_mutex, and then you could probably go head and tear out the
> rules_mod_lock since they would all be covered under the net_mutex.
>

Good catch, or it can simply hold rtnl lock in module init path.
Anyway, it is for net-next.