[RFC PATCH 1/4] rseq: Fix: Reject unknown flags on rseq unregister

[RFC PATCH 1/4] rseq: Fix: Reject unknown flags on rseq unregister

[Mathieu Desnoyers]

It is preferrable to reject unknown flags within rseq unregistration
rather than to ignore them. It is an oversight caused by the fact that
the check for unknown flags is after the rseq unregister flag check.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Paul Turner <pjt@google.com>
Cc: linux-api@vger.kernel.org
---
 kernel/rseq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 27c48eb7de40..a4f86a9d6937 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -310,6 +310,8 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32, rseq_len,
 	int ret;
 
 	if (flags & RSEQ_FLAG_UNREGISTER) {
+		if (flags & ~RSEQ_FLAG_UNREGISTER)
+			return -EINVAL;
 		/* Unregister rseq for current thread. */
 		if (current->rseq != rseq || !current->rseq)
 			return -EINVAL;
-- 
2.17.1

[RFC PATCH 2/4] rseq: Fix: Unregister rseq for CLONE_TLS

[Mathieu Desnoyers]

It has been reported by Google that rseq is not behaving properly
with respect to clone when CLONE_VM is used without CLONE_THREAD.
It keeps the prior thread's rseq TLS registered when the TLS of the
thread has moved, so the kernel deals with the wrong TLS.

The approach of clearing the per task-struct rseq registration
on clone with CLONE_THREAD flag is incomplete. It does not cover
the use-case of clone with CLONE_VM set, but without CLONE_THREAD.

Looking more closely at each of the clone flags:

- CLONE_THREAD,
- CLONE_VM,
- CLONE_SETTLS.

It appears that the flag we really want to track is CLONE_SETTLS, which
moves the location of the TLS for the child, which makes the rseq
registration point to the wrong TLS.

Suggested-by: "H . Peter Anvin" <hpa@zytor.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Paul Turner <pjt@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: linux-api@vger.kernel.org
---
 include/linux/sched.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 9f51932bd543..deb4154dbf11 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1919,11 +1919,11 @@ static inline void rseq_migrate(struct task_struct *t)
 
 /*
  * If parent process has a registered restartable sequences area, the
- * child inherits. Only applies when forking a process, not a thread.
+ * child inherits. Unregister rseq for a clone with CLONE_TLS set.
  */
 static inline void rseq_fork(struct task_struct *t, unsigned long clone_flags)
 {
-	if (clone_flags & CLONE_THREAD) {
+	if (clone_flags & CLONE_TLS) {
 		t->rseq = NULL;
 		t->rseq_sig = 0;
 		t->rseq_event_mask = 0;
-- 
2.17.1

Re: [RFC PATCH 2/4] rseq: Fix: Unregister rseq for CLONE_TLS

[Mathieu Desnoyers]

Of course, this patch title should read:

  rseq: Fix: Unregister rseq for CLONE_SETTLS

----- On Sep 11, 2019, at 1:27 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

 
> /*
>  * If parent process has a registered restartable sequences area, the
> - * child inherits. Only applies when forking a process, not a thread.
> + * child inherits. Unregister rseq for a clone with CLONE_TLS set.

and here CLONE_SETTLS as well.

>  */
> static inline void rseq_fork(struct task_struct *t, unsigned long clone_flags)
> {
> -	if (clone_flags & CLONE_THREAD) {
> +	if (clone_flags & CLONE_TLS) {

.. and here.

Thanks,

Mathieu

> 		t->rseq = NULL;
> 		t->rseq_sig = 0;
> 		t->rseq_event_mask = 0;
> --
> 2.17.1

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[RFC PATCH 3/4] rseq: Introduce unreg_clone_flags

[Mathieu Desnoyers]

Considering that some custom libc could possibly choose not to use
CLONE_SETTLS, we should allow the libc to override the choice of clone
flags meant to unregister rseq. This is a policy decision which should
not be made by the kernel.

Therefore, introduce a new RSEQ_FLAG_UNREG_CLONE_FLAGS, which makes the
rseq system call expect an additional 5th argument: a mask of all the
clone flags which may each ensure rseq is unregistered upon clone.

So even if CLONE_SETTLS is eventually replaced by some other flag in the
future, the libc will be able to adapt and pass this new flag upon rseq
registration as well.

The default when RSEQ_FLAG_UNREG_CLONE_FLAGS is unset is to unregister
rseq on clone with CLONE_SETTLS.

Suggested-by: "H . Peter Anvin" <hpa@zytor.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Paul Turner <pjt@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: linux-api@vger.kernel.org
---
 include/linux/sched.h     |  9 +++++++--
 include/linux/syscalls.h  |  2 +-
 include/uapi/linux/rseq.h |  1 +
 kernel/rseq.c             | 14 +++++++++++---
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index deb4154dbf11..c8faa6f8493d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1138,6 +1138,7 @@ struct task_struct {
 	 * with respect to preemption.
 	 */
 	unsigned long rseq_event_mask;
+	int rseq_unreg_clone_flags;
 #endif
 
 	struct tlbflush_unmap_batch	tlb_ubc;
@@ -1919,18 +1920,21 @@ static inline void rseq_migrate(struct task_struct *t)
 
 /*
  * If parent process has a registered restartable sequences area, the
- * child inherits. Unregister rseq for a clone with CLONE_TLS set.
+ * child inherits, except if it has been required to be explicitly
+ * unregistered when any of the rseq_unreg_clone_flags are passed to clone.
  */
 static inline void rseq_fork(struct task_struct *t, unsigned long clone_flags)
 {
-	if (clone_flags & CLONE_TLS) {
+	if (clone_flags & t->rseq_unreg_clone_flags) {
 		t->rseq = NULL;
 		t->rseq_sig = 0;
 		t->rseq_event_mask = 0;
+		t->rseq_unreg_clone_flags = 0;
 	} else {
 		t->rseq = current->rseq;
 		t->rseq_sig = current->rseq_sig;
 		t->rseq_event_mask = current->rseq_event_mask;
+		t->rseq_unreg_clone_flags = current->rseq_unreg_clone_flags;
 	}
 }
 
@@ -1939,6 +1943,7 @@ static inline void rseq_execve(struct task_struct *t)
 	t->rseq = NULL;
 	t->rseq_sig = 0;
 	t->rseq_event_mask = 0;
+	t->rseq_unreg_clone_flags = 0;
 }
 
 #else
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 88145da7d140..6a242cfcc360 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -987,7 +987,7 @@ asmlinkage long sys_pkey_free(int pkey);
 asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
 			  unsigned mask, struct statx __user *buffer);
 asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
-			 int flags, uint32_t sig);
+			 int flags, uint32_t sig, int unreg_clone_flags);
 asmlinkage long sys_open_tree(int dfd, const char __user *path, unsigned flags);
 asmlinkage long sys_move_mount(int from_dfd, const char __user *from_path,
 			       int to_dfd, const char __user *to_path,
diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index 9a402fdb60e9..d71e3c6b7fdb 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -20,6 +20,7 @@ enum rseq_cpu_id_state {
 
 enum rseq_flags {
 	RSEQ_FLAG_UNREGISTER = (1 << 0),
+	RSEQ_FLAG_UNREG_CLONE_FLAGS = (1 << 1),
 };
 
 enum rseq_cs_flags_bit {
diff --git a/kernel/rseq.c b/kernel/rseq.c
index a4f86a9d6937..c59b8d3dc275 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -304,8 +304,8 @@ void rseq_syscall(struct pt_regs *regs)
 /*
  * sys_rseq - setup restartable sequences for caller thread.
  */
-SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32, rseq_len,
-		int, flags, u32, sig)
+SYSCALL_DEFINE5(rseq, struct rseq __user *, rseq, u32, rseq_len,
+		int, flags, u32, sig, int, unreg_clone_flags)
 {
 	int ret;
 
@@ -324,12 +324,16 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32, rseq_len,
 			return ret;
 		current->rseq = NULL;
 		current->rseq_sig = 0;
+		current->rseq_unreg_clone_flags = 0;
 		return 0;
 	}
 
-	if (unlikely(flags))
+	if (unlikely(flags & ~RSEQ_FLAG_UNREG_CLONE_FLAGS))
 		return -EINVAL;
 
+	if (!(flags & RSEQ_FLAG_UNREG_CLONE_FLAGS))
+		unreg_clone_flags = CLONE_SETTLS;
+
 	if (current->rseq) {
 		/*
 		 * If rseq is already registered, check whether
@@ -338,6 +342,9 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32, rseq_len,
 		 */
 		if (current->rseq != rseq || rseq_len != sizeof(*rseq))
 			return -EINVAL;
+		if ((flags & RSEQ_FLAG_UNREG_CLONE_FLAGS) &&
+		    current->rseq_unreg_clone_flags != unreg_clone_flags)
+			return -EINVAL;
 		if (current->rseq_sig != sig)
 			return -EPERM;
 		/* Already registered. */
@@ -355,6 +362,7 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32, rseq_len,
 		return -EFAULT;
 	current->rseq = rseq;
 	current->rseq_sig = sig;
+	current->rseq_unreg_clone_flags = unreg_clone_flags;
 	/*
 	 * If rseq was previously inactive, and has just been
 	 * registered, ensure the cpu_id_start and cpu_id fields
-- 
2.17.1

Re: [RFC PATCH 3/4] rseq: Introduce unreg_clone_flags

[Mathieu Desnoyers]

----- On Sep 10, 2019, at 8:27 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> Considering that some custom libc could possibly choose not to use
> CLONE_SETTLS, we should allow the libc to override the choice of clone
> flags meant to unregister rseq. This is a policy decision which should
> not be made by the kernel.
> 
> Therefore, introduce a new RSEQ_FLAG_UNREG_CLONE_FLAGS, which makes the
> rseq system call expect an additional 5th argument: a mask of all the
> clone flags which may each ensure rseq is unregistered upon clone.
> 
> So even if CLONE_SETTLS is eventually replaced by some other flag in the
> future, the libc will be able to adapt and pass this new flag upon rseq
> registration as well.
> 
> The default when RSEQ_FLAG_UNREG_CLONE_FLAGS is unset is to unregister
> rseq on clone with CLONE_SETTLS.

Now that I think about this a bit more, perhaps it's better to just
clear on clone CLONE_SETTLS, and wait until a libc actually requires this
unreg_clone_flags override before introducing it.

Anyway, a "custom" libc could unregister / re-register around clone()
as a stopgap solution.

So I am tempted to drop this specific patch for now until actual libc
requiring it show up.

Thoughts ?

Thanks,

Mathieu

> 
> Suggested-by: "H . Peter Anvin" <hpa@zytor.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: "H . Peter Anvin" <hpa@zytor.com>
> Cc: Paul Turner <pjt@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: linux-api@vger.kernel.org
> ---
> include/linux/sched.h     |  9 +++++++--
> include/linux/syscalls.h  |  2 +-
> include/uapi/linux/rseq.h |  1 +
> kernel/rseq.c             | 14 +++++++++++---
> 4 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index deb4154dbf11..c8faa6f8493d 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1138,6 +1138,7 @@ struct task_struct {
> 	 * with respect to preemption.
> 	 */
> 	unsigned long rseq_event_mask;
> +	int rseq_unreg_clone_flags;
> #endif
> 
> 	struct tlbflush_unmap_batch	tlb_ubc;
> @@ -1919,18 +1920,21 @@ static inline void rseq_migrate(struct task_struct *t)
> 
> /*
>  * If parent process has a registered restartable sequences area, the
> - * child inherits. Unregister rseq for a clone with CLONE_TLS set.
> + * child inherits, except if it has been required to be explicitly
> + * unregistered when any of the rseq_unreg_clone_flags are passed to clone.
>  */
> static inline void rseq_fork(struct task_struct *t, unsigned long clone_flags)
> {
> -	if (clone_flags & CLONE_TLS) {
> +	if (clone_flags & t->rseq_unreg_clone_flags) {
> 		t->rseq = NULL;
> 		t->rseq_sig = 0;
> 		t->rseq_event_mask = 0;
> +		t->rseq_unreg_clone_flags = 0;
> 	} else {
> 		t->rseq = current->rseq;
> 		t->rseq_sig = current->rseq_sig;
> 		t->rseq_event_mask = current->rseq_event_mask;
> +		t->rseq_unreg_clone_flags = current->rseq_unreg_clone_flags;
> 	}
> }
> 
> @@ -1939,6 +1943,7 @@ static inline void rseq_execve(struct task_struct *t)
> 	t->rseq = NULL;
> 	t->rseq_sig = 0;
> 	t->rseq_event_mask = 0;
> +	t->rseq_unreg_clone_flags = 0;
> }
> 
> #else
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 88145da7d140..6a242cfcc360 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -987,7 +987,7 @@ asmlinkage long sys_pkey_free(int pkey);
> asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
> 			  unsigned mask, struct statx __user *buffer);
> asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
> -			 int flags, uint32_t sig);
> +			 int flags, uint32_t sig, int unreg_clone_flags);
> asmlinkage long sys_open_tree(int dfd, const char __user *path, unsigned flags);
> asmlinkage long sys_move_mount(int from_dfd, const char __user *from_path,
> 			       int to_dfd, const char __user *to_path,
> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
> index 9a402fdb60e9..d71e3c6b7fdb 100644
> --- a/include/uapi/linux/rseq.h
> +++ b/include/uapi/linux/rseq.h
> @@ -20,6 +20,7 @@ enum rseq_cpu_id_state {
> 
> enum rseq_flags {
> 	RSEQ_FLAG_UNREGISTER = (1 << 0),
> +	RSEQ_FLAG_UNREG_CLONE_FLAGS = (1 << 1),
> };
> 
> enum rseq_cs_flags_bit {
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index a4f86a9d6937..c59b8d3dc275 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -304,8 +304,8 @@ void rseq_syscall(struct pt_regs *regs)
> /*
>  * sys_rseq - setup restartable sequences for caller thread.
>  */
> -SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32, rseq_len,
> -		int, flags, u32, sig)
> +SYSCALL_DEFINE5(rseq, struct rseq __user *, rseq, u32, rseq_len,
> +		int, flags, u32, sig, int, unreg_clone_flags)
> {
> 	int ret;
> 
> @@ -324,12 +324,16 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32,
> rseq_len,
> 			return ret;
> 		current->rseq = NULL;
> 		current->rseq_sig = 0;
> +		current->rseq_unreg_clone_flags = 0;
> 		return 0;
> 	}
> 
> -	if (unlikely(flags))
> +	if (unlikely(flags & ~RSEQ_FLAG_UNREG_CLONE_FLAGS))
> 		return -EINVAL;
> 
> +	if (!(flags & RSEQ_FLAG_UNREG_CLONE_FLAGS))
> +		unreg_clone_flags = CLONE_SETTLS;
> +
> 	if (current->rseq) {
> 		/*
> 		 * If rseq is already registered, check whether
> @@ -338,6 +342,9 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32,
> rseq_len,
> 		 */
> 		if (current->rseq != rseq || rseq_len != sizeof(*rseq))
> 			return -EINVAL;
> +		if ((flags & RSEQ_FLAG_UNREG_CLONE_FLAGS) &&
> +		    current->rseq_unreg_clone_flags != unreg_clone_flags)
> +			return -EINVAL;
> 		if (current->rseq_sig != sig)
> 			return -EPERM;
> 		/* Already registered. */
> @@ -355,6 +362,7 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32,
> rseq_len,
> 		return -EFAULT;
> 	current->rseq = rseq;
> 	current->rseq_sig = sig;
> +	current->rseq_unreg_clone_flags = unreg_clone_flags;
> 	/*
> 	 * If rseq was previously inactive, and has just been
> 	 * registered, ensure the cpu_id_start and cpu_id fields
> --
> 2.17.1

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[RFC PATCH 4/4] rseq/selftests: Use RSEQ_FLAG_UNREG_CLONE_FLAGS

[Mathieu Desnoyers]

Use the new RSEQ_FLAG_UNREG_CLONE_FLAGS rseq flag in the rseq selftests.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Paul Turner <pjt@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
---
 tools/testing/selftests/rseq/rseq.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/rseq/rseq.c b/tools/testing/selftests/rseq/rseq.c
index 7159eb777fd3..d5268570014c 100644
--- a/tools/testing/selftests/rseq/rseq.c
+++ b/tools/testing/selftests/rseq/rseq.c
@@ -68,9 +68,10 @@ static void signal_restore(sigset_t oldset)
 }
 
 static int sys_rseq(volatile struct rseq *rseq_abi, uint32_t rseq_len,
-		    int flags, uint32_t sig)
+		    int flags, uint32_t sig, int unreg_clone_flags)
 {
-	return syscall(__NR_rseq, rseq_abi, rseq_len, flags, sig);
+	return syscall(__NR_rseq, rseq_abi, rseq_len, flags, sig,
+		       unreg_clone_flags);
 }
 
 int rseq_register_current_thread(void)
@@ -87,7 +88,9 @@ int rseq_register_current_thread(void)
 	}
 	if (__rseq_refcount++)
 		goto end;
-	rc = sys_rseq(&__rseq_abi, sizeof(struct rseq), 0, RSEQ_SIG);
+	rc = sys_rseq(&__rseq_abi, sizeof(struct rseq),
+		      RSEQ_FLAG_UNREG_CLONE_FLAGS, RSEQ_SIG,
+		      CLONE_SETTLS);
 	if (!rc) {
 		assert(rseq_current_cpu_raw() >= 0);
 		goto end;
@@ -116,7 +119,7 @@ int rseq_unregister_current_thread(void)
 	if (--__rseq_refcount)
 		goto end;
 	rc = sys_rseq(&__rseq_abi, sizeof(struct rseq),
-		      RSEQ_FLAG_UNREGISTER, RSEQ_SIG);
+		      RSEQ_FLAG_UNREGISTER, RSEQ_SIG, 0);
 	if (!rc)
 		goto end;
 	__rseq_refcount = 1;
-- 
2.17.1