Re: [PATCH 2/2] mm: kasan: unified support for SLUB and SLAB allocators
From: Alexander Potapenko
Date: Mon Nov 02 2015 - 13:39:34 EST
I must admit the original patch was of poor quality, and my decision
to submit it unchanged wasn't a well-thought one.
I'll probably need to rework it and send again.
The idea behind removing the alloc/dealloc stacks was to replace them
with a better stack storage in
https://github.com/steelannelida/kasan/commit/7c9b30f499dfd5f48b39fbbd0006c788bd72f72a
However the patch shouldn't have introduced regressions against the
current behavior.
I am going to follow this plan:
-- introduce support for SLAB allocator (which will also include
addition of GFP flags to the API)
-- add the stack depot storage for SLAB
-- switch KASAN/SLUB to use stack depot instead of SLUB_DEBUG
-- add the allocator quarantine
(https://github.com/steelannelida/kasan/commit/b9fa66aa2057eeb6a4f537b6edfb85e4961d06ea)
On Fri, Oct 30, 2015 at 2:22 AM, Andrey Ryabinin <ryabinin.a.a@xxxxxxxxx> wrote:
> On 10/28/2015 07:41 PM, Alexander Potapenko wrote:
>> With this patch kasan can be compiled with both SLAB and SLUB allocators,
>> using minimal dependencies on allocator internal structures and minimum
>> allocator-dependent code.
>>
>> Dependency from SLUB_DEBUG is also removed. The metadata storage is made
>> more efficient, so the redzones aren't as large for small objects. The
>> redzone size is calculated based on the object size.
>>
>> This is the second part of the "mm: kasan: unified support for SLUB and
>> SLAB allocators" patch originally prepared by Dmitry Chernenkov.
>>
>> Signed-off-by: Dmitry Chernenkov <dmitryc@xxxxxxxxxx>
>> Signed-off-by: Alexander Potapenko <glider@xxxxxxxxxx>
>>
>
> Besides adding SLAB support, this patch seriously messes up SLUB-KASAN part.
> Changelog doesn't mention why this was done and what was done.
> So this patch should be split into two patches at least, 1 - mess up SLUB part,
> 2 - add SLAB support.
> And "mess up SLUB part" patch should very well explain why doing what you are doing.
> E.g. you just removed user tracking (object's Alloc/Free backtraces), and I'm failing
> to see, how this is an improvement.
> Therefore I didn't look into details, just commented some evident things, see below.
>
>> + ==================================================================
>> + BUG: KASan: out of bounds access in kmalloc_oob_right+0xce/0x117 [test_kasan] at addr ffff8800b91250fb
>> + Read of size 1 by task insmod/2754
>> + CPU: 0 PID: 2754 Comm: insmod Not tainted 4.0.0-rc4+ #1
>> + Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> + ffff8800b9125080 ffff8800b9aff958 ffffffff82c97b9e 0000000000000022
>> + ffff8800b9affa00 ffff8800b9aff9e8 ffffffff813fc8c9 ffff8800b9aff988
>> + ffffffff813fb3ff ffff8800b9aff998 0000000000000296 000000000000007b
>> + Call Trace:
>> + [<ffffffff82c97b9e>] dump_stack+0x45/0x57
>> + [<ffffffff813fc8c9>] kasan_report_error+0x129/0x420
>> + [<ffffffff813fb3ff>] ? kasan_poison_shadow+0x2f/0x40
>> + [<ffffffff813fb3ff>] ? kasan_poison_shadow+0x2f/0x40
>> + [<ffffffff813fbeff>] ? kasan_kmalloc+0x5f/0x100
>> + [<ffffffffa0008f3d>] ? kmalloc_node_oob_right+0x11f/0x11f [test_kasan]
>> + [<ffffffff813fcc05>] __asan_report_load1_noabort+0x45/0x50
>> + [<ffffffffa0008f00>] ? kmalloc_node_oob_right+0xe2/0x11f [test_kasan]
>> + [<ffffffffa00087bf>] ? kmalloc_oob_right+0xce/0x117 [test_kasan]
>> + [<ffffffffa00087bf>] kmalloc_oob_right+0xce/0x117 [test_kasan]
>> + [<ffffffffa00086f1>] ? kmalloc_oob_left+0xe9/0xe9 [test_kasan]
>> + [<ffffffff819cc140>] ? kvasprintf+0xf0/0xf0
>> + [<ffffffffa00086f1>] ? kmalloc_oob_left+0xe9/0xe9 [test_kasan]
>> + [<ffffffffa000001e>] run_test+0x1e/0x40 [test_kasan]
>> + [<ffffffffa0008f54>] init_module+0x17/0x128 [test_kasan]
>> + [<ffffffff81000351>] do_one_initcall+0x111/0x2b0
>> + [<ffffffff81000240>] ? try_to_run_init_process+0x40/0x40
>> + [<ffffffff813fb3ff>] ? kasan_poison_shadow+0x2f/0x40
>> + [<ffffffff813fbeff>] ? kasan_kmalloc+0x5f/0x100
>> + [<ffffffff813fb3ff>] ? kasan_poison_shadow+0x2f/0x40
>> + [<ffffffff813fbde4>] ? kasan_unpoison_shadow+0x14/0x40
>> + [<ffffffff813fb3ff>] ? kasan_poison_shadow+0x2f/0x40
>> + [<ffffffff813fbe80>] ? __asan_register_globals+0x70/0x90
>> + [<ffffffff82c934a4>] do_init_module+0x1d2/0x531
>> + [<ffffffff8122d5bf>] load_module+0x55cf/0x73e0
>> + [<ffffffff81224020>] ? symbol_put_addr+0x50/0x50
>> + [<ffffffff81227ff0>] ? module_frob_arch_sections+0x20/0x20
>> + [<ffffffff810c213a>] ? trace_do_page_fault+0x6a/0x1d0
>> + [<ffffffff810b5454>] ? do_async_page_fault+0x14/0x80
>> + [<ffffffff82cb0c88>] ? async_page_fault+0x28/0x30
>> + [<ffffffff8122f4da>] SyS_init_module+0x10a/0x140
>> + [<ffffffff8122f3d0>] ? load_module+0x73e0/0x73e0
>> + [<ffffffff82caef89>] system_call_fastpath+0x12/0x17
>> + Object at ffff8800b9125080, in cache kmalloc-128
>> + Object allocated with size 123 bytes.
>> + Allocation:
>> + PID = 2754, CPU = 0, timestamp = 4294707705
>
> Really? Just this instead of Alloc/Free backtraces?
>
>
>> + Memory state around the buggy address:
>> + ffff8800b9124f80: fc fc fc fc fc fc fc fc 00 00 00 00 00 00 00 00
>> + ffff8800b9125000: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
>> + >ffff8800b9125080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 03
>> + ^
>> + ffff8800b9125100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> + ffff8800b9125180: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> + ==================================================================
>>
>> In the last section the report shows memory state around the accessed address.
>> Reading this part requires some more understanding of how KASAN works.
>> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
>> index e1ce960..e37d934 100644
>> --- a/include/linux/kasan.h
>> +++ b/include/linux/kasan.h
>> @@ -7,6 +7,12 @@ struct kmem_cache;
>> struct page;
>> struct vm_struct;
>>
>> +#ifdef SLAB
>> +#define cache_size_t size_t
>> +#else
>> +#define cache_size_t unsigned long
>> +#endif
>> +
>
> Ugh... Why we can't use same type in all allocators?
>
>> #ifdef CONFIG_KASAN
>>
>> #define KASAN_SHADOW_SCALE_SHIFT 3
>> @@ -46,6 +52,9 @@ void kasan_unpoison_shadow(const void *address, size_t size);
>> void kasan_alloc_pages(struct page *page, unsigned int order);
>> void kasan_free_pages(struct page *page, unsigned int order);
>>
>> +void kasan_cache_create(struct kmem_cache *cache, cache_size_t *size,
>> + unsigned long *flags);
>> +
>> void kasan_poison_slab(struct page *page);
>> void kasan_unpoison_object_data(struct kmem_cache *cache, void *object);
>> void kasan_poison_object_data(struct kmem_cache *cache, void *object);
>> @@ -60,6 +69,11 @@ void kasan_krealloc(const void *object, size_t new_size, gfp_t flags);
>> void kasan_slab_alloc(struct kmem_cache *s, void *object, gfp_t flags);
>> void kasan_slab_free(struct kmem_cache *s, void *object);
>>
>> +struct kasan_cache {
>> + int alloc_meta_offset;
>> + int free_meta_offset;
>> +};
>> +
>> int kasan_module_alloc(void *addr, size_t size);
>> void kasan_free_shadow(const struct vm_struct *vm);
>>
>> @@ -73,6 +87,10 @@ static inline void kasan_disable_current(void) {}
>> static inline void kasan_alloc_pages(struct page *page, unsigned int order) {}
>> static inline void kasan_free_pages(struct page *page, unsigned int order) {}
>>
>> +static inline void kasan_cache_create(struct kmem_cache *cache,
>> + cache_size_t *size,
>> + unsigned long *flags) {}
>> +
>> static inline void kasan_poison_slab(struct page *page) {}
>> static inline void kasan_unpoison_object_data(struct kmem_cache *cache,
>> void *object) {}
>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>> index 7e37d44..b4de362 100644
>> --- a/include/linux/slab.h
>> +++ b/include/linux/slab.h
>> @@ -87,6 +87,12 @@
>> # define SLAB_FAILSLAB 0x00000000UL
>> #endif
>>
>> +#ifdef CONFIG_KASAN
>> +#define SLAB_KASAN 0x08000000UL
>> +#else
>> +#define SLAB_KASAN 0x00000000UL
>> +#endif
>> +
>
> What's this for? KASAN tracks all kmem_caches, so why we need the runtime flag?
>
>
>> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
>> index c1efb1b..0ae338c 100644
>> --- a/lib/test_kasan.c
>> +++ b/lib/test_kasan.c
>> @@ -259,7 +259,9 @@ static int __init kmalloc_tests_init(void)
>> kmalloc_oob_right();
>> kmalloc_oob_left();
>> kmalloc_node_oob_right();
>> +#ifdef CONFIG_SLUB
>> kmalloc_large_oob_right();
>> +#endif
>
> I don't understand this.
>
>
>>
>> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
>> index c242adf..6530880 100644
>> --- a/mm/kasan/kasan.h
>> +++ b/mm/kasan/kasan.h
>> @@ -54,6 +54,39 @@ struct kasan_global {
>> #endif
>> };
>>
>> +/**
>> + * Structures to keep alloc and free tracks *
>> + */
>> +
>> +enum kasan_state {
>> + KASAN_STATE_INIT,
>> + KASAN_STATE_ALLOC,
>> + KASAN_STATE_FREE
>> +};
>> +
>> +/* TODO: rethink the structs and field sizes */
>> +struct kasan_track {
>> + u64 cpu : 6; /* for NR_CPUS = 64 */
>> + u64 pid : 16; /* 65536 processes */
>> + u64 when : 48; /* ~9000 years */
>> +};
>> +
>> +struct kasan_alloc_meta {
>> + enum kasan_state state : 2;
>> + size_t alloc_size : 30;
>> + struct kasan_track track;
>> +};
>> +
>> +struct kasan_free_meta {
>> + void **freelist;
>
> This field is unused.
>
>
>> + struct kasan_track track;
>> +};
>> +
>> +struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
>> + const void *object);
>> +struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
>> + const void *object);
>> +
>> void kasan_report_error(struct kasan_access_info *info);
>> void kasan_report_user_access(struct kasan_access_info *info);
>>
>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>> index e07c94f..7dbe5be 100644
>> --- a/mm/kasan/report.c
>> +++ b/mm/kasan/report.c
>> @@ -97,6 +97,58 @@ static inline bool init_task_stack_addr(const void *addr)
>> sizeof(init_thread_union.stack));
>> }
>>
>> +static void print_track(struct kasan_track *track)
>> +{
>> + pr_err("PID = %lu, CPU = %lu, timestamp = %lu\n", track->pid,
>> + track->cpu, track->when);
>> +}
>> +
>> +static void print_object(struct kmem_cache *cache, void *object)
>> +{
>> + struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
>> + struct kasan_free_meta *free_info;
>> +
>> + pr_err("Object at %p, in cache %s\n", object, cache->name);
>> + if (!(cache->flags & SLAB_KASAN))
>> + return;
>> + switch (alloc_info->state) {
>> + case KASAN_STATE_INIT:
>> + pr_err("Object not allocated yet\n");
>> + break;
>> + case KASAN_STATE_ALLOC:
>> + pr_err("Object allocated with size %lu bytes.\n",
>> + alloc_info->alloc_size);
>> + pr_err("Allocation:\n");
>> + print_track(&alloc_info->track);
>> + break;
>> + case KASAN_STATE_FREE:
>> + pr_err("Object freed, allocated with size %lu bytes\n",
>> + alloc_info->alloc_size);
>> + free_info = get_free_info(cache, object);
>> + pr_err("Allocation:\n");
>> + print_track(&alloc_info->track);
>> + pr_err("Deallocation:\n");
>> + print_track(&free_info->track);
>> + break;
>> + }
>> +}
>> +
>> +static inline void *nearest_obj(struct kmem_cache *cache, struct page *page,
>> + void *x) {
>> +#if defined(CONFIG_SLUB)
>> + void *object = x - (x - page_address(page)) % cache->size;
>> + void *last_object = page_address(page) +
>> + (page->objects - 1) * cache->size;
>> +#elif defined(CONFIG_SLAB)
>> + void *object = x - (x - page->s_mem) % cache->size;
>> + void *last_object = page->s_mem + (cache->num - 1) * cache->size;
>> +#endif
>
> Instead of ifdefs, provide functions per allocator in respective headers (include/linux/sl?b_def.h).
>
>> + if (unlikely(object > last_object))
>> + return last_object;
>> + else
>> + return object;
>> +}
>> +
>> static void print_address_description(struct kasan_access_info *info)
>> {
>> const void *addr = info->access_addr;
>> @@ -108,17 +160,10 @@ static void print_address_description(struct kasan_access_info *info)
>> if (PageSlab(page)) {
>> void *object;
>> struct kmem_cache *cache = page->slab_cache;
>> - void *last_object;
>> -
>> - object = virt_to_obj(cache, page_address(page), addr);
>> - last_object = page_address(page) +
>> - page->objects * cache->size;
>>
>> - if (unlikely(object > last_object))
>> - object = last_object; /* we hit into padding */
>> -
>> - object_err(cache, page, object,
>> - "kasan: bad access detected");
>> + object = nearest_obj(cache, page,
>> + (void *)info->access_addr);
>> + print_object(cache, object);
>> return;
>> }
>> dump_page(page, "kasan: bad access detected");
>> @@ -128,8 +173,6 @@ static void print_address_description(struct kasan_access_info *info)
>> if (!init_task_stack_addr(addr))
>> pr_err("Address belongs to variable %pS\n", addr);
>> }
>> -
>> - dump_stack();
>> }
>>
>> static bool row_is_guilty(const void *row, const void *guilty)
>> @@ -186,21 +229,25 @@ void kasan_report_error(struct kasan_access_info *info)
>> {
>> unsigned long flags;
>>
>> + kasan_disable_current();
>
> We already have patch doing this in -mm tree.
>
>> spin_lock_irqsave(&report_lock, flags);
>> pr_err("================================="
>> "=================================\n");
>> print_error_description(info);
>> + dump_stack();
>> print_address_description(info);
>> print_shadow_for_address(info->first_bad_addr);
>> pr_err("================================="
>> "=================================\n");
>> spin_unlock_irqrestore(&report_lock, flags);
>> + kasan_enable_current();
>> }
>>
>
>
>
>> slab_bug(s, "%s", reason);
>> @@ -1303,7 +1308,6 @@ static inline void slab_free_hook(struct kmem_cache *s, void *x)
>> if (!(s->flags & SLAB_DEBUG_OBJECTS))
>> debug_check_no_obj_freed(x, s->object_size);
>>
>> - kasan_slab_free(s, x);
>> }
>>
>
> ...
>
>> @@ -2698,6 +2703,15 @@ slab_empty:
>> static __always_inline void slab_free(struct kmem_cache *s,
>> struct page *page, void *x, unsigned long addr)
>> {
>> +#ifdef CONFIG_KASAN
>> + kasan_slab_free(s, x);
>> + nokasan_free(s, x, addr);
>> +}
>> +
>> +void nokasan_free(struct kmem_cache *s, void *x, unsigned long addr)
>> +{
>> + struct page *page = virt_to_head_page(x);
>> +#endif
>
> What is this? The only reason I could imagine for this crap is, to make
> this code ugly. This change has no any functional effect.
>
>
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
DienerstraÃe 12
80331 MÃnchen
GeschÃftsfÃhrer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und lÃschen Sie die E-Mail und alle AnhÃnge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/