Re: [PATCH V1] kthread: Unify kernel_thread() and user_mode_thread()

[Kees Cook]

On Sat, Jun 03, 2023 at 09:53:02AM +0800, Huacai Chen wrote:
> Commit 343f4c49f2438d8 ("kthread: Don't allocate kthread_struct for init
> and umh") introduces a new function user_mode_thread() for init and umh.
> 
> init and umh are different from typical kernel threads since the don't
> need a "kthread" struct and they will finally become user processes by
> calling kernel_execve(), but on the other hand, they are also different
> from typical user mode threads (they have no "mm" structs at creation
> time, which is traditionally used to distinguish a user thread and a
> kernel thread).
> 
> So I think it is reasonable to treat init and umh as "special kernel
> threads". Then let's unify the kernel_thread() and user_mode_thread()
> to kernel_thread() again, and add a new 'user' parameter for init and
> umh.
> 
> This also makes code simpler. 
> 
> Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx>
> ---
> RFC -> V1: Update commit message and change "user" from int to bool.
> 
>  include/linux/sched/task.h |  3 +--
>  init/main.c                |  4 ++--
>  kernel/fork.c              | 20 ++------------------
>  kernel/kthread.c           |  2 +-
>  kernel/umh.c               |  6 +++---
>  5 files changed, 9 insertions(+), 26 deletions(-)
> 
> diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
> index 537cbf9a2ade..02eb953bc809 100644
> --- a/include/linux/sched/task.h
> +++ b/include/linux/sched/task.h
> @@ -98,8 +98,7 @@ struct task_struct *copy_process(struct pid *pid, int trace, int node,
>  struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node);
>  struct task_struct *fork_idle(int);
>  extern pid_t kernel_thread(int (*fn)(void *), void *arg, const char *name,
> -			    unsigned long flags);
> -extern pid_t user_mode_thread(int (*fn)(void *), void *arg, unsigned long flags);
> +			    unsigned long flags, bool user);

Please make this an enum not a bool, otherwise it's not obvious when
reading calling code what it means.

>  extern long kernel_wait4(pid_t, int __user *, int, struct rusage *);
>  int kernel_wait(pid_t pid, int *stat);
>  
> diff --git a/init/main.c b/init/main.c
> index af50044deed5..469cebbd35e0 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -697,7 +697,7 @@ noinline void __ref __noreturn rest_init(void)
>  	 * the init task will end up wanting to create kthreads, which, if
>  	 * we schedule it before we create kthreadd, will OOPS.
>  	 */
> -	pid = user_mode_thread(kernel_init, NULL, CLONE_FS);
> +	pid = kernel_thread(kernel_init, NULL, NULL, CLONE_FS, true);

i.e. instead of "true", if this said USER_MODE_THREAD, it would be
easier to under.

>  	/*
>  	 * Pin init on the boot CPU. Task migration is not properly working
>  	 * until sched_init_smp() has been run. It will set the allowed
> @@ -710,7 +710,7 @@ noinline void __ref __noreturn rest_init(void)
>  	rcu_read_unlock();
>  
>  	numa_default_policy();
> -	pid = kernel_thread(kthreadd, NULL, NULL, CLONE_FS | CLONE_FILES);
> +	pid = kernel_thread(kthreadd, NULL, NULL, CLONE_FS | CLONE_FILES, false);

And similarly, KERNEL_THREAD instead of "false".

-Kees

-- 
Kees Cook