Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Mon, May 9, 2016 at 2:43 PM, Andrey Ryabinin  wrote:
>
>
> On 05/09/2016 01:31 PM, Dmitry Vyukov wrote:
>> On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
>>  wrote:
>>>
>>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>>> index b3c122d..c2b0e51 100644
>>> --- a/mm/kasan/report.c
>>> +++ b/mm/kasan/report.c
>>> @@ -140,18 +140,12 @@ static void object_err(struct kmem_cache *cache, 
>>> struct page *page,
>>> pr_err("Object at %p, in cache %s\n", object, cache->name);
>>> if (!(cache->flags & SLAB_KASAN))
>>> return;
>>> -   switch (alloc_info->state) {
>>> -   case KASAN_STATE_INIT:
>>> -   pr_err("Object not allocated yet\n");
>>> -   break;
>>> -   case KASAN_STATE_ALLOC:
>>> +   if (test_bit(KASAN_STATE_ALLOCATED, _info->state)) {
>>> pr_err("Object allocated with size %u bytes.\n",
>>>alloc_info->alloc_size);
>>> pr_err("Allocation:\n");
>>> print_track(_info->track);
>>
>> alloc_info->track is not necessary initialized when
>> KASAN_STATE_ALLOCATED is set.
>
> It should be initialized to something. If it's not initialized, than that 
> object wasn't allocated.
> So this would be a *very* random pointer access. Also this would mean that 
> ->state itself might be not initialized too.
> Anyway, we can't do much in such scenario since we can't trust any data.
> And I don't think that we should because very likely this will cause panic 
> eventually.
>
>> Worse, it can be initialized to a wrong
>> stack.
>>
>
> Define "wrong stack" here.
>
> I assume that you are talking about race in the following scenario:
>
> --
> Proccess A:  Proccess B:
>
> p_A = kmalloc();
> /* use p_A */
> kfree(p_A);
>  p_A = kmalloc();
>   
>   set_bit(KASAN_STATE_ALLOCATED); //bit 
> set, but stack is not saved yet.
> /* use after free p_A */
>
> if (test_bit(KASAN_STATE_ALLOCATED))
> print_stack() // will print track from Proccess A
> -
>
> So, would be the stack trace from A wrong in such situation? I don't think so.
> We could change ->state and save stack trace in the 'right' order with proper 
> barriers,
> but would it prevent us from showing wrong stacktrace? - It wouldn't.
>
> Now, the only real problem with current code, is that we don't print free 
> stack if we think that object is in
> allocated state. We should do this, because more information is always 
> better, e.g. we might hit long-delayed use-after-free,
> in which case free stack would be useful (just like in scenario above).



If we report a use-after-free on an object, we need to print
allocation stack of that object and free object for that object. If we
report a double-free, we need to print allocation and free stacks for
the object. That's correct stacks. Everything else is wrong.

With your patch we don't print free stack on double-free. We can't
print free_info->track because it is not necessary initialized at that
point. We can't fix it by simply reversing order of stores of state
and track, because then there is a data race on track during
double-free.

I agree that if we have a use-after-free when the block is being
reused for another allocation, we can't always print the right stacks
(they are already overwritten for the new allocation). But we should
avoid printing non-matching malloc/free stacks and treating random
bytes as stack trace handle (both cases are possible with your patch).
If we print bogus info that does not make sense, we compromise trust
in the tool. Some developers postulate that a tool is totally broken
and stop looking at reports as soon as they notice any incorrect info.

Kuthonuzo's patch allows to always print matching malloc/free stacks,
never treat random bytes as stack handle and detect some cases when we
know we have stale information (due to block reuse). There well may be
ways to simplify and improve it, but it introduces and uses a
consistent discipline for header updates/reads that will be useful as
we go forward.

Re block size increase: it's not that is super big deal. But the
increase is completely unnecessary. I've checked /proc/slabinfo on my
non-instrumented machine, and most objects are exactly in 32/64-byte
objects consuming 80MB in total. We've just got rid of fat inlined
stack traces and refactored alloc/free info to occupy 16 bytes each.
Let's not start increasing them again on any occasion.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Mon, May 9, 2016 at 2:43 PM, Andrey Ryabinin  wrote:
>
>
> On 05/09/2016 01:31 PM, Dmitry Vyukov wrote:
>> On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
>>  wrote:
>>>
>>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>>> index b3c122d..c2b0e51 100644
>>> --- a/mm/kasan/report.c
>>> +++ b/mm/kasan/report.c
>>> @@ -140,18 +140,12 @@ static void object_err(struct kmem_cache *cache, 
>>> struct page *page,
>>> pr_err("Object at %p, in cache %s\n", object, cache->name);
>>> if (!(cache->flags & SLAB_KASAN))
>>> return;
>>> -   switch (alloc_info->state) {
>>> -   case KASAN_STATE_INIT:
>>> -   pr_err("Object not allocated yet\n");
>>> -   break;
>>> -   case KASAN_STATE_ALLOC:
>>> +   if (test_bit(KASAN_STATE_ALLOCATED, _info->state)) {
>>> pr_err("Object allocated with size %u bytes.\n",
>>>alloc_info->alloc_size);
>>> pr_err("Allocation:\n");
>>> print_track(_info->track);
>>
>> alloc_info->track is not necessary initialized when
>> KASAN_STATE_ALLOCATED is set.
>
> It should be initialized to something. If it's not initialized, than that 
> object wasn't allocated.
> So this would be a *very* random pointer access. Also this would mean that 
> ->state itself might be not initialized too.
> Anyway, we can't do much in such scenario since we can't trust any data.
> And I don't think that we should because very likely this will cause panic 
> eventually.
>
>> Worse, it can be initialized to a wrong
>> stack.
>>
>
> Define "wrong stack" here.
>
> I assume that you are talking about race in the following scenario:
>
> --
> Proccess A:  Proccess B:
>
> p_A = kmalloc();
> /* use p_A */
> kfree(p_A);
>  p_A = kmalloc();
>   
>   set_bit(KASAN_STATE_ALLOCATED); //bit 
> set, but stack is not saved yet.
> /* use after free p_A */
>
> if (test_bit(KASAN_STATE_ALLOCATED))
> print_stack() // will print track from Proccess A
> -
>
> So, would be the stack trace from A wrong in such situation? I don't think so.
> We could change ->state and save stack trace in the 'right' order with proper 
> barriers,
> but would it prevent us from showing wrong stacktrace? - It wouldn't.
>
> Now, the only real problem with current code, is that we don't print free 
> stack if we think that object is in
> allocated state. We should do this, because more information is always 
> better, e.g. we might hit long-delayed use-after-free,
> in which case free stack would be useful (just like in scenario above).



If we report a use-after-free on an object, we need to print
allocation stack of that object and free object for that object. If we
report a double-free, we need to print allocation and free stacks for
the object. That's correct stacks. Everything else is wrong.

With your patch we don't print free stack on double-free. We can't
print free_info->track because it is not necessary initialized at that
point. We can't fix it by simply reversing order of stores of state
and track, because then there is a data race on track during
double-free.

I agree that if we have a use-after-free when the block is being
reused for another allocation, we can't always print the right stacks
(they are already overwritten for the new allocation). But we should
avoid printing non-matching malloc/free stacks and treating random
bytes as stack trace handle (both cases are possible with your patch).
If we print bogus info that does not make sense, we compromise trust
in the tool. Some developers postulate that a tool is totally broken
and stop looking at reports as soon as they notice any incorrect info.

Kuthonuzo's patch allows to always print matching malloc/free stacks,
never treat random bytes as stack handle and detect some cases when we
know we have stale information (due to block reuse). There well may be
ways to simplify and improve it, but it introduces and uses a
consistent discipline for header updates/reads that will be useful as
we go forward.

Re block size increase: it's not that is super big deal. But the
increase is completely unnecessary. I've checked /proc/slabinfo on my
non-instrumented machine, and most objects are exactly in 32/64-byte
objects consuming 80MB in total. We've just got rid of fat inlined
stack traces and refactored alloc/free info to occupy 16 bytes each.
Let's not start increasing them again on any occasion.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/09/2016 04:20 PM, Dmitry Vyukov wrote:
> On Mon, May 9, 2016 at 3:01 PM, Andrey Ryabinin  
> wrote:
>>
>>
>> On 05/09/2016 02:35 PM, Luruo, Kuthonuzo wrote:
>>>
>>> This patch with atomic bit op is similar in spirit to v1 except that it 
>>> increases metadata size.
>>>
>>
>> I don't think that this is a big deal. That will slightly increase size of 
>> objects <= (128 - 32) bytes.
>> And if someone think otherwise, we can completely remove 'alloc_size'
>> (we use it only to print size in report - not very useful).
> 
> 
> Where did 128 come from?
> We now should allocate only 32 bytes for 16-byte user object. If not,
> there is something to fix.
> 

I just said this wrong. I mean that the patch increases size of objects that 
have object_size <= (128 - 32).
For bigger objects, the new 'struct kasan_[alloc,free]_meta' still fits into 
optimal redzone.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/09/2016 04:20 PM, Dmitry Vyukov wrote:
> On Mon, May 9, 2016 at 3:01 PM, Andrey Ryabinin  
> wrote:
>>
>>
>> On 05/09/2016 02:35 PM, Luruo, Kuthonuzo wrote:
>>>
>>> This patch with atomic bit op is similar in spirit to v1 except that it 
>>> increases metadata size.
>>>
>>
>> I don't think that this is a big deal. That will slightly increase size of 
>> objects <= (128 - 32) bytes.
>> And if someone think otherwise, we can completely remove 'alloc_size'
>> (we use it only to print size in report - not very useful).
> 
> 
> Where did 128 come from?
> We now should allocate only 32 bytes for 16-byte user object. If not,
> there is something to fix.
> 

I just said this wrong. I mean that the patch increases size of objects that 
have object_size <= (128 - 32).
For bigger objects, the new 'struct kasan_[alloc,free]_meta' still fits into 
optimal redzone.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Mon, May 9, 2016 at 3:01 PM, Andrey Ryabinin  wrote:
>
>
> On 05/09/2016 02:35 PM, Luruo, Kuthonuzo wrote:
>>
>> This patch with atomic bit op is similar in spirit to v1 except that it 
>> increases metadata size.
>>
>
> I don't think that this is a big deal. That will slightly increase size of 
> objects <= (128 - 32) bytes.
> And if someone think otherwise, we can completely remove 'alloc_size'
> (we use it only to print size in report - not very useful).


Where did 128 come from?
We now should allocate only 32 bytes for 16-byte user object. If not,
there is something to fix.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Mon, May 9, 2016 at 3:01 PM, Andrey Ryabinin  wrote:
>
>
> On 05/09/2016 02:35 PM, Luruo, Kuthonuzo wrote:
>>
>> This patch with atomic bit op is similar in spirit to v1 except that it 
>> increases metadata size.
>>
>
> I don't think that this is a big deal. That will slightly increase size of 
> objects <= (128 - 32) bytes.
> And if someone think otherwise, we can completely remove 'alloc_size'
> (we use it only to print size in report - not very useful).


Where did 128 come from?
We now should allocate only 32 bytes for 16-byte user object. If not,
there is something to fix.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/09/2016 02:35 PM, Luruo, Kuthonuzo wrote:
> 
> This patch with atomic bit op is similar in spirit to v1 except that it 
> increases metadata size.
> 

I don't think that this is a big deal. That will slightly increase size of 
objects <= (128 - 32) bytes.
And if someone think otherwise, we can completely remove 'alloc_size'
(we use it only to print size in report - not very useful).

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/09/2016 02:35 PM, Luruo, Kuthonuzo wrote:
> 
> This patch with atomic bit op is similar in spirit to v1 except that it 
> increases metadata size.
> 

I don't think that this is a big deal. That will slightly increase size of 
objects <= (128 - 32) bytes.
And if someone think otherwise, we can completely remove 'alloc_size'
(we use it only to print size in report - not very useful).

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/09/2016 01:31 PM, Dmitry Vyukov wrote:
> On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
>  wrote:
>>
>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>> index b3c122d..c2b0e51 100644
>> --- a/mm/kasan/report.c
>> +++ b/mm/kasan/report.c
>> @@ -140,18 +140,12 @@ static void object_err(struct kmem_cache *cache, 
>> struct page *page,
>> pr_err("Object at %p, in cache %s\n", object, cache->name);
>> if (!(cache->flags & SLAB_KASAN))
>> return;
>> -   switch (alloc_info->state) {
>> -   case KASAN_STATE_INIT:
>> -   pr_err("Object not allocated yet\n");
>> -   break;
>> -   case KASAN_STATE_ALLOC:
>> +   if (test_bit(KASAN_STATE_ALLOCATED, _info->state)) {
>> pr_err("Object allocated with size %u bytes.\n",
>>alloc_info->alloc_size);
>> pr_err("Allocation:\n");
>> print_track(_info->track);
> 
> alloc_info->track is not necessary initialized when
> KASAN_STATE_ALLOCATED is set.

It should be initialized to something. If it's not initialized, than that 
object wasn't allocated.
So this would be a *very* random pointer access. Also this would mean that 
->state itself might be not initialized too.
Anyway, we can't do much in such scenario since we can't trust any data.
And I don't think that we should because very likely this will cause panic 
eventually.

> Worse, it can be initialized to a wrong
> stack.
> 

Define "wrong stack" here.

I assume that you are talking about race in the following scenario:

--
Proccess A:  Proccess B:

p_A = kmalloc();
/* use p_A */
kfree(p_A);
 p_A = kmalloc();
  
  set_bit(KASAN_STATE_ALLOCATED); //bit 
set, but stack is not saved yet.
/* use after free p_A */

if (test_bit(KASAN_STATE_ALLOCATED))
print_stack() // will print track from Proccess A
-

So, would be the stack trace from A wrong in such situation? I don't think so.
We could change ->state and save stack trace in the 'right' order with proper 
barriers,
but would it prevent us from showing wrong stacktrace? - It wouldn't.

Now, the only real problem with current code, is that we don't print free stack 
if we think that object is in
allocated state. We should do this, because more information is always better, 
e.g. we might hit long-delayed use-after-free,
in which case free stack would be useful (just like in scenario above).

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/09/2016 01:31 PM, Dmitry Vyukov wrote:
> On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
>  wrote:
>>
>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>> index b3c122d..c2b0e51 100644
>> --- a/mm/kasan/report.c
>> +++ b/mm/kasan/report.c
>> @@ -140,18 +140,12 @@ static void object_err(struct kmem_cache *cache, 
>> struct page *page,
>> pr_err("Object at %p, in cache %s\n", object, cache->name);
>> if (!(cache->flags & SLAB_KASAN))
>> return;
>> -   switch (alloc_info->state) {
>> -   case KASAN_STATE_INIT:
>> -   pr_err("Object not allocated yet\n");
>> -   break;
>> -   case KASAN_STATE_ALLOC:
>> +   if (test_bit(KASAN_STATE_ALLOCATED, _info->state)) {
>> pr_err("Object allocated with size %u bytes.\n",
>>alloc_info->alloc_size);
>> pr_err("Allocation:\n");
>> print_track(_info->track);
> 
> alloc_info->track is not necessary initialized when
> KASAN_STATE_ALLOCATED is set.

It should be initialized to something. If it's not initialized, than that 
object wasn't allocated.
So this would be a *very* random pointer access. Also this would mean that 
->state itself might be not initialized too.
Anyway, we can't do much in such scenario since we can't trust any data.
And I don't think that we should because very likely this will cause panic 
eventually.

> Worse, it can be initialized to a wrong
> stack.
> 

Define "wrong stack" here.

I assume that you are talking about race in the following scenario:

--
Proccess A:  Proccess B:

p_A = kmalloc();
/* use p_A */
kfree(p_A);
 p_A = kmalloc();
  
  set_bit(KASAN_STATE_ALLOCATED); //bit 
set, but stack is not saved yet.
/* use after free p_A */

if (test_bit(KASAN_STATE_ALLOCATED))
print_stack() // will print track from Proccess A
-

So, would be the stack trace from A wrong in such situation? I don't think so.
We could change ->state and save stack trace in the 'right' order with proper 
barriers,
but would it prevent us from showing wrong stacktrace? - It wouldn't.

Now, the only real problem with current code, is that we don't print free stack 
if we think that object is in
allocated state. We should do this, because more information is always better, 
e.g. we might hit long-delayed use-after-free,
in which case free stack would be useful (just like in scenario above).

RE: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Luruo, Kuthonuzo]

> > Currently, KASAN may fail to detect concurrent deallocations of the same
> > object due to a race in kasan_slab_free(). This patch makes double-free
> > detection more reliable by serializing access to KASAN object metadata.
> > New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
> > lock/unlock per-object metadata. Double-free errors are now reported via
> > kasan_report().
> >
> > Testing:
> > - Tested with a modified version of the 'slab_test' microbenchmark where
> >   allocs occur on CPU 0; then all other CPUs concurrently attempt to free
> >   the same object.
> > - Tested with new 'test_kasan' kasan_double_free() test in accompanying
> >   patch.
> >
> > Signed-off-by: Kuthonuzo Luruo 
> > ---
> >
> > Changes in v2:
> > - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
> >   lock/unlock functions; kasan_alloc_meta modified to add new state while
> >   using fewer bits overall.
> > - Double-free pr_err promoted to kasan_report().
> > - kasan_init_object() introduced to initialize KASAN object metadata
> >   during slab creation. KASAN_STATE_INIT initialization removed from
> >   kasan_poison_object_data().
> >
> > ---
> >  include/linux/kasan.h |8 +++
> >  mm/kasan/kasan.c  |  118 --
> ---
> >  mm/kasan/kasan.h  |   15 +-
> >  mm/kasan/quarantine.c |7 +++-
> >  mm/kasan/report.c |   31 +++--
> >  mm/slab.c |1 +
> >  6 files changed, 142 insertions(+), 38 deletions(-)
> >
> 
> Sorry, but this patch is crap.
> 
> Something like this, will fix the race:
> 
> ---
>  mm/kasan/kasan.c  | 20 
>  mm/kasan/kasan.h  | 10 +++---
>  mm/kasan/quarantine.c |  1 -
>  mm/kasan/report.c | 11 ++-
>  4 files changed, 9 insertions(+), 33 deletions(-)
> 
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..8d078dc 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -419,13 +419,6 @@ 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;
> - }
> -#endif
>  }
> 
>  #ifdef CONFIG_SLAB
> @@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache,
> void *object)
>   struct kasan_free_meta *free_info =
>   get_free_info(cache, object);
> 
> - switch (alloc_info->state) {
> - case KASAN_STATE_ALLOC:
> - alloc_info->state = KASAN_STATE_QUARANTINE;
> + if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
> + _info->state)) {
>   quarantine_put(free_info, cache);
>   set_track(_info->track, GFP_NOWAIT);
>   kasan_poison_slab_free(cache, object);
>   return true;
> - case KASAN_STATE_QUARANTINE:
> - case KASAN_STATE_FREE:
> + } else {
>   pr_err("Double free");
>   dump_stack();
> - break;
> - default:
> - break;
>   }
>   }
>   return false;
> @@ -571,7 +559,7 @@ void kasan_kmalloc(struct kmem_cache *cache, const
> void *object, size_t size,
>   struct kasan_alloc_meta *alloc_info =
>   get_alloc_info(cache, object);
> 
> - alloc_info->state = KASAN_STATE_ALLOC;
> + set_bit(KASAN_STATE_ALLOCATED, _info->state);
>   alloc_info->alloc_size = size;
>   set_track(_info->track, flags);
>   }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..2dcdc8f 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -60,10 +60,7 @@ struct kasan_global {
>   */
> 
>  enum kasan_state {
> - KASAN_STATE_INIT,
> - KASAN_STATE_ALLOC,
> - KASAN_STATE_QUARANTINE,
> - KASAN_STATE_FREE
> + KASAN_STATE_ALLOCATED,
>  };
> 
>  #define KASAN_STACK_DEPTH 64
> @@ -75,9 +72,8 @@ struct kasan_track {
> 
>  struct kasan_alloc_meta {
>   struct kasan_track track;
> - u32 state : 2;  /* enum kasan_state */
> - u32 alloc_size : 30;
> - u32 reserved;
> + unsigned long state;
> + u32 alloc_size;
>  };
> 
>  struct kasan_free_meta {
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 40159a6..ca33fd3 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -147,7 +147,6 @@ static void qlink_free(void **qlink, struct kmem_cache
> *cache)
>   unsigned long flags;
> 
>   local_irq_save(flags);
> - 

RE: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Luruo, Kuthonuzo]

> > Currently, KASAN may fail to detect concurrent deallocations of the same
> > object due to a race in kasan_slab_free(). This patch makes double-free
> > detection more reliable by serializing access to KASAN object metadata.
> > New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
> > lock/unlock per-object metadata. Double-free errors are now reported via
> > kasan_report().
> >
> > Testing:
> > - Tested with a modified version of the 'slab_test' microbenchmark where
> >   allocs occur on CPU 0; then all other CPUs concurrently attempt to free
> >   the same object.
> > - Tested with new 'test_kasan' kasan_double_free() test in accompanying
> >   patch.
> >
> > Signed-off-by: Kuthonuzo Luruo 
> > ---
> >
> > Changes in v2:
> > - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
> >   lock/unlock functions; kasan_alloc_meta modified to add new state while
> >   using fewer bits overall.
> > - Double-free pr_err promoted to kasan_report().
> > - kasan_init_object() introduced to initialize KASAN object metadata
> >   during slab creation. KASAN_STATE_INIT initialization removed from
> >   kasan_poison_object_data().
> >
> > ---
> >  include/linux/kasan.h |8 +++
> >  mm/kasan/kasan.c  |  118 --
> ---
> >  mm/kasan/kasan.h  |   15 +-
> >  mm/kasan/quarantine.c |7 +++-
> >  mm/kasan/report.c |   31 +++--
> >  mm/slab.c |1 +
> >  6 files changed, 142 insertions(+), 38 deletions(-)
> >
> 
> Sorry, but this patch is crap.
> 
> Something like this, will fix the race:
> 
> ---
>  mm/kasan/kasan.c  | 20 
>  mm/kasan/kasan.h  | 10 +++---
>  mm/kasan/quarantine.c |  1 -
>  mm/kasan/report.c | 11 ++-
>  4 files changed, 9 insertions(+), 33 deletions(-)
> 
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..8d078dc 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -419,13 +419,6 @@ 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;
> - }
> -#endif
>  }
> 
>  #ifdef CONFIG_SLAB
> @@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache,
> void *object)
>   struct kasan_free_meta *free_info =
>   get_free_info(cache, object);
> 
> - switch (alloc_info->state) {
> - case KASAN_STATE_ALLOC:
> - alloc_info->state = KASAN_STATE_QUARANTINE;
> + if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
> + _info->state)) {
>   quarantine_put(free_info, cache);
>   set_track(_info->track, GFP_NOWAIT);
>   kasan_poison_slab_free(cache, object);
>   return true;
> - case KASAN_STATE_QUARANTINE:
> - case KASAN_STATE_FREE:
> + } else {
>   pr_err("Double free");
>   dump_stack();
> - break;
> - default:
> - break;
>   }
>   }
>   return false;
> @@ -571,7 +559,7 @@ void kasan_kmalloc(struct kmem_cache *cache, const
> void *object, size_t size,
>   struct kasan_alloc_meta *alloc_info =
>   get_alloc_info(cache, object);
> 
> - alloc_info->state = KASAN_STATE_ALLOC;
> + set_bit(KASAN_STATE_ALLOCATED, _info->state);
>   alloc_info->alloc_size = size;
>   set_track(_info->track, flags);
>   }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..2dcdc8f 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -60,10 +60,7 @@ struct kasan_global {
>   */
> 
>  enum kasan_state {
> - KASAN_STATE_INIT,
> - KASAN_STATE_ALLOC,
> - KASAN_STATE_QUARANTINE,
> - KASAN_STATE_FREE
> + KASAN_STATE_ALLOCATED,
>  };
> 
>  #define KASAN_STACK_DEPTH 64
> @@ -75,9 +72,8 @@ struct kasan_track {
> 
>  struct kasan_alloc_meta {
>   struct kasan_track track;
> - u32 state : 2;  /* enum kasan_state */
> - u32 alloc_size : 30;
> - u32 reserved;
> + unsigned long state;
> + u32 alloc_size;
>  };
> 
>  struct kasan_free_meta {
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 40159a6..ca33fd3 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -147,7 +147,6 @@ static void qlink_free(void **qlink, struct kmem_cache
> *cache)
>   unsigned long flags;
> 
>   local_irq_save(flags);
> - alloc_info->state = 

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
 wrote:
>
>
> On 05/06/2016 02:47 PM, Kuthonuzo Luruo wrote:
>> Currently, KASAN may fail to detect concurrent deallocations of the same
>> object due to a race in kasan_slab_free(). This patch makes double-free
>> detection more reliable by serializing access to KASAN object metadata.
>> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
>> lock/unlock per-object metadata. Double-free errors are now reported via
>> kasan_report().
>>
>> Testing:
>> - Tested with a modified version of the 'slab_test' microbenchmark where
>>   allocs occur on CPU 0; then all other CPUs concurrently attempt to free
>>   the same object.
>> - Tested with new 'test_kasan' kasan_double_free() test in accompanying
>>   patch.
>>
>> Signed-off-by: Kuthonuzo Luruo 
>> ---
>>
>> Changes in v2:
>> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>>   using fewer bits overall.
>> - Double-free pr_err promoted to kasan_report().
>> - kasan_init_object() introduced to initialize KASAN object metadata
>>   during slab creation. KASAN_STATE_INIT initialization removed from
>>   kasan_poison_object_data().
>>
>> ---
>>  include/linux/kasan.h |8 +++
>>  mm/kasan/kasan.c  |  118 
>> -
>>  mm/kasan/kasan.h  |   15 +-
>>  mm/kasan/quarantine.c |7 +++-
>>  mm/kasan/report.c |   31 +++--
>>  mm/slab.c |1 +
>>  6 files changed, 142 insertions(+), 38 deletions(-)
>>
>
> Sorry, but this patch is crap.
>
> Something like this, will fix the race:
>
> ---
>  mm/kasan/kasan.c  | 20 
>  mm/kasan/kasan.h  | 10 +++---
>  mm/kasan/quarantine.c |  1 -
>  mm/kasan/report.c | 11 ++-
>  4 files changed, 9 insertions(+), 33 deletions(-)
>
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..8d078dc 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -419,13 +419,6 @@ 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;
> -   }
> -#endif
>  }
>
>  #ifdef CONFIG_SLAB
> @@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache, void 
> *object)
> struct kasan_free_meta *free_info =
> get_free_info(cache, object);
>
> -   switch (alloc_info->state) {
> -   case KASAN_STATE_ALLOC:
> -   alloc_info->state = KASAN_STATE_QUARANTINE;
> +   if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
> +   _info->state)) {
> quarantine_put(free_info, cache);
> set_track(_info->track, GFP_NOWAIT);
> kasan_poison_slab_free(cache, object);
> return true;
> -   case KASAN_STATE_QUARANTINE:
> -   case KASAN_STATE_FREE:
> +   } else {
> pr_err("Double free");
> dump_stack();
> -   break;
> -   default:
> -   break;
> }
> }
> return false;
> @@ -571,7 +559,7 @@ void kasan_kmalloc(struct kmem_cache *cache, const void 
> *object, size_t size,
> struct kasan_alloc_meta *alloc_info =
> get_alloc_info(cache, object);
>
> -   alloc_info->state = KASAN_STATE_ALLOC;
> +   set_bit(KASAN_STATE_ALLOCATED, _info->state);
> alloc_info->alloc_size = size;
> set_track(_info->track, flags);
> }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..2dcdc8f 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -60,10 +60,7 @@ struct kasan_global {
>   */
>
>  enum kasan_state {
> -   KASAN_STATE_INIT,
> -   KASAN_STATE_ALLOC,
> -   KASAN_STATE_QUARANTINE,
> -   KASAN_STATE_FREE
> +   KASAN_STATE_ALLOCATED,
>  };
>
>  #define KASAN_STACK_DEPTH 64
> @@ -75,9 +72,8 @@ struct kasan_track {
>
>  struct kasan_alloc_meta {
> struct kasan_track track;
> -   u32 state : 2;  /* enum kasan_state */
> -   u32 alloc_size : 30;
> -   u32 reserved;
> +   unsigned long state;
> +   u32 alloc_size;
>  };
>
>  struct kasan_free_meta {
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 40159a6..ca33fd3 100644
> --- a/mm/kasan/quarantine.c
> +++ 

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
 wrote:
>
>
> On 05/06/2016 02:47 PM, Kuthonuzo Luruo wrote:
>> Currently, KASAN may fail to detect concurrent deallocations of the same
>> object due to a race in kasan_slab_free(). This patch makes double-free
>> detection more reliable by serializing access to KASAN object metadata.
>> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
>> lock/unlock per-object metadata. Double-free errors are now reported via
>> kasan_report().
>>
>> Testing:
>> - Tested with a modified version of the 'slab_test' microbenchmark where
>>   allocs occur on CPU 0; then all other CPUs concurrently attempt to free
>>   the same object.
>> - Tested with new 'test_kasan' kasan_double_free() test in accompanying
>>   patch.
>>
>> Signed-off-by: Kuthonuzo Luruo 
>> ---
>>
>> Changes in v2:
>> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>>   using fewer bits overall.
>> - Double-free pr_err promoted to kasan_report().
>> - kasan_init_object() introduced to initialize KASAN object metadata
>>   during slab creation. KASAN_STATE_INIT initialization removed from
>>   kasan_poison_object_data().
>>
>> ---
>>  include/linux/kasan.h |8 +++
>>  mm/kasan/kasan.c  |  118 
>> -
>>  mm/kasan/kasan.h  |   15 +-
>>  mm/kasan/quarantine.c |7 +++-
>>  mm/kasan/report.c |   31 +++--
>>  mm/slab.c |1 +
>>  6 files changed, 142 insertions(+), 38 deletions(-)
>>
>
> Sorry, but this patch is crap.
>
> Something like this, will fix the race:
>
> ---
>  mm/kasan/kasan.c  | 20 
>  mm/kasan/kasan.h  | 10 +++---
>  mm/kasan/quarantine.c |  1 -
>  mm/kasan/report.c | 11 ++-
>  4 files changed, 9 insertions(+), 33 deletions(-)
>
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..8d078dc 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -419,13 +419,6 @@ 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;
> -   }
> -#endif
>  }
>
>  #ifdef CONFIG_SLAB
> @@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache, void 
> *object)
> struct kasan_free_meta *free_info =
> get_free_info(cache, object);
>
> -   switch (alloc_info->state) {
> -   case KASAN_STATE_ALLOC:
> -   alloc_info->state = KASAN_STATE_QUARANTINE;
> +   if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
> +   _info->state)) {
> quarantine_put(free_info, cache);
> set_track(_info->track, GFP_NOWAIT);
> kasan_poison_slab_free(cache, object);
> return true;
> -   case KASAN_STATE_QUARANTINE:
> -   case KASAN_STATE_FREE:
> +   } else {
> pr_err("Double free");
> dump_stack();
> -   break;
> -   default:
> -   break;
> }
> }
> return false;
> @@ -571,7 +559,7 @@ void kasan_kmalloc(struct kmem_cache *cache, const void 
> *object, size_t size,
> struct kasan_alloc_meta *alloc_info =
> get_alloc_info(cache, object);
>
> -   alloc_info->state = KASAN_STATE_ALLOC;
> +   set_bit(KASAN_STATE_ALLOCATED, _info->state);
> alloc_info->alloc_size = size;
> set_track(_info->track, flags);
> }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..2dcdc8f 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -60,10 +60,7 @@ struct kasan_global {
>   */
>
>  enum kasan_state {
> -   KASAN_STATE_INIT,
> -   KASAN_STATE_ALLOC,
> -   KASAN_STATE_QUARANTINE,
> -   KASAN_STATE_FREE
> +   KASAN_STATE_ALLOCATED,
>  };
>
>  #define KASAN_STACK_DEPTH 64
> @@ -75,9 +72,8 @@ struct kasan_track {
>
>  struct kasan_alloc_meta {
> struct kasan_track track;
> -   u32 state : 2;  /* enum kasan_state */
> -   u32 alloc_size : 30;
> -   u32 reserved;
> +   unsigned long state;
> +   u32 alloc_size;
>  };
>
>  struct kasan_free_meta {
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 40159a6..ca33fd3 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -147,7 +147,6 @@ 

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/06/2016 02:47 PM, Kuthonuzo Luruo wrote:
> Currently, KASAN may fail to detect concurrent deallocations of the same
> object due to a race in kasan_slab_free(). This patch makes double-free
> detection more reliable by serializing access to KASAN object metadata.
> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
> lock/unlock per-object metadata. Double-free errors are now reported via
> kasan_report().
> 
> Testing:
> - Tested with a modified version of the 'slab_test' microbenchmark where
>   allocs occur on CPU 0; then all other CPUs concurrently attempt to free
>   the same object.
> - Tested with new 'test_kasan' kasan_double_free() test in accompanying
>   patch.
> 
> Signed-off-by: Kuthonuzo Luruo 
> ---
> 
> Changes in v2:
> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>   using fewer bits overall.
> - Double-free pr_err promoted to kasan_report().
> - kasan_init_object() introduced to initialize KASAN object metadata
>   during slab creation. KASAN_STATE_INIT initialization removed from
>   kasan_poison_object_data().
>  
> ---
>  include/linux/kasan.h |8 +++
>  mm/kasan/kasan.c  |  118 
> -
>  mm/kasan/kasan.h  |   15 +-
>  mm/kasan/quarantine.c |7 +++-
>  mm/kasan/report.c |   31 +++--
>  mm/slab.c |1 +
>  6 files changed, 142 insertions(+), 38 deletions(-)
> 

Sorry, but this patch is crap.

Something like this, will fix the race:

---
 mm/kasan/kasan.c  | 20 
 mm/kasan/kasan.h  | 10 +++---
 mm/kasan/quarantine.c |  1 -
 mm/kasan/report.c | 11 ++-
 4 files changed, 9 insertions(+), 33 deletions(-)

diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index ef2e87b..8d078dc 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -419,13 +419,6 @@ 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;
-   }
-#endif
 }
 
 #ifdef CONFIG_SLAB
@@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache, void 
*object)
struct kasan_free_meta *free_info =
get_free_info(cache, object);
 
