[Devel] [PATCH rh7] fib_rules: mark default fib reles as BLACK

[Stanislav Kinsburskiу]

23 нояб. 2015 г. 16:34 пользователь Andrey Ryabinin <aryabinin at odin.com> написал:
>
>
>
> On 11/23/2015 06:02 PM, Stanislav Kinsburskiy wrote: 
> > 
> > 
> > 23.11.2015 14:49, Andrey Ryabinin пишет: 
> >> On 11/23/2015 03:47 PM, Stanislav Kinsburskiy wrote: 
> >>> This patch fixed flase positive, reported by KASan. 
> >>> 
> >> s/flase/false 
> >> s/KASan/kmemleak 
> >> 
> >>> https://jira.sw.ru/browse/PSBM-41453 
> >>> 
> >>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky at virtuozzo.com> 
> >>> --- 
> >>>   net/core/fib_rules.c |    4 ++++ 
> >>>   1 file changed, 4 insertions(+) 
> >>> 
> >>> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c 
> >>> index 0e91311..bc69cef 100644 
> >>> --- a/net/core/fib_rules.c 
> >>> +++ b/net/core/fib_rules.c 
> >>> @@ -36,6 +36,10 @@ int fib_default_rule_add(struct fib_rules_ops *ops, 
> >>>       /* The lock is not required here, the list in unreacheable 
> >>>        * at the moment this function is called */ 
> >>>       list_add_tail(&r->list, &ops->rules_list); 
> >>> + 
> >>> +    /* This object is not referenced by any user and will be removed on net 
> >>> +     * ns stop in fib_rules_cleanup_ops */ 
> >> If it's not referenced, than how it can be removed? 
> >> Changelog and this comment is rather poor, and doesn't explain why it's a false-positive. 
> >> 
> >> To me, it doesn't look so: 
> >> 
> >> 
> >> 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);          // remove from list, so object will become unerferenced 
> >> 
> >>         fib_rule_put(rule) => 
> >>             if (atomic_dec_and_test(&rule->refcnt)) 
> >>                 call_rcu(&rule->rcu, fib_rule_put_rcu); //release rule iff refcnt was 1 
> >> 
> >> 
> >> 
> >> So if 'rule->refcnt > 1' fib_rules_cleanups_ops() will remove rule from list, but won't free it. 
> >> 
> >> If you look at hex dump from report, you will see that refcnt is 4: 
> >> 
> >> unreferenced object 0xffff880255d42520 (size 128): 
> >> comm "vzctl", pid 184011, jiffies 4324028137 (age 193.266s) 
> >> hex dump (first 32 bytes): 
> >>      50 06 05 68 00 88 ff ff 00 02 20 00 00 00 ad de  P..h...... ..... 
> >>      04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................ 
> >>      ^^^^^^^^^^^ 
> >>    fib_rule->refcnt 
> > 
> > Ok, 4 is awesome. 
> > Then I have to notice: 
> > 1) The idea of this object, is that it's added to global rules list with initial counter, equal to 1. Which means, it's hold by the kernel itself. And this doesn't allow to destroy this rule before network namespace is alive. At least this was the intention in the original patch. 
> > 2) It is removed in the place you mentioned in fib_rules_cleanup_ops. If refcnt is really equal to 4, then something is seriously broken, and we have to leak such rules all the time. 
>
> Yes, and fib_rules_lookup() looks very suspicious to me. It could increment refcnt, but I'm failed to find corresponding decrement. 
>

+1
I'm curious, why network namespace is not reported as leaked...

>
> > 3) This object holds network namespace. And it also have to be leaked leaked after CT stop. Is it? 
> > 
> >> 
> >> 
> >> 
> >>> +    kmemleak_ignore(r); 
> >>>       return 0; 
> >>>   } 
> >>>   EXPORT_SYMBOL(fib_default_rule_add); 
> >>> 
> >