[PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[Walter Wu]

DMA_ATTR_NO_KERNEL_MAPPING is to avoid creating a kernel mapping
for the allocated buffer, but current implementation is that
PTE of allocated buffer in kernel page table is valid. So we
should set invalid for PTE of allocate buffer so that there are
no kernel mapping for the allocated buffer.

In some cases, we don't hope the allocated buffer to be read
by cpu or speculative execution, so we use DMA_ATTR_NO_KERNEL_MAPPING
to get no kernel mapping in order to achieve this goal.

Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 kernel/dma/direct.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 4c6c5e0635e3..aa10b4c5d762 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -13,6 +13,7 @@
 #include <linux/vmalloc.h>
 #include <linux/set_memory.h>
 #include <linux/slab.h>
+#include <asm/cacheflush.h>
 #include "direct.h"
 
 /*
@@ -169,6 +170,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 		if (!PageHighMem(page))
 			arch_dma_prep_coherent(page, size);
 		*dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
+		/* remove kernel mapping for pages */
+		set_memory_valid((unsigned long)phys_to_virt(dma_to_phys(dev, *dma_handle)),
+				size >> PAGE_SHIFT, 0);
 		/* return the page pointer as the opaque cookie */
 		return page;
 	}
@@ -278,6 +282,10 @@ void dma_direct_free(struct device *dev, size_t size,
 
 	if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
 	    !force_dma_unencrypted(dev) && !is_swiotlb_for_alloc(dev)) {
+		size = PAGE_ALIGN(size);
+		/* create kernel mapping for pages */
+		set_memory_valid((unsigned long)phys_to_virt(dma_to_phys(dev, dma_addr)),
+				size >> PAGE_SHIFT, 1);
 		/* cpu_addr is a struct page cookie, not a kernel address */
 		dma_free_contiguous(dev, cpu_addr, size);
 		return;
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6953 bytes --]

