| Message ID | 20200514112711.1858252-1-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | a0e710a7def471b8eb779ff551fc27701da49599 |
| Headers | show |
| Series | USB: usbfs: fix mmap dma mismatch | expand |
Hi, So looking at hcd_buffer_alloc() again, there are 4 cases, localmem_pool, hcd_uses_dma, dma_pool_alloc and dma_alloc_coherent directly. The dma_pool_alloc appears to just be using dma_alloc_coherent, so its really three cases. Those three cases appear to be handled below: So: Reviewed-by: Jeremy Linton <jeremy.linton@arm.com> I'm testing it now... Thanks, On 5/14/20 6:27 AM, Greg Kroah-Hartman wrote: > In commit 2bef9aed6f0e ("usb: usbfs: correct kernel->user page attribute > mismatch") we switched from always calling remap_pfn_range() to call > dma_mmap_coherent() to handle issues with systems with non-coherent USB host > controller drivers. Unfortunatly, as syzbot quickly told us, not all the world > is host controllers with DMA support, so we need to check what host controller > we are attempting to talk to before doing this type of allocation. > > Thanks to Christoph for the quick idea of how to fix this. > > Cc: Christoph Hellwig <hch@lst.de> > Cc: Hillf Danton <hdanton@sina.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Jeremy Linton <jeremy.linton@arm.com> > Reported-by: syzbot+353be47c9ce21b68b7ed@syzkaller.appspotmail.com > Fixes: 2bef9aed6f0e ("usb: usbfs: correct kernel->user page attribute mismatch") > Cc: stable <stable@vger.kernel.org> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/usb/core/devio.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > 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; >
On Thu, May 14, 2020 at 01:27:11PM +0200, Greg Kroah-Hartman wrote: > + 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; > + } What about a goto label to share the error handling path? Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, May 14, 2020 at 01:58:29PM +0200, Christoph Hellwig wrote: > On Thu, May 14, 2020 at 01:27:11PM +0200, Greg Kroah-Hartman wrote: > > + 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; > > + } > > What about a goto label to share the error handling path? I thought about that, but that's a bit messier than the duplicated lines here :) > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> thanks for the review, and the help with this. greg k-h
On Thu, May 14, 2020 at 02:09:44PM +0200, Greg Kroah-Hartman wrote: > > > + if (dma_mmap_coherent(hcd->self.sysdev, vma, mem, dma_handle, > > > + size)) { > > > + dec_usb_memory_use_count(usbm, &usbm->vma_use_count); > > > + return -EAGAIN; > > > + } > > > > What about a goto label to share the error handling path? > > I thought about that, but that's a bit messier than the duplicated lines > here :) Actually the error handling looks weird, we can just use normal unwinding here with an extra call to usb_free_coherent. Also -EAGAIN is a strange error to return in this case, as it is simply incorrect. I think passing through the errors from dma_mmap_coherent and remap_pfn_range would make a lot more sense. Last but not least I wonder if this is the right place to open code the localmem and has_dma checks - from a layering POV it should be a usb_mmap_coherent helper at the same level as usb_alloc_coherent.
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;
In commit 2bef9aed6f0e ("usb: usbfs: correct kernel->user page attribute mismatch") we switched from always calling remap_pfn_range() to call dma_mmap_coherent() to handle issues with systems with non-coherent USB host controller drivers. Unfortunatly, as syzbot quickly told us, not all the world is host controllers with DMA support, so we need to check what host controller we are attempting to talk to before doing this type of allocation. Thanks to Christoph for the quick idea of how to fix this. Cc: Christoph Hellwig <hch@lst.de> Cc: Hillf Danton <hdanton@sina.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Jeremy Linton <jeremy.linton@arm.com> Reported-by: syzbot+353be47c9ce21b68b7ed@syzkaller.appspotmail.com Fixes: 2bef9aed6f0e ("usb: usbfs: correct kernel->user page attribute mismatch") Cc: stable <stable@vger.kernel.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/usb/core/devio.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)