[PATCH 0/7] Fix CLONE_SETTLS with clone3

[PATCH 0/7] Fix CLONE_SETTLS with clone3

[Amanieu d'Antras]

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

[PATCH 1/7] arm64: Move __ARCH_WANT_SYS_CLONE3 definition to uapi headers

[Amanieu d'Antras]

Previously this was only defined in the internal headers which
resulted in __NR_clone3 not being defined in the user headers.

Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: <stable@vger.kernel.org> # 5.3.x
---
 arch/arm64/include/asm/unistd.h      | 1 -
 arch/arm64/include/uapi/asm/unistd.h | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index 2629a68b8724..5af82587909e 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -42,7 +42,6 @@
 #endif
 
 #define __ARCH_WANT_SYS_CLONE
-#define __ARCH_WANT_SYS_CLONE3
 
 #ifndef __COMPAT_SYSCALL_NR
 #include <uapi/asm/unistd.h>
diff --git a/arch/arm64/include/uapi/asm/unistd.h b/arch/arm64/include/uapi/asm/unistd.h
index 4703d218663a..f83a70e07df8 100644
--- a/arch/arm64/include/uapi/asm/unistd.h
+++ b/arch/arm64/include/uapi/asm/unistd.h
@@ -19,5 +19,6 @@
 #define __ARCH_WANT_NEW_STAT
 #define __ARCH_WANT_SET_GET_RLIMIT
 #define __ARCH_WANT_TIME32_SYSCALLS
+#define __ARCH_WANT_SYS_CLONE3
 
 #include <asm-generic/unistd.h>
-- 
2.24.1

[PATCH 2/7] arm64: Implement copy_thread_tls

[Amanieu d'Antras]

This is required for clone3 which passes the TLS value through a
struct rather than a register.

Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: <stable@vger.kernel.org> # 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

[PATCH 3/7] arm: Implement copy_thread_tls

[Amanieu d'Antras]

This is required for clone3 which passes the TLS value through a
struct rather than a register.

Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: <stable@vger.kernel.org> # 5.3.x
---
 arch/arm/Kconfig          | 1 +
 arch/arm/kernel/process.c | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index ba75e3661a41..96dab76da3b3 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -72,6 +72,7 @@ config ARM
 	select HAVE_ARM_SMCCC if CPU_V7
 	select HAVE_EBPF_JIT if !CPU_ENDIAN_BE32
 	select HAVE_CONTEXT_TRACKING
+	select HAVE_COPY_THREAD_TLS
 	select HAVE_C_RECORDMCOUNT
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DMA_CONTIGUOUS if MMU
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index cea1c27c29cb..46e478fb5ea2 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -226,8 +226,8 @@ void release_thread(struct task_struct *dead_task)
 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)
+copy_thread_tls(unsigned long clone_flags, unsigned long stack_start,
+	    unsigned long stk_sz, struct task_struct *p, unsigned long tls)
 {
 	struct thread_info *thread = task_thread_info(p);
 	struct pt_regs *childregs = task_pt_regs(p);
@@ -261,7 +261,7 @@ copy_thread(unsigned long clone_flags, unsigned long stack_start,
 	clear_ptrace_hw_breakpoint(p);
 
 	if (clone_flags & CLONE_SETTLS)
-		thread->tp_value[0] = childregs->ARM_r3;
+		thread->tp_value[0] = tls;
 	thread->tp_value[1] = get_tpuser();
 
 	thread_notify(THREAD_NOTIFY_COPY, thread);
-- 
2.24.1

[PATCH 4/7] parisc: Implement copy_thread_tls

[Amanieu d'Antras]

This is required for clone3 which passes the TLS value through a
struct rather than a register.

Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
Cc: linux-parisc@vger.kernel.org
Cc: <stable@vger.kernel.org> # 5.3.x
---
 arch/parisc/Kconfig          | 1 +
 arch/parisc/kernel/process.c | 8 ++++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index b16237c95ea3..0c29d6cb2c8d 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -62,6 +62,7 @@ config PARISC
 	select HAVE_FTRACE_MCOUNT_RECORD if HAVE_DYNAMIC_FTRACE
 	select HAVE_KPROBES_ON_FTRACE
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS
+	select HAVE_COPY_THREAD_TLS
 
 	help
 	  The PA-RISC microprocessor is designed by Hewlett-Packard and used
diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c
index ecc5c2771208..230a6422b99f 100644
--- a/arch/parisc/kernel/process.c
+++ b/arch/parisc/kernel/process.c
@@ -208,8 +208,8 @@ arch_initcall(parisc_idle_init);
  * Copy architecture-specific thread state
  */
 int
-copy_thread(unsigned long clone_flags, unsigned long usp,
-	    unsigned long kthread_arg, struct task_struct *p)
+copy_thread_tls(unsigned long clone_flags, unsigned long usp,
+	    unsigned long kthread_arg, struct task_struct *p, unsigned long tls)
 {
 	struct pt_regs *cregs = &(p->thread.regs);
 	void *stack = task_stack_page(p);
@@ -254,9 +254,9 @@ copy_thread(unsigned long clone_flags, unsigned long usp,
 		cregs->ksp = (unsigned long)stack + THREAD_SZ_ALGN + FRAME_SIZE;
 		cregs->kpc = (unsigned long) &child_return;
 
-		/* Setup thread TLS area from the 4th parameter in clone */
+		/* Setup thread TLS area */
 		if (clone_flags & CLONE_SETTLS)
-			cregs->cr27 = cregs->gr[23];
+			cregs->cr27 = tls;
 	}
 
 	return 0;
-- 
2.24.1

[PATCH 5/7] riscv: Implement copy_thread_tls

[Amanieu d'Antras]

This is required for clone3 which passes the TLS value through a
struct rather than a register.

Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
Cc: linux-riscv@lists.infradead.org
Cc: <stable@vger.kernel.org> # 5.3.x
---
 arch/riscv/Kconfig          | 1 +
 arch/riscv/kernel/process.c | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index d8efbaa78d67..d6c3d44f96b5 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -64,6 +64,7 @@ config RISCV
 	select SPARSEMEM_STATIC if 32BIT
 	select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
 	select HAVE_ARCH_MMAP_RND_BITS if MMU
+	select HAVE_COPY_THREAD_TLS
 
 config ARCH_MMAP_RND_BITS_MIN
 	default 18 if 64BIT
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index 95a3031e5c7c..817cf7b0974c 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -99,8 +99,8 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 	return 0;
 }
 
-int copy_thread(unsigned long clone_flags, unsigned long usp,
-	unsigned long arg, struct task_struct *p)
+int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
+	unsigned long arg, struct task_struct *p, unsigned long tls)
 {
 	struct pt_regs *childregs = task_pt_regs(p);
 
@@ -121,7 +121,7 @@ int copy_thread(unsigned long clone_flags, unsigned long usp,
 		if (usp) /* User fork */
 			childregs->sp = usp;
 		if (clone_flags & CLONE_SETTLS)
-			childregs->tp = childregs->a5;
+			childregs->tp = tls;
 		childregs->a0 = 0; /* Return value of fork() */
 		p->thread.ra = (unsigned long)ret_from_fork;
 	}
-- 
2.24.1

[PATCH 6/7] xtensa: Implement copy_thread_tls

[Amanieu d'Antras]

This is required for clone3 which passes the TLS value through a
struct rather than a register.

Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
Cc: linux-xtensa@linux-xtensa.org
Cc: <stable@vger.kernel.org> # 5.3.x
---
 arch/xtensa/Kconfig          | 1 +
 arch/xtensa/kernel/process.c | 8 ++++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/xtensa/Kconfig b/arch/xtensa/Kconfig
index 4a3fa295d8fe..296c5324dace 100644
--- a/arch/xtensa/Kconfig
+++ b/arch/xtensa/Kconfig
@@ -24,6 +24,7 @@ config XTENSA
 	select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL
 	select HAVE_ARCH_KASAN if MMU && !XIP_KERNEL
 	select HAVE_ARCH_TRACEHOOK
+	select HAVE_COPY_THREAD_TLS
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DMA_CONTIGUOUS
 	select HAVE_EXIT_THREAD
diff --git a/arch/xtensa/kernel/process.c b/arch/xtensa/kernel/process.c
index 9e1c49134c07..3edecc41ef8c 100644
--- a/arch/xtensa/kernel/process.c
+++ b/arch/xtensa/kernel/process.c
@@ -202,8 +202,9 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
  * involved.  Much simpler to just not copy those live frames across.
  */
 
-int copy_thread(unsigned long clone_flags, unsigned long usp_thread_fn,
-		unsigned long thread_fn_arg, struct task_struct *p)
+int copy_thread_tls(unsigned long clone_flags, unsigned long usp_thread_fn,
+		unsigned long thread_fn_arg, struct task_struct *p,
+		unsigned long tls)
 {
 	struct pt_regs *childregs = task_pt_regs(p);
 
@@ -266,9 +267,8 @@ int copy_thread(unsigned long clone_flags, unsigned long usp_thread_fn,
 
 		childregs->syscall = regs->syscall;
 
-		/* The thread pointer is passed in the '4th argument' (= a5) */
 		if (clone_flags & CLONE_SETTLS)
-			childregs->threadptr = childregs->areg[5];
+			childregs->threadptr = tls;
 	} else {
 		p->thread.ra = MAKE_RA_FOR_CALL(
 				(unsigned long)ret_from_kernel_thread, 1);
-- 
2.24.1

[PATCH 7/7] clone3: ensure copy_thread_tls is implemented

[Amanieu d'Antras]

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 <amanieu@gmail.com>
Cc: <stable@vger.kernel.org> # 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

Re: [PATCH 1/7] arm64: Move __ARCH_WANT_SYS_CLONE3 definition to uapi headers

[Christian Brauner]

[Cc Arnd. I'd like his Ack on this]

On Thu, Jan 02, 2020 at 06:24:07PM +0100, Amanieu d'Antras wrote:
> Previously this was only defined in the internal headers which
> resulted in __NR_clone3 not being defined in the user headers.
> 
> Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: <stable@vger.kernel.org> # 5.3.x
> ---
>  arch/arm64/include/asm/unistd.h      | 1 -
>  arch/arm64/include/uapi/asm/unistd.h | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
> index 2629a68b8724..5af82587909e 100644
> --- a/arch/arm64/include/asm/unistd.h
> +++ b/arch/arm64/include/asm/unistd.h
> @@ -42,7 +42,6 @@
>  #endif
>  
>  #define __ARCH_WANT_SYS_CLONE
> -#define __ARCH_WANT_SYS_CLONE3
>  
>  #ifndef __COMPAT_SYSCALL_NR
>  #include <uapi/asm/unistd.h>
> diff --git a/arch/arm64/include/uapi/asm/unistd.h b/arch/arm64/include/uapi/asm/unistd.h
> index 4703d218663a..f83a70e07df8 100644
> --- a/arch/arm64/include/uapi/asm/unistd.h
> +++ b/arch/arm64/include/uapi/asm/unistd.h
> @@ -19,5 +19,6 @@
>  #define __ARCH_WANT_NEW_STAT
>  #define __ARCH_WANT_SET_GET_RLIMIT
>  #define __ARCH_WANT_TIME32_SYSCALLS
> +#define __ARCH_WANT_SYS_CLONE3
>  
>  #include <asm-generic/unistd.h>
> -- 
> 2.24.1
> 

Re: [PATCH 2/7] arm64: Implement copy_thread_tls

[Christian Brauner]

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 <amanieu@gmail.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: <stable@vger.kernel.org> # 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 <christian.brauner@ubuntu.com>

> ---
>  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
> 

Re: [PATCH 3/7] arm: Implement copy_thread_tls

[Christian Brauner]

On Thu, Jan 02, 2020 at 06:24:09PM +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 <amanieu@gmail.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: <stable@vger.kernel.org> # 5.3.x

Again, looks good to me but I'd like an ack from someone closer to the
architecture itself.
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

> ---
>  arch/arm/Kconfig          | 1 +
>  arch/arm/kernel/process.c | 6 +++---
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index ba75e3661a41..96dab76da3b3 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -72,6 +72,7 @@ config ARM
>  	select HAVE_ARM_SMCCC if CPU_V7
>  	select HAVE_EBPF_JIT if !CPU_ENDIAN_BE32
>  	select HAVE_CONTEXT_TRACKING
> +	select HAVE_COPY_THREAD_TLS
>  	select HAVE_C_RECORDMCOUNT
>  	select HAVE_DEBUG_KMEMLEAK
>  	select HAVE_DMA_CONTIGUOUS if MMU
> diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
> index cea1c27c29cb..46e478fb5ea2 100644
> --- a/arch/arm/kernel/process.c
> +++ b/arch/arm/kernel/process.c
> @@ -226,8 +226,8 @@ void release_thread(struct task_struct *dead_task)
>  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)
> +copy_thread_tls(unsigned long clone_flags, unsigned long stack_start,
> +	    unsigned long stk_sz, struct task_struct *p, unsigned long tls)
>  {
>  	struct thread_info *thread = task_thread_info(p);
>  	struct pt_regs *childregs = task_pt_regs(p);
> @@ -261,7 +261,7 @@ copy_thread(unsigned long clone_flags, unsigned long stack_start,
>  	clear_ptrace_hw_breakpoint(p);
>  
>  	if (clone_flags & CLONE_SETTLS)
> -		thread->tp_value[0] = childregs->ARM_r3;
> +		thread->tp_value[0] = tls;
>  	thread->tp_value[1] = get_tpuser();
>  
>  	thread_notify(THREAD_NOTIFY_COPY, thread);
> -- 
> 2.24.1
> 

Re: [PATCH 7/7] clone3: ensure copy_thread_tls is implemented

[Christian Brauner]

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 <amanieu@gmail.com>
> Cc: <stable@vger.kernel.org> # 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 <christian.brauner@ubuntu.com>

Re: [PATCH 0/7] Fix CLONE_SETTLS with clone3

[Christian Brauner]

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 <christian.brauner@ubuntu.com>

Re: [PATCH 7/7] clone3: ensure copy_thread_tls is implemented

[Amanieu d'Antras]

On Thu, Jan 2, 2020 at 7:09 PM Christian Brauner
<christian.brauner@ubuntu.com> 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.

Re: [PATCH 7/7] clone3: ensure copy_thread_tls is implemented

[Christian Brauner]

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
> <christian.brauner@ubuntu.com> 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

Re: [PATCH 1/7] arm64: Move __ARCH_WANT_SYS_CLONE3 definition to uapi headers

[Arnd Bergmann]

On Thu, Jan 2, 2020 at 6:50 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
> On Thu, Jan 02, 2020 at 06:24:07PM +0100, Amanieu d'Antras wrote:
> > Previously this was only defined in the internal headers which
> > resulted in __NR_clone3 not being defined in the user headers.
> >
> > Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: <stable@vger.kernel.org> # 5.3.x
> > ---
> >  arch/arm64/include/asm/unistd.h      | 1 -
> >  arch/arm64/include/uapi/asm/unistd.h | 1 +
> >  2 files changed, 1 insertion(+), 1 deletion(-)

Good catch, this is clearly needed, but please make the patch change
every copy of asm/unistd.h that defines this, not just the arm64 one.

       Arnd

Re: [PATCH 1/7] arm64: Move __ARCH_WANT_SYS_CLONE3 definition to uapi headers

[Amanieu d'Antras]

On Thu, Jan 2, 2020 at 8:25 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Jan 2, 2020 at 6:50 PM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> > On Thu, Jan 02, 2020 at 06:24:07PM +0100, Amanieu d'Antras wrote:
> > > Previously this was only defined in the internal headers which
> > > resulted in __NR_clone3 not being defined in the user headers.
> > >
> > > Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > Cc: <stable@vger.kernel.org> # 5.3.x
> > > ---
> > >  arch/arm64/include/asm/unistd.h      | 1 -
> > >  arch/arm64/include/uapi/asm/unistd.h | 1 +
> > >  2 files changed, 1 insertion(+), 1 deletion(-)
>
> Good catch, this is clearly needed, but please make the patch change
> every copy of asm/unistd.h that defines this, not just the arm64 one.

Actually __ARCH_WANT_SYS_CLONE3 only needs to be in the uapi headers
for architectures that use the asm-generic/unistd.h header, which uses
it to guard the definition of __NR_clone3. Architectures not using the
asm-generic header don't need this define to export __NR_clone3. The
only other architecture with clone3 that uses the asm-generic header
is riscv, which already defines __ARCH_WANT_SYS_CLONE3 in the uapi
headers.

Re: [PATCH 1/7] arm64: Move __ARCH_WANT_SYS_CLONE3 definition to uapi headers

[Arnd Bergmann]

On Thu, Jan 2, 2020 at 8:33 PM Amanieu d'Antras <amanieu@gmail.com> wrote:
>
> On Thu, Jan 2, 2020 at 8:25 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thu, Jan 2, 2020 at 6:50 PM Christian Brauner
> > <christian.brauner@ubuntu.com> wrote:
> > > On Thu, Jan 02, 2020 at 06:24:07PM +0100, Amanieu d'Antras wrote:
> > > > Previously this was only defined in the internal headers which
> > > > resulted in __NR_clone3 not being defined in the user headers.
> > > >
> > > > Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
> > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > Cc: <stable@vger.kernel.org> # 5.3.x
> > > > ---
> > > >  arch/arm64/include/asm/unistd.h      | 1 -
> > > >  arch/arm64/include/uapi/asm/unistd.h | 1 +
> > > >  2 files changed, 1 insertion(+), 1 deletion(-)
> >
> > Good catch, this is clearly needed, but please make the patch change
> > every copy of asm/unistd.h that defines this, not just the arm64 one.
>
> Actually __ARCH_WANT_SYS_CLONE3 only needs to be in the uapi headers
> for architectures that use the asm-generic/unistd.h header, which uses
> it to guard the definition of __NR_clone3. Architectures not using the
> asm-generic header don't need this define to export __NR_clone3. The
> only other architecture with clone3 that uses the asm-generic header
> is riscv, which already defines __ARCH_WANT_SYS_CLONE3 in the uapi
> headers.

Ah, of course. The patch looks fine to me then.

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

[PATCH] um: Implement copy_thread_tls

[Amanieu d'Antras]

This is required for clone3 which passes the TLS value through a
struct rather than a register.

Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
Cc: linux-um@lists.infradead.org
Cc: <stable@vger.kernel.org> # 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(&current->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

Re: [PATCH] um: Implement copy_thread_tls

[Christian Brauner]

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 <amanieu@gmail.com>
> Cc: linux-um@lists.infradead.org
> Cc: <stable@vger.kernel.org> # 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/20200102172413.654385-1-amanieu@gmail.com
for more context.

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

> ---
>  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(&current->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
> 

Re: [PATCH 2/7] arm64: Implement copy_thread_tls

[Will Deacon]

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 <amanieu@gmail.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: <stable@vger.kernel.org> # 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 <will@kernel.org>

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

Re: [PATCH 2/7] arm64: Implement copy_thread_tls

[Amanieu d'Antras]

On Mon, Jan 6, 2020 at 6:39 PM Will Deacon <will@kernel.org> 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());
}

Re: [PATCH 2/7] arm64: Implement copy_thread_tls

[Christian Brauner]

[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 <will@kernel.org> 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());
> }

Re: [PATCH 0/7] Fix CLONE_SETTLS with clone3

[Christian Brauner]

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

Re: [PATCH] um: Implement copy_thread_tls

[Christian Brauner]

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 <amanieu@gmail.com>
> > Cc: linux-um@lists.infradead.org
> > Cc: <stable@vger.kernel.org> # 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/20200102172413.654385-1-amanieu@gmail.com
> for more context.
> 
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

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

Re: [PATCH 2/7] arm64: Implement copy_thread_tls

[Will Deacon]

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 <will@kernel.org> 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

Re: [PATCH 2/7] arm64: Implement copy_thread_tls

[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]
> > [...]
> > 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

Re: [PATCH 2/7] arm64: Implement copy_thread_tls

[Christian Brauner]

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 <will@kernel.org> 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

Re: [PATCH 2/7] arm64: Implement copy_thread_tls

[Christian Brauner]

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

Re: [PATCH] um: Implement copy_thread_tls

[Richard Weinberger]

On Tue, Jan 7, 2020 at 1:36 PM Christian Brauner
<christian.brauner@ubuntu.com> 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 <amanieu@gmail.com>
> > > Cc: linux-um@lists.infradead.org
> > > Cc: <stable@vger.kernel.org> # 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/20200102172413.654385-1-amanieu@gmail.com
> > for more context.
> >
> > Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
>
> 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 <richard@nod.at>

-- 
Thanks,
//richard

Re: [PATCH] um: Implement copy_thread_tls

[Christian Brauner]

On Fri, Jan 17, 2020 at 01:12:31AM +0100, Richard Weinberger wrote:
> On Tue, Jan 7, 2020 at 1:36 PM Christian Brauner
> <christian.brauner@ubuntu.com> 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 <amanieu@gmail.com>
> > > > Cc: linux-um@lists.infradead.org
> > > > Cc: <stable@vger.kernel.org> # 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/20200102172413.654385-1-amanieu@gmail.com
> > > for more context.
> > >
> > > Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> >
> > 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 <richard@nod.at>

Still worth having it. :)

Thanks!
Christian