out_unlock: does not only drop the locks, it also drops the refcount
on the pi_state. Really intuitive.
Move the label after the put_pi_state() call and use 'break' in the
error handling path of the requeue loop.
Signed-off-by: Thomas Gleixner <[email protected]>
---
kernel/futex.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1842,20 +1842,21 @@ static int futex_requeue(u32 __user *uad
*/
this->pi_state = NULL;
put_pi_state(pi_state);
- goto out_unlock;
+ break;
}
}
requeue_futex(this, hb1, hb2, &key2);
drop_count++;
}
-out_unlock:
/*
* We took an extra initial reference to the pi_state either
* in futex_proxy_trylock_atomic() or in lookup_pi_state(). We
* need to drop it here again.
*/
put_pi_state(pi_state);
+
+out_unlock:
double_unlock_hb(hb1, hb2);
wake_up_q(&wake_q);
hb_waiters_dec(hb2);
On Sat, Dec 19, 2015 at 08:07:41PM -0000, Thomas Gleixner wrote:
> out_unlock: does not only drop the locks, it also drops the refcount
> on the pi_state. Really intuitive.
>
> Move the label after the put_pi_state() call and use 'break' in the
> error handling path of the requeue loop.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> kernel/futex.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1842,20 +1842,21 @@ static int futex_requeue(u32 __user *uad
> */
> this->pi_state = NULL;
> put_pi_state(pi_state);
> - goto out_unlock;
> + break;
> }
> }
> requeue_futex(this, hb1, hb2, &key2);
> drop_count++;
> }
>
> -out_unlock:
> /*
> * We took an extra initial reference to the pi_state either
> * in futex_proxy_trylock_atomic() or in lookup_pi_state(). We
> * need to drop it here again.
> */
> put_pi_state(pi_state);
> +
> +out_unlock:
> double_unlock_hb(hb1, hb2);
> wake_up_q(&wake_q);
> hb_waiters_dec(hb2);
Thanks for catching the leak Thomas, sorry I missed it :-/
I thought you missed one point early on shortly after retry_private: where we
goto out_unlock, but we haven't claimed the pi_state yet - so this appears to
have been another unnecessary (harmless) put_pi_state call previously.
For the series:
Reviewed-by: Darren Hart <[email protected]>
As a follow-on, I think it might be worthwhile to create a symmetrical
get_pi_state() to the put_pi_state(), rather than handling the atomic_inc
directly.
And finally, while the break; in futex_requeue works, that function is quite
long and an explicit out_put_pi_state: label would make the intention clear and
also avoid inadvertently breaking the implicit behavior of the break.
Thanks,
--
Darren Hart
Intel Open Source Technology Center
On Sat, 2015-12-19 at 21:15 -0800, Darren Hart wrote:
> As a follow-on, I think it might be worthwhile to create a symmetrical
> get_pi_state() to the put_pi_state(), rather than handling the atomic_inc
> directly.
Ditto, immediate thought was future auditors will look for it.
-Mike
On Sat, Dec 19, 2015 at 09:15:24PM -0800, Darren Hart wrote:
>
> As a follow-on, I think it might be worthwhile to create a symmetrical
> get_pi_state() to the put_pi_state(), rather than handling the atomic_inc
> directly.
>
> And finally, while the break; in futex_requeue works, that function is quite
> long and an explicit out_put_pi_state: label would make the intention clear and
> also avoid inadvertently breaking the implicit behavior of the break.
>
And while prototyping these changes, I've changed my mind, neither is a
worthwhile change.
The plist_for_each in futex_requeue really isn't that long and the breaks occur
in a logical way and are now well documented with this series. An inadvertent
change to this behavior seems unlikely.
There is only one open coded atomic_inc in futex.c for the pi_state refcount,
hardly worth a wrapper.
Regards,
--
Darren Hart
Intel Open Source Technology Center
On Sun, Dec 20, 2015 at 06:40:07AM +0100, Mike Galbraith wrote:
> On Sat, 2015-12-19 at 21:15 -0800, Darren Hart wrote:
>
> > As a follow-on, I think it might be worthwhile to create a symmetrical
> > get_pi_state() to the put_pi_state(), rather than handling the atomic_inc
> > directly.
>
> Ditto, immediate thought was future auditors will look for it.
Hrm, well, I had just dismissed this after some digging, but OK, let's think it
through a bit...
Turns out there is only one open coded atomic_inc. the other occurs through
attach_to_pi_state, sometimes via lookup_pi_state. We might be able to
consolidate that some so it had a more symmetric and consistent usage pattern.
A "get' in the futex op functions currently occurs via:
1) lookup_pi_state -> attach_to_pi_state()
2) attach_to_pi_state()
(directly - nearly copying lookup_pi_state at the call site)
3) futex_lock_pi_atomic -> attach_to_pi_state()
4) atomic_inc(pi_state->ref_count) in futex_requeue_pi on behalf of the waiter
in futex_wait_requeue_pi.
Newly allocated or re-purposed pi_states get their refcount set to 1 which
doesn't really count as a "get" until a lookup_pi_state or implicit acquisition
through futex_lock_pi_atomic->attach_to_pi_state.
As it turns out, lookup_pi_state is only used once in futex.c, but it really
does make that section more readable. Despite an overall savings of a couple of
lines, I think it's worth keeping, even if it adds another layer to the "get".
As a further cleanup, Thomas removed the unnecessary calls to put_pi_state, and
those that remain have no ambiguity about whether or not the pi_state is NULL.
We *could* drop the NULL check in put_pi_state, which would make it's use all
the more explicit. Perhaps a BUG_ON(!pi_state)? Not included below.
The first get is still implicit in the way the caching of the pi_state works.
This gets assigned with an existing refcount of 1 in attach_to_pi_owner from the
cache via alloc_pi_state. I don't know if there is a reason for starting it off
as 1 instead of 0. Perhaps we could make this more explicit by keeping it at 0
in the cache and using get_pi_state in alloc_pi_state before giving it to the
waiter?
Something like this perhaps? (Untested):
>From 2d4cce06d36b16dcd038d7a68a6efc7740848ee1 Mon Sep 17 00:00:00 2001
Message-Id: <2d4cce06d36b16dcd038d7a68a6efc7740848ee1.1450596757.git.dvhart@linux.intel.com>
From: Darren Hart <[email protected]>
Date: Sat, 19 Dec 2015 22:57:05 -0800
Subject: [PATCH] futex: Add a get_pi_state wrapper
For audit purposes, add an inline wrapper, get_pi_state(), around
atomic_inc(&pi_state->refcount) to parallel the newly renamed
put_pi_state().
To make the get/set parallel and more explicit, keep the refcount at 0
while in the cache and only inc to 1 when it is allocated to a waiter
via alloc_pi_state().
Signed-off-by: Darren Hart <[email protected]>
---
kernel/futex.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index ae83ea7..4c71c86 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -706,7 +706,7 @@ static int refill_pi_state_cache(void)
INIT_LIST_HEAD(&pi_state->list);
/* pi_mutex gets initialized later */
pi_state->owner = NULL;
- atomic_set(&pi_state->refcount, 1);
+ atomic_set(&pi_state->refcount, 0);
pi_state->key = FUTEX_KEY_INIT;
current->pi_state_cache = pi_state;
@@ -714,12 +714,21 @@ static int refill_pi_state_cache(void)
return 0;
}
+/*
+ * Adds a reference to the pi_state object.
+ */
+static inline void get_pi_state(struct futex_pi_state *pi_state)
+{
+ atomic_inc(&pi_state->refcount);
+}
+
static struct futex_pi_state * alloc_pi_state(void)
{
struct futex_pi_state *pi_state = current->pi_state_cache;
WARN_ON(!pi_state);
current->pi_state_cache = NULL;
+ get_pi_state(pi_state);
return pi_state;
}
@@ -756,10 +765,9 @@ static void put_pi_state(struct futex_pi_state *pi_state)
/*
* pi_state->list is already empty.
* clear pi_state->owner.
- * refcount is at 0 - put it back to 1.
+ * refcount is already at 0.
*/
pi_state->owner = NULL;
- atomic_set(&pi_state->refcount, 1);
current->pi_state_cache = pi_state;
}
}
@@ -954,7 +962,7 @@ static int attach_to_pi_state(u32 uval, struct futex_pi_state *pi_state,
if (pid != task_pid_vnr(pi_state->owner))
return -EINVAL;
out_state:
- atomic_inc(&pi_state->refcount);
+ get_pi_state(pi_state);
*ps = pi_state;
return 0;
}
@@ -1813,7 +1821,7 @@ retry_private:
* refcount on the pi_state and store the pointer in
* the futex_q object of the waiter.
*/
- atomic_inc(&pi_state->refcount);
+ get_pi_state(pi_state);
this->pi_state = pi_state;
ret = rt_mutex_start_proxy_lock(&pi_state->pi_mutex,
this->rt_waiter,
--
2.1.4
--
Darren Hart
Intel Open Source Technology Center
Commit-ID: 885c2cb770b5ac2507c41bc9f91a5d1c98337bee
Gitweb: http://git.kernel.org/tip/885c2cb770b5ac2507c41bc9f91a5d1c98337bee
Author: Thomas Gleixner <[email protected]>
AuthorDate: Sat, 19 Dec 2015 20:07:41 +0000
Committer: Thomas Gleixner <[email protected]>
CommitDate: Sun, 20 Dec 2015 12:43:25 +0100
futex: Cleanup the goto confusion in requeue_pi()
out_unlock: does not only drop the locks, it also drops the refcount
on the pi_state. Really intuitive.
Move the label after the put_pi_state() call and use 'break' in the
error handling path of the requeue loop.
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Darren Hart <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: [email protected]
Cc: Andy Lowe <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Thomas Gleixner <[email protected]>
---
kernel/futex.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index dcec018..461d438 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1839,20 +1839,25 @@ retry_private:
*/
this->pi_state = NULL;
put_pi_state(pi_state);
- goto out_unlock;
+ /*
+ * We stop queueing more waiters and let user
+ * space deal with the mess.
+ */
+ break;
}
}
requeue_futex(this, hb1, hb2, &key2);
drop_count++;
}
-out_unlock:
/*
* We took an extra initial reference to the pi_state either
* in futex_proxy_trylock_atomic() or in lookup_pi_state(). We
* need to drop it here again.
*/
put_pi_state(pi_state);
+
+out_unlock:
double_unlock_hb(hb1, hb2);
wake_up_q(&wake_q);
hb_waiters_dec(hb2);