Avoid that sd_shutdown() triggers a kernel warning

Since kernel v3.19-rc1 module_refcount() returns 1 instead of 0 when called from inside module_exit(). This breaks the module_refcount() test in scsi_device_put() and hence causes the following kernel warning to be reported when unloading the ib_srp kernel module:

From f92b9687a9663d4bf304303658acfe03167e0667 Mon Sep 17 00:00:00 2001

From c056e4f177617d095eca495f94753f16043eb38b Mon Sep 17 00:00:00 2001

From 53378be96577c3fdb31dc1bdc0a73ec49171c763 Mon Sep 17 00:00:00 2001

Quoted 5 lines · Bart Van Assche

Since kernel v3.19-rc1 module_refcount() returns 1 instead of 0 when called from inside module_exit(). This breaks the module_refcount() test in scsi_device_put() and hence causes the following kernel warning to be reported when unloading the ib_srp kernel module:

This is getting better, but I still think we need to sort out the root cause.

The problemt started with commit 39b7f1e25 ("[SCSI] sd: Fix refcounting"), which added the calls to scsi_device_get in various struct device_driver/scsi_driver methods. From the BZ it seems like the rationale was to protect against races between ->rescan and ->remove, but instead of doing that using refcounting we better ensure that in the SCSI midlayer by taking scan_mutex around calls to ->rescan. The first attached patch does that, which allows us to functionally revert 39b7f1e25, which then also allows to revert 85b6c720 ("[SCSI] sd: fix cache flushing on module removal (and individual device removal)").

See the attached series to do that. Warnings: so far it only got minimal testing.

This seems like a good idea and the obvious (once it has been pointed out!) approach.

Perhaps not directly related to the issue at hand is this question: In scsi_rescan_device() we will now have:

	mutex_lock(&shost->scan_mutex);
	if (dev->driver && try_module_get(dev->driver->owner)) {
		struct scsi_driver *drv = to_scsi_driver(dev->driver);

		if (drv->rescan)
			drv->rescan(dev);
		module_put(dev->driver->owner);
	}
	mutex_unlock(&shost->scan_mutex);

What prevents the device from being unbound from its driver while the rescan runs? Evaluating the argument to the module_put() would then dereference a NULL pointer.

Unbind events that happen through the normal scsi_remove_host() mechanism are fine, because scsi_remove_host() locks the scan_mutex. But what about writes to the driver's sysfs "unbind" attribute?

Quoted 20 lines · Alan Stern

This seems like a good idea and the obvious (once it has been pointed out!) approach.

Perhaps not directly related to the issue at hand is this question: In scsi_rescan_device() we will now have:

	mutex_lock(&shost->scan_mutex);
	if (dev->driver && try_module_get(dev->driver->owner)) {
		struct scsi_driver *drv = to_scsi_driver(dev->driver);

		if (drv->rescan)
			drv->rescan(dev);
		module_put(dev->driver->owner);
	}
	mutex_unlock(&shost->scan_mutex);

What prevents the device from being unbound from its driver while the rescan runs? Evaluating the argument to the module_put() would then dereference a NULL pointer.

Quoted 3 lines

Unbind events that happen through the normal scsi_remove_host() mechanism are fine, because scsi_remove_host() locks the scan_mutex. But what about writes to the driver's sysfs "unbind" attribute?

Looks like we should still get an unconditional reference to the device using get_device in scsi_rescan_device at least.

But this seems like a more generic problem, and at least a quick glance at the pci_driver methods seems like others don't have a good synchroniation of ->remove against random driver methods.

and the kernfs core insures that the underlying device won't be deallocated while a sysfs method runs.

Quoted 3 lines

But this seems like a more generic problem, and at least a quick glance at the pci_driver methods seems like others don't have a good synchroniation of ->remove against random driver methods.

Can you give one or two examples?

Quoted 2 lines · Alan Stern

and the kernfs core insures that the underlying device won't be deallocated while a sysfs method runs.

It has a reference to keep it from beeing freed, but so far I can't find anything that prevents ->remove from beeing called while we are in or just before a method call.

Quoted 5 lines

Quoted 3 lines

But this seems like a more generic problem, and at least a quick glance at the pci_driver methods seems like others don't have a good synchroniation of ->remove against random driver methods.

Can you give one or two examples?

I look at the sriov_configure PCI method, or the various sub-methods under pci_driver.err_handler.

Quoted 3 lines

It has a reference to keep it from beeing freed, but so far I can't find anything that prevents ->remove from beeing called while we are in or just before a method call.

There are two types of methods to think about: Those registered by the subsystem and those registered by the driver.

