LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] x86/fsgsbase/64: Fix the base write helper functions
@ 2018-10-30 17:27 Chang S. Bae
  2018-10-30 17:54 ` Bae, Chang Seok
  2018-10-30 21:25 ` Andy Lutomirski
  0 siblings, 2 replies; 5+ messages in thread
From: Chang S. Bae @ 2018-10-30 17:27 UTC (permalink / raw)
  To: Andy Lutomirski, Ingo Molnar, Thomas Gleixner, H . Peter Anvin
  Cc: Andi Kleen, Dave Hansen, Ravi Shankar, Chang S . Bae, LKML

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().

The helper functions that purport to write the base register should just
write the base register only. It shouldn't have magic optimizations to
change the index.

putreg() in ptrace does not write the current task, but a stopped task.

Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
---
 arch/x86/kernel/process_64.c | 67 +++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 31b4755369f0..4fd865fb7097 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -339,19 +339,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);
 }
 
@@ -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)
 		fsbase = task->thread.fsbase;
 	else
 		fsbase = x86_fsgsbase_read_task(task, task->thread.fsindex);
@@ -373,9 +363,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);
@@ -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;
-	preempt_enable();
 
 	return 0;
 }
@@ -407,12 +391,8 @@ int x86_gsbase_write_task(struct task_struct *task, unsigned long gsbase)
 	if (unlikely(gsbase >= TASK_SIZE_MAX))
 		return -EPERM;
 
-	preempt_disable();
 	task->thread.gsbase = gsbase;
-	if (task == current)
-		x86_gsbase_write_cpu_inactive(gsbase);
 	task->thread.gsindex = 0;
-	preempt_enable();
 
 	return 0;
 }
@@ -757,22 +737,53 @@ long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2)
 	int ret = 0;
 
 	switch (option) {
-	case ARCH_SET_GS: {
-		ret = x86_gsbase_write_task(task, arg2);
-		break;
-	}
 	case ARCH_SET_FS: {
+		preempt_disable();
 		ret = x86_fsbase_write_task(task, arg2);
+		if (task == current && ret == 0) {
+			/*
+			 * Set the selector to 0, implying that the base is
+			 * overwritten. The index value will be checked
+			 * during context switch for skipping segment reload.
+			 */
+			loadseg(FS, 0);
+			x86_fsbase_write_cpu(arg2);
+		}
+		preempt_enable();
+		break;
+	}
+	case ARCH_SET_GS: {
+		preempt_disable();
+		ret = x86_gsbase_write_task(task, arg2);
+		if (task == current && ret == 0) {
+			/*
+			 * Set the selector to 0 for the same reason
+			 * as %fs above.
+			 */
+			loadseg(GS, 0);
+			x86_gsbase_write_cpu_inactive(arg2);
+		}
+		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;
-- 
2.19.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] x86/fsgsbase/64: Fix the base write helper functions
  2018-10-30 17:27 [PATCH] x86/fsgsbase/64: Fix the base write helper functions Chang S. Bae
@ 2018-10-30 17:54 ` Bae, Chang Seok
  2018-10-30 21:25 ` Andy Lutomirski
  1 sibling, 0 replies; 5+ messages in thread
From: Bae, Chang Seok @ 2018-10-30 17:54 UTC (permalink / raw)
  To: Andy Lutomirski, Ingo Molnar, Thomas Gleixner, H . Peter Anvin
  Cc: Andi Kleen, Dave Hansen, Shankar, Ravi V, LKML

> 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().
> 
> The helper functions that purport to write the base register should just write the
> base register only. It shouldn't have magic optimizations to change the index.
> 
> putreg() in ptrace does not write the current task, but a stopped task.
> 

Wm, I just realized that the changelog should also clearly include this:
	"While at here, subsequently factor out the read for the current task's base 
	from x86_fsbase_read_task() and x86_gsbase_read_task() to do_arch_prctl_64()."
Sorry for this. Let me know if you feel it should go as a separate patch.

Chang

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] x86/fsgsbase/64: Fix the base write helper functions
  2018-10-30 17:27 [PATCH] x86/fsgsbase/64: Fix the base write helper functions Chang S. Bae
  2018-10-30 17:54 ` Bae, Chang Seok
@ 2018-10-30 21:25 ` Andy Lutomirski
  2018-10-31 18:50   ` Bae, Chang Seok
  1 sibling, 1 reply; 5+ messages in thread
From: Andy Lutomirski @ 2018-10-30 21:25 UTC (permalink / raw)
  To: Bae, Chang Seok
  Cc: Andrew Lutomirski, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
	Andi Kleen, Dave Hansen, Ravi V. Shankar, LKML

On Tue, Oct 30, 2018 at 10:28 AM Chang S. Bae <chang.seok.bae@intel.com> 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.

>                 fsbase = task->thread.fsbase;
>         else
>                 fsbase = x86_fsgsbase_read_task(task, task->thread.fsindex);
> @@ -373,9 +363,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)

Same here.

>                 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.

> -       preempt_enable();
>
>         return 0;
>  }
> @@ -407,12 +391,8 @@ int x86_gsbase_write_task(struct task_struct *task, unsigned long gsbase)
>         if (unlikely(gsbase >= TASK_SIZE_MAX))
>                 return -EPERM;
>
> -       preempt_disable();
>         task->thread.gsbase = gsbase;
> -       if (task == current)
> -               x86_gsbase_write_cpu_inactive(gsbase);
>         task->thread.gsindex = 0;

Same here.

> @@ -757,22 +737,53 @@ long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2)
>         int ret = 0;
>
>         switch (option) {
> -       case ARCH_SET_GS: {
> -               ret = x86_gsbase_write_task(task, arg2);
> -               break;
> -       }
>         case ARCH_SET_FS: {
> +               preempt_disable();
>                 ret = x86_fsbase_write_task(task, arg2);
> +               if (task == current && ret == 0) {
> +                       /*
> +                        * Set the selector to 0, implying that the base is
> +                        * overwritten. The index value will be checked
> +                        * during context switch for skipping segment reload.
> +                        */
> +                       loadseg(FS, 0);
> +                       x86_fsbase_write_cpu(arg2);
> +               }
> +               preempt_enable();
> +               break;
> +       }
> +       case ARCH_SET_GS: {
> +               preempt_disable();
> +               ret = x86_gsbase_write_task(task, arg2);

This would trip the warning that I'm asking you to add.  This makes me
think you should not make that change.

> +               if (task == current && ret == 0) {
> +                       /*
> +                        * Set the selector to 0 for the same reason
> +                        * as %fs above.
> +                        */
> +                       loadseg(GS, 0);
> +                       x86_gsbase_write_cpu_inactive(arg2);
> +               }

Why only if task == current?

I think this code should be:

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;
}

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] x86/fsgsbase/64: Fix the base write helper functions
  2018-10-30 21:25 ` Andy Lutomirski
