[PATCH] kthread: Make it clear that create_kthread() might be terminated by any fatal signal

[PATCH] kthread: Make it clear that create_kthread() might be terminated by any fatal signal

[Petr Mladek]

The comments in kernel/kthread.c create a feeling that only SIGKILL
is able to break create_kthread().

In reality, wait_for_completion_killable() might be killed by any
fatal signal that does not have a custom handler:

	(!siginmask(signr, SIG_KERNEL_IGNORE_MASK|SIG_KERNEL_STOP_MASK) && \
	 (t)->sighand->action[(signr)-1].sa.sa_handler == SIG_DFL)

static inline void signal_wake_up(struct task_struct *t, bool resume)
{
	signal_wake_up_state(t, resume ? TASK_WAKEKILL : 0);
}

static void complete_signal(int sig, struct task_struct *p, enum pid_type type)
{
[...]
	/*
	 * Found a killable thread.  If the signal will be fatal,
	 * then start taking the whole group down immediately.
	 */
	if (sig_fatal(p, sig) ...) {
		if (!sig_kernel_coredump(sig)) {
		[...]
			do {
				task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
				sigaddset(&t->pending.signal, SIGKILL);
				signal_wake_up(t, 1);
			} while_each_thread(p, t);
			return;
		}
	}
}

Update the comments in kernel/kthread.c to make this more obvious.

The motivation for this change was debugging why a module initialization
failed. The module was being loaded from initrd. It "magically" failed
when systemd was switching to the real root. The clean up operations
sent SIGTERM to various pending processed that were started from initrd.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/kthread.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 38c6dd822da8..6f994c354adb 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -341,7 +341,7 @@ static int kthread(void *_create)
 
 	self = to_kthread(current);
 
