Re: [PATCH 1/4] parisc: Drop strnlen_user() in favour of generic version

[Helge Deller <deller@gmx.de>]

* Arnd Bergmann <arnd@kernel.org>:
> On Wed, Sep 8, 2021 at 11:40 PM Helge Deller <deller@gmx.de> wrote:
> > On 9/8/21 11:26 PM, Arnd Bergmann wrote:
> > >>   /*
> > >>    * Complex access routines -- macros
> > >>    */
> > >> -#define user_addr_max() (~0UL)
> > >> +#define user_addr_max() (uaccess_kernel() ? ~0UL : TASK_SIZE)
> >
> > I noticed that our user_addr_max() was actually wrong.
> > It's used in the generic strnlen_user() so fixing it seemed appropriate.
>
> The user_addr_max() definition should match what you do in access_ok()
> for USER_DS though, which is a nop on architectures that have split user
> address space, so I think the ~0UL was correct here.
>
> No matter what the arguments are, there is no danger of spilling over
> into kernel space, which is what the user_addr_max() check is trying
> to prevent on other architectures.
>
> > > We are getting very close to completely removing set_fs()/get_fs(),
> > > uaccess_kernel() and some related bits from the kernel, so I think
> > > it would be better to the other way here and finish off removing
> > > CONFIG_SET_FS from parisc.
> > >
> > > I think this will also simplify your asm/uaccess.h a lot, in particular
> > > since it has separate address spaces for __get_user() and
> > > __get_kernel_nofault(), and without set_fs() you can leave out
> > > the runtime conditional to switch between them.
> >
> > That's a good idea and should probably be done.
> > Do you have some pointers where to start, e.g. initial commits from other arches ?
>
> Russell just merged my series for arch/arm in linux-5.15, you
> can look at that but it's probably easier for parisc.
>
> I think the only part you need to add is __get_kernel_nofault()
> and __put_kernel_nofault(). You can see in mm/maccess.c
> what the difference between the two versions in there is.
>
> Once you have those, you define HAVE_GET_KERNEL_NOFAULT
> and then remove CONFIG_SET_FS, set_fs(), get_fs(), load_sr2(),
> thread_info->addr_limit, KERNEL_DS, and USER_DS.

Thanks for those hints!
The attached initial patch below seems to work, it boots into systemd.
It needs further cleanups though.

I wonder if you should add a workaround for 32-bit kernels
to not copy 64bit-wise, see my patch below in
copy_from_kernel_nofault():

-       copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
+       /* avoid 64-bit accesses on 32-bit platforms */
+       if (sizeof(unsigned long) > 4)
+               copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);

Helge

---

[PATCH] parisc: Implement __get/put_kernel_nofault()

Add __get_kernel_nofault() and __put_kernel_nofault(), define
HAVE_GET_KERNEL_NOFAULT and remove CONFIG_SET_FS, set_fs(), get_fs(),
load_sr2(), thread_info->addr_limit, KERNEL_DS, and USER_DS.

Signed-off-by: Helge Deller <deller@gmx.de>
Suggested-by: Arnd Bergmann <arnd@kernel.org>

diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index 3ae71994399c..b5bc346d464a 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -64,7 +64,6 @@ config PARISC
 	select HAVE_KPROBES_ON_FTRACE
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS
 	select HAVE_SOFTIRQ_ON_OWN_STACK if IRQSTACKS
-	select SET_FS

 	help
 	  The PA-RISC microprocessor is designed by Hewlett-Packard and used
diff --git a/arch/parisc/include/asm/processor.h b/arch/parisc/include/asm/processor.h
index b5fbcd2c1780..eeb7da064289 100644
--- a/arch/parisc/include/asm/processor.h
+++ b/arch/parisc/include/asm/processor.h
@@ -101,10 +101,6 @@ DECLARE_PER_CPU(struct cpuinfo_parisc, cpu_data);

 #define CPU_HVERSION ((boot_cpu_data.hversion >> 4) & 0x0FFF)

