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.
> - 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.
> }
>
> -#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.
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.
> 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.
> + 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.
> 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?
> + 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
...
