Re: [PATCH 5/6] coredump: Don't perform any cleanups before dumping core

On Fri, Sep 24, 2021 at 04:28:58PM -0500, Eric W. Biederman wrote:

So everything should work from a locking perspective as I am not
changing the locking I am simply moving the call from exit_mm earlier.

To explain how the locking works.

Coredumps are not handled in complete_signal, so when
a thread dequeues the signal the other threads are all running.

If two threads dequeue core dumping signals at the same time both will
contend for mmap_write_lock one will get it and set core_state the
second will return -EBUSY from coredump_wait and return from do_coredump
and proceed to do_exit.

There similar set of races that zap_threads called from coredump_wait
resolves by testing for signal_group_exit inside of sighand lock.

The normal case is one thread runs through do_coredump, coredump_wait,
and zap_threads, counts the threads and waits in coredump_wait for
all of the other threads to stop.

The other threads proceed to do_exit and  coredump_task_exit.
From their the discover that core_state is set.  And holding
the mmap_read_lock is enough to know ensure that either
no coredump is in progress or all of the setup is complete in
coredump_wait.

If core_state is found it is known that there is a waiter waiting in
coredump_wait.  So the tasks add themselves to the list.  Decrement
the count to indicate they don't need to be waited for any longer
and the last one waits up the waiter in coredump_wait.

The threads then spin in TASK_UNINTERRUPTLE until the coredump
completes.

The dumping thread takes the coredump then calls coredump_finish.
In coredump_finish the linked list is torn down, and each element
of the linked list has sets ".task = NULL"  Then the task is woken
up.

The task waiting in TASK_UNINTERRUPTIBLE wakes up. Sees that .task =
NULL and proceeds with the rest of do_exit.


The only change  this patch makes to that entire process is
"task->mm = NULL" is replaced by setting PF_POSTCOREDUMP.

Does that help?

That does! Thanks very much for the run-down on the flow there. I'm
convinced. :)

Reviewed-by: Kees Cook <redacted>

-Kees

-- 
Kees Cook