From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrzej Hajda Subject: Re: [PATCH] arm64/dma-mapping: fix DMA_ATTR_FORCE_CONTIGUOUS mmaping code Date: Thu, 30 Mar 2017 08:51:40 +0200 Message-ID: <45fde94b-9487-c28e-9d4e-3b36c4b4a934@samsung.com> References: <1490781926-6209-1-git-send-email-a.hajda@samsung.com> <15b1be13-625f-db74-d213-ad1df86f7eb5@arm.com> <073f1feb-d6da-c93f-8e67-0910befec27b@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-reply-to: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Robin Murphy , Catalin Marinas , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, Will Deacon , Geert Uytterhoeven , Bartlomiej Zolnierkiewicz List-Id: iommu@lists.linux-foundation.org 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 >>>> --- >>>> 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, >>>> >>> >>> > > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: a.hajda@samsung.com (Andrzej Hajda) Date: Thu, 30 Mar 2017 08:51:40 +0200 Subject: [PATCH] arm64/dma-mapping: fix DMA_ATTR_FORCE_CONTIGUOUS mmaping code In-Reply-To: References: <1490781926-6209-1-git-send-email-a.hajda@samsung.com> <15b1be13-625f-db74-d213-ad1df86f7eb5@arm.com> <073f1feb-d6da-c93f-8e67-0910befec27b@samsung.com> Message-ID: <45fde94b-9487-c28e-9d4e-3b36c4b4a934@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >>>> --- >>>> 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, >>>> >>> >>> > > >