linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH for 4.18 0/5] Restartable Sequences updates
@ 2018-07-05 18:05 Mathieu Desnoyers
  2018-07-05 18:05 ` [RFC PATCH for 4.18 1/5] rseq: use __u64 for rseq_cs fields, validate user inputs Mathieu Desnoyers
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Mathieu Desnoyers @ 2018-07-05 18:05 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E . McKenney,
	Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
	Andrew Morton, Russell King, Ingo Molnar, H . Peter Anvin,
	Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
	Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
	Michael Kerrisk, Joel Fernandes, Mathieu Desnoyers

Following the recent discussion thread [1] about rseq uapi, here is
a set of updates submitted for comments.

Thanks,

Mathieu

[1] https://lkml.kernel.org/r/20180702223143.4663-1-mathieu.desnoyers@efficios.com

Mathieu Desnoyers (5):
  rseq: use __u64 for rseq_cs fields, validate user inputs
  rseq: uapi: update uapi comments
  rseq: uapi: declare rseq_cs field as union, update includes
  rseq: remove unused types_32_64.h uapi header
  rseq/selftests: cleanup: update comment above rseq_prepare_unload

 include/uapi/linux/rseq.h           | 102 ++++++++++++++++++++----------------
 include/uapi/linux/types_32_64.h    |  50 ------------------
 kernel/rseq.c                       |  26 +++++----
 tools/testing/selftests/rseq/rseq.h |  24 ++++++---
 4 files changed, 92 insertions(+), 110 deletions(-)
 delete mode 100644 include/uapi/linux/types_32_64.h

-- 
2.11.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH for 4.18 1/5] rseq: use __u64 for rseq_cs fields, validate user inputs
  2018-07-05 18:05 [RFC PATCH for 4.18 0/5] Restartable Sequences updates Mathieu Desnoyers
@ 2018-07-05 18:05 ` Mathieu Desnoyers
  2018-07-05 18:05 ` [RFC PATCH for 4.18 2/5] rseq: uapi: update uapi comments Mathieu Desnoyers
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Mathieu Desnoyers @ 2018-07-05 18:05 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E . McKenney,
	Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
	Andrew Morton, Russell King, Ingo Molnar, H . Peter Anvin,
	Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
	Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
	Michael Kerrisk, Joel Fernandes, Mathieu Desnoyers

Change the rseq ABI so rseq_cs start_ip, post_commit_offset and abort_ip
fields are seen as 64-bit fields by both 32-bit and 64-bit kernels rather
that ignoring the 32 upper bits on 32-bit kernels. This ensures we have a
consistent behavior for a 32-bit binary executed on 32-bit kernels and in
compat mode on 64-bit kernels.

Validating the value of abort_ip field to be below TASK_SIZE ensures the
kernel don't return to an invalid address when returning to userspace
after an abort. I don't fully trust each architecture code to consistently
deal with invalid return addresses.

Validating the value of the start_ip and post_commit_offset fields
prevents overflow on arithmetic performed on those values, used to
check whether abort_ip is within the rseq critical section.

If validation fails, the process is killed with a segmentation fault.

When the signature encountered before abort_ip does not match the expected
signature, return -EINVAL rather than -EPERM to be consistent with other
input validation return codes from rseq_get_rseq_cs().

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Paul Turner <pjt@google.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Andi Kleen <andi@firstfloor.org>
CC: Dave Watson <davejwatson@fb.com>
CC: Chris Lameter <cl@linux.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ben Maurer <bmaurer@fb.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Russell King <linux@arm.linux.org.uk>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: linux-api@vger.kernel.org
---
 include/uapi/linux/rseq.h |  6 +++---
 kernel/rseq.c             | 14 ++++++++++----
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index d620fa43756c..519ad6e176d1 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -52,10 +52,10 @@ struct rseq_cs {
 	__u32 version;
 	/* enum rseq_cs_flags */
 	__u32 flags;
-	LINUX_FIELD_u32_u64(start_ip);
+	__u64 start_ip;
 	/* Offset from start_ip. */
-	LINUX_FIELD_u32_u64(post_commit_offset);
-	LINUX_FIELD_u32_u64(abort_ip);
+	__u64 post_commit_offset;
+	__u64 abort_ip;
 } __attribute__((aligned(4 * sizeof(__u64))));
 
 /*
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 22b6acf1ad63..16b38c5342f9 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -130,14 +130,20 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
 	urseq_cs = (struct rseq_cs __user *)ptr;
 	if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
 		return -EFAULT;
-	if (rseq_cs->version > 0)
-		return -EINVAL;
 
+	if (rseq_cs->start_ip >= TASK_SIZE ||
+	    rseq_cs->start_ip + rseq_cs->post_commit_offset >= TASK_SIZE ||
+	    rseq_cs->abort_ip >= TASK_SIZE ||
+	    rseq_cs->version > 0)
+		return -EINVAL;
+	/* Check for overflow. */
+	if (rseq_cs->start_ip + rseq_cs->post_commit_offset < rseq_cs->start_ip)
+		return -EINVAL;
 	/* Ensure that abort_ip is not in the critical section. */
 	if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
 		return -EINVAL;
 
-	usig = (u32 __user *)(rseq_cs->abort_ip - sizeof(u32));
+	usig = (u32 __user *)(unsigned long)(rseq_cs->abort_ip - sizeof(u32));
 	ret = get_user(sig, usig);
 	if (ret)
 		return ret;
@@ -146,7 +152,7 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
 		printk_ratelimited(KERN_WARNING
 			"Possible attack attempt. Unexpected rseq signature 0x%x, expecting 0x%x (pid=%d, addr=%p).\n",
 			sig, current->rseq_sig, current->pid, usig);
-		return -EPERM;
+		return -EINVAL;
 	}
 	return 0;
 }
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH for 4.18 2/5] rseq: uapi: update uapi comments
  2018-07-05 18:05 [RFC PATCH for 4.18 0/5] Restartable Sequences updates Mathieu Desnoyers
  2018-07-05 18:05 ` [RFC PATCH for 4.18 1/5] rseq: use __u64 for rseq_cs fields, validate user inputs Mathieu Desnoyers
