This is required for clone3 which passes the TLS value through a
struct rather than a register.
Signed-off-by: Amanieu d'Antras <[email protected]>
Cc: [email protected]
Cc: <[email protected]> # 5.3.x
---
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/process.c | 10 +++++-----
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index b1b4476ddb83..e688dfad0b72 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -138,6 +138,7 @@ config ARM64
select HAVE_CMPXCHG_DOUBLE
select HAVE_CMPXCHG_LOCAL
select HAVE_CONTEXT_TRACKING
+ select HAVE_COPY_THREAD_TLS
select HAVE_DEBUG_BUGVERBOSE
select HAVE_DEBUG_KMEMLEAK
select HAVE_DMA_CONTIGUOUS
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 71f788cd2b18..d54586d5b031 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -360,8 +360,8 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
asmlinkage void ret_from_fork(void) asm("ret_from_fork");
-int copy_thread(unsigned long clone_flags, unsigned long stack_start,
- unsigned long stk_sz, struct task_struct *p)
+int copy_thread_tls(unsigned long clone_flags, unsigned long stack_start,
+ unsigned long stk_sz, struct task_struct *p, unsigned long tls)
{
struct pt_regs *childregs = task_pt_regs(p);
@@ -394,11 +394,11 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
}
/*
- * If a TLS pointer was passed to clone (4th argument), use it
- * for the new thread.
+ * If a TLS pointer was passed to clone, use it for the new
+ * thread.
*/
if (clone_flags & CLONE_SETTLS)
- p->thread.uw.tp_value = childregs->regs[3];
+ p->thread.uw.tp_value = tls;
} else {
memset(childregs, 0, sizeof(struct pt_regs));
childregs->pstate = PSR_MODE_EL1h;
--
2.24.1
On Thu, Jan 02, 2020 at 06:24:08PM +0100, Amanieu d'Antras wrote:
> This is required for clone3 which passes the TLS value through a
> struct rather than a register.
>
> Signed-off-by: Amanieu d'Antras <[email protected]>
> Cc: [email protected]
> Cc: <[email protected]> # 5.3.x
This looks sane to me but I'd like an ack from someone who knows his arm
from his arse before taking this. :)
Acked-by: Christian Brauner <[email protected]>
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/kernel/process.c | 10 +++++-----
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index b1b4476ddb83..e688dfad0b72 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -138,6 +138,7 @@ config ARM64
> select HAVE_CMPXCHG_DOUBLE
> select HAVE_CMPXCHG_LOCAL
> select HAVE_CONTEXT_TRACKING
> + select HAVE_COPY_THREAD_TLS
> select HAVE_DEBUG_BUGVERBOSE
> select HAVE_DEBUG_KMEMLEAK
> select HAVE_DMA_CONTIGUOUS
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 71f788cd2b18..d54586d5b031 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -360,8 +360,8 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
>
> asmlinkage void ret_from_fork(void) asm("ret_from_fork");
>
> -int copy_thread(unsigned long clone_flags, unsigned long stack_start,
> - unsigned long stk_sz, struct task_struct *p)
> +int copy_thread_tls(unsigned long clone_flags, unsigned long stack_start,
> + unsigned long stk_sz, struct task_struct *p, unsigned long tls)
> {
> struct pt_regs *childregs = task_pt_regs(p);
>
> @@ -394,11 +394,11 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
> }
>
> /*
> - * If a TLS pointer was passed to clone (4th argument), use it
> - * for the new thread.
> + * If a TLS pointer was passed to clone, use it for the new
> + * thread.
> */
> if (clone_flags & CLONE_SETTLS)
> - p->thread.uw.tp_value = childregs->regs[3];
> + p->thread.uw.tp_value = tls;
> } else {
> memset(childregs, 0, sizeof(struct pt_regs));
> childregs->pstate = PSR_MODE_EL1h;
> --
> 2.24.1
>
On Thu, Jan 02, 2020 at 07:01:33PM +0100, Christian Brauner wrote:
> On Thu, Jan 02, 2020 at 06:24:08PM +0100, Amanieu d'Antras wrote:
> > This is required for clone3 which passes the TLS value through a
> > struct rather than a register.
> >
> > Signed-off-by: Amanieu d'Antras <[email protected]>
> > Cc: [email protected]
> > Cc: <[email protected]> # 5.3.x
>
> This looks sane to me but I'd like an ack from someone who knows his arm
> from his arse before taking this. :)
That's *ME*! Code looks fine:
Acked-by: Will Deacon <[email protected]>
I also ran the native and compat selftests but, unfortunately, they all
pass even without this patch. Do you reckon it would be possible to update
them to check the tls pointer?
Will
On Mon, Jan 6, 2020 at 6:39 PM Will Deacon <[email protected]> wrote:
> I also ran the native and compat selftests but, unfortunately, they all
> pass even without this patch. Do you reckon it would be possible to update
> them to check the tls pointer?
Here's the program I used for testing on arm64. I considered adding it
to the selftests but there is no portable way of reading the TLS
register on all architectures.
#include <sys/syscall.h>
#include <unistd.h>
#include <stdio.h>
#include <stdint.h>
#define __NR_clone3 435
struct clone_args {
uint64_t flags;
uint64_t pidfd;
uint64_t child_tid;
uint64_t parent_tid;
uint64_t exit_signal;
uint64_t stack;
uint64_t stack_size;
uint64_t tls;
};
#define USE_CLONE3
int main() {
printf("Before fork: tp = %p\n", __builtin_thread_pointer());
#ifdef USE_CLONE3
struct clone_args args = {
.flags = CLONE_SETTLS,
.tls = (uint64_t)__builtin_thread_pointer(),
};
int ret = syscall(__NR_clone3, &args, sizeof(args));
#else
int ret = syscall(__NR_clone, CLONE_SETTLS, 0, 0,
__builtin_thread_pointer(), 0);
#endif
printf("Fork returned %d, tp = %p\n", ret, __builtin_thread_pointer());
}
[Cc Kees in case he knows something about where arch specific tests live
or whether we have a framework for this]
On Mon, Jan 06, 2020 at 07:03:32PM +0100, Amanieu d'Antras wrote:
> On Mon, Jan 6, 2020 at 6:39 PM Will Deacon <[email protected]> wrote:
> > I also ran the native and compat selftests but, unfortunately, they all
> > pass even without this patch. Do you reckon it would be possible to update
> > them to check the tls pointer?
>
> Here's the program I used for testing on arm64. I considered adding it
> to the selftests but there is no portable way of reading the TLS
> register on all architectures.
I'm not saying you need to do this right now.
It feels like we must've run into the "this is architecture
specific"-and-we-want-to-test-this issue before... Do we have a place
where architecture specific selftests live?
>
> #include <sys/syscall.h>
> #include <unistd.h>
> #include <stdio.h>
> #include <stdint.h>
>
> #define __NR_clone3 435
> struct clone_args {
> uint64_t flags;
> uint64_t pidfd;
> uint64_t child_tid;
> uint64_t parent_tid;
> uint64_t exit_signal;
> uint64_t stack;
> uint64_t stack_size;
> uint64_t tls;
> };
>
> #define USE_CLONE3
>
> int main() {
> printf("Before fork: tp = %p\n", __builtin_thread_pointer());
> #ifdef USE_CLONE3
> struct clone_args args = {
> .flags = CLONE_SETTLS,
> .tls = (uint64_t)__builtin_thread_pointer(),
> };
> int ret = syscall(__NR_clone3, &args, sizeof(args));
> #else
> int ret = syscall(__NR_clone, CLONE_SETTLS, 0, 0,
> __builtin_thread_pointer(), 0);
> #endif
> printf("Fork returned %d, tp = %p\n", ret, __builtin_thread_pointer());
> }
On Tue, Jan 07, 2020 at 10:02:27AM +0100, Christian Brauner wrote:
> [Cc Kees in case he knows something about where arch specific tests live
> or whether we have a framework for this]
>
> On Mon, Jan 06, 2020 at 07:03:32PM +0100, Amanieu d'Antras wrote:
> > On Mon, Jan 6, 2020 at 6:39 PM Will Deacon <[email protected]> wrote:
> > > I also ran the native and compat selftests but, unfortunately, they all
> > > pass even without this patch. Do you reckon it would be possible to update
> > > them to check the tls pointer?
> >
> > Here's the program I used for testing on arm64. I considered adding it
> > to the selftests but there is no portable way of reading the TLS
> > register on all architectures.
>
> I'm not saying you need to do this right now.
Agreed, these patches should be merged in their current state and my ack
stands for that.
> It feels like we must've run into the "this is architecture
> specific"-and-we-want-to-test-this issue before... Do we have a place
> where architecture specific selftests live?
For arch-specific selftests there are tools/testing/selftests/$ARCH
directories, although in this case maybe it's better to have an #ifdef
in a header so that architectures with __builtin_thread_pointer can use
that.
Will
On Tue, Jan 07, 2020 at 05:45:09PM +0000, Will Deacon wrote:
> On Tue, Jan 07, 2020 at 10:02:27AM +0100, Christian Brauner wrote:
> > [Cc Kees in case he knows something about where arch specific tests live
> > or whether we have a framework for this]
> > [...]
> > It feels like we must've run into the "this is architecture
> > specific"-and-we-want-to-test-this issue before... Do we have a place
> > where architecture specific selftests live?
>
> For arch-specific selftests there are tools/testing/selftests/$ARCH
> directories, although in this case maybe it's better to have an #ifdef
> in a header so that architectures with __builtin_thread_pointer can use
> that.
Yup, I agree: that's the current best-practice for arch-specific
selftests.
--
Kees Cook
On Tue, Jan 07, 2020 at 05:45:09PM +0000, Will Deacon wrote:
> On Tue, Jan 07, 2020 at 10:02:27AM +0100, Christian Brauner wrote:
> > [Cc Kees in case he knows something about where arch specific tests live
> > or whether we have a framework for this]
> >
> > On Mon, Jan 06, 2020 at 07:03:32PM +0100, Amanieu d'Antras wrote:
> > > On Mon, Jan 6, 2020 at 6:39 PM Will Deacon <[email protected]> wrote:
> > > > I also ran the native and compat selftests but, unfortunately, they all
> > > > pass even without this patch. Do you reckon it would be possible to update
> > > > them to check the tls pointer?
> > >
> > > Here's the program I used for testing on arm64. I considered adding it
> > > to the selftests but there is no portable way of reading the TLS
> > > register on all architectures.
> >
> > I'm not saying you need to do this right now.
>
> Agreed, these patches should be merged in their current state and my ack
> stands for that.
Oh yeah, that's how I took your Ack.
Thanks! :)
>
> > It feels like we must've run into the "this is architecture
> > specific"-and-we-want-to-test-this issue before... Do we have a place
> > where architecture specific selftests live?
>
> For arch-specific selftests there are tools/testing/selftests/$ARCH
> directories, although in this case maybe it's better to have an #ifdef
> in a header so that architectures with __builtin_thread_pointer can use
> that.
Yeah, I think the #ifdef approach might make the most sense.
Christian
On Tue, Jan 07, 2020 at 10:12:39AM -0800, Kees Cook wrote:
> On Tue, Jan 07, 2020 at 05:45:09PM +0000, Will Deacon wrote:
> > On Tue, Jan 07, 2020 at 10:02:27AM +0100, Christian Brauner wrote:
> > > [Cc Kees in case he knows something about where arch specific tests live
> > > or whether we have a framework for this]
> > > [...]
> > > It feels like we must've run into the "this is architecture
> > > specific"-and-we-want-to-test-this issue before... Do we have a place
> > > where architecture specific selftests live?
> >
> > For arch-specific selftests there are tools/testing/selftests/$ARCH
> > directories, although in this case maybe it's better to have an #ifdef
> > in a header so that architectures with __builtin_thread_pointer can use
> > that.
>
> Yup, I agree: that's the current best-practice for arch-specific
> selftests.
Thanks! I think using #ifdef in this case with __builtin_thread_pointer
sounds good.
So the tests can be moved into the clone3() test-suite for those
architectures.
Christian