* Re: [PATCH] dma-direct: zero out DMA_ATTR_NO_KERNEL_MAPPING buf [not found] <20200904152550.17964-1-hdanton@sina.com> @ 2020-09-04 15:34 ` James Bottomley [not found] ` <20200905073528.9464-1-hdanton@sina.com> 1 sibling, 0 replies; 5+ messages in thread From: James Bottomley @ 2020-09-04 15:34 UTC (permalink / raw) To: Hillf Danton, Christoph Hellwig Cc: linux-nvme, linux-kernel, Kees Cook, Matthew Wilcox, Marek Szyprowski On Fri, 2020-09-04 at 23:25 +0800, Hillf Danton wrote: > The DMA buffer allocated is always cleared in DMA core and this is > making DMA_ATTR_NO_KERNEL_MAPPING non-special. > > Fixes: d98849aff879 ("dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING > in common code") > Cc: Kees Cook <keescook@chromium.org> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > Signed-off-by: Hillf Danton <hdanton@sina.com> > --- > > --- a/kernel/dma/direct.c > +++ b/kernel/dma/direct.c > @@ -178,9 +178,17 @@ void *dma_direct_alloc_pages(struct devi > > if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) && > !force_dma_unencrypted(dev)) { > + int i; > + > /* remove any dirty cache lines on the kernel alias > */ > if (!PageHighMem(page)) > arch_dma_prep_coherent(page, size); > + > + for (i = 0; i < size/PAGE_SIZE; i++) { > + ret = kmap_atomic(page + i); > + memset(ret, 0, PAGE_SIZE); > + kunmap_atomic(ret); This is massively expensive on PARISC and likely other VIPT/VIVT architectures. What's the reason for clearing it? This could also be really inefficient even on PIPT architectures if the memory is device remote. If we really have to do this, it should likely be done in the arch or driver hooks because there are potentially more efficient ways we can do this knowing how the architecture behaves. James ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20200905073528.9464-1-hdanton@sina.com>]
* Re: [PATCH] dma-direct: zero out DMA_ATTR_NO_KERNEL_MAPPING buf [not found] ` <20200905073528.9464-1-hdanton@sina.com> @ 2020-09-05 15:46 ` James Bottomley 2020-09-05 15:50 ` James Bottomley 1 sibling, 0 replies; 5+ messages in thread From: James Bottomley @ 2020-09-05 15:46 UTC (permalink / raw) To: Hillf Danton Cc: Christoph Hellwig, linux-nvme, linux-kernel, Kees Cook, Matthew Wilcox, Marek Szyprowski, linux-arch On Sat, 2020-09-05 at 15:35 +0800, Hillf Danton wrote: > On Fri, 04 Sep 2020 08:34:39 -0700 James Bottomley wrote: > > On Fri, 2020-09-04 at 23:25 +0800, Hillf Danton wrote: > > > The DMA buffer allocated is always cleared in DMA core and this > > > is making DMA_ATTR_NO_KERNEL_MAPPING non-special. > > > > > > Fixes: d98849aff879 ("dma-direct: handle > > > DMA_ATTR_NO_KERNEL_MAPPING > > > in common code") > > > Cc: Kees Cook <keescook@chromium.org> > > > Cc: Matthew Wilcox <willy@infradead.org> > > > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > > > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > > > Signed-off-by: Hillf Danton <hdanton@sina.com> > > > --- > > > > > > --- a/kernel/dma/direct.c > > > +++ b/kernel/dma/direct.c > > > @@ -178,9 +178,17 @@ void *dma_direct_alloc_pages(struct devi > > > > > > if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) && > > > !force_dma_unencrypted(dev)) { > > > + int i; > > > + > > > /* remove any dirty cache lines on the kernel > > > alias > > > */ > > > if (!PageHighMem(page)) > > > arch_dma_prep_coherent(page, size); > > > + > > > + for (i = 0; i < size/PAGE_SIZE; i++) { > > > + ret = kmap_atomic(page + i); > > > + memset(ret, 0, PAGE_SIZE); > > > + kunmap_atomic(ret); > > Hi James > > > > This is massively expensive on PARISC and likely other VIPT/VIVT > > architectures. > > Correct. > > > What's the reason for clearing it? This could also be > > /* we always manually zero the memory once we are done: */ > gfp &= ~__GFP_ZERO; > gfp |= dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask, > &phys_limit); That's not a reason ... that comment was put in for coherent mappings. What is the reason we should incur all this expense for clearing pages which aren't unmapped in the kernel, because we can update the comment? The usual rationale for kernel mapped pages is security, because they may leak information but unmapped pages shouldn't have this problem. > > really inefficient even on PIPT architectures if the memory is > > device remote. > > > > If we really have to do this, it should likely be done in the arch > > or driver hooks because there are potentially more efficient ways > > we can do this knowing how the architecture behaves. > > I'm open to any vintage ideas in your mind wrt clearing dma buf e.g > on platforms like PARISC. Or feel free to offload me the work if it > makes sense to you who are rich of PARISC knowledge. OK, I've cc'd linux-arch because this is a problem for more than just parisc. However, not having to do it is the best solution ... sort of the doctor, doctor it hurts when I do this answer. James ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dma-direct: zero out DMA_ATTR_NO_KERNEL_MAPPING buf [not found] ` <20200905073528.9464-1-hdanton@sina.com> 2020-09-05 15:46 ` James Bottomley @ 2020-09-05 15:50 ` James Bottomley 2020-09-07 7:02 ` Marek Szyprowski 1 sibling, 1 reply; 5+ messages in thread From: James Bottomley @ 2020-09-05 15:50 UTC (permalink / raw) To: Hillf Danton Cc: Christoph Hellwig, linux-nvme, linux-kernel, Kees Cook, Matthew Wilcox, Marek Szyprowski, linux-arch [resend with correct linux-arch address] On Sat, 2020-09-05 at 15:35 +0800, Hillf Danton wrote: > On Fri, 04 Sep 2020 08:34:39 -0700 James Bottomley wrote: > > On Fri, 2020-09-04 at 23:25 +0800, Hillf Danton wrote: > > > The DMA buffer allocated is always cleared in DMA core and this > > > is making DMA_ATTR_NO_KERNEL_MAPPING non-special. > > > > > > Fixes: d98849aff879 ("dma-direct: handle > > > DMA_ATTR_NO_KERNEL_MAPPING > > > in common code") > > > Cc: Kees Cook <keescook@chromium.org> > > > Cc: Matthew Wilcox <willy@infradead.org> > > > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > > > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > > > Signed-off-by: Hillf Danton <hdanton@sina.com> > > > --- > > > > > > --- a/kernel/dma/direct.c > > > +++ b/kernel/dma/direct.c > > > @@ -178,9 +178,17 @@ void *dma_direct_alloc_pages(struct devi > > > > > > if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) && > > > !force_dma_unencrypted(dev)) { > > > + int i; > > > + > > > /* remove any dirty cache lines on the kernel > > > alias > > > */ > > > if (!PageHighMem(page)) > > > arch_dma_prep_coherent(page, size); > > > + > > > + for (i = 0; i < size/PAGE_SIZE; i++) { > > > + ret = kmap_atomic(page + i); > > > + memset(ret, 0, PAGE_SIZE); > > > + kunmap_atomic(ret); > > Hi James > > > > This is massively expensive on PARISC and likely other VIPT/VIVT > > architectures. > > Correct. > > > What's the reason for clearing it? This could also be > > /* we always manually zero the memory once we are done: */ > gfp &= ~__GFP_ZERO; > gfp |= dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask, > &phys_limit); That's not a reason ... that comment was put in for coherent mappings. What is the reason we should incur all this expense for clearing pages which aren't unmapped in the kernel, because we can update the comment? The usual rationale for kernel mapped pages is security, because they may leak information but unmapped pages shouldn't have this problem. > > really inefficient even on PIPT architectures if the memory is > > device remote. > > > > If we really have to do this, it should likely be done in the arch > > or driver hooks because there are potentially more efficient ways > > we can do this knowing how the architecture behaves. > > I'm open to any vintage ideas in your mind wrt clearing dma buf e.g > on platforms like PARISC. Or feel free to offload me the work if it > makes sense to you who are rich of PARISC knowledge. OK, I've cc'd linux-arch because this is a problem for more than just parisc. However, not having to do it is the best solution ... sort of the doctor, doctor it hurts when I do this answer. James ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dma-direct: zero out DMA_ATTR_NO_KERNEL_MAPPING buf 2020-09-05 15:50 ` James Bottomley @ 2020-09-07 7:02 ` Marek Szyprowski 2020-09-07 7:49 ` Christoph Hellwig 0 siblings, 1 reply; 5+ messages in thread From: Marek Szyprowski @ 2020-09-07 7:02 UTC (permalink / raw) To: James Bottomley, Hillf Danton Cc: Christoph Hellwig, linux-nvme, linux-kernel, Kees Cook, Matthew Wilcox, linux-arch Hi James, On 05.09.2020 17:50, James Bottomley wrote: > [resend with correct linux-arch address] > On Sat, 2020-09-05 at 15:35 +0800, Hillf Danton wrote: >> On Fri, 04 Sep 2020 08:34:39 -0700 James Bottomley wrote: >>> On Fri, 2020-09-04 at 23:25 +0800, Hillf Danton wrote: >>>> The DMA buffer allocated is always cleared in DMA core and this >>>> is making DMA_ATTR_NO_KERNEL_MAPPING non-special. >>>> >>>> Fixes: d98849aff879 ("dma-direct: handle >>>> DMA_ATTR_NO_KERNEL_MAPPING >>>> in common code") >>>> Cc: Kees Cook <keescook@chromium.org> >>>> Cc: Matthew Wilcox <willy@infradead.org> >>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com> >>>> Cc: James Bottomley <James.Bottomley@HansenPartnership.com> >>>> Signed-off-by: Hillf Danton <hdanton@sina.com> >>>> --- >>>> >>>> --- a/kernel/dma/direct.c >>>> +++ b/kernel/dma/direct.c >>>> @@ -178,9 +178,17 @@ void *dma_direct_alloc_pages(struct devi >>>> >>>> if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) && >>>> !force_dma_unencrypted(dev)) { >>>> + int i; >>>> + >>>> /* remove any dirty cache lines on the kernel >>>> alias >>>> */ >>>> if (!PageHighMem(page)) >>>> arch_dma_prep_coherent(page, size); >>>> + >>>> + for (i = 0; i < size/PAGE_SIZE; i++) { >>>> + ret = kmap_atomic(page + i); >>>> + memset(ret, 0, PAGE_SIZE); >>>> + kunmap_atomic(ret); >> Hi James >>> This is massively expensive on PARISC and likely other VIPT/VIVT >>> architectures. >> Correct. >> >>> What's the reason for clearing it? This could also be >> /* we always manually zero the memory once we are done: */ >> gfp &= ~__GFP_ZERO; >> gfp |= dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask, >> &phys_limit); > That's not a reason ... that comment was put in for coherent mappings. > What is the reason we should incur all this expense for clearing pages > which aren't unmapped in the kernel, because we can update the comment? > The usual rationale for kernel mapped pages is security, because they > may leak information but unmapped pages shouldn't have this problem. Any dma_alloc_attrs() buffer might be mmaped to userspace, so the security reason is still valid. Possible lack if kernel mapping was only a hint that driver doesn't need it, so it might be skipped on some architectures, where creating it requires significant resources (i.e. vmalloc area). >>> really inefficient even on PIPT architectures if the memory is >>> device remote. >>> >>> If we really have to do this, it should likely be done in the arch >>> or driver hooks because there are potentially more efficient ways >>> we can do this knowing how the architecture behaves. >> I'm open to any vintage ideas in your mind wrt clearing dma buf e.g >> on platforms like PARISC. Or feel free to offload me the work if it >> makes sense to you who are rich of PARISC knowledge. > OK, I've cc'd linux-arch because this is a problem for more than just > parisc. However, not having to do it is the best solution ... sort of > the doctor, doctor it hurts when I do this answer. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dma-direct: zero out DMA_ATTR_NO_KERNEL_MAPPING buf 2020-09-07 7:02 ` Marek Szyprowski @ 2020-09-07 7:49 ` Christoph Hellwig 0 siblings, 0 replies; 5+ messages in thread From: Christoph Hellwig @ 2020-09-07 7:49 UTC (permalink / raw) To: Marek Szyprowski Cc: James Bottomley, Hillf Danton, Christoph Hellwig, linux-nvme, linux-kernel, Kees Cook, Matthew Wilcox, linux-arch On Mon, Sep 07, 2020 at 09:02:34AM +0200, Marek Szyprowski wrote: > > That's not a reason ... that comment was put in for coherent mappings. > > What is the reason we should incur all this expense for clearing pages > > which aren't unmapped in the kernel, because we can update the comment? > > The usual rationale for kernel mapped pages is security, because they > > may leak information but unmapped pages shouldn't have this problem. > > Any dma_alloc_attrs() buffer might be mmaped to userspace, so the > security reason is still valid. Possible lack if kernel mapping was only > a hint that driver doesn't need it, so it might be skipped on some > architectures, where creating it requires significant resources (i.e. > vmalloc area). Yes. It seems actually mapping it to userspace in media/drm drivers seems to be one of the big use cases. There other one is memory not used by the host at all and just as an extra buffer for hardware so that the PCIe device can cut down on DRAM cost. For that could potentially skip the zeroing, but then again you'd need to trust the device, which with thunderbolt might not always be a good idea. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-07 7:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200904152550.17964-1-hdanton@sina.com>
2020-09-04 15:34 ` [PATCH] dma-direct: zero out DMA_ATTR_NO_KERNEL_MAPPING buf James Bottomley
[not found] ` <20200905073528.9464-1-hdanton@sina.com>
2020-09-05 15:46 ` James Bottomley
2020-09-05 15:50 ` James Bottomley
2020-09-07 7:02 ` Marek Szyprowski
2020-09-07 7:49 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).