[PATCH v2 0/3] Fix double allocation in swiotlb_alloc()

[PATCH v2 0/3] Fix double allocation in swiotlb_alloc()

[Will Deacon]

Hi all,

This is version two of the patches I posted last week:

https://lore.kernel.org/r/20240126151956.10014-1-will@kernel.org

Many thanks to Petr and Christoph for the discussion on that.

Changes since v1 include:

  - Fix swiotlb_alloc() to honour the alignment requirements of
    dma_alloc_coherent(). This is a new patch, and I think it's been
    broken forever (practically stopping at page alignment). I've left
    swiotlb_map() alone, so that doesn't necessarily return page-aligned
    DMA addresses, but I think that's ok.

  - Avoid updating 'alloc_align_mask' and instead just compute the
    'stride' directly to avoid a superfluous alignment requirement
    for mapping requests greater than a page.

  - Use get_max_slots() instead of open-coding the same logic.

  - Remove the extra 'goto' in swiotlb_search_pool_area() and collapse
    the conditionals instead.

  - Reword warning message when swiotlb_alloc() receives non page-aligned
    allocation.

  - Annotate non page-aligned case with unlikely().

Cheers,

Will

Cc: iommu@lists.linux.dev
Cc: Christoph Hellwig <hch@lst.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
Cc: Dexuan Cui <decui@microsoft.com>

--->8

Will Deacon (3):
  swiotlb: Fix allocation alignment requirement when searching slots
  swiotlb: Enforce page alignment in swiotlb_alloc()
  swiotlb: Honour dma_alloc_coherent() alignment in swiotlb_alloc()

 kernel/dma/swiotlb.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v2 1/3] swiotlb: Fix allocation alignment requirement when searching slots

[Will Deacon]

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:

 # 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)
 | virtio-pci 0000:00:07.0: orig_addr 0x0 alloc_size 0x2000, iotlb_align_mask 0x800 stride 0x2: got slot 1649-1653/7168 (0x98328800)
 | virtio-pci 0000:00:07.0: orig_addr 0x0 alloc_size 0x2000, iotlb_align_mask 0x800 stride 0x2: got slot 1653-1657/7168 (0x9832a800)

This ends in tears (typically buffer corruption and/or a hang) because
swiotlb_alloc() blindly returns a pointer to the 'struct page'
corresponding to the allocation and therefore the first half of the page
ends up being allocated twice.

Fix the problem by treating the allocation alignment separately to any
additional alignment requirements from the device, using the maximum
of the two as the stride to search the buffer slots.

Fixes: bbb73a103fbb ("swiotlb: fix a braino in the alignment check fix")
Fixes: 0eee5ae10256 ("swiotlb: fix slot alignment checks")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
Cc: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/dma/swiotlb.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index b079a9a8e087..56cc08b1fbd6 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -982,7 +982,7 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
 		phys_to_dma_unencrypted(dev, pool->start) & boundary_mask;
 	unsigned long max_slots = get_max_slots(boundary_mask);
 	unsigned int iotlb_align_mask =
-		dma_get_min_align_mask(dev) | alloc_align_mask;
+		dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);
 	unsigned int nslots = nr_slots(alloc_size), stride;
 	unsigned int offset = swiotlb_align_offset(dev, orig_addr);
 	unsigned int index, slots_checked, count = 0, i;
@@ -993,19 +993,18 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
 	BUG_ON(!nslots);
 	BUG_ON(area_index >= pool->nareas);
 
+	/*
+	 * For mappings with an alignment requirement don't bother looping to
+	 * unaligned slots once we found an aligned one.
+	 */
+	stride = get_max_slots(max(alloc_align_mask, iotlb_align_mask));
+
 	/*
 	 * For allocations of PAGE_SIZE or larger only look for page aligned
 	 * allocations.
 	 */
 	if (alloc_size >= PAGE_SIZE)
