Messages in this thread | | | Date | Thu, 17 Sep 2015 20:09:19 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH] kernel: fix data race in put_pid |
| |
On 09/17, Dmitry Vyukov wrote: > > I can update the patch description, but let me explain it here first.
Yes thanks.
> Here is the essence of what happens:
Aha, so you really meant that 2 put_pid's can race with each other,
> // thread 1 > 1: pid->foo = 1; // foo is the first word of pid object > // then it does put_pid > 2: atomic_dec_and_test(&pid->count) // decrements count to 1 and > returns false so the function returns > > // thread 2 > // executes put_pid > 3: atomic_load(&pid->count); // returns 1, so proceed to kmem_cache_free > // then kmem_cache_free does: > 5: head->freelist = (void*)pid; > > This can be executed as: > > 4: *(void**)pid = head->freelist; > 1: pid->foo = 1; // foo is the first word of pid object > 2: atomic_dec_and_test(&pid->count) // decrements count to 1 and > returns false so the function returns > 3: atomic_load(&pid->count); // returns 1, so proceed to kmem_cache_free > 5: head->freelist = (void*)pid;
Unless I am totally confused, everything is simpler. We can forget about the hoisting, freelist, etc.
Thread 2 can see the result of atomic_dec_and_test(), but not the result of "pid->foo = 1". In this case in can free the object which can be re-allocated _before_ STORE(pid->foo) completes. Of course, this would be really bad.
I need to recheck, but afaics this is not possible. This optimization is fine, but probably needs a comment. We rely on delayed_put_pid() called by RCU. And note that nobody can write to this pid after it is removed from the rcu-protected list.
So I think this is false alarm, but I'll try to recheck tomorrow, it is too late for me today.
Oleg.
| |