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

[Andrzej Hajda]

On 29.03.2017 17:33, Robin Murphy wrote:
> On 29/03/17 16:12, Andrzej Hajda wrote:
>> 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 at samsung.com>
>>>> ---
>>>> 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?
> Ah, my mistake, sorry - I managed to forget that cpu_addr is always
> remapped for FORCE_CONTIGUOUS (*and* somehow overlook the use of
> vmalloc_to_page() in the patch itself), so the virt_to_phys() part of my
> example is indeed bogus. The general point still stands, though.

I guess Geert's proposition to create pages permanently is also not
acceptable[2]. So how to fix crashes which appeared after patch adding
support to DMA_ATTR_FORCE_CONTIGUOUS in IOMMU on arm64 [1]?
Maybe temporary solution is to drop the patch until proper handling of
mmapping is proposed, without it the patch looks incomplete and causes
regression.
Moreover __iommu_alloc_attrs uses also dma_common_contiguous_remap which
also assumes existence of struct pages.

[1]: https://patchwork.kernel.org/patch/9609551/
[2]: https://www.spinics.net/lists/arm-kernel/msg572688.html

Regards
Andrzej


>
> Robin.
>
>>
>> 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,
>>>>
>>>
>>>
>
>
>