-   switch (alloc_info->state) {
-   case KASAN_STATE_ALLOC:
-   alloc_info->state = KASAN_STATE_QUARANTINE;
+   if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
+   _info->state)) {
quarantine_put(free_info, cache);
set_track(_info->track, GFP_NOWAIT);
kasan_poison_slab_free(cache, object);
return true;
-   case KASAN_STATE_QUARANTINE:
-   case KASAN_STATE_FREE:
+   } else {
pr_err("Double free");
dump_stack();
-   break;
-   default:
-   break;
}
}
return false;
@@ -571,7 +559,7 @@ void kasan_kmalloc(struct kmem_cache *cache, const void 
*object, size_t size,
struct kasan_alloc_meta *alloc_info =
get_alloc_info(cache, object);
 
-   alloc_info->state = KASAN_STATE_ALLOC;
+   set_bit(KASAN_STATE_ALLOCATED, _info->state);
alloc_info->alloc_size = size;
set_track(_info->track, flags);
}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 7da78a6..2dcdc8f 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -60,10 +60,7 @@ struct kasan_global {
  */
 
 enum kasan_state {
-   KASAN_STATE_INIT,
-   KASAN_STATE_ALLOC,
-   KASAN_STATE_QUARANTINE,
-   KASAN_STATE_FREE
+   KASAN_STATE_ALLOCATED,
 };
 
 #define KASAN_STACK_DEPTH 64
@@ -75,9 +72,8 @@ struct kasan_track {
 
 struct kasan_alloc_meta {
struct kasan_track track;
-   u32 state : 2;  /* enum kasan_state */
-   u32 alloc_size : 30;
-   u32 reserved;
+   unsigned long state;
+   u32 alloc_size;
 };
 
 struct kasan_free_meta {
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 40159a6..ca33fd3 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -147,7 +147,6 @@ static void qlink_free(void **qlink, struct kmem_cache 
*cache)
unsigned long flags;
 
local_irq_save(flags);
-   alloc_info->state = KASAN_STATE_FREE;
___cache_free(cache, object, _THIS_IP_);

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/06/2016 02:47 PM, Kuthonuzo Luruo wrote:
> Currently, KASAN may fail to detect concurrent deallocations of the same
> object due to a race in kasan_slab_free(). This patch makes double-free
> detection more reliable by serializing access to KASAN object metadata.
> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
> lock/unlock per-object metadata. Double-free errors are now reported via
> kasan_report().
> 
> Testing:
> - Tested with a modified version of the 'slab_test' microbenchmark where
>   allocs occur on CPU 0; then all other CPUs concurrently attempt to free
>   the same object.
> - Tested with new 'test_kasan' kasan_double_free() test in accompanying
>   patch.
> 
> Signed-off-by: Kuthonuzo Luruo 
> ---
> 
> Changes in v2:
> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>   using fewer bits overall.
> - Double-free pr_err promoted to kasan_report().
> - kasan_init_object() introduced to initialize KASAN object metadata
>   during slab creation. KASAN_STATE_INIT initialization removed from
>   kasan_poison_object_data().
>  
> ---
>  include/linux/kasan.h |8 +++
>  mm/kasan/kasan.c  |  118 
> -
>  mm/kasan/kasan.h  |   15 +-
>  mm/kasan/quarantine.c |7 +++-
>  mm/kasan/report.c |   31 +++--
>  mm/slab.c |1 +
>  6 files changed, 142 insertions(+), 38 deletions(-)
> 

Sorry, but this patch is crap.

Something like this, will fix the race:

---
 mm/kasan/kasan.c  | 20 
 mm/kasan/kasan.h  | 10 +++---
 mm/kasan/quarantine.c |  1 -
 mm/kasan/report.c | 11 ++-
 4 files changed, 9 insertions(+), 33 deletions(-)

diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index ef2e87b..8d078dc 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -419,13 +419,6 @@ 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;
-   }
-#endif
 }
 
 #ifdef CONFIG_SLAB
@@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache, void 
*object)
struct kasan_free_meta *free_info =
get_free_info(cache, object);
 
