Re: [patch 2/9] x86/process: Unify copy_thread_tls()

Andy Lutomirski
2019-11-08T22:31:53Z

On 11/6/19 11:35 AM, Thomas Gleixner wrote:
> While looking at the TSS io bitmap it turned out that any change in that
> area would require identical changes to copy_thread_tls(). The 32 and 64
> bit variants share sufficient code to consolidate them into a common
> function to avoid duplication of upcoming modifications.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> arch/x86/include/asm/ptrace.h | 6 ++
> arch/x86/include/asm/switch_to.h | 10 ++++
> arch/x86/kernel/process.c | 94 +++++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/process_32.c | 68 ----------------------------
> arch/x86/kernel/process_64.c | 75 -------------------------------
> 5 files changed, 110 insertions(+), 143 deletions(-)
>
> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -361,5 +361,11 @@ extern int do_get_thread_area(struct tas
> extern int do_set_thread_area(struct task_struct *p, int idx,
> struct user_desc __user *info, int can_allocate);
>
> +#ifdef CONFIG_X86_64
> +# define do_set_thread_area_64(p, s, t) do_arch_prctl_64(p, s, t)
> +#else
> +# define do_set_thread_area_64(p, s, t) (0)
> +#endif
> +
> #endif /* !__ASSEMBLY__ */
> #endif /* _ASM_X86_PTRACE_H */
> --- a/arch/x86/include/asm/switch_to.h
> +++ b/arch/x86/include/asm/switch_to.h
> @@ -103,7 +103,17 @@ static inline void update_task_stack(str
> if (static_cpu_has(X86_FEATURE_XENPV))
> load_sp0(task_top_of_stack(task));
> #endif
> +}
>
> +static inline void kthread_frame_init(struct inactive_task_frame *frame,
> + unsigned long fun, unsigned long arg)
> +{
> + frame->bx = fun;
> +#ifdef CONFIG_X86_32
> + frame->di = arg;
> +#else
> + frame->r12 = arg;
> +#endif
> }
>
> #endif /* _ASM_X86_SWITCH_TO_H */
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -132,6 +132,100 @@ void exit_thread(struct task_struct *tsk
> fpu__drop(fpu);
> }
>
> +static int set_new_tls(struct task_struct *p, unsigned long tls)
> +{
> + struct user_desc __user *utls = (struct user_desc __user *)tls;
> +
> + if (in_ia32_syscall())
> + return do_set_thread_area(p, -1, utls, 0);
> + else
> + return do_set_thread_area_64(p, ARCH_SET_FS, tls);
> +}
> +
> +static inline int copy_io_bitmap(struct task_struct *tsk)
> +{
> + if (likely(!test_tsk_thread_flag(current, TIF_IO_BITMAP)))
> + return 0;
> +
> + tsk->thread.io_bitmap_ptr = kmemdup(current->thread.io_bitmap_ptr,
> + IO_BITMAP_BYTES, GFP_KERNEL);

tsk->thread.io_bitmap_max = current->thread.io_bitmap_max?

I realize you inherited this from the code you're refactoring, but it
does seem to be missing.

> + if (!tsk->thread.io_bitmap_ptr) {
> + tsk->thread.io_bitmap_max = 0;
> + return -ENOMEM;
> + }
> + set_tsk_thread_flag(tsk, TIF_IO_BITMAP);
> + return 0;
> +}
> +
> +static inline void free_io_bitmap(struct task_struct *tsk)
> +{
> + if (tsk->thread.io_bitmap_ptr) {
> + kfree(tsk->thread.io_bitmap_ptr);
> + tsk->thread.io_bitmap_ptr = NULL;
> + tsk->thread.io_bitmap_max = 0;
> + }
> +}
> +
> +int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
> + unsigned long arg, struct task_struct *p, unsigned long tls)
> +{
> + struct inactive_task_frame *frame;
> + struct fork_frame *fork_frame;
> + struct pt_regs *childregs;
> + int ret;
> +
> + childregs = task_pt_regs(p);
> + fork_frame = container_of(childregs, struct fork_frame, regs);
> + frame = &fork_frame->frame;
> +
> + frame->bp = 0;
> + frame->ret_addr = (unsigned long) ret_from_fork;
> + p->thread.sp = (unsigned long) fork_frame;
> + p->thread.io_bitmap_ptr = NULL;
> + memset(p->thread.ptrace_bps, 0, sizeof(p->thread.ptrace_bps));
> +
> +#ifdef CONFIG_X86_64
> + savesegment(gs, p->thread.gsindex);
> + p->thread.gsbase = p->thread.gsindex ? 0 : current->thread.gsbase;
> + savesegment(fs, p->thread.fsindex);
> + p->thread.fsbase = p->thread.fsindex ? 0 : current->thread.fsbase;
> + savesegment(es, p->thread.es);
> + savesegment(ds, p->thread.ds);
> +#else
> + /* Clear all status flags including IF and set fixed bit. */
> + frame->flags = X86_EFLAGS_FIXED;
> +#endif

Want to do another commit to make the eflags fixup unconditional? I'm
wondering if we have a bug.

Other than these questions:

Acked-by: Andy Lutomirski <luto@kernel.org>