Re: [PATCH] x86/fsgsbase/64: Fix the base write helper functions

[Bae, Chang Seok]

> On Oct 30, 2018, at 14:25, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
> 
> On Tue, Oct 30, 2018 at 10:28 AM Chang S. Bae <chang.seok.bae@xxxxxxxxx> wrote:
>> 
>> Factor out the code to change index from x86_fsbase_write_cpu() and
>> x86_gsbase_write_cpu_inactive(). Now the code is located in
>> do_arch_prctl_64().
>> 
> 
>> @@ -359,9 +351,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)
> 
> I'm okay with this change but, if you do it, please add:
> 
> WARN_ON_ONCE(task == current);
> 
> and make it be in a separate patch.
> 

Okay. Let me unchange those read functions, as it does what is claimed to do.

>>                gsbase = task->thread.gsbase;
>>        else
>>                gsbase = x86_fsgsbase_read_task(task, task->thread.gsindex);
>> @@ -392,12 +380,8 @@ int x86_fsbase_write_task(struct task_struct *task, unsigned long fsbase)
>>        if (unlikely(fsbase >= TASK_SIZE_MAX))
>>                return -EPERM;
>> 
>> -       preempt_disable();
>>        task->thread.fsbase = fsbase;
>> -       if (task == current)
>> -               x86_fsbase_write_cpu(fsbase);
>>        task->thread.fsindex = 0;
> 
> I'm confused.  You're still setting fsindex to zero here.
> 

These task write functions are getting thiner, although I agree
the index update should move out.

> ret = x86_gsbase_write_task(task, arg);
> if (ret == 0) {
>  /* ARCH_SET_GS has always overwritten the index and the base.  Zero
> is the most sensible value to put in the index, and is the only value
> that makes any sense if FSGSBASE is unavailable. */
>  if (task == current)
>    loadseg(GS, 0);
>  else
>    task->thread.gsindex = 0;
> }

Thank you for the clarification. I think writing base  should come right after loadseg().

Chang