Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1623369 > unrolled thread

[PATCH] ARM: dma-mapping: add check for coherent DMA memory without struct page

Back to article view | Back to linux.kernel

Contents

  [PATCH] ARM: dma-mapping: add check for coherent DMA memory without struct page Shuah Khan <shuahkh@osg.samsung.com> - 2017-04-13 23:50 +0200
    Re: [PATCH] ARM: dma-mapping: add check for coherent DMA memory  without struct page Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-04-14 00:20 +0200
      Re: [PATCH] ARM: dma-mapping: add check for coherent DMA memory  without struct page Shuah Khan <shuahkh@osg.samsung.com> - 2017-04-14 01:50 +0200

#1623369 — [PATCH] ARM: dma-mapping: add check for coherent DMA memory without struct page

When coherent DMA memory without struct page is shared, importer
fails to find the page and runs into kernel page fault when it
tries to dmabuf_ops_attach/map_sg/map_page the invalid page found
in the sg_table.

Add a new dma_check_dev_coherent() interface to check if memory is
from the device coherent area. There is no way to tell where the
memory returned by dma_alloc_attrs() came from.

arm_dma_get_sgtable() checks for invalid pages, however this check
could pass even for memory obtained the coherent allocator. Add an
additional check to call dma_check_dev_coherent() to confirm that it
is indeed the coherent DMA memory and fail the sgtable creation with
-EINVAL.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 arch/arm/mm/dma-mapping.c   | 11 ++++++++---
 drivers/base/dma-coherent.c | 25 +++++++++++++++++++++++++
 include/linux/dma-mapping.h |  2 ++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 475811f..27c7d9a 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -954,9 +954,14 @@ int arm_dma_get_sgtable(struct device *dev, struct sg_table *sgt,
 	struct page *page;
 	int ret;
 
-	/* If the PFN is not valid, we do not have a struct page */
-	if (!pfn_valid(pfn))
-		return -ENXIO;
+	/*
+	 * If the PFN is not valid, we do not have a struct page
+	 * As this check can pass even for memory obtained through
+	 * the coherent allocator, do an additional check to determine
+	 * if this is coherent DMA memory.
+	 */
+	if (!pfn_valid(pfn) && dma_check_dev_coherent(dev, handle, cpu_addr))
+		return -EINVAL;
 
 	page = pfn_to_page(pfn);
 
diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c
index 640a7e6..d08cf44 100644
--- a/drivers/base/dma-coherent.c
+++ b/drivers/base/dma-coherent.c
@@ -209,6 +209,31 @@ int dma_alloc_from_coherent(struct device *dev, ssize_t size,
 EXPORT_SYMBOL(dma_alloc_from_coherent);
 
 /**
+ * dma_check_dev_coherent() - checks if memory is from the device coherent area
+ *
+ * @dev:	device whose coherent area is checked to validate memory
+ * @dma_handle:	dma handle associated with the allocated memory
+ * @vaddr:	the virtual address to the allocated area.
+ *
+ * Returns true if memory does belong to the per-device cohrent area.
+ * false otherwise.
+ */
+bool dma_check_dev_coherent(struct device *dev, dma_addr_t dma_handle,
+				void *vaddr)
+{
+	struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
+
+	if (mem && vaddr >= mem->virt_base &&
+	    vaddr < (mem->virt_base + (mem->size << PAGE_SHIFT)) &&
+	    dma_handle >= mem->device_base &&
+	    dma_handle < (mem->device_base + (mem->size << PAGE_SHIFT)))
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL(dma_check_dev_coherent);
+
+/**
  * dma_release_from_coherent() - try to free the memory allocated from per-device coherent memory pool
  * @dev:	device from which the memory was allocated
  * @order:	the order of pages allocated
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 0977317..b10e70d 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -160,6 +160,8 @@ static inline int is_device_dma_capable(struct device *dev)
  */
 int dma_alloc_from_coherent(struct device *dev, ssize_t size,
 				       dma_addr_t *dma_handle, void **ret);
+bool dma_check_dev_coherent(struct device *dev, dma_addr_t dma_handle,
+				void *vaddr);
 int dma_release_from_coherent(struct device *dev, int order, void *vaddr);
 
 int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
-- 
2.7.4

[toc] | [next] | [standalone]

#1623386 — Re: [PATCH] ARM: dma-mapping: add check for coherent DMA memory without struct page

On Thu, Apr 13, 2017 at 03:47:56PM -0600, Shuah Khan wrote:
> When coherent DMA memory without struct page is shared, importer
> fails to find the page and runs into kernel page fault when it
> tries to dmabuf_ops_attach/map_sg/map_page the invalid page found
> in the sg_table.
> 
> Add a new dma_check_dev_coherent() interface to check if memory is
> from the device coherent area. There is no way to tell where the
> memory returned by dma_alloc_attrs() came from.
> 
> arm_dma_get_sgtable() checks for invalid pages, however this check
> could pass even for memory obtained the coherent allocator. Add an
> additional check to call dma_check_dev_coherent() to confirm that it
> is indeed the coherent DMA memory and fail the sgtable creation with
> -EINVAL.

Sorry, this doesn't make much sense to me.

pfn_valid(pfn) must *never* return true if 'pfn' does not have a struct
page associated with it.  If it returns true (so we allow
arm_dma_get_sgtable() to succeed) then we know we have a valid struct
page in the supplied scatterlist.

> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  arch/arm/mm/dma-mapping.c   | 11 ++++++++---
>  drivers/base/dma-coherent.c | 25 +++++++++++++++++++++++++
>  include/linux/dma-mapping.h |  2 ++
>  3 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 475811f..27c7d9a 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -954,9 +954,14 @@ int arm_dma_get_sgtable(struct device *dev, struct sg_table *sgt,
>  	struct page *page;
>  	int ret;
>  
> -	/* If the PFN is not valid, we do not have a struct page */
> -	if (!pfn_valid(pfn))
> -		return -ENXIO;
> +	/*
> +	 * If the PFN is not valid, we do not have a struct page
> +	 * As this check can pass even for memory obtained through
> +	 * the coherent allocator, do an additional check to determine
> +	 * if this is coherent DMA memory.
> +	 */
> +	if (!pfn_valid(pfn) && dma_check_dev_coherent(dev, handle, cpu_addr))
> +		return -EINVAL;

Right, so what this says is:

	if we do not haev a valid PFN
	_and_ if the memory is from the coherent section
	_then_ fail

Why the extra check?  Under what circunstances do we end up with memory
where the PFN is valid, but we do not have a valid struct page.  It
seems to me that such a scenario is a bug in pfn_valid() and not
something that should be worked around like this.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1623435 — Re: [PATCH] ARM: dma-mapping: add check for coherent DMA memory without struct page

On 04/13/2017 04:10 PM, Russell King - ARM Linux wrote:
> On Thu, Apr 13, 2017 at 03:47:56PM -0600, Shuah Khan wrote:
>> When coherent DMA memory without struct page is shared, importer
>> fails to find the page and runs into kernel page fault when it
>> tries to dmabuf_ops_attach/map_sg/map_page the invalid page found
>> in the sg_table.
>>
>> Add a new dma_check_dev_coherent() interface to check if memory is
>> from the device coherent area. There is no way to tell where the
>> memory returned by dma_alloc_attrs() came from.
>>
>> arm_dma_get_sgtable() checks for invalid pages, however this check
>> could pass even for memory obtained the coherent allocator. Add an
>> additional check to call dma_check_dev_coherent() to confirm that it
>> is indeed the coherent DMA memory and fail the sgtable creation with
>> -EINVAL.
> 
> Sorry, this doesn't make much sense to me.
> 
> pfn_valid(pfn) must *never* return true if 'pfn' does not have a struct
> page associated with it.  If it returns true (so we allow
> arm_dma_get_sgtable() to succeed) then we know we have a valid struct
> page in the supplied scatterlist.
> 
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  arch/arm/mm/dma-mapping.c   | 11 ++++++++---
>>  drivers/base/dma-coherent.c | 25 +++++++++++++++++++++++++
>>  include/linux/dma-mapping.h |  2 ++
>>  3 files changed, 35 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index 475811f..27c7d9a 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -954,9 +954,14 @@ int arm_dma_get_sgtable(struct device *dev, struct sg_table *sgt,
>>  	struct page *page;
>>  	int ret;
>>  
>> -	/* If the PFN is not valid, we do not have a struct page */
>> -	if (!pfn_valid(pfn))
>> -		return -ENXIO;
>> +	/*
>> +	 * If the PFN is not valid, we do not have a struct page
>> +	 * As this check can pass even for memory obtained through
>> +	 * the coherent allocator, do an additional check to determine
>> +	 * if this is coherent DMA memory.
>> +	 */
>> +	if (!pfn_valid(pfn) && dma_check_dev_coherent(dev, handle, cpu_addr))
>> +		return -EINVAL;
> 
> Right, so what this says is:
> 
> 	if we do not haev a valid PFN
> 	_and_ if the memory is from the coherent section
> 	_then_ fail
> 
> Why the extra check?  Under what circunstances do we end up with memory
> where the PFN is valid, but we do not have a valid struct page.  It
> seems to me that such a scenario is a bug in pfn_valid() and not
> something that should be worked around like this.
> 

DMA_ATTR_NO_KERNEL_MAPPING case is the one I am concerned about.
pfn_valid() would fail on this if I am understanding it correctly.
A few drm drivers set this attr and use sg_table for passing buffers.

My reasoning behind adding this check is to not have this fail on
NO_KERNEL_MAPPING cases. So I thought adding a restrictive check for
just the per-device memory would help. However, I don't have a good
understanding of the drm case, hence I could be trying to address a
case that doesn't need to be addressed.

In any case, thanks for your patience and a quick reply.

-- Shuah

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web