Q: PTRACE_ATTACH && -EINTR

Q: PTRACE_ATTACH && -EINTR

[Oleg Nesterov]

I just realized that ->cred_exec_mutex added a user-visible change
which may confuse user-space.

	ptrace_attach:

		retval = mutex_lock_interruptible(&task->cred_exec_mutex);
		if (retval  < 0)
			goto out;

This doesn't look good, we return -EINTR. Suppose that strace tries to
attach to all sub-threads and ptrace(PTRACE_ATTACH) returns -EINTR just
because the already traced thread sends SIGCHLD. Or tracer's sub-thread
does recalc_sigpending_and_wake().

I think we should at least do

		retval = -ERESTARTSYS;
		if (mutex_lock_interruptible(&task->cred_exec_mutex))
			goto out;

Or even -ERESTARTNOINTR ? Or just mutex_lock() ?

Or ignore this problem since nobody complained?

Oleg.

Re: Q: PTRACE_ATTACH && -EINTR

[Roland McGrath]

> Or even -ERESTARTNOINTR ? Or just mutex_lock() ?

-ERESTARTNOINTR is right.  

There is nothing wrong with making it interruptible, and that might help
something or other overall, or even be important to avoid a deadlock or
something in some strange situation.  But since the call could never return
-EINTR before, we can't make it start now.

> Or ignore this problem since nobody complained?

There has barely been time for anyone to do something strange enough to hit
it, and they would probably not have realized what was going on even if it
did hit.  We know we broke the ABI contract, we have to fix it.

Note that every use of mutex_lock_interruptible and also down_interruptible
can return -EINTR.  This means these really should never be used in the way
where their return value is returned directly from some system call.  Every
user-visible call that gets interrupted needs to return some -ERESTART*
code and never -EINTR directly.


Thanks,
Roland

Re: Q: PTRACE_ATTACH && -EINTR

[Oleg Nesterov]

On 06/08, Roland McGrath wrote:
>
> > Or even -ERESTARTNOINTR ? Or just mutex_lock() ?
>
> -ERESTARTNOINTR is right.
>
> There is nothing wrong with making it interruptible, and that might help
> something or other overall, or even be important to avoid a deadlock or
> something in some strange situation.

Agreed.

> But since the call could never return
> -EINTR before, we can't make it start now.

Yes. -EINTR just wrong.

> > Or ignore this problem since nobody complained?
>
> There has barely been time for anyone to do something strange enough to hit
> it, and they would probably not have realized what was going on even if it
> did hit.  We know we broke the ABI contract, we have to fix it.
>
> Note that every use of mutex_lock_interruptible and also down_interruptible
> can return -EINTR.  This means these really should never be used in the way
> where their return value is returned directly from some system call.  Every
> user-visible call that gets interrupted needs to return some -ERESTART*
> code and never -EINTR directly.

Sure. And we have other users of mutex_lock_interruptible() which deserve
a fix.

As for ->cred_exec_mutex, I think do_execve() needs a fix as well.

It was renamed in -next. Should I send these fixes now for 2.6.20, or we can
wait for 2.6.31 ?

Oleg.

Re: Q: PTRACE_ATTACH && -EINTR

[Roland McGrath]

> It was renamed in -next. Should I send these fixes now for 2.6.20, or we can
								 30
> wait for 2.6.31 ?

IMHO they should go in ASAP since we know this is a regression just
introduced in 2.6.29.  To me, the fact that nobody has noticed yet
makes it more important not to delay--this new problem is so obscure
that whoever is affected by it is likely to waste a lot of time figuring
out what has started happening deep down in a huge pile of userland code.


Thanks,
Roland

Re: Q: PTRACE_ATTACH && -EINTR

[Oleg Nesterov]

On 06/08, Roland McGrath wrote:
>
> > It was renamed in -next. Should I send these fixes now for 2.6.20, or we can
> 								 30
> > wait for 2.6.31 ?
>
> IMHO they should go in ASAP since we know this is a regression just
> introduced in 2.6.29.  To me, the fact that nobody has noticed yet
> makes it more important not to delay--this new problem is so obscure
> that whoever is affected by it is likely to waste a lot of time figuring
> out what has started happening deep down in a huge pile of userland code.

