[patch 0/5] futex: Plug a pi_state leak and clarify the refcounting

[patch 0/5] futex: Plug a pi_state leak and clarify the refcounting

[Thomas Gleixner]

A recent patch claimed that there is a double free in the requeue_pi
code, which is not the case.

While analysing the issue I found the contrary, i.e. a leak. This
series fixes the leak and clarifies the code so it's more clear how
that refcounting on the pi state works.

Thanks,

	tglx

[patch 1/5] futex: Drop refcount if requeue_pi() acquired the rtmutex

[Thomas Gleixner]

[-- Attachment #1: futex--Drop-refcount-if-requeue_pi-acquired-the-rtmutex --]
[-- Type: text/plain, Size: 810 bytes --]

If the proxy lock in the requeue loop acquires the rtmutex for a
waiter then it acquired also refcount on the pi_state related to the
futex, but the waiter side does not drop the reference count.

Add the missing free_pi_state() call.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 kernel/futex.c |    5 +++++
 1 file changed, 5 insertions(+)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2755,6 +2755,11 @@ static int futex_wait_requeue_pi(u32 __u
 		if (q.pi_state && (q.pi_state->owner != current)) {
 			spin_lock(q.lock_ptr);
 			ret = fixup_pi_state_owner(uaddr2, &q, current);
+			/*
+			 * Drop the reference to the pi state which
+			 * the requeue_pi() code acquired for us.
+			 */
+			free_pi_state(q.pi_state);
 			spin_unlock(q.lock_ptr);
 		}
 	} else {

[tip:locking/core] futex: Drop refcount if requeue_pi() acquired the rtmutex

[tip-bot for Thomas Gleixner]

Commit-ID:  fb75a4282d0d9a3c7c44d940582c2d226cf3acfb
Gitweb:     http://git.kernel.org/tip/fb75a4282d0d9a3c7c44d940582c2d226cf3acfb
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 19 Dec 2015 20:07:38 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 20 Dec 2015 12:43:24 +0100

futex: Drop refcount if requeue_pi() acquired the rtmutex

If the proxy lock in the requeue loop acquires the rtmutex for a
waiter then it acquired also refcount on the pi_state related to the
futex, but the waiter side does not drop the reference count.

Add the missing free_pi_state() call.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Darren Hart <darren@dvhart.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Bhuvanesh_Surachari@mentor.com
Cc: Andy Lowe <Andy_Lowe@mentor.com>
Link: http://lkml.kernel.org/r/20151219200607.178132067@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 kernel/futex.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/futex.c b/kernel/futex.c
index 684d754..24fbc77 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2755,6 +2755,11 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 		if (q.pi_state && (q.pi_state->owner != current)) {
 			spin_lock(q.lock_ptr);
 			ret = fixup_pi_state_owner(uaddr2, &q, current);
+			/*
+			 * Drop the reference to the pi state which
+			 * the requeue_pi() code acquired for us.
+			 */
+			free_pi_state(q.pi_state);
 			spin_unlock(q.lock_ptr);
 		}
 	} else {

[patch 2/5] futex: Rename free_pi_state() to put_pi_state()

[Thomas Gleixner]

[-- Attachment #1: futex--Rename-free_pi_state-to-put_pi_state --]
[-- Type: text/plain, Size: 2188 bytes --]

free_pi_state() is confusing as it is in fact only freeing/caching the
pi state when the last reference is gone. Rename it to put_pi_state()
which reflects better what it is doing.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/futex.c |   17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -725,9 +725,12 @@ static struct futex_pi_state * alloc_pi_
 }
 
 /*
+ * Drops a reference to the pi_state object and frees or caches it
+ * when the last reference is gone.
+ *
  * Must be called with the hb lock held.
  */
-static void free_pi_state(struct futex_pi_state *pi_state)
+static void put_pi_state(struct futex_pi_state *pi_state)
 {
 	if (!pi_state)
 		return;
@@ -1729,7 +1732,7 @@ static int futex_requeue(u32 __user *uad
 		case 0:
 			break;
 		case -EFAULT:
-			free_pi_state(pi_state);
+			put_pi_state(pi_state);
 			pi_state = NULL;
 			double_unlock_hb(hb1, hb2);
 			hb_waiters_dec(hb2);
@@ -1746,7 +1749,7 @@ static int futex_requeue(u32 __user *uad
 			 *   exit to complete.
 			 * - The user space value changed.
 			 */
-			free_pi_state(pi_state);
+			put_pi_state(pi_state);
 			pi_state = NULL;
 			double_unlock_hb(hb1, hb2);
 			hb_waiters_dec(hb2);
@@ -1815,7 +1818,7 @@ static int futex_requeue(u32 __user *uad
 			} else if (ret) {
 				/* -EDEADLK */
 				this->pi_state = NULL;
-				free_pi_state(pi_state);
+				put_pi_state(pi_state);
 				goto out_unlock;
 			}
 		}
@@ -1824,7 +1827,7 @@ static int futex_requeue(u32 __user *uad
 	}
 
 out_unlock:
-	free_pi_state(pi_state);
+	put_pi_state(pi_state);
 	double_unlock_hb(hb1, hb2);
 	wake_up_q(&wake_q);
 	hb_waiters_dec(hb2);
@@ -1973,7 +1976,7 @@ static void unqueue_me_pi(struct futex_q
 	__unqueue_futex(q);
 
 	BUG_ON(!q->pi_state);
-	free_pi_state(q->pi_state);
+	put_pi_state(q->pi_state);
 	q->pi_state = NULL;
 
 	spin_unlock(q->lock_ptr);
@@ -2759,7 +2762,7 @@ static int futex_wait_requeue_pi(u32 __u
 			 * Drop the reference to the pi state which
 			 * the requeue_pi() code acquired for us.
 			 */
-			free_pi_state(q.pi_state);
+			put_pi_state(q.pi_state);
 			spin_unlock(q.lock_ptr);
 		}
 	} else {

[tip:locking/core] futex: Rename free_pi_state() to put_pi_state( )

[tip-bot for Thomas Gleixner]

Commit-ID:  29e9ee5d48c35d6cf8afe09bdf03f77125c9ac11
Gitweb:     http://git.kernel.org/tip/29e9ee5d48c35d6cf8afe09bdf03f77125c9ac11
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 19 Dec 2015 20:07:39 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 20 Dec 2015 12:43:24 +0100

futex: Rename free_pi_state() to put_pi_state()

free_pi_state() is confusing as it is in fact only freeing/caching the
pi state when the last reference is gone. Rename it to put_pi_state()
which reflects better what it is doing.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Darren Hart <darren@dvhart.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Bhuvanesh_Surachari@mentor.com
Cc: Andy Lowe <Andy_Lowe@mentor.com>
Link: http://lkml.kernel.org/r/20151219200607.259636467@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/futex.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 24fbc77..f1581ff 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -725,9 +725,12 @@ static struct futex_pi_state * alloc_pi_state(void)
 }
 
 /*
+ * Drops a reference to the pi_state object and frees or caches it
+ * when the last reference is gone.
+ *
  * Must be called with the hb lock held.
  */
-static void free_pi_state(struct futex_pi_state *pi_state)
+static void put_pi_state(struct futex_pi_state *pi_state)
 {
 	if (!pi_state)
 		return;
@@ -1729,7 +1732,7 @@ retry_private:
 		case 0:
 			break;
 		case -EFAULT:
-			free_pi_state(pi_state);
+			put_pi_state(pi_state);
 			pi_state = NULL;
 			double_unlock_hb(hb1, hb2);
 			hb_waiters_dec(hb2);
@@ -1746,7 +1749,7 @@ retry_private:
 			 *   exit to complete.
 			 * - The user space value changed.
 			 */
-			free_pi_state(pi_state);
+			put_pi_state(pi_state);
 			pi_state = NULL;
 			double_unlock_hb(hb1, hb2);
 			hb_waiters_dec(hb2);
@@ -1815,7 +1818,7 @@ retry_private:
 			} else if (ret) {
 				/* -EDEADLK */
 				this->pi_state = NULL;
-				free_pi_state(pi_state);
+				put_pi_state(pi_state);
 				goto out_unlock;
 			}
 		}
@@ -1824,7 +1827,7 @@ retry_private:
 	}
 
 out_unlock:
-	free_pi_state(pi_state);
+	put_pi_state(pi_state);
 	double_unlock_hb(hb1, hb2);
 	wake_up_q(&wake_q);
 	hb_waiters_dec(hb2);
@@ -1973,7 +1976,7 @@ static void unqueue_me_pi(struct futex_q *q)
 	__unqueue_futex(q);
 
 	BUG_ON(!q->pi_state);
-	free_pi_state(q->pi_state);
+	put_pi_state(q->pi_state);
 	q->pi_state = NULL;
 
 	spin_unlock(q->lock_ptr);
@@ -2759,7 +2762,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 			 * Drop the reference to the pi state which
 			 * the requeue_pi() code acquired for us.
 			 */
-			free_pi_state(q.pi_state);
+			put_pi_state(q.pi_state);
 			spin_unlock(q.lock_ptr);
 		}
 	} else {

[patch 3/5] futex: Document pi_state refcounting in requeue code

[Thomas Gleixner]

[-- Attachment #1: futex--Document-pi_state-refcounting-in-requeue-code --]
[-- Type: text/plain, Size: 2859 bytes --]

Documentation of the pi_state refcounting in the requeue code is non
existent. Add it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/futex.c |   36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1709,6 +1709,8 @@ static int futex_requeue(u32 __user *uad
 		 * exist yet, look it up one more time to ensure we have a
 		 * reference to it. If the lock was taken, ret contains the
 		 * vpid of the top waiter task.
+		 * If the lock was not taken, we have pi_state and an initial
+		 * refcount on it. In case of an error we have nothing.
 		 */
 		if (ret > 0) {
 			WARN_ON(pi_state);
@@ -1724,12 +1726,16 @@ static int futex_requeue(u32 __user *uad
 			 * it. So we rather use the known value than
 			 * rereading and handing potential crap to
 			 * lookup_pi_state.
+			 *
+			 * If that call succeeds then we have pi_state
+			 * and an initial refcount on it.
 			 */
 			ret = lookup_pi_state(ret, hb2, &key2, &pi_state);
 		}
 
 		switch (ret) {
 		case 0:
+			/* We hold a reference on the pi state. */
 			break;
 		case -EFAULT:
 			put_pi_state(pi_state);
@@ -1804,19 +1810,38 @@ static int futex_requeue(u32 __user *uad
 		 * of requeue_pi if we couldn't acquire the lock atomically.
 		 */
 		if (requeue_pi) {
-			/* Prepare the waiter to take the rt_mutex. */
+			/*
+			 * Prepare the waiter to take the rt_mutex. Take a
+			 * refcount on the pi_state and store the pointer in
+			 * the futex_q object of the waiter.
+			 */
 			atomic_inc(&pi_state->refcount);
 			this->pi_state = pi_state;
 			ret = rt_mutex_start_proxy_lock(&pi_state->pi_mutex,
 							this->rt_waiter,
 							this->task);
 			if (ret == 1) {
-				/* We got the lock. */
+				/*
+				 * We got the lock. We do neither drop
+				 * the refcount on pi_state nor clear
+				 * this->pi_state because the waiter
+				 * needs the pi_state for cleaning up
+				 * the user space value. It will drop
+				 * the refcount after doing so.
+				 */
 				requeue_pi_wake_futex(this, &key2, hb2);
 				drop_count++;
 				continue;
 			} else if (ret) {
-				/* -EDEADLK */
+				/*
+				 * rt_mutex_start_proxy_lock()
+				 * detected a potential deadlock when
+				 * we tried to queue that waiter. Drop
+				 * the pi_state reference which we
+				 * took above and remove the pointer
+				 * to the state from the waiters
+				 * futex_q object.
+				 */
 				this->pi_state = NULL;
 				put_pi_state(pi_state);
 				goto out_unlock;
@@ -1827,6 +1852,11 @@ static int futex_requeue(u32 __user *uad
 	}
 
 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);
 	double_unlock_hb(hb1, hb2);
 	wake_up_q(&wake_q);

Re: [patch 3/5] futex: Document pi_state refcounting in requeue code

[Darren Hart]

On Sat, Dec 19, 2015 at 08:07:39PM -0000, Thomas Gleixner wrote:
> Documentation of the pi_state refcounting in the requeue code is non
> existent. Add it.
> 

OK, one nitpic on this one I guess - 80 characters is pretty narrow as it is in
my humble opinion, could we expand the newly added comment blocks to use all of
the 80 character limit?

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/futex.c |   36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> --- a/kernel/futex.c
> +++ b/kernel/futex.c

...

> -				/* We got the lock. */
> +				/*
> +				 * We got the lock. We do neither drop
> +				 * the refcount on pi_state nor clear
> +				 * this->pi_state because the waiter
> +				 * needs the pi_state for cleaning up
> +				 * the user space value. It will drop
> +				 * the refcount after doing so.
> +				 */
>  				requeue_pi_wake_futex(this, &key2, hb2);
>  				drop_count++;
>  				continue;
>  			} else if (ret) {
> -				/* -EDEADLK */
> +				/*
> +				 * rt_mutex_start_proxy_lock()
> +				 * detected a potential deadlock when
> +				 * we tried to queue that waiter. Drop
> +				 * the pi_state reference which we
> +				 * took above and remove the pointer
> +				 * to the state from the waiters
> +				 * futex_q object.
> +				 */

Especially the two paragraphs above ^

-- 
Darren Hart
Intel Open Source Technology Center

[tip:locking/core] futex: Document pi_state refcounting in requeue code

[tip-bot for Thomas Gleixner]

Commit-ID:  ecb38b78f698a51988ec456751b20440e54702fb
Gitweb:     http://git.kernel.org/tip/ecb38b78f698a51988ec456751b20440e54702fb
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 19 Dec 2015 20:07:39 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 20 Dec 2015 12:43:24 +0100

futex: Document pi_state refcounting in requeue code

Documentation of the pi_state refcounting in the requeue code is non
existent. Add it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Darren Hart <darren@dvhart.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Bhuvanesh_Surachari@mentor.com
Cc: Andy Lowe <Andy_Lowe@mentor.com>
Link: http://lkml.kernel.org/r/20151219200607.335938312@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/futex.c | 51 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 12 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index f1581ff..20c4683 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1709,27 +1709,31 @@ retry_private:
 		 * exist yet, look it up one more time to ensure we have a
 		 * reference to it. If the lock was taken, ret contains the
 		 * vpid of the top waiter task.
+		 * If the lock was not taken, we have pi_state and an initial
+		 * refcount on it. In case of an error we have nothing.
 		 */
 		if (ret > 0) {
 			WARN_ON(pi_state);
 			drop_count++;
 			task_count++;
 			/*
-			 * If we acquired the lock, then the user
-			 * space value of uaddr2 should be vpid. It
-			 * cannot be changed by the top waiter as it
-			 * is blocked on hb2 lock if it tries to do
-			 * so. If something fiddled with it behind our
-			 * back the pi state lookup might unearth
-			 * it. So we rather use the known value than
-			 * rereading and handing potential crap to
-			 * lookup_pi_state.
+			 * If we acquired the lock, then the user space value
+			 * of uaddr2 should be vpid. It cannot be changed by
+			 * the top waiter as it is blocked on hb2 lock if it
+			 * tries to do so. If something fiddled with it behind
+			 * our back the pi state lookup might unearth it. So
+			 * we rather use the known value than rereading and
+			 * handing potential crap to lookup_pi_state.
+			 *
+			 * If that call succeeds then we have pi_state and an
+			 * initial refcount on it.
 			 */
 			ret = lookup_pi_state(ret, hb2, &key2, &pi_state);
 		}
 
 		switch (ret) {
 		case 0:
+			/* We hold a reference on the pi state. */
 			break;
 		case -EFAULT:
 			put_pi_state(pi_state);
@@ -1804,19 +1808,37 @@ retry_private:
 		 * of requeue_pi if we couldn't acquire the lock atomically.
 		 */
 		if (requeue_pi) {
-			/* Prepare the waiter to take the rt_mutex. */
+			/*
+			 * Prepare the waiter to take the rt_mutex. Take a
+			 * refcount on the pi_state and store the pointer in
+			 * the futex_q object of the waiter.
+			 */
 			atomic_inc(&pi_state->refcount);
 			this->pi_state = pi_state;
 			ret = rt_mutex_start_proxy_lock(&pi_state->pi_mutex,
 							this->rt_waiter,
 							this->task);
 			if (ret == 1) {
-				/* We got the lock. */
+				/*
+				 * We got the lock. We do neither drop the
+				 * refcount on pi_state nor clear
+				 * this->pi_state because the waiter needs the
+				 * pi_state for cleaning up the user space
+				 * value. It will drop the refcount after
+				 * doing so.
+				 */
 				requeue_pi_wake_futex(this, &key2, hb2);
 				drop_count++;
 				continue;
 			} else if (ret) {
-				/* -EDEADLK */
+				/*
+				 * rt_mutex_start_proxy_lock() detected a
+				 * potential deadlock when we tried to queue
+				 * that waiter. Drop the pi_state reference
+				 * which we took above and remove the pointer
+				 * to the state from the waiters futex_q
+				 * object.
+				 */
 				this->pi_state = NULL;
 				put_pi_state(pi_state);
 				goto out_unlock;
@@ -1827,6 +1849,11 @@ retry_private:
 	}
 
 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);
 	double_unlock_hb(hb1, hb2);
 	wake_up_q(&wake_q);

[patch 4/5] futex: Remove pointless put_pi_state calls in requeue()

[Thomas Gleixner]

[-- Attachment #1: futex--Remove-pointless-free_pi_state-calls-in-requeue --]
[-- Type: text/plain, Size: 889 bytes --]

In the error handling cases we neither have pi_state nor a reference
to it. Remove the pointless code.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/futex.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1737,9 +1737,9 @@ static int futex_requeue(u32 __user *uad
 		case 0:
 			/* We hold a reference on the pi state. */
 			break;
+
+			/* If the above failed, then pi_state is NULL */
 		case -EFAULT:
-			put_pi_state(pi_state);
-			pi_state = NULL;
 			double_unlock_hb(hb1, hb2);
 			hb_waiters_dec(hb2);
 			put_futex_key(&key2);
@@ -1755,8 +1755,6 @@ static int futex_requeue(u32 __user *uad
 			 *   exit to complete.
 			 * - The user space value changed.
 			 */
-			put_pi_state(pi_state);
-			pi_state = NULL;
 			double_unlock_hb(hb1, hb2);
 			hb_waiters_dec(hb2);
 			put_futex_key(&key2);

[tip:locking/core] futex: Remove pointless put_pi_state calls in requeue()

[tip-bot for Thomas Gleixner]

Commit-ID:  4959f2de11ca532a120a337429e5576fd283700f
Gitweb:     http://git.kernel.org/tip/4959f2de11ca532a120a337429e5576fd283700f
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 19 Dec 2015 20:07:40 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 20 Dec 2015 12:43:25 +0100

futex: Remove pointless put_pi_state calls in requeue()

In the error handling cases we neither have pi_state nor a reference
to it. Remove the pointless code.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Darren Hart <darren@dvhart.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Bhuvanesh_Surachari@mentor.com
Cc: Andy Lowe <Andy_Lowe@mentor.com>
Link: http://lkml.kernel.org/r/20151219200607.432780944@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/futex.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 20c4683..dcec018 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1735,9 +1735,9 @@ retry_private:
 		case 0:
 			/* We hold a reference on the pi state. */
 			break;
+
+			/* If the above failed, then pi_state is NULL */
 		case -EFAULT:
-			put_pi_state(pi_state);
-			pi_state = NULL;
 			double_unlock_hb(hb1, hb2);
 			hb_waiters_dec(hb2);
 			put_futex_key(&key2);
@@ -1753,8 +1753,6 @@ retry_private:
 			 *   exit to complete.
 			 * - The user space value changed.
 			 */
-			put_pi_state(pi_state);
-			pi_state = NULL;
 			double_unlock_hb(hb1, hb2);
 			hb_waiters_dec(hb2);
 			put_futex_key(&key2);

[patch 5/5] futex: Cleanup the goto confusion in requeue_pi()

[Thomas Gleixner]

[-- Attachment #1: futex--Cleanup-goto-mess-in-requeue-pi --]
[-- Type: text/plain, Size: 935 bytes --]

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 <tglx@linutronix.de>
---
 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);

Re: [patch 5/5] futex: Cleanup the goto confusion in requeue_pi()

[Darren Hart]

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 <tglx@linutronix.de>
> ---
>  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 <dvhart@linux.intel.com>

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

Re: [patch 5/5] futex: Cleanup the goto confusion in requeue_pi()

[Mike Galbraith]

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

Re: [patch 5/5] futex: Cleanup the goto confusion in requeue_pi()

[Darren Hart]

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 <dvhart@linux.intel.com>
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 <dvhart@linux.intel.com>
---
 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

Re: [patch 5/5] futex: Cleanup the goto confusion in requeue_pi()

[Darren Hart]

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

[tip:locking/core] futex: Cleanup the goto confusion in requeue_pi()

[tip-bot for Thomas Gleixner]

Commit-ID:  885c2cb770b5ac2507c41bc9f91a5d1c98337bee
Gitweb:     http://git.kernel.org/tip/885c2cb770b5ac2507c41bc9f91a5d1c98337bee
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 19 Dec 2015 20:07:41 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
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 <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Darren Hart <darren@dvhart.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Bhuvanesh_Surachari@mentor.com
Cc: Andy Lowe <Andy_Lowe@mentor.com>
Link: http://lkml.kernel.org/r/20151219200607.526665141@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 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);