[PATCH] CRED: Further fix execve error handling

[PATCH] CRED: Further fix execve error handling

[David Howells]

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

Re: [PATCH] CRED: Further fix execve error handling

[Alexander Beregalov]

2008/8/20 David Howells <dhowells@redhat.com>:
> 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>

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

Re: [PATCH] CRED: Further fix execve error handling

[David Howells]

Alexander Beregalov <a.beregalov@gmail.com> 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

Re: [PATCH] CRED: Further fix execve error handling

[Alan Cox]

On Wed, 20 Aug 2008 18:21:59 +0400
"Alexander Beregalov" <a.beregalov@gmail.com> wrote:

> 2008/8/20 David Howells <dhowells@redhat.com>:
> > 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>
> 
> 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

Re: [PATCH] CRED: Further fix execve error handling

[James Morris]

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

Re: [PATCH] CRED: Further fix execve error handling

[David Howells]

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