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

Darren Hart
2009-03-12T23:22:20Z

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