Re: [PATCH 2/6] Additional (get|put)_futex_key() fixes

[Darren Hart <dvhltc@us.ibm.com>]

Thomas Gleixner wrote:
> On Thu, 12 Mar 2009, Ingo Molnar wrote:
>> * Darren Hart <dvhltc@us.ibm.com> wrote:
>>
>>> futex_requeue and futex_lock_pi still had some bad 
>>> (get|put)_futex_key() usage. This patch adds the missing 
>>> put_futex_keys() and corrects a goto in futex_lock_pi() to 
>>> avoid a double get.
>>>
>>> Build and boot tested on a 4 way Intel x86_64 workstation.  
>>> Passes basic pthread_mutex and PI tests out of 
>>> ltp/testcases/realtime.
>> hm, how bad is the impact - do we need this in v2.6.29?
> 
> I think so. We leak key references in some of the error/retry code
> pathes.  Darrens patch does not apply to mainline. Backport below.
> 

I think you may have made a mistake in the application of the patch.  I 
did a "git cherry-pick" of this patch onto linux-2.6.tip master and it 
didn't complain, the patch itself was only different by a couple of line 
numbers.  Trying to apply this patch manually resulted in:

$ patch -p1 < fixes.diff
patching file kernel/futex.c
Hunk #1 succeeded at 805 (offset 3 lines).
Hunk #2 succeeded at 883 (offset 3 lines).
Hunk #3 succeeded at 1468 (offset 10 lines).
Hunk #4 succeeded at 1611 (offset 10 lines).
Hunk #5 succeeded at 1720 (offset 10 lines).

So I think this patch should be fine.  Before I wrote the patch I 
checked to make sure that my branch had merged tip/master which had the 
most recent futex patches from mainline.

Thanks,

Darren

> Thanks,
> 
> 	tglx
> ---
> Subject: futex: fix key reference leaks
> From: Darren Hart <dvhltc@us.ibm.com>
> Date: Thu, 12 Mar 2009 12:10:01 +0100
> 
> Impact: bugfix
> 
> futex_wake_op, futex_requeue, futex_lock_pi and futex_unlock_pi still
> had some bad (get|put)_futex_key() usage. This patch adds the missing
> put_futex_keys() and corrects a goto in futex_lock_pi() to avoid a
> double get.
> 
> [ tglx: backport to mainline ]
> 
> Signed-off-by: Darren Hart <dvhltc@us.ibm.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> ---
> 
>  kernel/futex.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/kernel/futex.c
> ===================================================================
> --- linux-2.6.orig/kernel/futex.c
> +++ linux-2.6/kernel/futex.c
> @@ -803,6 +803,9 @@ retry:
>  			goto retry;
>  		}
> 
> +		put_futex_key(fshared, &key2);
> +		put_futex_key(fshared, &key1);
> +
>  		ret = get_user(dummy, uaddr2);
>  		if (ret)
>  			return ret;
> @@ -881,12 +884,15 @@ retry:
>  			if (hb1 != hb2)
>  				spin_unlock(&hb2->lock);
> 
> +			put_futex_key(fshared, &key2);
> +			put_futex_key(fshared, &key1);
> +
>  			ret = get_user(curval, uaddr1);
> 
>  			if (!ret)
>  				goto retry;
> 
> -			goto out_put_keys;
> +			return ret;
>  		}
>  		if (curval != *cmpval) {
>  			ret = -EAGAIN;
> @@ -1459,7 +1465,7 @@ retry_locked:
>  			 */
>  			queue_unlock(&q, hb);
>  			cond_resched();
> -			goto retry;
> +			goto retry_unlocked;
> 
>  		case -ESRCH:
>  			/*
> @@ -1598,6 +1604,7 @@ uaddr_faulted:
>  		goto retry_unlocked;
>  	}
> 
> +	put_futex_key(fshared, &q.key);
>  	ret = get_user(uval, uaddr);
>  	if (!ret)
>  		goto retry;
> @@ -1709,6 +1716,8 @@ pi_faulted:
>  		goto retry_unlocked;
>  	}
> 
> +	put_futex_key(fshared, &key);
> +
>  	ret = get_user(uval, uaddr);
>  	if (!ret)
>  		goto retry;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team