remove set_fs for riscv

remove set_fs for riscv

[Christoph Hellwig]

Hi all,

this series converts riscv to the new set_fs less world and is on top of this
branch:

    https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=base.set_fs

The first four patches are general improvements and enablement for all nommu
ports, and might make sense to merge through the above base branch.

Diffstat
 arch/riscv/Kconfig                   |    2 
 arch/riscv/include/asm/thread_info.h |    6 -
 arch/riscv/include/asm/uaccess.h     |  177 +++++++++++++++++------------------
 arch/riscv/kernel/process.c          |    1 
 arch/riscv/lib/Makefile              |    2 
 include/asm-generic/uaccess.h        |   42 +++++---
 include/linux/uaccess.h              |    4 
 7 files changed, 127 insertions(+), 107 deletions(-)

[PATCH 1/8] maccess: add a generic __{get,put}_kernel_nofault for nommu

[Christoph Hellwig]

Nommu architectures do not have page tables and thus no page faults.
Implement the maccess helpers using get_unaligned and put_unaligned and
enable them unconditionally for !CONFIG_MMU.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/asm-generic/uaccess.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index ba68ee4dabfaa7..cc3b2c8b68fab4 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -9,6 +9,26 @@
  */
 #include <linux/string.h>
 
+#ifndef CONFIG_MMU
+#include <asm/unaligned.h>
+
+#define __get_kernel_nofault(dst, src, type, err_label)			\
+do {									\
+	*((type *)dst) = get_unaligned((type *)(src));			\
+	if (0) /* make sure the label looks used to the compiler */	\
+		goto err_label;						\
+} while (0)
+
+#define __put_kernel_nofault(dst, src, type, err_label)			\
+do {									\
+	put_unaligned(*((type *)src), (type *)(dst));			\
+	if (0) /* make sure the label looks used to the compiler */	\
+		goto err_label;						\
+} while (0)
+
+#define HAVE_GET_KERNEL_NOFAULT 1
+#endif /* !CONFIG_MMU */
+
 #ifdef CONFIG_UACCESS_MEMCPY
 static inline __must_check unsigned long
 raw_copy_from_user(void *to, const void __user * from, unsigned long n)
-- 
2.28.0

[PATCH 2/8] uaccess: provide a generic TASK_SIZE_MAX definition

[Christoph Hellwig]

Define TASK_SIZE_MAX as TASK_SIZE if not otherwise defined.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/uaccess.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 70073c802b48ed..d0e43761c708d8 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -31,6 +31,10 @@ typedef struct {
 	/* empty dummy */
 } mm_segment_t;
 
+#ifndef TASK_SIZE_MAX
+#define TASK_SIZE_MAX			TASK_SIZE
+#endif
+
 #define uaccess_kernel()		(false)
 #define user_addr_max()			(TASK_SIZE_MAX)
 
-- 
2.28.0

[PATCH 3/8] asm-generic: fix unaligned access hamdling in raw_copy_{from,to}_user

[Christoph Hellwig]

Use get_unaligned and put_unaligned for the small constant size cases
in the generic uaccess routines.  This ensures they can be used for
architectures that do not support unaligned loads and stores, while
being a no-op for those that do.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/asm-generic/uaccess.h | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index cc3b2c8b68fab4..768502bbfb154e 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -36,19 +36,17 @@ raw_copy_from_user(void *to, const void __user * from, unsigned long n)
 	if (__builtin_constant_p(n)) {
 		switch(n) {
 		case 1:
-			*(u8 *)to = *(u8 __force *)from;
+			*(u8 *)to = get_unaligned((u8 __force *)from);
 			return 0;
 		case 2:
-			*(u16 *)to = *(u16 __force *)from;
+			*(u16 *)to = get_unaligned((u16 __force *)from);
 			return 0;
 		case 4:
-			*(u32 *)to = *(u32 __force *)from;
+			*(u32 *)to = get_unaligned((u32 __force *)from);
 			return 0;
-#ifdef CONFIG_64BIT
 		case 8:
-			*(u64 *)to = *(u64 __force *)from;
+			*(u64 *)to = get_unaligned((u64 __force *)from);
 			return 0;
-#endif
 		}
 	}
 
