On Thu, 14 Oct 2010, Louis Rilling wrote:
Once process P is woken up, it should unqueue, drop reference to 'key2'
(the one referring to the futex_q, this is done in unqueue_me())
and to 'key1' (the one referring to futex_wait operation). Without this
patch it drops reference to 'key2' instead of 'key1'.
I can see the bug, but while the patch fixes it I don't think it is
the proper solution. Aside of that we might have a similar problem in
the futex_wait_requeue_pi() code.
The real underlying problem is, that futex_wait_setup() returns with
two references held in the case of success. That's what needs to be
fixed in the first place.
The futex_wait() case can be fixed with the patch below, still looking
into the futex_wait_requeue_pi() maze.
Darren, this whole key refcounting needs to be simplified _AND_
documented.
Thanks,
tglx
---
Index: linux-2.6-tip/kernel/futex.c
===================================================================
--- linux-2.6-tip.orig/kernel/futex.c
+++ linux-2.6-tip/kernel/futex.c
@@ -1786,8 +1786,14 @@ retry_private:
}
out:
- if (ret)
- put_futex_key(fshared,&q->key);
+ /*
+ * On success we hold here two references acquired in
+ * get_futex_key() and queue_lock(). Drop one.
+ *
+ * On failure we hold one reference acquired in
+ * get_futex_key(). Drop it.
+ */
+ put_futex_key(fshared,&q->key);
return ret;
}
@@ -1819,7 +1825,7 @@ static int futex_wait(u32 __user *uaddr,
}
retry:
- /* Prepare to wait on uaddr. */
+ /* Prepare to wait on uaddr. Hold hb lock and q.key ref on success */
ret = futex_wait_setup(uaddr, val, fshared,&q,&hb);
if (ret)
goto out;
@@ -1829,24 +1835,23 @@ retry:
/* If we were woken (and unqueued), we succeeded, whatever. */
ret = 0;
+ /* unqueue_me() drops q.key ref */
if (!unqueue_me(&q))
- goto out_put_key;
+ goto out;
ret = -ETIMEDOUT;
if (to&& !to->task)
- goto out_put_key;
+ goto out;
/*
* We expect signal_pending(current), but we might be the
* victim of a spurious wakeup as well.
*/
- if (!signal_pending(current)) {
- put_futex_key(fshared,&q.key);
+ if (!signal_pending(current))
goto retry;
- }
ret = -ERESTARTSYS;
if (!abs_time)
- goto out_put_key;
+ goto out;
restart =¤t_thread_info()->restart_block;
restart->fn = futex_wait_restart;
@@ -1863,8 +1868,6 @@ retry:
ret = -ERESTART_RESTARTBLOCK;
-out_put_key:
- put_futex_key(fshared,&q.key);
out:
if (to) {
hrtimer_cancel(&to->timer);