Re: [PATCH] arm64/dma-mapping: fix DMA_ATTR_FORCE_CONTIGUOUS mmaping code

[Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>]

On 29.03.2017 14:55, Robin Murphy wrote:
> On 29/03/17 11:05, Andrzej Hajda wrote:
>> In case of DMA_ATTR_FORCE_CONTIGUOUS allocations vm_area->pages
>> is invalid. __iommu_mmap_attrs and __iommu_get_sgtable cannot use
>> it. In first case temporary pages array is passed to iommu_dma_mmap,
>> in 2nd case single entry sg table is created directly instead
>> of calling helper.
>>
>> Fixes: 44176bb ("arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU")
>> Signed-off-by: Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>> Hi,
>>
>> I am not familiar with this framework so please don't be too cruel ;)
>> Alternative solution I see is to always create vm_area->pages,
>> I do not know which approach is preferred.
>>
>> Regards
>> Andrzej
>> ---
>>  arch/arm64/mm/dma-mapping.c | 40 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>> index f7b5401..bba2bc8 100644
>> --- a/arch/arm64/mm/dma-mapping.c
>> +++ b/arch/arm64/mm/dma-mapping.c
>> @@ -704,7 +704,30 @@ static int __iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
>>  		return ret;
>>  
>>  	area = find_vm_area(cpu_addr);
>> -	if (WARN_ON(!area || !area->pages))
>> +	if (WARN_ON(!area))
> >From the look of things, it doesn't seem strictly necessary to change
> this, but whether that's a good thing is another matter. I'm not sure
> that dma_common_contiguous_remap() should really be leaving a dangling
> pointer in area->pages as it apparently does... :/
>
>> +		return -ENXIO;
>> +
>> +	if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) {
>> +		struct page *page = vmalloc_to_page(cpu_addr);
>> +		unsigned int count = size >> PAGE_SHIFT;
>> +		struct page **pages;
>> +		unsigned long pfn;
>> +		int i;
>> +
>> +		pages = kmalloc_array(count, sizeof(*pages), GFP_KERNEL);
>> +		if (!pages)
>> +			return -ENOMEM;
>> +
>> +		for (i = 0, pfn = page_to_pfn(page); i < count; i++)
>> +			pages[i] = pfn_to_page(pfn + i);
>> +
>> +		ret = iommu_dma_mmap(pages, size, vma);
> /**
>  * iommu_dma_mmap - Map a buffer into provided user VMA
>  * @pages: Array representing buffer from iommu_dma_alloc()
>    ...
>
> In this case, the buffer has not come from iommu_dma_alloc(), so passing
> into iommu_dma_mmap() is wrong by contract, even if having to fake up a
> page array wasn't enough of a red flag. Given that a FORCE_CONTIGUOUS
> allocation is essentially the same as for the non-IOMMU case, I think it
> should be sufficient to defer to that path, i.e.:
>
> 	if (attrs & DMA_ATTR_FORCE_CONTIGUOUS)
> 		return __swiotlb_mmap(dev, vma, cpu_addr,
> 				phys_to_dma(virt_to_phys(cpu_addr)),
> 				size, attrs);

Maybe I have make mistake somewhere but it does not work, here and below
(hangs or crashes). I suppose it can be due to different address
translations, my patch uses
    page = vmalloc_to_page(cpu_addr).
And here we have:
    handle = phys_to_dma(dev, virt_to_phys(cpu_addr)); // in
__iommu_mmap_attrs
    page = phys_to_page(dma_to_phys(dev, handle)); // in
__swiotlb_get_sgtable
I guess it is similarly in __swiotlb_mmap.

Are these translations equivalent?


Regards
Andrzej

>> +		kfree(pages);
>> +
>> +		return ret;
>> +	}
>> +
>> +	if (WARN_ON(!area->pages))
>>  		return -ENXIO;
>>  
>>  	return iommu_dma_mmap(area->pages, size, vma);
>> @@ -717,7 +740,20 @@ static int __iommu_get_sgtable(struct device *dev, struct sg_table *sgt,
>>  	unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
>>  	struct vm_struct *area = find_vm_area(cpu_addr);
>>  
>> -	if (WARN_ON(!area || !area->pages))
>> +	if (WARN_ON(!area))
>> +		return -ENXIO;
>> +
>> +	if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) {
>> +		int ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
>> +
>> +		if (!ret)
>> +			sg_set_page(sgt->sgl, vmalloc_to_page(cpu_addr),
>> +				PAGE_ALIGN(size), 0);
>> +
>> +		return ret;
>> +	}
> As above, this may as well just go straight to the non-IOMMU version,
> although I agree with Russell's concerns[1] that in general is is not
> safe to assume a coherent allocation is backed by struct pages at all.
>
> Robin.
>
> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2017-March/496068.html
>
>> +
>> +	if (WARN_ON(!area->pages))
>>  		return -ENXIO;
>>  
>>  	return sg_alloc_table_from_pages(sgt, area->pages, count, 0, size,
>>
>
>
>