Re: [PATCH 26/35] x86/process: Change copy_thread() argument 'arg' to 'stack_size'
From: Edgecombe, Rick P
Date: Mon Feb 14 2022 - 20:22:22 EST
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.
But copy_thread() has a ton of arch specific implementations. So they
would all need to be updated to do this.
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);