lkml.org 
[lkml]   [2015]   [Jan]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v10 01/17] Add kernel address sanitizer infrastructure.
On 01/30/2015 02:12 AM, Andrew Morton wrote:
> On Thu, 29 Jan 2015 18:11:45 +0300 Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
>
>> Kernel Address sanitizer (KASan) is a dynamic memory error detector. It provides
>> fast and comprehensive solution for finding use-after-free and out-of-bounds bugs.
>>
>> KASAN uses compile-time instrumentation for checking every memory access,
>> therefore GCC >= v4.9.2 required.
>>
>> ...
>>
>> Based on work by Andrey Konovalov <adech.fo@gmail.com>
>
> Can we obtain Andrey's signed-off-by: please?
>

I'll ask.

...

>> +static __always_inline bool memory_is_poisoned_1(unsigned long addr)
>
> What's with all the __always_inline in this file? When I remove them
> all, kasan.o .text falls from 8294 bytes down to 4543 bytes. That's
> massive, and quite possibly faster.
>
> If there's some magical functional reason for this then can we please
> get a nice prominent comment into this code apologetically explaining
> it?
>

The main reason is performance. __always_inline especially needed for check_memory_region()
and memory_is_poisoned() to optimize away switch in memory_is_poisoned():

if (__builtin_constant_p(size)) {
switch (size) {
case 1:
return memory_is_poisoned_1(addr);
case 2:
return memory_is_poisoned_2(addr);
case 4:
return memory_is_poisoned_4(addr);
case 8:
return memory_is_poisoned_8(addr);
case 16:
return memory_is_poisoned_16(addr);
default:
BUILD_BUG();
}
}

Always inlining memory_is_poisoned_x() gives additionally about 7%-10%.

According to my simple testing __always_inline gives about 20% versus
not inlined version of kasan.c


...

>> +
>> +void __asan_loadN(unsigned long addr, size_t size)
>> +{
>> + check_memory_region(addr, size, false);
>> +}
>> +EXPORT_SYMBOL(__asan_loadN);
>> +
>> +__attribute__((alias("__asan_loadN")))
>
> Maybe we need a __alias. Like __packed and various other helpers.
>

Ok.

....

>> +
>> +static __always_inline void kasan_report(unsigned long addr,
>> + size_t size,
>> + bool is_write)
>
> Again, why the inline? This is presumably not a hotpath and
> kasan_report has sixish call sites.
>

The reason of __always_inline here is to get correct _RET_IP_.
I could pass it from above and drop always inline here.

>
>> +{
>> + struct access_info info;
>> +
>> + if (likely(!kasan_enabled()))
>> + return;
>> +
>> + info.access_addr = addr;
>> + info.access_size = size;
>> + info.is_write = is_write;
>> + info.ip = _RET_IP_;
>> + kasan_report_error(&info);
>> +}
>>
...

>> +
>> +static void print_address_description(struct access_info *info)
>> +{
>> + dump_stack();
>> +}
>
> dump_stack() uses KERN_INFO but the callers or
> print_address_description() use KERN_ERR. This means that at some
> settings of `dmesg -n', the kasan output will have large missing
> chunks.
>
> Please test this and deide how bad it is. A proper fix will be
> somewhat messy (new_dump_stack(KERN_ERR)).
>

This new_dump_stack() could be useful in other places.
E.g. object_err()/slab_err() in SLUB also use pr_err() + dump_stack() combination.





\
 
 \ /
  Last update: 2015-01-30 17:21    [W:0.225 / U:0.732 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site