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 <dhowells@redhat.com>
---

 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(&current->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(&current->cred_exec_mutex);

--

[ message continues ]

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
--

[ message continues ]

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


--

[ message continues ]

On Wed, 20 Aug 2008 18:21:59 +0400

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

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
--

[ message continues ]

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
<jmorris@namei.org>
--

[ message continues ]

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
--

[ message continues ]