Re: slub - extended kmalloc redzone and dma alignment

[Catalin Marinas <catalin.marinas@arm.com>]

Hi Vlastimil, Feng,

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). 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. Note that this only applies
to DMA_FROM_DEVICE or DMA_BIDIRECTIONAL.

> >> > 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.

dma_direct_map_page() for example bounces the DMA if it detects a small
size, just in case it came from kmalloc(). Similarly in the iommu code -
dev_use_sg_swiotlb().

> > 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.

With kmem_cache_create() (or kmalloc() as well), if the object size is
not cacheline-aligned, is there risk of redzoning around the object
without any alignment restrictions? The logic in
dma_kmalloc_size_aligned() would fail for sufficiently large buffers but
with unaligned red zone around the object. Not sure how to fix this
though in the DMA API. At least for kmem_cache_create() one can pass
SLAB_HWCACHE_ALIGN and it will force the alignment. I need to check what
redzoning does in this case (won't have time until tomorrow, I just got
back from holiday and lots of emails to go through).

-- 
Catalin