-   switch (alloc_info->state) {
-   case KASAN_STATE_ALLOC:
-   alloc_info->state = KASAN_STATE_QUARANTINE;
+   if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
+   _info->state)) {
quarantine_put(free_info, cache);
set_track(_info->track, GFP_NOWAIT);
kasan_poison_slab_free(cache, object);
return true;
-   case KASAN_STATE_QUARANTINE:
-   case KASAN_STATE_FREE:
+   } else {
pr_err("Double free");
dump_stack();
-   break;
-   default:
-   break;
}
}
return false;
@@ -571,7 +559,7 @@ void kasan_kmalloc(struct kmem_cache *cache, const void 
*object, size_t size,
struct kasan_alloc_meta *alloc_info =
get_alloc_info(cache, object);
 
-   alloc_info->state = KASAN_STATE_ALLOC;
+   set_bit(KASAN_STATE_ALLOCATED, _info->state);
alloc_info->alloc_size = size;
set_track(_info->track, flags);
}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 7da78a6..2dcdc8f 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -60,10 +60,7 @@ struct kasan_global {
  */
 
 enum kasan_state {
-   KASAN_STATE_INIT,
-   KASAN_STATE_ALLOC,
-   KASAN_STATE_QUARANTINE,
-   KASAN_STATE_FREE
+   KASAN_STATE_ALLOCATED,
 };
 
 #define KASAN_STACK_DEPTH 64
@@ -75,9 +72,8 @@ struct kasan_track {
 
 struct kasan_alloc_meta {
struct kasan_track track;
-   u32 state : 2;  /* enum kasan_state */
-   u32 alloc_size : 30;
-   u32 reserved;
+   unsigned long state;
+   u32 alloc_size;
 };
 
 struct kasan_free_meta {
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 40159a6..ca33fd3 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -147,7 +147,6 @@ static void qlink_free(void **qlink, struct kmem_cache 
*cache)
unsigned long flags;
 
local_irq_save(flags);
-   alloc_info->state = KASAN_STATE_FREE;
___cache_free(cache, object, _THIS_IP_);
local_irq_restore(flags);
 }
diff --git 

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Sat, May 7, 2016 at 5:15 PM, Luruo, Kuthonuzo
 wrote:
> Thank you for the review!
>
>> > +
>> > +/* acquire per-object lock for access to KASAN metadata. */
>>
>> I believe there's strong reason not to use standard spin_lock() or
>> similar. I think it's proper place to explain it.
>>
>
> will do.
>
>> > +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
>> > +{
>> > +   union kasan_alloc_data old, new;
>> > +
>> > +   preempt_disable();
>>
>> It's better to disable and enable preemption inside the loop
>> on each iteration, to decrease contention.
>>
>
> ok, makes sense; will do.
>
>> > +   for (;;) {
>> > +   old.packed = READ_ONCE(alloc_info->data);
>> > +   if (unlikely(old.lock)) {
>> > +   cpu_relax();
>> > +   continue;
>> > +   }
>> > +   new.packed = old.packed;
>> > +   new.lock = 1;
>> > +   if (likely(cmpxchg(_info->data, old.packed, new.packed)
>> > +   == old.packed))
>> > +   break;
>> > +   }
>> > +}
>> > +
>> > +/* release lock after a kasan_meta_lock(). */
>> > +void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
>> > +{
>> > +   union kasan_alloc_data alloc_data;
>> > +
>> > +   alloc_data.packed = READ_ONCE(alloc_info->data);
>> > +   alloc_data.lock = 0;
>> > +   if (unlikely(xchg(_info->data, alloc_data.packed) !=
>> > +   (alloc_data.packed | 0x1U)))
>> > +   WARN_ONCE(1, "%s: lock not held!\n", __func__);
>>
>> Nitpick. It never happens in normal case, correct?. Why don't you place it 
>> under
>> some developer config, or even leave at dev branch? The function will
>> be twice shorter without it.
>
> ok, will remove/shorten

My concern here is performance.
We do lock/unlock 3 times per allocated object. Currently that's 6
atomic RMW. The unlock one is not necessary, so that would reduce
number of atomic RMWs to 3.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Sat, May 7, 2016 at 5:15 PM, Luruo, Kuthonuzo
 wrote:
> Thank you for the review!
>
>> > +
>> > +/* acquire per-object lock for access to KASAN metadata. */
>>
>> I believe there's strong reason not to use standard spin_lock() or
>> similar. I think it's proper place to explain it.
>>
>
> will do.
>
>> > +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
>> > +{
>> > +   union kasan_alloc_data old, new;
>> > +
>> > +   preempt_disable();
>>
>> It's better to disable and enable preemption inside the loop
>> on each iteration, to decrease contention.
>>
>
> ok, makes sense; will do.
>
>> > +   for (;;) {
>> > +   old.packed = READ_ONCE(alloc_info->data);
>> > +   if (unlikely(old.lock)) {
>> > +   cpu_relax();
>> > +   continue;
>> > +   }
>> > +   new.packed = old.packed;
>> > +   new.lock = 1;
>> > +   if (likely(cmpxchg(_info->data, old.packed, new.packed)
>> > +   == old.packed))
>> > +   break;
>> > +   }
>> > +}
>> > +
>> > +/* release lock after a kasan_meta_lock(). */
>> > +void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
>> > +{
>> > +   union kasan_alloc_data alloc_data;
>> > +
>> > +   alloc_data.packed = READ_ONCE(alloc_info->data);
>> > +   alloc_data.lock = 0;
>> > +   if (unlikely(xchg(_info->data, alloc_data.packed) !=
>> > +   (alloc_data.packed | 0x1U)))
>> > +   WARN_ONCE(1, "%s: lock not held!\n", __func__);
>>
>> Nitpick. It never happens in normal case, correct?. Why don't you place it 
>> under
>> some developer config, or even leave at dev branch? The function will
>> be twice shorter without it.
>
> ok, will remove/shorten

My concern here is performance.
We do lock/unlock 3 times per allocated object. Currently that's 6
atomic RMW. The unlock one is not necessary, so that would reduce
number of atomic RMWs to 3.

RE: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Luruo, Kuthonuzo]

> >> Thank you for the review!
> >>
> >> > > + switch (alloc_data.state) {
> >> > > + case KASAN_STATE_QUARANTINE:
> >> > > + case KASAN_STATE_FREE:
> >> > > + kasan_report((unsigned long)object, 0, false,
> >> > > + (unsigned long)__builtin_return_address(1));
> >> >
> >> > __builtin_return_address() is unsafe if argument is non-zero. Use
> >> > return_address() instead.
> >>
> >> hmm, I/cscope can't seem to find an x86 implementation for
> return_address().
> >> Will dig further; thanks.
> >>
> >
> > It seems there's no generic interface to obtain return address. x86
> > has  working __builtin_return_address() and it's ok with it, others
> > use their own return_adderss(), and ok as well.
> >
> > I think unification is needed here.
> 
> 
> We use _RET_IP_ in other places in portable part of kasan.

Yeah, _RET_IP_ is the way to go here.

Not directly related but: while looking into kasan_slab_free() callers, it seems
to me that, with SLAB + quarantine, kasan_poison_kfree() should _not_ be
calling into kasan_slab_free(). The intent in the call-chain thru
kasan_poison_kree() seems to be only to poison object shadow, not actually
free it.

Alexander, can you please comment/confirm? Thanks.

Kuthonuzo

RE: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Luruo, Kuthonuzo]

> >> Thank you for the review!
> >>
> >> > > + switch (alloc_data.state) {
> >> > > + case KASAN_STATE_QUARANTINE:
> >> > > + case KASAN_STATE_FREE:
> >> > > + kasan_report((unsigned long)object, 0, false,
> >> > > + (unsigned long)__builtin_return_address(1));
> >> >
> >> > __builtin_return_address() is unsafe if argument is non-zero. Use
> >> > return_address() instead.
> >>
> >> hmm, I/cscope can't seem to find an x86 implementation for
> return_address().
> >> Will dig further; thanks.
> >>
> >
> > It seems there's no generic interface to obtain return address. x86
> > has  working __builtin_return_address() and it's ok with it, others
> > use their own return_adderss(), and ok as well.
> >
> > I think unification is needed here.
> 
> 
> We use _RET_IP_ in other places in portable part of kasan.

Yeah, _RET_IP_ is the way to go here.

Not directly related but: while looking into kasan_slab_free() callers, it seems
to me that, with SLAB + quarantine, kasan_poison_kfree() should _not_ be
calling into kasan_slab_free(). The intent in the call-chain thru
kasan_poison_kree() seems to be only to poison object shadow, not actually
free it.

Alexander, can you please comment/confirm? Thanks.

Kuthonuzo

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Sun, May 8, 2016 at 11:17 AM, Yury Norov  wrote:
> On Sat, May 07, 2016 at 03:15:59PM +, Luruo, Kuthonuzo wrote:
>> Thank you for the review!
>>
>> > > + switch (alloc_data.state) {
>> > > + case KASAN_STATE_QUARANTINE:
>> > > + case KASAN_STATE_FREE:
>> > > + kasan_report((unsigned long)object, 0, false,
>> > > + (unsigned long)__builtin_return_address(1));
>> >
>> > __builtin_return_address() is unsafe if argument is non-zero. Use
>> > return_address() instead.
>>
>> hmm, I/cscope can't seem to find an x86 implementation for return_address().
>> Will dig further; thanks.
>>
>
> It seems there's no generic interface to obtain return address. x86
> has  working __builtin_return_address() and it's ok with it, others
> use their own return_adderss(), and ok as well.
>
> I think unification is needed here.


We use _RET_IP_ in other places in portable part of kasan.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Sun, May 8, 2016 at 11:17 AM, Yury Norov  wrote:
> On Sat, May 07, 2016 at 03:15:59PM +, Luruo, Kuthonuzo wrote:
>> Thank you for the review!
>>
>> > > + switch (alloc_data.state) {
>> > > + case KASAN_STATE_QUARANTINE:
>> > > + case KASAN_STATE_FREE:
>> > > + kasan_report((unsigned long)object, 0, false,
>> > > + (unsigned long)__builtin_return_address(1));
>> >
>> > __builtin_return_address() is unsafe if argument is non-zero. Use
>> > return_address() instead.
>>
>> hmm, I/cscope can't seem to find an x86 implementation for return_address().
>> Will dig further; thanks.
>>
>
> It seems there's no generic interface to obtain return address. x86
> has  working __builtin_return_address() and it's ok with it, others
> use their own return_adderss(), and ok as well.
>
> I think unification is needed here.


We use _RET_IP_ in other places in portable part of kasan.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Yury Norov]

On Sat, May 07, 2016 at 03:15:59PM +, Luruo, Kuthonuzo wrote:
> Thank you for the review!
> 
> > > + switch (alloc_data.state) {
> > > + case KASAN_STATE_QUARANTINE:
> > > + case KASAN_STATE_FREE:
> > > + kasan_report((unsigned long)object, 0, false,
> > > + (unsigned long)__builtin_return_address(1));
> > 
> > __builtin_return_address() is unsafe if argument is non-zero. Use
> > return_address() instead.
> 
> hmm, I/cscope can't seem to find an x86 implementation for return_address().
> Will dig further; thanks.
> 

It seems there's no generic interface to obtain return address. x86
has  working __builtin_return_address() and it's ok with it, others
use their own return_adderss(), and ok as well.

I think unification is needed here.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Yury Norov]

On Sat, May 07, 2016 at 03:15:59PM +, Luruo, Kuthonuzo wrote:
> Thank you for the review!
> 
> > > + switch (alloc_data.state) {
> > > + case KASAN_STATE_QUARANTINE:
> > > + case KASAN_STATE_FREE:
> > > + kasan_report((unsigned long)object, 0, false,
> > > + (unsigned long)__builtin_return_address(1));
> > 
> > __builtin_return_address() is unsafe if argument is non-zero. Use
> > return_address() instead.
> 
> hmm, I/cscope can't seem to find an x86 implementation for return_address().
> Will dig further; thanks.
> 

It seems there's no generic interface to obtain return address. x86
has  working __builtin_return_address() and it's ok with it, others
use their own return_adderss(), and ok as well.

I think unification is needed here.

RE: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Luruo, Kuthonuzo]

Thank you for the review!

> > +
> > +/* acquire per-object lock for access to KASAN metadata. */
> 
> I believe there's strong reason not to use standard spin_lock() or
> similar. I think it's proper place to explain it.
> 

will do.

> > +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
> > +{
> > +   union kasan_alloc_data old, new;
> > +
> > +   preempt_disable();
> 
> It's better to disable and enable preemption inside the loop
> on each iteration, to decrease contention.
> 

ok, makes sense; will do.

> > +   for (;;) {
> > +   old.packed = READ_ONCE(alloc_info->data);
> > +   if (unlikely(old.lock)) {
> > +   cpu_relax();
> > +   continue;
> > +   }
> > +   new.packed = old.packed;
> > +   new.lock = 1;
> > +   if (likely(cmpxchg(_info->data, old.packed, new.packed)
> > +   == old.packed))
> > +   break;
> > +   }
> > +}
> > +
> > +/* release lock after a kasan_meta_lock(). */
> > +void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
> > +{
> > +   union kasan_alloc_data alloc_data;
> > +
> > +   alloc_data.packed = READ_ONCE(alloc_info->data);
> > +   alloc_data.lock = 0;
> > +   if (unlikely(xchg(_info->data, alloc_data.packed) !=
> > +   (alloc_data.packed | 0x1U)))
> > +   WARN_ONCE(1, "%s: lock not held!\n", __func__);
> 
> Nitpick. It never happens in normal case, correct?. Why don't you place it 
> under
> some developer config, or even leave at dev branch? The function will
> be twice shorter without it.

ok, will remove/shorten.

> > +   alloc_data.packed = alloc_info->data;
> > +   if (alloc_data.state == KASAN_STATE_ALLOC) {
> > +   free_info = get_free_info(cache, object);
> > +   quarantine_put(free_info, cache);
> 
> I just pulled master and didn't find this function. If your patchset
> is based on other branch, please notice it.

Sorry; patchset is based on linux-next 'next-20160506' which has Alexander
Potapenko's patches for KASAN SLAB support with memory quarantine +
stackdepot features.

> 
> > +   set_track(_info->track, GFP_NOWAIT);
> 
> It may fail for many reasons. Is it OK to ignore it? If OK, I think it
> should be explained.

It's ok. A subsequent bug report on object would have a missing alloc/dealloc
stack trace. 

> 
> > +   kasan_poison_slab_free(cache, object);
> > +   alloc_data.state = KASAN_STATE_QUARANTINE;
> > +   alloc_info->data = alloc_data.packed;
> > +   kasan_meta_unlock(alloc_info);
> > +   return true;
> > }
> > +   switch (alloc_data.state) {
> > +   case KASAN_STATE_QUARANTINE:
> > +   case KASAN_STATE_FREE:
> > +   kasan_report((unsigned long)object, 0, false,
> > +   (unsigned long)__builtin_return_address(1));
> 
> __builtin_return_address() is unsafe if argument is non-zero. Use
> return_address() instead.

hmm, I/cscope can't seem to find an x86 implementation for return_address().
Will dig further; thanks.

> > +   local_irq_save(flags);
> > +   kasan_meta_lock(alloc_info);
> > +   alloc_data.packed = alloc_info->data;
> > +   alloc_data.state = KASAN_STATE_ALLOC;
> > +   alloc_data.size_delta = cache->object_size - size;
> > +   alloc_info->data = alloc_data.packed;
> > set_track(_info->track, flags);
> 
> Same as above
>
As above. 

Kuthonuzo

RE: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Luruo, Kuthonuzo]

Thank you for the review!

> > +
> > +/* acquire per-object lock for access to KASAN metadata. */
> 
> I believe there's strong reason not to use standard spin_lock() or
> similar. I think it's proper place to explain it.
> 

will do.

> > +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
> > +{
> > +   union kasan_alloc_data old, new;
> > +
> > +   preempt_disable();
> 
> It's better to disable and enable preemption inside the loop
> on each iteration, to decrease contention.
> 

ok, makes sense; will do.

> > +   for (;;) {
> > +   old.packed = READ_ONCE(alloc_info->data);
> > +   if (unlikely(old.lock)) {
> > +   cpu_relax();
> > +   continue;
> > +   }
> > +   new.packed = old.packed;
> > +   new.lock = 1;
> > +   if (likely(cmpxchg(_info->data, old.packed, new.packed)
> > +   == old.packed))
> > +   break;
> > +   }
> > +}
> > +
> > +/* release lock after a kasan_meta_lock(). */
> > +void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
> > +{
> > +   union kasan_alloc_data alloc_data;
> > +
> > +   alloc_data.packed = READ_ONCE(alloc_info->data);
> > +   alloc_data.lock = 0;
> > +   if (unlikely(xchg(_info->data, alloc_data.packed) !=
> > +   (alloc_data.packed | 0x1U)))
> > +   WARN_ONCE(1, "%s: lock not held!\n", __func__);
> 
> Nitpick. It never happens in normal case, correct?. Why don't you place it 
> under
> some developer config, or even leave at dev branch? The function will
> be twice shorter without it.

ok, will remove/shorten.

> > +   alloc_data.packed = alloc_info->data;
> > +   if (alloc_data.state == KASAN_STATE_ALLOC) {
> > +   free_info = get_free_info(cache, object);
> > +   quarantine_put(free_info, cache);
> 
> I just pulled master and didn't find this function. If your patchset
> is based on other branch, please notice it.

Sorry; patchset is based on linux-next 'next-20160506' which has Alexander
Potapenko's patches for KASAN SLAB support with memory quarantine +
stackdepot features.

> 
> > +   set_track(_info->track, GFP_NOWAIT);
> 
> It may fail for many reasons. Is it OK to ignore it? If OK, I think it
> should be explained.

It's ok. A subsequent bug report on object would have a missing alloc/dealloc
stack trace. 

> 
> > +   kasan_poison_slab_free(cache, object);
> > +   alloc_data.state = KASAN_STATE_QUARANTINE;
> > +   alloc_info->data = alloc_data.packed;
> > +   kasan_meta_unlock(alloc_info);
> > +   return true;
> > }
> > +   switch (alloc_data.state) {
> > +   case KASAN_STATE_QUARANTINE:
> > +   case KASAN_STATE_FREE:
> > +   kasan_report((unsigned long)object, 0, false,
> > +   (unsigned long)__builtin_return_address(1));
> 
> __builtin_return_address() is unsafe if argument is non-zero. Use
> return_address() instead.

hmm, I/cscope can't seem to find an x86 implementation for return_address().
Will dig further; thanks.

> > +   local_irq_save(flags);
> > +   kasan_meta_lock(alloc_info);
> > +   alloc_data.packed = alloc_info->data;
> > +   alloc_data.state = KASAN_STATE_ALLOC;
> > +   alloc_data.size_delta = cache->object_size - size;
> > +   alloc_info->data = alloc_data.packed;
> > set_track(_info->track, flags);
> 
> Same as above
>
As above. 

Kuthonuzo

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Yury Norov]

On Fri, May 06, 2016 at 05:17:27PM +0530, Kuthonuzo Luruo wrote:
> Currently, KASAN may fail to detect concurrent deallocations of the same
> object due to a race in kasan_slab_free(). This patch makes double-free
> detection more reliable by serializing access to KASAN object metadata.
> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
> lock/unlock per-object metadata. Double-free errors are now reported via
> kasan_report().
> 
> Testing:
> - Tested with a modified version of the 'slab_test' microbenchmark where
>   allocs occur on CPU 0; then all other CPUs concurrently attempt to free
>   the same object.
> - Tested with new 'test_kasan' kasan_double_free() test in accompanying
>   patch.
> 
> Signed-off-by: Kuthonuzo Luruo 
> ---
> 
> Changes in v2:
> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>   using fewer bits overall.
> - Double-free pr_err promoted to kasan_report().
> - kasan_init_object() introduced to initialize KASAN object metadata
>   during slab creation. KASAN_STATE_INIT initialization removed from
>   kasan_poison_object_data().
>  
> ---
>  include/linux/kasan.h |8 +++
>  mm/kasan/kasan.c  |  118 
> -
>  mm/kasan/kasan.h  |   15 +-
>  mm/kasan/quarantine.c |7 +++-
>  mm/kasan/report.c |   31 +++--
>  mm/slab.c |1 +
>  6 files changed, 142 insertions(+), 38 deletions(-)
> 
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 645c280..c7bf625 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -78,6 +78,10 @@ struct kasan_cache {
>  int kasan_module_alloc(void *addr, size_t size);
>  void kasan_free_shadow(const struct vm_struct *vm);
>  
> +#ifdef CONFIG_SLAB
> +void kasan_init_object(struct kmem_cache *cache, void *object);
> +#endif
> +
>  #else /* CONFIG_KASAN */
>  
>  static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
> @@ -124,6 +128,10 @@ static inline void kasan_poison_slab_free(struct 
> kmem_cache *s, void *object) {}
>  static inline int kasan_module_alloc(void *addr, size_t size) { return 0; }
>  static inline void kasan_free_shadow(const struct vm_struct *vm) {}
>  
> +#ifdef CONFIG_SLAB
> +static inline void kasan_init_object(struct kmem_cache *cache, void *object) 
> {}
> +#endif
> +
>  #endif /* CONFIG_KASAN */
>  
>  #endif /* LINUX_KASAN_H */
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..a840b49 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -34,6 +34,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "kasan.h"
>  #include "../slab.h"
> @@ -419,13 +420,6 @@ 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;
> - }
> -#endif
>  }
>  
>  #ifdef CONFIG_SLAB
> @@ -470,6 +464,18 @@ static inline depot_stack_handle_t save_stack(gfp_t 
> flags)
>   return depot_save_stack(, flags);
>  }
>  
> +void kasan_init_object(struct kmem_cache *cache, void *object)
> +{
> + struct kasan_alloc_meta *alloc_info;
> +
> + if (cache->flags & SLAB_KASAN) {
> + kasan_unpoison_object_data(cache, object);
> + alloc_info = get_alloc_info(cache, object);
> + __memset(alloc_info, 0, sizeof(*alloc_info));
> + kasan_poison_object_data(cache, object);
> + }
> +}
> +
>  static inline void set_track(struct kasan_track *track, gfp_t flags)
>  {
>   track->pid = current->pid;
> @@ -489,6 +495,39 @@ struct kasan_free_meta *get_free_info(struct kmem_cache 
> *cache,
>   BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32);
>   return (void *)object + cache->kasan_info.free_meta_offset;
>  }
> +
> +/* acquire per-object lock for access to KASAN metadata. */

I believe there's strong reason not to use standard spin_lock() or
similar. I think it's proper place to explain it.

> +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
> +{
> + union kasan_alloc_data old, new;
> +
> + preempt_disable();

It's better to disable and enable preemption inside the loop
on each iteration, to decrease contention.

> + for (;;) {
> + old.packed = READ_ONCE(alloc_info->data);
> + if (unlikely(old.lock)) {
> + cpu_relax();
> + continue;
> + }
> + new.packed = old.packed;
> + new.lock = 1;
> + if (likely(cmpxchg(_info->data, old.packed, new.packed)
> + 

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Yury Norov]

On Fri, May 06, 2016 at 05:17:27PM +0530, Kuthonuzo Luruo wrote:
> Currently, KASAN may fail to detect concurrent deallocations of the same
> object due to a race in kasan_slab_free(). This patch makes double-free
> detection more reliable by serializing access to KASAN object metadata.
> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
> lock/unlock per-object metadata. Double-free errors are now reported via
> kasan_report().
> 
> Testing:
> - Tested with a modified version of the 'slab_test' microbenchmark where
>   allocs occur on CPU 0; then all other CPUs concurrently attempt to free
>   the same object.
> - Tested with new 'test_kasan' kasan_double_free() test in accompanying
>   patch.
> 
> Signed-off-by: Kuthonuzo Luruo 
> ---
> 
> Changes in v2:
> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>   using fewer bits overall.
> - Double-free pr_err promoted to kasan_report().
> - kasan_init_object() introduced to initialize KASAN object metadata
>   during slab creation. KASAN_STATE_INIT initialization removed from
>   kasan_poison_object_data().
>  
> ---
>  include/linux/kasan.h |8 +++
>  mm/kasan/kasan.c  |  118 
> -
>  mm/kasan/kasan.h  |   15 +-
>  mm/kasan/quarantine.c |7 +++-
>  mm/kasan/report.c |   31 +++--
>  mm/slab.c |1 +
>  6 files changed, 142 insertions(+), 38 deletions(-)
> 
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 645c280..c7bf625 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -78,6 +78,10 @@ struct kasan_cache {
>  int kasan_module_alloc(void *addr, size_t size);
>  void kasan_free_shadow(const struct vm_struct *vm);
>  
> +#ifdef CONFIG_SLAB
> +void kasan_init_object(struct kmem_cache *cache, void *object);
> +#endif
> +
>  #else /* CONFIG_KASAN */
>  
>  static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
> @@ -124,6 +128,10 @@ static inline void kasan_poison_slab_free(struct 
> kmem_cache *s, void *object) {}
>  static inline int kasan_module_alloc(void *addr, size_t size) { return 0; }
>  static inline void kasan_free_shadow(const struct vm_struct *vm) {}
>  
> +#ifdef CONFIG_SLAB
> +static inline void kasan_init_object(struct kmem_cache *cache, void *object) 
> {}
> +#endif
> +
>  #endif /* CONFIG_KASAN */
>  
>  #endif /* LINUX_KASAN_H */
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..a840b49 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -34,6 +34,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "kasan.h"
>  #include "../slab.h"
> @@ -419,13 +420,6 @@ 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;
> - }
> -#endif
>  }
>  
>  #ifdef CONFIG_SLAB
> @@ -470,6 +464,18 @@ static inline depot_stack_handle_t save_stack(gfp_t 
> flags)
>   return depot_save_stack(, flags);
>  }
>  
> +void kasan_init_object(struct kmem_cache *cache, void *object)
> +{
> + struct kasan_alloc_meta *alloc_info;
> +
> + if (cache->flags & SLAB_KASAN) {
> + kasan_unpoison_object_data(cache, object);
> + alloc_info = get_alloc_info(cache, object);
> + __memset(alloc_info, 0, sizeof(*alloc_info));
> + kasan_poison_object_data(cache, object);
> + }
> +}
> +
>  static inline void set_track(struct kasan_track *track, gfp_t flags)
>  {
>   track->pid = current->pid;
> @@ -489,6 +495,39 @@ struct kasan_free_meta *get_free_info(struct kmem_cache 
> *cache,
>   BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32);
>   return (void *)object + cache->kasan_info.free_meta_offset;
>  }
> +
> +/* acquire per-object lock for access to KASAN metadata. */

I believe there's strong reason not to use standard spin_lock() or
similar. I think it's proper place to explain it.

> +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
> +{
> + union kasan_alloc_data old, new;
> +
> + preempt_disable();

It's better to disable and enable preemption inside the loop
on each iteration, to decrease contention.

> + for (;;) {
> + old.packed = READ_ONCE(alloc_info->data);
> + if (unlikely(old.lock)) {
> + cpu_relax();
> + continue;
> + }
> + new.packed = old.packed;
> + new.lock = 1;
> + if (likely(cmpxchg(_info->data, old.packed, new.packed)
> +