Re: [PATCH] mm, kasan: switch SLUB to stackdepot, enable memory quarantine for SLUB

From: Alexander Potapenko
Date: Thu Jun 09 2016 - 14:22:41 EST

Next message: Mel Gorman: "[PATCH 26/27] mm: vmstat: Replace __count_zone_vm_events with a zone id equivalent"
Previous message: kbuild test robot: "Re: [PATCH 5/6] memory: omap-gpmc: Enable omap-gpmc by default when dependency is met"
In reply to: Andrey Ryabinin: "Re: [PATCH] mm, kasan: switch SLUB to stackdepot, enable memory quarantine for SLUB"
Next in thread: Alexander Potapenko: "Re: [PATCH] mm, kasan: switch SLUB to stackdepot, enable memory quarantine for SLUB"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Jun 9, 2016 at 6:45 PM, Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx> wrote:
>
>
> On 06/08/2016 09:40 PM, Alexander Potapenko wrote:
>> For KASAN builds:
>> - switch SLUB allocator to using stackdepot instead of storing the
>> allocation/deallocation stacks in the objects;
>> - define SLAB_RED_ZONE, SLAB_POISON, SLAB_STORE_USER to zero,
>> effectively disabling these debug features, as they're redundant in
>> the presence of KASAN;
>
> Instead of having duplicated functionality, I think it might be better to switch SLAB_STORE_USER to stackdepot instead.
> Because now, we have two piles of code which do basically the same thing, but
> do differently.
Fine, I'll try that out.
>> - refactor the slab freelist hook, put freed memory into the quarantine.
>>
>
> What you did with slab_freelist_hook() is not refactoring, it's an obfuscation.
Whatever you call it.
The problem is that if a list of heterogeneous objects is passed into
slab_free_freelist_hook(), some of them may end up in the quarantine,
while others will not.
Therefore we need to filter that list and remove the objects that
don't need to be freed from it.
>
>> }
>>
>> -#ifdef CONFIG_SLAB
>> /*
>> * Adaptive redzone policy taken from the userspace AddressSanitizer runtime.
>> * For larger allocations larger redzones are used.
>> @@ -372,17 +371,21 @@ static size_t optimal_redzone(size_t object_size)
>> void kasan_cache_create(struct kmem_cache *cache, size_t *size,
>> unsigned long *flags)
>> {
>> - int redzone_adjust;
>> - /* Make sure the adjusted size is still less than
>> - * KMALLOC_MAX_CACHE_SIZE.
>> - * TODO: this check is only useful for SLAB, but not SLUB. We'll need
>> - * to skip it for SLUB when it starts using kasan_cache_create().
>> + int redzone_adjust, orig_size = *size;
>> +
>> +#ifdef CONFIG_SLAB
>> + /*
>> + * Make sure the adjusted size is still less than
>> + * KMALLOC_MAX_CACHE_SIZE, i.e. we don't use the page allocator.
>> */
>> +
>> if (*size > KMALLOC_MAX_CACHE_SIZE -
>
> This is wrong. You probably wanted KMALLOC_MAX_SIZE here.
Yeah, sonds right.
> However, we should get rid of SLAB_KASAN altogether. It's absolutely useless, and only complicates
> the code. And if we don't fit in KMALLOC_MAX_SIZE, just don't create cache.
Thanks, I'll look into this. Looks like you are right, once we remove
this check every existing cache will have SLAB_KASAN set.
It's handy for debugging, but not really needed.
>
>> sizeof(struct kasan_alloc_meta) -
>> sizeof(struct kasan_free_meta))
>> return;
>> +#endif
>> *flags |= SLAB_KASAN;
>> +
>> /* Add alloc meta. */
>> cache->kasan_info.alloc_meta_offset = *size;
>> *size += sizeof(struct kasan_alloc_meta);
>> @@ -392,17 +395,37 @@ void kasan_cache_create(struct kmem_cache *cache, size_t *size,
>> cache->object_size < sizeof(struct kasan_free_meta)) {
>> cache->kasan_info.free_meta_offset = *size;
>> *size += sizeof(struct kasan_free_meta);
>> + } else {
>> + cache->kasan_info.free_meta_offset = 0;
>> }
>> redzone_adjust = optimal_redzone(cache->object_size) -
>> (*size - cache->object_size);
>> +
>> if (redzone_adjust > 0)
>> *size += redzone_adjust;
>> +
>> +#ifdef CONFIG_SLAB
>> *size = min(KMALLOC_MAX_CACHE_SIZE,
>> max(*size,
>> cache->object_size +
>> optimal_redzone(cache->object_size)));
>> -}
>> + /*
>> + * If the metadata doesn't fit, disable KASAN at all.
>> + */
>> + if (*size <= cache->kasan_info.alloc_meta_offset ||
>> + *size <= cache->kasan_info.free_meta_offset) {
>> + *flags &= ~SLAB_KASAN;
>> + *size = orig_size;
>> + cache->kasan_info.alloc_meta_offset = -1;
>> + cache->kasan_info.free_meta_offset = -1;
>> + }
>> +#else
>> + *size = max(*size,
>> + cache->object_size +
>> + optimal_redzone(cache->object_size));
>> +
>> #endif
>> +}
>>
>> void kasan_cache_shrink(struct kmem_cache *cache)
>> {
>> @@ -431,16 +454,14 @@ void kasan_poison_object_data(struct kmem_cache *cache, void *object)
>> kasan_poison_shadow(object,
>> round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE),
>> KASAN_KMALLOC_REDZONE);
>> -#ifdef CONFIG_SLAB
>> if (cache->flags & SLAB_KASAN) {
>> struct kasan_alloc_meta *alloc_info =
>> get_alloc_info(cache, object);
>> - alloc_info->state = KASAN_STATE_INIT;
>> + if (alloc_info)
>
> If I read the code right alloc_info can be NULL only if SLAB_KASAN is set.
This has been left over from tracking down some nasty bugs, but, yes,
we can assume alloc_info and free_info are always valid.
>
>> + alloc_info->state = KASAN_STATE_INIT;
>> }
>> -#endif
>> }
>>
>> -#ifdef CONFIG_SLAB
>> static inline int in_irqentry_text(unsigned long ptr)
>> {
>> return (ptr >= (unsigned long)&__irqentry_text_start &&
>> @@ -492,6 +513,8 @@ struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
>> const void *object)
>> {
>> BUILD_BUG_ON(sizeof(struct kasan_alloc_meta) > 32);
>> + if (cache->kasan_info.alloc_meta_offset == -1)
>> + return NULL;
>
> What's the point of this ? This should be always false.
Agreed, will remove this (and other similar cases).
>> return (void *)object + cache->kasan_info.alloc_meta_offset;
>> }
>>
>> @@ -499,9 +522,10 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
>> const void *object)
>> {
>> BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32);
>> + if (cache->kasan_info.free_meta_offset == -1)
>> + return NULL;
>> return (void *)object + cache->kasan_info.free_meta_offset;
>> }
>> -#endif
>>
>> void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
>> {
>> @@ -522,7 +546,6 @@ void kasan_poison_slab_free(struct kmem_cache *cache, void *object)
>>
>> bool kasan_slab_free(struct kmem_cache *cache, void *object)
>> {
>> -#ifdef CONFIG_SLAB
>> /* RCU slabs could be legally used after free within the RCU period */
>> if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
>> return false;
>> @@ -532,7 +555,10 @@ bool kasan_slab_free(struct kmem_cache *cache, void *object)
>> get_alloc_info(cache, object);
>> struct kasan_free_meta *free_info =
>> get_free_info(cache, object);
>> -
>> + WARN_ON(!alloc_info);
>> + WARN_ON(!free_info);
>> + if (!alloc_info || !free_info)
>> + return;
>
> Again, never possible.
>
>
>> switch (alloc_info->state) {
>> case KASAN_STATE_ALLOC:
>> alloc_info->state = KASAN_STATE_QUARANTINE;
>> @@ -550,10 +576,6 @@ bool kasan_slab_free(struct kmem_cache *cache, void *object)
>> }
>> }
>> return false;
>> -#else
>> - kasan_poison_slab_free(cache, object);
>> - return false;
>> -#endif
>> }
>>
>> void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
>> @@ -568,24 +590,29 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
>> if (unlikely(object == NULL))
>> return;
>>
>> + if (!(cache->flags & SLAB_KASAN))
>> + return;
>> +
>> redzone_start = round_up((unsigned long)(object + size),
>> KASAN_SHADOW_SCALE_SIZE);
>> redzone_end = round_up((unsigned long)object + cache->object_size,
>> KASAN_SHADOW_SCALE_SIZE);
>>
>> kasan_unpoison_shadow(object, size);
>> + WARN_ON(redzone_start > redzone_end);
>> + if (redzone_start > redzone_end)
>
> How that's can happen?
This was possible because of incorrect ksize implementation, should be
now ok. Removed.
>> + return;
>> kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start,
>> KASAN_KMALLOC_REDZONE);
>> -#ifdef CONFIG_SLAB
>> if (cache->flags & SLAB_KASAN) {
>> struct kasan_alloc_meta *alloc_info =
>> get_alloc_info(cache, object);
>> -
>> - alloc_info->state = KASAN_STATE_ALLOC;
>> - alloc_info->alloc_size = size;
>> - set_track(&alloc_info->track, flags);
>> + if (alloc_info) {
>
> And again...
>
>
>> + alloc_info->state = KASAN_STATE_ALLOC;
>> + alloc_info->alloc_size = size;
>> + set_track(&alloc_info->track, flags);
>> + }
>> }
>> -#endif
>> }
>> EXPORT_SYMBOL(kasan_kmalloc);
>>
>
>
> [..]
>
>> diff --git a/mm/slab.h b/mm/slab.h
>> index dedb1a9..fde1fea 100644
>> --- a/mm/slab.h
>> +++ b/mm/slab.h
>> @@ -366,6 +366,10 @@ static inline size_t slab_ksize(const struct kmem_cache *s)
>> if (s->flags & (SLAB_RED_ZONE | SLAB_POISON))
>> return s->object_size;
>> # endif
>> +# ifdef CONFIG_KASAN
>
> Gush, you love ifdefs, don't you? Hint: it's redundant here.
>
>> + if (s->flags & SLAB_KASAN)
>> + return s->object_size;
>> +# endif
>> /*
>> * If we have the need to store the freelist pointer
> ...

--
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-StraÃe, 33
80636 MÃnchen

GeschÃftsfÃhrer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Next message: Mel Gorman: "[PATCH 26/27] mm: vmstat: Replace __count_zone_vm_events with a zone id equivalent"
Previous message: kbuild test robot: "Re: [PATCH 5/6] memory: omap-gpmc: Enable omap-gpmc by default when dependency is met"
In reply to: Andrey Ryabinin: "Re: [PATCH] mm, kasan: switch SLUB to stackdepot, enable memory quarantine for SLUB"
Next in thread: Alexander Potapenko: "Re: [PATCH] mm, kasan: switch SLUB to stackdepot, enable memory quarantine for SLUB"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]