Hi Walter,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on linus/master v5.15 next-20211029]
[cannot apply to hch-configfs/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Walter-Wu/dma-direct-fix-DMA_ATTR_NO_KERNEL_MAPPING/20211101-111657
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2f111a6fd5b5297b4e92f53798ca086f7c7d33a4
config: i386-randconfig-r036-20211101 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 82ed106567063ea269c6d5669278b733e173a42f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4694d2ac8f4f9a7476f829f9f43a25111424eca8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Walter-Wu/dma-direct-fix-DMA_ATTR_NO_KERNEL_MAPPING/20211101-111657
        git checkout 4694d2ac8f4f9a7476f829f9f43a25111424eca8
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> kernel/dma/direct.c:174:3: error: implicit declaration of function 'set_memory_valid' [-Werror,-Wimplicit-function-declaration]
                   set_memory_valid((unsigned long)phys_to_virt(dma_to_phys(dev, *dma_handle)),
                   ^
   kernel/dma/direct.c:287:3: error: implicit declaration of function 'set_memory_valid' [-Werror,-Wimplicit-function-declaration]
                   set_memory_valid((unsigned long)phys_to_virt(dma_to_phys(dev, dma_addr)),
                   ^
   2 errors generated.


vim +/set_memory_valid +174 kernel/dma/direct.c

   152	
   153	void *dma_direct_alloc(struct device *dev, size_t size,
   154			dma_addr_t *dma_handle, gfp_t gfp, unsigned long attrs)
   155	{
   156		struct page *page;
   157		void *ret;
   158		int err;
   159	
   160		size = PAGE_ALIGN(size);
   161		if (attrs & DMA_ATTR_NO_WARN)
   162			gfp |= __GFP_NOWARN;
   163	
   164		if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
   165		    !force_dma_unencrypted(dev) && !is_swiotlb_for_alloc(dev)) {
   166			page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO);
   167			if (!page)
   168				return NULL;
   169			/* remove any dirty cache lines on the kernel alias */
   170			if (!PageHighMem(page))
   171				arch_dma_prep_coherent(page, size);
   172			*dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
   173			/* remove kernel mapping for pages */
 > 174			set_memory_valid((unsigned long)phys_to_virt(dma_to_phys(dev, *dma_handle)),
   175					size >> PAGE_SHIFT, 0);
   176			/* return the page pointer as the opaque cookie */
   177			return page;
   178		}
   179	
   180		if (!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED) &&
   181		    !IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
   182		    !IS_ENABLED(CONFIG_DMA_GLOBAL_POOL) &&
   183		    !dev_is_dma_coherent(dev) &&
   184		    !is_swiotlb_for_alloc(dev))
   185			return arch_dma_alloc(dev, size, dma_handle, gfp, attrs);
   186	
   187		if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL) &&
   188		    !dev_is_dma_coherent(dev))
   189			return dma_alloc_from_global_coherent(dev, size, dma_handle);
   190	
   191		/*
   192		 * Remapping or decrypting memory may block. If either is required and
   193		 * we can't block, allocate the memory from the atomic pools.
   194		 * If restricted DMA (i.e., is_swiotlb_for_alloc) is required, one must
   195		 * set up another device coherent pool by shared-dma-pool and use
   196		 * dma_alloc_from_dev_coherent instead.
   197		 */
   198		if (IS_ENABLED(CONFIG_DMA_COHERENT_POOL) &&
   199		    !gfpflags_allow_blocking(gfp) &&
   200		    (force_dma_unencrypted(dev) ||
   201		     (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
   202		      !dev_is_dma_coherent(dev))) &&
   203		    !is_swiotlb_for_alloc(dev))
   204			return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
   205	
   206		/* we always manually zero the memory once we are done */
   207		page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO);
   208		if (!page)
   209			return NULL;
   210	
   211		if ((IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
   212		     !dev_is_dma_coherent(dev)) ||
   213		    (IS_ENABLED(CONFIG_DMA_REMAP) && PageHighMem(page))) {
   214			/* remove any dirty cache lines on the kernel alias */
   215			arch_dma_prep_coherent(page, size);
   216	
   217			/* create a coherent mapping */
   218			ret = dma_common_contiguous_remap(page, size,
   219					dma_pgprot(dev, PAGE_KERNEL, attrs),
   220					__builtin_return_address(0));
   221			if (!ret)
   222				goto out_free_pages;
   223			if (force_dma_unencrypted(dev)) {
   224				err = set_memory_decrypted((unsigned long)ret,
   225							   1 << get_order(size));
   226				if (err)
   227					goto out_free_pages;
   228			}
   229			memset(ret, 0, size);
   230			goto done;
   231		}
   232	
   233		if (PageHighMem(page)) {
   234			/*
   235			 * Depending on the cma= arguments and per-arch setup
   236			 * dma_alloc_contiguous could return highmem pages.
   237			 * Without remapping there is no way to return them here,
   238			 * so log an error and fail.
   239			 */
   240			dev_info(dev, "Rejecting highmem page from CMA.\n");
   241			goto out_free_pages;
   242		}
   243	
   244		ret = page_address(page);
   245		if (force_dma_unencrypted(dev)) {
   246			err = set_memory_decrypted((unsigned long)ret,
   247						   1 << get_order(size));
   248			if (err)
   249				goto out_free_pages;
   250		}
   251	
   252		memset(ret, 0, size);
   253	
   254		if (IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED) &&
   255		    !dev_is_dma_coherent(dev)) {
   256			arch_dma_prep_coherent(page, size);
   257			ret = arch_dma_set_uncached(ret, size);
   258			if (IS_ERR(ret))
   259				goto out_encrypt_pages;
   260		}
   261	done:
   262		*dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
   263		return ret;
   264	
   265	out_encrypt_pages:
   266		if (force_dma_unencrypted(dev)) {
   267			err = set_memory_encrypted((unsigned long)page_address(page),
   268						   1 << get_order(size));
   269			/* If memory cannot be re-encrypted, it must be leaked */
   270			if (err)
   271				return NULL;
   272		}
   273	out_free_pages:
   274		__dma_direct_free_pages(dev, page, size);
   275		return NULL;
   276	}
   277	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35151 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[Ard Biesheuvel]

