From: Jamie Lokier I had an inkling when I sent out the recent futex hashed-lock fix that there was a race condition, but I was too tired to be sure or to work out a solution. This patch fixes it. In unqueue_me(), if certain events occur in another task we take the wrong spinlock. I realised that yesterday, but proving that the fix works to my satisfaction required a state of higher consciousness not normally attained until 5am (and soon lost...). The form of the fix is inspired by an idea from Rusty. Please folks, take a look at the fix in unqueue_me(). It looks simple but the proof isn't obvious. Oh, and some comment formatting requested by Andrew Morton. :) kernel/futex.c | 77 +++++++++++++++++++++++++++++++++++++++------------------ 1 files changed, 53 insertions(+), 24 deletions(-) diff -puN kernel/futex.c~futex-locking-fix-fix kernel/futex.c --- 25/kernel/futex.c~futex-locking-fix-fix 2003-10-03 01:38:04.000000000 -0700 +++ 25-akpm/kernel/futex.c 2003-10-03 01:38:04.000000000 -0700 @@ -256,18 +256,24 @@ static inline void drop_key_refs(union f } } -/* The hash bucket lock must be held when this is called. - Afterwards, the futex_q must not be accessed. */ +/* + * The hash bucket lock must be held when this is called. + * Afterwards, the futex_q must not be accessed. + */ static inline void wake_futex(struct futex_q *q) { list_del_init(&q->list); if (q->filp) send_sigio(&q->filp->f_owner, q->fd, POLL_IN); - /* The lock in wake_up_all() is a crucial memory barrier after the - list_del_init() and also before assigning to q->lock_ptr. */ + /* + * The lock in wake_up_all() is a crucial memory barrier after the + * list_del_init() and also before assigning to q->lock_ptr. + */ wake_up_all(&q->waiters); - /* The waiting task can free the futex_q as soon as this is written, - without taking any locks. This must come last. */ + /* + * The waiting task can free the futex_q as soon as this is written, + * without taking any locks. This must come last. + */ q->lock_ptr = 0; } @@ -397,15 +403,34 @@ static inline void queue_me(struct futex } /* Return 1 if we were still queued (ie. 0 means we were woken) */ -static inline int unqueue_me(struct futex_q *q) +static int unqueue_me(struct futex_q *q) { int ret = 0; - spinlock_t *lock_ptr = q->lock_ptr; + spinlock_t *lock_ptr; /* In the common case we don't take the spinlock, which is nice. */ + retry: + lock_ptr = q->lock_ptr; if (lock_ptr != 0) { spin_lock(lock_ptr); - if (!list_empty(&q->list)) { + /* + * q->lock_ptr can change between reading it and + * spin_lock(), causing us to take the wrong lock. This + * corrects the race condition. + * + * Reasoning goes like this: if we have the wrong lock, + * q->lock_ptr must have changed (maybe several times) + * between reading it and the spin_lock(). It can + * change again after the spin_lock() but only if it was + * already changed before the spin_lock(). It cannot, + * however, change back to the original value. Therefore + * we can detect whether we acquired the correct lock. + */ + if (unlikely(lock_ptr != q->lock_ptr)) { + spin_unlock(lock_ptr); + goto retry; + } + if (likely(!list_empty(&q->list))) { list_del(&q->list); ret = 1; } @@ -462,8 +487,10 @@ static int futex_wait(unsigned long uadd /* add_wait_queue is the barrier after __set_current_state. */ __set_current_state(TASK_INTERRUPTIBLE); add_wait_queue(&q.waiters, &wait); - /* !list_empty() is safe here without any lock. - q.lock_ptr != 0 is not safe, because of ordering against wakeup. */ + /* + * !list_empty() is safe here without any lock. + * q.lock_ptr != 0 is not safe, because of ordering against wakeup. + */ if (likely(!list_empty(&q.list))) time = schedule_timeout(time); __set_current_state(TASK_RUNNING); @@ -505,18 +532,16 @@ static unsigned int futex_poll(struct fi struct poll_table_struct *wait) { struct futex_q *q = filp->private_data; - spinlock_t *lock_ptr; - int ret = POLLIN | POLLRDNORM; + int ret = 0; poll_wait(filp, &q->waiters, wait); - lock_ptr = q->lock_ptr; - if (lock_ptr != 0) { - spin_lock(lock_ptr); - if (!list_empty(&q->list)) - ret = 0; - spin_unlock(lock_ptr); - } + /* + * list_empty() is safe here without any lock. + * q->lock_ptr != 0 is not safe, because of ordering against wakeup. + */ + if (list_empty(&q->list)) + ret = POLLIN | POLLRDNORM; return ret; } @@ -526,8 +551,10 @@ static struct file_operations futex_fops .poll = futex_poll, }; -/* Signal allows caller to avoid the race which would occur if they - set the sigio stuff up afterwards. */ +/* + * Signal allows caller to avoid the race which would occur if they + * set the sigio stuff up afterwards. + */ static int futex_fd(unsigned long uaddr, int signal) { struct futex_q *q; @@ -583,8 +610,10 @@ static int futex_fd(unsigned long uaddr, return err; } - /* queue_me() must be called before releasing mmap_sem, because - key->shared.inode needs to be referenced while holding it. */ + /* + * queue_me() must be called before releasing mmap_sem, because + * key->shared.inode needs to be referenced while holding it. + */ filp->private_data = q; queue_me(q, ret, filp); _