Messages in this thread | | | Date | Wed, 30 Sep 2009 16:08:23 -0700 | From | Andrew Morton <> | Subject | Re: [Patch] rwsem: fix rwsem_is_locked() bug |
| |
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.
> > --- > 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.
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?
| |