On Mon, 1 Nov 2021 at 04:17, Walter Wu <walter-zh.wu@mediatek.com> wrote:
>
> DMA_ATTR_NO_KERNEL_MAPPING is to avoid creating a kernel mapping
> for the allocated buffer, but current implementation is that
> PTE of allocated buffer in kernel page table is valid. So we
> should set invalid for PTE of allocate buffer so that there are
> no kernel mapping for the allocated buffer.
>
> In some cases, we don't hope the allocated buffer to be read
> by cpu or speculative execution, so we use DMA_ATTR_NO_KERNEL_MAPPING
> to get no kernel mapping in order to achieve this goal.
>
> Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  kernel/dma/direct.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 4c6c5e0635e3..aa10b4c5d762 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -13,6 +13,7 @@
>  #include <linux/vmalloc.h>
>  #include <linux/set_memory.h>
>  #include <linux/slab.h>
> +#include <asm/cacheflush.h>
>  #include "direct.h"
>
>  /*
> @@ -169,6 +170,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,
>                 if (!PageHighMem(page))
>                         arch_dma_prep_coherent(page, size);
>                 *dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
> +               /* remove kernel mapping for pages */
> +               set_memory_valid((unsigned long)phys_to_virt(dma_to_phys(dev, *dma_handle)),
> +                               size >> PAGE_SHIFT, 0);

This only works if the memory is mapped at page granularity in the
linear region, and you cannot rely on that. Many architectures prefer
block mappings for the linear region, and arm64 will only use page
mappings if rodata=full is set (which is set by default but can be
overridden on the kernel command line)


