On Thu, Sep 10, 2015 at 07:26:09PM +0200, Dmitry Vyukov wrote:
> On Thu, Sep 10, 2015 at 7:13 PM, Paul E. McKenney
> <paulmck@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx> 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.
Fair point!
Thanx, Paul
> >> 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@xxxxxxxxxx
> >> 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@xxxxxxxxxx
> 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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>