-		iotlb_align_mask |= ~PAGE_MASK;
-	iotlb_align_mask &= ~(IO_TLB_SIZE - 1);
-
-	/*
-	 * For mappings with an alignment requirement don't bother looping to
-	 * unaligned slots once we found an aligned one.
-	 */
-	stride = (iotlb_align_mask >> IO_TLB_SHIFT) + 1;
+		stride = max(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
 
 	spin_lock_irqsave(&area->lock, flags);
 	if (unlikely(nslots > pool->area_nslabs - area->used))
@@ -1015,14 +1014,16 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
 	index = area->index;
 
 	for (slots_checked = 0; slots_checked < pool->area_nslabs; ) {
-		slot_index = slot_base + index;
+		phys_addr_t tlb_addr;
 
-		if (orig_addr &&
-		    (slot_addr(tbl_dma_addr, slot_index) &
-		     iotlb_align_mask) != (orig_addr & iotlb_align_mask)) {
+		slot_index = slot_base + index;
+		tlb_addr = slot_addr(tbl_dma_addr, slot_index);
+
+		if ((tlb_addr & alloc_align_mask) ||
+		    (orig_addr && (tlb_addr & iotlb_align_mask) !=
+				  (orig_addr & iotlb_align_mask))) {
 			index = wrap_area_index(pool, index + 1);
 			slots_checked++;
-			continue;
 		}
 
 		if (!iommu_is_span_boundary(slot_index, nslots,
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v2 2/3] swiotlb: Enforce page alignment in swiotlb_alloc()

[Will Deacon]

When allocating pages from a restricted DMA pool in swiotlb_alloc(),
the buffer address is blindly converted to a 'struct page *' that is
returned to the caller. In the unlikely event of an allocation bug,
page-unaligned addresses are not detected and slots can silently be
double-allocated.

Add a simple check of the buffer alignment in swiotlb_alloc() to make
debugging a little easier if something has gone wonky.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
Cc: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/dma/swiotlb.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 56cc08b1fbd6..4485f216e620 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -1642,6 +1642,12 @@ struct page *swiotlb_alloc(struct device *dev, size_t size)
 		return NULL;
 
 	tlb_addr = slot_addr(pool->start, index);
+	if (unlikely(!PAGE_ALIGNED(tlb_addr))) {
+		dev_WARN_ONCE(dev, 1, "Cannot allocate pages from non page-aligned swiotlb addr 0x%pa.\n",
+			      &tlb_addr);
+		swiotlb_release_slots(dev, tlb_addr);
+		return NULL;
+	}
 
 	return pfn_to_page(PFN_DOWN(tlb_addr));
 }
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v2 3/3] swiotlb: Honour dma_alloc_coherent() alignment in swiotlb_alloc()

[Will Deacon]

core-api/dma-api-howto.rst states the following properties of
dma_alloc_coherent():

  | The CPU virtual address and the DMA address are both guaranteed to
  | be aligned to the smallest PAGE_SIZE order which is greater than or
  | equal to the requested size.

However, swiotlb_alloc() passes zero for the 'alloc_align_mask'
parameter of swiotlb_find_slots() and so this property is not upheld.
Instead, allocations larger than a page are aligned to PAGE_SIZE,

Calculate the mask corresponding to the page order suitable for holding
the allocation and pass that to swiotlb_find_slots().

Cc: Christoph Hellwig <hch@lst.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
Cc: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/dma/swiotlb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 4485f216e620..8ec37006ac70 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -1632,12 +1632,14 @@ struct page *swiotlb_alloc(struct device *dev, size_t size)
 	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
 	struct io_tlb_pool *pool;
 	phys_addr_t tlb_addr;
+	unsigned int align;
 	int index;
 
 	if (!mem)
 		return NULL;
 
-	index = swiotlb_find_slots(dev, 0, size, 0, &pool);
+	align = (1 << (get_order(size) + PAGE_SHIFT)) - 1;
+	index = swiotlb_find_slots(dev, 0, size, align, &pool);
 	if (index == -1)
 		return NULL;
 
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH v2 2/3] swiotlb: Enforce page alignment in swiotlb_alloc()

[Robin Murphy]

On 31/01/2024 12:25 pm, Will Deacon wrote:
> When allocating pages from a restricted DMA pool in swiotlb_alloc(),
> the buffer address is blindly converted to a 'struct page *' that is
> returned to the caller. In the unlikely event of an allocation bug,
> page-unaligned addresses are not detected and slots can silently be
> double-allocated.
> 
> Add a simple check of the buffer alignment in swiotlb_alloc() to make
> debugging a little easier if something has gone wonky.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> Cc: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>   kernel/dma/swiotlb.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 56cc08b1fbd6..4485f216e620 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -1642,6 +1642,12 @@ struct page *swiotlb_alloc(struct device *dev, size_t size)
>   		return NULL;
>   
>   	tlb_addr = slot_addr(pool->start, index);
> +	if (unlikely(!PAGE_ALIGNED(tlb_addr))) {
> +		dev_WARN_ONCE(dev, 1, "Cannot allocate pages from non page-aligned swiotlb addr 0x%pa.\n",
> +			      &tlb_addr);

Nit: if there's cause for another respin, I'd be inclined to use a 
straightforward "if (WARN_ON(...))" here - this condition should 
represent SWIOTLB itself going badly wrong, which isn't really 
attributable to whatever device happened to be involved in the call.

Cheers,
Robin.

> +		swiotlb_release_slots(dev, tlb_addr);
> +		return NULL;
> +	}
>   
>   	return pfn_to_page(PFN_DOWN(tlb_addr));
>   }

