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

[Dmitry Vyukov <dvyukov@google.com>]

On Tue, Sep 8, 2015 at 5:13 PM, Christoph Lameter <cl@linux.com> wrote:
> On Tue, 8 Sep 2015, Dmitry Vyukov wrote:
>
>> >>
>> >> // 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.
>
> The atomic_dec_and_test will therefore not be executed for count == 1?
> Strange code. The atomic_dec_and_test suggests there are concurrency
> concerns. The count test with a simple comparison does not share these
> concerns it seems.

Yes, it skips atomic decrement when counter is equal to 1. This is
relatively common optimization for basic-thread-safety reference
counting (when you can acquire a new reference only when you already
have a one). If counter == 1, then the only owner is the current
thread, so other threads cannot change counter concurrently. So there
is no point in doing the atomic decrement (we know that the counter
will go to 0).

>> >> 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.
>
> The subsystem must ensure no other references exist before a call to free.
> So this cannot occur. If it does then these are cases of an object being
> used after free which can be caught by a number of diagnostic tools in the
> kernel.


Yes, this is a case of use-after-free bug. But the use-after-free can
happen only due to memory access reordering in a multithreaded
environment.
OK, here is a simpler code snippet:

void *p; // = NULL

// thread 1
p = kmalloc(8);

// thread 2
void *r = READ_ONCE(p);
if (r != NULL)
    kfree(r);

I would expect that this is illegal code. Is my understanding correct?


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