Messages in this thread | | | Date | Thu, 17 Sep 2015 18:08:37 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH] kernel: fix data race in put_pid |
| |
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 >
| |