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

[Stanislav Kinsburskiy]

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