Messages in this thread | | | Date | Thu, 17 Sep 2015 10:56:57 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH] kernel: fix data race in put_pid |
| |
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. >
| |