@@ -62,19 +60,17 @@ raw_copy_to_user(void __user *to, const void *from, unsigned long n)
 	if (__builtin_constant_p(n)) {
 		switch(n) {
 		case 1:
-			*(u8 __force *)to = *(u8 *)from;
+			put_unaligned(*(u8 *)from, (u8 __force *)to);
 			return 0;
 		case 2:
-			*(u16 __force *)to = *(u16 *)from;
+			put_unaligned(*(u16 *)from, (u16 __force *)to);
 			return 0;
 		case 4:
-			*(u32 __force *)to = *(u32 *)from;
+			put_unaligned(*(u32 *)from, (u32 __force *)to);
 			return 0;
-#ifdef CONFIG_64BIT
 		case 8:
-			*(u64 __force *)to = *(u64 *)from;
+			put_unaligned(*(u64 *)from, (u64 __force *)to);
 			return 0;
-#endif
 		default:
 			break;
 		}
-- 
2.28.0

[PATCH 4/8] asm-generic: prepare uaccess.h for a set_fs-less world

[Christoph Hellwig]

Put all the set_fs related code under CONFIG_SET_FS so that
asm-generic/uaccess.h can be used for set_fs-less builds.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/asm-generic/uaccess.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index 768502bbfb154e..a9e6da7cce25e2 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -83,6 +83,7 @@ raw_copy_to_user(void __user *to, const void *from, unsigned long n)
 #define INLINE_COPY_TO_USER
 #endif /* CONFIG_UACCESS_MEMCPY */
 
+#ifdef CONFIG_SET_FS
 #define MAKE_MM_SEG(s)	((mm_segment_t) { (s) })
 
 #ifndef KERNEL_DS
@@ -105,6 +106,7 @@ static inline void set_fs(mm_segment_t fs)
 #ifndef uaccess_kernel
 #define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
 #endif
+#endif /* CONFIG_SET_FS */
 
 #define access_ok(addr, size) __access_ok((unsigned long)(addr),(size))
 
-- 
2.28.0

[PATCH 5/8] riscv: use memcpy based uaccess for nommu again

[Christoph Hellwig]

This reverts commit adccfb1a805ea84d2db38eb53032533279bdaa97.

Now that the generic uaccess by mempcy code handles unaligned addresses
the generic code can be used for all RISC-V CPUs.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/Kconfig               |  1 +
 arch/riscv/include/asm/uaccess.h | 36 ++++++++++++++++----------------
 arch/riscv/lib/Makefile          |  2 +-
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 07d53044013ede..460e3971a80fde 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -87,6 +87,7 @@ config RISCV
 	select SYSCTL_EXCEPTION_TRACE
 	select THREAD_INFO_IN_TASK
 	select SET_FS
+	select UACCESS_MEMCPY if !MMU
 
 config ARCH_MMAP_RND_BITS_MIN
 	default 18 if 64BIT
diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index f56c66b3f5fe21..e8eedf22e90747 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -13,24 +13,6 @@
 /*
  * User space memory access functions
  */
-
-extern unsigned long __must_check __asm_copy_to_user(void __user *to,
-	const void *from, unsigned long n);
-extern unsigned long __must_check __asm_copy_from_user(void *to,
-	const void __user *from, unsigned long n);
-
-static inline unsigned long
-raw_copy_from_user(void *to, const void __user *from, unsigned long n)
-{
-	return __asm_copy_from_user(to, from, n);
-}
-
-static inline unsigned long
-raw_copy_to_user(void __user *to, const void *from, unsigned long n)
-{
-	return __asm_copy_to_user(to, from, n);
-}
-
 #ifdef CONFIG_MMU
 #include <linux/errno.h>
 #include <linux/compiler.h>
@@ -385,6 +367,24 @@ do {								\
 		-EFAULT;					\
 })
 
+
+unsigned long __must_check __asm_copy_to_user(void __user *to,
+	const void *from, unsigned long n);
+unsigned long __must_check __asm_copy_from_user(void *to,
+	const void __user *from, unsigned long n);
+
+static inline unsigned long
+raw_copy_from_user(void *to, const void __user *from, unsigned long n)
+{
+	return __asm_copy_from_user(to, from, n);
+}
+
+static inline unsigned long
+raw_copy_to_user(void __user *to, const void *from, unsigned long n)
+{
+	return __asm_copy_to_user(to, from, n);
+}
+
 extern long strncpy_from_user(char *dest, const char __user *src, long count);
 
 extern long __must_check strlen_user(const char __user *str);
diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
index 0d0db80800c4ed..47e7a82044608d 100644
--- a/arch/riscv/lib/Makefile
+++ b/arch/riscv/lib/Makefile
@@ -2,5 +2,5 @@
 lib-y			+= delay.o
 lib-y			+= memcpy.o
 lib-y			+= memset.o
-lib-y			+= uaccess.o
+lib-$(CONFIG_MMU)	+= uaccess.o
 lib-$(CONFIG_64BIT)	+= tishift.o
-- 
2.28.0

[PATCH 6/8] riscv: refactor __get_user and __put_user

[Christoph Hellwig]

Add new __get_user_nocheck and __put_user_nocheck that switch on the size
and call the actual inline assembly helpers, and move the uaccess enable
/ disable into the actual __get_user and __put_user.  This prepares for
natively implementing __get_kernel_nofault and __put_kernel_nofault.

Also don't bother with the deprecated register keyword for the error
return.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/uaccess.h | 94 ++++++++++++++++++--------------
 1 file changed, 52 insertions(+), 42 deletions(-)

diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index e8eedf22e90747..b67d1c616ec348 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -107,7 +107,6 @@ static inline int __access_ok(unsigned long addr, unsigned long size)
 do {								\
 	uintptr_t __tmp;					\
 	__typeof__(x) __x;					\
-	__enable_user_access();					\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
 		"	" insn " %1, %3\n"			\
@@ -125,7 +124,6 @@ do {								\
 		"	.previous"				\
 		: "+r" (err), "=&r" (__x), "=r" (__tmp)		\
 		: "m" (*(ptr)), "i" (-EFAULT));			\
-	__disable_user_access();				\
 	(x) = __x;						\
 } while (0)
 