@ 2018-07-05 18:05 ` Mathieu Desnoyers
  2018-07-05 18:05 ` [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes Mathieu Desnoyers
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Mathieu Desnoyers @ 2018-07-05 18:05 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E . McKenney,
	Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
	Andrew Morton, Russell King, Ingo Molnar, H . Peter Anvin,
	Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
	Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
	Michael Kerrisk, Joel Fernandes, Mathieu Desnoyers

Update rseq uapi header comments to reflect that user-space need to do
thread-local loads/stores from/to the struct rseq fields.

As a consequence of this added requirement, the kernel does not need
to perform loads/stores with single-copy atomicity.

Update the comment associated to the "flags" fields to describe
more accurately that it's only useful to facilitate single-stepping
through rseq critical sections with debuggers.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Paul Turner <pjt@google.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Andi Kleen <andi@firstfloor.org>
CC: Dave Watson <davejwatson@fb.com>
CC: Chris Lameter <cl@linux.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ben Maurer <bmaurer@fb.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Russell King <linux@arm.linux.org.uk>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: linux-api@vger.kernel.org
---
 include/uapi/linux/rseq.h | 69 ++++++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 33 deletions(-)

diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index 519ad6e176d1..bf4188c13bec 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -67,28 +67,30 @@ struct rseq_cs {
 struct rseq {
 	/*
 	 * Restartable sequences cpu_id_start field. Updated by the
-	 * kernel, and read by user-space with single-copy atomicity
-	 * semantics. Aligned on 32-bit. Always contains a value in the
-	 * range of possible CPUs, although the value may not be the
-	 * actual current CPU (e.g. if rseq is not initialized). This
-	 * CPU number value should always be compared against the value
-	 * of the cpu_id field before performing a rseq commit or
-	 * returning a value read from a data structure indexed using
-	 * the cpu_id_start value.
+	 * kernel. Read by user-space with single-copy atomicity
+	 * semantics. This field should only be read by the thread which
+	 * registered this data structure. Aligned on 32-bit. Always
+	 * contains a value in the range of possible CPUs, although the
+	 * value may not be the actual current CPU (e.g. if rseq is not
+	 * initialized). This CPU number value should always be compared
+	 * against the value of the cpu_id field before performing a rseq
+	 * commit or returning a value read from a data structure indexed
+	 * using the cpu_id_start value.
 	 */
 	__u32 cpu_id_start;
 	/*
-	 * Restartable sequences cpu_id field. Updated by the kernel,
-	 * and read by user-space with single-copy atomicity semantics.
-	 * Aligned on 32-bit. Values RSEQ_CPU_ID_UNINITIALIZED and
-	 * RSEQ_CPU_ID_REGISTRATION_FAILED have a special semantic: the
-	 * former means "rseq uninitialized", and latter means "rseq
-	 * initialization failed". This value is meant to be read within
-	 * rseq critical sections and compared with the cpu_id_start
-	 * value previously read, before performing the commit instruction,
-	 * or read and compared with the cpu_id_start value before returning
-	 * a value loaded from a data structure indexed using the
-	 * cpu_id_start value.
+	 * Restartable sequences cpu_id field. Updated by the kernel.
+	 * Read by user-space with single-copy atomicity semantics. This
+	 * field should only be read by the thread which registered this
+	 * data structure. Aligned on 32-bit. Values
+	 * RSEQ_CPU_ID_UNINITIALIZED and RSEQ_CPU_ID_REGISTRATION_FAILED
+	 * have a special semantic: the former means "rseq uninitialized",
+	 * and latter means "rseq initialization failed". This value is
+	 * meant to be read within rseq critical sections and compared
+	 * with the cpu_id_start value previously read, before performing
+	 * the commit instruction, or read and compared with the
+	 * cpu_id_start value before returning a value loaded from a data
+	 * structure indexed using the cpu_id_start value.
 	 */
 	__u32 cpu_id;
 	/*
@@ -105,27 +107,28 @@ struct rseq {
 	 * targeted by the rseq_cs. Also needs to be set to NULL by user-space
 	 * before reclaiming memory that contains the targeted struct rseq_cs.
 	 *
-	 * Read and set by the kernel with single-copy atomicity semantics.
-	 * Set by user-space with single-copy atomicity semantics. Aligned
-	 * on 64-bit.
+	 * Read and set by the kernel. Set by user-space with single-copy
+	 * atomicity semantics. This field should only be updated by the
+	 * thread which registered this data structure. Aligned on 64-bit.
 	 */
 	LINUX_FIELD_u32_u64(rseq_cs);
 	/*
-	 * - RSEQ_DISABLE flag:
+	 * Restartable sequences flags field.
+	 *
+	 * This field should only be updated by the thread which
+	 * registered this data structure. Read by the kernel.
+	 * Mainly used for single-stepping through rseq critical sections
+	 * with debuggers.
 	 *
-	 * Fallback fast-track flag for single-stepping.
-	 * Set by user-space if lack of progress is detected.
-	 * Cleared by user-space after rseq finish.
-	 * Read by the kernel.
 	 * - RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT
-	 *     Inhibit instruction sequence block restart and event
-	 *     counter increment on preemption for this thread.
+	 *     Inhibit instruction sequence block restart on preemption
+	 *     for this thread.
 	 * - RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
-	 *     Inhibit instruction sequence block restart and event
-	 *     counter increment on signal delivery for this thread.
+	 *     Inhibit instruction sequence block restart on signal
+	 *     delivery for this thread.
 	 * - RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
-	 *     Inhibit instruction sequence block restart and event
-	 *     counter increment on migration for this thread.
+	 *     Inhibit instruction sequence block restart on migration for
+	 *     this thread.
 	 */
 	__u32 flags;
 } __attribute__((aligned(4 * sizeof(__u64))));
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes
  2018-07-05 18:05 [RFC PATCH for 4.18 0/5] Restartable Sequences updates Mathieu Desnoyers
  2018-07-05 18:05 ` [RFC PATCH for 4.18 1/5] rseq: use __u64 for rseq_cs fields, validate user inputs Mathieu Desnoyers
  2018-07-05 18:05 ` [RFC PATCH for 4.18 2/5] rseq: uapi: update uapi comments Mathieu Desnoyers
@ 2018-07-05 18:05 ` Mathieu Desnoyers
  2018-07-06 16:02   ` Mathieu Desnoyers
  2018-07-05 18:06 ` [RFC PATCH for 4.18 4/5] rseq: remove unused types_32_64.h uapi header Mathieu Desnoyers
  2018-07-05 18:06 ` [RFC PATCH for 4.18 5/5] rseq/selftests: cleanup: update comment above rseq_prepare_unload Mathieu Desnoyers
  4 siblings, 1 reply; 14+ messages in thread
From: Mathieu Desnoyers @ 2018-07-05 18:05 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E . McKenney,
	Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
	Andrew Morton, Russell King, Ingo Molnar, H . Peter Anvin,
	Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
	Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
	Michael Kerrisk, Joel Fernandes, Mathieu Desnoyers

Declaring the rseq_cs field as a union between __u64 and two __u32
allows both 32-bit and 64-bit kernels to read the full __u64, and
therefore validate that a 32-bit user-space cleared the upper 32
bits, thus ensuring a consistent behavior between native 32-bit
kernels and 32-bit compat tasks on 64-bit kernels.

Check that the rseq_cs value read is < TASK_SIZE.

The asm/byteorder.h header needs to be included by rseq.h, now
that it is not using linux/types_32_64.h anymore.

Considering that only __32 and __u64 types are declared in linux/rseq.h,
the linux/types.h header should always be included for both kernel and
user-space code: including stdint.h is just for u64 and u32, which are
not used in this header at all.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Paul Turner <pjt@google.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Andi Kleen <andi@firstfloor.org>
CC: Dave Watson <davejwatson@fb.com>
CC: Chris Lameter <cl@linux.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ben Maurer <bmaurer@fb.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Russell King <linux@arm.linux.org.uk>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: linux-api@vger.kernel.org
---
 include/uapi/linux/rseq.h           | 27 +++++++++++++++++++--------
 kernel/rseq.c                       | 12 +++++++-----
 tools/testing/selftests/rseq/rseq.h | 11 ++++++++++-
 3 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index bf4188c13bec..9a402fdb60e9 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -10,13 +10,8 @@
  * Copyright (c) 2015-2018 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
  */
 
-#ifdef __KERNEL__
-# include <linux/types.h>
-#else
-# include <stdint.h>
-#endif
-
-#include <linux/types_32_64.h>
+#include <linux/types.h>
+#include <asm/byteorder.h>
 
 enum rseq_cpu_id_state {
 	RSEQ_CPU_ID_UNINITIALIZED		= -1,
@@ -111,7 +106,23 @@ struct rseq {
 	 * atomicity semantics. This field should only be updated by the
 	 * thread which registered this data structure. Aligned on 64-bit.
 	 */
-	LINUX_FIELD_u32_u64(rseq_cs);
+	union {
+		__u64 ptr64;
+#ifdef __LP64__
+		__u64 ptr;
+#else
+		struct {
+#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || defined(__BIG_ENDIAN)
+			__u32 padding;		/* Initialized to zero. */
+			__u32 ptr32;
+#else /* LITTLE */
+			__u32 ptr32;
+			__u32 padding;		/* Initialized to zero. */
+#endif /* ENDIAN */
+		} ptr;
+#endif
+	} rseq_cs;
+
 	/*
 	 * Restartable sequences flags field.
 	 *
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 16b38c5342f9..3081e6783cce 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -115,19 +115,21 @@ static int rseq_reset_rseq_cpu_id(struct task_struct *t)
 static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
 {
 	struct rseq_cs __user *urseq_cs;
-	unsigned long ptr;
+	u64 ptr;
 	u32 __user *usig;
 	u32 sig;
 	int ret;
 
-	ret = __get_user(ptr, &t->rseq->rseq_cs);
+	ret = __get_user(ptr, &t->rseq->rseq_cs.ptr64);
 	if (ret)
 		return ret;
 	if (!ptr) {
 		memset(rseq_cs, 0, sizeof(*rseq_cs));
 		return 0;
 	}
-	urseq_cs = (struct rseq_cs __user *)ptr;
+	if (ptr >= TASK_SIZE)
+		return -EINVAL;
+	urseq_cs = (struct rseq_cs __user *)(unsigned long)ptr;
 	if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
 		return -EFAULT;
 
@@ -201,9 +203,9 @@ static int clear_rseq_cs(struct task_struct *t)
 	 * of code outside of the rseq assembly block. This performs
 	 * a lazy clear of the rseq_cs field.
 	 *
-	 * Set rseq_cs to NULL with single-copy atomicity.
+	 * Set rseq_cs to NULL.
 	 */
-	return __put_user(0UL, &t->rseq->rseq_cs);
+	return __put_user(0ULL, &t->rseq->rseq_cs.ptr64);
 }
 
 /*
diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h
index a4684112676c..f2073cfa4448 100644
--- a/tools/testing/selftests/rseq/rseq.h
+++ b/tools/testing/selftests/rseq/rseq.h
@@ -133,6 +133,15 @@ static inline uint32_t rseq_current_cpu(void)
 	return cpu;
 }
 
+static inline void rseq_clear_rseq_cs(void)
+{
+#ifdef __LP64__
+	__rseq_abi.rseq_cs.ptr = 0;
+#else
+	__rseq_abi.rseq_cs.ptr.ptr32 = 0;
+#endif
+}
+
 /*
  * rseq_prepare_unload() should be invoked by each thread using rseq_finish*()
  * at least once between their last rseq_finish*() and library unload of the
@@ -143,7 +152,7 @@ static inline uint32_t rseq_current_cpu(void)
  */
 static inline void rseq_prepare_unload(void)
 {
-	__rseq_abi.rseq_cs = 0;
+	rseq_clear_rseq_cs();
 }
 
 #endif  /* RSEQ_H_ */
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH for 4.18 4/5] rseq: remove unused types_32_64.h uapi header
  2018-07-05 18:05 [RFC PATCH for 4.18 0/5] Restartable Sequences updates Mathieu Desnoyers
                   ` (2 preceding siblings ...)
  2018-07-05 18:05 ` [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes Mathieu Desnoyers
@ 2018-07-05 18:06 ` Mathieu Desnoyers
  2018-07-05 18:06 ` [RFC PATCH for 4.18 5/5] rseq/selftests: cleanup: update comment above rseq_prepare_unload Mathieu Desnoyers
  4 siblings, 0 replies; 14+ messages in thread
From: Mathieu Desnoyers @ 2018-07-05 18:06 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E . McKenney,
	Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
	Andrew Morton, Russell King, Ingo Molnar, H . Peter Anvin,
	Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
	Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
	Michael Kerrisk, Joel Fernandes, Mathieu Desnoyers

This header was introduced in the 4.18 merge window, and rseq does
not need it anymore. Nuke it before the final release.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Paul Turner <pjt@google.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Andi Kleen <andi@firstfloor.org>
CC: Dave Watson <davejwatson@fb.com>
CC: Chris Lameter <cl@linux.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ben Maurer <bmaurer@fb.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Russell King <linux@arm.linux.org.uk>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: linux-api@vger.kernel.org
---
 include/uapi/linux/types_32_64.h | 50 ----------------------------------------
 1 file changed, 50 deletions(-)
 delete mode 100644 include/uapi/linux/types_32_64.h

diff --git a/include/uapi/linux/types_32_64.h b/include/uapi/linux/types_32_64.h
deleted file mode 100644
index 0a87ace34a57..000000000000
--- a/include/uapi/linux/types_32_64.h
+++ /dev/null
@@ -1,50 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
-#ifndef _UAPI_LINUX_TYPES_32_64_H
-#define _UAPI_LINUX_TYPES_32_64_H
-
-/*
- * linux/types_32_64.h
- *
- * Integer type declaration for pointers across 32-bit and 64-bit systems.
- *
- * Copyright (c) 2015-2018 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
- */
-
-#ifdef __KERNEL__
-# include <linux/types.h>
-#else
-# include <stdint.h>
-#endif
-
-#include <asm/byteorder.h>
-
-#ifdef __BYTE_ORDER
-# if (__BYTE_ORDER == __BIG_ENDIAN)
-#  define LINUX_BYTE_ORDER_BIG_ENDIAN
-# else
-#  define LINUX_BYTE_ORDER_LITTLE_ENDIAN
-# endif
-#else
-# ifdef __BIG_ENDIAN
-#  define LINUX_BYTE_ORDER_BIG_ENDIAN
-# else
-#  define LINUX_BYTE_ORDER_LITTLE_ENDIAN
-# endif
-#endif
-
-#ifdef __LP64__
-# define LINUX_FIELD_u32_u64(field)			__u64 field
-# define LINUX_FIELD_u32_u64_INIT_ONSTACK(field, v)	field = (intptr_t)v
-#else
-# ifdef LINUX_BYTE_ORDER_BIG_ENDIAN
-#  define LINUX_FIELD_u32_u64(field)	__u32 field ## _padding, field
-#  define LINUX_FIELD_u32_u64_INIT_ONSTACK(field, v)	\
-	field ## _padding = 0, field = (intptr_t)v
-# else
-#  define LINUX_FIELD_u32_u64(field)	__u32 field, field ## _padding
-#  define LINUX_FIELD_u32_u64_INIT_ONSTACK(field, v)	\
-	field = (intptr_t)v, field ## _padding = 0
-# endif
-#endif
-
-#endif /* _UAPI_LINUX_TYPES_32_64_H */
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH for 4.18 5/5] rseq/selftests: cleanup: update comment above rseq_prepare_unload
  2018-07-05 18:05 [RFC PATCH for 4.18 0/5] Restartable Sequences updates Mathieu Desnoyers
                   ` (3 preceding siblings ...)
  2018-07-05 18:06 ` [RFC PATCH for 4.18 4/5] rseq: remove unused types_32_64.h uapi header Mathieu Desnoyers
@ 2018-07-05 18:06 ` Mathieu Desnoyers
  4 siblings, 0 replies; 14+ messages in thread
From: Mathieu Desnoyers @ 2018-07-05 18:06 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E . McKenney,
	Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
	Andrew Morton, Russell King, Ingo Molnar, H . Peter Anvin,
	Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
	Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
	Michael Kerrisk, Joel Fernandes, Mathieu Desnoyers

rseq as it was merged does not have rseq_finish_*() in the user-space
selftests anymore. Update the rseq_prepare_unload() helper comment to
adapt to this reality.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Paul Turner <pjt@google.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Andi Kleen <andi@firstfloor.org>
CC: Dave Watson <davejwatson@fb.com>
CC: Chris Lameter <cl@linux.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ben Maurer <bmaurer@fb.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Russell King <linux@arm.linux.org.uk>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: linux-api@vger.kernel.org
---
 tools/testing/selftests/rseq/rseq.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h
index f2073cfa4448..86ce22417e0d 100644
--- a/tools/testing/selftests/rseq/rseq.h
+++ b/tools/testing/selftests/rseq/rseq.h
@@ -143,12 +143,13 @@ static inline void rseq_clear_rseq_cs(void)
 }
 
 /*
- * rseq_prepare_unload() should be invoked by each thread using rseq_finish*()
- * at least once between their last rseq_finish*() and library unload of the
- * library defining the rseq critical section (struct rseq_cs). This also
- * applies to use of rseq in code generated by JIT: rseq_prepare_unload()
- * should be invoked at least once by each thread using rseq_finish*() before
- * reclaim of the memory holding the struct rseq_cs.
+ * rseq_prepare_unload() should be invoked by each thread executing a rseq
+ * critical section at least once between their last critical section and
+ * library unload of the library defining the rseq critical section
+ * (struct rseq_cs). This also applies to use of rseq in code generated by
+ * JIT: rseq_prepare_unload() should be invoked at least once by each
+ * thread executing a rseq critical section before reclaim of the memory
+ * holding the struct rseq_cs.
  */
 static inline void rseq_prepare_unload(void)
 {
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes
  2018-07-05 18:05 ` [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes Mathieu Desnoyers
@ 2018-07-06 16:02   ` Mathieu Desnoyers
  2018-07-06 19:23     ` Mathieu Desnoyers
  0 siblings, 1 reply; 14+ messages in thread
From: Mathieu Desnoyers @ 2018-07-06 16:02 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E. McKenney,
	Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
	Andrew Morton, Russell King, Ingo Molnar, H. Peter Anvin,
	Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
	Linus Torvalds, Catalin Marinas, Will Deacon, Michael Kerrisk,
	Joel Fernandes

----- On Jul 5, 2018, at 2:05 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> Declaring the rseq_cs field as a union between __u64 and two __u32
> allows both 32-bit and 64-bit kernels to read the full __u64, and
> therefore validate that a 32-bit user-space cleared the upper 32
> bits, thus ensuring a consistent behavior between native 32-bit
> kernels and 32-bit compat tasks on 64-bit kernels.
> 
> Check that the rseq_cs value read is < TASK_SIZE.
> 
> The asm/byteorder.h header needs to be included by rseq.h, now
> that it is not using linux/types_32_64.h anymore.
> 
> Considering that only __32 and __u64 types are declared in linux/rseq.h,
> the linux/types.h header should always be included for both kernel and
> user-space code: including stdint.h is just for u64 and u32, which are
> not used in this header at all.

The 0-day bot noticed that __get_user() is unimplemented for 64-bit
values on arm32 (although get_user() is implemented).

The following diff fixes this discrepancy, and allows this rseq patch
to build on arm32:

commit dde99f3310c76acb0a160c0572f40b6aa279594c
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Fri Jul 6 11:29:39 2018 -0400

    arm: implement 64-bit __get_user
    
    get_user() is implemented on arm32 for 64-bit user-space values, but
    not its __get_user() counterpart.
    
    Implement __get_user() as two __get_user_asm_word().
    
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
    CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
    CC: Peter Zijlstra <peterz@infradead.org>
    CC: Paul Turner <pjt@google.com>
    CC: Thomas Gleixner <tglx@linutronix.de>
    CC: Andy Lutomirski <luto@amacapital.net>
    CC: Andi Kleen <andi@firstfloor.org>
    CC: Dave Watson <davejwatson@fb.com>
    CC: Chris Lameter <cl@linux.com>
    CC: Ingo Molnar <mingo@redhat.com>
    CC: "H. Peter Anvin" <hpa@zytor.com>
    CC: Ben Maurer <bmaurer@fb.com>
    CC: Steven Rostedt <rostedt@goodmis.org>
    CC: Josh Triplett <josh@joshtriplett.org>
    CC: Linus Torvalds <torvalds@linux-foundation.org>
    CC: Andrew Morton <akpm@linux-foundation.org>
    CC: Russell King <linux@arm.linux.org.uk>
    CC: Catalin Marinas <catalin.marinas@arm.com>
    CC: Will Deacon <will.deacon@arm.com>
    CC: Michael Kerrisk <mtk.manpages@gmail.com>
    CC: Boqun Feng <boqun.feng@gmail.com>
    CC: linux-api@vger.kernel.org

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 3d614e9..38659c6 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -265,10 +265,16 @@ static inline void set_fs(mm_segment_t fs)
        (void) 0;                                                       \
 })
 
+union __gu_u64 {
+       u64 val64;
+       u32 word[2];
+};
+
 #define __get_user_err(x, ptr, err)                                    \
 do {                                                                   \
        unsigned long __gu_addr = (unsigned long)(ptr);                 \
        unsigned long __gu_val;                                         \
+       union __gu_u64 __gu_tmp64;                                      \
        unsigned int __ua_flags;                                        \
        __chk_user_ptr(ptr);                                            \
        might_fault();                                                  \
@@ -277,10 +283,28 @@ static inline void set_fs(mm_segment_t fs)
        case 1: __get_user_asm_byte(__gu_val, __gu_addr, err);  break;  \
        case 2: __get_user_asm_half(__gu_val, __gu_addr, err);  break;  \
        case 4: __get_user_asm_word(__gu_val, __gu_addr, err);  break;  \
+       case 8:                                                         \
+       {                                                               \
+               union __gu_u64 __user *__gu_addr64 =                    \
+                       (union __gu_u64 __user *)__gu_addr;             \
+               __get_user_asm_word(__gu_tmp64.word[0],                 \
+                        &__gu_addr64->word[0], err);                   \
+               if (err)                                                \
+                       break;                                          \
+               __get_user_asm_word(__gu_tmp64.word[1],                 \
+                        &__gu_addr64->word[1], err);                   \
+               break;                                                  \
+       };                                                              \
        default: (__gu_val) = __get_user_bad();                         \
        }                                                               \
        uaccess_restore(__ua_flags);                                    \
-       (x) = (__typeof__(*(ptr)))__gu_val;                             \
+       switch (sizeof(*(ptr))) {                                       \
+       case 1:                                                         \
+       case 2:                                                         \
+       case 4:                                                         \
+       default: (x) = (__typeof__(*(ptr)))__gu_val; break;             \
+       case 8: (x) = (__typeof__(*(ptr)))__gu_tmp64.val64; break;      \
+       }                                                               \
 } while (0)
 
 #define __get_user_asm(x, addr, err, instr)                    \


> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Paul Turner <pjt@google.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Andi Kleen <andi@firstfloor.org>
> CC: Dave Watson <davejwatson@fb.com>
> CC: Chris Lameter <cl@linux.com>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Ben Maurer <bmaurer@fb.com>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Josh Triplett <josh@joshtriplett.org>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Russell King <linux@arm.linux.org.uk>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Will Deacon <will.deacon@arm.com>
> CC: Michael Kerrisk <mtk.manpages@gmail.com>
> CC: Boqun Feng <boqun.feng@gmail.com>
> CC: linux-api@vger.kernel.org
> ---
> include/uapi/linux/rseq.h           | 27 +++++++++++++++++++--------
> kernel/rseq.c                       | 12 +++++++-----
> tools/testing/selftests/rseq/rseq.h | 11 ++++++++++-
> 3 files changed, 36 insertions(+), 14 deletions(-)
> 
> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
> index bf4188c13bec..9a402fdb60e9 100644
> --- a/include/uapi/linux/rseq.h
> +++ b/include/uapi/linux/rseq.h
> @@ -10,13 +10,8 @@
>  * Copyright (c) 2015-2018 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>  */
> 
> -#ifdef __KERNEL__
> -# include <linux/types.h>
> -#else
> -# include <stdint.h>
> -#endif
> -
> -#include <linux/types_32_64.h>
> +#include <linux/types.h>
> +#include <asm/byteorder.h>
> 
> enum rseq_cpu_id_state {
> 	RSEQ_CPU_ID_UNINITIALIZED		= -1,
> @@ -111,7 +106,23 @@ struct rseq {
> 	 * atomicity semantics. This field should only be updated by the
> 	 * thread which registered this data structure. Aligned on 64-bit.
> 	 */
> -	LINUX_FIELD_u32_u64(rseq_cs);
> +	union {
> +		__u64 ptr64;
> +#ifdef __LP64__
> +		__u64 ptr;
> +#else
> +		struct {
> +#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) ||
> defined(__BIG_ENDIAN)
> +			__u32 padding;		/* Initialized to zero. */
> +			__u32 ptr32;
> +#else /* LITTLE */
> +			__u32 ptr32;
> +			__u32 padding;		/* Initialized to zero. */
> +#endif /* ENDIAN */
> +		} ptr;
> +#endif
> +	} rseq_cs;
> +
> 	/*
> 	 * Restartable sequences flags field.
> 	 *
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index 16b38c5342f9..3081e6783cce 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -115,19 +115,21 @@ static int rseq_reset_rseq_cpu_id(struct task_struct *t)
> static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
> {
> 	struct rseq_cs __user *urseq_cs;
> -	unsigned long ptr;
> +	u64 ptr;
> 	u32 __user *usig;
> 	u32 sig;
> 	int ret;
> 
> -	ret = __get_user(ptr, &t->rseq->rseq_cs);
> +	ret = __get_user(ptr, &t->rseq->rseq_cs.ptr64);
> 	if (ret)
> 		return ret;
> 	if (!ptr) {
> 		memset(rseq_cs, 0, sizeof(*rseq_cs));
> 		return 0;
> 	}
> -	urseq_cs = (struct rseq_cs __user *)ptr;
> +	if (ptr >= TASK_SIZE)
> +		return -EINVAL;
> +	urseq_cs = (struct rseq_cs __user *)(unsigned long)ptr;
> 	if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
> 		return -EFAULT;
> 
> @@ -201,9 +203,9 @@ static int clear_rseq_cs(struct task_struct *t)
> 	 * of code outside of the rseq assembly block. This performs
> 	 * a lazy clear of the rseq_cs field.
> 	 *
> -	 * Set rseq_cs to NULL with single-copy atomicity.
> +	 * Set rseq_cs to NULL.
> 	 */
> -	return __put_user(0UL, &t->rseq->rseq_cs);
> +	return __put_user(0ULL, &t->rseq->rseq_cs.ptr64);
> }
> 
> /*
> diff --git a/tools/testing/selftests/rseq/rseq.h
> b/tools/testing/selftests/rseq/rseq.h
> index a4684112676c..f2073cfa4448 100644
> --- a/tools/testing/selftests/rseq/rseq.h
> +++ b/tools/testing/selftests/rseq/rseq.h
> @@ -133,6 +133,15 @@ static inline uint32_t rseq_current_cpu(void)
> 	return cpu;
> }
> 
> +static inline void rseq_clear_rseq_cs(void)
> +{
> +#ifdef __LP64__
> +	__rseq_abi.rseq_cs.ptr = 0;
> +#else
> +	__rseq_abi.rseq_cs.ptr.ptr32 = 0;
> +#endif
> +}
> +
> /*
>  * rseq_prepare_unload() should be invoked by each thread using rseq_finish*()
>  * at least once between their last rseq_finish*() and library unload of the
> @@ -143,7 +152,7 @@ static inline uint32_t rseq_current_cpu(void)
>  */
> static inline void rseq_prepare_unload(void)
> {
> -	__rseq_abi.rseq_cs = 0;
> +	rseq_clear_rseq_cs();
> }
> 
> #endif  /* RSEQ_H_ */
> --
> 2.11.0

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes
  2018-07-06 16:02   ` Mathieu Desnoyers
@ 2018-07-06 19:23     ` Mathieu Desnoyers
  2018-07-06 19:31       ` Linus Torvalds
  0 siblings, 1 reply; 14+ messages in thread
From: Mathieu Desnoyers @ 2018-07-06 19:23 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E. McKenney,
	Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
	Andrew Morton, Russell King, Ingo Molnar, H. Peter Anvin,
	Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
	Linus Torvalds, Catalin Marinas, Will Deacon, Michael Kerrisk,
	Joel Fernandes

----- On Jul 6, 2018, at 12:02 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- On Jul 5, 2018, at 2:05 PM, Mathieu Desnoyers
> mathieu.desnoyers@efficios.com wrote:
> 
[...]
> The 0-day bot noticed that __get_user() is unimplemented for 64-bit
> values on arm32 (although get_user() is implemented).
> 
> The following diff fixes this discrepancy, and allows this rseq patch
> to build on arm32:
> 

For -rc, I would favor the following simpler approach. Or I could even
just use get_user() instead. Thoughts ?

    rseq: implement work-around for missing 8-byte __get_user on arm
    
    Now that rseq uses __u64 for its pointer fields, 32-bit architectures
    need to read this 64-bit value from user-space.
    
    __get_user is used to read this value, given that its access check has
    already been performed with access_ok() on rseq registration.
    
    arm does not implement 8-byte __get_user. Work-around this limitation
    by using get_user() on ARM instead, with its redundant access check.
    
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
    CC: Thomas Gleixner <tglx@linutronix.de>
    Cc: Joel Fernandes <joelaf@google.com>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Catalin Marinas <catalin.marinas@arm.com>
    Cc: Dave Watson <davejwatson@fb.com>
    Cc: Will Deacon <will.deacon@arm.com>
    Cc: Andi Kleen <andi@firstfloor.org>
    Cc: "H . Peter Anvin" <hpa@zytor.com>
    Cc: Chris Lameter <cl@linux.com>
    Cc: Russell King <linux@arm.linux.org.uk>
    Cc: Andrew Hunter <ahh@google.com>
    Cc: Michael Kerrisk <mtk.manpages@gmail.com>
    Cc: "Paul E . McKenney" <paulmck@linux.vnet.ibm.com>
    Cc: Paul Turner <pjt@google.com>
    Cc: Boqun Feng <boqun.feng@gmail.com>
    Cc: Josh Triplett <josh@joshtriplett.org>
    Cc: Steven Rostedt <rostedt@goodmis.org>
    Cc: Ben Maurer <bmaurer@fb.com>
    Cc: linux-api@vger.kernel.org
    CC: linux-arm-kernel@lists.infradead.org
    Cc: Andy Lutomirski <luto@amacapital.net>
    Cc: Andrew Morton <akpm@linux-foundation.org>
    Cc: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 3081e67..0e67625 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -18,6 +18,16 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/rseq.h>
 
+/*
+ * ARM does not implement 8 bytes __get_user. Use get_user on that
+ * architecture instead.
+ */
+#ifdef CONFIG_ARM
+#define __rseq_get_user                get_user
+#else
+#define __rseq_get_user                __get_user
+#endif
+
 #define RSEQ_CS_PREEMPT_MIGRATE_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE | \
                                       RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)
 
@@ -120,7 +130,7 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rs
        u32 sig;
        int ret;
 
-       ret = __get_user(ptr, &t->rseq->rseq_cs.ptr64);
+       ret = __rseq_get_user(ptr, &t->rseq->rseq_cs.ptr64);
        if (ret)
                return ret;
        if (!ptr) {



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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes
  2018-07-06 19:23     ` Mathieu Desnoyers
@ 2018-07-06 19:31       ` Linus Torvalds
  2018-07-06 19:35         ` Mathieu Desnoyers
  2018-07-06 19:56         ` Andy Lutomirski
  0 siblings, 2 replies; 14+ messages in thread
From: Linus Torvalds @ 2018-07-06 19:31 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Thomas Gleixner, Linux Kernel Mailing List, Linux API,
	Peter Zijlstra, Paul McKenney, Boqun Feng, Andy Lutomirski,
	Dave Watson, Paul Turner, Andrew Morton,
	Russell King - ARM Linux, Ingo Molnar, Peter Anvin, Andi Kleen,
	Christoph Lameter, Ben Maurer, Steven Rostedt, Josh Triplett,
	Catalin Marinas, Will Deacon, Michael Kerrisk, Joel Fernandes

On Fri, Jul 6, 2018 at 12:23 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> For -rc, I would favor the following simpler approach. Or I could even
> just use get_user() instead. Thoughts ?

Please just use "get_user()".

In fact, we should be thinking seriosly about just removing
__get_user() entirely. It's wrong. It optimizes the wrong thing
entirely. It _used_ to be that the range check was noticeable, and it
really isn't any more. These days the expensive parts are the SMAP
costs, and both get_user() and __get_user() have those, except
get_user() is safer and doesn't waste I$ on inlining the code to
disable and re-enable SMAP.

                Linus

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes
  2018-07-06 19:31       ` Linus Torvalds
@ 2018-07-06 19:35         ` Mathieu Desnoyers
  2018-07-06 19:38           ` Mathieu Desnoyers
  2018-07-06 19:56         ` Andy Lutomirski
  1 sibling, 1 reply; 14+ messages in thread
From: Mathieu Desnoyers @ 2018-07-06 19:35 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Thomas Gleixner, linux-kernel, linux-api, Peter Zijlstra,
	Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
	Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
	H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
	Josh Triplett, Catalin Marinas, Will Deacon, Michael Kerrisk,
	Joel Fernandes

----- On Jul 6, 2018, at 3:31 PM, Linus Torvalds torvalds@linux-foundation.org wrote:

> On Fri, Jul 6, 2018 at 12:23 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> For -rc, I would favor the following simpler approach. Or I could even
>> just use get_user() instead. Thoughts ?
> 
> Please just use "get_user()".
> 
> In fact, we should be thinking seriosly about just removing
> __get_user() entirely. It's wrong. It optimizes the wrong thing
> entirely. It _used_ to be that the range check was noticeable, and it
> really isn't any more. These days the expensive parts are the SMAP
> costs, and both get_user() and __get_user() have those, except
> get_user() is safer and doesn't waste I$ on inlining the code to
> disable and re-enable SMAP.

Will do, thanks!

Mathieu


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes
  2018-07-06 19:35         ` Mathieu Desnoyers
@ 2018-07-06 19:38           ` Mathieu Desnoyers
  2018-07-06 19:56             ` Linus Torvalds
  0 siblings, 1 reply; 14+ messages in thread
From: Mathieu Desnoyers @ 2018-07-06 19:38 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Thomas Gleixner, linux-kernel, linux-api, Peter Zijlstra,
	Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
	Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
	H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
	Josh Triplett, Catalin Marinas, Will Deacon, Michael Kerrisk,
	Joel Fernandes

----- On Jul 6, 2018, at 3:35 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- On Jul 6, 2018, at 3:31 PM, Linus Torvalds torvalds@linux-foundation.org
> wrote:
> 
>> On Fri, Jul 6, 2018 at 12:23 PM Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>>>
>>> For -rc, I would favor the following simpler approach. Or I could even
>>> just use get_user() instead. Thoughts ?
>> 
>> Please just use "get_user()".
>> 
>> In fact, we should be thinking seriosly about just removing
>> __get_user() entirely. It's wrong. It optimizes the wrong thing
>> entirely. It _used_ to be that the range check was noticeable, and it
>> really isn't any more. These days the expensive parts are the SMAP
>> costs, and both get_user() and __get_user() have those, except
>> get_user() is safer and doesn't waste I$ on inlining the code to
>> disable and re-enable SMAP.
> 
> Will do, thanks!

Should I change all 4 bytes __get_user()/__put_user() in kernel/rseq.c
for get_user()/put_user() to ensure consistency ?

Thanks,

Mathieu

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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes
  2018-07-06 19:31       ` Linus Torvalds
  2018-07-06 19:35         ` Mathieu Desnoyers
@ 2018-07-06 19:56         ` Andy Lutomirski
  1 sibling, 0 replies; 14+ messages in thread
From: Andy Lutomirski @ 2018-07-06 19:56 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Mathieu Desnoyers, Thomas Gleixner, Linux Kernel Mailing List,
	Linux API, Peter Zijlstra, Paul McKenney, Boqun Feng,
	Dave Watson, Paul Turner, Andrew Morton,
	Russell King - ARM Linux, Ingo Molnar, Peter Anvin, Andi Kleen,
	Christoph Lameter, Ben Maurer, Steven Rostedt, Josh Triplett,
	Catalin Marinas, Will Deacon, Michael Kerrisk, Joel Fernandes

On Fri, Jul 6, 2018 at 12:31 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jul 6, 2018 at 12:23 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> For -rc, I would favor the following simpler approach. Or I could even
>> just use get_user() instead. Thoughts ?
>
> Please just use "get_user()".
>
> In fact, we should be thinking seriosly about just removing
> __get_user() entirely. It's wrong. It optimizes the wrong thing
> entirely. It _used_ to be that the range check was noticeable, and it
> really isn't any more. These days the expensive parts are the SMAP
> costs, and both get_user() and __get_user() have those, except
> get_user() is safer and doesn't waste I$ on inlining the code to
> disable and re-enable SMAP.

If Al and Christoph ever manage to get rid of set_fs(), I bet we can
rewrite access_ok() and get_user() so that gcc can fold redundant
checks together and generate optimal code for get_user() of
consecutive struct fields all by itself.  Or maybe I'm giving gcc more
credit than it deserves.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes
  2018-07-06 19:38           ` Mathieu Desnoyers
@ 2018-07-06 19:56             ` Linus Torvalds
  2018-07-07 15:06               ` Russell King - ARM Linux
  0 siblings, 1 reply; 14+ messages in thread
From: Linus Torvalds @ 2018-07-06 19:56 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Thomas Gleixner, Linux Kernel Mailing List, Linux API,
	Peter Zijlstra, Paul McKenney, Boqun Feng, Andy Lutomirski,
	Dave Watson, Paul Turner, Andrew Morton,
	Russell King - ARM Linux, Ingo Molnar, Peter Anvin, Andi Kleen,
	Christoph Lameter, Ben Maurer, Steven Rostedt, Josh Triplett,
	Catalin Marinas, Will Deacon, Michael Kerrisk, Joel Fernandes

On Fri, Jul 6, 2018 at 12:38 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> Should I change all 4 bytes __get_user()/__put_user() in kernel/rseq.c
> for get_user()/put_user() to ensure consistency ?

Probably.

*If* this actually turns out to be somethinig that shows up on
profiles, it's almost certainly going to be the STAC/CLAC instructions
("perf report" tends to report them as three one-byte nop's because
that's how they look before instruction replacement).

And then it's not __get/put_user() that will improve things, but doing a

        user_access_begin();

        .. do unsafe_get/put_user() ..

        user_access_end();

that will improve performance.

But it is *very* seldom useful. We have it in a handful of places in
the kernel, and the most noticeable one is
lib/{strnlen,strncpy_from}_user.c

                       Linus

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes
  2018-07-06 19:56             ` Linus Torvalds
@ 2018-07-07 15:06               ` Russell King - ARM Linux
  0 siblings, 0 replies; 14+ messages in thread
From: Russell King - ARM Linux @ 2018-07-07 15:06 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Mathieu Desnoyers, Thomas Gleixner, Linux Kernel Mailing List,
	Linux API, Peter Zijlstra, Paul McKenney, Boqun Feng,
	Andy Lutomirski, Dave Watson, Paul Turner, Andrew Morton,
	Ingo Molnar, Peter Anvin, Andi Kleen, Christoph Lameter,
	Ben Maurer, Steven Rostedt, Josh Triplett, Catalin Marinas,
	Will Deacon, Michael Kerrisk, Joel Fernandes

On Fri, Jul 06, 2018 at 12:56:58PM -0700, Linus Torvalds wrote:
> On Fri, Jul 6, 2018 at 12:38 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
> >
> > Should I change all 4 bytes __get_user()/__put_user() in kernel/rseq.c
> > for get_user()/put_user() to ensure consistency ?
> 
> Probably.
> 
> *If* this actually turns out to be somethinig that shows up on
> profiles, it's almost certainly going to be the STAC/CLAC instructions
> ("perf report" tends to report them as three one-byte nop's because
> that's how they look before instruction replacement).
> 
> And then it's not __get/put_user() that will improve things, but doing a
> 
>         user_access_begin();
> 
>         .. do unsafe_get/put_user() ..
> 
>         user_access_end();
> 
> that will improve performance.
> 
> But it is *very* seldom useful. We have it in a handful of places in
> the kernel, and the most noticeable one is
> lib/{strnlen,strncpy_from}_user.c

Also, __get_user() is probably going to become the same as get_user()
when I finish the Spectre v1 ARM mitigations, because there'll be no
point in __get_user() being any different.  For those mitigations,
we're going to have to check the pointer against the address limit
inside __get_user() and NULL it out, just like get_user() does, which
makes the whole distinction between the two completely pointless.

Is this not also the case on other architectures affected by Spectre
variant 1, hmm?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2018-07-07 15:07 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-05 18:05 [RFC PATCH for 4.18 0/5] Restartable Sequences updates Mathieu Desnoyers
2018-07-05 18:05 ` [RFC PATCH for 4.18 1/5] rseq: use __u64 for rseq_cs fields, validate user inputs Mathieu Desnoyers
2018-07-05 18:05 ` [RFC PATCH for 4.18 2/5] rseq: uapi: update uapi comments Mathieu Desnoyers
2018-07-05 18:05 ` [RFC PATCH for 4.18 3/5] rseq: uapi: declare rseq_cs field as union, update includes Mathieu Desnoyers
2018-07-06 16:02   ` Mathieu Desnoyers
2018-07-06 19:23     ` Mathieu Desnoyers
2018-07-06 19:31       ` Linus Torvalds
2018-07-06 19:35         ` Mathieu Desnoyers
2018-07-06 19:38           ` Mathieu Desnoyers
2018-07-06 19:56             ` Linus Torvalds
2018-07-07 15:06               ` Russell King - ARM Linux
2018-07-06 19:56         ` Andy Lutomirski
2018-07-05 18:06 ` [RFC PATCH for 4.18 4/5] rseq: remove unused types_32_64.h uapi header Mathieu Desnoyers
2018-07-05 18:06 ` [RFC PATCH for 4.18 5/5] rseq/selftests: cleanup: update comment above rseq_prepare_unload Mathieu Desnoyers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).