* [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment
@ 2009-04-23 21:22 Oleg Nesterov
2009-04-23 23:27 ` Roland McGrath
0 siblings, 1 reply; 12+ messages in thread
From: Oleg Nesterov @ 2009-04-23 21:22 UTC (permalink / raw)
To: Andrew Morton; +Cc: Roland McGrath, linux-kernel
tracehook_unsafe_exec() doesn't need task_lock(), remove the old comment.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
--- PTRACE/include/linux/tracehook.h~ 2009-04-06 00:03:42.000000000 +0200
+++ PTRACE/include/linux/tracehook.h 2009-04-23 23:20:20.000000000 +0200
@@ -142,8 +142,6 @@ static inline void tracehook_report_sysc
* @task: current task doing exec
*
* Return %LSM_UNSAFE_* bits applied to an exec because of tracing.
- *
- * Called with task_lock() held on @task.
*/
static inline int tracehook_unsafe_exec(struct task_struct *task)
{
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment
2009-04-23 21:22 [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment Oleg Nesterov
@ 2009-04-23 23:27 ` Roland McGrath
2009-04-24 16:39 ` ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment) Oleg Nesterov
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Roland McGrath @ 2009-04-23 23:27 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, linux-kernel
> tracehook_unsafe_exec() doesn't need task_lock(), remove the old comment.
Please make it instead say that cred_exec_mutex is held by the caller
through the exec. That locking takes the place of what task_lock() was
doing before (e.g. interlock with PTRACE_ATTACH).
Thanks,
Roland
^ permalink raw reply [flat|nested] 12+ messages in thread
* ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment)
2009-04-23 23:27 ` Roland McGrath
@ 2009-04-24 16:39 ` Oleg Nesterov
2009-04-24 19:38 ` Roland McGrath
2009-04-25 10:54 ` David Howells
2009-04-27 20:05 ` [PATCH] ptrace-tracehook_unsafe_exec-remove-the-stale-comment-fix Oleg Nesterov
2 siblings, 1 reply; 12+ messages in thread
From: Oleg Nesterov @ 2009-04-24 16:39 UTC (permalink / raw)
To: Roland McGrath
Cc: Andrew Morton, linux-kernel, David Howells, James Morris, Hugh Dickins
(change subject, add more CCs)
On 04/23, Roland McGrath wrote:
>
> > tracehook_unsafe_exec() doesn't need task_lock(), remove the old comment.
>
> Please make it instead say that cred_exec_mutex is held by the caller
> through the exec.
Yes. Except it looks like ->cred_exec_mutex is never used in fact.
ptrace_attach() takes current->cred_exec_mutex ? iow, ptrace and exec
use different locks ?
Oleg.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment)
2009-04-24 16:39 ` ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment) Oleg Nesterov
@ 2009-04-24 19:38 ` Roland McGrath
0 siblings, 0 replies; 12+ messages in thread
From: Roland McGrath @ 2009-04-24 19:38 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrew Morton, linux-kernel, David Howells, James Morris, Hugh Dickins
> (change subject, add more CCs)
>
> On 04/23, Roland McGrath wrote:
> >
> > > tracehook_unsafe_exec() doesn't need task_lock(), remove the old comment.
> >
> > Please make it instead say that cred_exec_mutex is held by the caller
> > through the exec.
>
> Yes. Except it looks like ->cred_exec_mutex is never used in fact.
>
> ptrace_attach() takes current->cred_exec_mutex ? iow, ptrace and exec
> use different locks ?
Good point! That sure looks broken to me. David?
Either that should be task->cred_exec_mutex, or Oleg and I
are completely confused about what cred_exec_mutex is meant for.
Thanks,
Roland
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment)
2009-04-23 23:27 ` Roland McGrath
2009-04-24 16:39 ` ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment) Oleg Nesterov
@ 2009-04-25 10:54 ` David Howells
2009-04-25 17:25 ` Oleg Nesterov
2009-04-25 19:06 ` David Howells
2009-04-27 20:05 ` [PATCH] ptrace-tracehook_unsafe_exec-remove-the-stale-comment-fix Oleg Nesterov
2 siblings, 2 replies; 12+ messages in thread
From: David Howells @ 2009-04-25 10:54 UTC (permalink / raw)
To: Oleg Nesterov
Cc: dhowells, Roland McGrath, Andrew Morton, linux-kernel,
James Morris, Hugh Dickins
Oleg Nesterov <oleg@redhat.com> wrote:
> Yes. Except it looks like ->cred_exec_mutex is never used in fact.
I must to be missing something... I see that:
int ptrace_attach(struct task_struct *task)
{
...
/* Protect exec's credential calculations against our interference;
* SUID, SGID and LSM creds get determined differently under ptrace.
*/
retval = mutex_lock_interruptible(¤t->cred_exec_mutex);
...
}
And:
int do_execve(...)
{
...
retval = mutex_lock_interruptible(¤t->cred_exec_mutex);
if (retval < 0)
goto out_free;
...
}
So how is it not used?
David
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment)
2009-04-25 10:54 ` David Howells
@ 2009-04-25 17:25 ` Oleg Nesterov
2009-04-25 19:06 ` David Howells
1 sibling, 0 replies; 12+ messages in thread
From: Oleg Nesterov @ 2009-04-25 17:25 UTC (permalink / raw)
To: David Howells
Cc: Roland McGrath, Andrew Morton, linux-kernel, James Morris, Hugh Dickins
On 04/25, David Howells wrote:
>
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > Yes. Except it looks like ->cred_exec_mutex is never used in fact.
>
> I must to be missing something... I see that:
>
> int ptrace_attach(struct task_struct *task)
> {
> ...
> /* Protect exec's credential calculations against our interference;
> * SUID, SGID and LSM creds get determined differently under ptrace.
> */
> retval = mutex_lock_interruptible(¤t->cred_exec_mutex);
> ...
> }
>
> And:
>
> int do_execve(...)
> {
> ...
> retval = mutex_lock_interruptible(¤t->cred_exec_mutex);
> if (retval < 0)
> goto out_free;
> ...
> }
Sorry David, I was very unclear.
These 2 current's are different tasks, and hence we take to unrelated locks.
We can never block taking current->cred_exec_mutex because nobody else
touches this mutex, we always use current. This means this lock is "nop".
Unless I missed something, ptrace_attach() should take task->cred_exec_mutex.
Oleg.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment)
2009-04-25 10:54 ` David Howells
2009-04-25 17:25 ` Oleg Nesterov
@ 2009-04-25 19:06 ` David Howells
2009-04-26 23:41 ` [PATCH] ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex Oleg Nesterov
2009-04-27 8:47 ` David Howells
1 sibling, 2 replies; 12+ messages in thread
From: David Howells @ 2009-04-25 19:06 UTC (permalink / raw)
To: Oleg Nesterov
Cc: dhowells, Roland McGrath, Andrew Morton, linux-kernel,
James Morris, Hugh Dickins
Oleg Nesterov <oleg@redhat.com> wrote:
> Unless I missed something, ptrace_attach() should take task->cred_exec_mutex.
Bah! Yes.
David
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex
2009-04-25 19:06 ` David Howells
@ 2009-04-26 23:41 ` Oleg Nesterov
2009-04-27 2:11 ` Roland McGrath
2009-04-27 8:47 ` David Howells
1 sibling, 1 reply; 12+ messages in thread
From: Oleg Nesterov @ 2009-04-26 23:41 UTC (permalink / raw)
To: David Howells, Andrew Morton
Cc: Roland McGrath, linux-kernel, James Morris, Hugh Dickins, stable
ptrace_attach() needs task->cred_exec_mutex, not current->cred_exec_mutex.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
--- PTRACE/kernel/ptrace.c~ 2009-04-15 12:53:58.000000000 +0200
+++ PTRACE/kernel/ptrace.c 2009-04-27 01:22:51.000000000 +0200
@@ -188,7 +188,7 @@ int ptrace_attach(struct task_struct *ta
/* Protect exec's credential calculations against our interference;
* SUID, SGID and LSM creds get determined differently under ptrace.
*/
- retval = mutex_lock_interruptible(¤t->cred_exec_mutex);
+ retval = mutex_lock_interruptible(&task->cred_exec_mutex);
if (retval < 0)
goto out;
@@ -232,7 +232,7 @@ repeat:
bad:
write_unlock_irqrestore(&tasklist_lock, flags);
task_unlock(task);
- mutex_unlock(¤t->cred_exec_mutex);
+ mutex_unlock(&task->cred_exec_mutex);
out:
return retval;
}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex
2009-04-26 23:41 ` [PATCH] ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex Oleg Nesterov
@ 2009-04-27 2:11 ` Roland McGrath
0 siblings, 0 replies; 12+ messages in thread
From: Roland McGrath @ 2009-04-27 2:11 UTC (permalink / raw)
To: Oleg Nesterov
Cc: David Howells, Andrew Morton, linux-kernel, James Morris,
Hugh Dickins, stable
Acked-by: Roland McGrath <roland@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex
2009-04-25 19:06 ` David Howells
2009-04-26 23:41 ` [PATCH] ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex Oleg Nesterov
@ 2009-04-27 8:47 ` David Howells
2009-04-27 10:52 ` [GIT] " James Morris
1 sibling, 1 reply; 12+ messages in thread
From: David Howells @ 2009-04-27 8:47 UTC (permalink / raw)
To: Oleg Nesterov
Cc: dhowells, Andrew Morton, Roland McGrath, linux-kernel,
James Morris, Hugh Dickins, stable
Oleg Nesterov <oleg@redhat.com> wrote:
> ptrace_attach() needs task->cred_exec_mutex, not current->cred_exec_mutex.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: David Howells <dhowells@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [GIT] ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex
2009-04-27 8:47 ` David Howells
@ 2009-04-27 10:52 ` James Morris
0 siblings, 0 replies; 12+ messages in thread
From: James Morris @ 2009-04-27 10:52 UTC (permalink / raw)
To: Linus Torvalds
Cc: Oleg Nesterov, Andrew Morton, Roland McGrath, linux-kernel,
Hugh Dickins, stable, David Howells
Here's the patch ready to pull for 2.6.30.
The following changes since commit ce8a7424d23a36f043d0de8484f888971c831119:
Tim Abbott (1):
sparc: convert to use __HEAD and HEAD_TEXT macros.
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6 for-linus
Oleg Nesterov (1):
ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex
kernel/ptrace.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] ptrace-tracehook_unsafe_exec-remove-the-stale-comment-fix
2009-04-23 23:27 ` Roland McGrath
2009-04-24 16:39 ` ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment) Oleg Nesterov
2009-04-25 10:54 ` David Howells
@ 2009-04-27 20:05 ` Oleg Nesterov
2 siblings, 0 replies; 12+ messages in thread
From: Oleg Nesterov @ 2009-04-27 20:05 UTC (permalink / raw)
To: Roland McGrath; +Cc: Andrew Morton, linux-kernel
(on top of ptrace-tracehook_unsafe_exec-remove-the-stale-comment.patch)
On 04/23, Roland McGrath wrote:
>
> > tracehook_unsafe_exec() doesn't need task_lock(), remove the old comment.
>
> Please make it instead say that cred_exec_mutex is held by the caller
> through the exec.
Thanks.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
--- PTRACE/include/linux/tracehook.h~COMMENT_FIX 2009-04-23 23:20:20.000000000 +0200
+++ PTRACE/include/linux/tracehook.h 2009-04-27 21:59:54.000000000 +0200
@@ -142,6 +142,8 @@ static inline void tracehook_report_sysc
* @task: current task doing exec
*
* Return %LSM_UNSAFE_* bits applied to an exec because of tracing.
+ *
+ * task->cred_exec_mutex is held by the caller through the do_execve().
*/
static inline int tracehook_unsafe_exec(struct task_struct *task)
{
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2009-04-27 20:10 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-04-23 21:22 [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment Oleg Nesterov
2009-04-23 23:27 ` Roland McGrath
2009-04-24 16:39 ` ptrace && cred_exec_mutex (Was: [PATCH] ptrace: tracehook_unsafe_exec: remove the stale comment) Oleg Nesterov
2009-04-24 19:38 ` Roland McGrath
2009-04-25 10:54 ` David Howells
2009-04-25 17:25 ` Oleg Nesterov
2009-04-25 19:06 ` David Howells
2009-04-26 23:41 ` [PATCH] ptrace: ptrace_attach: fix the usage of ->cred_exec_mutex Oleg Nesterov
2009-04-27 2:11 ` Roland McGrath
2009-04-27 8:47 ` David Howells
2009-04-27 10:52 ` [GIT] " James Morris
2009-04-27 20:05 ` [PATCH] ptrace-tracehook_unsafe_exec-remove-the-stale-comment-fix Oleg Nesterov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.