-	/* If user was SIGKILLed, I release the structure. */
+	/* Release the structure when caller killed by a fatal signal. */
 	done = xchg(&create->done, NULL);
 	if (!done) {
 		kfree(create);
@@ -399,7 +399,7 @@ static void create_kthread(struct kthread_create_info *create)
 	/* We want our own signal handler (we take no signals by default). */
 	pid = kernel_thread(kthread, create, CLONE_FS | CLONE_FILES | SIGCHLD);
 	if (pid < 0) {
-		/* If user was SIGKILLed, I release the structure. */
+		/* Release the structure when caller killed by a fatal signal. */
 		struct completion *done = xchg(&create->done, NULL);
 
 		if (!done) {
@@ -441,9 +441,9 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 	 */
 	if (unlikely(wait_for_completion_killable(&done))) {
 		/*
-		 * If I was SIGKILLed before kthreadd (or new kernel thread)
-		 * calls complete(), leave the cleanup of this structure to
-		 * that thread.
+		 * If I was killed by a fatal signal before kthreadd (or new
+		 * kernel thread) calls complete(), leave the cleanup of this
+		 * structure to that thread.
 		 */
 		if (xchg(&create->done, NULL))
 			return ERR_PTR(-EINTR);
@@ -877,7 +877,7 @@ __kthread_create_worker(int cpu, unsigned int flags,
  *
  * Returns a pointer to the allocated worker on success, ERR_PTR(-ENOMEM)
  * when the needed structures could not get allocated, and ERR_PTR(-EINTR)
- * when the worker was SIGKILLed.
+ * when the caller was killed by a fatal signal.
  */
 struct kthread_worker *
 kthread_create_worker(unsigned int flags, const char namefmt[], ...)
@@ -926,7 +926,7 @@ EXPORT_SYMBOL(kthread_create_worker);
  * Return:
  * The pointer to the allocated worker on success, ERR_PTR(-ENOMEM)
  * when the needed structures could not get allocated, and ERR_PTR(-EINTR)
- * when the worker was SIGKILLed.
+ * when the caller was killed by a fatal signal.
  */
 struct kthread_worker *
 kthread_create_worker_on_cpu(int cpu, unsigned int flags,
-- 
2.26.2

Re: [PATCH] kthread: Make it clear that create_kthread() might be terminated by any fatal signal

[Eric W. Biederman]

Petr Mladek <pmladek@suse.com> writes:

> The comments in kernel/kthread.c create a feeling that only SIGKILL
> is able to break create_kthread().
                   ^^^^^^^^^^^^^^^^ __kthread_create_on_node

Signals can't affect create_kthread in any was as it is only called by
kthreadd.  It is __kthread_create_on_node which gets interrupted by
fatal signals.

>
> In reality, wait_for_completion_killable() might be killed by any
> fatal signal that does not have a custom handler:
>
> 	(!siginmask(signr, SIG_KERNEL_IGNORE_MASK|SIG_KERNEL_STOP_MASK) && \
> 	 (t)->sighand->action[(signr)-1].sa.sa_handler == SIG_DFL)
>
> static inline void signal_wake_up(struct task_struct *t, bool resume)
> {
> 	signal_wake_up_state(t, resume ? TASK_WAKEKILL : 0);
> }
>
> static void complete_signal(int sig, struct task_struct *p, enum pid_type type)
> {
> [...]
> 	/*
> 	 * Found a killable thread.  If the signal will be fatal,
> 	 * then start taking the whole group down immediately.
> 	 */
> 	if (sig_fatal(p, sig) ...) {
> 		if (!sig_kernel_coredump(sig)) {
> 		[...]
> 			do {
> 				task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
> 				sigaddset(&t->pending.signal, SIGKILL);
> 				signal_wake_up(t, 1);
> 			} while_each_thread(p, t);
> 			return;
> 		}
> 	}
> }
>
> Update the comments in kernel/kthread.c to make this more obvious.
>
> The motivation for this change was debugging why a module initialization
> failed. The module was being loaded from initrd. It "magically" failed
> when systemd was switching to the real root. The clean up operations
> sent SIGTERM to various pending processed that were started from initrd.

Except for the minor nit in the change description.
Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>


> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/kthread.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 38c6dd822da8..6f994c354adb 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -341,7 +341,7 @@ static int kthread(void *_create)
>  
>  	self = to_kthread(current);
>  
> -	/* If user was SIGKILLed, I release the structure. */
> +	/* Release the structure when caller killed by a fatal signal. */
>  	done = xchg(&create->done, NULL);
>  	if (!done) {
>  		kfree(create);
> @@ -399,7 +399,7 @@ static void create_kthread(struct kthread_create_info *create)
>  	/* We want our own signal handler (we take no signals by default). */
>  	pid = kernel_thread(kthread, create, CLONE_FS | CLONE_FILES | SIGCHLD);
>  	if (pid < 0) {
> -		/* If user was SIGKILLed, I release the structure. */
> +		/* Release the structure when caller killed by a fatal signal. */
>  		struct completion *done = xchg(&create->done, NULL);
>  
>  		if (!done) {
> @@ -441,9 +441,9 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
>  	 */
>  	if (unlikely(wait_for_completion_killable(&done))) {
>  		/*
> -		 * If I was SIGKILLed before kthreadd (or new kernel thread)
> -		 * calls complete(), leave the cleanup of this structure to
> -		 * that thread.
> +		 * If I was killed by a fatal signal before kthreadd (or new
> +		 * kernel thread) calls complete(), leave the cleanup of this
> +		 * structure to that thread.
>  		 */
>  		if (xchg(&create->done, NULL))
>  			return ERR_PTR(-EINTR);
> @@ -877,7 +877,7 @@ __kthread_create_worker(int cpu, unsigned int flags,
>   *
>   * Returns a pointer to the allocated worker on success, ERR_PTR(-ENOMEM)
>   * when the needed structures could not get allocated, and ERR_PTR(-EINTR)
> - * when the worker was SIGKILLed.
> + * when the caller was killed by a fatal signal.
>   */
>  struct kthread_worker *
>  kthread_create_worker(unsigned int flags, const char namefmt[], ...)
> @@ -926,7 +926,7 @@ EXPORT_SYMBOL(kthread_create_worker);
>   * Return:
>   * The pointer to the allocated worker on success, ERR_PTR(-ENOMEM)
>   * when the needed structures could not get allocated, and ERR_PTR(-EINTR)
> - * when the worker was SIGKILLed.
> + * when the caller was killed by a fatal signal.
>   */
>  struct kthread_worker *
>  kthread_create_worker_on_cpu(int cpu, unsigned int flags,

Re: [PATCH] kthread: Make it clear that create_kthread() might be terminated by any fatal signal

[Petr Mladek]

On Thu 2022-03-10 12:14:28, Eric W. Biederman wrote:
> Petr Mladek <pmladek@suse.com> writes:
> 
> > The comments in kernel/kthread.c create a feeling that only SIGKILL
> > is able to break create_kthread().
>                    ^^^^^^^^^^^^^^^^ __kthread_create_on_node
> 
> Signals can't affect create_kthread in any was as it is only called by
> kthreadd.  It is __kthread_create_on_node which gets interrupted by
> fatal signals.

Great catch! I wanted to use the public API. I missed that
create_kthread() is used by the "kthreadd" side.

Heh, there actually is "kthread_create()" macro that substitutes
kthread_create_on_node().


> > In reality, wait_for_completion_killable() might be killed by any
> > fatal signal that does not have a custom handler:
> >
> > Update the comments in kernel/kthread.c to make this more obvious.
> 
> Except for the minor nit in the change description.
> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>

Thanks.

I am going to send v2 next week to give time more potential
reviewers.

Best Regards,
Petr