Re: slub - extended kmalloc redzone and dma alignment

[Petr Tesarik <ptesarik@suse.com>]

On Fri, 4 Apr 2025 14:45:14 +0200
Vlastimil Babka <vbabka@suse.cz> wrote:

> On 4/4/25 13:12, Petr Tesarik wrote:
> > On Fri, 4 Apr 2025 19:30:09 +0900
> > Harry Yoo <harry.yoo@oracle.com> wrote:
> >   
> >> On Fri, Apr 04, 2025 at 11:30:49AM +0200, Vlastimil Babka wrote:  
> >> > Hi,
> >> > 
> >> > due to some off-list inquiry I have realized that since 946fa0dbf2d8
> >> > ("mm/slub: extend redzone check to extra allocated kmalloc space than
> >> > requested")
> >> > we might be reporting false positives due to dma writing into the redzone.
> >> > 
> >> > It wasn't confirmed (yet) during the conversation but AFAICS it can be
> >> > happening. We have this ARCH_DMA_MINALIGN and kmalloc() will guarantee it,
> >> > but the redzone check doesn't take it into account.    
> >> 
> >> Sounds valid to me.  
> > 
> > I'm not sure I understand your concerns.  
> 
> I'd be happy to be proven wrong and you're more familiar with DMA details
> than me :)
> 
> > Are you afraid that another device on the bus caches a copy of the
> > redzone before it was poisoned, so it overwrites the redzone with stale
> > data on a memory write operation? IMO that's buggy, because if a
> > bus-mastering device implements such cache, it is the device driver's
> > responsibility to flush it before starting a DMA transfer. FTR I'm not
> > aware of any such devices, except GPUs, but there's a whole lot to do
> > about CPU<->GPU coherency management, including device-specific ioctl's
> > to expose some gory details all the way down to userspace.  
> 
> OK, guess not that.
> 
> > Or are you concerned about bus data word size? I would again argue that
> > allocating a DMA buffer with a size that is not a multiple of the
> > transfer size is a bug. IOW the driver must make sure the buffer size
> > is a multiple of 4 if it is used for 32-bit DMA transfers, or a
> > multiple of 8 if it is used for 64-bit DMA transfers.  
> 
> Yeah I think it's that, and I thought drivers don't need to care themselves
> because ARCH_DMA_MINALIGN means kmalloc() layer provides that guarantee
> itself. I also remember this series (incidentally just recently the
> discussion was revived).
> 
> https://lore.kernel.org/all/20230612153201.554742-1-catalin.marinas@arm.com/

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.

@Catalin: I can see you're already in Cc. If I'm still missing
something, please, correct me.

Petr T