coredump & signals: an impossible affair

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. That means copy_process() allows the creation. It's also possible for an exiting io-wq worker to push new work onto the SQPOLL thread.

zap_threads() counts the number of coredumping threads. The coredump client waits in coredump_wait_inactive() until all threads in the thread-group are parked. The new thread exits right away because the workqueue is going down. It inherited PF_SIGNALED from its creator so it links itself onto core_state->tasks in coredump_task_exit(). The problem is that it then decrements "threads_remaining" even though zap_process() never actually counted the new thread. So the count goes to zero too early. So either the coredump misses the thread or it dumps a thread that is still alive.

The same race exists during exec. de_thread() zaps the other threads the same way and counts them in signal->notify_count, and a zapped user worker that has already dequeued its SIGKILL can still clone while de_thread() waits for that count. And once de_thread() has cleared signal->group_exec_task the exec'ing thread itself runs task work in io_uring_task_cancel() and a pending create_worker_cb() creates a thread after the group was made single-threaded.

Close all of that in copy_process(). Refuse to create a thread while:

(1) SIGNAL_GROUP_EXIT is set (set together with core_state by zap_process()
(2) signal->group_exec_task is set
(3) while current is in execve

Conditions (1) and (2) are handled with siglock help which means copy_process() and zap_process() synchronize on it. create_io_thread() treats the failure as a failed task creation and doesn't retry.

Fixes 3bfe6106693b ("io-wq: fork worker threads from original task")

Cc [email protected]

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
kernel/fork.c | 6 ++++--

1 file changed, 4 insertions(+), 2 deletions(-)

kernel/fork.c +0 -0

index 10be4a0ecb3f..6cd167a2b27b 100644

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.

Quoted 7 lines · Christian Brauner

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.

Quoted 3 lines

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.

kernel/fork.c __latent_entropy struct task_struct *copy_process( · line 2497

Quoted 13 lines

kernel/fork.c +4 -2

@@ -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.

kernel/fork.c +3 -0

@@ -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);
 }
 

Quoted 8 lines · Oleg Nesterov

Quoted 7 lines · Christian Brauner

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.

Quoted 7 lines

Quoted 3 lines

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.

Yeah, I agree but the io_uring solution I came up with all where a bit involved...

Quoted 20 lines

kernel/fork.c __latent_entropy struct task_struct *copy_process( · line 2497

Quoted 13 lines

kernel/fork.c +4 -2

@@ -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()...

Quoted 4 lines

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?

Hm, I like this idea...
Let's see.

kernel/fork.c struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node) · line 2712

Quoted 11 lines · Christian Brauner

I think this works but they also need to check ->in_execve as the exec'ing thread is obviously never signaled:

kernel/fork.c +3 -0

@@ -2709,6 +2707,10 @@ struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node)
                .user_worker    = 1,
        };
 
+       /* A creator past its fatal signal or in execve gets no thread. */
+       if ((current->flags & PF_SIGNALED) || current->in_execve)
+               return ERR_PTR(-EINTR);

Ah, I forgot to mention...

Can we shift io_uring_task_cancel() up, after setting bprm->point_of_no_return but before de_thread() ?

I know nothing about io_uring, not sure this would be enough...

Quoted 18 lines · Oleg Nesterov

Review struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node) · line 2712

Quoted 11 lines · Christian Brauner

I think this works but they also need to check ->in_execve as the exec'ing thread is obviously never signaled:

(quoted diff) +3 -0

@@ -2709,6 +2707,10 @@ struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node)
                .user_worker    = 1,
        };
 
+       /* A creator past its fatal signal or in execve gets no thread. */
+       if ((current->flags & PF_SIGNALED) || current->in_execve)
+               return ERR_PTR(-EINTR);

Ah, I forgot to mention...

Can we shift io_uring_task_cancel() up, after setting bprm->point_of_no_return but before de_thread() ?

Quoted 1 line

I know nothing about io_uring, not sure this would be enough...

From my reading of this code it works.
The cancel runs task work and any create_worker_cb() queued before the exec can still create an io-wq worker. If that happens before de_thread(), that worker just becomes a sibling that de_thread() kills and waits for. After the cancel nothing new can be queued.

Quoted 19 lines · Christian Brauner

Quoted 18 lines · Oleg Nesterov

kernel/fork.c struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node) · line 2712

Quoted 11 lines · Christian Brauner

I think this works but they also need to check ->in_execve as the exec'ing thread is obviously never signaled:

kernel/fork.c +3 -0

@@ -2709,6 +2707,10 @@ struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node)
                .user_worker    = 1,
        };
 
