revert dma-mapping and vmap API abuse in qcom_q6v5_mss

revert dma-mapping and vmap API abuse in qcom_q6v5_mss

[Christoph Hellwig]

Hi all,

this series reverts a completely broken commit to qcom_q6v5_mss the
abuses the dma-mapping and vmap APIs in multiple, and mostly clearly
documented ways, and then adds a patch to document and reject the
so far undocumented API abuse.

Diffstat:
 drivers/remoteproc/qcom_q6v5_mss.c |   38 +++++--------------------------------
 mm/vmalloc.c                       |    3 ++
 2 files changed, 9 insertions(+), 32 deletions(-)

[PATCH 1/2] Revert "remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use"

[Christoph Hellwig]

This reverts commit fc156629b23a21181e473e60341e3a78af25a1d4.

This commit manages to do three API violations at once:

 - dereference the return value of dma_alloc_attrs with the
   DMA_ATTR_NO_KERNEL_MAPPING mapping, which is clearly forbidden and
   will do the wrong thing on various dma mapping implementations.  The
   fact that dma-direct uses a struct page as a cookie is an undocumented
   implementation detail
 - include dma-map-ops.h and use pgprot_dmacoherent despite a clear
   comment documenting that this is not acceptable
 - use of the VM_DMA_COHERENT for something that is not the dma-mapping
   code
 - use of VM_FLUSH_RESET_PERMS for vmap, while it is only supported for
   vmalloc

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 38 +++++-------------------------
 1 file changed, 6 insertions(+), 32 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index fddb63cffee078..a8b141db4de63f 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -10,7 +10,6 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/devcoredump.h>
-#include <linux/dma-map-ops.h>
 #include <linux/dma-mapping.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -933,52 +932,27 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
 static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
 				const char *fw_name)
 {
-	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_KERNEL_MAPPING;
-	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
-	struct page **pages;
-	struct page *page;
+	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
 	dma_addr_t phys;
 	void *metadata;
 	int mdata_perm;
 	int xferop_ret;
 	size_t size;
-	void *vaddr;
-	int count;
+	void *ptr;
 	int ret;
-	int i;
 
 	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
 	if (IS_ERR(metadata))
 		return PTR_ERR(metadata);
 
-	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
-	if (!page) {
+	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
+	if (!ptr) {
 		kfree(metadata);
 		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
 		return -ENOMEM;
 	}
 
-	count = PAGE_ALIGN(size) >> PAGE_SHIFT;
-	pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
-	if (!pages) {
-		ret = -ENOMEM;
-		goto free_dma_attrs;
-	}
-
-	for (i = 0; i < count; i++)
-		pages[i] = nth_page(page, i);
-
-	vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
-	kfree(pages);
-	if (!vaddr) {
-		dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", &phys, size);
-		ret = -EBUSY;
-		goto free_dma_attrs;
-	}
-
-	memcpy(vaddr, metadata, size);
-
-	vunmap(vaddr);
+	memcpy(ptr, metadata, size);
 
 	/* Hypervisor mapping to access metadata by modem */
 	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
@@ -1008,7 +982,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
 			 "mdt buffer not reclaimed system may become unstable\n");
 
 free_dma_attrs:
-	dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
+	dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
 	kfree(metadata);
 
 	return ret < 0 ? ret : 0;
-- 
2.35.1

[PATCH 2/2] vmalloc: reject vmap with VM_FLUSH_RESET_PERMS

[Christoph Hellwig]

VM_FLUSH_RESET_PERMS is just for use with vmalloc as it is tied to freeing
the underlying pages.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/vmalloc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 9e30f0b3920325..88a644cde9fb12 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2849,6 +2849,9 @@ void *vmap(struct page **pages, unsigned int count,
 
 	might_sleep();
 
+	if (WARN_ON_ONCE(flags & VM_FLUSH_RESET_PERMS))
+		return NULL;
+
 	/*
 	 * Your top guard is someone else's bottom guard. Not having a top
 	 * guard compromises someone else's mappings too.
-- 
2.35.1

Re: [PATCH 2/2] vmalloc: reject vmap with VM_FLUSH_RESET_PERMS

[Lorenzo Stoakes]

On Fri, Dec 23, 2022 at 10:27:03AM +0100, Christoph Hellwig wrote:
> VM_FLUSH_RESET_PERMS is just for use with vmalloc as it is tied to freeing
> the underlying pages.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  mm/vmalloc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 9e30f0b3920325..88a644cde9fb12 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2849,6 +2849,9 @@ void *vmap(struct page **pages, unsigned int count,
>
>  	might_sleep();
>
> +	if (WARN_ON_ONCE(flags & VM_FLUSH_RESET_PERMS))
> +		return NULL;
> +

Might it be worth adding a specific vmap mask that explicitly indicates what
flags are permissible on vmap()? Then this could become e.g.:-

	if (WARN_ON_ONCE(flags & ~VM_VMAP_PERMITTED_MASK))
		return NULL;

And would be self-documenting as to why we are disallowing flags (i.e. they are
not part of the permitted vmap mask).

Re: [PATCH 2/2] vmalloc: reject vmap with VM_FLUSH_RESET_PERMS

[Christoph Hellwig]

On Fri, Dec 23, 2022 at 10:24:25AM +0000, Lorenzo Stoakes wrote:
> Might it be worth adding a specific vmap mask that explicitly indicates what
> flags are permissible on vmap()? Then this could become e.g.:-
> 
> 	if (WARN_ON_ONCE(flags & ~VM_VMAP_PERMITTED_MASK))
> 		return NULL;
> 
> And would be self-documenting as to why we are disallowing flags (i.e. they are
> not part of the permitted vmap mask).

That's probably a good idea.  It might need some time to audit
for use of all the flags, though.

Re: [PATCH 2/2] vmalloc: reject vmap with VM_FLUSH_RESET_PERMS

[Lorenzo Stoakes]

On Fri, Dec 23, 2022 at 03:03:12PM +0100, Christoph Hellwig wrote:
> On Fri, Dec 23, 2022 at 10:24:25AM +0000, Lorenzo Stoakes wrote:
> > Might it be worth adding a specific vmap mask that explicitly indicates what
> > flags are permissible on vmap()? Then this could become e.g.:-
> >
> > 	if (WARN_ON_ONCE(flags & ~VM_VMAP_PERMITTED_MASK))
> > 		return NULL;
> >
> > And would be self-documenting as to why we are disallowing flags (i.e. they are
> > not part of the permitted vmap mask).
>
> That's probably a good idea.  It might need some time to audit
> for use of all the flags, though.

Perhaps leave that for a later patch (I could take a look as well), but in the
meantime might be worth adding a quick comment here indicating why the flag is
prohibited?

Re: [PATCH 1/2] Revert "remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use"

[Manivannan Sadhasivam]

On Fri, Dec 23, 2022 at 10:27:02AM +0100, Christoph Hellwig wrote:
> This reverts commit fc156629b23a21181e473e60341e3a78af25a1d4.
> 
> This commit manages to do three API violations at once:
> 
>  - dereference the return value of dma_alloc_attrs with the
>    DMA_ATTR_NO_KERNEL_MAPPING mapping, which is clearly forbidden and
>    will do the wrong thing on various dma mapping implementations.  The
>    fact that dma-direct uses a struct page as a cookie is an undocumented
>    implementation detail
>  - include dma-map-ops.h and use pgprot_dmacoherent despite a clear
>    comment documenting that this is not acceptable
>  - use of the VM_DMA_COHERENT for something that is not the dma-mapping
>    code
>  - use of VM_FLUSH_RESET_PERMS for vmap, while it is only supported for
>    vmalloc
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 38 +++++-------------------------
>  1 file changed, 6 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index fddb63cffee078..a8b141db4de63f 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -10,7 +10,6 @@
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/devcoredump.h>
> -#include <linux/dma-map-ops.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -933,52 +932,27 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>  static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>  				const char *fw_name)
>  {
> -	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_KERNEL_MAPPING;
> -	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
> -	struct page **pages;
> -	struct page *page;
> +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>  	dma_addr_t phys;
>  	void *metadata;
>  	int mdata_perm;
>  	int xferop_ret;
>  	size_t size;
> -	void *vaddr;
> -	int count;
> +	void *ptr;
>  	int ret;
> -	int i;
>  
>  	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
>  	if (IS_ERR(metadata))
>  		return PTR_ERR(metadata);
>  
> -	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> -	if (!page) {
> +	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> +	if (!ptr) {
>  		kfree(metadata);
>  		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>  		return -ENOMEM;
>  	}
>  
> -	count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> -	pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
> -	if (!pages) {
> -		ret = -ENOMEM;
> -		goto free_dma_attrs;
> -	}
> -
> -	for (i = 0; i < count; i++)
> -		pages[i] = nth_page(page, i);
> -
> -	vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
> -	kfree(pages);
> -	if (!vaddr) {
> -		dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", &phys, size);
> -		ret = -EBUSY;
> -		goto free_dma_attrs;
> -	}
> -
> -	memcpy(vaddr, metadata, size);
> -
> -	vunmap(vaddr);
> +	memcpy(ptr, metadata, size);
>  
>  	/* Hypervisor mapping to access metadata by modem */
>  	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
> @@ -1008,7 +982,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>  			 "mdt buffer not reclaimed system may become unstable\n");
>  
>  free_dma_attrs:
> -	dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
> +	dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
>  	kfree(metadata);
>  
>  	return ret < 0 ? ret : 0;
> -- 
> 2.35.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/2] Revert "remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use"

