lkml.org 
[lkml]   [2020]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: Validating dma_mmap_coherent() parameters before calling (was Re: WARNING in memtype_reserve)
    From
    Date
    On 5/14/20 2:46 AM, Greg KH wrote:
    > On Thu, May 14, 2020 at 08:31:58AM +0200, Christoph Hellwig wrote:
    >> On Thu, May 14, 2020 at 08:27:50AM +0200, Greg KH wrote:
    >>> On Thu, May 14, 2020 at 08:14:17AM +0200, Christoph Hellwig wrote:
    >>>> Guys, can you please start formal thread on this? I have no
    >>>> idea where this came from and what the rationale is. Btw, if the
    >>>> pfn is crap in dma_direct_mmap then the dma_addr_t passed in is
    >>>> crap, as it is derived from that. What is the caller, and how is
    >>>> this triggered?
    >>>
    >>>
    >>> Ok, to summarize, commit 2bef9aed6f0e ("usb: usbfs: correct kernel->user
    >>> page attribute mismatch") changed a call from remap_pfn_range() to
    >>> dma_mmap_coherent() for usb data buffers being sent from userspace.
    >>
    >> I only need to look at the commit for 3 seconds to tell you that it is
    >> completely buggy. While using dma_mmap_coherent is fundamentally the
    >> right thing and absolutely required for dma_alloc_* allocations, USB
    >> also uses it's own local gen pool allocator or plain kmalloc for not
    >> DMA capable controller. This need to use remap_pfn_range. I'm pretty
    >> sure you hit one of those cases.
    >>
    >> The logic should be something like:
    >>
    >> if (hcd->localmem_pool || !hcd_uses_dma(hcd))
    >> remap_pfn_range()
    >> else
    >> dma_mmap_coherent()
    >
    > Ok, that's simple enough, patch is below.
    >
    > Jeremy, any objection to this change?

    No, thats fine but since I just translated usb_alloc_coherent() to
    dma_map_coherent in my not fully away head. Putting this as
    "usb_map_cohernet()" sort of makes more sense.


    >
    > thanks,
    >
    > greg k-h
    >
    >
    > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
    > index b9db9812d6c5..d93d94d7ff50 100644
    > --- a/drivers/usb/core/devio.c
    > +++ b/drivers/usb/core/devio.c
    > @@ -251,9 +251,19 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
    > usbm->vma_use_count = 1;
    > INIT_LIST_HEAD(&usbm->memlist);
    >
    > - if (dma_mmap_coherent(hcd->self.sysdev, vma, mem, dma_handle, size)) {
    > - dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
    > - return -EAGAIN;
    > + if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
    > + if (remap_pfn_range(vma, vma->vm_start,
    > + virt_to_phys(usbm->mem) >> PAGE_SHIFT,
    > + size, vma->vm_page_prot) < 0) {
    > + dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
    > + return -EAGAIN;
    > + }
    > + } else {
    > + if (dma_mmap_coherent(hcd->self.sysdev, vma, mem, dma_handle,
    > + size)) {
    > + dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
    > + return -EAGAIN;
    > + }
    > }
    >
    > vma->vm_flags |= VM_IO;
    >

    \
     
     \ /
      Last update: 2020-05-14 13:19    [W:3.172 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site