asm-generic: strncpy_from_user/strnlen_user cleanup

PATCH Superseded Arnd Bergmann 2 participants 13 messages May 15, 2021 – May 17, 2021

Subsystems (6)

RISC-V Generic include/asm header files User-mode Linux (UML) m68k architecture +2 more subsystems

lore

Share & tools

lore

Arnd Bergmann

author

2021-05-15 10:17 UTC #0 lore raw

v2 cover asm-generic: strncpy_from_user/strnlen_user cleanup

From: Arnd Bergmann <[email protected]>

As I've queued up some patches for asm-generic, I remembered an older series that I created but never submitted.

These two functions appear to be unnecessarily different between architectures, and the asm-generic version is a bit questionable, even for NOMMU architectures.

Clean this up to just use the generic library version for anything that uses the generic version today. I've expanded on the patch descriptions a little, as suggested by Christoph Hellwig, but I suspect a more detailed review would uncover additional problems with the custom versions that are getting removed.

Arnd

Arnd Bergmann (6):
[v2] asm-generic/uaccess.h: remove __strncpy_from_user/__strnlen_user [v2] h8300: remove stale strncpy_from_user
[v2] hexagon: use generic strncpy/strnlen from_user
[v2] arc: use generic strncpy/strnlen from_user
[v2] asm-generic: uaccess: remove inline
strncpy_from_user/strnlen_user
[v2] asm-generic: remove extra strn{cpy_from,len}_user declarations

 arch/arc/Kconfig                    |   2 +
 arch/arc/include/asm/uaccess.h      |  72 ----------------
 arch/arc/mm/extable.c               |  12 ---
 arch/h8300/Kconfig                  |   2 +
 arch/h8300/kernel/h8300_ksyms.c     |   2 -
 arch/h8300/lib/Makefile             |   2 +-
 arch/h8300/lib/strncpy.S            |  35 --------
 arch/hexagon/Kconfig                |   2 +
 arch/hexagon/include/asm/uaccess.h  |  31 -------
 arch/hexagon/kernel/hexagon_ksyms.c |   1 -
 arch/hexagon/mm/Makefile            |   2 +-
 arch/hexagon/mm/strnlen_user.S      | 126 ----------------------------
 arch/m68k/Kconfig                   |   4 +-
 arch/riscv/Kconfig                  |   4 +-
 arch/um/include/asm/uaccess.h       |   5 +-
 arch/um/kernel/skas/uaccess.c       |   5 +-
 include/asm-generic/uaccess.h       |  52 ++----------
 17 files changed, 25 insertions(+), 334 deletions(-)
 delete mode 100644 arch/h8300/lib/strncpy.S
 delete mode 100644 arch/hexagon/mm/strnlen_user.S

Signature · 26 lines

2.29.2

Cc: Christoph Hellwig <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Vineet Gupta <[email protected]>
Cc: Yoshinori Sato <[email protected]>
Cc: Brian Cain <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Paul Walmsley <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Albert Ou <[email protected]>
Cc: Jeff Dike <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: Anton Ivanov <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Sid Manning <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Mike Rapoport <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

Arnd Bergmann

author

2021-05-15 10:17 UTC #1 lore raw

PATCH v2 1/6 asm-generic/uaccess.h: remove __strncpy_from_user/__strnlen_user

From: Arnd Bergmann <[email protected]>

This is a preparation for changing over architectures to the generic implementation one at a time. As there are no callers of either __strncpy_from_user() or __strnlen_user(), fold these into the strncpy_from_user() strnlen_user() functions to make each implementation independent of the others.

Many of these implementations have known bugs, but the intention here is to not change behavior at all and stay compatible with those bugs for the moment.

Signed-off-by Arnd Bergmann <[email protected]>

---
 arch/arc/include/asm/uaccess.h     | 14 ++++++++++----
 arch/hexagon/include/asm/uaccess.h | 22 +++++++++++++---------
 arch/um/include/asm/uaccess.h      |  8 ++++----
 arch/um/kernel/skas/uaccess.c      |  5 ++++-
 include/asm-generic/uaccess.h      | 28 +++++++++++-----------------
 5 files changed, 42 insertions(+), 35 deletions(-)

arch/arc/include/asm/uaccess.h +10 -4

index ea40ec7f6cae..3476348f361e 100644
@@ -661,6 +661,9 @@ __arc_strncpy_from_user(char *dst, const char __user *src, long count)
 	long res = 0;
 	char val;
 
+	if (!access_ok(src, 1))
+		return -EFAULT;
+
 	if (count == 0)
 		return 0;

@@ -693,6 +696,9 @@ static inline long __arc_strnlen_user(const char __user *s, long n)
 	long res, tmp1, cnt;
 	char val;
 
+	if (!access_ok(s, 1))
+		return 0;
+
 	__asm__ __volatile__(
 	"	mov %2, %1			\n"
 	"1:	ldb.ab  %3, [%0, 1]		\n"

@@ -724,8 +730,8 @@ static inline long __arc_strnlen_user(const char __user *s, long n)
 #define INLINE_COPY_FROM_USER
 
 #define __clear_user(d, n)		__arc_clear_user(d, n)
-#define __strncpy_from_user(d, s, n)	__arc_strncpy_from_user(d, s, n)
-#define __strnlen_user(s, n)		__arc_strnlen_user(s, n)
+#define strncpy_from_user(d, s, n)	__arc_strncpy_from_user(d, s, n)
+#define strnlen_user(s, n)		__arc_strnlen_user(s, n)
 #else
 extern unsigned long arc_clear_user_noinline(void __user *to,
 		unsigned long n);

@@ -734,8 +740,8 @@ extern long arc_strncpy_from_user_noinline (char *dst, const char __user *src,
 extern long arc_strnlen_user_noinline(const char __user *src, long n);
 
 #define __clear_user(d, n)		arc_clear_user_noinline(d, n)
-#define __strncpy_from_user(d, s, n)	arc_strncpy_from_user_noinline(d, s, n)
-#define __strnlen_user(s, n)		arc_strnlen_user_noinline(s, n)
+#define strncpy_from_user(d, s, n)	arc_strncpy_from_user_noinline(d, s, n)
+#define strnlen_user(s, n)		arc_strnlen_user_noinline(s, n)
 
 #endif

arch/hexagon/include/asm/uaccess.h +13 -9

index c1019a736ff1..59aa3a50744f 100644
@@ -57,23 +57,27 @@ unsigned long raw_copy_to_user(void __user *to, const void *from,
 __kernel_size_t __clear_user_hexagon(void __user *dest, unsigned long count);
 #define __clear_user(a, s) __clear_user_hexagon((a), (s))
 
-#define __strncpy_from_user(dst, src, n) hexagon_strncpy_from_user(dst, src, n)
+extern long __strnlen_user(const char __user *src, long n);
 
-/*  get around the ifndef in asm-generic/uaccess.h  */
-#define __strnlen_user __strnlen_user
+static inline strnlen_user(const char __user *src, long n)
+{
+        if (!access_ok(src, 1))
+		return 0;
 
-extern long __strnlen_user(const char __user *src, long n);
+	return __strnlen_user(src, n);
+}
+/*  get around the ifndef in asm-generic/uaccess.h  */
+#define strnlen_user strnlen_user
 
-static inline long hexagon_strncpy_from_user(char *dst, const char __user *src,
-					     long n);
+static inline long strncpy_from_user(char *dst, const char __user *src, long n);
+#define strncpy_from_user strncpy_from_user
 
 #include <asm-generic/uaccess.h>
 
 /*  Todo:  an actual accelerated version of this.  */
-static inline long hexagon_strncpy_from_user(char *dst, const char __user *src,
-					     long n)
+static inline long strncpy_from_user(char *dst, const char __user *src, long n)
 {
-	long res = __strnlen_user(src, n);
+	long res = strnlen_user(src, n);
 
 	if (unlikely(!res))
 		return -EFAULT;

arch/um/include/asm/uaccess.h +4 -4

index fe66d659acad..3bf209f683f8 100644
@@ -23,16 +23,16 @@
 
 extern unsigned long raw_copy_from_user(void *to, const void __user *from, unsigned long n);
 extern unsigned long raw_copy_to_user(void __user *to, const void *from, unsigned long n);
-extern long __strncpy_from_user(char *dst, const char __user *src, long count);
-extern long __strnlen_user(const void __user *str, long len);
+extern long strncpy_from_user(char *dst, const char __user *src, long count);
+extern long strnlen_user(const void __user *str, long len);
 extern unsigned long __clear_user(void __user *mem, unsigned long len);
 static inline int __access_ok(unsigned long addr, unsigned long size);
 
 /* Teach asm-generic/uaccess.h that we have C functions for these. */
 #define __access_ok __access_ok
 #define __clear_user __clear_user
-#define __strnlen_user __strnlen_user
-#define __strncpy_from_user __strncpy_from_user
+#define strnlen_user strnlen_user
+#define strncpy_from_user strncpy_from_user
 #define INLINE_COPY_FROM_USER
 #define INLINE_COPY_TO_USER

arch/um/kernel/skas/uaccess.c +4 -1

index 2dec915abe6f..205679cc4bb7 100644
@@ -188,11 +188,14 @@ static int strncpy_chunk_from_user(unsigned long from, int len, void *arg)
 	return 0;
 }
 
-long __strncpy_from_user(char *dst, const char __user *src, long count)
+long strncpy_from_user(char *dst, const char __user *src, long count)
 {
 	long n;
 	char *ptr = dst;
 
+	if (!access_ok(src, 1))
+		return -EFAULT;
+
 	if (uaccess_kernel()) {
 		strncpy(dst, (__force void *) src, count);
 		return strnlen(dst, count);

include/asm-generic/uaccess.h +11 ~~-17~~

index 4973328f3c6e..c03889cc904c 100644
@@ -246,11 +246,15 @@ extern int __get_user_bad(void) __attribute__((noreturn));
 /*
  * Copy a null terminated string from userspace.
  */
-#ifndef __strncpy_from_user
+#ifndef strncpy_from_user
 static inline long
-__strncpy_from_user(char *dst, const char __user *src, long count)
+strncpy_from_user(char *dst, const char __user *src, long count)
 {
 	char *tmp;
+
+	if (!access_ok(src, 1))
+		return -EFAULT;
+
 	strncpy(dst, (const char __force *)src, count);
 	for (tmp = dst; *tmp && count > 0; tmp++, count--)
 		;

@@ -258,24 +262,12 @@ __strncpy_from_user(char *dst, const char __user *src, long count)
 }
 #endif
 
-static inline long
-strncpy_from_user(char *dst, const char __user *src, long count)
-{
-	if (!access_ok(src, 1))
-		return -EFAULT;
-	return __strncpy_from_user(dst, src, count);
-}
-
+#ifndef strnlen_user
 /*
  * Return the size of a string (including the ending 0)
  *
  * Return 0 on exception, a value greater than N if too long
- */
-#ifndef __strnlen_user
-#define __strnlen_user(s, n) (strnlen((s), (n)) + 1)
-#endif
-
-/*
+ *
  * Unlike strnlen, strnlen_user includes the nul terminator in
  * its returned count. Callers should check for a returned value
  * greater than N as an indication the string is too long.

@@ -284,8 +276,10 @@ static inline long strnlen_user(const char __user *src, long n)
 {
 	if (!access_ok(src, 1))
 		return 0;
-	return __strnlen_user(src, n);
+
+	return strnlen(src, n) + 1;
 }
+#endif
 
 /*
  * Zero Userspace

Signature · 1 line

2.29.2

Arnd Bergmann

author

2021-05-15 10:17 UTC #2 lore raw

PATCH v2 2/6 h8300: remove stale strncpy_from_user

In reply to Arnd Bergmann

From: Arnd Bergmann <[email protected]>

This function is never called because h8300 uses the asm-generic inline function version.

Signed-off-by Arnd Bergmann <[email protected]>

---
 arch/h8300/kernel/h8300_ksyms.c |  2 --
 arch/h8300/lib/Makefile         |  2 +-
 arch/h8300/lib/strncpy.S        | 35 ---------------------------------
 3 files changed, 1 insertion(+), 38 deletions(-)
 delete mode 100644 arch/h8300/lib/strncpy.S

arch/h8300/kernel/h8300_ksyms.c +0 -2

index 1c6f902e82a5..853d6e886477 100644
@@ -19,7 +19,6 @@ asmlinkage long __mulsi3(long, long);
 asmlinkage long __udivsi3(long, long);
 asmlinkage void *memcpy(void *, const void *, size_t);
 asmlinkage void *memset(void *, int, size_t);
-asmlinkage long strncpy_from_user(void *to, void *from, size_t n);
 
 	/* gcc lib functions */
 EXPORT_SYMBOL(__ucmpdi2);

@@ -34,4 +33,3 @@ EXPORT_SYMBOL(__mulsi3);
 EXPORT_SYMBOL(__udivsi3);
 EXPORT_SYMBOL(memcpy);
 EXPORT_SYMBOL(memset);
-EXPORT_SYMBOL(strncpy_from_user);

arch/h8300/lib/Makefile +1 -1

index 685fa837c1f7..5911c1fa856d 100644
@@ -3,7 +3,7 @@
 # Makefile for H8/300-specific library files..
 #
 
-lib-y  = memcpy.o memset.o abs.o strncpy.o \
+lib-y  = memcpy.o memset.o abs.o \
 	 mulsi3.o udivsi3.o muldi3.o moddivsi3.o \
 	 ashldi3.o lshrdi3.o ashrdi3.o ucmpdi2.o \
 	 delay.o

arch/h8300/lib/strncpy.S deleted +0 ~~-35~~

deleted file mode 100644
index 8b65d7c4727b..000000000000
@@ -1,35 +0,0 @@
-;;; SPDX-License-Identifier: GPL-2.0
-;;; strncpy.S
-
-#include <asm/linkage.h>
-
-	.text
-.global strncpy_from_user
-
-;;; long strncpy_from_user(void *to, void *from, size_t n)
-strncpy_from_user:
-	mov.l	er2,er2
-	bne	1f
-	sub.l	er0,er0
-	rts
-1:
-	mov.l	er4,@-sp
-	sub.l	er3,er3
-2:
-	mov.b	@er1+,r4l
-	mov.b	r4l,@er0
-	adds	#1,er0
-	beq	3f
-	inc.l	#1,er3
-	dec.l	#1,er2
-	bne	2b
-3:
-	dec.l	#1,er2
-4:
-	mov.b	r4l,@er0
-	adds	#1,er0
-	dec.l	#1,er2
-	bne	4b
-	mov.l	er3,er0
-	mov.l	@sp+,er4
-	rts

Signature · 1 line

2.29.2

Arnd Bergmann

author

2021-05-15 10:18 UTC #3 lore raw

PATCH v2 3/6 hexagon: use generic strncpy/strnlen from_user

In reply to Arnd Bergmann

From: Arnd Bergmann <[email protected]>

Most per-architecture versions of these functions are broken in some form, and they are almost certainly slower than the generic code as well.

Remove the ones for hexagon and instead use the generic version. This custom version reads the data twice for strncpy() by doing an extra strnlen(), and it apparently lacks a check for user_addr_max().

Signed-off-by Arnd Bergmann <[email protected]>

---
 arch/hexagon/Kconfig                |   2 +
 arch/hexagon/include/asm/uaccess.h  |  33 +-------
 arch/hexagon/kernel/hexagon_ksyms.c |   1 -
 arch/hexagon/mm/Makefile            |   2 +-
 arch/hexagon/mm/strnlen_user.S      | 126 ----------------------------
 5 files changed, 5 insertions(+), 159 deletions(-)
 delete mode 100644 arch/hexagon/mm/strnlen_user.S

arch/hexagon/Kconfig +2 -0

index f18ec6f49c15..d094186188df 100644
@@ -19,6 +19,8 @@ config HEXAGON
 	# GENERIC_ALLOCATOR is used by dma_alloc_coherent()
 	select GENERIC_ALLOCATOR
 	select GENERIC_IRQ_SHOW
+	select GENERIC_STRNCPY_FROM_USER
+	select GENERIC_STRNLEN_USER
 	select HAVE_ARCH_KGDB
 	select HAVE_ARCH_TRACEHOOK
 	select NEED_SG_DMA_LENGTH

arch/hexagon/include/asm/uaccess.h +2 ~~-31~~

index 59aa3a50744f..d950df12d8c5 100644
@@ -57,42 +57,13 @@ unsigned long raw_copy_to_user(void __user *to, const void *from,
 __kernel_size_t __clear_user_hexagon(void __user *dest, unsigned long count);
 #define __clear_user(a, s) __clear_user_hexagon((a), (s))
 
-extern long __strnlen_user(const char __user *src, long n);
-
-static inline strnlen_user(const char __user *src, long n)
-{
-        if (!access_ok(src, 1))
-		return 0;
-
-	return __strnlen_user(src, n);
-}
-/*  get around the ifndef in asm-generic/uaccess.h  */
+extern long strnlen_user(const char __user *src, long n);
 #define strnlen_user strnlen_user
 
-static inline long strncpy_from_user(char *dst, const char __user *src, long n);
+extern long strncpy_from_user(char *dst, const char __user *src, long n)
 #define strncpy_from_user strncpy_from_user
 
 #include <asm-generic/uaccess.h>
 
-/*  Todo:  an actual accelerated version of this.  */
-static inline long strncpy_from_user(char *dst, const char __user *src, long n)
-{
-	long res = strnlen_user(src, n);
-
-	if (unlikely(!res))
-		return -EFAULT;
-
-	if (res > n) {
-		long left = raw_copy_from_user(dst, src, n);
-		if (unlikely(left))
-			memset(dst + (n - left), 0, left);
-		return n;
-	} else {
-		long left = raw_copy_from_user(dst, src, res);
-		if (unlikely(left))
-			memset(dst + (res - left), 0, left);
-		return res-1;
-	}
-}
 
 #endif

arch/hexagon/kernel/hexagon_ksyms.c +0 -1

index 35545a7386a0..ec56ce2d92a2 100644
@@ -15,7 +15,6 @@ EXPORT_SYMBOL(__clear_user_hexagon);
 EXPORT_SYMBOL(raw_copy_from_user);
 EXPORT_SYMBOL(raw_copy_to_user);
 EXPORT_SYMBOL(iounmap);
-EXPORT_SYMBOL(__strnlen_user);
 EXPORT_SYMBOL(__vmgetie);
 EXPORT_SYMBOL(__vmsetie);
 EXPORT_SYMBOL(__vmyield);

arch/hexagon/mm/Makefile +1 -1

index 893838499591..49911a906fd0 100644
@@ -4,4 +4,4 @@
 #
 
 obj-y := init.o ioremap.o uaccess.o vm_fault.o cache.o
-obj-y += copy_to_user.o copy_from_user.o strnlen_user.o vm_tlb.o
+obj-y += copy_to_user.o copy_from_user.o vm_tlb.o

arch/hexagon/mm/strnlen_user.S deleted +0 ~~-126~~

126 lines not on this page · raw message

Signature · 1 line

2.29.2

Christoph Hellwig

2021-05-17 06:16 UTC #4 lore raw

Re: [PATCH 3/6] [v2] hexagon: use generic strncpy/strnlen from_user

Quoted 8 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
Most per-architecture versions of these functions are broken in some form, and they are almost certainly slower than the generic code as well.
Remove the ones for hexagon and instead use the generic version. This custom version reads the data twice for strncpy() by doing an extra strnlen(), and it apparently lacks a check for user_addr_max().

I'd be tempted to just remove the first paragraph and reword the second as:

Remove the hexagon implementation of strncpy/strnlen and instead use the generic versions. The hexago version of strncpy reads the data twice by doing an extra strnlen(), and it apparently lacks a check for user_addr_max().

Arnd Bergmann

author

2021-05-17 06:46 UTC #5 lore raw

Re: [PATCH 3/6] [v2] hexagon: use generic strncpy/strnlen from_user

Quoted 11 lines · Christoph Hellwig

Quoted 8 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
Most per-architecture versions of these functions are broken in some form, and they are almost certainly slower than the generic code as well.
Remove the ones for hexagon and instead use the generic version. This custom version reads the data twice for strncpy() by doing an extra strnlen(), and it apparently lacks a check for user_addr_max().

Quoted 7 lines

I'd be tempted to just remove the first paragraph and reword the second as:
Remove the hexagon implementation of strncpy/strnlen and instead use the generic versions. The hexago version of strncpy reads the data twice by doing an extra strnlen(), and it apparently lacks a check for user_addr_max().

Changed to your version now, thanks!

Arnd

Arnd Bergmann

author

2021-05-15 10:18 UTC #6 lore raw

PATCH v2 4/6 arc: use generic strncpy/strnlen from_user

In reply to Arnd Bergmann

From: Arnd Bergmann <[email protected]>

Most per-architecture versions of these functions are broken in some form, and they are almost certainly slower than the generic code as well.

This version is fairly slow because it always does byte accesses even for aligned data, and its checks for user_addr_max() differ from the generic code.

Remove the ones for arc and instead use the generic version.

Signed-off-by Arnd Bergmann <[email protected]>

---
 arch/arc/Kconfig               |  2 +
 arch/arc/include/asm/uaccess.h | 83 ++--------------------------------
 arch/arc/mm/extable.c          | 12 -----
 3 files changed, 7 insertions(+), 90 deletions(-)

arch/arc/Kconfig +2 -0

index 2d98501c0897..a38f403a8811 100644
@@ -27,6 +27,8 @@ config ARC
 	select GENERIC_PENDING_IRQ if SMP
 	select GENERIC_SCHED_CLOCK
 	select GENERIC_SMP_IDLE_THREAD
+	select GENERIC_STRNCPY_FROM_USER
+	select GENERIC_STRNLEN_USER
 	select HAVE_ARCH_KGDB
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_ARCH_TRANSPARENT_HUGEPAGE if ARC_MMU_V4

arch/arc/include/asm/uaccess.h +5 ~~-78~~

101 lines not on this page · raw message

arch/arc/mm/extable.c +0 ~~-12~~

index b06b09ddf924..4e14c4244ea2 100644
@@ -32,16 +32,4 @@ unsigned long arc_clear_user_noinline(void __user *to,
 }
 EXPORT_SYMBOL(arc_clear_user_noinline);
 
-long arc_strncpy_from_user_noinline(char *dst, const char __user *src,
-		long count)
-{
-	return __arc_strncpy_from_user(dst, src, count);
-}
-EXPORT_SYMBOL(arc_strncpy_from_user_noinline);
-
-long arc_strnlen_user_noinline(const char __user *src, long n)
-{
-	return __arc_strnlen_user(src, n);
-}
-EXPORT_SYMBOL(arc_strnlen_user_noinline);
 #endif

Signature · 1 line

2.29.2

Christoph Hellwig

2021-05-17 06:16 UTC #7 lore raw

Re: [PATCH 4/6] [v2] arc: use generic strncpy/strnlen from_user

Quoted 11 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
Most per-architecture versions of these functions are broken in some form, and they are almost certainly slower than the generic code as well.
This version is fairly slow because it always does byte accesses even for aligned data, and its checks for user_addr_max() differ from the generic code.
Remove the ones for arc and instead use the generic version.

Same comment as for hexaon before.

Arnd Bergmann

author

2021-05-17 06:47 UTC #8 lore raw

Re: [PATCH 4/6] [v2] arc: use generic strncpy/strnlen from_user

Quoted 8 lines · Christoph Hellwig

Quoted 6 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
Most per-architecture versions of these functions are broken in some form, and they are almost certainly slower than the generic code as well.

Quoted 7 lines

Quoted 5 lines
This version is fairly slow because it always does byte accesses even for aligned data, and its checks for user_addr_max() differ from the generic code.
Remove the ones for arc and instead use the generic version.
Same comment as for hexaon before.

Changed now.

Arnd

Arnd Bergmann

author

2021-05-15 10:18 UTC #9 lore raw

PATCH v2 5/6 asm-generic: uaccess: remove inline strncpy_from_user/strnlen_user

In reply to Arnd Bergmann

From: Arnd Bergmann <[email protected]>

Consolidate the asm-generic implementation with the library version that is used everywhere else.

These are the three versions for NOMMU kernels, which can in principle fall back to strncpy()/strnlen() as the version in asm-generic does, but this version is particularly inefficient when it scans the string one byte at a time twice. The generic version also lacks a check for user_addr_max(), but this is probably ok on NOMMU targets.

Signed-off-by Arnd Bergmann <[email protected]>

---
 arch/h8300/Kconfig            |  2 ++
 arch/m68k/Kconfig             |  4 +--
 arch/riscv/Kconfig            |  4 +--
 include/asm-generic/uaccess.h | 46 ++++++-----------------------------
 4 files changed, 14 insertions(+), 42 deletions(-)

arch/h8300/Kconfig +2 -0

index 3e3e0f16f7e0..53dfd2d47e0e 100644
@@ -11,6 +11,8 @@ config H8300
 	select GENERIC_IRQ_SHOW
 	select FRAME_POINTER
 	select GENERIC_CPU_DEVICES
+	select GENERIC_STRNCPY_FROM_USER
+	select GENERIC_STRNLEN_USER
 	select MODULES_USE_ELF_RELA
 	select COMMON_CLK
 	select ARCH_WANT_FRAME_POINTERS

arch/m68k/Kconfig +2 -2

index ed6b8050e3ed..5f1aafa7b2e2 100644
@@ -17,8 +17,8 @@ config M68K
 	select GENERIC_CPU_DEVICES
 	select GENERIC_IOMAP
 	select GENERIC_IRQ_SHOW
-	select GENERIC_STRNCPY_FROM_USER if MMU
-	select GENERIC_STRNLEN_USER if MMU
+	select GENERIC_STRNCPY_FROM_USER
+	select GENERIC_STRNLEN_USER
 	select HAVE_AOUT if MMU
 	select HAVE_ASM_MODVERSIONS
 	select HAVE_DEBUG_BUGVERBOSE

arch/riscv/Kconfig +2 -2

index a8ad8eb76120..ada7a2918c05 100644
@@ -55,8 +55,8 @@ config RISCV
 	select GENERIC_PTDUMP if MMU
 	select GENERIC_SCHED_CLOCK
 	select GENERIC_SMP_IDLE_THREAD
-	select GENERIC_STRNCPY_FROM_USER if MMU
-	select GENERIC_STRNLEN_USER if MMU
+	select GENERIC_STRNCPY_FROM_USER
+	select GENERIC_STRNLEN_USER
 	select GENERIC_TIME_VSYSCALL if MMU && 64BIT
 	select HANDLE_DOMAIN_IRQ
 	select HAVE_ARCH_AUDITSYSCALL

include/asm-generic/uaccess.h +8 ~~-38~~

62 lines not on this page · raw message

Signature · 1 line

2.29.2

Christoph Hellwig

2021-05-17 06:20 UTC #10 lore raw

Re: [PATCH 5/6] [v2] asm-generic: uaccess: remove inline strncpy_from_user/strnlen_user

Quoted 6 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
Consolidate the asm-generic implementation with the library version that is used everywhere else.
These are the three versions for NOMMU kernels,

I don't get the three versions part?

Review

Quoted 2 lines

(quoted diff) +2 -0

+	select GENERIC_STRNCPY_FROM_USER
+	select GENERIC_STRNLEN_USER

Given that most architetures select the generic version I wonder if it might be worth to add another patch to invert the logic so that architectures with their own implementation need to sekect a symbol.

include/asm-generic/uaccess.h clear_user(void __user *to, unsigned long n) · line 276

Quoted 2 lines

include/asm-generic/uaccess.h +2 -0

+extern long strncpy_from_user(char *dst, const char __user *src, long count);
+extern long strnlen_user(const char __user *src, long n);

No need for the extern here.

Arnd Bergmann

author

2021-05-17 07:27 UTC #11 lore raw

Re: [PATCH 5/6] [v2] asm-generic: uaccess: remove inline strncpy_from_user/strnlen_user

Quoted 10 lines · Christoph Hellwig

Quoted 6 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
Consolidate the asm-generic implementation with the library version that is used everywhere else.
These are the three versions for NOMMU kernels,
I don't get the three versions part?

Right, that was confusing. Rewording to

| The inline version is used on three NOMMU architectures and is
| particularly inefficient when it scans the string one byte at a time
| twice. It also lacks a check for user_addr_max(), but this is
| probably ok on NOMMU targets.
|
| Consolidate the asm-generic implementation with the library version
| that is used everywhere else.  This version is generalized enough to
| work efficiently on both MMU and NOMMU targets, and using the
| same code everywhere reduces the potential for subtle bugs.

Quoted 6 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+     select GENERIC_STRNCPY_FROM_USER
+     select GENERIC_STRNLEN_USER
Given that most architetures select the generic version I wonder if it might be worth to add another patch to invert the logic so that architectures with their own implementation need to sekect a symbol.

Done now, using 'CONFIG_ARCH_HAS_{STRNCPY_FROM,STRNLEN}_USER'.

There are still seven or eight architectures that provide their own though.

Quoted 4 lines

include/asm-generic/uaccess.h clear_user(void __user *to, unsigned long n) · line 276
Quoted 2 lines
include/asm-generic/uaccess.h +2 -0
+extern long strncpy_from_user(char *dst, const char __user *src, long count);
+extern long strnlen_user(const char __user *src, long n);
No need for the extern here.

Removed.

Arnd

Arnd Bergmann

author

2021-05-15 10:18 UTC #12 lore raw

PATCH v2 6/6 asm-generic: remove extra strn{cpy_from,len}_user declarations

In reply to Arnd Bergmann

From: Arnd Bergmann <[email protected]>

As these are now in asm-generic, it's no longer necessary to declare them in the architecture.

Signed-off-by Arnd Bergmann <[email protected]>

---
 arch/arc/include/asm/uaccess.h     | 5 -----
 arch/hexagon/include/asm/uaccess.h | 6 ------
 arch/um/include/asm/uaccess.h      | 5 +----
 3 files changed, 1 insertion(+), 15 deletions(-)

arch/arc/include/asm/uaccess.h +0 -5

index 754a23f26736..783bfdb3bfa3 100644
@@ -667,11 +667,6 @@ extern unsigned long arc_clear_user_noinline(void __user *to,
 #define __clear_user(d, n)		arc_clear_user_noinline(d, n)
 #endif
 
-extern long strncpy_from_user(char *dst, const char __user *src, long count);
-#define strncpy_from_user(d, s, n)	strncpy_from_user(d, s, n)
-extern long strnlen_user(const char __user *src, long n);
-#define strnlen_user(s, n)		strnlen_user(s, n)
-
 #include <asm/segment.h>
 #include <asm-generic/uaccess.h>

arch/hexagon/include/asm/uaccess.h +0 -6

index d950df12d8c5..ef5bfef8d490 100644
@@ -57,12 +57,6 @@ unsigned long raw_copy_to_user(void __user *to, const void *from,
 __kernel_size_t __clear_user_hexagon(void __user *dest, unsigned long count);
 #define __clear_user(a, s) __clear_user_hexagon((a), (s))
 
-extern long strnlen_user(const char __user *src, long n);
-#define strnlen_user strnlen_user
-
-extern long strncpy_from_user(char *dst, const char __user *src, long n)
-#define strncpy_from_user strncpy_from_user
-
 #include <asm-generic/uaccess.h>

arch/um/include/asm/uaccess.h +1 -4

index 3bf209f683f8..191ef36dd543 100644
@@ -23,16 +23,13 @@
 
 extern unsigned long raw_copy_from_user(void *to, const void __user *from, unsigned long n);
 extern unsigned long raw_copy_to_user(void __user *to, const void *from, unsigned long n);
-extern long strncpy_from_user(char *dst, const char __user *src, long count);
-extern long strnlen_user(const void __user *str, long len);
 extern unsigned long __clear_user(void __user *mem, unsigned long len);
 static inline int __access_ok(unsigned long addr, unsigned long size);
 
 /* Teach asm-generic/uaccess.h that we have C functions for these. */
 #define __access_ok __access_ok
 #define __clear_user __clear_user
-#define strnlen_user strnlen_user
-#define strncpy_from_user strncpy_from_user
+
 #define INLINE_COPY_FROM_USER
 #define INLINE_COPY_TO_USER

Signature · 1 line

2.29.2