X86_64 BUG: missing FS/GS LDT reload on fork()

X86_64 BUG: missing FS/GS LDT reload on fork()

[Samuel Thibault]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

Hello,

I have an issue with FS/GS LDT reload in the child of fork(). The
attached testcase fails quite often.  It sets an LDT entry up, uses
prctl to set gs's base to a 64bit value, then loads gs with the LDT
entry. The LDT entry is now in effect. After a fork call, the LDT entry
is not in effect any more, the 64bit base is back!

It can be noticed that setting a 32bit base doesn't hurt, and enabling a
small nanosleep makes it work (I guess due to the induced save/restore
cycle).

I guess there's something bogus in the context save/load cycle across
fork().

This is vanilla 2.6.33 with the cpu below, but it also fails with a
2.6.32, 2.6.30, 2.6.27, and a 2.6.18 on various 64bit CPUs.

processor	: 0
vendor_id	: GenuineIntel
cpu family	: 6
model		: 15
model name	: Intel(R) Core(TM)2 Duo CPU     U7700  @ 1.33GHz
stepping	: 13
cpu MHz		: 800.000
cache size	: 2048 KB
physical id	: 0
siblings	: 2
core id		: 0
cpu cores	: 2
apicid		: 0
initial apicid	: 0
fpu		: yes
fpu_exception	: yes
cpuid level	: 10
wp		: yes
flags		: fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm constant_tsc arch_perfmon pebs bts rep_good aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm tpr_shadow vnmi flexpriority
bogomips	: 2660.22
clflush size	: 64
cache_alignment	: 64
address sizes	: 36 bits physical, 48 bits virtual
power management:

processor	: 1
vendor_id	: GenuineIntel
cpu family	: 6
model		: 15
model name	: Intel(R) Core(TM)2 Duo CPU     U7700  @ 1.33GHz
stepping	: 13
cpu MHz		: 800.000
cache size	: 2048 KB
physical id	: 0
siblings	: 2
core id		: 1
cpu cores	: 2
apicid		: 1
initial apicid	: 1
fpu		: yes
fpu_exception	: yes
cpuid level	: 10
wp		: yes
flags		: fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm constant_tsc arch_perfmon pebs bts rep_good aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm tpr_shadow vnmi flexpriority
bogomips	: 2660.03
clflush size	: 64
cache_alignment	: 64
address sizes	: 36 bits physical, 48 bits virtual
power management:


Samuel

