Re: slub - extended kmalloc redzone and dma alignment

[Feng Tang <feng.tang@linux.alibaba.com>]

On Fri, Apr 04, 2025 at 03:53:03PM +0200, Petr Tesarik wrote:
> 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.

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")

Thanks,
Feng

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