Olivier Langlois <olivier@xxxxxxxxxxxxxx> writes:
> io worker threads are in most regards userspace threads except
> that they never resume userspace. Therefore, they need to explicitly
> handle signals.
>
> On delivering a fatal signal generating a core dump to a thread of
> a group having 1 or more io workers, it is possible for the io_workers
> to exit with pending signals.
>
> One example of this is the io_wqe_worker() function thread in fs/io-wq.c
> This thread can exit the function with pending signals when its
> IO_WQ_BIT_EXIT bit is set.
>
> The consequence of exiting with pending signals is that PF_SIGNALED
> will not be set. This flag is used in exit_mm() to engage into
> the synchronization between do_coredump() and exit_mm().
>
> The purpose of this synchronization is not well documented and all
> that I have found is that it is used to avoid corruption in the core file
> in the section "Deleting a Process Address Space", chapter 9 of the
> Bovet & Cesati book.
We added the check just a little while ago. I am surprised it shows up
in any book. What is the Bovett & Cesati book?
The flag PF_SIGNALED today is set in exactly one place, and that
is in get_signal. The meaning of PF_SIGNALED is that do_group_exit
was called from get_signal. AKA your task was killed by a signal.
The check in exit_mm() that tests PF_SIGNALED is empirically testing
to see if all of the necessary state is saved on the kernel stack.
That state is the state accessed by fs/binfmt_elf.c:fill_note_info.
The very good description from the original change can be found in
the commit 123cbec460db ("signal: Remove the helper signal_group_exit").
For alpha it is has the assembly function do_switch_stack been called
before your code path was called in the kernel.
Since io_uring does not have a userspace I don't know if testing
for PF_SIGNALED is at all meaningful to detect values saved on the
stack.
I suspect io_uring is simply broken on architectures that need
extra state saved on the stack, but I haven't looked yet.
> So I am not sure if the synchronizatin MUST be applied to io_workers
> or not but the proposed patch is making sure that it is applied in
> all cases if it is needed.
That patch is definitely wrong. If anything the check in exit_mm
should be updated.
Can you share which code paths in io_uring exit with a
fatal_signal_pending and don't bother to call get_signal?
I am currently looking to see if the wait for a coredump to read
a threads data can be moved from exit_mm into get_signal. Even
with that io_uring might need a some additional fixes.
Eric
> Signed-off-by: Olivier Langlois <olivier@xxxxxxxxxxxxxx>
> ---
> kernel/signal.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index f7c6ffcbd044..477bfe55fd3c 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2925,6 +2925,15 @@ void exit_signals(struct task_struct *tsk)
>
> if (thread_group_empty(tsk) || signal_group_exit(tsk->signal)) {
> tsk->flags |= PF_EXITING;
> + /*
> + * It is possible for an io worker thread to reach this
> + * function with a pending SIGKILL.
> + * Set PF_SIGNALED for proper core dump generation
> + * (See exit_mm())
> + */
> + if (tsk->flags & PF_IO_WORKER &&
> + signal_group_exit(tsk->signal))
> + tsk->flags |= PF_SIGNALED;
> cgroup_threadgroup_change_end(tsk);
> return;
> }
