On Tue, Feb 15, 2022 at 01:22:04AM +0000, Edgecombe, Rick P wrote:
> On Mon, 2022-02-14 at 13:33 +0100, Jann Horn wrote:
> > On Sun, Jan 30, 2022 at 10:22 PM Rick Edgecombe
> > <rick.p.edgecombe@xxxxxxxxx> wrote:
> > >
> > > From: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> > >
> > > The single call site of copy_thread() passes stack size in
> > > 'arg'. To make
> > > this clear and in preparation of using this argument for shadow
> > > stack
> > > allocation, change 'arg' to 'stack_size'. No functional changes.
> >
> > Actually that name is misleading - the single caller copy_process()
> > indeed does:
> >
> > retval = copy_thread(clone_flags, args->stack, args->stack_size, p,
> > args->tls);
> >
> > but the member "stack_size" of "struct kernel_clone_args" can
> > actually
> > also be a pointer argument given to a kthread, see create_io_thread()
> > and kernel_thread():
> >
> > pid_t kernel_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),
> > .stack = (unsigned long)fn,
> > .stack_size = (unsigned long)arg,
> > };
> >
> > return kernel_clone(&args);
> > }
> >
> > And then in copy_thread(), we have:
> >
> > kthread_frame_init(frame, sp, arg)
> >
> >
> > So I'm not sure whether this name change really makes sense, or
> > whether it just adds to the confusion.
>
> Thanks Jann. Yea I guess this makes it worse.
>
> Reading a bit of the history, it seems there used to be unwieldy
> argument lists which were replaced by a big "struct kernel_clone_args"
> to be passed around. The re-use of the stack_size argument is from
> before there was the struct. And then the struct just inherited the
> confusion when it was introduced.
>
> So if a separate *data member was added to kernel_clone_args for
> kernel_thread() and create_io_thread() to use, they wouldn't need to
> re-use stack_size. And copy_thread() could just take a struct
> kernel_clone_args pointer. It might make it more clear.
I'm honestly not sure it makes things that much better but I don't feel
strongly about it either.
>
> But copy_thread() has a ton of arch specific implementations. So they
> would all need to be updated to do this.
When struct kernel_clone_args was introduced I also removed the
copy_thread_tls() and copy_thread() split. So now we're only left with
copy_thread(). That already allowed us to get rid of some arch-specific
code. I didn't go further in trying to unify more arch-specific code. It
might be worth it but I didn't come to a clear conclusion.
>
> Just playing around with it, I did this which only makes the change on
> x86. Duplicating it for 21 copy_thread()s seems perfectly doable, but
> I'm not sure how much people care vs renaming arg to
> stacksize_or_data...
>
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 11bf09b60f9d..cfbba5b14609 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -132,9 +132,8 @@ static int set_new_tls(struct task_struct *p,
> unsigned long tls)
> return do_set_thread_area_64(p, ARCH_SET_FS, tls);
> }
>
> -int copy_thread(unsigned long clone_flags, unsigned long sp,
> - unsigned long stack_size, struct task_struct *p,
> - unsigned long tls)
> +int copy_thread(unsigned long clone_flags, struct kernel_clone_args
> *args,
> + struct task_struct *p)
> {
> struct inactive_task_frame *frame;
> struct fork_frame *fork_frame;
> @@ -178,7 +177,7 @@ int copy_thread(unsigned long clone_flags, unsigned
> long sp,
> if (unlikely(p->flags & PF_KTHREAD)) {
> p->thread.pkru = pkru_get_init_value();
> memset(childregs, 0, sizeof(struct pt_regs));
> - kthread_frame_init(frame, sp, stack_size);
> + kthread_frame_init(frame, args->stack, (unsigned
> long)args->data);
> return 0;
> }
>
> @@ -191,8 +190,8 @@ int copy_thread(unsigned long clone_flags, unsigned
> long sp,
> frame->bx = 0;
> *childregs = *current_pt_regs();
> childregs->ax = 0;
> - if (sp)
> - childregs->sp = sp;
> + if (args->stack)
> + childregs->sp = args->stack;
>
> #ifdef CONFIG_X86_32
> task_user_gs(p) = get_user_gs(current_pt_regs());
> @@ -211,17 +210,17 @@ int copy_thread(unsigned long clone_flags,
> unsigned long sp,
> */
> childregs->sp = 0;
> childregs->ip = 0;
> - kthread_frame_init(frame, sp, stack_size);
> + kthread_frame_init(frame, args->stack, (unsigned
> long)args->data);
> return 0;
> }
>
> /* Set a new TLS for the child thread? */
> if (clone_flags & CLONE_SETTLS)
> - ret = set_new_tls(p, tls);
> + ret = set_new_tls(p, args->tls);
>
> /* Allocate a new shadow stack for pthread */
> if (!ret)
> - ret = shstk_alloc_thread_stack(p, clone_flags,
> stack_size);
> + ret = shstk_alloc_thread_stack(p, clone_flags, args-
> >stack_size);
>
> if (!ret && unlikely(test_tsk_thread_flag(current,
> TIF_IO_BITMAP)))
> io_bitmap_share(p);
> diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
> index b9198a1b3a84..f138b23aee50 100644
> --- a/include/linux/sched/task.h
> +++ b/include/linux/sched/task.h
> @@ -34,6 +34,7 @@ struct kernel_clone_args {
> int io_thread;
> struct cgroup *cgrp;
> struct css_set *cset;
> + void *data;
> };
>
> /*
> @@ -67,8 +68,8 @@ extern void fork_init(void);
>
> extern void release_task(struct task_struct * p);
>
> -extern int copy_thread(unsigned long, unsigned long, unsigned long,
> - struct task_struct *, unsigned long);
> +extern int copy_thread(unsigned long clone_flags, struct
> kernel_clone_args *args,
> + struct task_struct *p);
>
> extern void flush_thread(void);
>
> @@ -85,10 +86,10 @@ extern void exit_files(struct task_struct *);
> extern void exit_itimers(struct signal_struct *);
>
> extern pid_t kernel_clone(struct kernel_clone_args *kargs);
> -struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int
> node);
> +struct task_struct *create_io_thread(int (*fn)(void *), void *data,
> int node);
> struct task_struct *fork_idle(int);
> struct mm_struct *copy_init_mm(void);
> -extern pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long
> flags);
> +extern pid_t kernel_thread(int (*fn)(void *), void *data, unsigned
> long flags);
> extern long kernel_wait4(pid_t, int __user *, int, struct rusage *);
> int kernel_wait(pid_t pid, int *stat);
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index d75a528f7b21..8af202e5651e 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2170,7 +2170,7 @@ static __latent_entropy struct task_struct
> *copy_process(
> retval = copy_io(clone_flags, p);
> if (retval)
> goto bad_fork_cleanup_namespaces;
> - retval = copy_thread(clone_flags, args->stack, args-
> >stack_size, p, args->tls);
> + retval = copy_thread(clone_flags, args, p);
> if (retval)
> goto bad_fork_cleanup_io;
>
> @@ -2487,7 +2487,7 @@ struct mm_struct *copy_init_mm(void)
> * The returned task is inactive, and the caller must fire it up
> through
> * wake_up_new_task(p). All signals are blocked in the created task.
> */
> -struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int
> node)
> +struct task_struct *create_io_thread(int (*fn)(void *), void *data,
> int node)
> {
> unsigned long flags =
> CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|
> CLONE_IO;
> @@ -2496,7 +2496,7 @@ struct task_struct *create_io_thread(int
> (*fn)(void *), void *arg, int node)
> CLONE_UNTRACED) & ~CSIGNAL),
> .exit_signal = (lower_32_bits(flags) & CSIGNAL),
> .stack = (unsigned long)fn,
> - .stack_size = (unsigned long)arg,
> + .data = data,
> .io_thread = 1,
> };
>
> @@ -2594,14 +2594,14 @@ pid_t kernel_clone(struct kernel_clone_args
> *args)
> /*
> * Create a kernel thread.
> */
> -pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long flags)
> +pid_t kernel_thread(int (*fn)(void *), void *data, 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),
> .stack = (unsigned long)fn,
> - .stack_size = (unsigned long)arg,
> + .data = data,
> };
>
> return kernel_clone(&args);
>
>
