2002-07-30 15:40:39

by Christoph Hellwig

[permalink] [raw]
Subject: [PATCH] sanitize TLS API

Currently sys_set_thread_area has a magic flags argument that might
change it's behaivour completly.

Split out the TLS_FLAG_CLEAR case that has nothing in common with the
rest into it's own syscall, sys_clear_thread_area and change the
second argument to int writable.


--- 1.39/arch/i386/kernel/entry.S Tue Jul 30 00:08:08 2002
+++ edited/arch/i386/kernel/entry.S Tue Jul 30 11:46:01 2002
@@ -753,6 +753,7 @@
.long sys_sched_setaffinity
.long sys_sched_getaffinity
.long sys_set_thread_area
+ .long sys_clear_thread_area

.rept NR_syscalls-(.-sys_call_table)/4
.long sys_ni_syscall
--- 1.29/arch/i386/kernel/process.c Mon Jul 29 04:07:12 2002
+++ edited/arch/i386/kernel/process.c Tue Jul 30 12:34:21 2002
@@ -831,38 +831,15 @@
/*
* Set the Thread-Local Storage area:
*/
-asmlinkage int sys_set_thread_area(unsigned long base, unsigned long flags)
+asmlinkage int sys_set_thread_area(unsigned long base, int writable)
{
struct thread_struct *t = &current->thread;
- int writable = 0;
- int cpu;
+ int cpu = get_cpu();

- /* do not allow unused flags */
- if (flags & ~TLS_FLAGS_MASK)
- return -EINVAL;
-
- /*
- * Clear the TLS?
- */
- if (flags & TLS_FLAG_CLEAR) {
- cpu = get_cpu();
- t->tls_desc.a = t->tls_desc.b = 0;
- load_TLS_desc(t, cpu);
- put_cpu();
- return 0;
- }
-
- if (flags & TLS_FLAG_WRITABLE)
- writable = 1;
-
- /*
- * We must not get preempted while modifying the TLS.
- */
- cpu = get_cpu();
+ writable = !!writable; /* must be one or zero */

- t->tls_desc.a = ((base & 0x0000ffff) << 16) | 0xffff;
-
- t->tls_desc.b = (base & 0xff000000) | ((base & 0x00ff0000) >> 16) |
+ t->tls_desc.a = ((base & 0x0000ffff) << 16) | 0xffff;
+ t->tls_desc.b = (base & 0xff000000) | ((base & 0x00ff0000) >> 16) |
0xf0000 | (writable << 9) | (1 << 15) |
(1 << 22) | (1 << 23) | 0x7000;

@@ -872,3 +849,17 @@
return TLS_ENTRY*8 + 3;
}

+
+/*
+ * Clear the Thread-Local Storage area:
+ */
+asmlinkage void sys_clear_thread_area(void)
+{
+ struct thread_struct *t = &current->thread;
+ int cpu = get_cpu();
+
+ t->tls_desc.a = t->tls_desc.b = 0;
+ load_TLS_desc(t, cpu);
+
+ put_cpu();
+}
--- 1.7/include/asm-i386/desc.h Mon Jul 29 04:07:52 2002
+++ edited/include/asm-i386/desc.h Tue Jul 30 11:48:40 2002
@@ -86,11 +86,6 @@
_set_tssldt_desc(&cpu_gdt_table[cpu][LDT_ENTRY], (int)addr, ((size << 3)-1), 0x82);
}

-#define TLS_FLAGS_MASK 0x00000003
-
-#define TLS_FLAG_WRITABLE 0x00000001
-#define TLS_FLAG_CLEAR 0x00000002
-
static inline void load_TLS_desc(struct thread_struct *t, unsigned int cpu)
{
cpu_gdt_table[cpu][TLS_ENTRY] = t->tls_desc;
--- 1.11/include/asm-i386/unistd.h Tue Jul 30 00:08:09 2002
+++ edited/include/asm-i386/unistd.h Tue Jul 30 11:47:20 2002
@@ -247,6 +247,8 @@
#define __NR_futex 240
#define __NR_sched_setaffinity 241
#define __NR_sched_getaffinity 242
+#define __NR_set_thread_area 243
+#define __NR_clear_thread_area 244

/* user-visible error numbers are in the range -1 - -124: see <asm-i386/errno.h> */


2002-07-30 18:59:12

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] sanitize TLS API


