On Tue, Jan 19, 2016 at 12:47 AM, Markus Rechberger <mrechberger@xxxxxxxxx> wrote: > 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. > regarding "supporting this interface" if we decide to use this interface we have to support it for our customers as well. Customers would go crazy if their devices would stop to work with Linux, so it's in our interest to have it work properly. And we have a quite good userbase all over the world. But I refuse any further support until this is committed, the initial patch which got modified here has a good impact on several systems. I'm not sure but I think I have seen a 3rd memory map approach for usbfs which also got dumped, this just proves that the attitude to get this done is extremely lousy and people are just wasting their time on touching this code. Markus -- 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
