Re: [RESEND PATCH] futex: fix key reference counter in case of requeue.

[Darren Hart]

On 10/15/2010 05:16 AM, Thomas Gleixner wrote:
> 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.


We do, and it's a bit more tangled. No surprises there.


> 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.


Agreed. We'll get this and futex_wait_requeue_pi fixed now, and then 
I'll spend some time cleaning this mess up after LPC.

I'll test the following along with a wait_requeue_pi fix and report back.

--
Darren

> 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 =&current_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);


--
Darren Hart
Embedded Linux Kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/