On Thu, Jun 9, 2016 at 8:22 PM, Alexander Potapenko <glider@xxxxxxxxxx> wrote:
> 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.
Turns out we cannot just skip cache creation if we don't fit in
KMALLOC_MAX_SIZE, because SLAB, among others, creates kmalloc-4194304.
So we must keep an attribute that denotes that this is a KASAN-enabled
slab (i.e. the SLAB_KASAN flag), or set alloc_meta_offset and
free_meta_offset to -1, which isn't so convenient.
I suggest we stick with SLAB_KASAN.
>>
>>> 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
--
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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href