Commit 4b105cbbaf7c06e01c27391957dc3c446328d087
Committed by
Linus Torvalds
1 parent
f2f0b00ad6
Exists in
master
and in
7 other branches
ptrace: do not use task_lock() for attach
Remove the "Nasty, nasty" lock dance in ptrace_attach()/ptrace_traceme() - from now task_lock() has nothing to do with ptrace at all. With the recent changes nobody uses task_lock() to serialize with ptrace, but in fact it was never needed and it was never used consistently. However ptrace_attach() calls __ptrace_may_access() and needs task_lock() to pin task->mm for get_dumpable(). But we can call __ptrace_may_access() before we take tasklist_lock, ->cred_exec_mutex protects us against do_execve() path which can change creds and MMF_DUMP* flags. (ugly, but we can't use ptrace_may_access() because it hides the error code, so we have to take task_lock() and use __ptrace_may_access()). NOTE: this change assumes that LSM hooks, security_ptrace_may_access() and security_ptrace_traceme(), can be called without task_lock() held. Signed-off-by: Oleg Nesterov <oleg@redhat.com> Cc: Chris Wright <chrisw@sous-sol.org> Acked-by: Roland McGrath <roland@redhat.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Showing 1 changed file with 13 additions and 46 deletions Side-by-side Diff
kernel/ptrace.c
... | ... | @@ -167,7 +167,6 @@ |
167 | 167 | int ptrace_attach(struct task_struct *task) |
168 | 168 | { |
169 | 169 | int retval; |
170 | - unsigned long flags; | |
171 | 170 | |
172 | 171 | audit_ptrace(task); |
173 | 172 | |
174 | 173 | |
175 | 174 | |
176 | 175 | |
177 | 176 | |
178 | 177 | |
179 | 178 | |
... | ... | @@ -185,34 +184,19 @@ |
185 | 184 | retval = mutex_lock_interruptible(&task->cred_guard_mutex); |
186 | 185 | if (retval < 0) |
187 | 186 | goto out; |
188 | -repeat: | |
189 | - /* | |
190 | - * Nasty, nasty. | |
191 | - * | |
192 | - * We want to hold both the task-lock and the | |
193 | - * tasklist_lock for writing at the same time. | |
194 | - * But that's against the rules (tasklist_lock | |
195 | - * is taken for reading by interrupts on other | |
196 | - * cpu's that may have task_lock). | |
197 | - */ | |
198 | - task_lock(task); | |
199 | - if (!write_trylock_irqsave(&tasklist_lock, flags)) { | |
200 | - task_unlock(task); | |
201 | - do { | |
202 | - cpu_relax(); | |
203 | - } while (!write_can_lock(&tasklist_lock)); | |
204 | - goto repeat; | |
205 | - } | |
206 | 187 | |
188 | + task_lock(task); | |
207 | 189 | retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH); |
190 | + task_unlock(task); | |
208 | 191 | if (retval) |
209 | - goto bad; | |
192 | + goto unlock_creds; | |
210 | 193 | |
194 | + write_lock_irq(&tasklist_lock); | |
211 | 195 | retval = -EPERM; |
212 | 196 | if (unlikely(task->exit_state)) |
213 | - goto bad; | |
197 | + goto unlock_tasklist; | |
214 | 198 | if (task->ptrace) |
215 | - goto bad; | |
199 | + goto unlock_tasklist; | |
216 | 200 | |
217 | 201 | task->ptrace = PT_PTRACED; |
218 | 202 | if (capable(CAP_SYS_PTRACE)) |
... | ... | @@ -222,9 +206,9 @@ |
222 | 206 | send_sig_info(SIGSTOP, SEND_SIG_FORCED, task); |
223 | 207 | |
224 | 208 | retval = 0; |
225 | -bad: | |
226 | - write_unlock_irqrestore(&tasklist_lock, flags); | |
227 | - task_unlock(task); | |
209 | +unlock_tasklist: | |
210 | + write_unlock_irq(&tasklist_lock); | |
211 | +unlock_creds: | |
228 | 212 | mutex_unlock(&task->cred_guard_mutex); |
229 | 213 | out: |
230 | 214 | return retval; |
231 | 215 | |
232 | 216 | |
... | ... | @@ -240,26 +224,10 @@ |
240 | 224 | { |
241 | 225 | int ret = -EPERM; |
242 | 226 | |
243 | - /* | |
244 | - * Are we already being traced? | |
245 | - */ | |
246 | -repeat: | |
247 | - task_lock(current); | |
227 | + write_lock_irq(&tasklist_lock); | |
228 | + /* Are we already being traced? */ | |
248 | 229 | if (!current->ptrace) { |
249 | - /* | |
250 | - * See ptrace_attach() comments about the locking here. | |
251 | - */ | |
252 | - unsigned long flags; | |
253 | - if (!write_trylock_irqsave(&tasklist_lock, flags)) { | |
254 | - task_unlock(current); | |
255 | - do { | |
256 | - cpu_relax(); | |
257 | - } while (!write_can_lock(&tasklist_lock)); | |
258 | - goto repeat; | |
259 | - } | |
260 | - | |
261 | 230 | ret = security_ptrace_traceme(current->parent); |
262 | - | |
263 | 231 | /* |
264 | 232 | * Check PF_EXITING to ensure ->real_parent has not passed |
265 | 233 | * exit_ptrace(). Otherwise we don't report the error but |
266 | 234 | |
... | ... | @@ -269,10 +237,9 @@ |
269 | 237 | current->ptrace = PT_PTRACED; |
270 | 238 | __ptrace_link(current, current->real_parent); |
271 | 239 | } |
272 | - | |
273 | - write_unlock_irqrestore(&tasklist_lock, flags); | |
274 | 240 | } |
275 | - task_unlock(current); | |
241 | + write_unlock_irq(&tasklist_lock); | |
242 | + | |
276 | 243 | return ret; |
277 | 244 | } |
278 | 245 |