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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
>> ---
>> 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