On Tue, 30 Jul 2002, Christoph Hellwig wrote:

> Currently sys_set_thread_area has a magic flags argument that might
> change it's behaivour completly.
>
> Split out the TLS_FLAG_CLEAR case that has nothing in common with the
> rest into it's own syscall, sys_clear_thread_area and change the second
> argument to int writable.

i did not feel like wasting two syscall slots for this, but the cleanups
look fine to me otherwise.

Ingo

2002-07-30 20:03:17

by Jakub Jelinek

[permalink] [raw]
Subject: Re: [PATCH] sanitize TLS API

On Tue, Jul 30, 2002 at 09:00:09PM +0200, Ingo Molnar wrote:
>
> On Tue, 30 Jul 2002, Christoph Hellwig wrote:
>
> > Currently sys_set_thread_area has a magic flags argument that might
> > change it's behaivour completly.
> >
> > Split out the TLS_FLAG_CLEAR case that has nothing in common with the
> > rest into it's own syscall, sys_clear_thread_area and change the second
> > argument to int writable.
>
> i did not feel like wasting two syscall slots for this, but the cleanups
> look fine to me otherwise.

Actually, is the clear operation really necessary?
IMHO the best clear is movw $0x03, %gs, then all accesses through %gs will
trap. Calling set_thread_area (0, 1); will result in 0xb segment
acting exactly like %ds or %es.

Jakub

2002-07-30 20:14:38

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] sanitize TLS API


On Tue, 30 Jul 2002, Jakub Jelinek wrote:

> Actually, is the clear operation really necessary? IMHO the best clear
> is movw $0x03, %gs, then all accesses through %gs will trap. Calling
> set_thread_area (0, 1); will result in 0xb segment acting exactly like
> %ds or %es.

indeed. I've attached a (tested) patch against BK-curr that removes just
the clear operation. I've left the flags mask and the writable flag just
so that we have the option to introduce extensions without breaking the
ABI.

Ingo

--- linux/arch/i386/kernel/process.c.orig Tue Jul 30 22:13:43 2002
+++ linux/arch/i386/kernel/process.c Tue Jul 30 22:13:52 2002
@@ -851,17 +851,6 @@
if (flags & ~TLS_FLAGS_MASK)
return -EINVAL;

- /*
- * Clear the TLS?
- */
- if (flags & TLS_FLAG_CLEAR) {
- cpu = get_cpu();
- t->tls_desc.a = t->tls_desc.b = 0;
- load_TLS_desc(t, cpu);
- put_cpu();
- return 0;
- }
-
if (flags & TLS_FLAG_WRITABLE)
writable = 1;

--- linux/include/asm-i386/desc.h.orig Tue Jul 30 22:14:10 2002
+++ linux/include/asm-i386/desc.h Tue Jul 30 22:14:27 2002
@@ -86,10 +86,9 @@
_set_tssldt_desc(&cpu_gdt_table[cpu][LDT_ENTRY], (int)addr, ((size << 3)-1), 0x82);
}

-#define TLS_FLAGS_MASK 0x00000003
+#define TLS_FLAGS_MASK 0x00000001

#define TLS_FLAG_WRITABLE 0x00000001
-#define TLS_FLAG_CLEAR 0x00000002

static inline void load_TLS_desc(struct thread_struct *t, unsigned int cpu)
{
--- linux/include/asm-i386/unistd.h.orig Tue Jul 30 22:14:40 2002
+++ linux/include/asm-i386/unistd.h Tue Jul 30 22:15:05 2002
@@ -247,6 +247,7 @@
#define __NR_futex 240
#define __NR_sched_setaffinity 241
#define __NR_sched_getaffinity 242
+#define __NR_set_thread_area 243

/* user-visible error numbers are in the range -1 - -124: see <asm-i386/errno.h> */


2002-07-31 08:49:35

by Andi Kleen

[permalink] [raw]
Subject: Re: [PATCH] sanitize TLS API

Jakub Jelinek <[email protected]> writes:

> Actually, is the clear operation really necessary?
> IMHO the best clear is movw $0x03, %gs, then all accesses through %gs will
> trap. Calling set_thread_area (0, 1); will result in 0xb segment
> acting exactly like %ds or %es.

At least on x86-64 it is useful to have a clear operation, because setting the
thread descriptors adds some cost to the context switch for various reasons.
This way processes you could disable it again when they don't need it anymore.

-Andi