@@ -138,7 +136,6 @@ do {								\
 	u32 __user *__ptr = (u32 __user *)(ptr);		\
 	u32 __lo, __hi;						\
 	uintptr_t __tmp;					\
-	__enable_user_access();					\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
 		"	lw %1, %4\n"				\
@@ -162,12 +159,30 @@ do {								\
 			"=r" (__tmp)				\
 		: "m" (__ptr[__LSW]), "m" (__ptr[__MSW]),	\
 			"i" (-EFAULT));				\
-	__disable_user_access();				\
 	(x) = (__typeof__(x))((__typeof__((x)-(x)))(		\
 		(((u64)__hi << 32) | __lo)));			\
 } while (0)
 #endif /* CONFIG_64BIT */
 
+#define __get_user_nocheck(x, __gu_ptr, __gu_err)		\
+do {								\
+	switch (sizeof(*__gu_ptr)) {				\
+	case 1:							\
+		__get_user_asm("lb", (x), __gu_ptr, __gu_err);	\
+		break;						\
+	case 2:							\
+		__get_user_asm("lh", (x), __gu_ptr, __gu_err);	\
+		break;						\
+	case 4:							\
+		__get_user_asm("lw", (x), __gu_ptr, __gu_err);	\
+		break;						\
+	case 8:							\
+		__get_user_8((x), __gu_ptr, __gu_err);	\
+		break;						\
+	default:						\
+		BUILD_BUG();					\
+	}							\
+} while (0)
 
 /**
  * __get_user: - Get a simple variable from user space, with less checking.
@@ -191,25 +206,15 @@ do {								\
  */
 #define __get_user(x, ptr)					\
 ({								\
-	register long __gu_err = 0;				\
 	const __typeof__(*(ptr)) __user *__gu_ptr = (ptr);	\
+	long __gu_err = 0;					\
+								\
 	__chk_user_ptr(__gu_ptr);				\
-	switch (sizeof(*__gu_ptr)) {				\
-	case 1:							\
-		__get_user_asm("lb", (x), __gu_ptr, __gu_err);	\
-		break;						\
-	case 2:							\
-		__get_user_asm("lh", (x), __gu_ptr, __gu_err);	\
-		break;						\
-	case 4:							\
-		__get_user_asm("lw", (x), __gu_ptr, __gu_err);	\
-		break;						\
-	case 8:							\
-		__get_user_8((x), __gu_ptr, __gu_err);	\
-		break;						\
-	default:						\
-		BUILD_BUG();					\
-	}							\
+								\
+	__enable_user_access();					\
+	__get_user_nocheck(x, __gu_ptr, __gu_err);		\
+	__disable_user_access();				\
+								\
 	__gu_err;						\
 })
 
@@ -243,7 +248,6 @@ do {								\
 do {								\
 	uintptr_t __tmp;					\
 	__typeof__(*(ptr)) __x = x;				\
-	__enable_user_access();					\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
 		"	" insn " %z3, %2\n"			\
@@ -260,7 +264,6 @@ do {								\
 		"	.previous"				\
 		: "+r" (err), "=r" (__tmp), "=m" (*(ptr))	\
 		: "rJ" (__x), "i" (-EFAULT));			\
-	__disable_user_access();				\
 } while (0)
 
 #ifdef CONFIG_64BIT
@@ -272,7 +275,6 @@ do {								\
 	u32 __user *__ptr = (u32 __user *)(ptr);		\
 	u64 __x = (__typeof__((x)-(x)))(x);			\
 	uintptr_t __tmp;					\
-	__enable_user_access();					\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
 		"	sw %z4, %2\n"				\
@@ -294,10 +296,28 @@ do {								\
 			"=m" (__ptr[__LSW]),			\
 			"=m" (__ptr[__MSW])			\
 		: "rJ" (__x), "rJ" (__x >> 32), "i" (-EFAULT));	\
-	__disable_user_access();				\
 } while (0)
 #endif /* CONFIG_64BIT */
 
+#define __put_user_nocheck(x, __gu_ptr, __pu_err)					\
+do {								\
+	switch (sizeof(*__gu_ptr)) {				\
+	case 1:							\
+		__put_user_asm("sb", (x), __gu_ptr, __pu_err);	\
+		break;						\
+	case 2:							\
+		__put_user_asm("sh", (x), __gu_ptr, __pu_err);	\
+		break;						\
+	case 4:							\
+		__put_user_asm("sw", (x), __gu_ptr, __pu_err);	\
+		break;						\
+	case 8:							\
+		__put_user_8((x), __gu_ptr, __pu_err);	\
+		break;						\
+	default:						\
+		BUILD_BUG();					\
+	}							\
+} while (0)
 
 /**
  * __put_user: - Write a simple value into user space, with less checking.
@@ -320,25 +340,15 @@ do {								\
  */
 #define __put_user(x, ptr)					\
 ({								\
-	register long __pu_err = 0;				\
 	__typeof__(*(ptr)) __user *__gu_ptr = (ptr);		\
+	long __pu_err = 0;					\
+								\
 	__chk_user_ptr(__gu_ptr);				\
-	switch (sizeof(*__gu_ptr)) {				\
-	case 1:							\
-		__put_user_asm("sb", (x), __gu_ptr, __pu_err);	\
-		break;						\
-	case 2:							\
-		__put_user_asm("sh", (x), __gu_ptr, __pu_err);	\
-		break;						\
-	case 4:							\
-		__put_user_asm("sw", (x), __gu_ptr, __pu_err);	\
-		break;						\
-	case 8:							\
-		__put_user_8((x), __gu_ptr, __pu_err);	\
-		break;						\
-	default:						\
-		BUILD_BUG();					\
-	}							\
+								\
+	__enable_user_access();					\
+	__put_user_nocheck(x, __gu_ptr, __pu_err);		\
+	__disable_user_access();				\
+								\
 	__pu_err;						\
 })
 
-- 
2.28.0

[PATCH 7/8] riscv: implement __get_kernel_nofault and __put_user_nofault

[Christoph Hellwig]

Implemet the non-faulting kernel access helpers directly instead of
abusing the uaccess routines under set_fs(KERNEL_DS).

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/uaccess.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index b67d1c616ec348..264e52fb62b143 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -486,6 +486,26 @@ unsigned long __must_check clear_user(void __user *to, unsigned long n)
 	__ret;							\
 })
 
