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: <alpine.DEB.2.20.1610262110220.5013@nanos>
Date:   Wed, 26 Oct 2016 21:30:39 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Brian Boylston <brian.boylston@....com>
cc:     linux-nvdimm@...1.01.org, linux-kernel@...r.kernel.org,
        toshi.kani@....com, oliver.moreno@....com,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Al Viro <viro@...IV.linux.org.uk>,
        Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH v2 1/3] introduce memcpy_nocache()

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.

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

> +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 *)..

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.

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?

 * @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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