Re: [PATCH] kernel: fix data race in put_pid

From	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH] kernel: fix data race in put_pid
Date	2015-09-17 20:00 +0200
Message-ID	<q9LDs-2vw-7@gated-at.bofh.it> (permalink)
References	<q9Hqa-4OV-5@gated-at.bofh.it> <q9K4G-np-11@gated-at.bofh.it> <q9KxJ-W9-47@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On Thu, Sep 17, 2015 at 06:41:46PM +0200, Dmitry Vyukov wrote:
> What happens here exactly matches what is described in CONTROL
> DEPENDENCIES section of Documentation/memory-barriers.txt. So all the
> bad things described there are possible here. The document explicitly
> requires usage of rmb/acquire/READ_ONCE_CTRL in such cases. I don't
> know what to add to that.

I suggest explicitly listing the steps leading to a specific instance
of the failure.  This isn't always easy to do, but it can be very helpful.

> Regarding reordering of "ns = pid->numbers[pid->level].ns". If we are
> talking about the thread that releases the last reference via the
> fast-path atomic_read check, then, yes, they can be reordered by both
> compiler and hardware, but only in a non-observable way, and so it
> does not matter. Only in a non-observable way because the freeing
> thread is the same as the thread that does "ns =
> pid->numbers[pid->level].ns" and both compilers and hardware visibly
> preserve program order for single-thread.

Agreed.  If the compiler or the CPU did that reordering in a way that was
visible, that would be a bug even in single-threaded code.  Therefore,
if that reordering matters, both the compiler and the CPU are forbidden
from doing it.

> If we are talking about threads that release all but last reference,
> then, no, they can't be reordered, because those threads execute
> atomic_dec_and_test which is a release operation.

Also agreed.

							Thanx, Paul

> On Thu, Sep 17, 2015 at 6:08 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > Honestly, I can not see how this can happen. So I do not really
> > understand the problem and the fix.
> >
> > And if this can happen I can't understand how this patch can help.
> > What about "ns = pid->numbers[pid->level].ns" ? It can be reordered
> > with atomic_read_acquire().
> >
> > I leave this to other reviewers, but perhaps you can spell the
> > "For example" part of the changelog.
> >
> >
> > On 09/17, Dmitry Vyukov wrote:
> >>
> >> put_pid checks whether the current thread has the only reference
> >> to the pid with atomic_read() which does not have any memory
> >> barriers, and if so proceeds directly to kmem_cache_free().
> >> As the result memory accesses to the object in kmem_cache_free()
> >> or user accesses to the object after reallocation (again without
> >> any memory barriers on fast path) can hoist above the atomic_read()
> >> check and conflict with memory accesses to the pid object in other
> >> threads before they released their references.
> >>
> >> There is a control dependency between the atomic_read() check and
> >> kmem_cache_free(), but control dependencies are disregarded by some
> >> architectures. Documentation/memory-barriers.txt explicitly states:
> >> "A load-load control dependency requires a full read memory barrier.
> >> ... please note that READ_ONCE_CTRL() is not optional! [even for stores]"
> >> in the CONTROL DEPENDENCIES section.
> >>
> >> For example, if store to the first word of the object to build a freelist
> >> in kmem_cache_free() hoists above the check, stores to the first word
> >> in other threads can corrupt the memory allocator freelist.
> >>
> >> Use atomic_read_acquire() for the fast path check to hand off properly
> >> acquired object to memory allocator.
> >>
> >> The data race was found with KernelThreadSanitizer (KTSAN).
> >>
> >> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> >> ---
> >>  kernel/pid.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/pid.c b/kernel/pid.c
> >> index ca36879..3b0b13d 100644
> >> --- a/kernel/pid.c
> >> +++ b/kernel/pid.c
> >> @@ -242,7 +242,7 @@ void put_pid(struct pid *pid)
> >>               return;
> >>
> >>       ns = pid->numbers[pid->level].ns;
> >> -     if ((atomic_read(&pid->count) == 1) ||
> >> +     if ((atomic_read_acquire(&pid->count) == 1) ||
> >>            atomic_dec_and_test(&pid->count)) {
> >>               kmem_cache_free(ns->pid_cachep, pid);
> >>               put_pid_ns(ns);
> >> --
> >> 2.6.0.rc0.131.gf624c3d
> >>
> >
> 
> 
> 
> -- 
> 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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-17 15:30 +0200
  Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-17 18:20 +0200
    Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-17 18:50 +0200
      Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-17 19:50 +0200
        Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-17 20:00 +0200
          Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-17 20:10 +0200
          Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-17 20:20 +0200
            Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-17 20:40 +0200
            Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 11:00 +0200
              Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 11:10 +0200
                Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 11:40 +0200
                Re: [PATCH] kernel: fix data race in put_pid James Hogan <james.hogan@imgtec.com> - 2015-09-18 14:40 +0200
                Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 14:50 +0200
                Re: [PATCH] kernel: fix data race in put_pid "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-18 18:40 +0200
              Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-18 11:10 +0200
                Re: [PATCH] kernel: fix data race in put_pid Will Deacon <will.deacon@arm.com> - 2015-09-18 11:30 +0200
                Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 11:40 +0200
                Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 13:30 +0200
                Re: [PATCH] kernel: fix data race in put_pid Will Deacon <will.deacon@arm.com> - 2015-09-18 13:40 +0200
                Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-18 14:00 +0200
                Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 14:10 +0200
                Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 14:30 +0200
                Re: [PATCH] kernel: fix data race in put_pid Will Deacon <will.deacon@arm.com> - 2015-09-18 14:50 +0200
                Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 15:20 +0200
                Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-18 15:50 +0200
                Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 16:00 +0200
                Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-18 16:00 +0200
                Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-18 16:50 +0200
                Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-22 10:40 +0200
                Re: [PATCH] kernel: fix data race in put_pid Eric Dumazet <eric.dumazet@gmail.com> - 2015-09-18 18:20 +0200
                Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 18:30 +0200
                Re: [PATCH] kernel: fix data race in put_pid "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-18 18:00 +0200
              Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-18 15:40 +0200
              Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-18 15:40 +0200
                Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 16:00 +0200
                Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-18 17:10 +0200
                Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-18 17:40 +0200
                Re: [PATCH] kernel: fix data race in put_pid "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-18 18:30 +0200
      Re: [PATCH] kernel: fix data race in put_pid "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-17 20:00 +0200

csiph-web