+#define HAVE_GET_KERNEL_NOFAULT
+
+#define __get_kernel_nofault(dst, src, type, err_label)			\
+do {									\
+	long __kr_err;							\
+									\
+	__get_user_nocheck(*((type *)(dst)), (type *)(src), __kr_err);	\
+	if (unlikely(__kr_err))						\
+		goto err_label;						\
+} while (0)
+
+#define __put_kernel_nofault(dst, src, type, err_label)			\
+do {									\
+	long __kr_err;							\
+									\
+	__put_user_nocheck(*((type *)(dst)), (type *)(src), __kr_err);	\
+	if (unlikely(__kr_err))						\
+		goto err_label;						\
+} while (0)
+
 #else /* CONFIG_MMU */
 #include <asm-generic/uaccess.h>
 #endif /* CONFIG_MMU */
-- 
2.28.0

[PATCH 8/8] riscv: remove address space overrides using set_fs()

[Christoph Hellwig]

Stop providing the possibility to override the address space using
set_fs() now that there is no need for that any more.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/Kconfig                   |  1 -
 arch/riscv/include/asm/thread_info.h |  6 ------
 arch/riscv/include/asm/uaccess.h     | 27 +--------------------------
 arch/riscv/kernel/process.c          |  1 -
 4 files changed, 1 insertion(+), 34 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 460e3971a80fde..33dde87218ddab 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -86,7 +86,6 @@ config RISCV
 	select SPARSE_IRQ
 	select SYSCTL_EXCEPTION_TRACE
 	select THREAD_INFO_IN_TASK
-	select SET_FS
 	select UACCESS_MEMCPY if !MMU
 
 config ARCH_MMAP_RND_BITS_MIN
diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h
index 464a2bbc97ea33..a390711129de64 100644
--- a/arch/riscv/include/asm/thread_info.h
+++ b/arch/riscv/include/asm/thread_info.h
@@ -24,10 +24,6 @@
 #include <asm/processor.h>
 #include <asm/csr.h>
 
-typedef struct {
-	unsigned long seg;
-} mm_segment_t;
-
 /*
  * low level task data that entry.S needs immediate access to
  * - this struct should fit entirely inside of one cache line
@@ -39,7 +35,6 @@ typedef struct {
 struct thread_info {
 	unsigned long		flags;		/* low level flags */
 	int                     preempt_count;  /* 0=>preemptible, <0=>BUG */
-	mm_segment_t		addr_limit;
 	/*
 	 * These stack pointers are overwritten on every system call or
 	 * exception.  SP is also saved to the stack it can be recovered when
@@ -59,7 +54,6 @@ struct thread_info {
 {						\
 	.flags		= 0,			\
 	.preempt_count	= INIT_PREEMPT_COUNT,	\
-	.addr_limit	= KERNEL_DS,		\
 }
 
 #endif /* !__ASSEMBLY__ */
diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 264e52fb62b143..c47e6b35c551f4 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -26,29 +26,6 @@
 #define __disable_user_access()							\
 	__asm__ __volatile__ ("csrc sstatus, %0" : : "r" (SR_SUM) : "memory")
 