>                 /* return the page pointer as the opaque cookie */
>                 return page;
>         }
> @@ -278,6 +282,10 @@ void dma_direct_free(struct device *dev, size_t size,
>
>         if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
>             !force_dma_unencrypted(dev) && !is_swiotlb_for_alloc(dev)) {
> +               size = PAGE_ALIGN(size);
> +               /* create kernel mapping for pages */
> +               set_memory_valid((unsigned long)phys_to_virt(dma_to_phys(dev, dma_addr)),
> +                               size >> PAGE_SHIFT, 1);
>                 /* cpu_addr is a struct page cookie, not a kernel address */
>                 dma_free_contiguous(dev, cpu_addr, size);
>                 return;
> --
> 2.18.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[Walter Wu]

Hi Ard,

On Mon, 2021-11-01 at 09:34 +0100, Ard Biesheuvel wrote:
> On Mon, 1 Nov 2021 at 04:17, Walter Wu <walter-zh.wu@mediatek.com>
> wrote:
> > 
> > DMA_ATTR_NO_KERNEL_MAPPING is to avoid creating a kernel mapping
> > for the allocated buffer, but current implementation is that
> > PTE of allocated buffer in kernel page table is valid. So we
> > should set invalid for PTE of allocate buffer so that there are
> > no kernel mapping for the allocated buffer.
> > 
> > In some cases, we don't hope the allocated buffer to be read
> > by cpu or speculative execution, so we use
> > DMA_ATTR_NO_KERNEL_MAPPING
> > to get no kernel mapping in order to achieve this goal.
> > 
> > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >  kernel/dma/direct.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> > index 4c6c5e0635e3..aa10b4c5d762 100644
> > --- a/kernel/dma/direct.c
> > +++ b/kernel/dma/direct.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/vmalloc.h>
> >  #include <linux/set_memory.h>
> >  #include <linux/slab.h>
> > +#include <asm/cacheflush.h>
> >  #include "direct.h"
> > 
> >  /*
> > @@ -169,6 +170,9 @@ void *dma_direct_alloc(struct device *dev,
> > size_t size,
> >                 if (!PageHighMem(page))
> >                         arch_dma_prep_coherent(page, size);
> >                 *dma_handle = phys_to_dma_direct(dev,
> > page_to_phys(page));
> > +               /* remove kernel mapping for pages */
> > +               set_memory_valid((unsigned
> > long)phys_to_virt(dma_to_phys(dev, *dma_handle)),
> > +                               size >> PAGE_SHIFT, 0);
> 
> This only works if the memory is mapped at page granularity in the
> linear region, and you cannot rely on that. Many architectures prefer
> block mappings for the linear region, and arm64 will only use page
> mappings if rodata=full is set (which is set by default but can be
> overridden on the kernel command line)
> 

We mainly want to solve arm64 arch. RODATA_FULL_DEFAULT_ENABLED should
be the arm64 config. If we use CONFIG_RODATA_FULL_DEFAULT_ENABLED to
check whether it call set_memory_valid(). It should avoid other
architectures. Do you think this method is work?

Thanks for your explaination and suggestion.

Walter

> 
> >                 /* return the page pointer as the opaque cookie */
> >                 return page;
> >         }
> > @@ -278,6 +282,10 @@ void dma_direct_free(struct device *dev,
> > size_t size,
> > 
> >         if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
> >             !force_dma_unencrypted(dev) &&
> > !is_swiotlb_for_alloc(dev)) {
> > +               size = PAGE_ALIGN(size);
> > +               /* create kernel mapping for pages */
> > +               set_memory_valid((unsigned
> > long)phys_to_virt(dma_to_phys(dev, dma_addr)),
> > +                               size >> PAGE_SHIFT, 1);
> >                 /* cpu_addr is a struct page cookie, not a kernel
> > address */
> >                 dma_free_contiguous(dev, cpu_addr, size);
> >                 return;
> > --
> > 2.18.0
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > 
https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!CTRNKA9wMg0ARbw!16dLCjnvtRkaRLeCO9AQ7Fund5XL0FicZmeVaU3-WkFymr-0lbITfzwrvoJpiHlqnqIu-g$
> >  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[Ard Biesheuvel]

On Mon, 1 Nov 2021 at 13:21, Walter Wu <walter-zh.wu@mediatek.com> wrote:
>
> Hi Ard,
>
> On Mon, 2021-11-01 at 09:34 +0100, Ard Biesheuvel wrote:
> > On Mon, 1 Nov 2021 at 04:17, Walter Wu <walter-zh.wu@mediatek.com>
> > wrote:
> > >
> > > DMA_ATTR_NO_KERNEL_MAPPING is to avoid creating a kernel mapping
> > > for the allocated buffer, but current implementation is that
> > > PTE of allocated buffer in kernel page table is valid. So we
> > > should set invalid for PTE of allocate buffer so that there are
> > > no kernel mapping for the allocated buffer.
> > >
> > > In some cases, we don't hope the allocated buffer to be read
> > > by cpu or speculative execution, so we use
> > > DMA_ATTR_NO_KERNEL_MAPPING
> > > to get no kernel mapping in order to achieve this goal.
> > >
> > > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > > Cc: Christoph Hellwig <hch@lst.de>
> > > Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > > Cc: Robin Murphy <robin.murphy@arm.com>
> > > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > ---
> > >  kernel/dma/direct.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> > > index 4c6c5e0635e3..aa10b4c5d762 100644
> > > --- a/kernel/dma/direct.c
> > > +++ b/kernel/dma/direct.c
> > > @@ -13,6 +13,7 @@
> > >  #include <linux/vmalloc.h>
> > >  #include <linux/set_memory.h>
> > >  #include <linux/slab.h>
> > > +#include <asm/cacheflush.h>
> > >  #include "direct.h"
> > >
> > >  /*
> > > @@ -169,6 +170,9 @@ void *dma_direct_alloc(struct device *dev,
> > > size_t size,
> > >                 if (!PageHighMem(page))
> > >                         arch_dma_prep_coherent(page, size);
> > >                 *dma_handle = phys_to_dma_direct(dev,
> > > page_to_phys(page));
> > > +               /* remove kernel mapping for pages */
> > > +               set_memory_valid((unsigned
> > > long)phys_to_virt(dma_to_phys(dev, *dma_handle)),
> > > +                               size >> PAGE_SHIFT, 0);
> >
> > This only works if the memory is mapped at page granularity in the
> > linear region, and you cannot rely on that. Many architectures prefer
> > block mappings for the linear region, and arm64 will only use page
> > mappings if rodata=full is set (which is set by default but can be
> > overridden on the kernel command line)
> >
>
> We mainly want to solve arm64 arch.

Solve what?

> RODATA_FULL_DEFAULT_ENABLED should
> be the arm64 config. If we use CONFIG_RODATA_FULL_DEFAULT_ENABLED to
> check whether it call set_memory_valid(). It should avoid other
> architectures. Do you think this method is work?
>

No. Try it with rodata=off (or rodata=on for that matter) and see what happens.

> Thanks for your explaination and suggestion.
>

YW

>
> >
> > >                 /* return the page pointer as the opaque cookie */
> > >                 return page;
> > >         }
> > > @@ -278,6 +282,10 @@ void dma_direct_free(struct device *dev,
> > > size_t size,
> > >
> > >         if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
> > >             !force_dma_unencrypted(dev) &&
> > > !is_swiotlb_for_alloc(dev)) {
> > > +               size = PAGE_ALIGN(size);
> > > +               /* create kernel mapping for pages */
> > > +               set_memory_valid((unsigned
> > > long)phys_to_virt(dma_to_phys(dev, dma_addr)),
> > > +                               size >> PAGE_SHIFT, 1);
> > >                 /* cpu_addr is a struct page cookie, not a kernel
> > > address */
> > >                 dma_free_contiguous(dev, cpu_addr, size);
> > >                 return;
> > > --
> > > 2.18.0
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > >
> https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!CTRNKA9wMg0ARbw!16dLCjnvtRkaRLeCO9AQ7Fund5XL0FicZmeVaU3-WkFymr-0lbITfzwrvoJpiHlqnqIu-g$
> > >
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[Walter Wu]

On Mon, 2021-11-01 at 15:17 +0100, Ard Biesheuvel wrote:
> On Mon, 1 Nov 2021 at 13:21, Walter Wu <walter-zh.wu@mediatek.com>
> wrote:
> > 
> > Hi Ard,
> > 
> > On Mon, 2021-11-01 at 09:34 +0100, Ard Biesheuvel wrote:
> > > On Mon, 1 Nov 2021 at 04:17, Walter Wu <walter-zh.wu@mediatek.com
> > > >
> > > wrote:
> > > > 
> > > > DMA_ATTR_NO_KERNEL_MAPPING is to avoid creating a kernel
> > > > mapping
> > > > for the allocated buffer, but current implementation is that
> > > > PTE of allocated buffer in kernel page table is valid. So we
> > > > should set invalid for PTE of allocate buffer so that there are
> > > > no kernel mapping for the allocated buffer.
> > > > 
> > > > In some cases, we don't hope the allocated buffer to be read
> > > > by cpu or speculative execution, so we use
> > > > DMA_ATTR_NO_KERNEL_MAPPING
> > > > to get no kernel mapping in order to achieve this goal.
> > > > 
> > > > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > Cc: Robin Murphy <robin.murphy@arm.com>
> > > > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > ---
> > > >  kernel/dma/direct.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> > > > index 4c6c5e0635e3..aa10b4c5d762 100644
> > > > --- a/kernel/dma/direct.c
> > > > +++ b/kernel/dma/direct.c
> > > > @@ -13,6 +13,7 @@
> > > >  #include <linux/vmalloc.h>
> > > >  #include <linux/set_memory.h>
> > > >  #include <linux/slab.h>
> > > > +#include <asm/cacheflush.h>
> > > >  #include "direct.h"
> > > > 
> > > >  /*
> > > > @@ -169,6 +170,9 @@ void *dma_direct_alloc(struct device *dev,
> > > > size_t size,
> > > >                 if (!PageHighMem(page))
> > > >                         arch_dma_prep_coherent(page, size);
> > > >                 *dma_handle = phys_to_dma_direct(dev,
> > > > page_to_phys(page));
> > > > +               /* remove kernel mapping for pages */
> > > > +               set_memory_valid((unsigned
> > > > long)phys_to_virt(dma_to_phys(dev, *dma_handle)),
> > > > +                               size >> PAGE_SHIFT, 0);
> > > 
> > > This only works if the memory is mapped at page granularity in
> > > the
> > > linear region, and you cannot rely on that. Many architectures
> > > prefer
> > > block mappings for the linear region, and arm64 will only use
> > > page
> > > mappings if rodata=full is set (which is set by default but can
> > > be
> > > overridden on the kernel command line)
> > > 
> > 
> > We mainly want to solve arm64 arch.
> 
> Solve what?

Our platform is arch64. We need a dynamic allocated buffer from CMA is
not to read by CPU peculative execution, so we need to remove its
kernel mapping.

> > RODATA_FULL_DEFAULT_ENABLED should
> > be the arm64 config. If we use CONFIG_RODATA_FULL_DEFAULT_ENABLED
> > to
> > check whether it call set_memory_valid(). It should avoid other
> > architectures. Do you think this method is work?
> > 
> 
> No. Try it with rodata=off (or rodata=on for that matter) and see
> what happens.
> 

I try with rodata=[off/on] on qemu, it looks like boot pass. see below.

[    0.000000] Kernel command line: root=/dev/ram0 rw rootfstype=ext4
rodata=off console=ttyAMA0 rdinit=qemu/ramdisk.img earlyprintk=serial


I also try with rodata=off on our platform and do stress test, it looks
like pass.


Thanks.
Walter

> > Thanks for your explaination and suggestion.
> > 
> 
> YW
> 
> > 
> > > 
> > > >                 /* return the page pointer as the opaque cookie
> > > > */
> > > >                 return page;
> > > >         }
> > > > @@ -278,6 +282,10 @@ void dma_direct_free(struct device *dev,
> > > > size_t size,
> > > > 
> > > >         if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
> > > >             !force_dma_unencrypted(dev) &&
> > > > !is_swiotlb_for_alloc(dev)) {
> > > > +               size = PAGE_ALIGN(size);
> > > > +               /* create kernel mapping for pages */
> > > > +               set_memory_valid((unsigned
> > > > long)phys_to_virt(dma_to_phys(dev, dma_addr)),
> > > > +                               size >> PAGE_SHIFT, 1);
> > > >                 /* cpu_addr is a struct page cookie, not a
> > > > kernel
> > > > address */
> > > >                 dma_free_contiguous(dev, cpu_addr, size);
> > > >                 return;
> > > > --
> > > > 2.18.0
> > > > 
> > > > 
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > 
> > 
> > 
https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!CTRNKA9wMg0ARbw!16dLCjnvtRkaRLeCO9AQ7Fund5XL0FicZmeVaU3-WkFymr-0lbITfzwrvoJpiHlqnqIu-g$
> > > > 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[Christoph Hellwig]

On Tue, Nov 02, 2021 at 11:21:16AM +0800, Walter Wu wrote:
> Our platform is arch64. We need a dynamic allocated buffer from CMA is
> not to read by CPU peculative execution, so we need to remove its
> kernel mapping.

If your CPU speculates into unused kernel direct mappings your have
a worse problem than this, because all the dma coherent allocations for
non-coherent devices still have a cachable direct mapping.  Will
mentioned he wanted to look into getting rid of that mapping now that
the core dma code has the infrastucture for that, so adding him here.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[Robin Murphy]

On 2021-11-01 03:15, Walter Wu wrote:
> DMA_ATTR_NO_KERNEL_MAPPING is to avoid creating a kernel mapping
> for the allocated buffer, but current implementation is that
> PTE of allocated buffer in kernel page table is valid. So we
> should set invalid for PTE of allocate buffer so that there are
> no kernel mapping for the allocated buffer.

No, the semantic of NO_KERNEL_MAPPING is an indication that the *caller* 
does not need a mapping, such that the DMA API implementation may choose 
to optimise for that internally. It has never given any guarantee of any 
particular behaviour - like most attributes it is only a hint.

> In some cases, we don't hope the allocated buffer to be read
> by cpu or speculative execution, so we use DMA_ATTR_NO_KERNEL_MAPPING
> to get no kernel mapping in order to achieve this goal.

If it's important that no CPU accesses to this memory can happen, then I 
think the only way to absolutely guarantee that is to exclude it from 
the kernel's memory map in the first place, e.g. as a DT reserved-memory 
region with the "no-map" property.

Robin.

> Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>   kernel/dma/direct.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 4c6c5e0635e3..aa10b4c5d762 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -13,6 +13,7 @@
>   #include <linux/vmalloc.h>
>   #include <linux/set_memory.h>
>   #include <linux/slab.h>
> +#include <asm/cacheflush.h>
>   #include "direct.h"
>   
>   /*
> @@ -169,6 +170,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,
>   		if (!PageHighMem(page))
>   			arch_dma_prep_coherent(page, size);
>   		*dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
> +		/* remove kernel mapping for pages */
> +		set_memory_valid((unsigned long)phys_to_virt(dma_to_phys(dev, *dma_handle)),
> +				size >> PAGE_SHIFT, 0);
>   		/* return the page pointer as the opaque cookie */
>   		return page;
>   	}
> @@ -278,6 +282,10 @@ void dma_direct_free(struct device *dev, size_t size,
>   
>   	if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
>   	    !force_dma_unencrypted(dev) && !is_swiotlb_for_alloc(dev)) {
> +		size = PAGE_ALIGN(size);
> +		/* create kernel mapping for pages */
> +		set_memory_valid((unsigned long)phys_to_virt(dma_to_phys(dev, dma_addr)),
> +				size >> PAGE_SHIFT, 1);
>   		/* cpu_addr is a struct page cookie, not a kernel address */
>   		dma_free_contiguous(dev, cpu_addr, size);
>   		return;
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[Walter Wu]

Hi Robin,

On Mon, 2021-11-01 at 10:29 +0000, Robin Murphy wrote:
> On 2021-11-01 03:15, Walter Wu wrote:
> > DMA_ATTR_NO_KERNEL_MAPPING is to avoid creating a kernel mapping
> > for the allocated buffer, but current implementation is that
> > PTE of allocated buffer in kernel page table is valid. So we
> > should set invalid for PTE of allocate buffer so that there are
> > no kernel mapping for the allocated buffer.
> 
> No, the semantic of NO_KERNEL_MAPPING is an indication that the
> *caller* 
> does not need a mapping, such that the DMA API implementation may
> choose 
> to optimise for that internally. It has never given any guarantee of
> any 
> particular behaviour - like most attributes it is only a hint.
> 
> > In some cases, we don't hope the allocated buffer to be read
> > by cpu or speculative execution, so we use
> > DMA_ATTR_NO_KERNEL_MAPPING
> > to get no kernel mapping in order to achieve this goal.
> 
> If it's important that no CPU accesses to this memory can happen,
> then I 
> think the only way to absolutely guarantee that is to exclude it
> from 
> the kernel's memory map in the first place, e.g. as a DT reserved-
> memory 
> region with the "no-map" property.
> 

Yes, this is our previous implementation, but we hope to use kernel
memory to fix it.

Thanks for your suggestion.

Walter

> Robin.
> 
> > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >   kernel/dma/direct.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> > index 4c6c5e0635e3..aa10b4c5d762 100644
> > --- a/kernel/dma/direct.c
> > +++ b/kernel/dma/direct.c
> > @@ -13,6 +13,7 @@
> >   #include <linux/vmalloc.h>
> >   #include <linux/set_memory.h>
> >   #include <linux/slab.h>
> > +#include <asm/cacheflush.h>
> >   #include "direct.h"
> >   
> >   /*
> > @@ -169,6 +170,9 @@ void *dma_direct_alloc(struct device *dev,
> > size_t size,
> >   		if (!PageHighMem(page))
> >   			arch_dma_prep_coherent(page, size);
> >   		*dma_handle = phys_to_dma_direct(dev,
> > page_to_phys(page));
> > +		/* remove kernel mapping for pages */
> > +		set_memory_valid((unsigned
> > long)phys_to_virt(dma_to_phys(dev, *dma_handle)),
> > +				size >> PAGE_SHIFT, 0);
> >   		/* return the page pointer as the opaque cookie */
> >   		return page;
> >   	}
> > @@ -278,6 +282,10 @@ void dma_direct_free(struct device *dev,
> > size_t size,
> >   
> >   	if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
> >   	    !force_dma_unencrypted(dev) && !is_swiotlb_for_alloc(dev))
> > {
> > +		size = PAGE_ALIGN(size);
> > +		/* create kernel mapping for pages */
> > +		set_memory_valid((unsigned
> > long)phys_to_virt(dma_to_phys(dev, dma_addr)),
> > +				size >> PAGE_SHIFT, 1);
> >   		/* cpu_addr is a struct page cookie, not a kernel
> > address */
> >   		dma_free_contiguous(dev, cpu_addr, size);
> >   		return;
> > 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[Christoph Hellwig]

As others pointed out, DMA_ATTR_NO_KERNEL_MAPPING just means the
caller can't rely on a kernel mapping.  So the "fix" here is
wrong.  That being said for cases where we can easily remove a page
from the kernel mapping it would be nice to do to:

 a) improve security
 b) as a debug check to see that no one actually tries to access it

