Re: [RFC][PATCH 05/14] ia64: csum_partial_copy_nocheck(): don't abuse csum_partial_copy_from_user()

Re: [RFC][PATCH 05/14] ia64: csum_partial_copy_nocheck(): don't abuse csum_partial_copy_from_user()

[Guenter Roeck]

On Wed, Jun 03, 2020 at 08:10:38PM +0100, Al Viro wrote:
> On Wed, Jun 03, 2020 at 08:37:14AM -0700, Guenter Roeck wrote:
> > On Fri, Mar 27, 2020 at 11:31:08PM +0000, Al Viro wrote:
> > > From: Al Viro <viro@zeniv.linux.org.uk>
> > > 
> > > Just inline the call and use memcpy() instead of __copy_from_user() and
> > > note that the tail is precisely ia64 csum_partial().
> > > 
> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > 
> > This patch results in:
> > 
> > arch/ia64/lib/csum_partial_copy.c: In function 'csum_partial_copy_nocheck':
> > arch/ia64/lib/csum_partial_copy.c:110:9: error: implicit declaration of function 'csum_partial'
> > 
> > for ia64:{defconfig, allnoconfig, tinyconfig}.
> 
> Argh...
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Yes, that does the trick.

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
> diff --git a/arch/ia64/lib/csum_partial_copy.c b/arch/ia64/lib/csum_partial_copy.c
> index 5d147a33d648..6e82e0be8040 100644
> --- a/arch/ia64/lib/csum_partial_copy.c
> +++ b/arch/ia64/lib/csum_partial_copy.c
> @@ -12,7 +12,7 @@
>  #include <linux/types.h>
>  #include <linux/string.h>
>  
> -#include <linux/uaccess.h>
> +#include <net/checksum.h>
>  
>  /*
>   * XXX Fixme: those 2 inlines are meant for debugging and will go away

[RFC][PATCHSET] uaccess: getting csum_and_copy_..._user() into saner shape

[Al Viro]

        In that area uaccess primitives actually used by the rest of the kernel
are csum_and_copy_{to,from}_user().  Currently we have a strange mix;
        * some architectures supply csum_and_copy_from_user().  That's
indicated by defining _HAVE_ARCH_COPY_AND_CSUM_FROM_USER
        * the rest end up using the wrapper from net/checksum.h instead;
that (fairly thin) wrapper expects to find csum_partial_copy_from_user().
Some among those have csum_partial_copy_from_user() that tries to be smart,
some end up picking a dumb one from lib/checksum.c, some have a literal
copy of that dumb version...
        * some architectures supply csum_and_copy_to_user().  Those define
HAVE_CSUM_COPY_USER.
        * the rest pick the dumb (inlined) implementation from net/checksum.h
        * to confuse the situation even more, there's a couple of architectures
that call their csum_and_copy_to_user() "csum_partial_copy_to_user" instead...
and have a macro defining csum_and_copy_to_user as csum_partial_copy_to_user.
        The rules for access_ok() location are also messy - in principle,
csum_partial_copy_from_user() expects to have access_ok() done in the wrapper,
but e.g. i386 one does that on its own.

        A saner approach would be to turn the csum_partial_copy_from_user()
instances into csum_and_copy_from_user() ones, getting rid of the wrapper
for those, then move the dumb one (also converted) into net/checksum.h.
The series below does that; it lives in
	git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #next.uaccess-4
based at #next.uaccess-3.  Individual patches in followups; please review.
Not sure which tree would that best go through, TBH...

Diffstat:
 arch/alpha/include/asm/checksum.h    |  3 ++-
 arch/alpha/lib/csum_partial_copy.c   |  6 +++---
 arch/arm/include/asm/checksum.h      | 14 ++++++++++++++
 arch/c6x/lib/checksum.c              | 22 ----------------------
 arch/ia64/include/asm/checksum.h     | 10 ----------
 arch/ia64/lib/csum_partial_copy.c    | 32 ++------------------------------
 arch/m68k/include/asm/checksum.h     |  3 ++-
 arch/m68k/lib/checksum.c             |  4 ++--
 arch/nios2/include/asm/checksum.h    |  2 --
 arch/parisc/include/asm/checksum.h   |  7 -------
 arch/parisc/lib/checksum.c           | 20 --------------------
 arch/s390/include/asm/checksum.h     | 19 -------------------
 arch/sh/include/asm/checksum_32.h    |  9 +++++++--
 arch/sparc/include/asm/checksum.h    |  1 +
 arch/sparc/include/asm/checksum_32.h | 15 ++++++++++-----
 arch/sparc/include/asm/checksum_64.h |  2 +-
 arch/x86/include/asm/checksum.h      |  2 ++
 arch/x86/include/asm/checksum_32.h   | 21 +++++++++++----------
 arch/x86/include/asm/checksum_64.h   | 12 ++----------
 arch/x86/lib/csum-wrappers_64.c      | 35 ++++++++++++++++++-----------------
 arch/x86/um/asm/checksum.h           | 20 --------------------
 arch/xtensa/include/asm/checksum.h   | 11 +++++++----
 include/asm-generic/checksum.h       |  9 ---------
 include/net/checksum.h               |  8 ++------
 lib/checksum.c                       | 20 --------------------
 25 files changed, 86 insertions(+), 221 deletions(-)

Shortlog:
      get rid of csum_partial_copy_to_user()
      x86_64: csum_..._copy_..._user(): switch to unsafe_..._user()
      x86: switch both 32bit and 64bit to providing csum_and_copy_from_user()
      x86: switch 32bit csum_and_copy_to_user() to user_access_{begin,end}()
      ia64: csum_partial_copy_nocheck(): don't abuse csum_partial_copy_from_user()
      ia64: turn csum_partial_copy_from_user() into csum_and_copy_from_user()
      alpha: turn csum_partial_copy_from_user() into csum_and_copy_from_user()
      parisc: turn csum_partial_copy_from_user() into csum_and_copy_from_user()
      sparc: switch to providing csum_and_copy_from_user()
      xtensa: switch to providing csum_and_copy_from_user()
      m68k: convert to csum_and_copy_from_user()
      sh32: convert to csum_and_copy_from_user()
      arm: switch to csum_and_copy_from_user()
      take the dummy csum_and_copy_from_user() into net/checksum.h

[RFC][PATCH 01/14] get rid of csum_partial_copy_to_user()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

For historical reasons some architectures call their csum_and_copy_to_user()
csum_partial_copy_to_user() instead (and supply a macro defining the
former as the latter).  That's the last remnants of old experiment that
went nowhere; time to bury them.  Rename those to csum_and_copy_to_user()
and get rid of the macros.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 arch/sparc/include/asm/checksum_32.h | 7 +++----
 arch/x86/include/asm/checksum_64.h   | 4 +---
 arch/x86/lib/csum-wrappers_64.c      | 6 +++---
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/arch/sparc/include/asm/checksum_32.h b/arch/sparc/include/asm/checksum_32.h
index 5fc98d80b03b..450ddfb444c8 100644
--- a/arch/sparc/include/asm/checksum_32.h
+++ b/arch/sparc/include/asm/checksum_32.h
@@ -83,8 +83,10 @@ csum_partial_copy_from_user(const void __user *src, void *dst, int len,
 	return (__force __wsum)ret;
 }
 
+#define HAVE_CSUM_COPY_USER
+
 static inline __wsum
-csum_partial_copy_to_user(const void *src, void __user *dst, int len,
+csum_and_copy_to_user(const void *src, void __user *dst, int len,
 			  __wsum sum, int *err)
 {
 	if (!access_ok(dst, len)) {
@@ -113,9 +115,6 @@ csum_partial_copy_to_user(const void *src, void __user *dst, int len,
 	}
 }
 
-#define HAVE_CSUM_COPY_USER
-#define csum_and_copy_to_user csum_partial_copy_to_user
-
 /* ihl is always 5 or greater, almost always is 5, and iph is word aligned
  * the majority of the time.
  */
diff --git a/arch/x86/include/asm/checksum_64.h b/arch/x86/include/asm/checksum_64.h
index 3ec6d3267cf9..ac9c06494827 100644
--- a/arch/x86/include/asm/checksum_64.h
+++ b/arch/x86/include/asm/checksum_64.h
@@ -141,13 +141,11 @@ extern __visible __wsum csum_partial_copy_generic(const void *src, const void *d
 
 extern __wsum csum_partial_copy_from_user(const void __user *src, void *dst,
 					  int len, __wsum isum, int *errp);
-extern __wsum csum_partial_copy_to_user(const void *src, void __user *dst,
+extern __wsum csum_and_copy_to_user(const void *src, void __user *dst,
 					int len, __wsum isum, int *errp);
 extern __wsum csum_partial_copy_nocheck(const void *src, void *dst,
 					int len, __wsum sum);
 
-/* Old names. To be removed. */
-#define csum_and_copy_to_user csum_partial_copy_to_user
 #define csum_and_copy_from_user csum_partial_copy_from_user
 
 /**
diff --git a/arch/x86/lib/csum-wrappers_64.c b/arch/x86/lib/csum-wrappers_64.c
index c66c8b00f236..875c2f5968a0 100644
--- a/arch/x86/lib/csum-wrappers_64.c
+++ b/arch/x86/lib/csum-wrappers_64.c
@@ -71,7 +71,7 @@ csum_partial_copy_from_user(const void __user *src, void *dst,
 EXPORT_SYMBOL(csum_partial_copy_from_user);
 
 /**
- * csum_partial_copy_to_user - Copy and checksum to user space.
+ * csum_and_copy_to_user - Copy and checksum to user space.
  * @src: source address
  * @dst: destination address (user space)
  * @len: number of bytes to be copied.
@@ -82,7 +82,7 @@ EXPORT_SYMBOL(csum_partial_copy_from_user);
  * src and dst are best aligned to 64bits.
  */
 __wsum
-csum_partial_copy_to_user(const void *src, void __user *dst,
+csum_and_copy_to_user(const void *src, void __user *dst,
 			  int len, __wsum isum, int *errp)
 {
 	__wsum ret;
@@ -116,7 +116,7 @@ csum_partial_copy_to_user(const void *src, void __user *dst,
 	clac();
 	return ret;
 }
-EXPORT_SYMBOL(csum_partial_copy_to_user);
+EXPORT_SYMBOL(csum_and_copy_to_user);
 
 /**
  * csum_partial_copy_nocheck - Copy and checksum.
-- 
2.11.0

[RFC][PATCH 05/14] ia64: csum_partial_copy_nocheck(): don't abuse csum_partial_copy_from_user()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

Just inline the call and use memcpy() instead of __copy_from_user() and
note that the tail is precisely ia64 csum_partial().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 arch/ia64/lib/csum_partial_copy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/lib/csum_partial_copy.c b/arch/ia64/lib/csum_partial_copy.c
index bf9396b1ed32..9ab570d0f756 100644
--- a/arch/ia64/lib/csum_partial_copy.c
+++ b/arch/ia64/lib/csum_partial_copy.c
@@ -134,8 +134,8 @@ EXPORT_SYMBOL(csum_partial_copy_from_user);
 __wsum
 csum_partial_copy_nocheck(const void *src, void *dst, int len, __wsum sum)
 {
-	return csum_partial_copy_from_user((__force const void __user *)src,
-					   dst, len, sum, NULL);
+	memcpy(dst, src, len);
+	return csum_partial(dst, len, sum);
 }
 
 EXPORT_SYMBOL(csum_partial_copy_nocheck);
-- 
2.11.0

Re: [RFC][PATCH 05/14] ia64: csum_partial_copy_nocheck(): don't abuse csum_partial_copy_from_user()

[Guenter Roeck]

On Fri, Mar 27, 2020 at 11:31:08PM +0000, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> Just inline the call and use memcpy() instead of __copy_from_user() and
> note that the tail is precisely ia64 csum_partial().
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

This patch results in:

arch/ia64/lib/csum_partial_copy.c: In function 'csum_partial_copy_nocheck':
arch/ia64/lib/csum_partial_copy.c:110:9: error: implicit declaration of function 'csum_partial'

for ia64:{defconfig, allnoconfig, tinyconfig}.

Guenter

---
# bad: [d6f9469a03d832dcd17041ed67774ffb5f3e73b3] Merge tag 'erofs-for-5.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs
# good: [b23c4771ff62de8ca9b5e4a2d64491b2fb6f8f69] Merge tag 'docs-5.8' of git://git.lwn.net/linux
git bisect start 'HEAD' 'b23c4771ff62'
# good: [6cf991611bc72c077f0cc64e23987341ad7ef41e] Merge tag 'drm-intel-next-2020-05-15' of git://anongit.freedesktop.org/drm/drm-intel into drm-next
git bisect good 6cf991611bc72c077f0cc64e23987341ad7ef41e
# bad: [faa392181a0bd42c5478175cef601adeecdc91b6] Merge tag 'drm-next-2020-06-02' of git://anongit.freedesktop.org/drm/drm
git bisect bad faa392181a0bd42c5478175cef601adeecdc91b6
# bad: [c5d6c13843880ad0112f0513f3eb041b258be66e] Merge tag 'mmc-v5.8' of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc
git bisect bad c5d6c13843880ad0112f0513f3eb041b258be66e
# bad: [94709049fb8442fb2f7b91fbec3c2897a75e18df] Merge branch 'akpm' (patches from Andrew)
git bisect bad 94709049fb8442fb2f7b91fbec3c2897a75e18df
# good: [a29adb6209cead1f6c34a8d72481fb183bfc2d68] mm: rename vmap_page_range to map_kernel_range
git bisect good a29adb6209cead1f6c34a8d72481fb183bfc2d68
# bad: [56446efab9ce4961fe0fe6bbc5bc66374b08b9f3] Merge branch 'uaccess.__copy_from_user' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
git bisect bad 56446efab9ce4961fe0fe6bbc5bc66374b08b9f3
# good: [87c233b8158a20a9c9ab1da96cb5cb1734d9006e] vmci_host: get rid of pointless access_ok()
git bisect good 87c233b8158a20a9c9ab1da96cb5cb1734d9006e
# bad: [001c1a655f0a4e4ebe5d9beb47466dc5c6ab4871] default csum_and_copy_to_user(): don't bother with access_ok()
git bisect bad 001c1a655f0a4e4ebe5d9beb47466dc5c6ab4871
# bad: [808b49da54e640cba5c5c92dee658018a529226b] alpha: turn csum_partial_copy_from_user() into csum_and_copy_from_user()
git bisect bad 808b49da54e640cba5c5c92dee658018a529226b
# good: [0a5ea224b2fdf9dca9291ef7b5a12fd846a5dc34] x86: switch both 32bit and 64bit to providing csum_and_copy_from_user()
git bisect good 0a5ea224b2fdf9dca9291ef7b5a12fd846a5dc34
# bad: [cc03f19cfd45f44a75f0445c5be0073bbd3dda1c] ia64: csum_partial_copy_nocheck(): don't abuse csum_partial_copy_from_user()
git bisect bad cc03f19cfd45f44a75f0445c5be0073bbd3dda1c
# good: [c281a6c1ac6b0867e4341ea801030fa9a62157f9] x86: switch 32bit csum_and_copy_to_user() to user_access_{begin,end}()
git bisect good c281a6c1ac6b0867e4341ea801030fa9a62157f9
# first bad commit: [cc03f19cfd45f44a75f0445c5be0073bbd3dda1c] ia64: csum_partial_copy_nocheck(): don't abuse csum_partial_copy_from_user()

Re: [RFC][PATCH 05/14] ia64: csum_partial_copy_nocheck(): don't abuse csum_partial_copy_from_user()

[Al Viro]

On Wed, Jun 03, 2020 at 08:37:14AM -0700, Guenter Roeck wrote:
> On Fri, Mar 27, 2020 at 11:31:08PM +0000, Al Viro wrote:
> > From: Al Viro <viro@zeniv.linux.org.uk>
> > 
> > Just inline the call and use memcpy() instead of __copy_from_user() and
> > note that the tail is precisely ia64 csum_partial().
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> This patch results in:
> 
> arch/ia64/lib/csum_partial_copy.c: In function 'csum_partial_copy_nocheck':
> arch/ia64/lib/csum_partial_copy.c:110:9: error: implicit declaration of function 'csum_partial'
> 
> for ia64:{defconfig, allnoconfig, tinyconfig}.

Argh...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/arch/ia64/lib/csum_partial_copy.c b/arch/ia64/lib/csum_partial_copy.c
index 5d147a33d648..6e82e0be8040 100644
--- a/arch/ia64/lib/csum_partial_copy.c
+++ b/arch/ia64/lib/csum_partial_copy.c
@@ -12,7 +12,7 @@
 #include <linux/types.h>
 #include <linux/string.h>
 
-#include <linux/uaccess.h>
+#include <net/checksum.h>
 
 /*
  * XXX Fixme: those 2 inlines are meant for debugging and will go away