Message-ID: <c235493f-c28d-43cf-969f-0fb148cb5dda@arm.com>
Date: Thu, 1 Feb 2024 13:30:15 +0000
From: Robin Murphy <robin.murphy@....com>
To: Will Deacon <will@...nel.org>
Cc: linux-kernel@...r.kernel.org, kernel-team@...roid.com,
 iommu@...ts.linux.dev, Christoph Hellwig <hch@....de>,
 Marek Szyprowski <m.szyprowski@...sung.com>,
 Petr Tesarik <petr.tesarik1@...wei-partners.com>,
 Dexuan Cui <decui@...rosoft.com>
Subject: Re: [PATCH v2 1/3] swiotlb: Fix allocation alignment requirement when
 searching slots

On 01/02/2024 12:46 pm, Will Deacon wrote:
> Hey Robin,
> 
> Cheers for having a look.
> 
> On Wed, Jan 31, 2024 at 03:54:03PM +0000, Robin Murphy wrote:
>> On 31/01/2024 12:25 pm, Will Deacon wrote:
>>> Commit bbb73a103fbb ("swiotlb: fix a braino in the alignment check fix"),
>>> which was a fix for commit 0eee5ae10256 ("swiotlb: fix slot alignment
>>> checks"), causes a functional regression with vsock in a virtual machine
>>> using bouncing via a restricted DMA SWIOTLB pool.
>>>
>>> When virtio allocates the virtqueues for the vsock device using
>>> dma_alloc_coherent(), the SWIOTLB search fails to take into account the
>>> 8KiB buffer size and returns page-unaligned allocations if 'area->index'
>>> was left unaligned by a previous allocation from the buffer:
>>
>> Hmm, but isn't this fundamentally swiotlb_alloc()'s fault for assuming it's
>> going to get a page-aligned address back despite asking for 0 alignment in
>> the first place? I'm not sure SWIOTLB has ever promised implicit
>> size-alignment, so it feels somewhat misplaced to be messing with the
>> algorithm before fixing the obvious issue in the caller :/
> 
> It's hard to tell which guarantees are intentional here given that this
> interface is all internal to swiotlb.c, but the 'alloc_align_mask'
> parameter didn't even exist prior to e81e99bacc9f ("swiotlb: Support
> aligned swiotlb buffers") and practically the implementation has ensured
> page-aligned allocations for buffers >= PAGE_SIZE prior to 0eee5ae10256
> ("swiotlb: fix slot alignment checks") by virtue of aligning the search
> index to the stride.
> 
> In any case, this patch is required because the current state of
> swiotlb_search_pool_area() conflates the DMA alignment mask, the
> allocation alignment mask and the stride so that even if a non-zero
> 'alloc_align_mask' is passed in, it won't necessarily be honoured.

Sure, I didn't mean to suggest there wasn't anything to fix here - if 
the existing code was intending to align to PAGE_SIZE even for a 
alloc_align_mask=0 and failing then that's clearly its own bug - I'm 
mostly being confused by the example of returning an unsuitably-aligned 
address for an 8KB dma_alloc_coherent() 75% of the time, if the end 
result of this fix is that we'll *still* return an incorrectly-aligned 
buffer for that same request 50% of the time (which just happens to be 
less fatal), since there are two separate bugs in that path.

Cheers,
Robin.

> 
> For example, I just gave it a spin with only patch #3 and then this log:
> 
>>>    # Final address in brackets is the SWIOTLB address returned to the caller
>>>    | virtio-pci 0000:00:07.0: orig_addr 0x0 alloc_size 0x2000, iotlb_align_mask 0x800 stride 0x2: got slot 1645-1649/7168 (0x98326800)
> 
> Becomes:
> 
>    | virtio-pci 0000:00:07.0: alloc_size 0x2000, iotlb_align_mask 0x1800 stride 0x4: got slot 1645-1649/7168 (0x98326800)
> 
> So even though the stride is correct, we still end up with a 2KiB aligned
> allocation.
> 
> Cheers,
> 
> Will

Powered by blists - more mailing lists