Re: [PATCH] Fix folio conversion in __dma_page_dev_to_cpu()

[Robin Murphy <robin.murphy@arm.com>]

On 23/08/2023 8:18 pm, Matthew Wilcox (Oracle) wrote:
> Russell and Marek pointed out some assumptions I was making about how sg
> lists work; eg that they are limited to 2GB and that the initial offset
> lies within the first page (or at least within the first folio that a
> page belongs to).  While I think those assumptions are true, it's not
> too hard to write a version which does not have those assumptions and
> also calculates folio_size() only once per loop iteration.

FWIW, sg->offset > PAGE_SIZE has certainly been known to happen for 
quasi-legitimate reasons. Last time it came up[1], I think the 
conclusion in that case was that the crypto scatterwalk code wasn't 
unreasonably wrong, though could perhaps do better, but it was also 
straightforward enough to be robust against it within the DMA API so we 
should just do that anyway (commit 29a90b708938).

As for >2GB segments, we've certainly seen cases of users mapping 
absurdly large buffers and overflowing dma_length[2], so I would imagine 
it's only the improbability of allocating that much 
physically-contiguous memory which keeps individual segment lengths from 
getting up to UINT_MAX ;)

Cheers,
Robin.

[1] 
https://lore.kernel.org/linux-iommu/be3bb850-a9f2-61fa-e378-eb44489256e0@chelsio.com/
[2] 
https://lore.kernel.org/linux-iommu/fbdbb8c0e550ae559ea3eedc1fea084c0111f202.1564418681.git.robin.murphy@arm.com/

> ---
>   arch/arm/mm/dma-mapping.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 0474840224d9..5409225b4abc 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -695,7 +695,6 @@ static void __dma_page_cpu_to_dev(struct page *page, unsigned long off,
>   static void __dma_page_dev_to_cpu(struct page *page, unsigned long off,
>   	size_t size, enum dma_data_direction dir)
>   {
> -	struct folio *folio = page_folio(page);
>   	phys_addr_t paddr = page_to_phys(page) + off;
>   
>   	/* FIXME: non-speculating: not required */
> @@ -710,18 +709,19 @@ static void __dma_page_dev_to_cpu(struct page *page, unsigned long off,
>   	 * Mark the D-cache clean for these pages to avoid extra flushing.
>   	 */
>   	if (dir != DMA_TO_DEVICE && size >= PAGE_SIZE) {
> -		ssize_t left = size;
> +		struct folio *folio = pfn_folio(paddr / PAGE_SIZE);
>   		size_t offset = offset_in_folio(folio, paddr);
>   
> -		if (offset) {
> -			left -= folio_size(folio) - offset;
> -			folio = folio_next(folio);
> -		}
> +		for (;;) {
> +			size_t sz = folio_size(folio) - offset;
>   
> -		while (left >= (ssize_t)folio_size(folio)) {
> -			left -= folio_size(folio);
> -			set_bit(PG_dcache_clean, &folio->flags);
> -			if (!left)
> +			if (size < sz)
> +				break;
> +			if (!offset)
> +				set_bit(PG_dcache_clean, &folio->flags);
> +			offset = 0;
> +			size -= sz;
> +			if (!size)
>   				break;
>   			folio = folio_next(folio);
>   		}