Re: [PATCH v3] dma-debug: fix incorrect pfn calculation

From: Miles Chen
Date: Mon Oct 02 2017 - 06:30:57 EST


On Sun, 2017-10-01 at 10:04 +0200, Christoph Hellwig wrote:
> On Wed, Sep 27, 2017 at 11:23:52AM +0100, Robin Murphy wrote:
> > > I found that debug_dma_alloc_coherent() and debug_dma_free_coherent()
> > > assume that dma_alloc_coherent() always returns a linear address.
> > > However it's possible that dma_alloc_coherent() returns a non-linear
> > > address. In this case, page_to_pfn(virt_to_page(virt)) will return an
> > > incorrect pfn. If the pfn is valid and mapped as a COW page,
> > > we will hit the warning when doing wp_page_copy().
>
> Hmm, can the debug code assume anything? Right now you're just patching
> it from supporting linear and vmalloc. But what about other
> potential mapping types?

thanks for the review.

ARCHs like metag and xtensa define their mappings (non-vmalloc and
non-linear) for dma allocation.
These mapping types are architecture-dependent and should not be used
outside arch folders. So it is hard to check the mappings and convert
a virtual address to a correct pfn in lib/dam-debug.c

How about recording only vmalloc (by is_vmalloc_addr()) and linear
address (by virt_addr_valid()) in lib/dma-debug? Since current
implementation is not correct for those ARCHs.

if (!is_vmalloc_addr(addr) && !virt_addr_valid(addr))
return;

or

every ARCH should define its own dmava-to-pfn API to convert
a dma-allocted virtual address to a correct pfn and lib/dma-debug.c
can use that API directly. (long-term)

>
> > > + entry->pfn = is_vmalloc_addr(virt) ? vmalloc_to_pfn(virt) :
> > > + page_to_pfn(virt_to_page(virt));
>
> Please use normal if/else conditionsals:

Is this for better readability? I'll send another patch for this.


thanks
>
> if (is_vmalloc_addr(virt))
> entry->pfn = vmalloc_to_pfn(virt);
> else
> entry->pfn = page_to_pfn(virt_to_page(virt));
>
> > > + .pfn = is_vmalloc_addr(virt) ? vmalloc_to_pfn(virt) :
> > > + page_to_pfn(virt_to_page(virt)),
>
> Same here.