If a method is registered by the driver, then the driver will unregister it when the ->remove routine runs. I don't know for certain, but I would expect that the sysfs/kernfs core will make sure that any existing method calls complete before unregister returns. This would prevent races.

If a method is registered by the subsystem, and if the method runs entirely within the subsystem's code, then ->remove doesn't matter. The driver could be unbound while the method is running and it would be okay.

The only time we have a problem is when the method is registered by the subsystem and the method calls into the driver. (Note that this is exactly what happens with scsi_rescan_device.)

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

But this seems like a more generic problem, and at least a quick glance at the pci_driver methods seems like others don't have a good synchroniation of ->remove against random driver methods.

Can you give one or two examples?

Quoted 2 lines

I look at the sriov_configure PCI method, or the various sub-methods under pci_driver.err_handler.

The sriov_numvfs_store method does have the same problem, and so does the reset_store method (by way of pci_reset_function -> pci_dev_save_and_disable -> pci_reset_notify).

Tejun, is my analysis correct? How should we fix these races?

Quoted 5 lines

If a method is registered by the driver, then the driver will unregister it when the ->remove routine runs. I don't know for certain, but I would expect that the sysfs/kernfs core will make sure that any existing method calls complete before unregister returns. This would prevent races.

Yes, attribute deletions are blocked till the on-going sysfs read/write operations are finished and further rw accesses are failed.

Quoted 5 lines

The sriov_numvfs_store method does have the same problem, and so does the reset_store method (by way of pci_reset_function -> pci_dev_save_and_disable -> pci_reset_notify).

Tejun, is my analysis correct? How should we fix these races?

I'm not really following what the actual problem case is, so SCSI subsystem store methods are derefing dev->driver without synchronizing against detach events? If that's the case, the solution would be synchronizing against attach/detach events? Sorry if I'm being totally idiotic. I'm having a bit of hard time jumping right in. :)

Quoted 6 lines · Tejun Heo

Quoted 5 lines

If a method is registered by the driver, then the driver will unregister it when the ->remove routine runs. I don't know for certain, but I would expect that the sysfs/kernfs core will make sure that any existing method calls complete before unregister returns. This would prevent races.

Quoted 2 lines

Yes, attribute deletions are blocked till the on-going sysfs read/write operations are finished and further rw accesses are failed.

Btw, where do we do that? I did a walk through the code starting from device_del, but must have missed the obvious.

Quoted 11 lines

Quoted 5 lines

The sriov_numvfs_store method does have the same problem, and so does the reset_store method (by way of pci_reset_function -> pci_dev_save_and_disable -> pci_reset_notify).

Tejun, is my analysis correct? How should we fix these races?

I'm not really following what the actual problem case is, so SCSI subsystem store methods are derefing dev->driver without synchronizing against detach events? If that's the case, the solution would be synchronizing against attach/detach events? Sorry if I'm being totally idiotic. I'm having a bit of hard time jumping right in. :)

No problem. That's the basic situation we are talking about. I have a serie fixing some long standing issues in the device model integration in SCSI, and pointed out a possible issue in that area.

So what is the proper lock to take to prevent ->remove from beeing called while in such a method? A mentioned about I tried to peel through all the layers of the onion^H^H^H^H^Hdriver core, but so far couldn't find anything obvious.

Quoted 16 lines · Alan Stern

This seems like a good idea and the obvious (once it has been pointed out!) approach.

Perhaps not directly related to the issue at hand is this question: In scsi_rescan_device() we will now have:

	mutex_lock(&shost->scan_mutex);
	if (dev->driver && try_module_get(dev->driver->owner)) {
		struct scsi_driver *drv = to_scsi_driver(dev->driver);

		if (drv->rescan)
			drv->rescan(dev);
		module_put(dev->driver->owner);
	}
	mutex_unlock(&shost->scan_mutex);

Quoted 7 lines

What prevents the device from being unbound from its driver while the rescan runs? Evaluating the argument to the module_put() would then dereference a NULL pointer.

Unbind events that happen through the normal scsi_remove_host() mechanism are fine, because scsi_remove_host() locks the scan_mutex. But what about writes to the driver's sysfs "unbind" attribute?

Looks like we should still get an unconditional reference to the device using get_device in scsi_rescan_device at least.

But this seems like a more generic problem, and at least a quick glance at the pci_driver methods seems like others don't have a good synchroniation of ->remove against random driver methods.

Since kernel v3.19-rc1 module_refcount() returns 1 instead of 0 when called from inside module_exit(). This breaks the module_refcount() test in scsi_device_put() and hence causes the following kernel warning to be reported when unloading the ib_srp kernel module: