Messages in this thread | | | From | (Eric W. Biederman) | Date | Wed, 22 Feb 2017 09:20:15 +1300 | Subject | Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held |
| |
Oleg Nesterov <oleg@redhat.com> writes:
> On 02/21, Eric W. Biederman wrote: >> >> Today cred_guard_mutex is part of making exec appear to be an atomic >> operation to ptrace and and proc. To make exec appear to be atomic >> we do need to take the mutex at the beginning and release it at the end >> of exec. >> >> The semantics of exec appear atomic to ptrace_attach and to proc readers >> are necessary to ensure we use the proper process credentials in the >> event of a suid exec. > > This is clear. My point is that imo a) it is over-used in fs/proc and b) > the scope of this mutex if execve is too huge. I see absolutely no reason > to do copy_strings() with this mutex held, for example. And note that > copy_strings() can use a lot of memory/time, it can trigger oom,swapping, > etc.
I agree that we can do things like copy_strings that don't change the data structures and of the task without before taking the cred_guard_mutex.
> But let me repeat, this is a bit off-topic right now, this patch doesn't > change anything in this respect, afaics. > > >> I believe making cred_guard_mutex per task is an option. Reducing the >> scope of cred_guard_mutex concerns me. There appear to be some fields >> like sighand that we currently expose in proc > > please see another email, collect_sigign_sigcatch() is called without this > mutex.
I agree that it is called without the mutex. It is not clear to me that is the correct behavior. It violates the fundamental property that exec of a setuid executable should be an atomic operation. I don't know how much we care but it disturbs me that we can read something of a processes signal handling state with the wrong credentials.
Adopting an implementation where we can never fix this apparent bug really really disturbs me.
>> Do you know if we can make cred_guard_mutex a per-task lock again? > > I think we can, but this needs some (afaics simple) changes too. > > But for what? Note that the problem fixed by this series won't go away > if we do this.
I believe it will if the other waiters use mutex_lock_killable.
> So what do you think about this series?
I like the second patch. That seems clean and reasonable.
I really don't like the first patch. It makes an information leak part a required detail of the implementation and as such possibly something we can never change. It attempts to paint a picture for a full fix in the future that appears to result in an incorrect kernel. That really bugs me.
I suspect that a good fix that respects that proc and ptrace_attach need to exclude the setuid exec case for semantic reasons would have a similar complexity.
I think a mutex doing the job that cred_guard_mutex is doing especially when we have multiple readers and a single writer is the wrong locking primative. A reader-writer lock or something even cheaper would probably be much better.
I think fixing the deadlock is important.
I think structuring the fix in such a way that the code is easily maintainable in the future and is also very important.
Right now it feels like your fix in patch 1 makes things a bit more brittle and I don't like that at all.
Eric
| |