23 нояб. 2015 г. 16:34 пользователь Andrey Ryabinin <aryabi...@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 <skinsbur...@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); > >>> > > _______________________________________________ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