-typedef struct {
-	int seg;
-} mm_segment_t;
-
 #define ARCH_MIN_TASKALIGN	8

 struct thread_struct {
diff --git a/arch/parisc/include/asm/thread_info.h b/arch/parisc/include/asm/thread_info.h
index 0bd38a972cea..00ad50fef769 100644
--- a/arch/parisc/include/asm/thread_info.h
+++ b/arch/parisc/include/asm/thread_info.h
@@ -11,7 +11,6 @@
 struct thread_info {
 	struct task_struct *task;	/* main task structure */
 	unsigned long flags;		/* thread_info flags (see TIF_*) */
-	mm_segment_t addr_limit;	/* user-level address space limit */
 	__u32 cpu;			/* current CPU */
 	int preempt_count;		/* 0=premptable, <0=BUG; will also serve as bh-counter */
 };
@@ -21,7 +20,6 @@ struct thread_info {
 	.task		= &tsk,			\
 	.flags		= 0,			\
 	.cpu		= 0,			\
-	.addr_limit	= KERNEL_DS,		\
 	.preempt_count	= INIT_PREEMPT_COUNT,	\
 }

diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h
index a4368731f2b4..6e0ca7756ceb 100644
--- a/arch/parisc/include/asm/uaccess.h
+++ b/arch/parisc/include/asm/uaccess.h
@@ -11,14 +11,6 @@
 #include <linux/bug.h>
 #include <linux/string.h>

-#define KERNEL_DS	((mm_segment_t){0})
-#define USER_DS 	((mm_segment_t){1})
-
-#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
-
-#define get_fs()	(current_thread_info()->addr_limit)
-#define set_fs(x)	(current_thread_info()->addr_limit = (x))
-
 /*
  * Note that since kernel addresses are in a separate address space on
  * parisc, we don't need to do anything for access_ok().
@@ -33,11 +25,11 @@
 #define get_user __get_user

 #if !defined(CONFIG_64BIT)
-#define LDD_USER(val, ptr)	__get_user_asm64(val, ptr)
-#define STD_USER(x, ptr)	__put_user_asm64(x, ptr)
+#define LDD_USER(sr, val, ptr, err_label)	__get_user_asm64(sr, val, ptr, err_label)
+#define STD_USER(sr, x, ptr, err_label)		__put_user_asm64(sr, x, ptr, err_label)
 #else
-#define LDD_USER(val, ptr)	__get_user_asm(val, "ldd", ptr)
-#define STD_USER(x, ptr)	__put_user_asm("std", x, ptr)
+#define LDD_USER(sr, val, ptr, err_label)	__get_user_asm(sr, val, "ldd", ptr, err_label)
+#define STD_USER(sr, x, ptr, err_label)		__put_user_asm(sr, "std", x, ptr, err_label)
 #endif

 /*
@@ -67,28 +59,15 @@ struct exception_table_entry {
 #define ASM_EXCEPTIONTABLE_ENTRY_EFAULT( fault_addr, except_addr )\
 	ASM_EXCEPTIONTABLE_ENTRY( fault_addr, except_addr + 1)

-/*
- * load_sr2() preloads the space register %%sr2 - based on the value of
- * get_fs() - with either a value of 0 to access kernel space (KERNEL_DS which
- * is 0), or with the current value of %%sr3 to access user space (USER_DS)
- * memory. The following __get_user_asm() and __put_user_asm() functions have
- * %%sr2 hard-coded to access the requested memory.
- */
-#define load_sr2() \
-	__asm__(" or,=  %0,%%r0,%%r0\n\t"	\
-		" mfsp %%sr3,%0\n\t"		\
-		" mtsp %0,%%sr2\n\t"		\
-		: : "r"(get_fs()) : )
-
-#define __get_user_internal(val, ptr)			\
+#define __get_user_internal(sr, val, ptr, err_label)	\
 ({							\
 	register long __gu_err __asm__ ("r8") = 0;	\
 							\
 	switch (sizeof(*(ptr))) {			\
-	case 1: __get_user_asm(val, "ldb", ptr); break;	\
-	case 2: __get_user_asm(val, "ldh", ptr); break; \
-	case 4: __get_user_asm(val, "ldw", ptr); break; \
-	case 8: LDD_USER(val, ptr); break;		\
+	case 1: __get_user_asm(sr, val, "ldb", ptr, err_label); break;	\
+	case 2: __get_user_asm(sr, val, "ldh", ptr, err_label); break;	\
+	case 4: __get_user_asm(sr, val, "ldw", ptr, err_label); break;	\
+	case 8: LDD_USER(sr, val, ptr, err_label); break;		\
 	default: BUILD_BUG();				\
 	}						\
 							\
@@ -97,26 +76,38 @@ struct exception_table_entry {

 #define __get_user(val, ptr)				\
 ({							\
-	load_sr2();					\
-	__get_user_internal(val, ptr);			\
+	__get_user_internal("%%sr3,", val, ptr, 9b);	\
 })

-#define __get_user_asm(val, ldx, ptr)			\
+#define __get_user_asm(sr, val, ldx, ptr, err_label)	\
 {							\
 	register long __gu_val;				\
 							\
-	__asm__("1: " ldx " 0(%%sr2,%2),%0\n"		\
+	__asm__("1: " ldx " 0(" sr "%2),%0\n"		\
 		"9:\n"					\
-		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b)	\
+		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, err_label) \
 		: "=r"(__gu_val), "=r"(__gu_err)        \
 		: "r"(ptr), "1"(__gu_err));		\
 							\
 	(val) = (__force __typeof__(*(ptr))) __gu_val;	\
 }

+#define HAVE_GET_KERNEL_NOFAULT
+#define __get_kernel_nofault(dst, src, type, err_label)	\
+{							\
+	type __z;					\
+	long __err;					\
+	__err = __get_user_internal("", __z, (type *)(src), 9b); \
+	if (unlikely(__err))				\
+		goto err_label;				\
+	else						\
+		*(type *)(dst) = __z;			\
+}
+
+
 #if !defined(CONFIG_64BIT)

-#define __get_user_asm64(val, ptr)			\
+#define __get_user_asm64(sr, val, ptr, err_label)		\
 {							\
 	union {						\
 		unsigned long long	l;		\
@@ -124,11 +115,11 @@ struct exception_table_entry {
 	} __gu_tmp;					\
 							\
 	__asm__("   copy %%r0,%R0\n"			\
-		"1: ldw 0(%%sr2,%2),%0\n"		\
-		"2: ldw 4(%%sr2,%2),%R0\n"		\
+		"1: ldw 0(" sr "%2),%0\n"		\
+		"2: ldw 4(" sr "%2),%R0\n"		\
 		"9:\n"					\
-		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b)	\
-		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b)	\
+		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, err_label) \
+		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, err_label) \
 		: "=&r"(__gu_tmp.l), "=r"(__gu_err)	\
 		: "r"(ptr), "1"(__gu_err));		\
 							\
@@ -138,16 +129,16 @@ struct exception_table_entry {
 #endif /* !defined(CONFIG_64BIT) */


-#define __put_user_internal(x, ptr)				\
+#define __put_user_internal(sr, x, ptr, err_label)			\
 ({								\
 	register long __pu_err __asm__ ("r8") = 0;      	\
         __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x);	\
 								\
 	switch (sizeof(*(ptr))) {				\
-	case 1: __put_user_asm("stb", __x, ptr); break;		\
-	case 2: __put_user_asm("sth", __x, ptr); break;		\
-	case 4: __put_user_asm("stw", __x, ptr); break;		\
-	case 8: STD_USER(__x, ptr); break;			\
+	case 1: __put_user_asm(sr, "stb", __x, ptr, err_label); break; \
+	case 2: __put_user_asm(sr, "sth", __x, ptr, err_label); break; \
+	case 4: __put_user_asm(sr, "stw", __x, ptr, err_label); break; \
+	case 8: STD_USER(sr, __x, ptr, err_label); break;		\
 	default: BUILD_BUG();					\
 	}							\
 								\
@@ -156,10 +147,20 @@ struct exception_table_entry {

 #define __put_user(x, ptr)					\
 ({								\
-	load_sr2();						\
-	__put_user_internal(x, ptr);				\
+	__put_user_internal("%%sr3,", x, ptr, 9b);		\
 })

+#define __put_kernel_nofault(dst, src, type, err_label)		\
+{								\
+	type __z = *(type *)(src);				\
+	long __err;						\
+	__err = __put_user_internal("", __z, (type *)(dst), 9b);\
+	if (unlikely(__err))					\
+		goto err_label;					\
+}
+
+
+

 /*
  * The "__put_user/kernel_asm()" macros tell gcc they read from memory
@@ -170,26 +171,26 @@ struct exception_table_entry {
  * r8 is already listed as err.
  */

-#define __put_user_asm(stx, x, ptr)                         \
-	__asm__ __volatile__ (                              \
-		"1: " stx " %2,0(%%sr2,%1)\n"		    \
-		"9:\n"					    \
-		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b)	    \
-		: "=r"(__pu_err)                            \
+#define __put_user_asm(sr, stx, x, ptr, err_label)		\
+	__asm__ __volatile__ (					\
+		"1: " stx " %2,0(" sr "%1)\n"			\
+		"9:\n"						\
+		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, err_label)	\
+		: "=r"(__pu_err)				\
 		: "r"(ptr), "r"(x), "0"(__pu_err))


 #if !defined(CONFIG_64BIT)

-#define __put_user_asm64(__val, ptr) do {	    	    \
-	__asm__ __volatile__ (				    \
-		"1: stw %2,0(%%sr2,%1)\n"		    \
-		"2: stw %R2,4(%%sr2,%1)\n"		    \
-		"9:\n"					    \
-		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b)	    \
-		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b)	    \
-		: "=r"(__pu_err)                            \
-		: "r"(ptr), "r"(__val), "0"(__pu_err));	    \
+#define __put_user_asm64(sr, __val, ptr, err_label) do {	\
+	__asm__ __volatile__ (					\
+		"1: stw %2,0(" sr "%1)\n"			\
+		"2: stw %R2,4(" sr "%1)\n"			\
+		"9:\n"						\
+		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, err_label)	\
+		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, err_label)	\
+		: "=r"(__pu_err)				\
+		: "r"(ptr), "r"(__val), "0"(__pu_err));		\
 } while (0)

 #endif /* !defined(CONFIG_64BIT) */
