On 01/30/2015 02:12 AM, Andrew Morton wrote:
> On Thu, 29 Jan 2015 18:11:45 +0300 Andrey Ryabinin <a.ryabinin@xxxxxxxxxxx> 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@xxxxxxxxx>
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
