[PATCH v1 00/15] powerpc: Cleanup of uaccess.h

[PATCH v1 00/15] powerpc: Cleanup of uaccess.h

[Christophe Leroy]

This series cleans up uaccess.h

Christophe Leroy (15):
  powerpc/uaccess: Remove __get_user_allowed() and unsafe_op_wrap()
  powerpc/uaccess: Define ___get_user_instr() for ppc32
  powerpc/uaccess: Remove __get/put_user_inatomic()
  powerpc/uaccess: Move get_user_instr helpers in asm/inst.h
  powerpc/align: Don't use __get_user_instr() on kernel addresses
  powerpc/uaccess: Call might_fault() inconditionaly
  powerpc/uaccess: Remove __unsafe_put_user_goto()
  powerpc/uaccess: Remove __chk_user_ptr() in __get/put_user
  powerpc/uaccess: Remove calls to __get_user_bad() and __put_user_bad()
  powerpc/uaccess: Split out __get_user_nocheck()
  powerpc/uaccess: Rename __get/put_user_check/nocheck
  powerpc/uaccess: Refactor get/put_user() and __get/put_user()
  powerpc/uaccess: Swap clear_user() and __clear_user()
  powerpc/uaccess: Also perform 64 bits copies in unsafe_copy_to_user()
    on ppc32
  powerpc/uaccess: Move copy_mc_xxx() functions down

 arch/powerpc/include/asm/inst.h               |  34 ++
 arch/powerpc/include/asm/uaccess.h            | 303 ++++++------------
 arch/powerpc/kernel/align.c                   |  38 ++-
 .../kernel/hw_breakpoint_constraints.c        |   2 +-
 arch/powerpc/kernel/traps.c                   |   2 +-
 5 files changed, 147 insertions(+), 232 deletions(-)

-- 
2.25.0

[PATCH v1 01/15] powerpc/uaccess: Remove __get_user_allowed() and unsafe_op_wrap()

[Christophe Leroy]

Those two macros have only one user which is unsafe_get_user().

Put everything in one place and remove them.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 78e2a3990eab..8cbf3e3874f1 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -53,9 +53,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
 #define __put_user(x, ptr) \
 	__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
 
-#define __get_user_allowed(x, ptr) \
-	__get_user_nocheck((x), (ptr), sizeof(*(ptr)), false)
-
 #define __get_user_inatomic(x, ptr) \
 	__get_user_nosleep((x), (ptr), sizeof(*(ptr)))
 #define __put_user_inatomic(x, ptr) \
@@ -482,8 +479,11 @@ user_write_access_begin(const void __user *ptr, size_t len)
 #define user_write_access_begin	user_write_access_begin
 #define user_write_access_end		prevent_current_write_to_user
 
-#define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0)
-#define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
+#define unsafe_get_user(x, p, e) do {					\
+	if (unlikely(__get_user_nocheck((x), (p), sizeof(*(p)), false)))\
+		goto e;							\
+} while (0)
+
 #define unsafe_put_user(x, p, e) \
 	__unsafe_put_user_goto((__typeof__(*(p)))(x), (p), sizeof(*(p)), e)
 
-- 
2.25.0

[PATCH v1 02/15] powerpc/uaccess: Define ___get_user_instr() for ppc32

[Christophe Leroy]

Define simple ___get_user_instr() for ppc32 instead of
defining ppc32 versions of the three get_user_instr()
helpers.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 8cbf3e3874f1..a08c482b1315 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -81,6 +81,10 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
 	}								\
 	__gui_ret;							\
 })
+#else /* !CONFIG_PPC64 */
+#define ___get_user_instr(gu_op, dest, ptr)				\
+	gu_op((dest).val, (u32 __user *)(ptr))
+#endif /* CONFIG_PPC64 */
 
 #define get_user_instr(x, ptr) \
 	___get_user_instr(get_user, x, ptr)
@@ -91,18 +95,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
 #define __get_user_instr_inatomic(x, ptr) \
 	___get_user_instr(__get_user_inatomic, x, ptr)
 
-#else /* !CONFIG_PPC64 */
-#define get_user_instr(x, ptr) \
-	get_user((x).val, (u32 __user *)(ptr))
-
-#define __get_user_instr(x, ptr) \
-	__get_user_nocheck((x).val, (u32 __user *)(ptr), sizeof(u32), true)
-
-#define __get_user_instr_inatomic(x, ptr) \
-	__get_user_nosleep((x).val, (u32 __user *)(ptr), sizeof(u32))
-
-#endif /* CONFIG_PPC64 */
-
 extern long __put_user_bad(void);
 
 #define __put_user_size(x, ptr, size, retval)			\
-- 
2.25.0

[PATCH v1 03/15] powerpc/uaccess: Remove __get/put_user_inatomic()

[Christophe Leroy]

