Messages in this thread | | | Date | Wed, 22 Feb 2017 18:41:03 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held |
| |
On 02/22, Eric W. Biederman wrote: > > Oleg Nesterov <oleg@redhat.com> writes: > > >> 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.
I fail to understand how/why this can be wrong.
> >> 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.
No. They already use mutex_lock_killable/interruptible. And the test-case can be killed, it is not the hard-lockup.
> I really don't like the first patch.
Just in case, I don't really like it too. Simply because it makes execve more complex, we need to wait for sub-threads twice.
> It makes an information leak part > a required detail of the implementation and as such possibly something > we can never change.
Again, I simply can't understand how flush_signal_handlers() outside of cred_guard_mutex can be treated as information leak. Even _if_ collect_sigign_sigcatch() was called with this mutex held.
Or do you mean something else?
> 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 am not sure I understand how we can do this. We need cred_guard_mutex or something else even if exec is not setuid and does not change the credentials, an LSM module can nack exec-under-ptrace by any reason.
> I think fixing the deadlock is important.
Yes. People actually hit this bug, it was reported several times.
> Right now it feels like your fix in patch 1 makes things a bit more > brittle and I don't like that at all.
See above, I am not proud of this change too. I even mentioned on 0/2 that it would be nice to reconsider this change in the long term.
But I do not see another simple and _backportable_ solution for now.
What do you think we can do instead for stable trees?
Oleg.
| |