[PATCH tip:x86/mm] x86/tls: Forcibly set the accessed bit in TLS segments

[PATCH tip:x86/mm] x86/tls: Forcibly set the accessed bit in TLS segments

[Andy Lutomirski]

For mysterious historical reasons, struct user_desc doesn't indicate
whether segments are accessed.  set_thread_area() has always
programmed segments as non-accessed, so the first write will set the
accessed bit.  This will fault if the GDT is read-only.

Fix it by making TLS segments start out accessed.

If this ends up breaking something, we could, in principle, leave
TLS segments non-accessed and fix them up when we get the page
fault.  I'd be surprised, though -- AFAIK all the nasty legacy
segmented programs (DOSEMU, Wine, things that run on DOSEMU and
Wine, etc.) do their nasty segmented things using the LDT and not
the GDT.  I assume this is mainly because old OSes (Linux and
otherwise) didn't historically provide APIs to do nasty things in
the GDT.

Fixes: 45fc8757d1d2 ("x86: Make the GDT remapping read-only on 64-bit")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Normally this would come with a test case update, but the relevant
testcase (ldt_gdt_32) currently has some issues.  I'm working on it,
but I don't want to delay this bugfix.

 arch/x86/kernel/tls.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/tls.c b/arch/x86/kernel/tls.c