Since commit 662bbcb2747c ("mm, sched: Allow uaccess in atomic with
pagefault_disable()"), __get/put_user() can be used in atomic parts
of the code, therefore the __get/put_user_inatomic() introduced
by commit e68c825bb016 ("[POWERPC] Add inatomic versions of __get_user
and __put_user") have become useless.

powerpc is the only one having such functions. There is a real
intention not to have to provide such _inatomic() helpers, see the
comment in might_fault() in mm/memory.c introduced by
commit 3ee1afa308f2 ("x86: some lock annotations for user
copy paths, v2"):

	/*
	 * it would be nicer only to annotate paths which are not under
	 * pagefault_disable, however that requires a larger audit and
	 * providing helpers like get_user_atomic.
	 */

So remove __get_user_inatomic() and __put_user_inatomic().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h            | 37 -------------------
 arch/powerpc/kernel/align.c                   | 32 ++++++++--------
 .../kernel/hw_breakpoint_constraints.c        |  2 +-
 arch/powerpc/kernel/traps.c                   |  2 +-
 4 files changed, 18 insertions(+), 55 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index a08c482b1315..01aea0df4dd0 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -53,11 +53,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
 #define __put_user(x, ptr) \
 	__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
 
-#define __get_user_inatomic(x, ptr) \
-	__get_user_nosleep((x), (ptr), sizeof(*(ptr)))
-#define __put_user_inatomic(x, ptr) \
-	__put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
-
 #ifdef CONFIG_PPC64
 
 #define ___get_user_instr(gu_op, dest, ptr)				\
@@ -92,9 +87,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
 #define __get_user_instr(x, ptr) \
 	___get_user_instr(__get_user, x, ptr)
 
-#define __get_user_instr_inatomic(x, ptr) \
-	___get_user_instr(__get_user_inatomic, x, ptr)
-
 extern long __put_user_bad(void);
 
 #define __put_user_size(x, ptr, size, retval)			\
@@ -141,20 +133,6 @@ __pu_failed:							\
 	__pu_err;							\
 })
 
-#define __put_user_nosleep(x, ptr, size)			\
-({								\
-	long __pu_err;						\
-	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
-	__typeof__(*(ptr)) __pu_val = (x);			\
-	__typeof__(size) __pu_size = (size);			\
-								\
-	__chk_user_ptr(__pu_addr);				\
-	__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
-								\
-	__pu_err;						\
-})
-
-
 /*
  * We don't tell gcc that we are accessing memory, but this is OK
  * because we do not write to any memory gcc knows about, so there
@@ -320,21 +298,6 @@ do {								\
 	__gu_err;							\
 })
 
-#define __get_user_nosleep(x, ptr, size)			\
-({								\
-	long __gu_err;						\
-	__long_type(*(ptr)) __gu_val;				\
-	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
-	__typeof__(size) __gu_size = (size);			\
-								\
-	__chk_user_ptr(__gu_addr);				\
-	__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
-	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
-								\
-	__gu_err;						\
-})
-
-
 /* more complex routines */
 
 extern unsigned long __copy_tofrom_user(void __user *to,
diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index c7797eb958c7..83b199026a1e 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -174,18 +174,18 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
 
 		switch (nb) {
 		case 8:
-			ret |= __get_user_inatomic(temp.v[0], p++);
-			ret |= __get_user_inatomic(temp.v[1], p++);
-			ret |= __get_user_inatomic(temp.v[2], p++);
-			ret |= __get_user_inatomic(temp.v[3], p++);
+			ret |= __get_user(temp.v[0], p++);
+			ret |= __get_user(temp.v[1], p++);
+			ret |= __get_user(temp.v[2], p++);
+			ret |= __get_user(temp.v[3], p++);
 			fallthrough;
 		case 4:
-			ret |= __get_user_inatomic(temp.v[4], p++);
-			ret |= __get_user_inatomic(temp.v[5], p++);
+			ret |= __get_user(temp.v[4], p++);
+			ret |= __get_user(temp.v[5], p++);
 			fallthrough;
 		case 2:
-			ret |= __get_user_inatomic(temp.v[6], p++);
-			ret |= __get_user_inatomic(temp.v[7], p++);
+			ret |= __get_user(temp.v[6], p++);
+			ret |= __get_user(temp.v[7], p++);
 			if (unlikely(ret))
 				return -EFAULT;
 		}
@@ -259,18 +259,18 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
 		p = addr;
 		switch (nb) {
 		case 8:
-			ret |= __put_user_inatomic(data.v[0], p++);
-			ret |= __put_user_inatomic(data.v[1], p++);
-			ret |= __put_user_inatomic(data.v[2], p++);
-			ret |= __put_user_inatomic(data.v[3], p++);
+			ret |= __put_user(data.v[0], p++);
+			ret |= __put_user(data.v[1], p++);
+			ret |= __put_user(data.v[2], p++);
+			ret |= __put_user(data.v[3], p++);
 			fallthrough;
 		case 4:
-			ret |= __put_user_inatomic(data.v[4], p++);
-			ret |= __put_user_inatomic(data.v[5], p++);
+			ret |= __put_user(data.v[4], p++);
+			ret |= __put_user(data.v[5], p++);
 			fallthrough;
 		case 2:
-			ret |= __put_user_inatomic(data.v[6], p++);
-			ret |= __put_user_inatomic(data.v[7], p++);
+			ret |= __put_user(data.v[6], p++);
+			ret |= __put_user(data.v[7], p++);
 		}
 		if (unlikely(ret))
 			return -EFAULT;
diff --git a/arch/powerpc/kernel/hw_breakpoint_constraints.c b/arch/powerpc/kernel/hw_breakpoint_constraints.c
index 867ee4aa026a..675d1f66ab72 100644
--- a/arch/powerpc/kernel/hw_breakpoint_constraints.c
+++ b/arch/powerpc/kernel/hw_breakpoint_constraints.c
@@ -141,7 +141,7 @@ void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
 {
 	struct instruction_op op;
 
-	if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip))
+	if (__get_user_instr(*instr, (void __user *)regs->nip))
 		return;
 
 	analyse_instr(&op, regs, *instr);
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 1583fd1c6010..1fa36bd08efe 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -864,7 +864,7 @@ static void p9_hmi_special_emu(struct pt_regs *regs)
 	unsigned long ea, msr, msr_mask;
 	bool swap;
 
-	if (__get_user_inatomic(instr, (unsigned int __user *)regs->nip))
+	if (__get_user(instr, (unsigned int __user *)regs->nip))
 		return;
 
 	/*
-- 
2.25.0

[PATCH v1 04/15] powerpc/uaccess: Move get_user_instr helpers in asm/inst.h

[Christophe Leroy]

Those helpers use get_user helpers but they don't participate
in their implementation, so they do not belong to asm/uaccess.h

Move them in asm/inst.h

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/inst.h    | 34 ++++++++++++++++++++++++++++++
 arch/powerpc/include/asm/uaccess.h | 34 ------------------------------
 2 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index cc73c1267572..19e18af2fac9 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -4,6 +4,40 @@
 
 #include <asm/ppc-opcode.h>
 
+#ifdef CONFIG_PPC64
+
+#define ___get_user_instr(gu_op, dest, ptr)				\
+({									\
+	long __gui_ret = 0;						\
+	unsigned long __gui_ptr = (unsigned long)ptr;			\
+	struct ppc_inst __gui_inst;					\
+	unsigned int __prefix, __suffix;				\
+	__gui_ret = gu_op(__prefix, (unsigned int __user *)__gui_ptr);	\
+	if (__gui_ret == 0) {						\
+		if ((__prefix >> 26) == OP_PREFIX) {			\
+			__gui_ret = gu_op(__suffix,			\
+				(unsigned int __user *)__gui_ptr + 1);	\
+			__gui_inst = ppc_inst_prefix(__prefix,		\
+						     __suffix);		\
+		} else {						\
+			__gui_inst = ppc_inst(__prefix);		\
+		}							\
+		if (__gui_ret == 0)					\
+			(dest) = __gui_inst;				\
+	}								\
+	__gui_ret;							\
+})
+#else /* !CONFIG_PPC64 */
+#define ___get_user_instr(gu_op, dest, ptr)				\
+	gu_op((dest).val, (u32 __user *)(ptr))
+#endif /* CONFIG_PPC64 */
+
+#define get_user_instr(x, ptr) \
+	___get_user_instr(get_user, x, ptr)
+
+#define __get_user_instr(x, ptr) \
+	___get_user_instr(__get_user, x, ptr)
+
 /*
  * Instruction data type for POWER
  */
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 01aea0df4dd0..eaa828a6a419 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -53,40 +53,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
 #define __put_user(x, ptr) \
 	__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
 
-#ifdef CONFIG_PPC64
-
-#define ___get_user_instr(gu_op, dest, ptr)				\
-({									\
-	long __gui_ret = 0;						\
-	unsigned long __gui_ptr = (unsigned long)ptr;			\
-	struct ppc_inst __gui_inst;					\
-	unsigned int __prefix, __suffix;				\
-	__gui_ret = gu_op(__prefix, (unsigned int __user *)__gui_ptr);	\
-	if (__gui_ret == 0) {						\
-		if ((__prefix >> 26) == OP_PREFIX) {			\
-			__gui_ret = gu_op(__suffix,			\
-				(unsigned int __user *)__gui_ptr + 1);	\
-			__gui_inst = ppc_inst_prefix(__prefix,		\
-						     __suffix);		\
-		} else {						\
-			__gui_inst = ppc_inst(__prefix);		\
-		}							\
-		if (__gui_ret == 0)					\
-			(dest) = __gui_inst;				\
-	}								\
-	__gui_ret;							\
-})
-#else /* !CONFIG_PPC64 */
-#define ___get_user_instr(gu_op, dest, ptr)				\
-	gu_op((dest).val, (u32 __user *)(ptr))
-#endif /* CONFIG_PPC64 */
-
-#define get_user_instr(x, ptr) \
-	___get_user_instr(get_user, x, ptr)
-
-#define __get_user_instr(x, ptr) \
-	___get_user_instr(__get_user, x, ptr)
-
 extern long __put_user_bad(void);
 
 #define __put_user_size(x, ptr, size, retval)			\
-- 
2.25.0

[PATCH v1 05/15] powerpc/align: Don't use __get_user_instr() on kernel addresses

[Christophe Leroy]

In the old days, when we didn't have kernel userspace access
protection and had set_fs(), it was wise to use __get_user()
and friends to read kernel memory.

Nowadays, get_user() is granting userspace access and is exclusively
for userspace access.

In alignment exception handler, use probe_kernel_read_inst()
instead of __get_user_instr() for reading instructions in kernel.

This will allow to remove the is_kernel_addr() check in
__get/put_user() in a following patch.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/align.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index 83b199026a1e..55e262627b53 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -305,7 +305,11 @@ int fix_alignment(struct pt_regs *regs)
 	 */
 	CHECK_FULL_REGS(regs);
 
-	if (unlikely(__get_user_instr(instr, (void __user *)regs->nip)))
+	if (is_kernel_addr(regs->nip))
+		r = probe_kernel_read_inst(&instr, (void *)regs->nip);
+	else
+		r = __get_user_instr(instr, (void __user *)regs->nip);
+	if (unlikely(r))
 		return -EFAULT;
 	if ((regs->msr & MSR_LE) != (MSR_KERNEL & MSR_LE)) {
 		/* We don't handle PPC little-endian any more... */
-- 
2.25.0

[PATCH v1 06/15] powerpc/uaccess: Call might_fault() inconditionaly

[Christophe Leroy]

Commit 6bfd93c32a50 ("powerpc: Fix incorrect might_sleep in
__get_user/__put_user on kernel addresses") added a check to not call
might_sleep() on kernel addresses. This was to enable the use of
__get_user() in the alignment exception handler for any address.

Then commit 95156f0051cb ("lockdep, mm: fix might_fault() annotation")
added a check of the address space in might_fault(), based on
set_fs() logic. But this didn't solve the powerpc alignment exception
case as it didn't call set_fs(KERNEL_DS).

Nowadays, set_fs() is gone, previous patch fixed the alignment
exception handler and __get_user/__put_user are not supposed to be
used anymore to read kernel memory.

Therefore the is_kernel_addr() check has become useless and can be
removed.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index eaa828a6a419..c4bbc64758a0 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -77,8 +77,7 @@ __pu_failed:							\
 	__typeof__(*(ptr)) __pu_val = (x);			\
 	__typeof__(size) __pu_size = (size);			\
 								\
-	if (!is_kernel_addr((unsigned long)__pu_addr))		\
-		might_fault();					\
+	might_fault();						\
 	__chk_user_ptr(__pu_addr);				\
 	__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err);	\
 								\
@@ -238,12 +237,12 @@ do {								\
 	__typeof__(size) __gu_size = (size);			\
 								\
 	__chk_user_ptr(__gu_addr);				\
-	if (do_allow && !is_kernel_addr((unsigned long)__gu_addr)) \
+	if (do_allow) {								\
 		might_fault();					\
-	if (do_allow)								\
 		__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err);	\
-	else									\
+	} else {									\
 		__get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
+	}									\
 	(x) = (__typeof__(*(ptr)))__gu_val;			\
 								\
 	__gu_err;						\
-- 
2.25.0

[PATCH v1 07/15] powerpc/uaccess: Remove __unsafe_put_user_goto()

[Christophe Leroy]

__unsafe_put_user_goto() is just an intermediate layer to
__put_user_size_goto() without added value other than doing
the __user pointer type checking.

Do the __user pointer type checking in __put_user_size_goto()
and remove __unsafe_put_user_goto().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index c4bbc64758a0..a6d3563cf3ee 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -130,23 +130,17 @@ __pu_failed:							\
 
 #define __put_user_size_goto(x, ptr, size, label)		\
 do {								\
+	__typeof__(*(ptr)) __user *__pus_addr = (ptr);		\
+								\
 	switch (size) {						\
-	case 1: __put_user_asm_goto(x, ptr, label, "stb"); break;	\
-	case 2: __put_user_asm_goto(x, ptr, label, "sth"); break;	\
-	case 4: __put_user_asm_goto(x, ptr, label, "stw"); break;	\
-	case 8: __put_user_asm2_goto(x, ptr, label); break;	\
+	case 1: __put_user_asm_goto(x, __pus_addr, label, "stb"); break;	\
+	case 2: __put_user_asm_goto(x, __pus_addr, label, "sth"); break;	\
+	case 4: __put_user_asm_goto(x, __pus_addr, label, "stw"); break;	\
+	case 8: __put_user_asm2_goto(x, __pus_addr, label); break;		\
 	default: __put_user_bad();				\
 	}							\
 } while (0)
 
-#define __unsafe_put_user_goto(x, ptr, size, label)		\
-do {								\
-	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
-	__chk_user_ptr(ptr);					\
-	__put_user_size_goto((x), __pu_addr, (size), label);	\
-} while (0)
-
-
 extern long __get_user_bad(void);
 
 /*
@@ -405,7 +399,7 @@ user_write_access_begin(const void __user *ptr, size_t len)
 } while (0)
 
 #define unsafe_put_user(x, p, e) \
-	__unsafe_put_user_goto((__typeof__(*(p)))(x), (p), sizeof(*(p)), e)
+	__put_user_size_goto((__typeof__(*(p)))(x), (p), sizeof(*(p)), e)
 
 #define unsafe_copy_to_user(d, s, l, e) \
 do {									\
-- 
2.25.0

[PATCH v1 08/15] powerpc/uaccess: Remove __chk_user_ptr() in __get/put_user

[Christophe Leroy]

Commit d02f6b7dab82 ("powerpc/uaccess: Evaluate macro arguments once,
before user access is allowed") changed the __chk_user_ptr()
argument from the passed ptr pointer to the locally
declared __gu_addr. But __gu_addr is locally defined as __user
so the check is pointless.

During kernel build __chk_user_ptr() voids and is only evaluated
during sparse checks so it should have been armless to leave the
original pointer check there.

Nevertheless, this check is indeed redundant with the assignment
above which casts the ptr pointer to the local __user __gu_addr.
In case of mismatch, sparse will detect it there, so the
__check_user_ptr() is not needed anywhere else than in access_ok().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index a6d3563cf3ee..a9f2639ca3a8 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -78,7 +78,6 @@ __pu_failed:							\
 	__typeof__(size) __pu_size = (size);			\
 								\
 	might_fault();						\
-	__chk_user_ptr(__pu_addr);				\
 	__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err);	\
 								\
 	__pu_err;						\
@@ -197,7 +196,6 @@ extern long __get_user_bad(void);
 #define __get_user_size_allowed(x, ptr, size, retval)		\
 do {								\
 	retval = 0;						\
-	__chk_user_ptr(ptr);					\
 	if (size > sizeof(x))					\
 		(x) = __get_user_bad();				\
 	switch (size) {						\
@@ -230,7 +228,6 @@ do {								\
 	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
 	__typeof__(size) __gu_size = (size);			\
 								\
-	__chk_user_ptr(__gu_addr);				\
 	if (do_allow) {								\
 		might_fault();					\
 		__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err);	\
-- 
2.25.0

[PATCH v1 09/15] powerpc/uaccess: Remove calls to __get_user_bad() and __put_user_bad()

[Christophe Leroy]

__get_user_bad() and __put_user_bad() are functions that are
declared but not defined, in order to make the build fails in
case they are called.

Nowadays, we have BUILD_BUG() and BUILD_BUG_ON() for that.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index a9f2639ca3a8..a8c683695ec7 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -53,8 +53,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
 #define __put_user(x, ptr) \
 	__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
 
-extern long __put_user_bad(void);
-
 #define __put_user_size(x, ptr, size, retval)			\
 do {								\
 	__label__ __pu_failed;					\
@@ -136,12 +134,10 @@ do {								\
 	case 2: __put_user_asm_goto(x, __pus_addr, label, "sth"); break;	\
 	case 4: __put_user_asm_goto(x, __pus_addr, label, "stw"); break;	\
 	case 8: __put_user_asm2_goto(x, __pus_addr, label); break;		\
-	default: __put_user_bad();				\
+	default: BUILD_BUG();					\
 	}							\
 } while (0)
 
-extern long __get_user_bad(void);
-
 /*
  * This does an atomic 128 byte aligned load from userspace.
  * Upto caller to do enable_kernel_vmx() before calling!
@@ -196,14 +192,13 @@ extern long __get_user_bad(void);
 #define __get_user_size_allowed(x, ptr, size, retval)		\
 do {								\
 	retval = 0;						\
-	if (size > sizeof(x))					\
-		(x) = __get_user_bad();				\
+	BUILD_BUG_ON(size > sizeof(x));				\
 	switch (size) {						\
 	case 1: __get_user_asm(x, (u8 __user *)ptr, retval, "lbz"); break;	\
 	case 2: __get_user_asm(x, (u16 __user *)ptr, retval, "lhz"); break;	\
 	case 4: __get_user_asm(x, (u32 __user *)ptr, retval, "lwz"); break;	\
 	case 8: __get_user_asm2(x, (u64 __user *)ptr, retval);  break;	\
-	default: (x) = __get_user_bad();			\
+	default: BUILD_BUG();					\
 	}							\
 } while (0)
 
-- 
2.25.0

[PATCH v1 10/15] powerpc/uaccess: Split out __get_user_nocheck()

[Christophe Leroy]

One part of __get_user_nocheck() is used for __get_user(),
the other part for unsafe_get_user().

Move the part dedicated to unsafe_get_user() in it.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index a8c683695ec7..678651a615c3 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -49,7 +49,7 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
 	__put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
 
 #define __get_user(x, ptr) \
-	__get_user_nocheck((x), (ptr), sizeof(*(ptr)), true)
+	__get_user_nocheck((x), (ptr), sizeof(*(ptr)))
 #define __put_user(x, ptr) \
 	__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
 
@@ -216,19 +216,15 @@ do {								\
 #define __long_type(x) \
 	__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 
-#define __get_user_nocheck(x, ptr, size, do_allow)			\
+#define __get_user_nocheck(x, ptr, size)			\
 ({								\
 	long __gu_err;						\
 	__long_type(*(ptr)) __gu_val;				\
 	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
 	__typeof__(size) __gu_size = (size);			\
 								\
-	if (do_allow) {								\
-		might_fault();					\
-		__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err);	\
-	} else {									\
-		__get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
-	}									\
+	might_fault();					\
+	__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err);	\
 	(x) = (__typeof__(*(ptr)))__gu_val;			\
 								\
 	__gu_err;						\
@@ -386,8 +382,14 @@ user_write_access_begin(const void __user *ptr, size_t len)
 #define user_write_access_end		prevent_current_write_to_user
 
 #define unsafe_get_user(x, p, e) do {					\
-	if (unlikely(__get_user_nocheck((x), (p), sizeof(*(p)), false)))\
-		goto e;							\
+	long __gu_err;						\
+	__long_type(*(p)) __gu_val;				\
+	__typeof__(*(p)) __user *__gu_addr = (p);		\
+								\
+	__get_user_size_allowed(__gu_val, __gu_addr, sizeof(*(p)), __gu_err); \
+	if (__gu_err)						\
+		goto e;						\
+	(x) = (__typeof__(*(p)))__gu_val;			\
 } while (0)
 
 #define unsafe_put_user(x, p, e) \
-- 
2.25.0

[PATCH v1 11/15] powerpc/uaccess: Rename __get/put_user_check/nocheck

[Christophe Leroy]

__get_user_check() becomes get_user()
__put_user_check() becomes put_user()
__get_user_nocheck() becomes __get_user()
__put_user_nocheck() becomes __put_user()

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 678651a615c3..616a3a7928c2 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -43,16 +43,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
  * exception handling means that it's no longer "just"...)
  *
  */
-#define get_user(x, ptr) \
-	__get_user_check((x), (ptr), sizeof(*(ptr)))
-#define put_user(x, ptr) \
-	__put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
-
-#define __get_user(x, ptr) \
-	__get_user_nocheck((x), (ptr), sizeof(*(ptr)))
-#define __put_user(x, ptr) \
-	__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
-
 #define __put_user_size(x, ptr, size, retval)			\
 do {								\
 	__label__ __pu_failed;					\
@@ -68,12 +58,12 @@ __pu_failed:							\
 	prevent_write_to_user(ptr, size);			\
 } while (0)
 
-#define __put_user_nocheck(x, ptr, size)			\
+#define __put_user(x, ptr)					\
 ({								\
 	long __pu_err;						\
 	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
-	__typeof__(*(ptr)) __pu_val = (x);			\
-	__typeof__(size) __pu_size = (size);			\
+	__typeof__(*(ptr)) __pu_val = (__typeof__(*(ptr)))(x);	\
+	__typeof__(sizeof(*(ptr))) __pu_size = sizeof(*(ptr));	\
 								\
 	might_fault();						\
 	__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err);	\
@@ -81,12 +71,12 @@ __pu_failed:							\
 	__pu_err;						\
 })
 
-#define __put_user_check(x, ptr, size)					\
+#define put_user(x, ptr)						\
 ({									\
 	long __pu_err = -EFAULT;					\
 	__typeof__(*(ptr)) __user *__pu_addr = (ptr);			\
-	__typeof__(*(ptr)) __pu_val = (x);				\
-	__typeof__(size) __pu_size = (size);				\
+	__typeof__(*(ptr)) __pu_val = (__typeof__(*(ptr)))(x);		\
+	__typeof__(sizeof(*(ptr))) __pu_size = sizeof(*(ptr));		\
 									\
 	might_fault();							\
 	if (access_ok(__pu_addr, __pu_size))				\
@@ -216,12 +206,12 @@ do {								\
 #define __long_type(x) \
 	__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 
-#define __get_user_nocheck(x, ptr, size)			\
+#define __get_user(x, ptr)					\
 ({								\
 	long __gu_err;						\
 	__long_type(*(ptr)) __gu_val;				\
 	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
-	__typeof__(size) __gu_size = (size);			\
+	__typeof__(sizeof(*(ptr))) __gu_size = sizeof(*(ptr));	\
 								\
 	might_fault();					\
 	__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err);	\
@@ -230,12 +220,12 @@ do {								\
 	__gu_err;						\
 })
 
-#define __get_user_check(x, ptr, size)					\
+#define get_user(x, ptr)						\
 ({									\
 	long __gu_err = -EFAULT;					\
 	__long_type(*(ptr)) __gu_val = 0;				\
 	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
-	__typeof__(size) __gu_size = (size);				\
+	__typeof__(sizeof(*(ptr))) __gu_size = sizeof(*(ptr));		\
 									\
 	might_fault();							\
 	if (access_ok(__gu_addr, __gu_size))				\
-- 
2.25.0

[PATCH v1 12/15] powerpc/uaccess: Refactor get/put_user() and __get/put_user()

[Christophe Leroy]

Make get_user() do the access_ok() check then call __get_user().
Make put_user() do the access_ok() check then call __put_user().

Then embed  __get_user_size() and __put_user_size() in
__get_user() and __put_user().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 66 +++++++++++-------------------
 1 file changed, 23 insertions(+), 43 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 616a3a7928c2..671c083f2f2f 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -43,21 +43,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
  * exception handling means that it's no longer "just"...)
  *
  */
-#define __put_user_size(x, ptr, size, retval)			\
-do {								\
-	__label__ __pu_failed;					\
-								\
-	retval = 0;						\
-	allow_write_to_user(ptr, size);				\
-	__put_user_size_goto(x, ptr, size, __pu_failed);	\
-	prevent_write_to_user(ptr, size);			\
-	break;							\
-								\
-__pu_failed:							\
-	retval = -EFAULT;					\
-	prevent_write_to_user(ptr, size);			\
-} while (0)
-
 #define __put_user(x, ptr)					\
 ({								\
 	long __pu_err;						\
@@ -66,23 +51,29 @@ __pu_failed:							\
 	__typeof__(sizeof(*(ptr))) __pu_size = sizeof(*(ptr));	\
 								\
 	might_fault();						\
-	__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err);	\
+	do {							\
+		__label__ __pu_failed;				\
+								\
+		allow_write_to_user(__pu_addr, __pu_size);	\
+		__put_user_size_goto(__pu_val, __pu_addr, __pu_size, __pu_failed);	\
+		prevent_write_to_user(__pu_addr, __pu_size);	\
+		__pu_err = 0;					\
+		break;						\
+								\
+__pu_failed:							\
+		prevent_write_to_user(__pu_addr, __pu_size);	\
+		__pu_err = -EFAULT;				\
+	} while (0);						\
 								\
 	__pu_err;						\
 })
 
 #define put_user(x, ptr)						\
 ({									\
-	long __pu_err = -EFAULT;					\
-	__typeof__(*(ptr)) __user *__pu_addr = (ptr);			\
-	__typeof__(*(ptr)) __pu_val = (__typeof__(*(ptr)))(x);		\
-	__typeof__(sizeof(*(ptr))) __pu_size = sizeof(*(ptr));		\
+	__typeof__(*(ptr)) __user *_pu_addr = (ptr);			\
 									\
-	might_fault();							\
-	if (access_ok(__pu_addr, __pu_size))				\
-		__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
-									\
-	__pu_err;							\
+	access_ok(_pu_addr, sizeof(*(ptr))) ?				\
+		  __put_user(x, _pu_addr) : -EFAULT;			\
 })
 
 /*
@@ -192,13 +183,6 @@ do {								\
 	}							\
 } while (0)
 
-#define __get_user_size(x, ptr, size, retval)			\
-do {								\
-	allow_read_from_user(ptr, size);			\
-	__get_user_size_allowed(x, ptr, size, retval);		\
-	prevent_read_from_user(ptr, size);			\
-} while (0)
-
 /*
  * This is a type: either unsigned long, if the argument fits into
  * that type, or otherwise unsigned long long.
@@ -214,7 +198,9 @@ do {								\
 	__typeof__(sizeof(*(ptr))) __gu_size = sizeof(*(ptr));	\
 								\
 	might_fault();					\
-	__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err);	\
+	allow_read_from_user(__gu_addr, __gu_size);		\
+	__get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err);	\
+	prevent_read_from_user(__gu_addr, __gu_size);		\
 	(x) = (__typeof__(*(ptr)))__gu_val;			\
 								\
 	__gu_err;						\
@@ -222,17 +208,11 @@ do {								\
 
 #define get_user(x, ptr)						\
 ({									\
-	long __gu_err = -EFAULT;					\
-	__long_type(*(ptr)) __gu_val = 0;				\
-	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
-	__typeof__(sizeof(*(ptr))) __gu_size = sizeof(*(ptr));		\
-									\
-	might_fault();							\
-	if (access_ok(__gu_addr, __gu_size))				\
-		__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
-	(x) = (__force __typeof__(*(ptr)))__gu_val;				\
+	__typeof__(*(ptr)) __user *_gu_addr = (ptr);			\
 									\
-	__gu_err;							\
+	access_ok(_gu_addr, sizeof(*(ptr))) ?				\
+		  __get_user(x, _gu_addr) :				\
+		  ((x) = (__force __typeof__(*(ptr)))0, -EFAULT);	\
 })
 
 /* more complex routines */
-- 
2.25.0

[PATCH v1 13/15] powerpc/uaccess: Swap clear_user() and __clear_user()

[Christophe Leroy]

It is clear_user() which is expected to call __clear_user(),
not the reverse.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 671c083f2f2f..b3bd1fb42242 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -283,21 +283,20 @@ raw_copy_to_user(void __user *to, const void *from, unsigned long n)
 
 unsigned long __arch_clear_user(void __user *addr, unsigned long size);
 
-static inline unsigned long clear_user(void __user *addr, unsigned long size)
+static inline unsigned long __clear_user(void __user *addr, unsigned long size)
 {
-	unsigned long ret = size;
+	unsigned long ret;
+
 	might_fault();
-	if (likely(access_ok(addr, size))) {
-		allow_write_to_user(addr, size);
-		ret = __arch_clear_user(addr, size);
-		prevent_write_to_user(addr, size);
-	}
+	allow_write_to_user(addr, size);
+	ret = __arch_clear_user(addr, size);
+	prevent_write_to_user(addr, size);
 	return ret;
 }
 
-static inline unsigned long __clear_user(void __user *addr, unsigned long size)
+static inline unsigned long clear_user(void __user *addr, unsigned long size)
 {
-	return clear_user(addr, size);
+	return likely(access_ok(addr, size)) ? __clear_user(addr, size) : size;
 }
 
 extern long strncpy_from_user(char *dst, const char __user *src, long count);
-- 
2.25.0

[PATCH v1 14/15] powerpc/uaccess: Also perform 64 bits copies in unsafe_copy_to_user() on ppc32

[Christophe Leroy]

ppc32 has an efficiant 64 bits __put_user(), so also use it in
order to unroll loops more.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index b3bd1fb42242..e831653db51a 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -371,9 +371,9 @@ do {									\
 	size_t _len = (l);						\
 	int _i;								\
 									\
-	for (_i = 0; _i < (_len & ~(sizeof(long) - 1)); _i += sizeof(long))		\
-		unsafe_put_user(*(long*)(_src + _i), (long __user *)(_dst + _i), e); \
-	if (IS_ENABLED(CONFIG_PPC64) && (_len & 4)) {			\
+	for (_i = 0; _i < (_len & ~(sizeof(u64) - 1)); _i += sizeof(u64))	\
+		unsafe_put_user(*(u64 *)(_src + _i), (u64 __user *)(_dst + _i), e); \
+	if (_len & 4) {							\
 		unsafe_put_user(*(u32*)(_src + _i), (u32 __user *)(_dst + _i), e); \
 		_i += 4;						\
 	}								\
-- 
2.25.0

[PATCH v1 15/15] powerpc/uaccess: Move copy_mc_xxx() functions down

[Christophe Leroy]

copy_mc_xxx() functions are in the middle of raw_copy functions.

For clarity, move them out of the raw_copy functions block.

They are using access_ok, so they need to be after the general
functions in order to eventually allow the inclusion of
asm-generic/uaccess.h in some future.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 52 +++++++++++++++---------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index e831653db51a..f1f9237ed857 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -220,32 +220,6 @@ do {								\
 extern unsigned long __copy_tofrom_user(void __user *to,
 		const void __user *from, unsigned long size);
 
-#ifdef CONFIG_ARCH_HAS_COPY_MC
-unsigned long __must_check
-copy_mc_generic(void *to, const void *from, unsigned long size);
-
-static inline unsigned long __must_check
-copy_mc_to_kernel(void *to, const void *from, unsigned long size)
-{
-	return copy_mc_generic(to, from, size);
-}
-#define copy_mc_to_kernel copy_mc_to_kernel
-
-static inline unsigned long __must_check
-copy_mc_to_user(void __user *to, const void *from, unsigned long n)
-{
-	if (likely(check_copy_size(from, n, true))) {
-		if (access_ok(to, n)) {
-			allow_write_to_user(to, n);
-			n = copy_mc_generic((void *)to, from, n);
-			prevent_write_to_user(to, n);
-		}
-	}
-
-	return n;
-}
-#endif
-
 #ifdef __powerpc64__
 static inline unsigned long
 raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
@@ -302,6 +276,32 @@ static inline unsigned long clear_user(void __user *addr, unsigned long size)
 extern long strncpy_from_user(char *dst, const char __user *src, long count);
 extern __must_check long strnlen_user(const char __user *str, long n);
 
+#ifdef CONFIG_ARCH_HAS_COPY_MC
+unsigned long __must_check
+copy_mc_generic(void *to, const void *from, unsigned long size);
+
+static inline unsigned long __must_check
+copy_mc_to_kernel(void *to, const void *from, unsigned long size)
+{
+	return copy_mc_generic(to, from, size);
+}
+#define copy_mc_to_kernel copy_mc_to_kernel
+
+static inline unsigned long __must_check
+copy_mc_to_user(void __user *to, const void *from, unsigned long n)
+{
+	if (likely(check_copy_size(from, n, true))) {
+		if (access_ok(to, n)) {
+			allow_write_to_user(to, n);
+			n = copy_mc_generic((void *)to, from, n);
+			prevent_write_to_user(to, n);
+		}
+	}
+
+	return n;
+}
+#endif
+
 extern long __copy_from_user_flushcache(void *dst, const void __user *src,
 		unsigned size);
 extern void memcpy_page_flushcache(char *to, struct page *page, size_t offset,
-- 
2.25.0

Re: [PATCH v1 01/15] powerpc/uaccess: Remove __get_user_allowed() and unsafe_op_wrap()

[Daniel Axtens]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:

> Those two macros have only one user which is unsafe_get_user().
>
> Put everything in one place and remove them.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/include/asm/uaccess.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 78e2a3990eab..8cbf3e3874f1 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -53,9 +53,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
>  #define __put_user(x, ptr) \
>  	__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>  
> -#define __get_user_allowed(x, ptr) \
> -	__get_user_nocheck((x), (ptr), sizeof(*(ptr)), false)
> -
>  #define __get_user_inatomic(x, ptr) \
>  	__get_user_nosleep((x), (ptr), sizeof(*(ptr)))
>  #define __put_user_inatomic(x, ptr) \
> @@ -482,8 +479,11 @@ user_write_access_begin(const void __user *ptr, size_t len)
>  #define user_write_access_begin	user_write_access_begin
>  #define user_write_access_end		prevent_current_write_to_user
>  
> -#define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0)
> -#define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
> +#define unsafe_get_user(x, p, e) do {					\
> +	if (unlikely(__get_user_nocheck((x), (p), sizeof(*(p)), false)))\
> +		goto e;							\
> +} while (0)
> +

This seems correct to me.

Checkpatch does have one check that is relevant:

CHECK: Macro argument reuse 'p' - possible side-effects?
#36: FILE: arch/powerpc/include/asm/uaccess.h:482:
+#define unsafe_get_user(x, p, e) do {					\
+	if (unlikely(__get_user_nocheck((x), (p), sizeof(*(p)), false)))\
+		goto e;							\
+} while (0)

Given that we are already creating a new block, should we do something
like this (completely untested):

#define unsafe_get_user(x, p, e) do {					\
        __typeof__(p) __p = (p);
	if (unlikely(__get_user_nocheck((x), (__p), sizeof(*(__p)), false)))\
		goto e;							\
} while (0)

Kind regards,
Daniel

>  #define unsafe_put_user(x, p, e) \
>  	__unsafe_put_user_goto((__typeof__(*(p)))(x), (p), sizeof(*(p)), e)
>  
> -- 
> 2.25.0

Re: [PATCH v1 02/15] powerpc/uaccess: Define ___get_user_instr() for ppc32

[Daniel Axtens]

Hi Christophe,

> +#else /* !CONFIG_PPC64 */
> +#define ___get_user_instr(gu_op, dest, ptr)				\
> +	gu_op((dest).val, (u32 __user *)(ptr))
> +#endif /* CONFIG_PPC64 */
>  
>  #define get_user_instr(x, ptr) \
>  	___get_user_instr(get_user, x, ptr)
> @@ -91,18 +95,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
>  #define __get_user_instr_inatomic(x, ptr) \
>  	___get_user_instr(__get_user_inatomic, x, ptr)
>  
> -#else /* !CONFIG_PPC64 */
> -#define get_user_instr(x, ptr) \
> -	get_user((x).val, (u32 __user *)(ptr))
> -
> -#define __get_user_instr(x, ptr) \
> -	__get_user_nocheck((x).val, (u32 __user *)(ptr), sizeof(u32), true)
> -
> -#define __get_user_instr_inatomic(x, ptr) \
> -	__get_user_nosleep((x).val, (u32 __user *)(ptr), sizeof(u32))
> -
> -#endif /* CONFIG_PPC64 */

The previous version of __get_user_instr called __get_user_nocheck,
this version calls __get_user. Likewise __get_user_instr_inatomic called
__get_user_nosleep and now it calls __get_user_inatomic. I was confused
by this until I chased the macro definitions and realised that both
names refer to the same thing:

#define __get_user(x, ptr) \
	__get_user_nocheck((x), (ptr), sizeof(*(ptr)), true)

#define __get_user_inatomic(x, ptr) \
	__get_user_nosleep((x), (ptr), sizeof(*(ptr)))

(I don't think you need to do anything here, I'm just documenting what I
considered while reviewing your patch.)

As such:
Reviewed-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel


> -
>  extern long __put_user_bad(void);
>  
>  #define __put_user_size(x, ptr, size, retval)			\
> -- 
> 2.25.0

Re: [PATCH v1 01/15] powerpc/uaccess: Remove __get_user_allowed() and unsafe_op_wrap()

[Segher Boessenkool]

On Tue, Mar 02, 2021 at 09:02:54AM +1100, Daniel Axtens wrote:
> Checkpatch does have one check that is relevant:
> 
> CHECK: Macro argument reuse 'p' - possible side-effects?
> #36: FILE: arch/powerpc/include/asm/uaccess.h:482:
> +#define unsafe_get_user(x, p, e) do {					\
> +	if (unlikely(__get_user_nocheck((x), (p), sizeof(*(p)), false)))\
> +		goto e;							\
> +} while (0)

sizeof (of something other than a VLA) does not evaluate its operand.
The checkpatch warning is incorrect (well, it does say "possible" --
it just didn't find a possible problem here).

You can write
  bla = sizeof *p++;
and p is *not* incremented.


Segher

Re: [PATCH v1 03/15] powerpc/uaccess: Remove __get/put_user_inatomic()

[Daniel Axtens]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:

> Since commit 662bbcb2747c ("mm, sched: Allow uaccess in atomic with
> pagefault_disable()"), __get/put_user() can be used in atomic parts
> of the code, therefore the __get/put_user_inatomic() introduced
> by commit e68c825bb016 ("[POWERPC] Add inatomic versions of __get_user
> and __put_user") have become useless.

I spent some time chasing these macro definitions.

Let me see if I understand you.

__get_user(x, ptr) becomes __get_user_nocheck(..., true)
__get_user_inatomic() become __get_user_nosleep()

The difference between how __get_user_nosleep() and
__get_user_nocheck(..., true) operate is that __get_user_nocheck calls
might_fault() and __get_user_nosleep() does not.

If I understand the commit you reference and mm/memory.c, you're saying
that we can indeed call might_fault() when page faults are disabled,
because __might_fault() checks if page faults are disabled and does not
fire a warning if it is called with page faults disabled.

Therefore, it is safe to remove our _inatomic version that does not call
might_fault and just to call might_fault unconditionally.

Is that right?

I haven't checked changes you made to the various .c files in fine
detail but they appear to be entirely mechanical.

> powerpc is the only one having such functions. There is a real
> intention not to have to provide such _inatomic() helpers, see the
> comment in might_fault() in mm/memory.c introduced by
> commit 3ee1afa308f2 ("x86: some lock annotations for user
> copy paths, v2"):
>
> 	/*
> 	 * it would be nicer only to annotate paths which are not under
> 	 * pagefault_disable, however that requires a larger audit and
> 	 * providing helpers like get_user_atomic.
> 	 */
>

I'm not fully sure I understand what you're saying in this part of the
commit message.

Kind regards,
Daniel

>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/include/asm/uaccess.h            | 37 -------------------
>  arch/powerpc/kernel/align.c                   | 32 ++++++++--------
>  .../kernel/hw_breakpoint_constraints.c        |  2 +-
>  arch/powerpc/kernel/traps.c                   |  2 +-
>  4 files changed, 18 insertions(+), 55 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index a08c482b1315..01aea0df4dd0 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -53,11 +53,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
>  #define __put_user(x, ptr) \
>  	__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>  
> -#define __get_user_inatomic(x, ptr) \
> -	__get_user_nosleep((x), (ptr), sizeof(*(ptr)))
> -#define __put_user_inatomic(x, ptr) \
> -	__put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
> -
>  #ifdef CONFIG_PPC64
>  
>  #define ___get_user_instr(gu_op, dest, ptr)				\
> @@ -92,9 +87,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
>  #define __get_user_instr(x, ptr) \
>  	___get_user_instr(__get_user, x, ptr)
>  
> -#define __get_user_instr_inatomic(x, ptr) \
> -	___get_user_instr(__get_user_inatomic, x, ptr)
> -
>  extern long __put_user_bad(void);
>  
>  #define __put_user_size(x, ptr, size, retval)			\
> @@ -141,20 +133,6 @@ __pu_failed:							\
>  	__pu_err;							\
>  })
>  
> -#define __put_user_nosleep(x, ptr, size)			\
> -({								\
> -	long __pu_err;						\
> -	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
> -	__typeof__(*(ptr)) __pu_val = (x);			\
> -	__typeof__(size) __pu_size = (size);			\
> -								\
> -	__chk_user_ptr(__pu_addr);				\
> -	__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
> -								\
> -	__pu_err;						\
> -})
> -
> -
>  /*
>   * We don't tell gcc that we are accessing memory, but this is OK
>   * because we do not write to any memory gcc knows about, so there
> @@ -320,21 +298,6 @@ do {								\
>  	__gu_err;							\
>  })
>  
> -#define __get_user_nosleep(x, ptr, size)			\
> -({								\
> -	long __gu_err;						\
> -	__long_type(*(ptr)) __gu_val;				\
> -	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
> -	__typeof__(size) __gu_size = (size);			\
> -								\
> -	__chk_user_ptr(__gu_addr);				\
> -	__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
> -	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
> -								\
> -	__gu_err;						\
> -})
> -
> -
>  /* more complex routines */
>  
>  extern unsigned long __copy_tofrom_user(void __user *to,
> diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
> index c7797eb958c7..83b199026a1e 100644
> --- a/arch/powerpc/kernel/align.c
> +++ b/arch/powerpc/kernel/align.c
> @@ -174,18 +174,18 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
>  
>  		switch (nb) {
>  		case 8:
> -			ret |= __get_user_inatomic(temp.v[0], p++);
> -			ret |= __get_user_inatomic(temp.v[1], p++);
> -			ret |= __get_user_inatomic(temp.v[2], p++);
> -			ret |= __get_user_inatomic(temp.v[3], p++);
> +			ret |= __get_user(temp.v[0], p++);
> +			ret |= __get_user(temp.v[1], p++);
> +			ret |= __get_user(temp.v[2], p++);
> +			ret |= __get_user(temp.v[3], p++);
>  			fallthrough;
>  		case 4:
> -			ret |= __get_user_inatomic(temp.v[4], p++);
> -			ret |= __get_user_inatomic(temp.v[5], p++);
> +			ret |= __get_user(temp.v[4], p++);
> +			ret |= __get_user(temp.v[5], p++);
>  			fallthrough;
>  		case 2:
> -			ret |= __get_user_inatomic(temp.v[6], p++);
> -			ret |= __get_user_inatomic(temp.v[7], p++);
> +			ret |= __get_user(temp.v[6], p++);
> +			ret |= __get_user(temp.v[7], p++);
>  			if (unlikely(ret))
>  				return -EFAULT;
>  		}
> @@ -259,18 +259,18 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
>  		p = addr;
>  		switch (nb) {
>  		case 8:
> -			ret |= __put_user_inatomic(data.v[0], p++);
> -			ret |= __put_user_inatomic(data.v[1], p++);
> -			ret |= __put_user_inatomic(data.v[2], p++);
> -			ret |= __put_user_inatomic(data.v[3], p++);
> +			ret |= __put_user(data.v[0], p++);
> +			ret |= __put_user(data.v[1], p++);
> +			ret |= __put_user(data.v[2], p++);
> +			ret |= __put_user(data.v[3], p++);
>  			fallthrough;
>  		case 4:
> -			ret |= __put_user_inatomic(data.v[4], p++);
> -			ret |= __put_user_inatomic(data.v[5], p++);
> +			ret |= __put_user(data.v[4], p++);
> +			ret |= __put_user(data.v[5], p++);
>  			fallthrough;
>  		case 2:
> -			ret |= __put_user_inatomic(data.v[6], p++);
> -			ret |= __put_user_inatomic(data.v[7], p++);
> +			ret |= __put_user(data.v[6], p++);
> +			ret |= __put_user(data.v[7], p++);
>  		}
>  		if (unlikely(ret))
>  			return -EFAULT;
> diff --git a/arch/powerpc/kernel/hw_breakpoint_constraints.c b/arch/powerpc/kernel/hw_breakpoint_constraints.c
> index 867ee4aa026a..675d1f66ab72 100644
> --- a/arch/powerpc/kernel/hw_breakpoint_constraints.c
> +++ b/arch/powerpc/kernel/hw_breakpoint_constraints.c
> @@ -141,7 +141,7 @@ void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
>  {
>  	struct instruction_op op;
>  
> -	if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip))
> +	if (__get_user_instr(*instr, (void __user *)regs->nip))
>  		return;
>  
>  	analyse_instr(&op, regs, *instr);
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 1583fd1c6010..1fa36bd08efe 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -864,7 +864,7 @@ static void p9_hmi_special_emu(struct pt_regs *regs)
>  	unsigned long ea, msr, msr_mask;
>  	bool swap;
>  
> -	if (__get_user_inatomic(instr, (unsigned int __user *)regs->nip))
> +	if (__get_user(instr, (unsigned int __user *)regs->nip))
>  		return;
>  
>  	/*
> -- 
> 2.25.0

Re: [PATCH v1 12/15] powerpc/uaccess: Refactor get/put_user() and __get/put_user()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5429 bytes --]

Hi Christophe,

I love your patch! Perhaps something to improve:

[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.12-rc2 next-20210305]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-Cleanup-of-uaccess-h/20210226-015715
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-s031-20210307 (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-245-gacc5c298-dirty
        # https://github.com/0day-ci/linux/commit/449bdbf978936e67e4919be8be0eec3e490a65e2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christophe-Leroy/powerpc-Cleanup-of-uaccess-h/20210226-015715
        git checkout 449bdbf978936e67e4919be8be0eec3e490a65e2
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected char [noderef] __user *_pu_addr @@     got char *buf @@
   drivers/w1/slaves/w1_ds28e04.c:342:13: sparse:     expected char [noderef] __user *_pu_addr
   drivers/w1/slaves/w1_ds28e04.c:342:13: sparse:     got char *buf
>> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected char const [noderef] __user *_gu_addr @@     got char const *buf @@
   drivers/w1/slaves/w1_ds28e04.c:356:13: sparse:     expected char const [noderef] __user *_gu_addr
   drivers/w1/slaves/w1_ds28e04.c:356:13: sparse:     got char const *buf
--
   drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: sparse: cast removes address space '__user' of expression
   drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: sparse: cast removes address space '__user' of expression
>> drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected unsigned int [noderef] __user *_pu_addr @@     got unsigned int [usertype] * @@
   drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse:     expected unsigned int [noderef] __user *_pu_addr
   drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse:     got unsigned int [usertype] *
   drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: sparse: cast removes address space '__user' of expression

vim +342 drivers/w1/slaves/w1_ds28e04.c

fa33a65a9cf7e2 Greg Kroah-Hartman 2013-08-21  338  
fa33a65a9cf7e2 Greg Kroah-Hartman 2013-08-21  339  static ssize_t crccheck_show(struct device *dev, struct device_attribute *attr,
fa33a65a9cf7e2 Greg Kroah-Hartman 2013-08-21  340  			     char *buf)
fbf7f7b4e2ae40 Markus Franke      2012-05-26  341  {
fbf7f7b4e2ae40 Markus Franke      2012-05-26 @342  	if (put_user(w1_enable_crccheck + 0x30, buf))
fbf7f7b4e2ae40 Markus Franke      2012-05-26  343  		return -EFAULT;
fbf7f7b4e2ae40 Markus Franke      2012-05-26  344  
fbf7f7b4e2ae40 Markus Franke      2012-05-26  345  	return sizeof(w1_enable_crccheck);
fbf7f7b4e2ae40 Markus Franke      2012-05-26  346  }
fbf7f7b4e2ae40 Markus Franke      2012-05-26  347  
fa33a65a9cf7e2 Greg Kroah-Hartman 2013-08-21  348  static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
fbf7f7b4e2ae40 Markus Franke      2012-05-26  349  			      const char *buf, size_t count)
fbf7f7b4e2ae40 Markus Franke      2012-05-26  350  {
fbf7f7b4e2ae40 Markus Franke      2012-05-26  351  	char val;
fbf7f7b4e2ae40 Markus Franke      2012-05-26  352  
fbf7f7b4e2ae40 Markus Franke      2012-05-26  353  	if (count != 1 || !buf)
fbf7f7b4e2ae40 Markus Franke      2012-05-26  354  		return -EINVAL;
fbf7f7b4e2ae40 Markus Franke      2012-05-26  355  
fbf7f7b4e2ae40 Markus Franke      2012-05-26 @356  	if (get_user(val, buf))
fbf7f7b4e2ae40 Markus Franke      2012-05-26  357  		return -EFAULT;
fbf7f7b4e2ae40 Markus Franke      2012-05-26  358  
fbf7f7b4e2ae40 Markus Franke      2012-05-26  359  	/* convert to decimal */
fbf7f7b4e2ae40 Markus Franke      2012-05-26  360  	val = val - 0x30;
fbf7f7b4e2ae40 Markus Franke      2012-05-26  361  	if (val != 0 && val != 1)
fbf7f7b4e2ae40 Markus Franke      2012-05-26  362  		return -EINVAL;
fbf7f7b4e2ae40 Markus Franke      2012-05-26  363  
fbf7f7b4e2ae40 Markus Franke      2012-05-26  364  	/* set the new value */
fbf7f7b4e2ae40 Markus Franke      2012-05-26  365  	w1_enable_crccheck = val;
fbf7f7b4e2ae40 Markus Franke      2012-05-26  366  
fbf7f7b4e2ae40 Markus Franke      2012-05-26  367  	return sizeof(w1_enable_crccheck);
fbf7f7b4e2ae40 Markus Franke      2012-05-26  368  }
fbf7f7b4e2ae40 Markus Franke      2012-05-26  369  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27591 bytes --]

Re: [PATCH v1 12/15] powerpc/uaccess: Refactor get/put_user() and __get/put_user()

[Christophe Leroy]

+Evgeniy for W1 Dallas
+Alex & Christian for RADEON

Le 07/03/2021 à 11:23, kernel test robot a écrit :
> Hi Christophe,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on powerpc/next]
> [also build test WARNING on v5.12-rc2 next-20210305]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-Cleanup-of-uaccess-h/20210226-015715
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> config: powerpc-randconfig-s031-20210307 (attached as .config)
> compiler: powerpc-linux-gcc (GCC) 9.3.0
> reproduce:
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # apt-get install sparse
>          # sparse version: v0.6.3-245-gacc5c298-dirty
>          # https://github.com/0day-ci/linux/commit/449bdbf978936e67e4919be8be0eec3e490a65e2
>          git remote add linux-review https://github.com/0day-ci/linux
>          git fetch --no-tags linux-review Christophe-Leroy/powerpc-Cleanup-of-uaccess-h/20210226-015715
>          git checkout 449bdbf978936e67e4919be8be0eec3e490a65e2
>          # save the attached .config to linux build tree
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=powerpc
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>


The mentioned patch is not the source of the problem, it only allows to spot it.

Christophe

> 
> 
> "sparse warnings: (new ones prefixed by >>)"
>>> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected char [noderef] __user *_pu_addr @@     got char *buf @@
>     drivers/w1/slaves/w1_ds28e04.c:342:13: sparse:     expected char [noderef] __user *_pu_addr
>     drivers/w1/slaves/w1_ds28e04.c:342:13: sparse:     got char *buf
>>> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected char const [noderef] __user *_gu_addr @@     got char const *buf @@
>     drivers/w1/slaves/w1_ds28e04.c:356:13: sparse:     expected char const [noderef] __user *_gu_addr
>     drivers/w1/slaves/w1_ds28e04.c:356:13: sparse:     got char const *buf
> --
>     drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: sparse: cast removes address space '__user' of expression
>     drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: sparse: cast removes address space '__user' of expression
>>> drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected unsigned int [noderef] __user *_pu_addr @@     got unsigned int [usertype] * @@
>     drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse:     expected unsigned int [noderef] __user *_pu_addr
>     drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse:     got unsigned int [usertype] *
>     drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: sparse: cast removes address space '__user' of expression
> 
> vim +342 drivers/w1/slaves/w1_ds28e04.c
> 
> fa33a65a9cf7e2 Greg Kroah-Hartman 2013-08-21  338
> fa33a65a9cf7e2 Greg Kroah-Hartman 2013-08-21  339  static ssize_t crccheck_show(struct device *dev, struct device_attribute *attr,
> fa33a65a9cf7e2 Greg Kroah-Hartman 2013-08-21  340  			     char *buf)
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  341  {
> fbf7f7b4e2ae40 Markus Franke      2012-05-26 @342  	if (put_user(w1_enable_crccheck + 0x30, buf))
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  343  		return -EFAULT;
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  344
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  345  	return sizeof(w1_enable_crccheck);
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  346  }
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  347
> fa33a65a9cf7e2 Greg Kroah-Hartman 2013-08-21  348  static ssize_t crccheck_store(struct device *dev, struct device_attribute *attr,
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  349  			      const char *buf, size_t count)
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  350  {
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  351  	char val;
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  352
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  353  	if (count != 1 || !buf)
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  354  		return -EINVAL;
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  355
> fbf7f7b4e2ae40 Markus Franke      2012-05-26 @356  	if (get_user(val, buf))
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  357  		return -EFAULT;
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  358
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  359  	/* convert to decimal */
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  360  	val = val - 0x30;
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  361  	if (val != 0 && val != 1)
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  362  		return -EINVAL;
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  363
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  364  	/* set the new value */
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  365  	w1_enable_crccheck = val;
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  366
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  367  	return sizeof(w1_enable_crccheck);
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  368  }
> fbf7f7b4e2ae40 Markus Franke      2012-05-26  369
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 

Re: [PATCH v1 12/15] powerpc/uaccess: Refactor get/put_user() and __get/put_user()

[Christian König]

The radeon warning is trivial to fix, going to send out a patch in a few 
moments.

Regards,
Christian.

Am 08.03.21 um 13:14 schrieb Christophe Leroy:
> +Evgeniy for W1 Dallas
> +Alex & Christian for RADEON
>
> Le 07/03/2021 à 11:23, kernel test robot a écrit :
>> Hi Christophe,
>>
>> I love your patch! Perhaps something to improve:
>>
>> [auto build test WARNING on powerpc/next]
>> [also build test WARNING on v5.12-rc2 next-20210305]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url: 
>> https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-Cleanup-of-uaccess-h/20210226-015715
>> base: 
>> https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
>> config: powerpc-randconfig-s031-20210307 (attached as .config)
>> compiler: powerpc-linux-gcc (GCC) 9.3.0
>> reproduce:
>>          wget 
>> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross 
>> -O ~/bin/make.cross
>>          chmod +x ~/bin/make.cross
>>          # apt-get install sparse
>>          # sparse version: v0.6.3-245-gacc5c298-dirty
>>          # 
>> https://github.com/0day-ci/linux/commit/449bdbf978936e67e4919be8be0eec3e490a65e2
>>          git remote add linux-review https://github.com/0day-ci/linux
>>          git fetch --no-tags linux-review 
>> Christophe-Leroy/powerpc-Cleanup-of-uaccess-h/20210226-015715
>>          git checkout 449bdbf978936e67e4919be8be0eec3e490a65e2
>>          # save the attached .config to linux build tree
>>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 
>> make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=powerpc
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>
>
> The mentioned patch is not the source of the problem, it only allows 
> to spot it.
>
> Christophe
>
>>
>>
>> "sparse warnings: (new ones prefixed by >>)"
>>>> drivers/w1/slaves/w1_ds28e04.c:342:13: sparse: sparse: incorrect 
>>>> type in initializer (different address spaces) @@     expected char 
>>>> [noderef] __user *_pu_addr @@     got char *buf @@
>>     drivers/w1/slaves/w1_ds28e04.c:342:13: sparse:     expected char 
>> [noderef] __user *_pu_addr
>>     drivers/w1/slaves/w1_ds28e04.c:342:13: sparse:     got char *buf
>>>> drivers/w1/slaves/w1_ds28e04.c:356:13: sparse: sparse: incorrect 
>>>> type in initializer (different address spaces) @@     expected char 
>>>> const [noderef] __user *_gu_addr @@     got char const *buf @@
>>     drivers/w1/slaves/w1_ds28e04.c:356:13: sparse:     expected char 
>> const [noderef] __user *_gu_addr
>>     drivers/w1/slaves/w1_ds28e04.c:356:13: sparse:     got char const 
>> *buf
>> -- 
>>     drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: sparse: cast 
>> removes address space '__user' of expression
>>     drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: sparse: cast 
>> removes address space '__user' of expression
>>>> drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: sparse: 
>>>> incorrect type in initializer (different address spaces) @@     
>>>> expected unsigned int [noderef] __user *_pu_addr @@     got 
>>>> unsigned int [usertype] * @@
>>     drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: expected 
>> unsigned int [noderef] __user *_pu_addr
>>     drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse:     got 
>> unsigned int [usertype] *
>>     drivers/gpu/drm/radeon/radeon_ttm.c:933:21: sparse: sparse: cast 
>> removes address space '__user' of expression
>>
>> vim +342 drivers/w1/slaves/w1_ds28e04.c
>>
>> fa33a65a9cf7e2 Greg Kroah-Hartman 2013-08-21  338
>> fa33a65a9cf7e2 Greg Kroah-Hartman 2013-08-21  339  static ssize_t 
>> crccheck_show(struct device *dev, struct device_attribute *attr,
>> fa33a65a9cf7e2 Greg Kroah-Hartman 2013-08-21 340                   
>> char *buf)
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  341  {
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26 @342      if 
>> (put_user(w1_enable_crccheck + 0x30, buf))
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  343 return -EFAULT;
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  344
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  345      return 
>> sizeof(w1_enable_crccheck);
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  346  }
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  347
>> fa33a65a9cf7e2 Greg Kroah-Hartman 2013-08-21  348  static ssize_t 
>> crccheck_store(struct device *dev, struct device_attribute *attr,
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26 349                    
>> const char *buf, size_t count)
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  350  {
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  351      char val;
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  352
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  353      if (count != 1 
>> || !buf)
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  354 return -EINVAL;
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  355
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26 @356      if 
>> (get_user(val, buf))
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  357 return -EFAULT;
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  358
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  359      /* convert to 
>> decimal */
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  360      val = val - 0x30;
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  361      if (val != 0 
>> && val != 1)
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  362 return -EINVAL;
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  363
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  364      /* set the new 
>> value */
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  365 w1_enable_crccheck 
>> = val;
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  366
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  367      return 
>> sizeof(w1_enable_crccheck);
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  368  }
>> fbf7f7b4e2ae40 Markus Franke      2012-05-26  369
>>
>> ---
>> 0-DAY CI Kernel Test Service, Intel Corporation
>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v1 03/15] powerpc/uaccess: Remove __get/put_user_inatomic()

[Christophe Leroy]

Le 01/03/2021 à 23:42, Daniel Axtens a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> 
>> Since commit 662bbcb2747c ("mm, sched: Allow uaccess in atomic with
>> pagefault_disable()"), __get/put_user() can be used in atomic parts
>> of the code, therefore the __get/put_user_inatomic() introduced
>> by commit e68c825bb016 ("[POWERPC] Add inatomic versions of __get_user
>> and __put_user") have become useless.
> 
> I spent some time chasing these macro definitions.

Ok, I'll try to make it more explicit.

> 
> Let me see if I understand you.
> 
> __get_user(x, ptr) becomes __get_user_nocheck(..., true)
> __get_user_inatomic() become __get_user_nosleep()
> 
> The difference between how __get_user_nosleep() and
> __get_user_nocheck(..., true) operate is that __get_user_nocheck calls
> might_fault() and __get_user_nosleep() does not.
> 
> If I understand the commit you reference and mm/memory.c, you're saying
> that we can indeed call might_fault() when page faults are disabled,
> because __might_fault() checks if page faults are disabled and does not
> fire a warning if it is called with page faults disabled.
> 
> Therefore, it is safe to remove our _inatomic version that does not call
> might_fault and just to call might_fault unconditionally.
> 
> Is that right?

Yes that's exactly that.

> 
> I haven't checked changes you made to the various .c files in fine
> detail but they appear to be entirely mechanical.
> 
>> powerpc is the only one having such functions. There is a real
>> intention not to have to provide such _inatomic() helpers, see the
>> comment in might_fault() in mm/memory.c introduced by
>> commit 3ee1afa308f2 ("x86: some lock annotations for user
>> copy paths, v2"):
>>
>> 	/*
>> 	 * it would be nicer only to annotate paths which are not under
>> 	 * pagefault_disable, however that requires a larger audit and
>> 	 * providing helpers like get_user_atomic.
>> 	 */
>>
> 
> I'm not fully sure I understand what you're saying in this part of the
> commit message.

The idea was to say, based of that unrelated comment I found, that really nobody wanted to have to 
use atomic dedicated helpers like.

As it brings more confusion than clarify, I'll remove it and only say that powperpc was the only 
architecture with such helpers.

Christophe

> 
> Kind regards,
> Daniel
> 
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/powerpc/include/asm/uaccess.h            | 37 -------------------
>>   arch/powerpc/kernel/align.c                   | 32 ++++++++--------
>>   .../kernel/hw_breakpoint_constraints.c        |  2 +-
>>   arch/powerpc/kernel/traps.c                   |  2 +-
>>   4 files changed, 18 insertions(+), 55 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
>> index a08c482b1315..01aea0df4dd0 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -53,11 +53,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
>>   #define __put_user(x, ptr) \
>>   	__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>>   
>> -#define __get_user_inatomic(x, ptr) \
>> -	__get_user_nosleep((x), (ptr), sizeof(*(ptr)))
>> -#define __put_user_inatomic(x, ptr) \
>> -	__put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>> -
>>   #ifdef CONFIG_PPC64
>>   
>>   #define ___get_user_instr(gu_op, dest, ptr)				\
>> @@ -92,9 +87,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
>>   #define __get_user_instr(x, ptr) \
>>   	___get_user_instr(__get_user, x, ptr)
>>   
>> -#define __get_user_instr_inatomic(x, ptr) \
>> -	___get_user_instr(__get_user_inatomic, x, ptr)
>> -
>>   extern long __put_user_bad(void);
>>   
>>   #define __put_user_size(x, ptr, size, retval)			\
>> @@ -141,20 +133,6 @@ __pu_failed:							\
>>   	__pu_err;							\
>>   })
>>   
>> -#define __put_user_nosleep(x, ptr, size)			\
>> -({								\
>> -	long __pu_err;						\
>> -	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
>> -	__typeof__(*(ptr)) __pu_val = (x);			\
>> -	__typeof__(size) __pu_size = (size);			\
>> -								\
>> -	__chk_user_ptr(__pu_addr);				\
>> -	__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
>> -								\
>> -	__pu_err;						\
>> -})
>> -
>> -
>>   /*
>>    * We don't tell gcc that we are accessing memory, but this is OK
>>    * because we do not write to any memory gcc knows about, so there
>> @@ -320,21 +298,6 @@ do {								\
>>   	__gu_err;							\
>>   })
>>   
>> -#define __get_user_nosleep(x, ptr, size)			\
>> -({								\
>> -	long __gu_err;						\
>> -	__long_type(*(ptr)) __gu_val;				\
>> -	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
>> -	__typeof__(size) __gu_size = (size);			\
>> -								\
>> -	__chk_user_ptr(__gu_addr);				\
>> -	__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
>> -	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
>> -								\
>> -	__gu_err;						\
>> -})
>> -
>> -
>>   /* more complex routines */
>>   
>>   extern unsigned long __copy_tofrom_user(void __user *to,
>> diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
>> index c7797eb958c7..83b199026a1e 100644
>> --- a/arch/powerpc/kernel/align.c
>> +++ b/arch/powerpc/kernel/align.c
>> @@ -174,18 +174,18 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
>>   
>>   		switch (nb) {
>>   		case 8:
>> -			ret |= __get_user_inatomic(temp.v[0], p++);
>> -			ret |= __get_user_inatomic(temp.v[1], p++);
>> -			ret |= __get_user_inatomic(temp.v[2], p++);
>> -			ret |= __get_user_inatomic(temp.v[3], p++);
>> +			ret |= __get_user(temp.v[0], p++);
>> +			ret |= __get_user(temp.v[1], p++);
>> +			ret |= __get_user(temp.v[2], p++);
>> +			ret |= __get_user(temp.v[3], p++);
>>   			fallthrough;
>>   		case 4:
>> -			ret |= __get_user_inatomic(temp.v[4], p++);
>> -			ret |= __get_user_inatomic(temp.v[5], p++);
>> +			ret |= __get_user(temp.v[4], p++);
>> +			ret |= __get_user(temp.v[5], p++);
>>   			fallthrough;
>>   		case 2:
>> -			ret |= __get_user_inatomic(temp.v[6], p++);
>> -			ret |= __get_user_inatomic(temp.v[7], p++);
>> +			ret |= __get_user(temp.v[6], p++);
>> +			ret |= __get_user(temp.v[7], p++);
>>   			if (unlikely(ret))
>>   				return -EFAULT;
>>   		}
>> @@ -259,18 +259,18 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
>>   		p = addr;
>>   		switch (nb) {
>>   		case 8:
>> -			ret |= __put_user_inatomic(data.v[0], p++);
>> -			ret |= __put_user_inatomic(data.v[1], p++);
>> -			ret |= __put_user_inatomic(data.v[2], p++);
>> -			ret |= __put_user_inatomic(data.v[3], p++);
>> +			ret |= __put_user(data.v[0], p++);
>> +			ret |= __put_user(data.v[1], p++);
>> +			ret |= __put_user(data.v[2], p++);
>> +			ret |= __put_user(data.v[3], p++);
>>   			fallthrough;
>>   		case 4:
>> -			ret |= __put_user_inatomic(data.v[4], p++);
>> -			ret |= __put_user_inatomic(data.v[5], p++);
>> +			ret |= __put_user(data.v[4], p++);
>> +			ret |= __put_user(data.v[5], p++);
>>   			fallthrough;
>>   		case 2:
>> -			ret |= __put_user_inatomic(data.v[6], p++);
>> -			ret |= __put_user_inatomic(data.v[7], p++);
>> +			ret |= __put_user(data.v[6], p++);
>> +			ret |= __put_user(data.v[7], p++);
>>   		}
>>   		if (unlikely(ret))
>>   			return -EFAULT;
>> diff --git a/arch/powerpc/kernel/hw_breakpoint_constraints.c b/arch/powerpc/kernel/hw_breakpoint_constraints.c
>> index 867ee4aa026a..675d1f66ab72 100644
>> --- a/arch/powerpc/kernel/hw_breakpoint_constraints.c
>> +++ b/arch/powerpc/kernel/hw_breakpoint_constraints.c
>> @@ -141,7 +141,7 @@ void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
>>   {
>>   	struct instruction_op op;
>>   
>> -	if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip))
>> +	if (__get_user_instr(*instr, (void __user *)regs->nip))
>>   		return;
>>   
>>   	analyse_instr(&op, regs, *instr);
>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>> index 1583fd1c6010..1fa36bd08efe 100644
>> --- a/arch/powerpc/kernel/traps.c
>> +++ b/arch/powerpc/kernel/traps.c
>> @@ -864,7 +864,7 @@ static void p9_hmi_special_emu(struct pt_regs *regs)
>>   	unsigned long ea, msr, msr_mask;
>>   	bool swap;
>>   
>> -	if (__get_user_inatomic(instr, (unsigned int __user *)regs->nip))
>> +	if (__get_user(instr, (unsigned int __user *)regs->nip))
>>   		return;
>>   
>>   	/*
>> -- 
>> 2.25.0

Re: [PATCH v1 01/15] powerpc/uaccess: Remove __get_user_allowed() and unsafe_op_wrap()

[Christophe Leroy]

Le 01/03/2021 à 23:02, Daniel Axtens a écrit :
> 
> 
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> 
>> Those two macros have only one user which is unsafe_get_user().
>>
>> Put everything in one place and remove them.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/powerpc/include/asm/uaccess.h | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
>> index 78e2a3990eab..8cbf3e3874f1 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -53,9 +53,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
>>   #define __put_user(x, ptr) \
>>   	__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>>   
>> -#define __get_user_allowed(x, ptr) \
>> -	__get_user_nocheck((x), (ptr), sizeof(*(ptr)), false)
>> -
>>   #define __get_user_inatomic(x, ptr) \
>>   	__get_user_nosleep((x), (ptr), sizeof(*(ptr)))
>>   #define __put_user_inatomic(x, ptr) \
>> @@ -482,8 +479,11 @@ user_write_access_begin(const void __user *ptr, size_t len)
>>   #define user_write_access_begin	user_write_access_begin
>>   #define user_write_access_end		prevent_current_write_to_user
>>   
>> -#define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0)
>> -#define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
>> +#define unsafe_get_user(x, p, e) do {					\
>> +	if (unlikely(__get_user_nocheck((x), (p), sizeof(*(p)), false)))\
>> +		goto e;							\
>> +} while (0)
>> +
> 
> This seems correct to me.
> 
> Checkpatch does have one check that is relevant:
> 
> CHECK: Macro argument reuse 'p' - possible side-effects?
> #36: FILE: arch/powerpc/include/asm/uaccess.h:482:
> +#define unsafe_get_user(x, p, e) do {					\
> +	if (unlikely(__get_user_nocheck((x), (p), sizeof(*(p)), false)))\
> +		goto e;							\
> +} while (0)
> 
> Given that we are already creating a new block, should we do something
> like this (completely untested):
> 
> #define unsafe_get_user(x, p, e) do {					\
>          __typeof__(p) __p = (p);
> 	if (unlikely(__get_user_nocheck((x), (__p), sizeof(*(__p)), false)))\
> 		goto e;							\
> } while (0)
> 

As mentioned by Segher, this is not needed, sizeof(p) doesn't evaluate (p) so (p) is only evaluated 
once in the macro, so no risk of side-effects with that.

Christophe