| Message ID | 20220130211838.8382-27-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadow stacks for userspace | expand |
On Sun, Jan 30 2022 at 13:18, Rick Edgecombe wrote: > -int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg, > - struct task_struct *p, unsigned long tls) > +int copy_thread(unsigned long clone_flags, unsigned long sp, > + unsigned long stack_size, struct task_struct *p, > + unsigned long tls) > { > struct inactive_task_frame *frame; > struct fork_frame *fork_frame; > @@ -175,7 +176,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg, > 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, arg); > + kthread_frame_init(frame, sp, stack_size); > return 0; > } > > @@ -208,7 +209,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg, > */ > childregs->sp = 0; > childregs->ip = 0; > - kthread_frame_init(frame, sp, arg); > + kthread_frame_init(frame, sp, stack_size); > return 0; > } Can you please change the prototypes too for completeness sake? Thanks, tglx
On Tue, 2022-02-08 at 09:38 +0100, Thomas Gleixner wrote: > On Sun, Jan 30 2022 at 13:18, Rick Edgecombe wrote: > > -int copy_thread(unsigned long clone_flags, unsigned long sp, > > unsigned long arg, > > - struct task_struct *p, unsigned long tls) > > +int copy_thread(unsigned long clone_flags, unsigned long sp, > > + unsigned long stack_size, struct task_struct *p, > > + unsigned long tls) > > { > > struct inactive_task_frame *frame; > > struct fork_frame *fork_frame; > > @@ -175,7 +176,7 @@ int copy_thread(unsigned long clone_flags, > > unsigned long sp, unsigned long arg, > > 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, arg); > > + kthread_frame_init(frame, sp, stack_size); > > return 0; > > } > > > > @@ -208,7 +209,7 @@ int copy_thread(unsigned long clone_flags, > > unsigned long sp, unsigned long arg, > > */ > > childregs->sp = 0; > > childregs->ip = 0; > > - kthread_frame_init(frame, sp, arg); > > + kthread_frame_init(frame, sp, stack_size); > > return 0; > > } > > Can you please change the prototypes too for completeness sake? In the header it's: extern int copy_thread(unsigned long, unsigned long, unsigned long, struct task_struct *, unsigned long); And the various arch implementations call the stack size: arg, kthread_arg, stk_sz, etc. Adding names to the prototype would conflict with the some arch's names unless they were all unified. Is it a worthwhile refactor?
On Sun, Jan 30, 2022 at 10:22 PM Rick Edgecombe <rick.p.edgecombe@intel.com> wrote: > > From: Yu-cheng Yu <yu-cheng.yu@intel.com> > > 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.
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@intel.com> wrote: > > > > From: Yu-cheng Yu <yu-cheng.yu@intel.com> > > > > 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);
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@intel.com> wrote: > > > > > > From: Yu-cheng Yu <yu-cheng.yu@intel.com> > > > > > > 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); > >
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c index 81d8ef036637..82a816178e7f 100644 --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c @@ -130,8 +130,9 @@ 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 arg, - struct task_struct *p, unsigned long tls) +int copy_thread(unsigned long clone_flags, unsigned long sp, + unsigned long stack_size, struct task_struct *p, + unsigned long tls) { struct inactive_task_frame *frame; struct fork_frame *fork_frame; @@ -175,7 +176,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg, 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, arg); + kthread_frame_init(frame, sp, stack_size); return 0; } @@ -208,7 +209,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg, */ childregs->sp = 0; childregs->ip = 0; - kthread_frame_init(frame, sp, arg); + kthread_frame_init(frame, sp, stack_size); return 0; }