Re: [PATCH] dma-direct: zero out DMA_ATTR_NO_KERNEL_MAPPING buf

[Marek Szyprowski]

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@xxxxxxxxxxxx>
>>>> Cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
>>>> Cc: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>>>> Cc: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
>>>> Signed-off-by: Hillf Danton <hdanton@xxxxxxxx>
>>>> ---
>>>>
>>>> --- 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