linux-kernel - Re: [PATCH 08/13] x86, libnvdimm, dax: stop abusing __copy_user

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: <20170328162135.GA27993@linux.intel.com>
Date:   Tue, 28 Mar 2017 10:21:35 -0600
From:   Ross Zwisler <ross.zwisler@...ux.intel.com>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     linux-nvdimm@...ts.01.org, Tony Luck <tony.luck@...el.com>,
        Jan Kara <jack@...e.cz>, Toshi Kani <toshi.kani@....com>,
        Matthew Wilcox <mawilcox@...rosoft.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, Christoph Hellwig <hch@....de>,
        Brian Boylston <brian.boylston@....com>,
        Jeff Moyer <jmoyer@...hat.com>, Ingo Molnar <mingo@...hat.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        "H. Peter Anvin" <hpa@...or.com>, linux-fsdevel@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>
Subject: Re: [PATCH 08/13] x86, libnvdimm, dax: stop abusing
 __copy_user_nocache

On Thu, Jan 19, 2017 at 07:50:56PM -0800, Dan Williams wrote:
<>
> diff --git a/drivers/nvdimm/x86-asm.S b/drivers/nvdimm/x86-asm.S
> new file mode 100644
> index 000000000000..23c5ec94e896
> --- /dev/null
> +++ b/drivers/nvdimm/x86-asm.S
> @@ -0,0 +1,71 @@
> +/*
> + * Copyright (c) 2017, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +#include <linux/linkage.h>
> +
> +/*
> + * __arch_memcpy_to_pmem - non-temporal + unordered memory copy
> + *
> + * 8-byte alignment for destination, source, and len. The results of
> + * this transfer are not persistent or globally visible until a
> + * sub-sequent sfence (REQ_FLUSH) to the pmem driver.
> + *
> + * Derived from __copy_user_nocache.
> + */
> +ENTRY(__arch_memcpy_to_pmem)
> +	/* Set 4x8-byte copy count and remainder */
> +	movl %edx,%ecx
> +	andl $63,%edx
> +	shrl $6,%ecx
> +	jz .L_8b_pmem_copy_entry	/* jump if count is 0 */
> +
> +	/* Perform 4x8-byte pmem loop-copy */
> +.L_4x8b_pmem_copy_loop:
> +	movq (%rsi),%r8
> +	movq 1*8(%rsi),%r9
> +	movq 2*8(%rsi),%r10
> +	movq 3*8(%rsi),%r11
> +	movnti %r8,(%rdi)
> +	movnti %r9,1*8(%rdi)
> +	movnti %r10,2*8(%rdi)
> +	movnti %r11,3*8(%rdi)
> +	movq 4*8(%rsi),%r8
> +	movq 5*8(%rsi),%r9
> +	movq 6*8(%rsi),%r10
> +	movq 7*8(%rsi),%r11
> +	movnti %r8,4*8(%rdi)
> +	movnti %r9,5*8(%rdi)
> +	movnti %r10,6*8(%rdi)
> +	movnti %r11,7*8(%rdi)
> +	leaq 64(%rsi),%rsi
> +	leaq 64(%rdi),%rdi
> +	decl %ecx
> +	jnz .L_4x8b_pmem_copy_loop
> +
> +	/* Set 8-byte copy count and remainder */
> +.L_8b_pmem_copy_entry:
> +	movl %edx,%ecx
> +	andl $7,%edx

I don't think you need to andl %edx here - in __copy_user_nocache() %edx was
used to keep the remaining count that couldn't be handled with the size of
transfers we were doing in a given loop, so in .L_8b_nocache_copy_entry we
mask with 7 so we can use the remaining count in .L_4b_nocache_copy_entry
and/or .L_1b_cache_copy_entry.

In the PMEM case, though, the 8 byte loop is the end of the line, so we just
ignore any trailing data that isn't 8 byte aligned.

I'm not sure if it's important to use %ecx as your local loop variable - is
this a widely held convention?  If not, you could just leave %ecx out of it
and use %edx directly in the 8 byte copy, i.e.:

        /* Set 8-byte copy count and remainder */
.L_8b_pmem_copy_entry:
        shrl $3,%edx
        jnz .L_8b_pmem_copy_loop /* continue if count non-zero */
        ret

        /* Perform 8-byte pmem loop-copy */
.L_8b_pmem_copy_loop:
        movq (%rsi),%r8
        movnti %r8,(%rdi)
        leaq 8(%rsi),%rsi
        leaq 8(%rdi),%rdi
        decl %edx
        jnz .L_8b_pmem_copy_loop
        ret
ENDPROC(__arch_memcpy_to_pmem)