Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit

[Alexander Duyck <alexander.h.duyck@redhat.com>]

On 03/30/2015 11:54 AM, Cong Wang wrote:
> On Fri, Mar 27, 2015 at 4:29 PM, Alexander Duyck
> <alexander.h.duyck@redhat.com> wrote:
>> On 03/27/2015 03:14 PM, Cong Wang wrote:
>>> On Fri, Mar 27, 2015 at 2:14 PM, Alexander Duyck
>>> <alexander.h.duyck@redhat.com> wrote:
>>>> The issue was that as a part of exiting the default rules were being
>>>> deleted which resulted in the local trie being unmerged.  By moving the
>>>> freeing of the FIB tables up we can avoid the unmerge since there is no
>>>> local table left when we call the fib4_rules_exit function.
>>>>
>>> This literally means we no longer need to call ops->delete()
>>> in netns unregister path.
>>
>> You are confusing table entries and rules.  The tables are cleared, the
>> rules still have to be deleted.  This patch breaks the reference counting
>> for fib_num_tclassid_users.
> It doesn't matter much here, the whole net is being unregistered,
> we are holding rtnl lock and existing readers don't mind to read an incorrect
> fib_num_tclassid_users.

Still best not to mess with this.  For the sake of completeness if we 
have delete implemented it should be called.


>>> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
>>> index 68ea695..27b6e04 100644
>>> --- a/net/core/fib_rules.c
>>> +++ b/net/core/fib_rules.c
>>> @@ -153,8 +153,6 @@ static void fib_rules_cleanup_ops(struct fib_rules_ops
>>> *ops)
>>>
>>>           list_for_each_entry_safe(rule, tmp, &ops->rules_list, list) {
>>>                   list_del_rcu(&rule->list);
>>> -               if (ops->delete)
>>> -                       ops->delete(rule);
>>>                   fib_rule_put(rule);
>>>           }
>>>    }
>>> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
>>> index e5b6b05..1481b23 100644
>>> --- a/net/ipv4/fib_frontend.c
>>> +++ b/net/ipv4/fib_frontend.c
>>> @@ -1178,6 +1178,7 @@ static void ip_fib_net_exit(struct net *net)
>>>
>>>    #ifdef CONFIG_IP_MULTIPLE_TABLES
>>>           fib4_rules_exit(net);
>>> +       fib_flush_external(net); // <-------- Maybe not needed either.
>>>    #endif
>>>
>>>           for (i = 0; i < FIB_TABLE_HASHSZ; i++) {
>>
>> Take a look at fib4_rule_delete.  There is more there than just unmerge and
>> flush external.
> I am not stupid, what otherwise do you think the above fib_flush_external()
> comes from?

The fact is you are choosing to overlook things that will lead to 
issues, if not now, then later, and as a result make the code more 
difficult to maintain.  It isn't as if this is hot-path code so there 
isn't any need to optimize it by dropping these calls.

> Read fib4_rule_delete(), everything it cleans up is per net, you can argue
> ipv4.fib_num_tclassid_users is a refcount for rules, but still the whole net
> is being unregistered.

Yes, the delete call will likely not do much more than update 
ipv4.fib_num_tclassid_users, but having that value updated until we free 
the net structure is useful for things like debugging.

If anything it would be useful to go through and audit the other users 
to make sure they are all following a similar pattern.  From what I can 
tell ip6mr_rules_exit is already in the same layout, and the same goes 
for ipmr_rules_exit though each is dealing with the RTNL lock 
differently.  Your efforts would be much better placed there than trying 
to alter code that really should be left as-is for completeness.

- Alex