index 6c8934406dc9..dcd699baea1b 100644
--- a/arch/x86/kernel/tls.c
+++ b/arch/x86/kernel/tls.c
@@ -92,10 +92,17 @@ static void set_tls_desc(struct task_struct *p, int idx,
 	cpu = get_cpu();
 
 	while (n-- > 0) {
-		if (LDT_empty(info) || LDT_zero(info))
+		if (LDT_empty(info) || LDT_zero(info)) {
 			desc->a = desc->b = 0;
-		else
+		} else {
 			fill_ldt(desc, info);
+
+			/*
+			 * Always set the accessed bit so that the CPU
+			 * doesn't try to write to the (read-only) GDT.
+			 */
+			desc->type |= 1;
+		}
 		++info;
 		++desc;
 	}
-- 
2.9.3

[tip:x86/mm] x86/tls: Forcibly set the accessed bit in TLS segments

[tip-bot for Andy Lutomirski]

Commit-ID:  5b781c7e317fcf9f74475dc82bfce2e359dfca13
Gitweb:     http://git.kernel.org/tip/5b781c7e317fcf9f74475dc82bfce2e359dfca13
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Sat, 18 Mar 2017 22:17:24 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 19 Mar 2017 12:14:35 +0100

x86/tls: Forcibly set the accessed bit in TLS segments

For mysterious historical reasons, struct user_desc doesn't indicate
whether segments are accessed.  set_thread_area() has always programmed
segments as non-accessed, so the first write will set the accessed bit.
This will fault if the GDT is read-only.

Fix it by making TLS segments start out accessed.

If this ends up breaking something, we could, in principle, leave TLS
segments non-accessed and fix them up when we get the page fault.  I'd be
surprised, though -- AFAIK all the nasty legacy segmented programs (DOSEMU,
Wine, things that run on DOSEMU and Wine, etc.) do their nasty segmented
things using the LDT and not the GDT.  I assume this is mainly because old
OSes (Linux and otherwise) didn't historically provide APIs to do nasty
things in the GDT.

Fixes: 45fc8757d1d2 ("x86: Make the GDT remapping read-only on 64-bit")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Thomas Garnier <thgarnie@google.com>
Link: http://lkml.kernel.org/r/62b7748542df0164af7e0a5231283b9b13858c45.1489900519.git.luto@kernel.org
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/x86/kernel/tls.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/tls.c b/arch/x86/kernel/tls.c
index 6c89344..dcd699b 100644
--- a/arch/x86/kernel/tls.c
+++ b/arch/x86/kernel/tls.c
@@ -92,10 +92,17 @@ static void set_tls_desc(struct task_struct *p, int idx,
 	cpu = get_cpu();
 
 	while (n-- > 0) {
-		if (LDT_empty(info) || LDT_zero(info))
+		if (LDT_empty(info) || LDT_zero(info)) {
 			desc->a = desc->b = 0;
-		else
+		} else {
 			fill_ldt(desc, info);
+
+			/*
+			 * Always set the accessed bit so that the CPU
+			 * doesn't try to write to the (read-only) GDT.
+			 */
+			desc->type |= 1;
+		}
 		++info;
 		++desc;
 	}

Re: [PATCH tip:x86/mm] x86/tls: Forcibly set the accessed bit in TLS segments

[Andy Lutomirski]

On Sat, Mar 18, 2017 at 10:17 PM, Andy Lutomirski <luto@kernel.org> wrote:
> For mysterious historical reasons, struct user_desc doesn't indicate
> whether segments are accessed.  set_thread_area() has always
> programmed segments as non-accessed, so the first write will set the
> accessed bit.  This will fault if the GDT is read-only.
>
> Fix it by making TLS segments start out accessed.
>
> If this ends up breaking something, we could, in principle, leave
> TLS segments non-accessed and fix them up when we get the page
> fault.  I'd be surprised, though -- AFAIK all the nasty legacy
> segmented programs (DOSEMU, Wine, things that run on DOSEMU and
> Wine, etc.) do their nasty segmented things using the LDT and not
> the GDT.  I assume this is mainly because old OSes (Linux and
> otherwise) didn't historically provide APIs to do nasty things in
> the GDT.
>
> Fixes: 45fc8757d1d2 ("x86: Make the GDT remapping read-only on 64-bit")
> Signed-off-by: Andy Lutomirski <luto@kernel.org>

FWIW, I'm now extra convinced that this won't break anything: the
accessed bit didn't work properly before this patch.  When we
scheduled a task in, we'd copy the TLS segment descriptors to the GDT,
but we never copied them back out when we scheduled out, so the
accessed bit would randomly clear itself.  Whoops :)

So arguably this patch would be a bugfix even without Thomas' changes.

Re: [PATCH tip:x86/mm] x86/tls: Forcibly set the accessed bit in TLS segments

[Ingo Molnar]

* Andy Lutomirski <luto@amacapital.net> wrote:

> On Sat, Mar 18, 2017 at 10:17 PM, Andy Lutomirski <luto@kernel.org> wrote:
> > For mysterious historical reasons, struct user_desc doesn't indicate
> > whether segments are accessed.  set_thread_area() has always
> > programmed segments as non-accessed, so the first write will set the
> > accessed bit.  This will fault if the GDT is read-only.
> >
> > Fix it by making TLS segments start out accessed.
> >
> > If this ends up breaking something, we could, in principle, leave
> > TLS segments non-accessed and fix them up when we get the page
> > fault.  I'd be surprised, though -- AFAIK all the nasty legacy
> > segmented programs (DOSEMU, Wine, things that run on DOSEMU and
> > Wine, etc.) do their nasty segmented things using the LDT and not
> > the GDT.  I assume this is mainly because old OSes (Linux and
> > otherwise) didn't historically provide APIs to do nasty things in
> > the GDT.
> >
> > Fixes: 45fc8757d1d2 ("x86: Make the GDT remapping read-only on 64-bit")
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> 
> FWIW, I'm now extra convinced that this won't break anything: the
> accessed bit didn't work properly before this patch.  When we
> scheduled a task in, we'd copy the TLS segment descriptors to the GDT,
> but we never copied them back out when we scheduled out, so the
> accessed bit would randomly clear itself.  Whoops :)
> 
> So arguably this patch would be a bugfix even without Thomas' changes.

It's probably even a small speedup per scheduling atom, as we'd avoid dirtying the 
GDT again and again, right?

On very high context switching rates it might even be measurable in principle, as 
this ought to be the only thing that dirtied the (per CPU) GDT cacheline, so if 
the workload is write bandwidth or store queue depth bound this change will 
slightly improve things.

Thanks,

	Ingo