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

[Andrey Ryabinin]

On 07/08/2016 01:36 PM, Alexander Potapenko wrote:
> On Tue, Jun 28, 2016 at 6:51 PM, Andrey Ryabinin
> <aryabinin@xxxxxxxxxxxxx> wrote:

>>>       *flags |= SLAB_KASAN;
>>> +
>>>       /* Add alloc meta. */
>>>       cache->kasan_info.alloc_meta_offset = *size;
>>>       *size += sizeof(struct kasan_alloc_meta);
>>> @@ -392,17 +387,35 @@ 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;
>>
>> Why is that required now?
> Because we want to store the free metadata in the object when it's possible.

We did the before this patch. free_meta_offset is 0 by default, thus there was no need to nullify it here.
But now this patch suddenly adds reset of free_meta_offset. So I'm asking why?
Is free_meta_offset not 0 by default anymore? 



>>>
>>>  void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
>>> @@ -568,6 +573,9 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
>>>       if (unlikely(object == NULL))
>>>               return;
>>>
>>> +     if (!(cache->flags & SLAB_KASAN))
>>> +             return;
>>> +
>>
>> This hunk is superfluous and wrong.
> Can you please elaborate?
> Do you mean we don't need to check for SLAB_KASAN here, or that we
> don't need SLAB_KASAN at all?

The former, we can poison/unpoison !SLAB_KASAN caches too.



>>>  }
>>>
>>> @@ -2772,12 +2788,22 @@ static __always_inline void slab_free(struct kmem_cache *s, struct page *page,
>>>                                     void *head, void *tail, int cnt,
>>>                                     unsigned long addr)
>>>  {
>>> +     void *free_head = head, *free_tail = tail;
>>> +
>>> +     slab_free_freelist_hook(s, &free_head, &free_tail, &cnt);
>>> +     /* slab_free_freelist_hook() could have emptied the freelist. */
>>> +     if (cnt == 0)
>>> +             return;
>>
>> I suppose that we can do something like following, instead of that mess in slab_free_freelist_hook() above
>>
>>         slab_free_freelist_hook(s, &free_head, &free_tail);
>>         if (s->flags & SLAB_KASAN && s->flags & SLAB_DESTROY_BY_RCU)
> Did you mean "&& !(s->flags & SLAB_DESTROY_BY_RCU)" ?

Sure.

>>                 return;
> Yes, my code is overly complicated given that kasan_slab_free() should
> actually return the same value for every element of the list.
> (do you think it makes sense to check that?)

IMO that's would be superfluous.

> I can safely remove those freelist manipulations.
>>
>>