2,6,30 is already released.

So, we need the trivial patch below, and perhaps a similar fix in
proc_pid_attr_write().

But giwen that ->cred_exec_mutex was renamed, I do not know where to send
this patch. This rename conflicts with ptrace changes in -mm, and the patch
below will add more confusion.

I'll wait until rename or -mm bits will be applied, then send this patch.
Fortunately the problem is not serious, ->cred_exec_mutex should be mostly
free.

Oleg.

--- T/fs/exec.c~	2009-05-24 21:46:20.000000000 +0200
+++ T/fs/exec.c	2009-06-10 14:58:27.000000000 +0200
@@ -1296,8 +1296,8 @@ int do_execve(char * filename,
 	if (!bprm)
 		goto out_files;
 
-	retval = mutex_lock_interruptible(&current->cred_exec_mutex);
-	if (retval < 0)
+	retval = -ERESTARTNOINTR;
+	if (mutex_lock_interruptible(&current->cred_exec_mutex))
 		goto out_free;
 	current->in_execve = 1;
 
--- T/kernel/ptrace.c~	2009-06-10 14:46:57.000000000 +0200
+++ T/kernel/ptrace.c	2009-06-10 14:54:24.000000000 +0200
@@ -189,8 +189,8 @@ 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(&task->cred_exec_mutex);
-	if (retval < 0)
+	retval = -ERESTARTNOINTR;
+	if (mutex_lock_interruptible(&task->cred_exec_mutex))
 		goto out;
 
 	task_lock(task);

Re: Q: PTRACE_ATTACH && -EINTR

[Roland McGrath]

> I'll wait until rename or -mm bits will be applied, then send this patch.

Or you could send it to stable@ for 2.6.29/2.6.30 and let akpm figure out
the order for merging with the rest.

> Fortunately the problem is not serious, ->cred_exec_mutex should be mostly
> free.

Agreed.


Thanks,
Roland

[PATCH] cred_guard_mutex: do not return -EINTR to user-space

[Oleg Nesterov]

do_execve() and ptrace_attach() return -EINTR if
mutex_lock_interruptible(->cred_guard_mutex) fails.

This is not right, change the code to return ERESTARTNOINTR.

Perhaps we should also change proc_pid_attr_write().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 fs/exec.c       |    4 ++--
 fs/compat.c     |    4 ++--
 kernel/ptrace.c |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1277,8 +1277,8 @@ int do_execve(char * filename,
 	if (!bprm)
 		goto out_files;
 
-	retval = mutex_lock_interruptible(&current->cred_guard_mutex);
-	if (retval < 0)
+	retval = -ERESTARTNOINTR;
+	if (mutex_lock_interruptible(&current->cred_guard_mutex))
 		goto out_free;
 	current->in_execve = 1;
 
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1486,8 +1486,8 @@ int compat_do_execve(char * filename,
 	if (!bprm)
 		goto out_files;
 
-	retval = mutex_lock_interruptible(&current->cred_guard_mutex);
-	if (retval < 0)
+	retval = -ERESTARTNOINTR;
+	if (mutex_lock_interruptible(&current->cred_guard_mutex))
 		goto out_free;
 	current->in_execve = 1;
 
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -181,8 +181,8 @@ int ptrace_attach(struct task_struct *ta
 	 * interference; SUID, SGID and LSM creds get determined differently
 	 * under ptrace.
 	 */
-	retval = mutex_lock_interruptible(&task->cred_guard_mutex);
-	if (retval < 0)
+	retval = -ERESTARTNOINTR;
+	if (mutex_lock_interruptible(&task->cred_guard_mutex))
 		goto out;
 
 	task_lock(task);

Re: [PATCH] cred_guard_mutex: do not return -EINTR to user-space

[Roland McGrath]

Acked-by: Roland McGrath <roland@redhat.com>