Re: slub - extended kmalloc redzone and dma alignment

[Petr Tesarik <ptesarik@suse.com>]

On Tue, 8 Apr 2025 16:07:19 +0100
Catalin Marinas <catalin.marinas@arm.com> wrote:

> On Tue, Apr 08, 2025 at 07:27:32AM +0200, Petr Tesarik wrote:
> > On Mon, 7 Apr 2025 18:12:09 +0100
> > Catalin Marinas <catalin.marinas@arm.com> wrote:  
> > > Thanks for looping me in. I'm just catching up with this thread.
> > > 
> > > On Mon, Apr 07, 2025 at 09:54:41AM +0200, Vlastimil Babka wrote:  
> > > > On 4/7/25 09:21, Feng Tang wrote:    
> > > > > On Sun, Apr 06, 2025 at 10:02:40PM +0800, Feng Tang wrote:
> > > > > [...]    
> > > > >> > I can remember this series, as well as my confusion why
> > > > >> > 192-byte kmalloc caches were missing on arm64.
> > > > >> > 
> > > > >> > Nevertheless, I believe ARCH_DMA_MINALIGN is required to avoid
> > > > >> > putting a DMA buffer on the same cache line as some other data
> > > > >> > that might be _written_ by the CPU while the corresponding
> > > > >> > main memory is modified by another bus-mastering device.
> > > > >> > 
> > > > >> > Consider this layout:
> > > > >> > 
> > > > >> >  ... | DMA buffer | other data | ...
> > > > >> >      ^                         ^
> > > > >> >      +-------------------------+-- cache line boundaries
> > > > >> > 
> > > > >> > When you prepare for DMA, you make sure that the DMA buffer is
> > > > >> > not cached by the CPU, so you flush the cache line (from all
> > > > >> > levels). Then you tell the device to write into the DMA
> > > > >> > buffer. However, before the device finishes the DMA
> > > > >> > transaction, the CPU accesses "other data", loading this cache
> > > > >> > line from main memory with partial results. Worse, if the CPU
> > > > >> > writes to "other data", it may write the cache line back into
> > > > >> > main memory, racing with the device writing to DMA buffer, and
> > > > >> > you end up with corrupted data in DMA buffer.    
> > > 
> > > Yes, cache evictions from 'other data; can override the DMA. Another
> > > problem, when the DMA completed, the kernel does a cache invalidation
> > > to remove any speculatively loaded cache lines from the DMA buffer
> > > but that would also invalidate 'other data', potentially corrupting
> > > it if it was dirty.
> > > 
> > > So it's not safe to have DMA into buffers less than ARCH_DMA_MINALIGN
> > > (and unaligned).  
> > 
> > It's not safe to DMA into buffers that share a CPU cache line with other
> > data, which could be before or after the DMA buffer, of course.  
>[...]
> While I think we are ok for arm64, other architectures may invalidate
> the caches in the arch_sync_dma_for_device() which could discard the red
> zone data. A quick grep for arch_sync_dma_for_device() shows several
> architectures invalidating the caches in the FROM_DEVICE case.

Wait. This sounds broken for partial writes into the DMA buffer, i.e.
where only part of the buffer is updated by a bus-mastering device.
When I worked on swiotlb, I was told that such partial updates must
be supported, and that's why the initial swiotlb_bounce() cannot be
removed from swiotlb_tbl_map_single(). In fact, the comment says:

	/*
	 * When the device is writing memory, i.e. dir == DMA_FROM_DEVICE, copy
	 * the original buffer to the TLB buffer before initiating DMA in order
	 * to preserve the original's data if the device does a partial write,
	 * i.e. if the device doesn't overwrite the entire buffer.  Preserving
	 * the original data, even if it's garbage, is necessary to match
	 * hardware behavior.  Use of swiotlb is supposed to be transparent,
	 * i.e. swiotlb must not corrupt memory by clobbering unwritten bytes.
	 */

You may want to check commit ddbd89deb7d3 ("swiotlb: fix info leak with
DMA_FROM_DEVICE"), commit aa6f8dcbab47 ("swiotlb: rework "fix info leak
with DMA_FROM_DEVICE") and commit 1132a1dc053e ("swiotlb: rewrite
comment explaining why the source is preserved on DMA_FROM_DEVICE").

I believe there is potential for a nasty race condition, and maybe even
info leak. Consider this:

1. DMA buffer is allocated by kmalloc(). The memory area previously
   contained sensitive information, which had been written to main
   memory.
2. The DMA buffer is initialized with zeroes, but this new content
   stays in a CPU cache (because this is kernel memory with a write
   behind cache policy).
3. DMA is set up, but nothing is written to main memory by the
   bus-mastering device.
4. The CPU cache line is now discarded in arch_sync_dma_for_cpu().

IIUC the zeroes were never written to main memory, and previous content
can now be read by the CPU through the DMA buffer.

I haven't checked if any architecture is affected, but I strongly
believe that the CPU cache MUST be flushed both before and after the
DMA transfer. Any architecture which does not do it that way should be
fixed.

Or did I miss a crucial detail (again)?

> > > What I did with reducing the minimum kmalloc()
> > > alignment was to force bouncing via swiotlb if the size passed to
> > > the DMA API is small. It may end up bouncing buffers that did not
> > > originate from kmalloc() or have proper alignment (with padding)
> > > but that's some heuristics we were willing to accept to be able
> > > to use small kmalloc() caches on arm64 - see
> > > dma_kmalloc_needs_bounce().
> > > 
> > > Does redzoning apply to kmalloc() or kmem_cache_create() (or
> > > both)? I haven't checked yet but if the red zone is within
> > > ARCH_DMA_MINALIGN (or rather dma_get_cache_alignment()), we could
> > > have issues with either corrupting the DMA buffer or the red
> > > zone. [...]  
> > 
> > I'm sorry if I'm being thick, but IIUC the red zone does not have
> > to be protected. Yes, we might miss red zone corruption if it
> > happens to race with a DMA transaction, but I have assumed that
> > this is permissible. I regard the red zone as a useful debugging
> > tool, not a safety measure that is guaranteed to detect any write
> > beyond the buffer end.  
> 
> Yeah, it's debugging, but too many false positives make the feature
> pretty useless.

Agreed. I was defending only sporadic false negatives. If there is room
for false positives, that's a no-go.

> > > > > I'm not familiar with DMA stuff, but Vlastimil's idea does make it
> > > > > easier for driver developer to write a driver to be used on
> > > > > different ARCHs, which have different DMA alignment requirement.
> > > > > Say if the minimal safe size is 8 bytes, the driver can just
> > > > > request 8 bytes and ARCH_DMA_MINALIGN will automatically chose
> > > > > the right size for it, which can save memory for ARCHs with
> > > > > smaller alignment requirement. Meanwhile it does sacrifice part
> > > > > of the redzone check ability, so I don't have preference here :)    
> > > > 
> > > > Let's clarify first who's expected to ensure the word alignment for
> > > > DMA, if it's not kmalloc() then I'd rather resist moving it there
> > > > :)    
> > > 
> > > In theory, the DMA API should handle the alignment as I tried to
> > > remove it from the kmalloc() code.  
> > 
> > Are we talking about the alignment of the starting address, or buffer
> > size, or both?  
> 
> The DMA API bouncing logic only checks the buffer size and assumes
> start/end would be aligned to kmalloc_size_roundup() with no valid data
> between them (FROM_DEVICE).

If that's correct behavior, then it must be documented, because other
places seem to disagree on the assumptions about prior content of a
DMA buffer (FROM_DEVICE). See the swiotlb comment above.

Petr T