@@ -205,7 +206,6 @@ extern __must_check long strnlen_user(const char __user *src, long n);
 /*
  * Complex access routines -- macros
  */
-#define user_addr_max() (~0UL)

 #define clear_user lclear_user
 #define __clear_user lclear_user
diff --git a/arch/parisc/kernel/asm-offsets.c b/arch/parisc/kernel/asm-offsets.c
index 33113ba24054..7c83915e4aed 100644
--- a/arch/parisc/kernel/asm-offsets.c
+++ b/arch/parisc/kernel/asm-offsets.c
@@ -230,7 +230,7 @@ int main(void)
 	DEFINE(TI_TASK, offsetof(struct thread_info, task));
 	DEFINE(TI_FLAGS, offsetof(struct thread_info, flags));
 	DEFINE(TI_CPU, offsetof(struct thread_info, cpu));
-	DEFINE(TI_SEGMENT, offsetof(struct thread_info, addr_limit));
+//	DEFINE(TI_SEGMENT, offsetof(struct thread_info, addr_limit));
 	DEFINE(TI_PRE_COUNT, offsetof(struct thread_info, preempt_count));
 	DEFINE(THREAD_SZ, sizeof(struct thread_info));
 	/* THREAD_SZ_ALGN includes space for a stack frame. */