@ 2018-10-31 18:50   ` Bae, Chang Seok
  0 siblings, 0 replies; 5+ messages in thread
From: Bae, Chang Seok @ 2018-10-31 18:50 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Ingo Molnar, Thomas Gleixner, H. Peter Anvin, Andi Kleen,
	Dave Hansen, Shankar, Ravi V, LKML



> On Oct 30, 2018, at 14:25, Andy Lutomirski <luto@kernel.org> wrote:
> 
> On Tue, Oct 30, 2018 at 10:28 AM Chang S. Bae <chang.seok.bae@intel.com> 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] x86/fsgsbase/64: Fix the base write helper functions
@ 2018-10-31 18:49 Chang S. Bae
  0 siblings, 0 replies; 5+ messages in thread
From: Chang S. Bae @ 2018-10-31 18:49 UTC (permalink / raw)
  To: Andy Lutomirski, Ingo Molnar, Thomas Gleixner, H . Peter Anvin
  Cc: Andi Kleen, Dave Hansen, Ravi Shankar, Chang S . Bae, LKML

Factor out the code to change index from the relavant helpers. Now,
the code is located in do_arch_prctl_64().

The helper functions that purport to write the base should just write it
only. It shouldn't have magic optimizations to change the index.

putreg() in ptrace does not write the current task, but a stopped task.

v2: Fix further on the task write functions. Revert the changes on the
task read helpers.

Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
---
 arch/x86/kernel/process_64.c | 48 ++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 31b4755369f0..ad849ce9cb73 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -339,19 +339,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);
 }
 
@@ -392,12 +384,7 @@ 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;
-	preempt_enable();
 
 	return 0;
 }
@@ -407,12 +394,7 @@ int x86_gsbase_write_task(struct task_struct *task, unsigned long gsbase)
 	if (unlikely(gsbase >= TASK_SIZE_MAX))
 		return -EPERM;
 
-	preempt_disable();
 	task->thread.gsbase = gsbase;
-	if (task == current)
-		x86_gsbase_write_cpu_inactive(gsbase);
-	task->thread.gsindex = 0;
-	preempt_enable();
 
 	return 0;
 }
@@ -758,11 +740,41 @@ long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2)
 
 	switch (option) {
 	case ARCH_SET_GS: {
+		preempt_disable();
 		ret = x86_gsbase_write_task(task, arg2);
+		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);
+				x86_gsbase_write_cpu_inactive(arg2);
+			} else {
+				task->thread.gsindex = 0;
+			}
+		}
+		preempt_enable();
 		break;
 	}
 	case ARCH_SET_FS: {
+		preempt_disable();
 		ret = x86_fsbase_write_task(task, arg2);
+		if (ret == 0) {
+			/*
+			 * Set the selector to 0 for the same reason
+			 * as %gs above.
+			 */
+			if (task == current) {
+				loadseg(FS, 0);
+				x86_fsbase_write_cpu(arg2);
+			} else {
+				task->thread.fsindex = 0;
+			}
+		}
+		preempt_enable();
 		break;
 	}
 	case ARCH_GET_FS: {
-- 
2.19.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-10-31 18:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-30 17:27 [PATCH] x86/fsgsbase/64: Fix the base write helper functions Chang S. Bae
2018-10-30 17:54 ` Bae, Chang Seok
2018-10-30 21:25 ` Andy Lutomirski
2018-10-31 18:50   ` Bae, Chang Seok
2018-10-31 18:49 Chang S. Bae

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).