Re: Is it OK to pass non-acquired objects to kfree?

[Dmitry Vyukov <dvyukov@google.com>]

On Tue, Sep 8, 2015 at 4:13 PM, Christoph Lameter <cl@linux.com> wrote:
> On Tue, 8 Sep 2015, Dmitry Vyukov wrote:
>
>> The question arose during work on KernelThreadSanitizer, a kernel data
>> race, and in particular caused by the following existing code:
>>
>> // kernel/pid.c
>>          if ((atomic_read(&pid->count) == 1) ||
>>               atomic_dec_and_test(&pid->count)) {
>>                  kmem_cache_free(ns->pid_cachep, pid);
>>                  put_pid_ns(ns);
>>          }
>
> It frees when there the refcount is one? Should this not be
>
>         if (atomic_read(&pid->count) === 0) || ...

The code is meant to do decrement of pid->count, but since
pid->count==1 it figures out that it is the only owner of the object,
so it just skips the "pid->count--" part and proceeds directly to
free.

>> //drivers/tty/tty_buffer.c
>> while ((next = buf->head->next) != NULL) {
>>      tty_buffer_free(port, buf->head);
>>      buf->head = next;
>> }
>> // Here another thread can concurrently append to the buffer list, and
>> tty_buffer_free eventually calls kfree.
>>
>> Both these cases don't contain proper memory barrier before handing
>> off the object to kfree. In my opinion the code should use
>> smp_load_acquire or READ_ONCE_CTRL ("control-dependnecy-acquire").
>> Otherwise there can be pending memory accesses to the object in other
>> threads that can interfere with slab code or the next usage of the
>> object after reuse.
>
> There can be pending reads maybe? But a write would require exclusive
> acccess to the cachelines.
>
>
>> Paul McKenney suggested that:
>>
>> "
>> The maintainers probably want this sort of code to be allowed:
>>         p->a++;
>>         if (p->b) {
>>                 kfree(p);
>>                 p = NULL;
>>         }
>> And the users even more so.
>
>
> Sure. What would be the problem with the above code? The write to the
> object (p->a++) results in exclusive access to a cacheline being obtained.
> So one cpu holds that cacheline. Then the object is freed and reused
> either

I am not sure what cache line states has to do with it...
Anyway, another thread can do p->c++ after this thread does p->a++,
then this thread loses its ownership. Or p->c can be located on a
separate cache line with p->a. And then we still free the object with
a pending write.

> 1. On the same cpu -> No problem.
>
> 2. On another cpu. This means that a hand off of the pointer to the object
> occurs in the slab allocators. The hand off involves a spinlock and thus
> implicit barriers. The other processor will acquire exclusive access to
> the cacheline when it initializes the object. At that point the cacheline
> ownership will transfer between the processors.
>



-- 
Dmitry Vyukov, Software Engineer, dvyukov@google.com
Google Germany GmbH, Dienerstraße 12, 80331, München
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
sind, leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>