Messages in this thread | | | Date | Mon, 05 Oct 2009 11:23:05 +0800 | From | Amerigo Wang <> | Subject | Re: [Patch] rwsem: fix rwsem_is_locked() bug |
| |
Andrew Morton wrote: > On Tue, 29 Sep 2009 23:19:02 -0400 > Amerigo Wang <amwang@redhat.com> wrote: > >> rwsem_is_locked() tests ->activity without locks, so we should always >> keep ->activity consistent. However, the code in __rwsem_do_wake() >> breaks this rule, it updates ->activity after _all_ readers waken up, >> this may give some reader a wrong ->activity value, thus cause >> rwsem_is_locked() behaves wrong. >> >> Brian has a kernel module to reproduce this, I can include it >> if any of you need. Of course, with Brian's approval. >> >> With this patch applied, I can't trigger that bug any more. >> > > Changelog doesn't describe the bug well.
Sorry for my English. :-/
> >> --- >> diff --git a/lib/rwsem-spinlock.c b/lib/rwsem-spinlock.c >> index 9df3ca5..44e4484 100644 >> --- a/lib/rwsem-spinlock.c >> +++ b/lib/rwsem-spinlock.c >> @@ -49,7 +49,6 @@ __rwsem_do_wake(struct rw_semaphore *sem, int wakewrite) >> { >> struct rwsem_waiter *waiter; >> struct task_struct *tsk; >> - int woken; >> >> waiter = list_entry(sem->wait_list.next, struct rwsem_waiter, list); >> >> @@ -78,24 +77,21 @@ __rwsem_do_wake(struct rw_semaphore *sem, int wakewrite) >> >> /* grant an infinite number of read locks to the front of the queue */ >> dont_wake_writers: >> - woken = 0; >> while (waiter->flags & RWSEM_WAITING_FOR_READ) { >> struct list_head *next = waiter->list.next; >> >> + sem->activity++; >> list_del(&waiter->list); >> tsk = waiter->task; >> smp_mb(); >> waiter->task = NULL; >> wake_up_process(tsk); >> put_task_struct(tsk); >> - woken++; >> if (list_empty(&sem->wait_list)) >> break; >> waiter = list_entry(next, struct rwsem_waiter, list); >> } >> >> - sem->activity += woken; >> - >> out: >> return sem; >> } > > So if I understand this correctly > > - we have one or more processes sleeping in down_read(), waiting for access. > > - we wake one or more processes up without altering ->activity > > - they start to run and they do rwsem_is_locked(). This incorrectly > returns "false", because the waker process is still crunching away in > __rwsem_do_wake(). > > - the waker now alters ->activity, but it was too late. > > And the patch fixes this by updating ->activity prior to waking the > sleeping processes. So when they run, they'll see a non-zero value of > ->activity. > > Fair enough, I guess.
Yes, exactly.
But after reading David's comments, I realized that rwsem_is_locked() has more problems, this only fixes one of them.
I will try another fix.
> > I don't know if we really need this in -stable. Do we expect that > there will be any real runtime bugs arising from this?
Not sure, I need an extra kernel module to trigger this bug, so probably it doesn't affect the real kernel.
Thanks!
| |