linux-kernel - RE: [PATCH v2 1/3] introduce memcpy

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]

Date:   Fri, 28 Oct 2016 01:52:47 +0000
From:   "Boylston, Brian" <brian.boylston@....com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     "linux-nvdimm@...1.01.org" <linux-nvdimm@...1.01.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Kani, Toshimitsu" <toshi.kani@....com>,
        "Moreno, Oliver" <oliver.moreno@....com>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        "x86@...nel.org" <x86@...nel.org>,
        Al Viro <viro@...IV.linux.org.uk>,
        Dan Williams <dan.j.williams@...el.com>,
        "boylston@...romesa.net" <boylston@...romesa.net>
Subject: RE: [PATCH v2 1/3] introduce memcpy_nocache()

Thomas Gleixner wrote on 2016-10-26:
> On Wed, 26 Oct 2016, Brian Boylston wrote:
>> --- a/arch/x86/include/asm/string_32.h
>> +++ b/arch/x86/include/asm/string_32.h
>> @@ -196,6 +196,9 @@ static inline void *__memcpy3d(void *to, const void *from, size_t len)
> 
>> +#define __HAVE_ARCH_MEMCPY_NOCACHE
>> +extern void *memcpy_nocache(void *dest, const void *src, size_t count);
> 
> Can we please move that prototype to linux/string.h instead of having it in
> both files and just define __HAVE_ARCH_MEMCPY_NOCACHE? And that define can
> move into x86/include/asm/string.h. There is no point in duplicating all
> that stuff.

Yes, sounds good.

> 
>> --- a/arch/x86/include/asm/string_64.h
>> +++ b/arch/x86/include/asm/string_64.h
>> @@ -51,6 +51,9 @@ extern void *__memcpy(void *to, const void *from, size_t len);
> 
>> +#define __HAVE_ARCH_MEMCPY_NOCACHE
>> +extern void *memcpy_nocache(void *dest, const void *src, size_t count)
> 
>> +#ifdef __HAVE_ARCH_MEMCPY_NOCACHE
> 
> You define __HAVE_ARCH_MEMCPY_NOCACHE for both 32 and 64 bit. What is
> this #ifdef for ?

Now that you raise the question, I'm not sure, and I see that the
x86 memcpy() definition isn't similarly wrapped.  I can adjust it.

> 
>> +void *memcpy_nocache(void *dest, const void *src, size_t count)
>> +{
>> +	__copy_from_user_inatomic_nocache(dest, src, count);
> 
> You want to replace a memcpy with this and then use copy from user. Why?
> That does not make any sense to me and even if it makes sense for whatever
> reason then this wants to be documented and the src argument needs a type
> cast to (void __user *)..

I agree that the documentation needs improvement.  The goal is to memcpy
while avoiding the processor cache.  Following x86's arch_memcpy_to_pmem(),
I was thinking that the user nocache implementation on x86 would work,
but I have none of the comments that arch_memcpy_to_pmem() has...

> 
> Further this uses the inatomic variant. Why? Does a memcpy replacement
> suddenly require to be called in atomic context? Again, this makes no sense
> and lacks any form of comment. The kernel doc below does not say anything
> about that.

I borrowed the use of the inatomic variant from arch_memcpy_to_pmem().

Perhaps I need to look at memcpy_to_pmem() as suggested by Boaz.

> 
> Neither provides the changelog any useful information which is
> complementary to what the patch actually does.
> 
>> +#ifndef __HAVE_ARCH_MEMCPY_NOCACHE
>> +/**
>> + * memcpy_nocache - Copy one area of memory to another, avoiding the
>> + * processor cache if possible
> 
> I'm not sure whether kerneldoc is happy about that line break. Did you
> build the docs?
> 
> Make the initial line shorter and explain the functionality in detail
> below the arguments
> 
>> + * @dest: Where to copy to
>> + * @src: Where to copy from
>> + * @count: The size of the area.
> 
> Nit. Can you please make this in tabular form for readability sake?

No, I did not try to build the docs.  I'll revisit this and your other
doc-related feedback.

Thanks!
Brian

> 
>  * @dest:	Where to copy to
>  * @src:	Where to copy from
>  * @count:	The size of the area.
>> + */
>> +static inline void *memcpy_nocache(void *dest, const void *src, size_t count)
>> +{
>> +	return memcpy(dest, src, count);
>> +}
>> +#endif
> 
> Thanks,
> 
> 	tglx