[Christoph Hellwig]

On Fri, Dec 23, 2022 at 08:17:31PM +0530, Manivannan Sadhasivam wrote:
> Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Btw, if the hardware really does not like a kernel mapping, the
right way is to just keep using the normal dma allocator, but make
sure that there shared-dma-pool with the no-map property for the
device.

Re: [PATCH 1/2] Revert "remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use"

[Manivannan Sadhasivam]

On Fri, Dec 23, 2022 at 03:57:22PM +0100, Christoph Hellwig wrote:
> On Fri, Dec 23, 2022 at 08:17:31PM +0530, Manivannan Sadhasivam wrote:
> > Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Btw, if the hardware really does not like a kernel mapping, the
> right way is to just keep using the normal dma allocator, but make
> sure that there shared-dma-pool with the no-map property for the
> device.

Sibi posted a series that uses a separate no-map carveout for this usecase:
https://lore.kernel.org/lkml/20221213140724.8612-1-quic_sibis@quicinc.com/

But that doesn't use dma allocator with shared-dma-pool.

Thanks,
Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/2] Revert "remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use"

[Christoph Hellwig]

On Fri, Dec 23, 2022 at 09:10:39PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Dec 23, 2022 at 03:57:22PM +0100, Christoph Hellwig wrote:
> > On Fri, Dec 23, 2022 at 08:17:31PM +0530, Manivannan Sadhasivam wrote:
> > > Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > Btw, if the hardware really does not like a kernel mapping, the
> > right way is to just keep using the normal dma allocator, but make
> > sure that there shared-dma-pool with the no-map property for the
> > device.
> 
> Sibi posted a series that uses a separate no-map carveout for this usecase:
> https://lore.kernel.org/lkml/20221213140724.8612-1-quic_sibis@quicinc.com/

Oh, I've missed that entire thread.  I actually stumbled over this
today while finding it during a vmap audit..