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

[Oleg Nesterov <oleg@redhat.com>]

On 02/17, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > de_thread() waits for other threads with ->cred_guard_mutex held and this
> > is really bad because the time is not bounded, debugger can delay the exit
> > and this lock has a lot of users (mostly abusers imo) in fs/proc and more.
> > And this leads to deadlock if debugger tries to take the same mutex:
>
> Oleg.  I looked at the history in proc of users of cred_guard_mutex
> and the proc users are grabbing cred_guard_mutex for the proper
> semantic reasons.  To avoid races with setuid exec that could result in
> an information disclosure.

This is clear. However I really think it is over-used. For example, I do
think that lock_trace() should die.

OK, of course I can be wrong. And in any case this is almost off-topic
right now, lets discuss this separately.

> On that score I believe we can incrementally approach that point and
> only grab the cred_guard_mutex in exec if we are performing an exec that
> changes the processes credentials.

May be... but afaics this is more complicated because of LSM hooks.
To me one the main problems is that lsm hook can fail if the task is
traced, that is why we can't simply take cred_guard_mutex and check
->ptrace after de_thread(). But again, lets discuss this later.

> Right now I don't think it introduces any new security information
> disclosures but the moving of flush_signal_handlers outside of
> cred_guard_mutex feels wrong.

Why?

Just in case, we can do flush_signal_handlers() only after we unshare
->sighand, that is why it was moved outside of cred_guard_mutex.

But why this is bad? collect_sigign_sigcatch() is called without this
mutex, who else can look at sa.sa_handler?

Oleg.