linux-kernel - Re: [PATCH RFC 01/14] x86/asm: add iosubmit_cmds512() based on movdir64b CPU instruction

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:   Wed, 20 Nov 2019 13:50:24 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     Dave Jiang <dave.jiang@...el.com>, dmaengine@...r.kernel.org,
        linux-kernel@...r.kernel.org, vkoul@...nel.org
Cc:     dan.j.williams@...el.com, tony.luck@...el.com, jing.lin@...el.com,
        ashok.raj@...el.com, sanjay.k.kumar@...el.com, megha.dey@...el.com,
        jacob.jun.pan@...el.com, yi.l.liu@...el.com, axboe@...nel.dk,
        akpm@...ux-foundation.org, tglx@...utronix.de, mingo@...hat.com,
        bp@...en8.de, fenghua.yu@...el.com, hpa@...or.com
Subject: Re: [PATCH RFC 01/14] x86/asm: add iosubmit_cmds512() based on
 movdir64b CPU instruction

On 11/20/19 1:23 PM, Dave Jiang wrote:
> +static inline void __iowrite512(void __iomem *__dst, const void *src)
> +{
> +	volatile struct { char _[64]; } *dst = __dst;

This _looks_ like gibberish.  I know it's not, but it is subtle enough
that it really needs specific comments.

> +static inline void iosubmit_cmds512(void __iomem *dst, const void *src,
> +				    size_t count)
> +{
> +	const u8 *from = src;
> +	const u8 *end = from + count * 64;
> +
> +	if (!cpu_has_write512())
> +		return;
> +
> +	while (from < end) {
> +		__iowrite512(dst, from);
> +		from += 64;
> +	}
> +}

Won't this silently just drop things if the CPU doesn't have movdir64b
support?

It seems like this shouldn't be called at all if
!cpu_has_write512(), but wouldn't something like this be mroe appropriate?

	if (!cpu_has_write512()) {
		WARN_ON_ONCE(1);
		return;
	}

Is the caller just supposed to infer that "dst" was never overwritten?