[-- Attachment #2: test.c --]
[-- Type: text/plain, Size: 1378 bytes --]

#define _GNU_SOURCE
#include <stdio.h>
#include <sys/syscall.h>
#include <asm/prctl.h>
#include <asm/ldt.h>
#include <sys/types.h>
#include <stdint.h>
#include <stdlib.h>

int var = 9;
int status = 0;

void print_base(char *who) {
	unsigned long base;
	int val;
	syscall(SYS_arch_prctl, ARCH_GET_GS, &base);
	asm("movl %%gs:0,%0":"=r"(val));
	printf("%s:\tbase %16lx val %d var %p\n", who, base, val, &var);
	if (val != var)
		status = 1;
}

int main(int argc, char *argv[]) {
	unsigned short entry = 1;
	unsigned short selector = (entry*8) | 0x4;
	struct user_desc desc = {
		.entry_number = entry,
		.base_addr = (unsigned) (uintptr_t) &var,
		.limit = 0xfffffffful,
		.contents = MODIFY_LDT_CONTENTS_DATA,
		.read_exec_only = 0,
		.limit_in_pages = 1,
		.seg_not_present = 0,
		.useable = 1,
	};
	pid_t pid;
	int i;

	if (syscall(SYS_modify_ldt, 0x11, &desc, sizeof(desc)))
		perror("modify_ldt");

#if 1
	syscall(SYS_arch_prctl, ARCH_SET_GS, &argc);
#else
	syscall(SYS_arch_prctl, ARCH_SET_GS, &status);
#endif
	asm volatile("movw %w0,%%gs"::"q"(selector));
	print_base("parent");

#if 0
	{
		struct timespec ts = {0, 1000000};
		nanosleep(&ts, NULL);
	}
#endif
	pid = syscall(SYS_fork);
	print_base(pid ? "parent" : "child");
	asm volatile("movw %w0,%%gs"::"q"(selector));
	print_base(pid ? "parent" : "child");
	if (pid)
		waitpid(pid, &status, 0);
	return status != 0;
}

Re: X86_64 BUG: missing FS/GS LDT reload on fork()

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On 04/23/2010 10:04 AM, Samuel Thibault wrote:
> Hello,
> 
> I have an issue with FS/GS LDT reload in the child of fork(). The
> attached testcase fails quite often.  It sets an LDT entry up, uses
> prctl to set gs's base to a 64bit value, then loads gs with the LDT
> entry. The LDT entry is now in effect. After a fork call, the LDT entry
> is not in effect any more, the 64bit base is back!
> 

Okay... I have to say that I'm more than a bit confused why you're doing
this, but the __switch_no code in process_64.c has the following:

                /*
                 * Check if the user used a selector != 0; if yes
                 *  clear 64bit base, since overloaded base is always
                 *  mapped to the Null selector
                 */
                if (fsindex)
                        prev->fs = 0;

[and the same for gs]

However, copy_thread() doesn't have the equivalent code, and __switch_to
clearly expects that to be maintained as an invariant -- it doesn't
check on entry, only on exit.

The following patch looks like it should address that.

	-hpa

[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 652 bytes --]

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index dc9690b..17cb329 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -276,12 +276,12 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
 
 	set_tsk_thread_flag(p, TIF_FORK);
 
-	p->thread.fs = me->thread.fs;
-	p->thread.gs = me->thread.gs;
 	p->thread.io_bitmap_ptr = NULL;
 
 	savesegment(gs, p->thread.gsindex);
+	p->thread.gs = p->thread.gsindex ? 0 : me->thread.gs;
 	savesegment(fs, p->thread.fsindex);
+	p->thread.fs = p->thread.fsindex ? 0 : me->thread.fs;
 	savesegment(es, p->thread.es);
 	savesegment(ds, p->thread.ds);
 

Re: X86_64 BUG: missing FS/GS LDT reload on fork()

[Samuel Thibault]

Hello,

H. Peter Anvin, le Fri 23 Apr 2010 11:01:05 -0700, a écrit :
> On 04/23/2010 10:04 AM, Samuel Thibault wrote:
> > I have an issue with FS/GS LDT reload in the child of fork(). The
> > attached testcase fails quite often.  It sets an LDT entry up, uses
> > prctl to set gs's base to a 64bit value, then loads gs with the LDT
> > entry. The LDT entry is now in effect. After a fork call, the LDT entry
> > is not in effect any more, the 64bit base is back!
> 
> Okay... I have to say that I'm more than a bit confused why you're doing
> this,

:D

I'm fixing a user-level thread library with TLS support.

> The following patch looks like it should address that.

Indeed, it fixes the issue here.

Samuel

[tip:x86/urgent] x86-64: Clear a 64-bit FS/GS base on fork if selector is nonzero

[tip-bot for H. Peter Anvin]

Commit-ID:  3e72c448927da6117007d402637b4dbd6b55998d
Gitweb:     http://git.kernel.org/tip/3e72c448927da6117007d402637b4dbd6b55998d
Author:     H. Peter Anvin <hpa@zytor.com>
AuthorDate: Fri, 23 Apr 2010 16:17:40 -0700
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Fri, 23 Apr 2010 16:36:09 -0700

x86-64: Clear a 64-bit FS/GS base on fork if selector is nonzero

When we do a thread switch, we clear the outgoing FS/GS base if the
corresponding selector is nonzero.  This is taken by __switch_to() as
an entry invariant; it does not verify that it is true on entry.
However, copy_thread() doesn't enforce this constraint, which can
result in inconsistent results after fork().

Make copy_thread() match the behavior of __switch_to().

Reported-and-tested-by: Samuel Thibault <samuel.thibault@inria.fr>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
LKML-Reference: <4BD1E061.8030605@zytor.com>
---
 arch/x86/kernel/process_64.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index dc9690b..17cb329 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -276,12 +276,12 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
 
 	set_tsk_thread_flag(p, TIF_FORK);
 
-	p->thread.fs = me->thread.fs;
-	p->thread.gs = me->thread.gs;
 	p->thread.io_bitmap_ptr = NULL;
 
 	savesegment(gs, p->thread.gsindex);
+	p->thread.gs = p->thread.gsindex ? 0 : me->thread.gs;
 	savesegment(fs, p->thread.fsindex);
+	p->thread.fs = p->thread.fsindex ? 0 : me->thread.fs;
 	savesegment(es, p->thread.es);
 	savesegment(ds, p->thread.ds);
 

[tip:x86/urgent] x86-64: Clear a 64-bit FS/GS base on fork if selector is nonzero

[tip-bot for H. Peter Anvin]

Commit-ID:  7ce5a2b9bb2e92902230e3121d8c3047fab9cb47
Gitweb:     http://git.kernel.org/tip/7ce5a2b9bb2e92902230e3121d8c3047fab9cb47
Author:     H. Peter Anvin <hpa@zytor.com>
AuthorDate: Fri, 23 Apr 2010 16:17:40 -0700
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Fri, 23 Apr 2010 16:49:51 -0700

x86-64: Clear a 64-bit FS/GS base on fork if selector is nonzero

When we do a thread switch, we clear the outgoing FS/GS base if the
corresponding selector is nonzero.  This is taken by __switch_to() as
an entry invariant; it does not verify that it is true on entry.
However, copy_thread() doesn't enforce this constraint, which can
result in inconsistent results after fork().

Make copy_thread() match the behavior of __switch_to().

Reported-and-tested-by: Samuel Thibault <samuel.thibault@inria.fr>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
LKML-Reference: <4BD1E061.8030605@zytor.com>
Cc: <stable@kernel.org>
---
 arch/x86/kernel/process_64.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index dc9690b..17cb329 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -276,12 +276,12 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
 
 	set_tsk_thread_flag(p, TIF_FORK);
 
-	p->thread.fs = me->thread.fs;
-	p->thread.gs = me->thread.gs;
 	p->thread.io_bitmap_ptr = NULL;
 
 	savesegment(gs, p->thread.gsindex);
+	p->thread.gs = p->thread.gsindex ? 0 : me->thread.gs;
 	savesegment(fs, p->thread.fsindex);
+	p->thread.fs = p->thread.fsindex ? 0 : me->thread.fs;
 	savesegment(es, p->thread.es);
 	savesegment(ds, p->thread.ds);