Re: PCI driver module unload race?

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

On Sat, Mar 08, 2003 at 11:12:37AM -0800, Greg KH wrote:
> On Sat, Mar 08, 2003 at 10:47:49AM +0000, Russell King wrote:
> > Hi,
> > 
> > What prevents the following scenario from happening?  It's purely
> > theoretical - I haven't seen this occuring.
> > 
> > - 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.
> 
> Ugh, yes you are correct, I can't believe I missed this before.
> 
> How does this patch look?

Bad...

What are you trying to solve? After driver calls pci_unregister_driver,
it is sure that there are no other users of this pci driver. So I do
not understand why you are adding these additional protections (and one
in remove looks suspicious to me: if you unplug device while module
in unloading, you do not call remove?! who will call it then? you'll
not get oopses in pci_unregister_driver? leaked memory?). Driver
authors simple have to unregister from subsystems in correct order,
no try_module_get() put anywhere is not going to solve this problem.

There is simple no relation between modules and drivers - and anyone
who is trying to force this is misguided. Driver starts its life
inside call to *_register_*, and ceases in *_unregister_*, not 
somewhere after call to init_module, and before cleanup_module.
And if module registers resources in wrong order, module has to
cope with its stupidity, not kernel (and I have yet to see two
kernel interfaces which cannot be properly ordered for both init 
and shutdown).
						Petr Vandrovec
						vandrove@vc.cvut.cz


> thanks,
> 
> greg k-h
> 
> 
> ===== drivers/pci/pci-driver.c 1.17 vs edited =====
> --- 1.17/drivers/pci/pci-driver.c	Sat Nov 23 13:23:03 2002
> +++ edited/drivers/pci/pci-driver.c	Sat Mar  8 11:21:06 2003
> @@ -48,6 +48,12 @@
>  	if (!pci_dev->driver && drv->probe) {
>  		const struct pci_device_id *id;
>  
> +		if (!try_module_get(drv->owner)) {
> +			dev_err(dev, "Can't get a module reference for %s\n",
> +				drv->name);
> +			error = -ENODEV;
> +			goto exit;
> +		}
>  		id = pci_match_device(drv->id_table, pci_dev);
>  		if (id)
>  			error = drv->probe(pci_dev, id);
> @@ -55,7 +61,9 @@
>  			pci_dev->driver = drv;
>  			error = 0;
>  		}
> +		module_put(drv->owner);
>  	}
> +exit:
>  	return error;
>  }
>  
> @@ -66,7 +74,10 @@
>  
>  	if (drv) {
>  		if (drv->remove)
> -			drv->remove(pci_dev);
> +			if (try_module_get(drv->owner)) {
> +				drv->remove(pci_dev);
> +				module_put(drv->owner);
> +			}
>  		pci_dev->driver = NULL;
>  	}
>  	return 0;
> ===== include/linux/pci.h 1.36 vs edited =====
> --- 1.36/include/linux/pci.h	Tue Mar  4 21:09:58 2003
> +++ edited/include/linux/pci.h	Sat Mar  8 11:20:15 2003
> @@ -493,6 +493,7 @@
>  
>  struct pci_driver {
>  	struct list_head node;
> +	struct module *owner;
>  	char *name;
>  	const struct pci_device_id *id_table;	/* NULL if wants all devices */
>  	int  (*probe)  (struct pci_dev *dev, const struct pci_device_id *id);	/* New device inserted */
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>