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