Further fix [compat_]do_execve() error handling. free_bprm() will release the
cred_exec_mutex, but only if bprm->cred is not NULL.
Signed-off-by: David Howells <[email protected]>
---
fs/compat.c | 3 ++-
fs/exec.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/compat.c b/fs/compat.c
index af24b8a..918f0f2 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1373,7 +1373,7 @@ int compat_do_execve(char * filename,
file = open_exec(filename);
retval = PTR_ERR(file);
if (IS_ERR(file))
- goto out_unlock;
+ goto out_free;
sched_exec();
@@ -1427,6 +1427,7 @@ out_file:
allow_write_access(bprm->file);
fput(bprm->file);
}
+ goto out_free;
out_unlock:
mutex_unlock(¤t->cred_exec_mutex);
diff --git a/fs/exec.c b/fs/exec.c
index 4b31a72..7b71679 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1319,7 +1319,7 @@ int do_execve(char * filename,
file = open_exec(filename);
retval = PTR_ERR(file);
if (IS_ERR(file))
- goto out_unlock;
+ goto out_free;
sched_exec();
@@ -1376,6 +1376,7 @@ out_file:
allow_write_access(bprm->file);
fput(bprm->file);
}
+ goto out_free;
out_unlock:
mutex_unlock(¤t->cred_exec_mutex);
2008/8/20 David Howells <[email protected]>:
> Further fix [compat_]do_execve() error handling. free_bprm() will release the
> cred_exec_mutex, but only if bprm->cred is not NULL.
>
> Signed-off-by: David Howells <[email protected]>
David, I applied this patch and got the following.
Is it a different problem?
I think it is. If yes I will create another topic.
[ 91.266507] [ INFO: possible recursive locking detected ]
[ 91.266862] 2.6.27-rc3-next-20080820-dirty #3
[ 91.267170] ---------------------------------------------
[ 91.267522] sshd/1455 is trying to acquire lock:
[ 91.267840] (&tty->termios_mutex){--..}, at: [<00000000005b8ca0>]
tty_do_resize+0x44/0x128
[ 91.268405]
[ 91.268411] but task is already holding lock:
[ 91.268885] (&tty->termios_mutex){--..}, at: [<00000000005b8c74>]
tty_do_resize+0x18/0x128
[ 91.269436]
[ 91.269442] other info that might help us debug this:
[ 91.269944] 1 lock held by sshd/1455:
[ 91.270219] #0: (&tty->termios_mutex){--..}, at:
[<00000000005b8c74>] tty_do_resize+0x18/0x128
[ 91.270812]
[ 91.270818] stack backtrace:
[ 91.271229] Call Trace:
[ 91.271474] [000000000047757c] __lock_acquire+0xfcc/0x1900
[ 91.271839] [0000000000477f0c] lock_acquire+0x5c/0x74
[ 91.272198] [00000000006af2a8] mutex_lock_nested+0xf0/0x310
[ 91.272565] [00000000005b8ca0] tty_do_resize+0x44/0x128
[ 91.272916] [00000000005ba3cc] tty_ioctl+0x360/0x99c
[ 91.273266] [00000000004c0050] vfs_ioctl+0x2c/0x94
[ 91.273600] [00000000004c03b4] do_vfs_ioctl+0x2fc/0x31c
[ 91.273953] [00000000004c0408] sys_ioctl+0x34/0x5c
[ 91.274296] [00000000004e98f0] do_ioctl32_pointer+0x18/0x2c
[ 91.274666] [00000000004ebb04] compat_sys_ioctl+0x3b8/0x40c
[ 91.275043] [0000000000406154] linux_sparc_syscall32+0x34/0x40
Alexander Beregalov <[email protected]> wrote:
> David, I applied this patch and got the following.
> Is it a different problem?
> I think it is. If yes I will create another topic.
It doesn't look like it's anything to do with my patches. I'd guess the TTY
patches from Alan Cox are the culprit.
David
On Wed, 20 Aug 2008 18:21:59 +0400
"Alexander Beregalov" <[email protected]> wrote:
> 2008/8/20 David Howells <[email protected]>:
> > Further fix [compat_]do_execve() error handling. free_bprm() will release the
> > cred_exec_mutex, but only if bprm->cred is not NULL.
> >
> > Signed-off-by: David Howells <[email protected]>
>
> David, I applied this patch and got the following.
> Is it a different problem?
> I think it is. If yes I will create another topic.
>
> [ 91.266507] [ INFO: possible recursive locking detected ]
> [ 91.266862] 2.6.27-rc3-next-20080820-dirty #3
Looks like a false trip of the lock debugging code. I've seen the same
and having dumped the values in tty_do_resize plus the entry/exit paths
it seems to be bogus.
> [ 91.267170] ---------------------------------------------
> [ 91.267522] sshd/1455 is trying to acquire lock:
> [ 91.267840] (&tty->termios_mutex){--..}, at: [<00000000005b8ca0>]
> tty_do_resize+0x44/0x128
> [ 91.268405]
> [ 91.268411] but task is already holding lock:
> [ 91.268885] (&tty->termios_mutex){--..}, at: [<00000000005b8c74>]
> tty_do_resize+0x18/0x128
Note that the lock is only acquired in one place in this code and that
the second lock is only take if the two locks differ. Also the second
lock take is not &tty->termios_mutex at all but real_tty.
So I'd say either broken compiler or broken lock debug tools but hey I
could be wrong ;)
Alan
On Wed, 20 Aug 2008, David Howells wrote:
> Further fix [compat_]do_execve() error handling. free_bprm() will release the
> cred_exec_mutex, but only if bprm->cred is not NULL.
This seems quite ugly and error-prone, with a mutex_unlock() being called
from a helper function rather than the body of the function which locked
it.
How about moving the mutex_unlock() out of free_bprm() and into the
calling code ?
- James
--
James Morris
<[email protected]>
James Morris <[email protected]> wrote:
> How about moving the mutex_unlock() out of free_bprm() and into the
> calling code ?
Okay, I've sent you a patch to do this. Note that it only affects the error
handling case. In the case of a successful execution, install_exec_creds()
will release the mutex when it is safe to do so. This then permits
PTRACE_ATTACH to take place from that point. I could shift the unlock so that
it always happens in [compat_]do_execve() - do you think it's worth it? It
would mean that ptrace wouldn't be able to attach to a process that's still
under construction by the binfmt, which is probably reasonable.
David