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

Huacai Chen posted 1 patch 10 months ago
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(-)
[PATCH V1] kthread: Unify kernel_thread() and user_mode_thread()
Posted by Huacai Chen 10 months ago
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@loongson.cn>
---
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);
 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);
 	/*
 	 * 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);
 	rcu_read_lock();
 	kthreadd_task = find_task_by_pid_ns(pid, &init_pid_ns);
 	rcu_read_unlock();
diff --git a/kernel/fork.c b/kernel/fork.c
index ed4e01daccaa..f91696904252 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2965,7 +2965,7 @@ pid_t kernel_clone(struct kernel_clone_args *args)
  * Create a kernel thread.
  */
 pid_t kernel_thread(int (*fn)(void *), void *arg, const char *name,
-		    unsigned long flags)
+		    unsigned long flags, bool user)
 {
 	struct kernel_clone_args args = {
 		.flags		= ((lower_32_bits(flags) | CLONE_VM |
@@ -2974,23 +2974,7 @@ pid_t kernel_thread(int (*fn)(void *), void *arg, const char *name,
 		.fn		= fn,
 		.fn_arg		= arg,
 		.name		= name,
-		.kthread	= 1,
-	};
-
-	return kernel_clone(&args);
-}
-
-/*
- * Create a user mode thread.
- */
-pid_t user_mode_thread(int (*fn)(void *), void *arg, unsigned long flags)
-{
-	struct kernel_clone_args args = {
-		.flags		= ((lower_32_bits(flags) | CLONE_VM |
-				    CLONE_UNTRACED) & ~CSIGNAL),
-		.exit_signal	= (lower_32_bits(flags) & CSIGNAL),
-		.fn		= fn,
-		.fn_arg		= arg,
+		.kthread	= !user,
 	};
 
 	return kernel_clone(&args);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 490792b1066e..5f025569eb38 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -400,7 +400,7 @@ static void create_kthread(struct kthread_create_info *create)
 #endif
 	/* We want our own signal handler (we take no signals by default). */
 	pid = kernel_thread(kthread, create, create->full_name,
-			    CLONE_FS | CLONE_FILES | SIGCHLD);
+			    CLONE_FS | CLONE_FILES | SIGCHLD, false);
 	if (pid < 0) {
 		/* Release the structure when caller killed by a fatal signal. */
 		struct completion *done = xchg(&create->done, NULL);
diff --git a/kernel/umh.c b/kernel/umh.c
index 60aa9e764a38..b0ead7cce761 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -130,7 +130,7 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
 
 	/* If SIGCLD is ignored do_wait won't populate the status. */
 	kernel_sigaction(SIGCHLD, SIG_DFL);
-	pid = user_mode_thread(call_usermodehelper_exec_async, sub_info, SIGCHLD);
+	pid = kernel_thread(call_usermodehelper_exec_async, sub_info, NULL, SIGCHLD, true);
 	if (pid < 0)
 		sub_info->retval = pid;
 	else
@@ -169,8 +169,8 @@ static void call_usermodehelper_exec_work(struct work_struct *work)
 		 * want to pollute current->children, and we need a parent
 		 * that always ignores SIGCHLD to ensure auto-reaping.
 		 */
-		pid = user_mode_thread(call_usermodehelper_exec_async, sub_info,
-				       CLONE_PARENT | SIGCHLD);
+		pid = kernel_thread(call_usermodehelper_exec_async, sub_info,
+				       NULL, CLONE_PARENT | SIGCHLD, true);
 		if (pid < 0) {
 			sub_info->retval = pid;
 			umh_complete(sub_info);
-- 
2.39.1
Re: [PATCH V1] kthread: Unify kernel_thread() and user_mode_thread()
Posted by Huacai Chen 9 months, 2 weeks ago
Hi, all,

I'm very sorry for wasting so much of your time. Although this patch
has dropped, I still agree with Andrews' comment "But the naming isn't
very good anyway.  Should have been usermode_thread/kernel_thread or
user_thread/kernel_thread.". And since Eric describes init and umh as
"user threads run in kernel mode", I want to rename user_mode_thread()
as kmuser_thread(), which is a little better than just user_thread().


Huacai

On Sat, Jun 3, 2023 at 9:53 AM Huacai Chen <chenhuacai@loongson.cn> 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@loongson.cn>
> ---
> 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);
>  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);
>         /*
>          * 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);
>         rcu_read_lock();
>         kthreadd_task = find_task_by_pid_ns(pid, &init_pid_ns);
>         rcu_read_unlock();
> diff --git a/kernel/fork.c b/kernel/fork.c
> index ed4e01daccaa..f91696904252 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2965,7 +2965,7 @@ pid_t kernel_clone(struct kernel_clone_args *args)
>   * Create a kernel thread.
>   */
>  pid_t kernel_thread(int (*fn)(void *), void *arg, const char *name,
> -                   unsigned long flags)
> +                   unsigned long flags, bool user)
>  {
>         struct kernel_clone_args args = {
>                 .flags          = ((lower_32_bits(flags) | CLONE_VM |
> @@ -2974,23 +2974,7 @@ pid_t kernel_thread(int (*fn)(void *), void *arg, const char *name,
>                 .fn             = fn,
>                 .fn_arg         = arg,
>                 .name           = name,
> -               .kthread        = 1,
> -       };
> -
> -       return kernel_clone(&args);
> -}
> -
> -/*
> - * Create a user mode thread.
> - */
> -pid_t user_mode_thread(int (*fn)(void *), void *arg, unsigned long flags)
> -{
> -       struct kernel_clone_args args = {
> -               .flags          = ((lower_32_bits(flags) | CLONE_VM |
> -                                   CLONE_UNTRACED) & ~CSIGNAL),
> -               .exit_signal    = (lower_32_bits(flags) & CSIGNAL),
> -               .fn             = fn,
> -               .fn_arg         = arg,
> +               .kthread        = !user,
>         };
>
>         return kernel_clone(&args);
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 490792b1066e..5f025569eb38 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -400,7 +400,7 @@ static void create_kthread(struct kthread_create_info *create)
>  #endif
>         /* We want our own signal handler (we take no signals by default). */
>         pid = kernel_thread(kthread, create, create->full_name,
> -                           CLONE_FS | CLONE_FILES | SIGCHLD);
> +                           CLONE_FS | CLONE_FILES | SIGCHLD, false);
>         if (pid < 0) {
>                 /* Release the structure when caller killed by a fatal signal. */
>                 struct completion *done = xchg(&create->done, NULL);
> diff --git a/kernel/umh.c b/kernel/umh.c
> index 60aa9e764a38..b0ead7cce761 100644
> --- a/kernel/umh.c
> +++ b/kernel/umh.c
> @@ -130,7 +130,7 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
>
>         /* If SIGCLD is ignored do_wait won't populate the status. */
>         kernel_sigaction(SIGCHLD, SIG_DFL);
> -       pid = user_mode_thread(call_usermodehelper_exec_async, sub_info, SIGCHLD);
> +       pid = kernel_thread(call_usermodehelper_exec_async, sub_info, NULL, SIGCHLD, true);
>         if (pid < 0)
>                 sub_info->retval = pid;
>         else
> @@ -169,8 +169,8 @@ static void call_usermodehelper_exec_work(struct work_struct *work)
>                  * want to pollute current->children, and we need a parent
>                  * that always ignores SIGCHLD to ensure auto-reaping.
>                  */
> -               pid = user_mode_thread(call_usermodehelper_exec_async, sub_info,
> -                                      CLONE_PARENT | SIGCHLD);
> +               pid = kernel_thread(call_usermodehelper_exec_async, sub_info,
> +                                      NULL, CLONE_PARENT | SIGCHLD, true);
>                 if (pid < 0) {
>                         sub_info->retval = pid;
>                         umh_complete(sub_info);
> --
> 2.39.1
>
Re: [PATCH V1] kthread: Unify kernel_thread() and user_mode_thread()
Posted by Kees Cook 9 months, 3 weeks ago
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@loongson.cn>
> ---
> 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
Re: [PATCH V1] kthread: Unify kernel_thread() and user_mode_thread()
Posted by Andrew Morton 9 months, 3 weeks ago
On Sat,  3 Jun 2023 09:53:02 +0800 Huacai Chen <chenhuacai@loongson.cn> 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. 

Seems fair enough.

If we're attached to the naming then we could do

static inline pid_t user_mode_thread(int (*fn)(void *), void *arg,
				     unsigned long flags)
{
	return __kernel_thread(fn, arg, flags, 0);
}

static inline pid_t kernel_thread(int (*fn)(void *), void *arg,
				     unsigned long flags)
{
	return __kernel_thread(fn, arg, flags, 1);
}

(and pass the 4th arg straight into .kthread to avoid the !user thing)


But the naming isn't very good anyway.  Should have been
usermode_thread/kernel_thread or user_thread/kernel_thread.
Re: [PATCH V1] kthread: Unify kernel_thread() and user_mode_thread()
Posted by Luis Chamberlain 9 months, 3 weeks ago
On Mon, Jun 05, 2023 at 04:10:52PM -0700, Andrew Morton wrote:
> On Sat,  3 Jun 2023 09:53:02 +0800 Huacai Chen <chenhuacai@loongson.cn> 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. 
> 
> Seems fair enough.

I thought Eric still disagreed?

  Luis