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

[Alexander Potapenko]

On Fri, Jul 8, 2016 at 5:31 PM, Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx> wrote:
>
>
> 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?
Yes, since the new cache is created using zalloc() (which I didn't
know before) I'd better remove this assignment.
>
>
>>>>
>>>>  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.
>>>
>>>



-- 
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