On Tue, Jan 5, 2016 at 6:13 PM, David Laight <David.Laight@xxxxxxxxxx> wrote: > From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx] >> Sent: 05 January 2016 15:58 >> On Tue, 5 Jan 2016, David Laight wrote: >> >> > From: Steinar H. Gunderson [mailto:sesse@xxxxxxxxxx] >> > > Sent: 26 November 2015 00:19 >> > >> > There is still a problem with this code, not sure how to fix it. >> > >> > ... >> > > +static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count) >> > > +{ >> > > + struct usb_dev_state *ps = usbm->ps; >> > > + unsigned long flags; >> > > + >> > > + spin_lock_irqsave(&ps->lock, flags); >> > > + --*count; >> > > + if (usbm->urb_use_count == 0 && usbm->vma_use_count == 0) { >> > ... >> > > + module_put(THIS_MODULE); >> > > + } else { >> > > + spin_unlock_irqrestore(&ps->lock, flags); >> > > + } >> > > +} >> > ... >> > > +static void usbdev_vm_close(struct vm_area_struct *vma) >> > > +{ >> > > + struct usb_memory *usbm = vma->vm_private_data; >> > > + >> > > + dec_usb_memory_use_count(usbm, &usbm->vma_use_count); >> > > +} >> > >> > If the last reference to the module is for an mmap request >> > (which is the only reason to reference count the module here) >> > then the module_put() in dec_usb_memory_use_count()) returns >> > back into freed memory. >> >> Ooh, you're right. > > In fact every call of module_put(THIS_MODULE) is probably either buggy > or unnecessary. > >> > There isn't much the module can do about it either apart >> > from using kthread_run() to call module_put_and_exit() >> > and even that is somewhat racy (a sleep in the kthread >> > is probably enough). >> > >> > The only real solution is for mmap() itself to take the >> > reference on the module. >> >> I agree; the vm_operations_struct structure ought to have a .owner >> member. But I don't feel like going through and changing all of them. > > The .owner from the device can be copied into the 'vm_area_struct', > vm_operations_struct doesn't need changing. > >> How do other drivers handle this problem? Perhaps they don't face it >> because they don't use DMA mapping to implement mmap. A normal MMIO >> sort of page mapping doesn't cause difficulties if you leave it in >> place after the device is unregistered. > > It can cause issues. > If you remove a PCIe device (echo 1 >/sys/devices/pci*/*/remove) and don't > invalidate any corresponding mmap vma then the same bus addresses could > be assigned to a different card if a rescan is done. > > I think that zap_page_range() could be called with parameters taken > from the vma in order to cause the .fault function be called. > However zap_page_range() isn't exported in mainline kernels. > > I'm trying to get the same effect by calling remap_pfn_range() to > replace the dma/io page with a vmalloc()ed one - but so far have > only have multiple oops and system lockups. > It might be because I'm trying to remap pages for the wrong process > (or I might just have the args to remap_pfn_range() wrong). > > There is a further problem that it is basically impossible to remove > timing windows between an application unmap() and a card removal event > because the required locks can't be obtained in the same order in > both cases. > >> Or maybe I just don't understand the problem very well and the core >> kernel handles all of this for us. I'll try asking. >> >> kthread_run plus sleep and module_put_and_exit might turn out to be the >> way to go. If anyone has a better suggestion, I'd like to hear it. > > If you know the device is open you can directly call module_put() > (with a mutex/sema held to block the close) since you know it > can't be the last reference. > I'd recommend to commit it as it is for now. This mmap code has been out there for years personally I'm not interested in touching this code anymore until there's a step forward. I've seen far worse issues with other APIs in the past and issues have been fixed once things were committed already. (example? the cuse/fuse framework which created oops'es on closing the device nodes.., linuxdvb API which comes up with regular regressions) Who's going to unload the usb controller stack while a USBFS driver is active? Practically this issue is absolutely irrelevant. I agree it should be fixed but no one is going to use it while the patch is just floating around on the Mailinglist for years. Markus > David > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
