Re: slub - extended kmalloc redzone and dma alignment

[Petr Tesarik <ptesarik@suse.com>]

On Mon, 7 Apr 2025 09:54:41 +0200
Vlastimil Babka <vbabka@suse.cz> 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.
> >> > 
> >> > But redzone poisoning should happen long before the DMA buffer cache
> >> > line is flushed. The device will not overwrite it unless it was given
> >> > wrong buffer length for the transaction, but then that would be a bug
> >> > that I'd rather detect.  
> >> 
> >> I alaso tend to think it's better for slub to detect these kind of DMA
> >> 'overflow'. We've added slub kunit test case for these in commmit 
> >> 6cd6d33ca41f ("mm/slub, kunit: Add a test case for kmalloc redzone check),
> >> which was inspired by a similar DMA related bug as described in
> >> commit 120ee599b5bf ("staging: octeon-usb: prevent memory corruption")  
> 
> OK so besides Petr's explanation that was about cache (in)coherency and is
> AFAIK tied to ARCH_DMA_MINALIGN, there is possibility of DMA that will
> really write garbage beyond the buffer that's not word aligned. Can we
> assume that this was really a bug in the usage and ensuring word alignment
> (not ARCH_DMA_MINALIGN alignment) is required from a different layer than
> kmalloc() itself? In that case it would be best to keep the reporting as it is.
>
> > 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 :)

I think it's clear. The granularity is mandated by the bus protocol.
It's not like the wires can add padding bits to a transaction by means
of magic. ;-) If a device becomes the bus master, a specific chip
drives the bus, and this chip must know about word size. In fact, the
hardware registers on that chip usually specify the transfer size in
units of bus words, not in bytes. And these hardware registers are
programmed by the corresponding device driver, so it must be the device
driver's job to ensure proper alignment.

Petr T