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

[Dmitry Vyukov <dvyukov@google.com>]

On Thu, Sep 10, 2015 at 7:13 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Thu, Sep 10, 2015 at 11:55:35AM +0200, Dmitry Vyukov wrote:
>> On Thu, Sep 10, 2015 at 1:31 AM, Christoph Lameter <cl@linux.com> wrote:
>> > On Wed, 9 Sep 2015, Paul E. McKenney wrote:
>> >
>> >> Either way, Dmitry's tool got a hit on real code using the slab
>> >> allocators.  If that hit is a false positive, then clearly Dmitry
>> >> needs to fix his tool, however, I am not (yet) convinced that it is a
>> >> false positive.  If it is not a false positive, we might well need to
>> >> articulate the rules for use of the slab allocators.
>> >
>> > Could I get a clear definiton as to what exactly is positive? Was this
>> > using SLAB, SLUB or SLOB?
>> >
>> >> > This would all use per cpu data. As soon as a handoff is required within
>> >> > the allocators locks are being used. So I would say no.
>> >>
>> >> As in "no, it is not necessary for the caller of kfree() to invoke barrier()
>> >> in this example", right?
>> >
>> > Actually SLUB contains a barrier already in kfree(). Has to be there
>> > because of the way the per cpu pointer is being handled.
>>
>> The positive was reporting of data races in the following 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);
>>          }
>>
>> //drivers/tty/tty_buffer.c
>> while ((next = buf->head->next) != NULL) {
>>      tty_buffer_free(port, buf->head);
>>      buf->head = next;
>> }
>>
>> Namely, the tool reported data races between usage of the object in
>> other threads before they released the object and kfree.
>>
>> I am not sure why we are so concentrated on details like SLAB vs SLUB
>> vs SLOB or cache coherency protocols. This looks like waste of time to
>> me. General kernel code should not be safe only when working with SLxB
>> due to current implementation details of SLxB, it should be safe
>> according to memory allocator contract. And this contract seem to be:
>> memory allocator can do arbitrary reads and writes to the object
>> inside of kmalloc and kfree.
>
> The reason we poked at this was to see if any of SLxB touched the
> memory being freed.  If none of them touched the memory being freed,
> and if that was a policy, then the idiom above would be legal.  However,
> one of them does touch the memory being freed, so, yes, the above code
> needs to be fixed.

No. The object can be instantly reallocated and user can write to the
object. Consider:

if (READ_ONCE(p->free))
  kfree(p);
y = kmalloc(8);
// assuming p's size is 8, y is most likely equal to p and there are
no barriers on the kmalloc fast path
*(void**)y = 0;

This is equivalent to kmalloc writing to the object in this respect.



>> Similarly for memory model. There is officially documented kernel
>> memory model, which all general kernel code must adhere to. Reasoning
>> about whether a particular piece of code works on architecture X, or
>> how exactly it can break on architecture Y in unnecessary in such
>> context. In the end, there can be memory allocator implementation and
>> new architectures.
>>
>> My question is about contracts, not about current implementation
>> details or specific architectures.
>>
>> There are memory allocator implementations that do reads and writes of
>> the object, and there are memory allocator implementations that do not
>> do any barriers on fast paths. From this follows that objects must be
>> passed in quiescent state to kfree.
>> Now, kernel memory model says "A load-load control dependency requires
>> a full read memory barrier".
>> >From this follows that the following code is broken:
>>
>> // 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);
>>          }
>>
>> and it should be:
>>
>> // kernel/pid.c
>>          if ((smp_load_acquire(&pid->count) == 1) ||
>
> If Will Deacon's patch providing generic support for relaxed atomics
> made it in, we want:
>
>           if ((atomic_read_acquire(&pid->count) == 1) ||
>
> Otherwise, we need an explicit barrier.
>
>                                                         Thanx, Paul
>
>>               atomic_dec_and_test(&pid->count)) {
>>                  kmem_cache_free(ns->pid_cachep, pid);
>>                  put_pid_ns(ns);
>>          }
>>
>>
>>
>> --
>> 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.
>>
>



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