| Subject: futex: Ensure the correct return value from futex_lock_pi() |
| From: Thomas Gleixner <tglx@linutronix.de> |
| Date: Wed Jan 20 16:00:24 2021 +0100 |
| |
| From: Thomas Gleixner <tglx@linutronix.de> |
| |
| commit 12bb3f7f1b03d5913b3f9d4236a488aa7774dfe9 upstream |
| |
| In case that futex_lock_pi() was aborted by a signal or a timeout and the |
| task returned without acquiring the rtmutex, but is the designated owner of |
| the futex due to a concurrent futex_unlock_pi() fixup_owner() is invoked to |
| establish consistent state. In that case it invokes fixup_pi_state_owner() |
| which in turn tries to acquire the rtmutex again. If that succeeds then it |
| does not propagate this success to fixup_owner() and futex_lock_pi() |
| returns -EINTR or -ETIMEOUT despite having the futex locked. |
| |
| Return success from fixup_pi_state_owner() in all cases where the current |
| task owns the rtmutex and therefore the futex and propagate it correctly |
| through fixup_owner(). Fixup the other callsite which does not expect a |
| positive return value. |
| |
| Fixes: c1e2f0eaf015 ("futex: Avoid violating the 10th rule of futex") |
| Signed-off-by: Thomas Gleixner <tglx@linutronix.de> |
| Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> |
| Cc: stable@vger.kernel.org |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| kernel/futex.c | 32 ++++++++++++++++---------------- |
| 1 file changed, 16 insertions(+), 16 deletions(-) |
| |
| --- a/kernel/futex.c |
| +++ b/kernel/futex.c |
| @@ -2506,8 +2506,8 @@ retry: |
| } |
| |
| if (__rt_mutex_futex_trylock(&pi_state->pi_mutex)) { |
| - /* We got the lock after all, nothing to fix. */ |
| - ret = 0; |
| + /* We got the lock. pi_state is correct. Tell caller. */ |
| + ret = 1; |
| goto out_unlock; |
| } |
| |
| @@ -2535,7 +2535,7 @@ retry: |
| * We raced against a concurrent self; things are |
| * already fixed up. Nothing to do. |
| */ |
| - ret = 0; |
| + ret = 1; |
| goto out_unlock; |
| } |
| newowner = argowner; |
| @@ -2581,7 +2581,7 @@ retry: |
| raw_spin_unlock(&newowner->pi_lock); |
| raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock); |
| |
| - return 0; |
| + return argowner == current; |
| |
| /* |
| * In order to reschedule or handle a page fault, we need to drop the |
| @@ -2623,7 +2623,7 @@ handle_err: |
| * Check if someone else fixed it for us: |
| */ |
| if (pi_state->owner != oldowner) { |
| - ret = 0; |
| + ret = argowner == current; |
| goto out_unlock; |
| } |
| |
| @@ -2656,8 +2656,6 @@ static long futex_wait_restart(struct re |
| */ |
| static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked) |
| { |
| - int ret = 0; |
| - |
| if (locked) { |
| /* |
| * Got the lock. We might not be the anticipated owner if we |
| @@ -2668,8 +2666,8 @@ static int fixup_owner(u32 __user *uaddr |
| * stable state, anything else needs more attention. |
| */ |
| if (q->pi_state->owner != current) |
| - ret = fixup_pi_state_owner(uaddr, q, current); |
| - goto out; |
| + return fixup_pi_state_owner(uaddr, q, current); |
| + return 1; |
| } |
| |
| /* |
| @@ -2680,10 +2678,8 @@ static int fixup_owner(u32 __user *uaddr |
| * Another speculative read; pi_state->owner == current is unstable |
| * but needs our attention. |
| */ |
| - if (q->pi_state->owner == current) { |
| - ret = fixup_pi_state_owner(uaddr, q, NULL); |
| - goto out; |
| - } |
| + if (q->pi_state->owner == current) |
| + return fixup_pi_state_owner(uaddr, q, NULL); |
| |
| /* |
| * Paranoia check. If we did not take the lock, then we should not be |
| @@ -2696,8 +2692,7 @@ static int fixup_owner(u32 __user *uaddr |
| q->pi_state->owner); |
| } |
| |
| -out: |
| - return ret ? ret : locked; |
| + return 0; |
| } |
| |
| /** |
| @@ -3406,7 +3401,7 @@ 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); |
| - if (ret && rt_mutex_owner(&q.pi_state->pi_mutex) == current) { |
| + if (ret < 0 && rt_mutex_owner(&q.pi_state->pi_mutex) == current) { |
| pi_state = q.pi_state; |
| get_pi_state(pi_state); |
| } |
| @@ -3416,6 +3411,11 @@ static int futex_wait_requeue_pi(u32 __u |
| */ |
| put_pi_state(q.pi_state); |
| spin_unlock(q.lock_ptr); |
| + /* |
| + * Adjust the return value. It's either -EFAULT or |
| + * success (1) but the caller expects 0 for success. |
| + */ |
| + ret = ret < 0 ? ret : 0; |
| } |
| } else { |
| struct rt_mutex *pi_mutex; |