Messages in this thread Patch in this message | | | From | "Bae, Chang Seok" <> | Subject | RE: [regression in -rc1] Re: [PATCH v6 2/8] x86/fsgsbase/64: Introduce FS/GS base helper functions | Date | Wed, 24 Oct 2018 22:50:19 +0000 |
| |
On Wed, Oct 24, 2018 at 12:43 PM Andy Lutomirski <luto@kernel.org> wrote: > I think x86_fsbase_write_task() makes plenty of sense, but I think > that callers need to be aware that the effect of writing a nonzero > fsbase *and* a nonzero fsindex is bizarre on non-FSGSBASE systems. So > that code should go in the callers. The oddities involved have little > to do with whether the caller is writing to current or to something > else. > > Arguably the code should be entirely split out into the code that > writes current (arch_prctl()) and the code that writes a stopped task > (ptrace). I don't think there are any code paths that genuinely can > write either. >
Can you check this patch is close to what in your mind?
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c index d6674a425714..5f986e15842e 100644 --- a/arch/x86/kernel/process_64.c +++ b/arch/x86/kernel/process_64.c @@ -341,19 +341,11 @@ static unsigned long x86_fsgsbase_read_task(struct task_struct *task, void x86_fsbase_write_cpu(unsigned long fsbase) { - /* - * Set the selector to 0 as a notion, that the segment base is - * overwritten, which will be checked for skipping the segment load - * during context switch. - */ - loadseg(FS, 0); wrmsrl(MSR_FS_BASE, fsbase); } void x86_gsbase_write_cpu_inactive(unsigned long gsbase) { - /* Set the selector to 0 for the same reason as %fs above. */ - loadseg(GS, 0); wrmsrl(MSR_KERNEL_GS_BASE, gsbase); } @@ -361,9 +353,7 @@ unsigned long x86_fsbase_read_task(struct task_struct *task) { unsigned long fsbase; - if (task == current) - fsbase = x86_fsbase_read_cpu(); - else if (task->thread.fsindex == 0) + if (task->thread.fsindex == 0) fsbase = task->thread.fsbase; else fsbase = x86_fsgsbase_read_task(task, task->thread.fsindex); @@ -375,9 +365,7 @@ unsigned long x86_gsbase_read_task(struct task_struct *task) { unsigned long gsbase; - if (task == current) - gsbase = x86_gsbase_read_cpu_inactive(); - else if (task->thread.gsindex == 0) + if (task->thread.gsindex == 0) gsbase = task->thread.gsbase; else gsbase = x86_fsgsbase_read_task(task, task->thread.gsindex); @@ -396,8 +384,6 @@ int x86_fsbase_write_task(struct task_struct *task, unsigned long fsbase) preempt_disable(); task->thread.fsbase = fsbase; - if (task == current) - x86_fsbase_write_cpu(fsbase); task->thread.fsindex = 0; preempt_enable(); @@ -411,8 +397,6 @@ int x86_gsbase_write_task(struct task_struct *task, unsigned long gsbase) preempt_disable(); task->thread.gsbase = gsbase; - if (task == current) - x86_gsbase_write_cpu_inactive(gsbase); task->thread.gsindex = 0; preempt_enable(); @@ -761,20 +745,42 @@ long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2) switch (option) { case ARCH_SET_GS: { ret = x86_gsbase_write_task(task, arg2); + if (task == current && ret == 0) { + preempt_disable(); + loadseg(GS, 0); + x86_gsbase_write_cpu_inactive(); + preempt_enable(); + } break; } case ARCH_SET_FS: { ret = x86_fsbase_write_task(task, arg2); + if (task == current && ret == 0) { + preempt_disable(); + loadseg(FS, 0); + x86_fsbase_write_cpu(); + preempt_enable(); + } break; } case ARCH_GET_FS: { - unsigned long base = x86_fsbase_read_task(task); + unsigned long base; + + if (task == current) + base = x86_fsbase_read_cpu(); + else + base = x86_fsbase_read_task(task); ret = put_user(base, (unsigned long __user *)arg2); break; } case ARCH_GET_GS: { - unsigned long base = x86_gsbase_read_task(task); + unsigned long base; + + if (task == current) + base = x86_gsbase_read_cpu_inactive(); + else + base = x86_gsbase_read_task(task); ret = put_user(base, (unsigned long __user *)arg2); break; | |