> +		/* remove kernel mapping for pages */
> +		set_memory_valid((unsigned long)phys_to_virt(dma_to_phys(dev, *dma_handle)),

Please avoid overly long lines.  Also this function only exists for arm64
also and others pointed out won't work for all cases.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[Walter Wu]

Hi Cristoph,

On Tue, 2021-11-02 at 07:41 +0100, Christoph Hellwig wrote:
> As others pointed out, DMA_ATTR_NO_KERNEL_MAPPING just means the
> caller can't rely on a kernel mapping.  So the "fix" here is
> wrong.  That being said for cases where we can easily remove a page
> from the kernel mapping it would be nice to do to:
> 
>  a) improve security
>  b) as a debug check to see that no one actually tries to access it
> 

I will modify my commit message. Thanks for your comment.

> > +		/* remove kernel mapping for pages */
> > +		set_memory_valid((unsigned
> > long)phys_to_virt(dma_to_phys(dev, *dma_handle)),
> 
> Please avoid overly long lines.  Also this function only exists for
> arm64
> also and others pointed out won't work for all cases.

Got it. I will send v2 patch.

Thanks for your review and suggestion.


Walter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING

[Walter Wu]

Hi Christoph,

I am sorry, fix my typo.


Walter

On Tue, 2021-11-02 at 07:41 +0100, Christoph Hellwig wrote:
> As others pointed out, DMA_ATTR_NO_KERNEL_MAPPING just means the
> caller can't rely on a kernel mapping.  So the "fix" here is
> wrong.  That being said for cases where we can easily remove a page
> from the kernel mapping it would be nice to do to:
> 
>  a) improve security
>  b) as a debug check to see that no one actually tries to access it
> 
> > +		/* remove kernel mapping for pages */
> > +		set_memory_valid((unsigned
> > long)phys_to_virt(dma_to_phys(dev, *dma_handle)),
> 
> Please avoid overly long lines.  Also this function only exists for
> arm64
> also and others pointed out won't work for all cases.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel