Re: [PATCH] secure unlock_task_sighand() call

[Oleg Nesterov <oleg@redhat.com>]

Naveen,

sorry for the terse and neglectful reply yesterday.

Actually, when I re-read the Linus's email, I think he already explained
everything, so let me repeat:

On 12/21, Linus Torvalds wrote:
>
> Did you actually *see* the problem, or was this just from looking at the code?

Yes. Because this code assumes that lock_task_sighand() must not fail.
If it fails, we have a problem which should be fixed.

> We have coredump serialization in exit_mm() that I think *should* make
> this all ok - if we still see p->mm matching our mm, I don't think it
> should be able to get to __exit_signal() and make the sighand go away,
> so the lock_task_sighand() shouldn't ever fail.

Yes, exactly.

Note that if we ignore exec, we do not need lock_task_sighand() at all,
we could simply do spin_lock_irq(p->sighand->siglock).

The caller holds mm->mmap_sem for writing, if we see p->mm == mm it
simply can not pass exit_mm() which does down_read(&mm->mmap_sem), so
this task can not exit.

The problem is, this task can change its ->sighand in de_thread(), that
is why we need lock_task_sighand(). But if it does exec, it can't pass
exec_mmap() by the same reason, we hold mmap_sem.

> >                         if (p->mm) {
> >                                 if (unlikely(p->mm == mm)) {
> > -                                       lock_task_sighand(p, &flags);
> > -                                       nr += zap_process(p, exit_code);
> > -                                       unlock_task_sighand(p, &flags);
> > +                                       if (lock_task_sighand(p, &flags) {
> > +                                               nr += zap_process(p, exit_code);

But we can't silently skip a process with the same ->mm. We can't even
skip the execing thread task if it is going to change its ->mm, even if
it is single-threaded. Note that exec_mmap() will notice mm->core_state
and fail. So every task with the same mm should be accounted because it
will play with core_state->nr_threads in exit_mm(). And it should be
killed because otherwise coredump_wait() can sleep "forever".

So this is not the right change in any case. If lock_task_sighand() can
fail we should fix something else.

Oleg.