The clone3 syscall is currently broken when used with CLONE_SETTLS on all
architectures that don't have an implementation of copy_thread_tls. The old
copy_thread function handles CLONE_SETTLS by reading the new TLS value from
pt_regs containing the clone syscall parameters. Since clone3 passes the TLS
value in clone_args, this results in the TLS register being initialized to a
garbage value.
This patch series implements copy_thread_tls on all architectures that currently
define __ARCH_WANT_SYS_CLONE3 and adds a compile-time check to ensure that any
architecture that enables clone3 in the future also implements copy_thread_tls.
I have also included a minor fix for the arm64 uapi headers which caused
__NR_clone3 to be missing from the exported user headers.
I have only tested this on arm64, but the copy_thread_tls implementations for
the various architectures are fairly straightforward.
Amanieu d'Antras (7):
arm64: Move __ARCH_WANT_SYS_CLONE3 definition to uapi headers
arm64: Implement copy_thread_tls
arm: Implement copy_thread_tls
parisc: Implement copy_thread_tls
riscv: Implement copy_thread_tls
xtensa: Implement copy_thread_tls
clone3: ensure copy_thread_tls is implemented
arch/arm/Kconfig | 1 +
arch/arm/kernel/process.c | 6 +++---
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/unistd.h | 1 -
arch/arm64/include/uapi/asm/unistd.h | 1 +
arch/arm64/kernel/process.c | 10 +++++-----
arch/parisc/Kconfig | 1 +
arch/parisc/kernel/process.c | 8 ++++----
arch/riscv/Kconfig | 1 +
arch/riscv/kernel/process.c | 6 +++---
arch/xtensa/Kconfig | 1 +
arch/xtensa/kernel/process.c | 8 ++++----
kernel/fork.c | 10 ++++++++++
13 files changed, 35 insertions(+), 20 deletions(-)
--
2.24.1
copy_thread implementations handle CLONE_SETTLS by reading the TLS
value from the registers containing the syscall arguments for
clone. This doesn't work with clone3 since the TLS value is passed
in clone_args instead.
Signed-off-by: Amanieu d'Antras <[email protected]>
Cc: <[email protected]> # 5.3.x
---
kernel/fork.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/kernel/fork.c b/kernel/fork.c
index 2508a4f238a3..080809560072 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2578,6 +2578,16 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, unsigned long, newsp,
#endif
#ifdef __ARCH_WANT_SYS_CLONE3
+
+/*
+ * copy_thread implementations handle CLONE_SETTLS by reading the TLS value from
+ * the registers containing the syscall arguments for clone. This doesn't work
+ * with clone3 since the TLS value is passed in clone_args instead.
+ */
+#ifndef CONFIG_HAVE_COPY_THREAD_TLS
+#error clone3 requires copy_thread_tls support in arch
+#endif
+
noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
struct clone_args __user *uargs,
size_t usize)
--
2.24.1
On Thu, Jan 02, 2020 at 06:24:13PM +0100, Amanieu d'Antras wrote:
> copy_thread implementations handle CLONE_SETTLS by reading the TLS
> value from the registers containing the syscall arguments for
> clone. This doesn't work with clone3 since the TLS value is passed
> in clone_args instead.
>
> Signed-off-by: Amanieu d'Antras <[email protected]>
> Cc: <[email protected]> # 5.3.x
I'm in favor of this change. But we need to make sure that any arch
which now has ARCH_WANTS_SYS_CLONE3 set but doesn't implement
copy_thread_tls() is fixed.
Once all architectures have clone3() support - and there are
just a few by now (IA64 comes to mind) this means we should also be able
to get rid of of copy_thread() completely. That seems desirable to me as
it makes the codepaths easier to follow.
Reviewed-by: Christian Brauner <[email protected]>
On Thu, Jan 02, 2020 at 06:24:06PM +0100, Amanieu d'Antras wrote:
> The clone3 syscall is currently broken when used with CLONE_SETTLS on all
> architectures that don't have an implementation of copy_thread_tls. The old
> copy_thread function handles CLONE_SETTLS by reading the new TLS value from
> pt_regs containing the clone syscall parameters. Since clone3 passes the TLS
> value in clone_args, this results in the TLS register being initialized to a
> garbage value.
>
> This patch series implements copy_thread_tls on all architectures that currently
> define __ARCH_WANT_SYS_CLONE3 and adds a compile-time check to ensure that any
> architecture that enables clone3 in the future also implements copy_thread_tls.
>
> I have also included a minor fix for the arm64 uapi headers which caused
> __NR_clone3 to be missing from the exported user headers.
>
> I have only tested this on arm64, but the copy_thread_tls implementations for
> the various architectures are fairly straightforward.
The series looks straightforward to me but I'd like a few Acks from some
of the arch maintainers before taking this.
Acked-by: Christian Brauner <[email protected]>
On Thu, Jan 2, 2020 at 7:09 PM Christian Brauner
<[email protected]> wrote:
> I'm in favor of this change. But we need to make sure that any arch
> which now has ARCH_WANTS_SYS_CLONE3 set but doesn't implement
> copy_thread_tls() is fixed.
>
> Once all architectures have clone3() support - and there are
> just a few by now (IA64 comes to mind) this means we should also be able
> to get rid of of copy_thread() completely. That seems desirable to me as
> it makes the codepaths easier to follow.
I've already implemented copy_thread_tls for all arches that currently
have ARCH_WANTS_SYS_CLONE3 in the previous 5 patches. The #error is
there so that any future arches that wire up clone3 don't forget to
implement copy_thread_tls as well.
On Thu, Jan 02, 2020 at 07:19:11PM +0100, Amanieu d'Antras wrote:
> On Thu, Jan 2, 2020 at 7:09 PM Christian Brauner
> <[email protected]> wrote:
> > I'm in favor of this change. But we need to make sure that any arch
> > which now has ARCH_WANTS_SYS_CLONE3 set but doesn't implement
> > copy_thread_tls() is fixed.
> >
> > Once all architectures have clone3() support - and there are
> > just a few by now (IA64 comes to mind) this means we should also be able
> > to get rid of of copy_thread() completely. That seems desirable to me as
> > it makes the codepaths easier to follow.
>
> I've already implemented copy_thread_tls for all arches that currently
> have ARCH_WANTS_SYS_CLONE3 in the previous 5 patches. The #error is
> there so that any future arches that wire up clone3 don't forget to
> implement copy_thread_tls as well.
Right, my point is: Once _all_ arches have ARCH_WANT_SYS_CLONE3 they
also must implement copy_thread_tls at which point we can remove:
- ARCH_WANT_SYS_CLONE3 ifdefines
- copy_thread()
We can't do this right now because e.g. IA64 does not set
ARCH_WANT_SYS_CLONE3 and also does not select HAVE_COPY_THREAD_TLS.
Christian
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/um/Kconfig | 1 +
arch/um/include/asm/ptrace-generic.h | 2 +-
arch/um/kernel/process.c | 6 +++---
arch/x86/um/tls_32.c | 6 ++----
arch/x86/um/tls_64.c | 7 +++----
5 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/arch/um/Kconfig b/arch/um/Kconfig
index 2a6d04fcb3e9..6f0edd0c0220 100644
--- a/arch/um/Kconfig
+++ b/arch/um/Kconfig
@@ -14,6 +14,7 @@ config UML
select HAVE_FUTEX_CMPXCHG if FUTEX
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_BUGVERBOSE
+ select HAVE_COPY_THREAD_TLS
select GENERIC_IRQ_SHOW
select GENERIC_CPU_DEVICES
select GENERIC_CLOCKEVENTS
diff --git a/arch/um/include/asm/ptrace-generic.h b/arch/um/include/asm/ptrace-generic.h
index 81c647ef9c6c..adf91ef553ae 100644
--- a/arch/um/include/asm/ptrace-generic.h
+++ b/arch/um/include/asm/ptrace-generic.h
@@ -36,7 +36,7 @@ extern long subarch_ptrace(struct task_struct *child, long request,
extern unsigned long getreg(struct task_struct *child, int regno);
extern int putreg(struct task_struct *child, int regno, unsigned long value);
-extern int arch_copy_tls(struct task_struct *new);
+extern int arch_set_tls(struct task_struct *new, unsigned long tls);
extern void clear_flushed_tls(struct task_struct *task);
extern int syscall_trace_enter(struct pt_regs *regs);
extern void syscall_trace_leave(struct pt_regs *regs);
diff --git a/arch/um/kernel/process.c b/arch/um/kernel/process.c
index 263a8f069133..17045e7211bf 100644
--- a/arch/um/kernel/process.c
+++ b/arch/um/kernel/process.c
@@ -153,8 +153,8 @@ void fork_handler(void)
userspace(¤t->thread.regs.regs, current_thread_info()->aux_fp_regs);
}
-int copy_thread(unsigned long clone_flags, unsigned long sp,
- unsigned long arg, struct task_struct * p)
+int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
+ unsigned long arg, struct task_struct * p, unsigned long tls)
{
void (*handler)(void);
int kthread = current->flags & PF_KTHREAD;
@@ -188,7 +188,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
* Set a new TLS for the child thread?
*/
if (clone_flags & CLONE_SETTLS)
- ret = arch_copy_tls(p);
+ ret = arch_set_tls(p, tls);
}
return ret;
diff --git a/arch/x86/um/tls_32.c b/arch/x86/um/tls_32.c
index 5bd949da7a4a..ac8eee093f9c 100644
--- a/arch/x86/um/tls_32.c
+++ b/arch/x86/um/tls_32.c
@@ -215,14 +215,12 @@ static int set_tls_entry(struct task_struct* task, struct user_desc *info,
return 0;
}
-int arch_copy_tls(struct task_struct *new)
+int arch_set_tls(struct task_struct *new, unsigned long tls)
{
struct user_desc info;
int idx, ret = -EFAULT;
- if (copy_from_user(&info,
- (void __user *) UPT_SI(&new->thread.regs.regs),
- sizeof(info)))
+ if (copy_from_user(&info, (void __user *) tls, sizeof(info)))
goto out;
ret = -EINVAL;
diff --git a/arch/x86/um/tls_64.c b/arch/x86/um/tls_64.c
index 3a621e0d3925..ebd3855d9b13 100644
--- a/arch/x86/um/tls_64.c
+++ b/arch/x86/um/tls_64.c
@@ -6,14 +6,13 @@ void clear_flushed_tls(struct task_struct *task)
{
}
-int arch_copy_tls(struct task_struct *t)
+int arch_set_tls(struct task_struct *t, unsigned long tls)
{
/*
* If CLONE_SETTLS is set, we need to save the thread id
- * (which is argument 5, child_tid, of clone) so it can be set
- * during context switches.
+ * so it can be set during context switches.
*/
- t->thread.arch.fs = t->thread.regs.regs.gp[R8 / sizeof(long)];
+ t->thread.arch.fs = tls;
return 0;
}
--
2.24.1
On Sat, Jan 04, 2020 at 01:39:30PM +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
Thanks. I'm picking this up as part of the copy_thread_tls() series.
(Leaving the patch in tact so people can Ack right here if they want to.)
If I could get an Ack from one of the maintainers that would be great;
see
https://lore.kernel.org/lkml/[email protected]
for more context.
Acked-by: Christian Brauner <[email protected]>
> ---
> arch/um/Kconfig | 1 +
> arch/um/include/asm/ptrace-generic.h | 2 +-
> arch/um/kernel/process.c | 6 +++---
> arch/x86/um/tls_32.c | 6 ++----
> arch/x86/um/tls_64.c | 7 +++----
> 5 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/arch/um/Kconfig b/arch/um/Kconfig
> index 2a6d04fcb3e9..6f0edd0c0220 100644
> --- a/arch/um/Kconfig
> +++ b/arch/um/Kconfig
> @@ -14,6 +14,7 @@ config UML
> select HAVE_FUTEX_CMPXCHG if FUTEX
> select HAVE_DEBUG_KMEMLEAK
> select HAVE_DEBUG_BUGVERBOSE
> + select HAVE_COPY_THREAD_TLS
> select GENERIC_IRQ_SHOW
> select GENERIC_CPU_DEVICES
> select GENERIC_CLOCKEVENTS
> diff --git a/arch/um/include/asm/ptrace-generic.h b/arch/um/include/asm/ptrace-generic.h
> index 81c647ef9c6c..adf91ef553ae 100644
> --- a/arch/um/include/asm/ptrace-generic.h
> +++ b/arch/um/include/asm/ptrace-generic.h
> @@ -36,7 +36,7 @@ extern long subarch_ptrace(struct task_struct *child, long request,
> extern unsigned long getreg(struct task_struct *child, int regno);
> extern int putreg(struct task_struct *child, int regno, unsigned long value);
>
> -extern int arch_copy_tls(struct task_struct *new);
> +extern int arch_set_tls(struct task_struct *new, unsigned long tls);
> extern void clear_flushed_tls(struct task_struct *task);
> extern int syscall_trace_enter(struct pt_regs *regs);
> extern void syscall_trace_leave(struct pt_regs *regs);
> diff --git a/arch/um/kernel/process.c b/arch/um/kernel/process.c
> index 263a8f069133..17045e7211bf 100644
> --- a/arch/um/kernel/process.c
> +++ b/arch/um/kernel/process.c
> @@ -153,8 +153,8 @@ void fork_handler(void)
> userspace(¤t->thread.regs.regs, current_thread_info()->aux_fp_regs);
> }
>
> -int copy_thread(unsigned long clone_flags, unsigned long sp,
> - unsigned long arg, struct task_struct * p)
> +int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
> + unsigned long arg, struct task_struct * p, unsigned long tls)
> {
> void (*handler)(void);
> int kthread = current->flags & PF_KTHREAD;
> @@ -188,7 +188,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
> * Set a new TLS for the child thread?
> */
> if (clone_flags & CLONE_SETTLS)
> - ret = arch_copy_tls(p);
> + ret = arch_set_tls(p, tls);
> }
>
> return ret;
> diff --git a/arch/x86/um/tls_32.c b/arch/x86/um/tls_32.c
> index 5bd949da7a4a..ac8eee093f9c 100644
> --- a/arch/x86/um/tls_32.c
> +++ b/arch/x86/um/tls_32.c
> @@ -215,14 +215,12 @@ static int set_tls_entry(struct task_struct* task, struct user_desc *info,
> return 0;
> }
>
> -int arch_copy_tls(struct task_struct *new)
> +int arch_set_tls(struct task_struct *new, unsigned long tls)
> {
> struct user_desc info;
> int idx, ret = -EFAULT;
>
> - if (copy_from_user(&info,
> - (void __user *) UPT_SI(&new->thread.regs.regs),
> - sizeof(info)))
> + if (copy_from_user(&info, (void __user *) tls, sizeof(info)))
> goto out;
>
> ret = -EINVAL;
> diff --git a/arch/x86/um/tls_64.c b/arch/x86/um/tls_64.c
> index 3a621e0d3925..ebd3855d9b13 100644
> --- a/arch/x86/um/tls_64.c
> +++ b/arch/x86/um/tls_64.c
> @@ -6,14 +6,13 @@ void clear_flushed_tls(struct task_struct *task)
> {
> }
>
> -int arch_copy_tls(struct task_struct *t)
> +int arch_set_tls(struct task_struct *t, unsigned long tls)
> {
> /*
> * If CLONE_SETTLS is set, we need to save the thread id
> - * (which is argument 5, child_tid, of clone) so it can be set
> - * during context switches.
> + * so it can be set during context switches.
> */
> - t->thread.arch.fs = t->thread.regs.regs.gp[R8 / sizeof(long)];
> + t->thread.arch.fs = tls;
>
> return 0;
> }
> --
> 2.24.1
>
On Thu, Jan 02, 2020 at 06:24:06PM +0100, Amanieu d'Antras wrote:
> The clone3 syscall is currently broken when used with CLONE_SETTLS on all
> architectures that don't have an implementation of copy_thread_tls. The old
> copy_thread function handles CLONE_SETTLS by reading the new TLS value from
> pt_regs containing the clone syscall parameters. Since clone3 passes the TLS
> value in clone_args, this results in the TLS register being initialized to a
> garbage value.
>
> This patch series implements copy_thread_tls on all architectures that currently
> define __ARCH_WANT_SYS_CLONE3 and adds a compile-time check to ensure that any
> architecture that enables clone3 in the future also implements copy_thread_tls.
>
> I have also included a minor fix for the arm64 uapi headers which caused
> __NR_clone3 to be missing from the exported user headers.
>
> I have only tested this on arm64, but the copy_thread_tls implementations for
> the various architectures are fairly straightforward.
I've picked up this series and moved it into
https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=clone3_tls
If I hear no objections I'll merge into into my fixes tree today or
tomorrow.
Thanks!
Christian
On Sun, Jan 05, 2020 at 04:19:28PM +0100, Christian Brauner wrote:
> On Sat, Jan 04, 2020 at 01:39:30PM +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
>
> Thanks. I'm picking this up as part of the copy_thread_tls() series.
> (Leaving the patch in tact so people can Ack right here if they want to.)
> If I could get an Ack from one of the maintainers that would be great;
> see
> https://lore.kernel.org/lkml/[email protected]
> for more context.
>
> Acked-by: Christian Brauner <[email protected]>
I've this up as part of the series link in above ^^ and moved it into
https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=clone3_tls
If I hear no objections I'll merge into into my fixes tree today or
tomorrow.
An Ack from one of the maintainers would still be appreciated.
Thanks!
Christian
On Fri, Jan 17, 2020 at 01:12:31AM +0100, Richard Weinberger wrote:
> On Tue, Jan 7, 2020 at 1:36 PM Christian Brauner
> <[email protected]> wrote:
> >
> > On Sun, Jan 05, 2020 at 04:19:28PM +0100, Christian Brauner wrote:
> > > On Sat, Jan 04, 2020 at 01:39:30PM +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
> > >
> > > Thanks. I'm picking this up as part of the copy_thread_tls() series.
> > > (Leaving the patch in tact so people can Ack right here if they want to.)
> > > If I could get an Ack from one of the maintainers that would be great;
> > > see
> > > https://lore.kernel.org/lkml/[email protected]
> > > for more context.
> > >
> > > Acked-by: Christian Brauner <[email protected]>
> >
> > I've this up as part of the series link in above ^^ and moved it into
> > https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=clone3_tls
> >
> > If I hear no objections I'll merge into into my fixes tree today or
> > tomorrow.
> >
> > An Ack from one of the maintainers would still be appreciated.
>
> For sure too late, but better than nothing? ;-)
>
> Acked-by: Richard Weinberger <[email protected]>
Still worth having it. :)
Thanks!
Christian
On Tue, Jan 7, 2020 at 1:36 PM Christian Brauner
<[email protected]> wrote:
>
> On Sun, Jan 05, 2020 at 04:19:28PM +0100, Christian Brauner wrote:
> > On Sat, Jan 04, 2020 at 01:39:30PM +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
> >
> > Thanks. I'm picking this up as part of the copy_thread_tls() series.
> > (Leaving the patch in tact so people can Ack right here if they want to.)
> > If I could get an Ack from one of the maintainers that would be great;
> > see
> > https://lore.kernel.org/lkml/[email protected]
> > for more context.
> >
> > Acked-by: Christian Brauner <[email protected]>
>
> I've this up as part of the series link in above ^^ and moved it into
> https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=clone3_tls
>
> If I hear no objections I'll merge into into my fixes tree today or
> tomorrow.
>
> An Ack from one of the maintainers would still be appreciated.
For sure too late, but better than nothing? ;-)
Acked-by: Richard Weinberger <[email protected]>
--
Thanks,
//richard