Re: [PATCH] dma-debug: Check for drivers mapping vmalloc addresses

[Stephen Boyd]

Quoting Robin Murphy (2018-09-26 06:24:42)
> On 21/09/18 18:39, Stephen Boyd wrote:
> > Quoting Robin Murphy (2018-09-21 04:09:50)
> >> Hi Stephen,
> >>
> >> On 20/09/18 23:35, Stephen Boyd wrote:
> >>> I recently debugged a DMA mapping oops where a driver was trying to map
> >>> a buffer returned from request_firmware() with dma_map_single(). Memory
> >>> returned from request_firmware() is mapped into the vmalloc region and
> >>> this isn't a valid region to map with dma_map_single() per the DMA
> >>> documentation's "What memory is DMA'able?" section.
> >>>
> >>> Unfortunately, we don't really check that in the DMA debugging code, so
> >>> enabling DMA debugging doesn't help catch this problem. Let's add a new
> >>> DMA debug function to check for a vmalloc address and print a warning if
> >>> this happens. This makes it a little easier to debug these sorts of
> >>> problems, instead of seeing odd behavior or crashes when drivers attempt
> >>> to map the vmalloc space for DMA.
> >>
> >> Good idea!
> >>
> >>> Cc: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> >>> Cc: Robin Murphy <robin.murphy@xxxxxxx>
> >>> Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> >>> ---
> >>>    include/linux/dma-debug.h   |  8 ++++++++
> >>>    include/linux/dma-mapping.h |  1 +
> >>>    kernel/dma/debug.c          | 12 ++++++++++++
> >>>    3 files changed, 21 insertions(+)
> >>
> >> However I can't help thinking this looks a little heavyweight for a
> >> single specific check. It seems like it would be enough to simply pass
> >> the VA as an extra argument to debug_dma_map_page(), since we already
> >> have the map_single argument which would indicate when it's valid. What
> >> do you reckon?
> > 
> > I thought about augmenting debug_dma_map_page() but that has another
> > problem where those checks are done after the page has been mapped. The
> > kernel can oops when it's operating on the incorrect page so we need to
> > check things before calling the dma ops.
> 
> Oh, right, as usual I've managed to overlook a subtlety :)
> 
> In fact, with memory now jogged, I think the virt_to_page() itself is 
> allowed to blow up if !virt_addr_valid(), so we probably do need 
> something like your original idea to be properly robust. I guess it 
> probably makes sense to implement that as debug_dma_map_single() then, 
> since the purpose is really to cover any VA checks specific to that case 
> which debug_dma_map_page() can't be expected to do after the fact.
> 

Ok. I can rename it to dma_debug_map_single() and throw in a
virt_addr_valid() check too. Thanks for the suggestion.