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

[Darren Hart]

Thomas Gleixner wrote:
> On Thu, 12 Mar 2009, Ingo Molnar wrote:
> > * Darren Hart <dvhltc@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>
> ---
>
>  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@xxxxxxxxxxxxxxx
> 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
--
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/