-/*
- * The fs value determines whether argument validity checking should be
- * performed or not.  If get_fs() == USER_DS, checking is performed, with
- * get_fs() == KERNEL_DS, checking is bypassed.
- *
- * For historical reasons, these macros are grossly misnamed.
- */
-
-#define MAKE_MM_SEG(s)	((mm_segment_t) { (s) })
-
-#define KERNEL_DS	MAKE_MM_SEG(~0UL)
-#define USER_DS		MAKE_MM_SEG(TASK_SIZE)
-
-#define get_fs()	(current_thread_info()->addr_limit)
-
-static inline void set_fs(mm_segment_t fs)
-{
-	current_thread_info()->addr_limit = fs;
-}
-
-#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
-#define user_addr_max()	(get_fs().seg)
-
 /**
  * access_ok: - Checks if a user space pointer is valid
  * @addr: User space pointer to start of block to check
@@ -76,9 +53,7 @@ static inline void set_fs(mm_segment_t fs)
  */
 static inline int __access_ok(unsigned long addr, unsigned long size)
 {
-	const mm_segment_t fs = get_fs();
-
-	return size <= fs.seg && addr <= fs.seg - size;
+	return size <= TASK_SIZE && addr <= TASK_SIZE - size;
 }
 
 /*
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index 2b97c493427c9e..19225ec65db62f 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -84,7 +84,6 @@ void start_thread(struct pt_regs *regs, unsigned long pc,
 	}
 	regs->epc = pc;
 	regs->sp = sp;
-	set_fs(USER_DS);
 }
 
 void flush_thread(void)
-- 
2.28.0

Re: [PATCH 3/8] asm-generic: fix unaligned access hamdling in raw_copy_{from,to}_user

[Arnd Bergmann]

On Fri, Sep 4, 2020 at 6:52 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Use get_unaligned and put_unaligned for the small constant size cases
> in the generic uaccess routines.  This ensures they can be used for
> architectures that do not support unaligned loads and stores, while
> being a no-op for those that do.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/asm-generic/uaccess.h | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
> index cc3b2c8b68fab4..768502bbfb154e 100644
> --- a/include/asm-generic/uaccess.h
> +++ b/include/asm-generic/uaccess.h
> @@ -36,19 +36,17 @@ raw_copy_from_user(void *to, const void __user * from, unsigned long n)
>         if (__builtin_constant_p(n)) {
>                 switch(n) {
>                 case 1:
> -                       *(u8 *)to = *(u8 __force *)from;
> +                       *(u8 *)to = get_unaligned((u8 __force *)from);
>                         return 0;
>                 case 2:
> -                       *(u16 *)to = *(u16 __force *)from;
> +                       *(u16 *)to = get_unaligned((u16 __force *)from);
>                         return 0;

The change look correct and necessary, but I wonder if this could be done
in a way that is a little easier on the compiler than the nested switch/case.

If I see it right, __put_user() and __get_user() can probably
be reduced to a plain put_unaligned() and get_unaligned() here,
which would simplify these a lot.

In turn it seems that the generic raw_copy_to_user() can just be the
a plain memcpy(), IIRC the optimization for small sizes should also
be done by modern compilers whenever they can.

     Arnd

Re: [PATCH 3/8] asm-generic: fix unaligned access hamdling in raw_copy_{from,to}_user

[Al Viro]

On Fri, Sep 04, 2020 at 06:52:11PM +0200, Christoph Hellwig wrote:
> Use get_unaligned and put_unaligned for the small constant size cases
> in the generic uaccess routines.  This ensures they can be used for
> architectures that do not support unaligned loads and stores, while
> being a no-op for those that do.

Frankly, I would rather get rid of those constant-sized cases entirely;
sure, we'd need to adjust asm-generic/uaccess.h defaults for __get_user(),
but there that kind of stuff would make sense; in raw_copy_from_user()
it really doesn't.

Re: remove set_fs for riscv

[Arnd Bergmann]

On Fri, Sep 4, 2020 at 6:52 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Hi all,
>
> this series converts riscv to the new set_fs less world and is on top of this
> branch:
>
>     https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=base.set_fs
>
> The first four patches are general improvements and enablement for all nommu
> ports, and might make sense to merge through the above base branch.

For asm-generic:

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

Is there a bigger plan for the rest? I can probably have a look at the Arm
OABI code if nobody else working on that yet.

      Arnd

Re: [PATCH 3/8] asm-generic: fix unaligned access hamdling in raw_copy_{from,to}_user

[Al Viro]

On Fri, Sep 04, 2020 at 07:06:17PM +0100, Al Viro wrote:
> On Fri, Sep 04, 2020 at 06:52:11PM +0200, Christoph Hellwig wrote:
> > Use get_unaligned and put_unaligned for the small constant size cases
> > in the generic uaccess routines.  This ensures they can be used for
> > architectures that do not support unaligned loads and stores, while
> > being a no-op for those that do.
> 
> Frankly, I would rather get rid of those constant-sized cases entirely;
> sure, we'd need to adjust asm-generic/uaccess.h defaults for __get_user(),
> but there that kind of stuff would make sense; in raw_copy_from_user()
> it really doesn't.

FWIW, we have asm-generic/uaccess.h used by
	arc
	c6x
	hexagon
	riscv/!MMU
	um
by direct includes from asm/uaccess.h
	h8300
picked as default from asm-generic, in place of absent native uaccess.h

In asm-generic/uaccess.h we have
	raw_copy_from_user(): CONFIG_UACCESS_MEMCPY
		[h8300, riscv with your series]
	raw_copy_to_user(): CONFIG_UACCESS_MEMCP
		[h8300, riscv with your series]
	set_fs group: MAKE_MM_SEG KERNEL_DS USER_DS set_fs() get_fs() uaccess_kernel()
		all, really
	access_ok()/__access_ok() (unless overridden)
		[c6x/!CONFIG_ACCESS_CHECK h8300 riscv]
	__put_user()/put_user()
		all, implemented via __put_user_fn()
	__put_user_fn(): raw_copy_to_user(), unless overridden [all except arc]
	__get_user()/get_user()
		all, implemented via __get_user_fn()
	__get_user_fn(): raw_copy_from_user(), unless overridden [all except arc]
	__strncpy_from_user() (unless overridden) [c6x h8300 riscv]
	strncpy_from_user()
	__strnlen_user() (unless overridden) [c6x h8300 riscv]
	strnlen_user()
	__clear_user() (unless overridden) [c6x h8300 riscv]
	clear_user()

__strncpy_from_user()/__strnlen_user()/__clear_user() are never used outside
of arch/*, and there almost all callers are non-__ variants of the same.
Exceptions:
arch/hexagon/include/asm/uaccess.h:76:  long res = __strnlen_user(src, n);
	racy implementation of __strncpy_from_user()
arch/c6x/kernel/signal.c:157:   err |= __clear_user(&frame->uc, offsetof(struct ucontext, uc_mcontext));
arch/x86/include/asm/fpu/internal.h:367:        err = __clear_user(&buf->header, sizeof(buf->header));
arch/x86/kernel/fpu/signal.c:138:       if (unlikely(err) && __clear_user(buf, fpu_user_xstate_size))

and that's it.

	Now, if you look at raw_copy_from_user() you'll see an interesting
picture: some architectures special-case the handling of small constant sizes.
Namely,
	arc (any size; inlining in there is obscene, constant size or not),
	c6x (1,4,8),
	m68k/MMU (1,2,3,4,5,6,7,8,9,10,12)
	ppc (1,2,4,8),
	h8300 (1,2,4),
	riscv (with your series)(1,2,4, 8 if 64bit).

	If you look at the callers of e.g. raw_copy_from_user(), you'll
see this:
	* default __get_user_fn() [relevant on c6x, h8300 and riscv - in
all cases it should be doing get_unaligned() instead]
	* __copy_from_user_inatomic()
	* __copy_from_user()
	* copy_from_user() in case of INLINE_COPY_FROM_USER [relevant on
arc, c6x and m68k]
	* lib/* callers, all with non-constant sizes.

Callers of __copy_from_user_inatomic() on relevant architectures, excluding the
ones with non-constant (or huge - several get PAGE_SIZE) sizes:
	* [ppc] p9_hmi_special_emu() - 16 bytes read; not recognized as special
	* [riscv] user_backtrace() - 2 words read; not recognized as special
	* __copy_from_user_inatomic_nocache()
	* arch_perf_out_copy_user()

All callers of __copy_from_user_inatomic_nocache() pass it non-constant sizes.
arch_perf_out_copy_user() is called (via layers of preprocessor abuse) via
__output_copy_user(), which gets non-constant size.

Callers of __copy_from_user() potentially hitting those:
arch/arc/kernel/signal.c:108:   err = __copy_from_user(&set, &sf->uc.uc_sigmask, sizeof(set));
arch/c6x/kernel/signal.c:82:    if (__copy_from_user(&set, &frame->uc.uc_sigmask, sizeof(set)))
arch/h8300/kernel/signal.c:114: if (__copy_from_user(&set, &frame->uc.uc_sigmask, sizeof(set)))
arch/m68k/kernel/signal.c:340:          if (__copy_from_user(current->thread.fpcntl,
arch/m68k/kernel/signal.c:794:       __copy_from_user(&set.sig[1], &frame->extramask,
arch/m68k/kernel/signal.c:817:  if (__copy_from_user(&set, &frame->uc.uc_sigmask, sizeof(set)))
arch/powerpc/kernel/signal_64.c:688:    if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
arch/powerpc/kernel/signal_64.c:719:    if (__copy_from_user(&set, &uc->uc_sigmask, sizeof(set)))
arch/powerpc/kvm/book3s_64_mmu_hv.c:1864:               if (__copy_from_user(&hdr, buf, sizeof(hdr)))
arch/riscv/kernel/signal.c:113: if (__copy_from_user(&set, &frame->uc.uc_sigmask, sizeof(set)))
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:2244:            if (__copy_from_user(&fence, user++, sizeof(fence))) {
include/linux/regset.h:256:             } else if (__copy_from_user(data, *ubuf, copy))

The last one is user_regset_copyin() and it's going to die.
A bunch of signal-related ones are in in sigreturn variants, reading
sigset_t.  Considering that shitloads of data get copied nearby for
each such call, I would be surprised if those would be worth bothering
with.   Remaining ppc case is kvm_htab_write(), which just might be
hot enough to care; we are copying a 64bit structure, and it might
be worth reading it as a single 64bit.  And i915 is reading 64bit
objects in a loop.  Hell knows, might or might not be hot.

copy_from_user() callers on arc, c6x and m68k boil down to one case:
arch/arc/kernel/disasm.c:37:            bytes_not_copied = copy_from_user(ins_buf,
8-byte read.  And that's it.

IOW, there's a scattering of potentially valid uses that might be better
off with get_user().  And there's generic non-MMU variant that gets
used in get_user()/__get_user() on h8300 and riscv.  This one *is*
valid, but I don't think that raw_copy_from_user() is the right layer
for that.

For raw_copy_to_user() the picture is similar.  And I'd like to get
rid of that magical crap.  Let's not make it harder...

Re: [PATCH 3/8] asm-generic: fix unaligned access hamdling in raw_copy_{from,to}_user

[Christoph Hellwig]

On Fri, Sep 04, 2020 at 08:04:34PM +0200, Arnd Bergmann wrote:
> >         if (__builtin_constant_p(n)) {
> >                 switch(n) {
> >                 case 1:
> > -                       *(u8 *)to = *(u8 __force *)from;
> > +                       *(u8 *)to = get_unaligned((u8 __force *)from);
> >                         return 0;
> >                 case 2:
> > -                       *(u16 *)to = *(u16 __force *)from;
> > +                       *(u16 *)to = get_unaligned((u16 __force *)from);
> >                         return 0;
> 
> The change look correct and necessary, but I wonder if this could be done
> in a way that is a little easier on the compiler than the nested switch/case.
> 
> If I see it right, __put_user() and __get_user() can probably
> be reduced to a plain put_unaligned() and get_unaligned() here,
> which would simplify these a lot.
> 
> In turn it seems that the generic raw_copy_to_user() can just be the
> a plain memcpy(), IIRC the optimization for small sizes should also
> be done by modern compilers whenever they can.

Sure, I can look into that.

Re: remove set_fs for riscv

[Christoph Hellwig]

On Fri, Sep 04, 2020 at 08:15:03PM +0200, Arnd Bergmann wrote:
> Is there a bigger plan for the rest? I can probably have a look at the Arm
> OABI code if nobody else working on that yet.

m68knommu seems mostly trivial and not interact much with m68k/mmu,
so that woud be my next target.  All the other seems to share more
code for the mmu and nommu case, so they'd have to be done per arch.

arm would be my first target because it is used widespread, and its
current set_fs implemenetation is very strange.  But given thar you
help maintaining arm SOCs and probably know the arch code much better
than I do I'd be more than happy to leave that to you.

Re: remove set_fs for riscv

[Arnd Bergmann]

On Sat, Sep 5, 2020 at 9:17 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Fri, Sep 04, 2020 at 08:15:03PM +0200, Arnd Bergmann wrote:
> > Is there a bigger plan for the rest? I can probably have a look at the Arm
> > OABI code if nobody else working on that yet.
>
> m68knommu seems mostly trivial and not interact much with m68k/mmu,
> so that woud be my next target.  All the other seems to share more
> code for the mmu and nommu case, so they'd have to be done per arch.

Ok.

> arm would be my first target because it is used widespread, and its
> current set_fs implemenetation is very strange.  But given thar you
> help maintaining arm SOCs and probably know the arch code much better
> than I do I'd be more than happy to leave that to you.

I would start with the syscall wrapper code that just needs a simple
set of changes to pass the arguments on as kernel pointers instead
of fake user pointers.

I'm also not too familiar with the domain handling on older Arm cores,
which I think is the main difference to other architectures. On modern
Armv6+, the set_fs() call is just an assignment to current_thread_info()->
addr_limit like on other architectures, whereas Armv5 and older
rely on special load/store instructions to perform get_user/put_user
as an unprivileged access. Removing set_fs() should allow to clean
that up nicely, but I'd worry about introducing regressions in the
process, and will probably stop short of that cleanup.

     Arnd

Re: [PATCH 3/8] asm-generic: fix unaligned access hamdling in raw_copy_{from,to}_user

[Al Viro]

On Fri, Sep 04, 2020 at 11:35:18PM +0100, Al Viro wrote:

> 	Now, if you look at raw_copy_from_user() you'll see an interesting
> picture: some architectures special-case the handling of small constant sizes.
> Namely,
> 	arc (any size; inlining in there is obscene, constant size or not),
> 	c6x (1,4,8),
> 	m68k/MMU (1,2,3,4,5,6,7,8,9,10,12)
> 	ppc (1,2,4,8),
> 	h8300 (1,2,4),
> 	riscv (with your series)(1,2,4, 8 if 64bit).

FWIW, on the raw_copy_to_user() side the same set of constant sizes is
recongized by the same architectures and we have
	* __put_user/put_user in asm-generic/uaccess.h make use of that
	* arc, c6x, ppc and riscv using it to store sigset_t on sigframe
	* 3 odd callers:
		* arc stash_usr_regs(), inlined and unrolled large copy_to_user()
		* ppc kvm_htab_read(), 64bit store.
		* i915_gem_execbuffer_ioctl():
                        if (__copy_to_user(&user_exec_list[i].offset,
                                           &exec2_list[i].offset,
                                           sizeof(user_exec_list[i].offset)))
		in a loop.  'offset' here is __u64.

That's it.  IOW, asm-generic put_user() is the only real cause to have those 
magic sizes recognized on raw_copy_to_user() side.

Re: remove set_fs for riscv

[Arnd Bergmann]

On Sat, Sep 5, 2020 at 9:17 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Fri, Sep 04, 2020 at 08:15:03PM +0200, Arnd Bergmann wrote:
> > Is there a bigger plan for the rest? I can probably have a look at the Arm
> > OABI code if nobody else working on that yet.
>
> m68knommu seems mostly trivial and not interact much with m68k/mmu,
> so that woud be my next target.  All the other seems to share more
> code for the mmu and nommu case, so they'd have to be done per arch.
>
> arm would be my first target because it is used widespread, and its
> current set_fs implemenetation is very strange.  But given thar you
> help maintaining arm SOCs and probably know the arch code much better
> than I do I'd be more than happy to leave that to you.

I've had a first pass at this now, see

https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=arm-kill-set_fs

There are a couple of things in there that ended up uglier than I was
hoping for, and it's completely untested beyond compilation. Is this
roughly what you had in mind? I can do some testing then and post
it to the Arm mailing list.

      Arnd

Re: remove set_fs for riscv

[Christoph Hellwig]

On Mon, Sep 07, 2020 at 12:14:59AM +0200, Arnd Bergmann wrote:
> I've had a first pass at this now, see
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=arm-kill-set_fs
> 
> There are a couple of things in there that ended up uglier than I was
> hoping for, and it's completely untested beyond compilation. Is this
> roughly what you had in mind? I can do some testing then and post
> it to the Arm mailing list.

Looks sensible.  The OABI hacks a are a little ugly, but so would be
every other alternative.

Note that you don't need to add a TASK_SIZE_MAX definition to arm if you
base it on my series as that provides a default one.  I also think with
these changes arm/nommu should be able to use UACCESS_MEMCPY.

Re: [PATCH 3/8] asm-generic: fix unaligned access hamdling in raw_copy_{from,to}_user

[Arnd Bergmann]

On Sat, Sep 5, 2020 at 12:35 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Fri, Sep 04, 2020 at 07:06:17PM +0100, Al Viro wrote:
> > On Fri, Sep 04, 2020 at 06:52:11PM +0200, Christoph Hellwig wrote:
> > > Use get_unaligned and put_unaligned for the small constant size cases
> > > in the generic uaccess routines.  This ensures they can be used for
> > > architectures that do not support unaligned loads and stores, while
> > > being a no-op for those that do.
> >
> > Frankly, I would rather get rid of those constant-sized cases entirely;
> > sure, we'd need to adjust asm-generic/uaccess.h defaults for __get_user(),
> > but there that kind of stuff would make sense; in raw_copy_from_user()
> > it really doesn't.

Right. When I originally wrote that part of asm-generic/uaccess.h, the
idea was that its __get_user()/__put_user() would end up being used
across most architectures, which then would only have to implement
custom __copy_from_user()/__copy_to_user() with those special cases
to get the optimum behavior. It didn't work out in the end, since
copy_from_user() also has to deal with unaligned or partial copies
that prevent it from degrading into a single instruction on anything
other than the simplest NOMMU architectures.

I'd still hope we can eventually come up with a generic
__get_user()/__put_user() helper that avoids all the common
architecture specific bugs in them, with a simpler way for
an architecture to hook into with a set of inline functions
while leaving the macros to common code, but that can be
done another time.

> IOW, there's a scattering of potentially valid uses that might be better
> off with get_user().  And there's generic non-MMU variant that gets
> used in get_user()/__get_user() on h8300 and riscv.  This one *is*
> valid, but I don't think that raw_copy_from_user() is the right layer
> for that.
>
> For raw_copy_to_user() the picture is similar.  And I'd like to get
> rid of that magical crap.  Let's not make it harder...

Agreed

         Arnd

Re: remove set_fs for riscv

[Arnd Bergmann]

On Mon, Sep 7, 2020 at 8:03 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Sep 07, 2020 at 12:14:59AM +0200, Arnd Bergmann wrote:
> > I've had a first pass at this now, see
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=arm-kill-set_fs
> >
> > There are a couple of things in there that ended up uglier than I was
> > hoping for, and it's completely untested beyond compilation. Is this
> > roughly what you had in mind? I can do some testing then and post
> > it to the Arm mailing list.
>
> Looks sensible.  The OABI hacks a are a little ugly, but so would be
> every other alternative.

Ok, thanks for taking a look. I've now managed to run the patched
kernel with OABI user space and tested the modified syscalls with
LTP. The 0-day bot found a regression that I have fixed.

I'll send out the series for review next.

> Note that you don't need to add a TASK_SIZE_MAX definition to arm if you
> base it on my series as that provides a default one.

I've rebased on that patch now and taken out those definitions.

> I also think with these changes arm/nommu should be able to use
> UACCESS_MEMCPY.

Probably yes, but I'd leave the current version for now. The Arm
implementation already supports combinations of range check
and domain settings, with the NOMMU targets using neither, but
sharing the same implementation as the others.

      Arnd

Re: [PATCH 3/8] asm-generic: fix unaligned access hamdling in raw_copy_{from, to}_user

[Sean Anderson]

> Re: [PATCH 3/8] asm-generic: fix unaligned access hamdling in raw_copy_{from, to}_user

nit: handling

--Sean