On 07/08/2016 01:36 PM, Alexander Potapenko wrote:
> On Tue, Jun 28, 2016 at 6:51 PM, Andrey Ryabinin
> <[email protected]> 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.
>>
>>
