Re: PCI driver module unload race?

[Petr Vandrovec <vandrove@vc.cvut.cz>]

[Scroll to the end...]

> The case where while probe() is called, the module is unloaded.
> Same thing for remove().
> 
> That's all.
> 
> > After driver calls pci_unregister_driver,
> > it is sure that there are no other users of this pci driver.
> 
> Sure, but that's not the case of what we are protecting here.  We want
> pci_unregister_driver() (which is usually called from the module_exit()

Usually! pci_unregister_driver has nothing to do with module_exit(), unless
there is some rule which says that pci_unregister_driver may be called
from module_exit() only.

> function), to not be called if we are in the middle of calling either
> probe() or release().
> 
> Do you have a way of protecting the race that is described by Russell
> here that differs from my patch?

There must be normal subsystem locking which prevents you from such race.

Russel King wrote:

> Load PCI driver.

> PCI driver registers using pci_module_init(), and adds itself to sysfs.

> Hot-plugin a PCI device which uses this driver.  sysfs matches the PCI
> driver, and calls the PCI drivers probe function.

> The probe function calls kmalloc or some other function which sleeps
> (or gets preempted, if CONFIG_PREEMPT is enabled.)

> We switch to another thread, which happens to be rmmod for this PCI
> driver.  We remove the driver since it has a use count of zero.

> We switch back to the PCI driver.  Oops.

Both calls to ->probe (& ->remove) must happen under same lock which
is used by pci_(un)register_driver. Driver expects that there is no
device registered after return from pci_unregister_driver, and
your if (try_module_get())... violates this - you simply skip call
to ->remove if module is in unloading state - although I see no reason
why you could not enter driver's ->remove function long before
pci_unregister_driver is called.

Also pci_unregister_driver() expects ->remove to be called for all
registered devices. If you'll guard this call by try_module_get(),
no device will get unregistered from driver, causing memory leaks or
even crashes.

Well, and after reading sysfs code: there is no problem (I would be
really surprised otherwise), as both hotplug event and driver 
registration/removal are guarded by bus->subsys.rwsem semaphore, 
so pci_unregister_driver() succeeds after previous ->remove 
(or ->probe or ...) finishes.

Just make sure that people do not call device_release_driver
unless they really know what they are doing. Proper interface is
bus_remove_device or bus_remove_driver...
					Best regards,
						Petr Vandrovec
						vandrove@vc.cvut.cz