linux-kernel - Re: [PATCH v2] powerpc: warn on emulation of dcbz instruction in kernel mode

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: <20240822071443.GA6395@lst.de>
Date: Thu, 22 Aug 2024 09:14:43 +0200
From: Christoph Hellwig <hch@....de>
To: LEROY Christophe <christophe.leroy2@...soprasteria.com>
Cc: Christoph Hellwig <hch@....de>,
	Christian Lamparter <christian.lamparter@....uni-stuttgart.de>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Paul Mackerras <paulus@...ba.org>,
	Michael Ellerman <mpe@...erman.id.au>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	Stan Johnson <userm57@...oo.com>,
	Finn Thain <fthain@...ux-m68k.org>
Subject: Re: [PATCH v2] powerpc: warn on emulation of dcbz instruction in
 kernel mode

On Thu, Aug 22, 2024 at 06:39:33AM +0000, LEROY Christophe wrote:
> powerpc has a magic instruction 'dcbz' which clears a full cacheline in 
> one go. It is far more efficient than a loop to store zeros, and since 
> 2015 memset(0) has been implemented with that instruction (commit 
> 5b2a32e80634 ("powerpc/32: memset(0): use cacheable_memzero"))
> 
> But that instruction generates an alignment exception when used on 
> non-cached memory (whether it is RAM or not doesn't matter). It is then 
> emulated by the kernel but it of course leads to a serious performance 
> degradation, hence the warning added by commit cbe654c77961 ("powerpc: 
> warn on emulation of dcbz instruction in kernel mode"). Until now it 
> helped identify and fix use of memset() on IO memory.
> 
> But if memset() is expected to be used with non-cached RAM, then I don't 
> know what to do. Any suggestion ?

I'd suggest two things:

 1) remove the warning.  The use case is perfectly valid and everything
    using uncached memory is already slow, so people will just have to
    deal with it.  Maybe offer a trace point instead if people care about
    it.
 2) figure out a way to avoid this case in the dma-coherent allocator,
    which is probably the only case where it happens frequently
    (a few drivers also zero or re-zero coherent memory, but most of the
     time that is cargo cult programming and not actually needed)

For 2 I can think of two options:

 a) provide a arch hook for zeroing the dma memory that defaults to
    memset, but which powerpc can override
 a) figure out a way to clear the memory before marking it uncached
    if we can

a) it obviously easier to verify, but b) is probably going to give
   way better performance.

Below is an untested implementation of b) for dma-direct, I just need to
find out if there is any architecture that requires the memory to be
zeroed after it іt has been remapped.  The iommu drivers might also
need similar treatment.

diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 4480a3cd92e087..66e94b32ab0081 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -275,6 +275,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 		if (force_dma_unencrypted(dev))
 			prot = pgprot_decrypted(prot);
 
+		if (!PageHighMem(page))
+			memset(page_address(page), 0, size);
+
 		/* remove any dirty cache lines on the kernel alias */
 		arch_dma_prep_coherent(page, size);
 
@@ -283,14 +286,15 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 				__builtin_return_address(0));
 		if (!ret)
 			goto out_free_pages;
+		if (PageHighMem(page))
+			memset(ret, 0, size);
 	} else {
 		ret = page_address(page);
 		if (dma_set_decrypted(dev, ret, size))
 			goto out_leak_pages;
+		memset(ret, 0, size);
 	}
 
-	memset(ret, 0, size);
-
 	if (set_uncached) {
 		arch_dma_prep_coherent(page, size);
 		ret = arch_dma_set_uncached(ret, size);