diff mbox series

[7/7] dma-mapping: reject __GFP_COMP in dma_alloc_attrs

Message ID 20221113163535.884299-8-hch@lst.de
State Accepted
Commit ffcb754584603adf7039d7972564fbf6febdc542
Headers show
Series [1/7] media: videobuf-dma-contig: use dma_mmap_coherent | expand

Commit Message

Christoph Hellwig Nov. 13, 2022, 4:35 p.m. UTC
DMA allocations can never be turned back into a page pointer, so
requesting compound pages doesn't make sense and it can't even be
supported at all by various backends.

Reject __GFP_COMP with a warning in dma_alloc_attrs, and stop clearing
the flag in the arm dma ops and dma-iommu.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/arm/mm/dma-mapping.c | 17 -----------------
 drivers/iommu/dma-iommu.c |  3 ---
 kernel/dma/mapping.c      |  8 ++++++++
 3 files changed, 8 insertions(+), 20 deletions(-)

Comments

Leon Romanovsky Nov. 14, 2022, 8:11 a.m. UTC | #1
On Sun, Nov 13, 2022 at 05:35:35PM +0100, Christoph Hellwig wrote:
> DMA allocations can never be turned back into a page pointer, so
> requesting compound pages doesn't make sense and it can't even be
> supported at all by various backends.
> 
> Reject __GFP_COMP with a warning in dma_alloc_attrs, and stop clearing
> the flag in the arm dma ops and dma-iommu.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/arm/mm/dma-mapping.c | 17 -----------------
>  drivers/iommu/dma-iommu.c |  3 ---
>  kernel/dma/mapping.c      |  8 ++++++++
>  3 files changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index d7909091cf977..c135f6e37a00c 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -564,14 +564,6 @@ static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
>  	if (mask < 0xffffffffULL)
>  		gfp |= GFP_DMA;
>  
> -	/*
> -	 * Following is a work-around (a.k.a. hack) to prevent pages
> -	 * with __GFP_COMP being passed to split_page() which cannot
> -	 * handle them.  The real problem is that this flag probably
> -	 * should be 0 on ARM as it is not supported on this
> -	 * platform; see CONFIG_HUGETLBFS.
> -	 */
> -	gfp &= ~(__GFP_COMP);
>  	args.gfp = gfp;
>  
>  	*handle = DMA_MAPPING_ERROR;
> @@ -1093,15 +1085,6 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,
>  		return __iommu_alloc_simple(dev, size, gfp, handle,
>  					    coherent_flag, attrs);
>  
> -	/*
> -	 * Following is a work-around (a.k.a. hack) to prevent pages
> -	 * with __GFP_COMP being passed to split_page() which cannot
> -	 * handle them.  The real problem is that this flag probably
> -	 * should be 0 on ARM as it is not supported on this
> -	 * platform; see CONFIG_HUGETLBFS.
> -	 */
> -	gfp &= ~(__GFP_COMP);
> -
>  	pages = __iommu_alloc_buffer(dev, size, gfp, attrs, coherent_flag);
>  	if (!pages)
>  		return NULL;
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 9297b741f5e80..f798c44e09033 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -744,9 +744,6 @@ static struct page **__iommu_dma_alloc_pages(struct device *dev,
>  	/* IOMMU can map any pages, so himem can also be used here */
>  	gfp |= __GFP_NOWARN | __GFP_HIGHMEM;
>  
> -	/* It makes no sense to muck about with huge pages */
> -	gfp &= ~__GFP_COMP;
> -
>  	while (count) {
>  		struct page *page = NULL;
>  		unsigned int order_size;
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 33437d6206445..c026a5a5e0466 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -498,6 +498,14 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
>  
>  	WARN_ON_ONCE(!dev->coherent_dma_mask);
>  
> +	/*
> +	 * DMA allocations can never be turned back into a page pointer, so
> +	 * requesting compound pages doesn't make sense (and can't even be
> +	 * supported at all by various backends).
> +	 */
> +	if (WARN_ON_ONCE(flag & __GFP_COMP))
> +		return NULL;

In RDMA patches, you wrote that GFP_USER is not legal flag either. So it
is better to WARN here for everything that is not allowed.

> +
>  	if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
>  		return cpu_addr;
>  
> -- 
> 2.30.2
>
Marek Szyprowski Nov. 14, 2022, 12:16 p.m. UTC | #2
On 13.11.2022 17:35, Christoph Hellwig wrote:
> DMA allocations can never be turned back into a page pointer, so
> requesting compound pages doesn't make sense and it can't even be
> supported at all by various backends.
>
> Reject __GFP_COMP with a warning in dma_alloc_attrs, and stop clearing
> the flag in the arm dma ops and dma-iommu.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   arch/arm/mm/dma-mapping.c | 17 -----------------
>   drivers/iommu/dma-iommu.c |  3 ---
>   kernel/dma/mapping.c      |  8 ++++++++
>   3 files changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index d7909091cf977..c135f6e37a00c 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -564,14 +564,6 @@ static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
>   	if (mask < 0xffffffffULL)
>   		gfp |= GFP_DMA;
>   
> -	/*
> -	 * Following is a work-around (a.k.a. hack) to prevent pages
> -	 * with __GFP_COMP being passed to split_page() which cannot
> -	 * handle them.  The real problem is that this flag probably
> -	 * should be 0 on ARM as it is not supported on this
> -	 * platform; see CONFIG_HUGETLBFS.
> -	 */
> -	gfp &= ~(__GFP_COMP);
>   	args.gfp = gfp;
>   
>   	*handle = DMA_MAPPING_ERROR;
> @@ -1093,15 +1085,6 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,
>   		return __iommu_alloc_simple(dev, size, gfp, handle,
>   					    coherent_flag, attrs);
>   
> -	/*
> -	 * Following is a work-around (a.k.a. hack) to prevent pages
> -	 * with __GFP_COMP being passed to split_page() which cannot
> -	 * handle them.  The real problem is that this flag probably
> -	 * should be 0 on ARM as it is not supported on this
> -	 * platform; see CONFIG_HUGETLBFS.
> -	 */
> -	gfp &= ~(__GFP_COMP);
> -
>   	pages = __iommu_alloc_buffer(dev, size, gfp, attrs, coherent_flag);
>   	if (!pages)
>   		return NULL;
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 9297b741f5e80..f798c44e09033 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -744,9 +744,6 @@ static struct page **__iommu_dma_alloc_pages(struct device *dev,
>   	/* IOMMU can map any pages, so himem can also be used here */
>   	gfp |= __GFP_NOWARN | __GFP_HIGHMEM;
>   
> -	/* It makes no sense to muck about with huge pages */
> -	gfp &= ~__GFP_COMP;
> -
>   	while (count) {
>   		struct page *page = NULL;
>   		unsigned int order_size;
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 33437d6206445..c026a5a5e0466 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -498,6 +498,14 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
>   
>   	WARN_ON_ONCE(!dev->coherent_dma_mask);
>   
> +	/*
> +	 * DMA allocations can never be turned back into a page pointer, so
> +	 * requesting compound pages doesn't make sense (and can't even be
> +	 * supported at all by various backends).
> +	 */
> +	if (WARN_ON_ONCE(flag & __GFP_COMP))
> +		return NULL;
> +
>   	if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
>   		return cpu_addr;
>   

Best regards
Leon Romanovsky Nov. 16, 2022, 7:11 a.m. UTC | #3
On Wed, Nov 16, 2022 at 07:11:06AM +0100, Christoph Hellwig wrote:
> On Mon, Nov 14, 2022 at 10:11:50AM +0200, Leon Romanovsky wrote:
> > In RDMA patches, you wrote that GFP_USER is not legal flag either. So it
> > is better to WARN here for everything that is not allowed.
> 
> So __GFP_COMP is actually problematic and changes behavior, and I plan
> to lift an optimization from the arm code to the generic one that
> only rounds up allocations to the next page size instead of the next
> power of two, so I need this check now.  Other flags including
> GFP_USER are pretty bogus to, but I actually need to do a full audit
> before rejecting them, which I've only done for GFP_COMP so far.

ok, let's do it later.

Thanks
diff mbox series

Patch

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index d7909091cf977..c135f6e37a00c 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -564,14 +564,6 @@  static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
 	if (mask < 0xffffffffULL)
 		gfp |= GFP_DMA;
 
-	/*
-	 * Following is a work-around (a.k.a. hack) to prevent pages
-	 * with __GFP_COMP being passed to split_page() which cannot
-	 * handle them.  The real problem is that this flag probably
-	 * should be 0 on ARM as it is not supported on this
-	 * platform; see CONFIG_HUGETLBFS.
-	 */
-	gfp &= ~(__GFP_COMP);
 	args.gfp = gfp;
 
 	*handle = DMA_MAPPING_ERROR;
@@ -1093,15 +1085,6 @@  static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,
 		return __iommu_alloc_simple(dev, size, gfp, handle,
 					    coherent_flag, attrs);
 
-	/*
-	 * Following is a work-around (a.k.a. hack) to prevent pages
-	 * with __GFP_COMP being passed to split_page() which cannot
-	 * handle them.  The real problem is that this flag probably
-	 * should be 0 on ARM as it is not supported on this
-	 * platform; see CONFIG_HUGETLBFS.
-	 */
-	gfp &= ~(__GFP_COMP);
-
 	pages = __iommu_alloc_buffer(dev, size, gfp, attrs, coherent_flag);
 	if (!pages)
 		return NULL;
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 9297b741f5e80..f798c44e09033 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -744,9 +744,6 @@  static struct page **__iommu_dma_alloc_pages(struct device *dev,
 	/* IOMMU can map any pages, so himem can also be used here */
 	gfp |= __GFP_NOWARN | __GFP_HIGHMEM;
 
-	/* It makes no sense to muck about with huge pages */
-	gfp &= ~__GFP_COMP;
-
 	while (count) {
 		struct page *page = NULL;
 		unsigned int order_size;
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 33437d6206445..c026a5a5e0466 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -498,6 +498,14 @@  void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
 
 	WARN_ON_ONCE(!dev->coherent_dma_mask);
 
+	/*
+	 * DMA allocations can never be turned back into a page pointer, so
+	 * requesting compound pages doesn't make sense (and can't even be
+	 * supported at all by various backends).
+	 */
+	if (WARN_ON_ONCE(flag & __GFP_COMP))
+		return NULL;
+
 	if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
 		return cpu_addr;