Re: [PATCH v2 1/3] swiotlb: Fix allocation alignment requirement when searching slots

[Robin Murphy]

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 :/

Cheers,
Robin.

>   # 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)
>   | virtio-pci 0000:00:07.0: orig_addr 0x0 alloc_size 0x2000, iotlb_align_mask 0x800 stride 0x2: got slot 1649-1653/7168 (0x98328800)
>   | virtio-pci 0000:00:07.0: orig_addr 0x0 alloc_size 0x2000, iotlb_align_mask 0x800 stride 0x2: got slot 1653-1657/7168 (0x9832a800)
> 
> This ends in tears (typically buffer corruption and/or a hang) because
> swiotlb_alloc() blindly returns a pointer to the 'struct page'
> corresponding to the allocation and therefore the first half of the page
> ends up being allocated twice.
> 
> Fix the problem by treating the allocation alignment separately to any
> additional alignment requirements from the device, using the maximum
> of the two as the stride to search the buffer slots.
> 
> Fixes: bbb73a103fbb ("swiotlb: fix a braino in the alignment check fix")
> Fixes: 0eee5ae10256 ("swiotlb: fix slot alignment checks")
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> Cc: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>   kernel/dma/swiotlb.c | 29 +++++++++++++++--------------
>   1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index b079a9a8e087..56cc08b1fbd6 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -982,7 +982,7 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>   		phys_to_dma_unencrypted(dev, pool->start) & boundary_mask;
>   	unsigned long max_slots = get_max_slots(boundary_mask);
>   	unsigned int iotlb_align_mask =
> -		dma_get_min_align_mask(dev) | alloc_align_mask;
> +		dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);
>   	unsigned int nslots = nr_slots(alloc_size), stride;
>   	unsigned int offset = swiotlb_align_offset(dev, orig_addr);
>   	unsigned int index, slots_checked, count = 0, i;
> @@ -993,19 +993,18 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>   	BUG_ON(!nslots);
>   	BUG_ON(area_index >= pool->nareas);
>   
> +	/*
> +	 * For mappings with an alignment requirement don't bother looping to
> +	 * unaligned slots once we found an aligned one.
> +	 */
> +	stride = get_max_slots(max(alloc_align_mask, iotlb_align_mask));
> +
>   	/*
>   	 * For allocations of PAGE_SIZE or larger only look for page aligned
>   	 * allocations.
>   	 */
>   	if (alloc_size >= PAGE_SIZE)
> -		iotlb_align_mask |= ~PAGE_MASK;
> -	iotlb_align_mask &= ~(IO_TLB_SIZE - 1);
> -
> -	/*
> -	 * For mappings with an alignment requirement don't bother looping to
> -	 * unaligned slots once we found an aligned one.
> -	 */
> -	stride = (iotlb_align_mask >> IO_TLB_SHIFT) + 1;
> +		stride = max(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
>   
>   	spin_lock_irqsave(&area->lock, flags);
>   	if (unlikely(nslots > pool->area_nslabs - area->used))
> @@ -1015,14 +1014,16 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>   	index = area->index;
>   
>   	for (slots_checked = 0; slots_checked < pool->area_nslabs; ) {
> -		slot_index = slot_base + index;
> +		phys_addr_t tlb_addr;
>   
> -		if (orig_addr &&
> -		    (slot_addr(tbl_dma_addr, slot_index) &
> -		     iotlb_align_mask) != (orig_addr & iotlb_align_mask)) {
> +		slot_index = slot_base + index;
> +		tlb_addr = slot_addr(tbl_dma_addr, slot_index);
> +
> +		if ((tlb_addr & alloc_align_mask) ||
> +		    (orig_addr && (tlb_addr & iotlb_align_mask) !=
> +				  (orig_addr & iotlb_align_mask))) {
>   			index = wrap_area_index(pool, index + 1);
>   			slots_checked++;
> -			continue;
>   		}
>   
>   		if (!iommu_is_span_boundary(slot_index, nslots,

Re: [PATCH v2 3/3] swiotlb: Honour dma_alloc_coherent() alignment in swiotlb_alloc()

[Robin Murphy]

On 31/01/2024 12:25 pm, Will Deacon wrote:
> core-api/dma-api-howto.rst states the following properties of
> dma_alloc_coherent():
> 
>    | The CPU virtual address and the DMA address are both guaranteed to
>    | be aligned to the smallest PAGE_SIZE order which is greater than or
>    | equal to the requested size.
> 
> However, swiotlb_alloc() passes zero for the 'alloc_align_mask'
> parameter of swiotlb_find_slots() and so this property is not upheld.
> Instead, allocations larger than a page are aligned to PAGE_SIZE,
> 
> Calculate the mask corresponding to the page order suitable for holding
> the allocation and pass that to swiotlb_find_slots().

I guess this goes back to at least e81e99bacc9f ("swiotlb: Support 
aligned swiotlb buffers") when the explicit argument was added - not 
sure what we do about 5.15 LTS though (unless the answer is to not care...)

As before, though, how much of patch #1 is needed if this comes first?

Cheers,
Robin.

> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> Cc: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>   kernel/dma/swiotlb.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 4485f216e620..8ec37006ac70 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -1632,12 +1632,14 @@ struct page *swiotlb_alloc(struct device *dev, size_t size)
>   	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
>   	struct io_tlb_pool *pool;
>   	phys_addr_t tlb_addr;
> +	unsigned int align;
>   	int index;
>   
>   	if (!mem)
>   		return NULL;
>   
> -	index = swiotlb_find_slots(dev, 0, size, 0, &pool);
> +	align = (1 << (get_order(size) + PAGE_SHIFT)) - 1;
> +	index = swiotlb_find_slots(dev, 0, size, align, &pool);
>   	if (index == -1)
>   		return NULL;
>   

Re: [PATCH v2 1/3] swiotlb: Fix allocation alignment requirement when searching slots

[Will Deacon]

On Wed, Jan 31, 2024 at 12:25:41PM +0000, Will Deacon wrote:
> @@ -1015,14 +1014,16 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>  	index = area->index;
>  
>  	for (slots_checked = 0; slots_checked < pool->area_nslabs; ) {
> -		slot_index = slot_base + index;
> +		phys_addr_t tlb_addr;
>  
> -		if (orig_addr &&
> -		    (slot_addr(tbl_dma_addr, slot_index) &
> -		     iotlb_align_mask) != (orig_addr & iotlb_align_mask)) {
> +		slot_index = slot_base + index;
> +		tlb_addr = slot_addr(tbl_dma_addr, slot_index);
> +
> +		if ((tlb_addr & alloc_align_mask) ||
> +		    (orig_addr && (tlb_addr & iotlb_align_mask) !=
> +				  (orig_addr & iotlb_align_mask))) {
>  			index = wrap_area_index(pool, index + 1);
>  			slots_checked++;
> -			continue;

Bah, I accidentally dropped this 'continue' when addressing the review
comments, so I'll add it back in v3.

Will

Re: [PATCH v2 1/3] swiotlb: Fix allocation alignment requirement when searching slots

[Will Deacon]

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.

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

Re: [PATCH v2 2/3] swiotlb: Enforce page alignment in swiotlb_alloc()

[Will Deacon]

On Wed, Jan 31, 2024 at 03:14:18PM +0000, Robin Murphy wrote:
> On 31/01/2024 12:25 pm, Will Deacon wrote:
> > When allocating pages from a restricted DMA pool in swiotlb_alloc(),
> > the buffer address is blindly converted to a 'struct page *' that is
> > returned to the caller. In the unlikely event of an allocation bug,
> > page-unaligned addresses are not detected and slots can silently be
> > double-allocated.
> > 
> > Add a simple check of the buffer alignment in swiotlb_alloc() to make
> > debugging a little easier if something has gone wonky.
> > 
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> > Cc: Dexuan Cui <decui@microsoft.com>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >   kernel/dma/swiotlb.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index 56cc08b1fbd6..4485f216e620 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -1642,6 +1642,12 @@ struct page *swiotlb_alloc(struct device *dev, size_t size)
> >   		return NULL;
> >   	tlb_addr = slot_addr(pool->start, index);
> > +	if (unlikely(!PAGE_ALIGNED(tlb_addr))) {
> > +		dev_WARN_ONCE(dev, 1, "Cannot allocate pages from non page-aligned swiotlb addr 0x%pa.\n",
> > +			      &tlb_addr);
> 
> Nit: if there's cause for another respin, I'd be inclined to use a
> straightforward "if (WARN_ON(...))" here - this condition should represent
> SWIOTLB itself going badly wrong, which isn't really attributable to
> whatever device happened to be involved in the call.

Well, there'll definitely be a v3 thanks to my idiotic dropping of the
'continue' statement when I reworked the searching loop for v2.

However, given that we're returning NULL, I think printing the device is
helpful as we're likely to cause some horrible error (e.g. probe failure)
in the caller and then it will be obvious why that happened from looking
at the logs. So I'd prefer to keep it unless you insist.

Cheers,

Will

Re: [PATCH v2 3/3] swiotlb: Honour dma_alloc_coherent() alignment in swiotlb_alloc()

[Will Deacon]

On Wed, Jan 31, 2024 at 04:03:38PM +0000, Robin Murphy wrote:
> On 31/01/2024 12:25 pm, Will Deacon wrote:
> > core-api/dma-api-howto.rst states the following properties of
> > dma_alloc_coherent():
> > 
> >    | The CPU virtual address and the DMA address are both guaranteed to
> >    | be aligned to the smallest PAGE_SIZE order which is greater than or
> >    | equal to the requested size.
> > 
> > However, swiotlb_alloc() passes zero for the 'alloc_align_mask'
> > parameter of swiotlb_find_slots() and so this property is not upheld.
> > Instead, allocations larger than a page are aligned to PAGE_SIZE,
> > 
> > Calculate the mask corresponding to the page order suitable for holding
> > the allocation and pass that to swiotlb_find_slots().
> 
> I guess this goes back to at least e81e99bacc9f ("swiotlb: Support aligned
> swiotlb buffers") when the explicit argument was added - not sure what we do
> about 5.15 LTS though (unless the answer is to not care...)

Thanks. I'll add the Fixes: tag but, to be honest, if we backport the first
patch then I'm not hugely fussed about this one in -stable kernels simply
because I spotted it my inspection rather than an real failure.

> As before, though, how much of patch #1 is needed if this comes first?

See my reply over there, but I think we need all of this.

Will

Re: [PATCH v2 1/3] swiotlb: Fix allocation alignment requirement when searching slots

[Robin Murphy]

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

Re: [PATCH v2 2/3] swiotlb: Enforce page alignment in swiotlb_alloc()

[Robin Murphy]

On 01/02/2024 12:48 pm, Will Deacon wrote:
> On Wed, Jan 31, 2024 at 03:14:18PM +0000, Robin Murphy wrote:
>> On 31/01/2024 12:25 pm, Will Deacon wrote:
>>> When allocating pages from a restricted DMA pool in swiotlb_alloc(),
>>> the buffer address is blindly converted to a 'struct page *' that is
>>> returned to the caller. In the unlikely event of an allocation bug,
>>> page-unaligned addresses are not detected and slots can silently be
>>> double-allocated.
>>>
>>> Add a simple check of the buffer alignment in swiotlb_alloc() to make
>>> debugging a little easier if something has gone wonky.
>>>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Cc: Robin Murphy <robin.murphy@arm.com>
>>> Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
>>> Cc: Dexuan Cui <decui@microsoft.com>
>>> Signed-off-by: Will Deacon <will@kernel.org>
>>> ---
>>>    kernel/dma/swiotlb.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
>>> index 56cc08b1fbd6..4485f216e620 100644
>>> --- a/kernel/dma/swiotlb.c
>>> +++ b/kernel/dma/swiotlb.c
>>> @@ -1642,6 +1642,12 @@ struct page *swiotlb_alloc(struct device *dev, size_t size)
>>>    		return NULL;
>>>    	tlb_addr = slot_addr(pool->start, index);
>>> +	if (unlikely(!PAGE_ALIGNED(tlb_addr))) {
>>> +		dev_WARN_ONCE(dev, 1, "Cannot allocate pages from non page-aligned swiotlb addr 0x%pa.\n",
>>> +			      &tlb_addr);
>>
>> Nit: if there's cause for another respin, I'd be inclined to use a
>> straightforward "if (WARN_ON(...))" here - this condition should represent
>> SWIOTLB itself going badly wrong, which isn't really attributable to
>> whatever device happened to be involved in the call.
> 
> Well, there'll definitely be a v3 thanks to my idiotic dropping of the
> 'continue' statement when I reworked the searching loop for v2.
> 
> However, given that we're returning NULL, I think printing the device is
> helpful as we're likely to cause some horrible error (e.g. probe failure)
> in the caller and then it will be obvious why that happened from looking
> at the logs. So I'd prefer to keep it unless you insist.

No, helping to clarify any knock-on effects sounds like a reasonable 
justification to me - I hadn't really considered that angle. I'd still 
be inclined to condense it down to "if (dev_WARN_ONCE(...))" to minimise 
redundancy, but that's really just a matter of personal preference.

Cheers,
Robin.

Re: [PATCH v2 1/3] swiotlb: Fix allocation alignment requirement when searching slots

[Will Deacon]

On Thu, Feb 01, 2024 at 01:30:15PM +0000, Robin Murphy wrote:
> On 01/02/2024 12:46 pm, Will Deacon wrote:
> > 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.

I'll have a go at improving the commit message a bit, since I wrote that
before I'd really appreciated that we weren't returning natural alignment
(and page-alignment seems to be sufficient for whatever vsock needs).

Thanks,

Will