diff --git a/arch/parisc/lib/lusercopy.S b/arch/parisc/lib/lusercopy.S
index 0aad5ce89f4d..29eafd963ac0 100644
--- a/arch/parisc/lib/lusercopy.S
+++ b/arch/parisc/lib/lusercopy.S
@@ -34,11 +34,11 @@
 	 */

 	.macro  get_sr
-	mfctl       %cr30,%r1
-	ldw         TI_SEGMENT(%r1),%r22
+//	mfctl       %cr30,%r1
+//	ldw         TI_SEGMENT(%r1),%r22
 	mfsp        %sr3,%r1
-	or,<>       %r22,%r0,%r0
-	copy        %r0,%r1
+//	or,<>       %r22,%r0,%r0
+//	copy        %r0,%r1
 	mtsp        %r1,%sr1
 	.endm

diff --git a/mm/maccess.c b/mm/maccess.c
index 3bd70405f2d8..85eb86d6cab4 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -28,7 +28,9 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
 		return -ERANGE;

 	pagefault_disable();
-	copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
+	/* avoid 64-bit accesses on 32-bit platforms */
+	if (sizeof(unsigned long) > 4)
+		copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
 	copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
 	copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
 	copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
@@ -51,7 +53,9 @@ EXPORT_SYMBOL_GPL(copy_from_kernel_nofault);
 long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
 {
 	pagefault_disable();
-	copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
+	/* avoid 64-bit accesses on 32-bit platforms */
+	if (sizeof(unsigned long) > 4)
+		copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
 	copy_to_kernel_nofault_loop(dst, src, size, u32, Efault);
 	copy_to_kernel_nofault_loop(dst, src, size, u16, Efault);
 	copy_to_kernel_nofault_loop(dst, src, size, u8, Efault);