Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Eric W. Biederman" <ebiederm@xmission.com>
To:     Petr Mladek <pmladek@suse.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>,
        Kees Cook <keescook@chromium.org>,
        Marco Elver <elver@google.com>, Jens Axboe <axboe@kernel.dk>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org
References: <20220310093455.15176-1-pmladek@suse.com>
Date:   Thu, 10 Mar 2022 12:14:28 -0600
In-Reply-To: <20220310093455.15176-1-pmladek@suse.com> (Petr Mladek's message
        of "Thu, 10 Mar 2022 10:34:55 +0100")
Message-ID: <871qz9brp7.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH] kthread: Make it clear that create_kthread() might be
 terminated by any fatal signal
Precedence: bulk

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,