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. 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
