[2557] | 1 | From cabef9fee397081ec3dfbde2955d4db675a96a4a Mon Sep 17 00:00:00 2001 |
---|
| 2 | From: Thomas Gleixner <tglx@linutronix.de> |
---|
| 3 | Date: Mon, 12 May 2014 20:45:34 +0000 |
---|
| 4 | Subject: [PATCH 1/1] futex: Add another early deadlock detection check |
---|
| 5 | |
---|
| 6 | commit 866293ee54227584ffcb4a42f69c1f365974ba7f upstream. |
---|
| 7 | |
---|
| 8 | Dave Jones trinity syscall fuzzer exposed an issue in the deadlock |
---|
| 9 | detection code of rtmutex: |
---|
| 10 | http://lkml.kernel.org/r/20140429151655.GA14277@redhat.com |
---|
| 11 | |
---|
| 12 | That underlying issue has been fixed with a patch to the rtmutex code, |
---|
| 13 | but the futex code must not call into rtmutex in that case because |
---|
| 14 | - it can detect that issue early |
---|
| 15 | - it avoids a different and more complex fixup for backing out |
---|
| 16 | |
---|
| 17 | If the user space variable got manipulated to 0x80000000 which means |
---|
| 18 | no lock holder, but the waiters bit set and an active pi_state in the |
---|
| 19 | kernel is found we can figure out the recursive locking issue by |
---|
| 20 | looking at the pi_state owner. If that is the current task, then we |
---|
| 21 | can safely return -EDEADLK. |
---|
| 22 | |
---|
| 23 | The check should have been added in commit 59fa62451 (futex: Handle |
---|
| 24 | futex_pi OWNER_DIED take over correctly) already, but I did not see |
---|
| 25 | the above issue caused by user space manipulation back then. |
---|
| 26 | |
---|
| 27 | Signed-off-by: Thomas Gleixner <tglx@linutronix.de> |
---|
| 28 | Cc: Dave Jones <davej@redhat.com> |
---|
| 29 | Cc: Linus Torvalds <torvalds@linux-foundation.org> |
---|
| 30 | Cc: Peter Zijlstra <peterz@infradead.org> |
---|
| 31 | Cc: Darren Hart <darren@dvhart.com> |
---|
| 32 | Cc: Davidlohr Bueso <davidlohr@hp.com> |
---|
| 33 | Cc: Steven Rostedt <rostedt@goodmis.org> |
---|
| 34 | Cc: Clark Williams <williams@redhat.com> |
---|
| 35 | Cc: Paul McKenney <paulmck@linux.vnet.ibm.com> |
---|
| 36 | Cc: Lai Jiangshan <laijs@cn.fujitsu.com> |
---|
| 37 | Cc: Roland McGrath <roland@hack.frob.com> |
---|
| 38 | Cc: Carlos ODonell <carlos@redhat.com> |
---|
| 39 | Cc: Jakub Jelinek <jakub@redhat.com> |
---|
| 40 | Cc: Michael Kerrisk <mtk.manpages@gmail.com> |
---|
| 41 | Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> |
---|
| 42 | Link: http://lkml.kernel.org/r/20140512201701.097349971@linutronix.de |
---|
| 43 | Signed-off-by: Thomas Gleixner <tglx@linutronix.de> |
---|
| 44 | Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
---|
| 45 | --- |
---|
| 46 | kernel/futex.c | 47 ++++++++++++++++++++++++++++++++++------------- |
---|
| 47 | 1 file changed, 34 insertions(+), 13 deletions(-) |
---|
| 48 | |
---|
| 49 | diff --git a/kernel/futex.c b/kernel/futex.c |
---|
| 50 | index 3bc18bf..66af37d 100644 |
---|
| 51 | --- a/kernel/futex.c |
---|
| 52 | +++ b/kernel/futex.c |
---|
| 53 | @@ -594,7 +594,8 @@ void exit_pi_state_list(struct task_struct *curr) |
---|
| 54 | |
---|
| 55 | static int |
---|
| 56 | lookup_pi_state(u32 uval, struct futex_hash_bucket *hb, |
---|
| 57 | - union futex_key *key, struct futex_pi_state **ps) |
---|
| 58 | + union futex_key *key, struct futex_pi_state **ps, |
---|
| 59 | + struct task_struct *task) |
---|
| 60 | { |
---|
| 61 | struct futex_pi_state *pi_state = NULL; |
---|
| 62 | struct futex_q *this, *next; |
---|
| 63 | @@ -638,6 +639,16 @@ lookup_pi_state(u32 uval, struct futex_hash_bucket *hb, |
---|
| 64 | return -EINVAL; |
---|
| 65 | } |
---|
| 66 | |
---|
| 67 | + /* |
---|
| 68 | + * Protect against a corrupted uval. If uval |
---|
| 69 | + * is 0x80000000 then pid is 0 and the waiter |
---|
| 70 | + * bit is set. So the deadlock check in the |
---|
| 71 | + * calling code has failed and we did not fall |
---|
| 72 | + * into the check above due to !pid. |
---|
| 73 | + */ |
---|
| 74 | + if (task && pi_state->owner == task) |
---|
| 75 | + return -EDEADLK; |
---|
| 76 | + |
---|
| 77 | atomic_inc(&pi_state->refcount); |
---|
| 78 | *ps = pi_state; |
---|
| 79 | |
---|
| 80 | @@ -787,7 +798,7 @@ retry: |
---|
| 81 | * We dont have the lock. Look up the PI state (or create it if |
---|
| 82 | * we are the first waiter): |
---|
| 83 | */ |
---|
| 84 | - ret = lookup_pi_state(uval, hb, key, ps); |
---|
| 85 | + ret = lookup_pi_state(uval, hb, key, ps, task); |
---|
| 86 | |
---|
| 87 | if (unlikely(ret)) { |
---|
| 88 | switch (ret) { |
---|
| 89 | @@ -1197,7 +1208,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key, |
---|
| 90 | * |
---|
| 91 | * Return: |
---|
| 92 | * 0 - failed to acquire the lock atomically; |
---|
| 93 | - * 1 - acquired the lock; |
---|
| 94 | + * >0 - acquired the lock, return value is vpid of the top_waiter |
---|
| 95 | * <0 - error |
---|
| 96 | */ |
---|
| 97 | static int futex_proxy_trylock_atomic(u32 __user *pifutex, |
---|
| 98 | @@ -1208,7 +1219,7 @@ static int futex_proxy_trylock_atomic(u32 __user *pifutex, |
---|
| 99 | { |
---|
| 100 | struct futex_q *top_waiter = NULL; |
---|
| 101 | u32 curval; |
---|
| 102 | - int ret; |
---|
| 103 | + int ret, vpid; |
---|
| 104 | |
---|
| 105 | if (get_futex_value_locked(&curval, pifutex)) |
---|
| 106 | return -EFAULT; |
---|
| 107 | @@ -1236,11 +1247,13 @@ static int futex_proxy_trylock_atomic(u32 __user *pifutex, |
---|
| 108 | * the contended case or if set_waiters is 1. The pi_state is returned |
---|
| 109 | * in ps in contended cases. |
---|
| 110 | */ |
---|
| 111 | + vpid = task_pid_vnr(top_waiter->task); |
---|
| 112 | ret = futex_lock_pi_atomic(pifutex, hb2, key2, ps, top_waiter->task, |
---|
| 113 | set_waiters); |
---|
| 114 | - if (ret == 1) |
---|
| 115 | + if (ret == 1) { |
---|
| 116 | requeue_pi_wake_futex(top_waiter, key2, hb2); |
---|
| 117 | - |
---|
| 118 | + return vpid; |
---|
| 119 | + } |
---|
| 120 | return ret; |
---|
| 121 | } |
---|
| 122 | |
---|
| 123 | @@ -1272,7 +1285,6 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags, |
---|
| 124 | struct futex_hash_bucket *hb1, *hb2; |
---|
| 125 | struct plist_head *head1; |
---|
| 126 | struct futex_q *this, *next; |
---|
| 127 | - u32 curval2; |
---|
| 128 | |
---|
| 129 | if (requeue_pi) { |
---|
| 130 | /* |
---|
| 131 | @@ -1358,16 +1370,25 @@ retry_private: |
---|
| 132 | * At this point the top_waiter has either taken uaddr2 or is |
---|
| 133 | * waiting on it. If the former, then the pi_state will not |
---|
| 134 | * exist yet, look it up one more time to ensure we have a |
---|
| 135 | - * reference to it. |
---|
| 136 | + * reference to it. If the lock was taken, ret contains the |
---|
| 137 | + * vpid of the top waiter task. |
---|
| 138 | */ |
---|
| 139 | - if (ret == 1) { |
---|
| 140 | + if (ret > 0) { |
---|
| 141 | WARN_ON(pi_state); |
---|
| 142 | drop_count++; |
---|
| 143 | task_count++; |
---|
| 144 | - ret = get_futex_value_locked(&curval2, uaddr2); |
---|
| 145 | - if (!ret) |
---|
| 146 | - ret = lookup_pi_state(curval2, hb2, &key2, |
---|
| 147 | - &pi_state); |
---|
| 148 | + /* |
---|
| 149 | + * If we acquired the lock, then the user |
---|
| 150 | + * space value of uaddr2 should be vpid. It |
---|
| 151 | + * cannot be changed by the top waiter as it |
---|
| 152 | + * is blocked on hb2 lock if it tries to do |
---|
| 153 | + * so. If something fiddled with it behind our |
---|
| 154 | + * back the pi state lookup might unearth |
---|
| 155 | + * it. So we rather use the known value than |
---|
| 156 | + * rereading and handing potential crap to |
---|
| 157 | + * lookup_pi_state. |
---|
| 158 | + */ |
---|
| 159 | + ret = lookup_pi_state(ret, hb2, &key2, &pi_state, NULL); |
---|
| 160 | } |
---|
| 161 | |
---|
| 162 | switch (ret) { |
---|
| 163 | -- |
---|
| 164 | 1.7.10.4 |
---|
| 165 | |
---|