mbox series

[v3,0/3] Fix double allocation in swiotlb_alloc()

Message ID 20240205190127.20685-1-will@kernel.org
Headers
Series Fix double allocation in swiotlb_alloc() |

Message

Will Deacon Feb. 5, 2024, 7:01 p.m. UTC 
  Hi all,

This is version three of the patches I posted recently:

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

Thanks to Robin for the comments on the most recent version.

Changes since v2 include:

  - Restore missing 'continue' statement that got accidentally dropped
    while addressing the initial round of review feedback.

  - Reword the commit message in patch #1

  - Add a Fixes: tag to the last patch

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 double-allocation of slots due to broken alignment
    handling
  swiotlb: Enforce page alignment in swiotlb_alloc()
  swiotlb: Honour dma_alloc_coherent() alignment in swiotlb_alloc()

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

Comments

Christoph Hellwig Feb. 19, 2024, 6:35 a.m. UTC | #1 
Robin and Petr, does this looks good to you now?

On Mon, Feb 05, 2024 at 07:01:24PM +0000, Will Deacon wrote:
> Hi all,
> 
> This is version three of the patches I posted recently:
> 
> v1: https://lore.kernel.org/r/20240126151956.10014-1-will@kernel.org
> v2: https://lore.kernel.org/r/20240131122543.14791-1-will@kernel.org
> 
> Thanks to Robin for the comments on the most recent version.
> 
> Changes since v2 include:
> 
>   - Restore missing 'continue' statement that got accidentally dropped
>     while addressing the initial round of review feedback.
> 
>   - Reword the commit message in patch #1
> 
>   - Add a Fixes: tag to the last patch
> 
> 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 double-allocation of slots due to broken alignment
>     handling
>   swiotlb: Enforce page alignment in swiotlb_alloc()
>   swiotlb: Honour dma_alloc_coherent() alignment in swiotlb_alloc()
> 
>  kernel/dma/swiotlb.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> -- 
> 2.43.0.594.gd9cf4e227d-goog
---end quoted text---
Will Deacon Feb. 19, 2024, 9:24 a.m. UTC | #2 
Hey Christoph,

On Mon, Feb 19, 2024 at 07:35:27AM +0100, Christoph Hellwig wrote:
> Robin and Petr, does this looks good to you now?

FWIW, I'm likely to send a v4 addressing another issue reported by
Nicolin with NVME and 64k pages [1], so you may as well wait for that.

Cheers,

Will

[1] https://lkml.kernel.org/r/cover.1707851466.git.nicolinc@nvidia.com
Petr Tesařík Feb. 19, 2024, 12:40 p.m. UTC | #3 
On Mon, 19 Feb 2024 09:24:39 +0000
Will Deacon <will@kernel.org> wrote:

> Hey Christoph,
> 
> On Mon, Feb 19, 2024 at 07:35:27AM +0100, Christoph Hellwig wrote:
> > Robin and Petr, does this looks good to you now?  

It looks good to me for the boot-time swiotlb pool. Dynamic allocation
of transient swiotlb pools does not take these additional alignment
constraints into account, so when allocation may fail. OTOH the
underlying allocator(s) do not provide a suitable API, so I don't think
it's worth fixing.

In the worst case, a DMA buffer will fail to map, which may already
happen today.

> FWIW, I'm likely to send a v4 addressing another issue reported by
> Nicolin with NVME and 64k pages [1], so you may as well wait for that.

I'm interested. The code look quite OK to me as it is, but I assume it
again uncovers something when the difference between PAGE_SHIFT and
IOTLB_SHIFT is more than one (which was motivation for my initial fix,
which in the end broke more than it fixed).

Petr T