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 <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Marek Szyprowski <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Matthias Brugger <[email protected]>
Cc: Andrew Morton <[email protected]>
---
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
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 <[email protected]>
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/[email protected]
On Mon, 1 Nov 2021 at 04:17, Walter Wu <[email protected]> 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 <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Marek Szyprowski <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Matthias Brugger <[email protected]>
> Cc: Andrew Morton <[email protected]>
> ---
> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Marek Szyprowski <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Matthias Brugger <[email protected]>
> Cc: Andrew Morton <[email protected]>
> ---
> 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;
>
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 <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Marek Szyprowski <[email protected]>
> > Cc: Robin Murphy <[email protected]>
> > Cc: Matthias Brugger <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > ---
> > 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;
> >
Hi Ard,
On Mon, 2021-11-01 at 09:34 +0100, Ard Biesheuvel wrote:
> On Mon, 1 Nov 2021 at 04:17, Walter Wu <[email protected]>
> 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 <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Marek Szyprowski <[email protected]>
> > Cc: Robin Murphy <[email protected]>
> > Cc: Matthias Brugger <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > ---
> > 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
> > [email protected]
> >
https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!CTRNKA9wMg0ARbw!16dLCjnvtRkaRLeCO9AQ7Fund5XL0FicZmeVaU3-WkFymr-0lbITfzwrvoJpiHlqnqIu-g$
> >
On Mon, 1 Nov 2021 at 13:21, Walter Wu <[email protected]> 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 <[email protected]>
> > 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 <[email protected]>
> > > Cc: Christoph Hellwig <[email protected]>
> > > Cc: Marek Szyprowski <[email protected]>
> > > Cc: Robin Murphy <[email protected]>
> > > Cc: Matthias Brugger <[email protected]>
> > > Cc: Andrew Morton <[email protected]>
> > > ---
> > > 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
> > > [email protected]
> > >
> https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!CTRNKA9wMg0ARbw!16dLCjnvtRkaRLeCO9AQ7Fund5XL0FicZmeVaU3-WkFymr-0lbITfzwrvoJpiHlqnqIu-g$
> > >
>
On Mon, 2021-11-01 at 15:17 +0100, Ard Biesheuvel wrote:
> On Mon, 1 Nov 2021 at 13:21, Walter Wu <[email protected]>
> 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 <[email protected]
> > > >
> > > 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 <[email protected]>
> > > > Cc: Christoph Hellwig <[email protected]>
> > > > Cc: Marek Szyprowski <[email protected]>
> > > > Cc: Robin Murphy <[email protected]>
> > > > Cc: Matthias Brugger <[email protected]>
> > > > Cc: Andrew Morton <[email protected]>
> > > > ---
> > > > 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
> > > > [email protected]
> > > >
> >
> >
https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!CTRNKA9wMg0ARbw!16dLCjnvtRkaRLeCO9AQ7Fund5XL0FicZmeVaU3-WkFymr-0lbITfzwrvoJpiHlqnqIu-g$
> > > >
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.
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.
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
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.