linux-kernel - Re: [PATCH v3 1/2] x86, powerpc: Rename memcpy_mcsafe() to copy_mc_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87d06z7x1a.fsf@mpe.ellerman.id.au>
Date:   Wed, 20 May 2020 19:53:05 +1000
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Dan Williams <dan.j.williams@...el.com>, tglx@...utronix.de,
        mingo@...hat.com
Cc:     x86@...nel.org, stable@...r.kernel.org,
        Borislav Petkov <bp@...en8.de>,
        Tony Luck <tony.luck@...el.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Paul Mackerras <paulus@...ba.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Mikulas Patocka <mpatocka@...hat.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        linux-kernel@...r.kernel.org, linux-nvdimm@...ts.01.org
Subject: Re: [PATCH v3 1/2] x86, powerpc: Rename memcpy_mcsafe() to copy_mc_to_{user, kernel}()

Hi Dan,

Just a couple of minor things ...

Dan Williams <dan.j.williams@...el.com> writes:
> In reaction to a proposal to introduce a memcpy_mcsafe_fast()
> implementation Linus points out that memcpy_mcsafe() is poorly named
> relative to communicating the scope of the interface. Specifically what
> addresses are valid to pass as source, destination, and what faults /
> exceptions are handled. Of particular concern is that even though x86
> might be able to handle the semantics of copy_mc_to_user() with its
> common copy_user_generic() implementation other archs likely need / want
> an explicit path for this case:
...

> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 0969285996cb..dcbbcbf3552c 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -348,6 +348,32 @@ do {								\
>  extern unsigned long __copy_tofrom_user(void __user *to,
>  		const void __user *from, unsigned long size);
>  
> +#ifdef CONFIG_ARCH_HAS_COPY_MC
> +extern unsigned long __must_check

We try not to add extern in headers anymore.

> +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

Otherwise that looks fine.

...

> diff --git a/tools/testing/selftests/powerpc/copyloops/Makefile b/tools/testing/selftests/powerpc/copyloops/Makefile
> index 0917983a1c78..959817e7567c 100644
> --- a/tools/testing/selftests/powerpc/copyloops/Makefile
> +++ b/tools/testing/selftests/powerpc/copyloops/Makefile
> @@ -45,9 +45,9 @@ $(OUTPUT)/memcpy_p7_t%:	memcpy_power7.S $(EXTRA_SOURCES)
>  		-D SELFTEST_CASE=$(subst memcpy_p7_t,,$(notdir $@)) \
>  		-o $@ $^
>  
> -$(OUTPUT)/memcpy_mcsafe_64: memcpy_mcsafe_64.S $(EXTRA_SOURCES)
> +$(OUTPUT)/copy_mc: copy_mc.S $(EXTRA_SOURCES)
>  	$(CC) $(CPPFLAGS) $(CFLAGS) \
> -		-D COPY_LOOP=test_memcpy_mcsafe \
> +		-D COPY_LOOP=test_copy_mc \

This needs a fixup:

diff --git a/tools/testing/selftests/powerpc/copyloops/Makefile b/tools/testing/selftests/powerpc/copyloops/Makefile
index 959817e7567c..b4eb5c4c6858 100644
--- a/tools/testing/selftests/powerpc/copyloops/Makefile
+++ b/tools/testing/selftests/powerpc/copyloops/Makefile
@@ -47,7 +47,7 @@ $(OUTPUT)/memcpy_p7_t%:	memcpy_power7.S $(EXTRA_SOURCES)
 
 $(OUTPUT)/copy_mc: copy_mc.S $(EXTRA_SOURCES)
 	$(CC) $(CPPFLAGS) $(CFLAGS) \
-		-D COPY_LOOP=test_copy_mc \
+		-D COPY_LOOP=test_copy_mc_generic \
 		-o $@ $^
 
 $(OUTPUT)/copyuser_64_exc_t%: copyuser_64.S exc_validate.c ../harness.c \


>  		-o $@ $^
>  
>  $(OUTPUT)/copyuser_64_exc_t%: copyuser_64.S exc_validate.c ../harness.c \
> diff --git a/tools/testing/selftests/powerpc/copyloops/memcpy_mcsafe_64.S b/tools/testing/selftests/powerpc/copyloops/copy_mc.S
> similarity index 100%
> rename from tools/testing/selftests/powerpc/copyloops/memcpy_mcsafe_64.S
> rename to tools/testing/selftests/powerpc/copyloops/copy_mc.S

This file is a symlink to the file in arch/powerpc/lib, so the name of
the link needs updating, as well as the target.

Also is there a reason you dropped the "_64"? It would make most sense
to keep it I think, as then the file in selftests and the file in arch/
have the same name.

If you want to keep the copy_mc.S name it needs the diff below. Though
as I said I think it would be better to use copy_mc_64.S.

cheers


diff --git a/tools/testing/selftests/powerpc/copyloops/copy_mc.S b/tools/testing/selftests/powerpc/copyloops/copy_mc.S
new file mode 120000
index 000000000000..dcbe06d500fb
--- /dev/null
+++ b/tools/testing/selftests/powerpc/copyloops/copy_mc.S
@@ -0,0 +1 @@
+../../../../../arch/powerpc/lib/copy_mc_64.S
\ No newline at end of file
diff --git a/tools/testing/selftests/powerpc/copyloops/memcpy_mcsafe_64.S b/tools/testing/selftests/powerpc/copyloops/memcpy_mcsafe_64.S
deleted file mode 120000
index f0feef3062f6..000000000000
--- a/tools/testing/selftests/powerpc/copyloops/memcpy_mcsafe_64.S
+++ /dev/null
@@ -1 +0,0 @@
-../../../../../arch/powerpc/lib/memcpy_mcsafe_64.S
\ No newline at end of file
-- 
2.25.1