+       /* A creator past its fatal signal or in execve gets no thread. */
+       if ((current->flags & PF_SIGNALED) || current->in_execve)
+               return ERR_PTR(-EINTR);

Ah, I forgot to mention...

Can we shift io_uring_task_cancel() up, after setting bprm->point_of_no_return but before de_thread() ?

Quoted 7 lines

Quoted 1 line

I know nothing about io_uring, not sure this would be enough...

From my reading of this code it works.
The cancel runs task work and any create_worker_cb() queued before the exec can still create an io-wq worker. If that happens before de_thread(), that worker just becomes a sibling that de_thread() kills and waits for. After the cancel nothing new can be queued.

Right, one more thing to think about with this...

zap_process() skips every thread that has PF_POSTCOREDUMP set. do_exit() sets PF_POSTCOREDUMP in synchronize_group_exit() and calls io_uring_files_cancel() right after that.

That runs task work, create_worker_cb() runs and creates a new io-wq worker. If a thread started a coredump rgith before that worker gets created from a thread with PF_POSTCOREDUMP set which zap_processes() doesn't see. So two ways of fixing this:

(1) mask off PF_POSTCOREDUMP in copy_process() -> probably the wrong place
(2) key on PF_SIGNALED | PF_POSTCOREDUMP

I think (2) is probably correct:

kernel/fork.c +2 -2

index a28fd3976cc0..94a652d933c8 100644
@@ -2707,8 +2707,8 @@ struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node)
                .user_worker    = 1,
        };
 
-       /* A creator past its fatal signal gets no thread. */
-       if (current->flags & PF_SIGNALED)
+       /* A creator past its fatal signal or its coredump point gets no thread. */
+       if (current->flags & (PF_SIGNALED | PF_POSTCOREDUMP))
                return ERR_PTR(-EINTR);
 
        return copy_process(NULL, 0, node, &args);

Better ideas?

zap_process() skips every thread that has PF_POSTCOREDUMP set. do_exit() sets PF_POSTCOREDUMP in synchronize_group_exit() and calls io_uring_files_cancel() right after that.

That runs task work, create_worker_cb() runs and creates a new io-wq worker. If a thread started a coredump rgith before that worker gets created from a thread with PF_POSTCOREDUMP set which zap_processes() doesn't see. So two ways of fixing this:

kernel/fork.c +2 -2

@@ -2707,8 +2707,8 @@ struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node)
                .user_worker    = 1,
        };
 
-       /* A creator past its fatal signal gets no thread. */
-       if (current->flags & PF_SIGNALED)
+       /* A creator past its fatal signal or its coredump point gets no thread. */
+       if (current->flags & (PF_SIGNALED | PF_POSTCOREDUMP))
                return ERR_PTR(-EINTR);

Quoted 5 lines · Christian Brauner

zap_process() skips every thread that has PF_POSTCOREDUMP set. do_exit() sets PF_POSTCOREDUMP in synchronize_group_exit() and calls io_uring_files_cancel() right after that.

Quoted 4 lines

That runs task work, create_worker_cb() runs and creates a new io-wq worker. If a thread started a coredump rgith before that worker gets created from a thread with PF_POSTCOREDUMP set which zap_processes() doesn't see. So two ways of fixing this:

Christian, I got lost ;)

TBH, I don't understand why the new io worker is really bad in this case.

But lets forget about coredump/zap_process. With the change below the io_uring_files_cancel() paths will no longer be able to create a new worker, even if coredump is not in progress.

Is it correct?

Quoted 13 lines · Oleg Nesterov

Quoted 9 lines · Christian Brauner

zap_process() skips every thread that has PF_POSTCOREDUMP set. do_exit() sets PF_POSTCOREDUMP in synchronize_group_exit() and calls io_uring_files_cancel() right after that.

That runs task work, create_worker_cb() runs and creates a new io-wq worker. If a thread started a coredump rgith before that worker gets created from a thread with PF_POSTCOREDUMP set which zap_processes() doesn't see. So two ways of fixing this:

Christian, I got lost ;)

Quoted 1 line

TBH, I don't understand why the new io worker is really bad in this case.

zap_processes() only counts threads it sends SIGKILL too. But every thread that later finds ->core_state decrements that count. An io-wq worker created from the exiting thread isn't counted and exits right away because the io-wq exit bit is already set. So it decrements without having been counted.

Quoted 5 lines

But lets forget about coredump/zap_process. With the change below the io_uring_files_cancel() paths will no longer be able to create a new worker, even if coredump is not in progress.

Is it correct?

Yes.