Re: [PATCH v2 1/7] fork: refuse new threads while a coredump or an exec is in progress

On 09/17, Christian Brauner wrote:
>
> Say a SQPOLL thread is a member of a thread-group that coredumps.
> The coredump code uses zap_process() and sends SIGKILL. The SQPOLL
> thread uses io_sqd_handle_event() and calls get_signal(). It removes
> SIGKILL from the pending set and returns. The SQPOLL thread breaks out
> of the loop and drains its own task work.
>
> Any pending io_req_task_submit() with REQ_F_FORCE_ASYNC creates a new
> worker when no other worker is free. So it ends up calling
> create_io_thread() from a thread whose fatal signal is gone.

Oh.. Can we fix this in the io_uring/ code somehow? The very fact that
copy_process() can be called after get_signal() returns SIGKILL looks
very wrong to me. See below.

> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2491,8 +2491,10 @@ __latent_entropy struct task_struct *copy_process(
>  		goto bad_fork_core_free;
>  	}
>
> -	/* Let kill terminate clone/fork in the middle */
> -	if (fatal_signal_pending(current)) {
> +	/* Let kill or a group exit, exec or coredump abort clone/fork */
> +	if (fatal_signal_pending(current) ||
> +	    (current->signal->flags & SIGNAL_GROUP_EXIT) ||
> +	    current->signal->group_exec_task || current->in_execve) {

Well, the comment doesn't explain why should we care about exec or coredump,
if we forget about the problem above fatal_signal_pending() must be true.

At least, can we move these additional checks into create_io_thread() ?
To not uglify copy_process()...

Hmm... get_signal() sets PF_SIGNALED before it checks PF_USER_WORKER, so
perhaps something like below can work?

And perhaps io_should_retry_thread() should check PF_SIGNALED too?

Oleg.

--- x/kernel/fork.c
+++ x/kernel/fork.c
@@ -2700,6 +2700,9 @@ struct task_struct *create_io_thread(int
 		.user_worker	= 1,
 	};
 
+	if (current->flags && PF_SIGNALED)
+		return -EINTR;
+
 	return copy_process(NULL, 0, node, &args);
 }