Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCHv3] arm: Preserve the user r/w register TPIDRURW on context switch and fork

14 views
Skip to first unread message

André Hentschel

unread,
May 7, 2013, 4:51:28 PM5/7/13
to linux...@vger.kernel.org, Russell King - ARM Linux, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, gre...@linuxfoundation.org, Will Deacon, Jonathan Austin
From: =?UTF-8?q?Andr=C3=A9=20Hentschel?= <ne...@dawncrow.de>

Since commit 6a1c53124aa1 the user writeable TLS register was zeroed to
prevent it from being used as a covert channel between two tasks.

There are more and more applications coming to WinRT, Wine could support them,
but mostly they expect to have the thread environment block (TEB) in TPIDRURW.

This patch preserves that register per thread instead of clearing it.
Unlike the TPIDRURO, which is already switched, the TPIDRURW
can be updated from userspace so needs careful treatment in the case that we
modify TPIDRURW and call fork(). To avoid this we must always read
TPIDRURW in copy_thread.

Signed-off-by: Andrᅵ Hentschel <ne...@dawncrow.de>
Signed-off-by: Will Deacon <will....@arm.com>
Signed-off-by: Jonathan Austin <jonatha...@arm.com>

---
This patch is against a86d52667d8eda5de39393ce737794403bdce1eb

I could only test it with kernel 3.4.6, beside using Wine for testing i also
used https://github.com/AndreRH/tpidrurw-test

Why so much Signed-off-bys? Some History:
The first patch had performance issues pointed out by Russel King,
so Will Deacon jumped in to help me with that. The second one again
had performance issues and the missing copy_thread part was uncovered.
After some iterations by me, Jonathan Austin proposed a patch and
Russel King sent his idea of the assembler part. All this was finally
merged and refined into this patch.
Thanks to everyone!

arch/arm/include/asm/thread_info.h | 2 +-
arch/arm/include/asm/tls.h | 41 ++++++++++++++++++++++++++++-------------
arch/arm/kernel/entry-armv.S | 4 ++--
arch/arm/kernel/process.c | 4 +++-
arch/arm/kernel/ptrace.c | 2 +-
arch/arm/kernel/traps.c | 4 ++--
6 files changed, 37 insertions(+), 20 deletions(-)



diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index cddda1f..d90be6d 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -58,7 +58,7 @@ struct thread_info {
struct cpu_context_save cpu_context; /* cpu context */
__u32 syscall; /* syscall number */
__u8 used_cp[16]; /* thread used copro */
- unsigned long tp_value;
+ unsigned long tp_value[2]; /* TLS registers */
#ifdef CONFIG_CRUNCH
struct crunch_state crunchstate;
#endif
diff --git a/arch/arm/include/asm/tls.h b/arch/arm/include/asm/tls.h
index 73409e6..22756ab 100644
--- a/arch/arm/include/asm/tls.h
+++ b/arch/arm/include/asm/tls.h
@@ -2,27 +2,30 @@
#define __ASMARM_TLS_H

#ifdef __ASSEMBLY__
- .macro set_tls_none, tp, tmp1, tmp2
+#include <asm/asm-offsets.h>
+ .macro switch_tls_none, base, tp, tpuser, tmp1, tmp2
.endm

- .macro set_tls_v6k, tp, tmp1, tmp2
+ .macro switch_tls_v6k, base, tp, tpuser, tmp1, tmp2
+ mrc p15, 0, \tmp2, c13, c0, 2 @ get the user r/w register
mcr p15, 0, \tp, c13, c0, 3 @ set TLS register
- mov \tmp1, #0
- mcr p15, 0, \tmp1, c13, c0, 2 @ clear user r/w TLS register
+ mcr p15, 0, \tpuser, c13, c0, 2 @ and the user r/w register
+ strne \tmp2, [\base, #TI_TP_VALUE + 4] @ save it
.endm

- .macro set_tls_v6, tp, tmp1, tmp2
+ .macro switch_tls_v6, base, tp, tpuser, tmp1, tmp2
ldr \tmp1, =elf_hwcap
ldr \tmp1, [\tmp1, #0]
mov \tmp2, #0xffff0fff
tst \tmp1, #HWCAP_TLS @ hardware TLS available?
- mcrne p15, 0, \tp, c13, c0, 3 @ yes, set TLS register
- movne \tmp1, #0
- mcrne p15, 0, \tmp1, c13, c0, 2 @ clear user r/w TLS register
streq \tp, [\tmp2, #-15] @ set TLS value at 0xffff0ff0
+ mrcne p15, 0, \tmp2, c13, c0, 2 @ get the user r/w register
+ mcrne p15, 0, \tp, c13, c0, 3 @ yes, set TLS register
+ mcrne p15, 0, \tpuser, c13, c0, 2 @ set user r/w register
+ strne \tmp2, [\base, #TI_TP_VALUE + 4] @ save it
.endm

- .macro set_tls_software, tp, tmp1, tmp2
+ .macro switch_tls_software, base, tp, tpuser, tmp1, tmp2
mov \tmp1, #0xffff0fff
str \tp, [\tmp1, #-15] @ set TLS value at 0xffff0ff0
.endm
@@ -31,19 +34,31 @@
#ifdef CONFIG_TLS_REG_EMUL
#define tls_emu 1
#define has_tls_reg 1
-#define set_tls set_tls_none
+#define switch_tls switch_tls_none
#elif defined(CONFIG_CPU_V6)
#define tls_emu 0
#define has_tls_reg (elf_hwcap & HWCAP_TLS)
-#define set_tls set_tls_v6
+#define switch_tls switch_tls_v6
#elif defined(CONFIG_CPU_32v6K)
#define tls_emu 0
#define has_tls_reg 1
-#define set_tls set_tls_v6k
+#define switch_tls switch_tls_v6k
#else
#define tls_emu 0
#define has_tls_reg 0
-#define set_tls set_tls_software
+#define switch_tls switch_tls_software
#endif

+#ifndef __ASSEMBLY__
+static inline unsigned long get_tlsuser(void)
+{
+ if (has_tls_reg && !tls_emu)
+ {
+ unsigned long t;
+ __asm__("mrc p15, 0, %0, c13, c0, 2" : "=r" (t));
+ return t;
+ }
+ return 0;
+}
+#endif
#endif /* __ASMARM_TLS_H */
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 0f82098..80f09fe 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -728,15 +728,15 @@ ENTRY(__switch_to)
UNWIND(.fnstart )
UNWIND(.cantunwind )
add ip, r1, #TI_CPU_SAVE
- ldr r3, [r2, #TI_TP_VALUE]
ARM( stmia ip!, {r4 - sl, fp, sp, lr} ) @ Store most regs on stack
THUMB( stmia ip!, {r4 - sl, fp} ) @ Store most regs on stack
THUMB( str sp, [ip], #4 )
THUMB( str lr, [ip], #4 )
+ ldrd r4, r5, [r2, #TI_TP_VALUE]
#ifdef CONFIG_CPU_USE_DOMAINS
ldr r6, [r2, #TI_CPU_DOMAIN]
#endif
- set_tls r3, r4, r5
+ switch_tls r1, r4, r5, r3, r7
#if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
ldr r7, [r2, #TI_TASK]
ldr r8, =__stack_chk_guard
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 047d3e4..24dbc72 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -39,6 +39,7 @@
#include <asm/thread_notify.h>
#include <asm/stacktrace.h>
#include <asm/mach/time.h>
+#include <asm/tls.h>

#ifdef CONFIG_CC_STACKPROTECTOR
#include <linux/stackprotector.h>
@@ -395,7 +396,8 @@ copy_thread(unsigned long clone_flags, unsigned long stack_start,
clear_ptrace_hw_breakpoint(p);

if (clone_flags & CLONE_SETTLS)
- thread->tp_value = childregs->ARM_r3;
+ thread->tp_value[0] = childregs->ARM_r3;
+ thread->tp_value[1] = get_tlsuser();

thread_notify(THREAD_NOTIFY_COPY, thread);

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 03deeff..2bc1514 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -849,7 +849,7 @@ long arch_ptrace(struct task_struct *child, long request,
#endif

case PTRACE_GET_THREAD_AREA:
- ret = put_user(task_thread_info(child)->tp_value,
+ ret = put_user(task_thread_info(child)->tp_value[0],
datap);
break;

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 1c08911..f9d6259 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -588,7 +588,7 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
return regs->ARM_r0;

case NR(set_tls):
- thread->tp_value = regs->ARM_r0;
+ thread->tp_value[0] = regs->ARM_r0;
if (tls_emu)
return 0;
if (has_tls_reg) {
@@ -706,7 +706,7 @@ static int get_tp_trap(struct pt_regs *regs, unsigned int instr)
int reg = (instr >> 12) & 15;
if (reg == 15)
return 1;
- regs->uregs[reg] = current_thread_info()->tp_value;
+ regs->uregs[reg] = current_thread_info()->tp_value[0];
regs->ARM_pc += 4;
return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Will Deacon

unread,
May 8, 2013, 4:59:05 AM5/8/13
to André Hentschel, linux...@vger.kernel.org, Russell King - ARM Linux, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, gre...@linuxfoundation.org, Jonathan Austin
Hi Andre,

On Tue, May 07, 2013 at 09:51:00PM +0100, Andr� Hentschel wrote:
> From: =?UTF-8?q?Andr=C3=A9=20Hentschel?= <ne...@dawncrow.de>

Might just be my mailer, but you should check that your name is intact here
otherwise the git log will be mangled.

> Since commit 6a1c53124aa1 the user writeable TLS register was zeroed to
> prevent it from being used as a covert channel between two tasks.
>
> There are more and more applications coming to WinRT, Wine could support them,
> but mostly they expect to have the thread environment block (TEB) in TPIDRURW.
>
> This patch preserves that register per thread instead of clearing it.
> Unlike the TPIDRURO, which is already switched, the TPIDRURW
> can be updated from userspace so needs careful treatment in the case that we
> modify TPIDRURW and call fork(). To avoid this we must always read
> TPIDRURW in copy_thread.
>
> Signed-off-by: Andr� Hentschel <ne...@dawncrow.de>
> Signed-off-by: Will Deacon <will....@arm.com>
> Signed-off-by: Jonathan Austin <jonatha...@arm.com>

[...]

> diff --git a/arch/arm/include/asm/tls.h b/arch/arm/include/asm/tls.h
> index 73409e6..22756ab 100644
> --- a/arch/arm/include/asm/tls.h
> +++ b/arch/arm/include/asm/tls.h
> @@ -2,27 +2,30 @@
> #define __ASMARM_TLS_H
>
> #ifdef __ASSEMBLY__
> - .macro set_tls_none, tp, tmp1, tmp2
> +#include <asm/asm-offsets.h>
> + .macro switch_tls_none, base, tp, tpuser, tmp1, tmp2
> .endm
>
> - .macro set_tls_v6k, tp, tmp1, tmp2
> + .macro switch_tls_v6k, base, tp, tpuser, tmp1, tmp2
> + mrc p15, 0, \tmp2, c13, c0, 2 @ get the user r/w register
> mcr p15, 0, \tp, c13, c0, 3 @ set TLS register
> - mov \tmp1, #0
> - mcr p15, 0, \tmp1, c13, c0, 2 @ clear user r/w TLS register
> + mcr p15, 0, \tpuser, c13, c0, 2 @ and the user r/w register
> + strne \tmp2, [\base, #TI_TP_VALUE + 4] @ save it

Why is this conditional?
This isn't standard kernel coding style. Please do something like:

static inline unsigned long get_tlsuser(void)
{
unsigned long reg = 0;

if (has_tls_reg && !tls_emu)
asm("mrc p15, 0, %0, c13, c0, 2" : "=r" (reg));

return reg;
}

Will

André Hentschel

unread,
May 8, 2013, 1:41:55 PM5/8/13
to Will Deacon, linux...@vger.kernel.org, Russell King - ARM Linux, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, gre...@linuxfoundation.org, Jonathan Austin
Hi Will,

thx for having a look.

Am 08.05.2013 10:57, schrieb Will Deacon:> Hi Andre,
>
> On Tue, May 07, 2013 at 09:51:00PM +0100, Andr� Hentschel wrote:
>> From: =?UTF-8?q?Andr=C3=A9=20Hentschel?= <ne...@dawncrow.de>
>
> Might just be my mailer, but you should check that your name is intact here
> otherwise the git log will be mangled.

That's for my acute accent and already worked with my first linux patch, it's git generated.
Seems like a copy&paste one, i'll send a v4

André Hentschel

unread,
May 8, 2013, 3:04:22 PM5/8/13
to linux...@vger.kernel.org, Russell King - ARM Linux, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, gre...@linuxfoundation.org, Will Deacon, Jonathan Austin
From: =?UTF-8?q?Andr=C3=A9=20Hentschel?= <ne...@dawncrow.de>

Since commit 6a1c53124aa1 the user writeable TLS register was zeroed to
prevent it from being used as a covert channel between two tasks.

There are more and more applications coming to WinRT, Wine could support them,
but mostly they expect to have the thread environment block (TEB) in TPIDRURW.

This patch preserves that register per thread instead of clearing it.
Unlike the TPIDRURO, which is already switched, the TPIDRURW
can be updated from userspace so needs careful treatment in the case that we
modify TPIDRURW and call fork(). To avoid this we must always read
TPIDRURW in copy_thread.

Signed-off-by: Andrᅵ Hentschel <ne...@dawncrow.de>
Signed-off-by: Will Deacon <will....@arm.com>
Signed-off-by: Jonathan Austin <jonatha...@arm.com>

---
This patch is against a86d52667d8eda5de39393ce737794403bdce1eb

Why so much Signed-off-bys? Some History:
The first patch had performance issues pointed out by Russel King,
so Will Deacon jumped in to help me with that. The second one again
had performance issues and the missing copy_thread part was uncovered.
After some iterations by me, Jonathan Austin proposed a patch and
Russel King sent his idea of the assembler part. All this was finally
merged and refined into this patch.
Thanks to everyone!

arch/arm/include/asm/thread_info.h | 2 +-
arch/arm/include/asm/tls.h | 40 ++++++++++++++++++++++++------------
arch/arm/kernel/entry-armv.S | 4 ++--
arch/arm/kernel/process.c | 4 +++-
arch/arm/kernel/ptrace.c | 2 +-
arch/arm/kernel/traps.c | 4 ++--
6 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index cddda1f..d90be6d 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -58,7 +58,7 @@ struct thread_info {
struct cpu_context_save cpu_context; /* cpu context */
__u32 syscall; /* syscall number */
__u8 used_cp[16]; /* thread used copro */
- unsigned long tp_value;
+ unsigned long tp_value[2]; /* TLS registers */
#ifdef CONFIG_CRUNCH
struct crunch_state crunchstate;
#endif
diff --git a/arch/arm/include/asm/tls.h b/arch/arm/include/asm/tls.h
index 73409e6..83259b8 100644
--- a/arch/arm/include/asm/tls.h
+++ b/arch/arm/include/asm/tls.h
@@ -2,27 +2,30 @@
#define __ASMARM_TLS_H

#ifdef __ASSEMBLY__
- .macro set_tls_none, tp, tmp1, tmp2
+#include <asm/asm-offsets.h>
+ .macro switch_tls_none, base, tp, tpuser, tmp1, tmp2
.endm

- .macro set_tls_v6k, tp, tmp1, tmp2
+ .macro switch_tls_v6k, base, tp, tpuser, tmp1, tmp2
+ mrc p15, 0, \tmp2, c13, c0, 2 @ get the user r/w register
mcr p15, 0, \tp, c13, c0, 3 @ set TLS register
- mov \tmp1, #0
- mcr p15, 0, \tmp1, c13, c0, 2 @ clear user r/w TLS register
+ mcr p15, 0, \tpuser, c13, c0, 2 @ and the user r/w register
+ str \tmp2, [\base, #TI_TP_VALUE + 4] @ save it
.endm

- .macro set_tls_v6, tp, tmp1, tmp2
+ .macro switch_tls_v6, base, tp, tpuser, tmp1, tmp2
ldr \tmp1, =elf_hwcap
ldr \tmp1, [\tmp1, #0]
mov \tmp2, #0xffff0fff
tst \tmp1, #HWCAP_TLS @ hardware TLS available?
- mcrne p15, 0, \tp, c13, c0, 3 @ yes, set TLS register
- movne \tmp1, #0
- mcrne p15, 0, \tmp1, c13, c0, 2 @ clear user r/w TLS register
streq \tp, [\tmp2, #-15] @ set TLS value at 0xffff0ff0
+ mrcne p15, 0, \tmp2, c13, c0, 2 @ get the user r/w register
+ mcrne p15, 0, \tp, c13, c0, 3 @ yes, set TLS register
+ mcrne p15, 0, \tpuser, c13, c0, 2 @ set user r/w register
+ strne \tmp2, [\base, #TI_TP_VALUE + 4] @ save it
.endm

- .macro set_tls_software, tp, tmp1, tmp2
+ .macro switch_tls_software, base, tp, tpuser, tmp1, tmp2
mov \tmp1, #0xffff0fff
str \tp, [\tmp1, #-15] @ set TLS value at 0xffff0ff0
.endm
@@ -31,19 +34,30 @@
#ifdef CONFIG_TLS_REG_EMUL
#define tls_emu 1
#define has_tls_reg 1
-#define set_tls set_tls_none
+#define switch_tls switch_tls_none
#elif defined(CONFIG_CPU_V6)
#define tls_emu 0
#define has_tls_reg (elf_hwcap & HWCAP_TLS)
-#define set_tls set_tls_v6
+#define switch_tls switch_tls_v6
#elif defined(CONFIG_CPU_32v6K)
#define tls_emu 0
#define has_tls_reg 1
-#define set_tls set_tls_v6k
+#define switch_tls switch_tls_v6k
#else
#define tls_emu 0
#define has_tls_reg 0
-#define set_tls set_tls_software
+#define switch_tls switch_tls_software
#endif

+#ifndef __ASSEMBLY__
+static inline unsigned long get_tpuser(void)
+{
+ unsigned long reg = 0;
+
+ if (has_tls_reg && !tls_emu)
+ __asm__("mrc p15, 0, %0, c13, c0, 2" : "=r" (reg));
+
+ return reg;
index 047d3e4..7635879 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -39,6 +39,7 @@
#include <asm/thread_notify.h>
#include <asm/stacktrace.h>
#include <asm/mach/time.h>
+#include <asm/tls.h>

#ifdef CONFIG_CC_STACKPROTECTOR
#include <linux/stackprotector.h>
@@ -395,7 +396,8 @@ copy_thread(unsigned long clone_flags, unsigned long stack_start,
clear_ptrace_hw_breakpoint(p);

if (clone_flags & CLONE_SETTLS)
- thread->tp_value = childregs->ARM_r3;
+ thread->tp_value[0] = childregs->ARM_r3;
+ thread->tp_value[1] = get_tpuser();

André Hentschel

unread,
May 18, 2013, 11:03:01 AM5/18/13
to linux...@vger.kernel.org, Russell King - ARM Linux, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, gre...@linuxfoundation.org, Will Deacon, Jonathan Austin
Am 08.05.2013 21:03, schrieb André Hentschel:
> From: =?UTF-8?q?Andr=C3=A9=20Hentschel?= <ne...@dawncrow.de>
>
> Since commit 6a1c53124aa1 the user writeable TLS register was zeroed to
> prevent it from being used as a covert channel between two tasks.
>
> There are more and more applications coming to WinRT, Wine could support them,
> but mostly they expect to have the thread environment block (TEB) in TPIDRURW.
>
> This patch preserves that register per thread instead of clearing it.
> Unlike the TPIDRURO, which is already switched, the TPIDRURW
> can be updated from userspace so needs careful treatment in the case that we
> modify TPIDRURW and call fork(). To avoid this we must always read
> TPIDRURW in copy_thread.
>
> Signed-off-by: André Hentschel <ne...@dawncrow.de>
> Signed-off-by: Will Deacon <will....@arm.com>
> Signed-off-by: Jonathan Austin <jonatha...@arm.com>
>

Hi,
I'm not yet very familiar with the development process here,
am i getting no feedback on v4 because of the mergewindow being closed?
Or is there another reason? Sry for being impatient.

Jonathan Austin

unread,
May 20, 2013, 7:04:08 AM5/20/13
to André Hentschel, linux...@vger.kernel.org, Russell King - ARM Linux, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, gre...@linuxfoundation.org, Will Deacon
Hi André

On 18/05/13 16:02, André Hentschel wrote:
> Am 08.05.2013 21:03, schrieb André Hentschel:
>> From: =?UTF-8?q?Andr=C3=A9=20Hentschel?= <ne...@dawncrow.de>
>>

This is strangely formatted for me too, and I use a different client
from Will so I'm not sure that the problem is just at our end...

(Also see that the list archive has weird formatting:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/167325.html )

Your first couple of patches didn't come out strangely formatted, so I'm
not really sure what's going on.

>> Since commit 6a1c53124aa1 the user writeable TLS register was zeroed to
>> prevent it from being used as a covert channel between two tasks.
>>
>> There are more and more applications coming to WinRT, Wine could support them,
>> but mostly they expect to have the thread environment block (TEB) in TPIDRURW.
>>
>> This patch preserves that register per thread instead of clearing it.
>> Unlike the TPIDRURO, which is already switched, the TPIDRURW
>> can be updated from userspace so needs careful treatment in the case that we
>> modify TPIDRURW and call fork(). To avoid this we must always read
>> TPIDRURW in copy_thread.
>>
>> Signed-off-by: André Hentschel <ne...@dawncrow.de>
>> Signed-off-by: Will Deacon <will....@arm.com>
>> Signed-off-by: Jonathan Austin <jonatha...@arm.com>
>>
>
> Hi,
> I'm not yet very familiar with the development process here,
> am i getting no feedback on v4 because of the mergewindow being closed?
> Or is there another reason? Sry for being impatient.
>

This is a feature, not a fix, so most likely it'll be included at the
next merge window. For that to happen it should be in Russell's tree
around the middle of this cycle.

Can you please rebase on 3.10-rc2 (when it happens) and post one more
version? After that, assuming nobody else has any final comments, you
could put it in to Russell's patch system...

Just as a hint - one thing you might have done to increase the chances
of getting comments to clarify what's different between v3 and v4 - as a
way to make life easier for reviewers you can highlight the differences
between versions after the "---" (where you currently have the
description of why there are so many S-o-Bs).

Hope that helps,
Jonny

André Hentschel

unread,
May 20, 2013, 8:52:05 AM5/20/13
to Jonathan Austin, linux...@vger.kernel.org, Russell King - ARM Linux, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, gre...@linuxfoundation.org, Will Deacon
Am 20.05.2013 13:03, schrieb Jonathan Austin:
> Hi André
>
> On 18/05/13 16:02, André Hentschel wrote:
>> Am 08.05.2013 21:03, schrieb André Hentschel:
>>> From: =?UTF-8?q?Andr=C3=A9=20Hentschel?= <ne...@dawncrow.de>
>>>
>
> This is strangely formatted for me too, and I use a different client from Will so I'm not sure that the problem is just at our end...
>
> (Also see that the list archive has weird formatting:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/167325.html )
>
> Your first couple of patches didn't come out strangely formatted, so I'm not really sure what's going on.

I'll retry it with plain Text or something. AFAIK git should understand this =UTF= style, though.

>>> Since commit 6a1c53124aa1 the user writeable TLS register was zeroed to
>>> prevent it from being used as a covert channel between two tasks.
>>>
>>> There are more and more applications coming to WinRT, Wine could support them,
>>> but mostly they expect to have the thread environment block (TEB) in TPIDRURW.
>>>
>>> This patch preserves that register per thread instead of clearing it.
>>> Unlike the TPIDRURO, which is already switched, the TPIDRURW
>>> can be updated from userspace so needs careful treatment in the case that we
>>> modify TPIDRURW and call fork(). To avoid this we must always read
>>> TPIDRURW in copy_thread.
>>>
>>> Signed-off-by: André Hentschel <ne...@dawncrow.de>
>>> Signed-off-by: Will Deacon <will....@arm.com>
>>> Signed-off-by: Jonathan Austin <jonatha...@arm.com>
>>>
>>
>> Hi,
>> I'm not yet very familiar with the development process here,
>> am i getting no feedback on v4 because of the mergewindow being closed?
>> Or is there another reason? Sry for being impatient.
>>
>
> This is a feature, not a fix, so most likely it'll be included at the next merge window. For that to happen it should be in Russell's tree around the middle of this cycle.
>
> Can you please rebase on 3.10-rc2 (when it happens) and post one more version? After that, assuming nobody else has any final comments, you could put it in to Russell's patch system...
>
> Just as a hint - one thing you might have done to increase the chances of getting comments to clarify what's different between v3 and v4 - as a way to make life easier for reviewers you can highlight the differences between versions after the "---" (where you currently have the description of why there are so many S-o-Bs).

I'll do, thank you very much for the reply and the clarification.

André Hentschel

unread,
May 22, 2013, 5:06:20 PM5/22/13
to linux...@vger.kernel.org, Russell King - ARM Linux, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, gre...@linuxfoundation.org, Will Deacon, Jonathan Austin
From: Andrᅵ Hentschel <ne...@dawncrow.de>

Since commit 6a1c53124aa1 the user writeable TLS register was zeroed to
prevent it from being used as a covert channel between two tasks.

There are more and more applications coming to Windows RT,
Wine could support them, but mostly they expect to have
the thread environment block (TEB) in TPIDRURW.

This patch preserves that register per thread instead of clearing it.
Unlike the TPIDRURO, which is already switched, the TPIDRURW
can be updated from userspace so needs careful treatment in the case that we
modify TPIDRURW and call fork(). To avoid this we must always read
TPIDRURW in copy_thread.

Signed-off-by: Andrᅵ Hentschel <ne...@dawncrow.de>
Signed-off-by: Will Deacon <will....@arm.com>
Signed-off-by: Jonathan Austin <jonatha...@arm.com>

---
This patch is against Linux 3.10-rc2 (c7788792a5e7b0d5d7f96d0766b4cb6112d47d75)

v2: rework and fixup of v1, based on a suggested patch by Will Deacon
v3: total rework and fixup of v2
v4: removed condition on assembler instruction,
adapted my code to kernel-style, both based on comments by Will Deacon
v5: rebased v4 on 3.10-rc2 and adding this version history

As suggested by Jonathan Austin, i'll send this patch to RMK's patch tracker in
case there are no more comments on it.

Why so much Signed-off-bys? Some History:
The first patch had performance issues pointed out by Russel King,
so Will Deacon jumped in to help me with that. The second one again
had performance issues and the missing copy_thread part was uncovered.
After some iterations by me, Jonathan Austin proposed a patch and
Russel King sent his idea of the assembler part. All this was finally
merged and refined into this patch.
Thanks to everyone!

arch/arm/include/asm/thread_info.h | 2 +-
arch/arm/include/asm/tls.h | 40 +++++++++++++++++++++++++-------------
arch/arm/kernel/entry-armv.S | 4 ++--
arch/arm/kernel/process.c | 4 +++-
arch/arm/kernel/ptrace.c | 2 +-
arch/arm/kernel/traps.c | 4 ++--
6 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 1995d1a..214d415 100644
index 582b405..ee1d257 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -685,15 +685,15 @@ ENTRY(__switch_to)
UNWIND(.fnstart )
UNWIND(.cantunwind )
add ip, r1, #TI_CPU_SAVE
- ldr r3, [r2, #TI_TP_VALUE]
ARM( stmia ip!, {r4 - sl, fp, sp, lr} ) @ Store most regs on stack
THUMB( stmia ip!, {r4 - sl, fp} ) @ Store most regs on stack
THUMB( str sp, [ip], #4 )
THUMB( str lr, [ip], #4 )
+ ldrd r4, r5, [r2, #TI_TP_VALUE]
#ifdef CONFIG_CPU_USE_DOMAINS
ldr r6, [r2, #TI_CPU_DOMAIN]
#endif
- set_tls r3, r4, r5
+ switch_tls r1, r4, r5, r3, r7
#if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
ldr r7, [r2, #TI_TASK]
ldr r8, =__stack_chk_guard
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index f219703..0870641 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -39,6 +39,7 @@
#include <asm/thread_notify.h>
#include <asm/stacktrace.h>
#include <asm/mach/time.h>
+#include <asm/tls.h>

#ifdef CONFIG_CC_STACKPROTECTOR
#include <linux/stackprotector.h>
@@ -343,7 +344,8 @@ copy_thread(unsigned long clone_flags, unsigned long stack_start,
clear_ptrace_hw_breakpoint(p);

if (clone_flags & CLONE_SETTLS)
- thread->tp_value = childregs->ARM_r3;
+ thread->tp_value[0] = childregs->ARM_r3;
+ thread->tp_value[1] = get_tpuser();

thread_notify(THREAD_NOTIFY_COPY, thread);

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 03deeff..2bc1514 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -849,7 +849,7 @@ long arch_ptrace(struct task_struct *child, long request,
#endif

case PTRACE_GET_THREAD_AREA:
- ret = put_user(task_thread_info(child)->tp_value,
+ ret = put_user(task_thread_info(child)->tp_value[0],
datap);
break;

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 18b32e8..517bfd4 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -581,7 +581,7 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
return regs->ARM_r0;

case NR(set_tls):
- thread->tp_value = regs->ARM_r0;
+ thread->tp_value[0] = regs->ARM_r0;
if (tls_emu)
return 0;
if (has_tls_reg) {
@@ -699,7 +699,7 @@ static int get_tp_trap(struct pt_regs *regs, unsigned int instr)
int reg = (instr >> 12) & 15;
if (reg == 15)
return 1;
- regs->uregs[reg] = current_thread_info()->tp_value;
+ regs->uregs[reg] = current_thread_info()->tp_value[0];
regs->ARM_pc += 4;
return 0;
}
--
1.8.1.2

Jonathan Austin

unread,
Jun 7, 2013, 3:06:09 PM6/7/13
to André Hentschel, linux...@vger.kernel.org, Russell King - ARM Linux, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, gre...@linuxfoundation.org, Will Deacon
Hi André

This looks good to go - I've tested this version of the patch too (on
Versatile Express)

The next step is to put it in to Russell's patch system. This is the way
Russell manages (small/non-pull-request) things that need to go in to
his tree.

There are instructions on using the patch-system here:
http://www.arm.linux.org.uk/developer/patches/info.php

If you're using git-format-patch and git-send-email then the main point
to note is the need to add the KernelVersion: tag described in that
documentation and also that Russell doesn't want '[Patch]' tags in
subject lines.

There's a form for creating yourself an account on the patch system at
http://www.arm.linux.org.uk/developer/patches/add.php - I'm not actually
sure that an account is necessary to submit via email - but I *do* have
one and I've only ever used email so I suspect it might be!

Hope that helps,

Jonny

On 22/05/13 22:04, André Hentschel wrote:
> From: André Hentschel <ne...@dawncrow.de>
>
> Since commit 6a1c53124aa1 the user writeable TLS register was zeroed to
> prevent it from being used as a covert channel between two tasks.
>
> There are more and more applications coming to Windows RT,
> Wine could support them, but mostly they expect to have
> the thread environment block (TEB) in TPIDRURW.
>
> This patch preserves that register per thread instead of clearing it.
> Unlike the TPIDRURO, which is already switched, the TPIDRURW
> can be updated from userspace so needs careful treatment in the case that we
> modify TPIDRURW and call fork(). To avoid this we must always read
> TPIDRURW in copy_thread.
>
> Signed-off-by: André Hentschel <ne...@dawncrow.de>

Russell King - ARM Linux

unread,
Jun 7, 2013, 4:29:00 PM6/7/13
to Jonathan Austin, André Hentschel, linux...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, gre...@linuxfoundation.org, Will Deacon
On Fri, Jun 07, 2013 at 08:05:37PM +0100, Jonathan Austin wrote:
> If you're using git-format-patch and git-send-email then the main point
> to note is the need to add the KernelVersion: tag described in that
> documentation and also that Russell doesn't want '[Patch]' tags in
> subject lines.

The main thing is the kernelversion: tag, which can appear anywhere before
the patch diff itself. [PATCH] stuff in the subject line is ignored and
removed.

> There's a form for creating yourself an account on the patch system at
> http://www.arm.linux.org.uk/developer/patches/add.php - I'm not actually
> sure that an account is necessary to submit via email - but I *do* have
> one and I've only ever used email so I suspect it might be!

Yes, the system automatically creates an "account" on the initial email -
it's not really an "account" from the email side, but just a name/email
association with the patch itself, which can then be "converted" to an
"account" for the website by the addition of a password.

Russell King - ARM Linux

unread,
Jun 17, 2013, 4:28:58 AM6/17/13
to Jonathan Austin, André Hentschel, linux...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, gre...@linuxfoundation.org, Will Deacon
On Fri, Jun 07, 2013 at 08:05:37PM +0100, Jonathan Austin wrote:
> The next step is to put it in to Russell's patch system. This is the way
> Russell manages (small/non-pull-request) things that need to go in to
> his tree.

And I'm going to drop this patch:

arch/arm/kernel/entry-armv.S:692: Error: selected processor does not support ARM mode `ldrd r4,r5,[r2,#96]'

LDRD is not supported on all the CPUs we have...
0 new messages