Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

[Oleg Nesterov]

On 02/22, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@xxxxxxxxxx> 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.