LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [PATCH for v3.19, v2] Avoid that sd_shutdown() triggers a kernel warning
       [not found] ` <Pine.LNX.4.44L0.1501121116220.1707-100000@iolanthe.rowland.org>
@ 2015-01-14  9:33   ` Christoph Hellwig
  2015-01-14 15:07     ` Alan Stern
  2015-01-20 15:11     ` [PATCH for v3.19, v2] Avoid that sd_shutdown() triggers a kernel warning Alan Stern
  0 siblings, 2 replies; 8+ messages in thread
From: Christoph Hellwig @ 2015-01-14  9:33 UTC (permalink / raw)
  To: Alan Stern
  Cc: Christoph Hellwig, Bart Van Assche, James Bottomley,
	Hannes Reinecke, linux-scsi, Greg Kroah-Hartman, linux-kernel

On Mon, Jan 12, 2015 at 11:29:15AM -0500, Alan Stern wrote:
> 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?

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH for v3.19, v2] Avoid that sd_shutdown() triggers a kernel warning
  2015-01-14  9:33   ` [PATCH for v3.19, v2] Avoid that sd_shutdown() triggers a kernel warning Christoph Hellwig
@ 2015-01-14 15:07     ` Alan Stern
  2015-01-15 16:06       ` Christoph Hellwig
  2015-01-20 15:11     ` [PATCH for v3.19, v2] Avoid that sd_shutdown() triggers a kernel warning Alan Stern
  1 sibling, 1 reply; 8+ messages in thread
From: Alan Stern @ 2015-01-14 15:07 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Bart Van Assche, James Bottomley, Hannes Reinecke, linux-scsi,
	Greg Kroah-Hartman, linux-kernel

On Wed, 14 Jan 2015, Christoph Hellwig wrote:

> On Mon, Jan 12, 2015 at 11:29:15AM -0500, Alan Stern wrote:
> > 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?
> 
> Looks like we should still get an unconditional reference to
> the device using get_device in scsi_rescan_device at least.

I'm not sure that's necessary.  scsi_rescan_device is exposed by sysfs, 
and the kernfs core insures that the underlying device won't be 
deallocated while a sysfs method runs.

(scsi_rescan_device is also EXPORTed, so in theory it could be called 
under less safe circumstances.  Perhaps then the burden should fall on 
the caller to guarantee that the device won't be deallocated.)

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

Alan Stern


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH for v3.19, v2] Avoid that sd_shutdown() triggers a kernel warning
  2015-01-14 15:07     ` Alan Stern
@ 2015-01-15 16:06       ` Christoph Hellwig
  2015-01-15 18:22         ` sysfs methods can race with ->remove Alan Stern
  0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2015-01-15 16:06 UTC (permalink / raw)
  To: Alan Stern
  Cc: Bart Van Assche, James Bottomley, Hannes Reinecke, linux-scsi,
	Greg Kroah-Hartman, linux-kernel

On Wed, Jan 14, 2015 at 10:07:00AM -0500, Alan Stern wrote:
> 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.

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* sysfs methods can race with ->remove
  2015-01-15 16:06       ` Christoph Hellwig
@ 2015-01-15 18:22         ` Alan Stern
  2015-01-15 19:40           ` Tejun Heo
  0 siblings, 1 reply; 8+ messages in thread
From: Alan Stern @ 2015-01-15 18:22 UTC (permalink / raw)
  To: Christoph Hellwig, Tejun Heo
  Cc: Bart Van Assche, James Bottomley, Hannes Reinecke, linux-scsi,
	Greg Kroah-Hartman, Kernel development list

Tejun:

The context is that we have been talking about
drivers/scsi/scsi_scan.c:scsi_rescan_device(), which is called by the
store_rescan_field() sysfs method in scsi_sysfs.c.  The problem is
this: What happens in scsi_rescan_device if the device is unbound from
its driver before the module_put call?  The dev->driver->owner
calculation would dereference a NULL pointer.

On Thu, 15 Jan 2015, Christoph Hellwig wrote:

> On Wed, Jan 14, 2015 at 10:07:00AM -0500, Alan Stern wrote:
> > 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.

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

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

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?

Alan Stern


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: sysfs methods can race with ->remove
  2015-01-15 18:22         ` sysfs methods can race with ->remove Alan Stern
@ 2015-01-15 19:40           ` Tejun Heo
  2015-01-26 17:19             ` Christoph Hellwig
  0 siblings, 1 reply; 8+ messages in thread
From: Tejun Heo @ 2015-01-15 19:40 UTC (permalink / raw)
  To: Alan Stern
  Cc: Christoph Hellwig, Bart Van Assche, James Bottomley,
	Hannes Reinecke, linux-scsi, Greg Kroah-Hartman,
	Kernel development list

Hello, Alan.

On Thu, Jan 15, 2015 at 01:22:03PM -0500, Alan Stern wrote:
> > 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.

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

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

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH for v3.19, v2] Avoid that sd_shutdown() triggers a kernel warning
  2015-01-14  9:33   ` [PATCH for v3.19, v2] Avoid that sd_shutdown() triggers a kernel warning Christoph Hellwig
  2015-01-14 15:07     ` Alan Stern
@ 2015-01-20 15:11     ` Alan Stern
  1 sibling, 0 replies; 8+ messages in thread
From: Alan Stern @ 2015-01-20 15:11 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Bart Van Assche, James Bottomley, Hannes Reinecke, linux-scsi,
	Greg Kroah-Hartman, Kernel development list

On Wed, 14 Jan 2015, Christoph Hellwig wrote:

> On Mon, Jan 12, 2015 at 11:29:15AM -0500, Alan Stern wrote:
> > 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?
> 
> 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.

This particular problem comes down to the fact that 
scsi_rescan_device() accesses dev->driver without appropriate mutual 
exclusion.  SCSI's scan_mutex won't help because it doesn't protect 
dev->driver.  Rather, dev->driver is protected by dev->mutex, and so 
scsi_rescan_device() needs to use device_lock/unlock.

This suggests that the scan_mutex may not be necessary at all.
Historically, it seems to be quite old, predating the device model.  
Now that we have the device model, maybe scan_mutex simply isn't 
needed.

Scanning for channels or targets beneath a host should be protected by
shost->gendev.mutex.  Scanning for logical units beneath a target
should be protected by starget->dev.mutex.  Scanning for partitions 
beneath a SCSI drive should be protected by sdev->sdev_gendev.mutex.

James, here's a related question.  Suppose userspace writes to the 
rescan attribute file for a disk drive for sd_probe_async() has 
started.  What will happen?  What _ought_ to happen?  Do we need to 
call

	async_synchronize_full_domain(&scsi_sd_probe_domain);

somewhere in this pathway, or will it be okay?

Alan Stern


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: sysfs methods can race with ->remove
  2015-01-15 19:40           ` Tejun Heo
@ 2015-01-26 17:19             ` Christoph Hellwig
  2015-01-26 18:38               ` Alan Stern
  0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2015-01-26 17:19 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Alan Stern, Bart Van Assche, James Bottomley, Hannes Reinecke,
	linux-scsi, Greg Kroah-Hartman, Kernel development list

On Thu, Jan 15, 2015 at 02:40:31PM -0500, Tejun Heo wrote:
> > 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.

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

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: sysfs methods can race with ->remove
  2015-01-26 17:19             ` Christoph Hellwig
@ 2015-01-26 18:38               ` Alan Stern
  0 siblings, 0 replies; 8+ messages in thread
From: Alan Stern @ 2015-01-26 18:38 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Tejun Heo, Bart Van Assche, James Bottomley, Hannes Reinecke,
	linux-scsi, Greg Kroah-Hartman, Kernel development list

On Mon, 26 Jan 2015, Christoph Hellwig wrote:

> On Thu, Jan 15, 2015 at 02:40:31PM -0500, Tejun Heo wrote:
> > > 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.
> 
> Btw, where do we do that?  I did a walk through the code starting
> from device_del, but must have missed the obvious.

It happens in fs/kernfs/dir.c:kernfs_drain().  That routine is called 
when a sysfs file is removed, and it waits until all ongoing read/write 
operations are finished.

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

The proper lock is dev->mutex, as I mentioned in an earlier email 
(http://marc.info/?l=linux-scsi&m=142176669519982&w=2).  That lock is 
held whenever a ->remove method is called.

Alan Stern


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-01-26 18:38 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20150108131508.GA31022@infradead.org>
     [not found] ` <Pine.LNX.4.44L0.1501121116220.1707-100000@iolanthe.rowland.org>
2015-01-14  9:33   ` [PATCH for v3.19, v2] Avoid that sd_shutdown() triggers a kernel warning Christoph Hellwig
2015-01-14 15:07     ` Alan Stern
2015-01-15 16:06       ` Christoph Hellwig
2015-01-15 18:22         ` sysfs methods can race with ->remove Alan Stern
2015-01-15 19:40           ` Tejun Heo
2015-01-26 17:19             ` Christoph Hellwig
2015-01-26 18:38               ` Alan Stern
2015-01-20 15:11     ` [PATCH for v3.19, v2] Avoid that sd_shutdown() triggers a kernel warning Alan Stern

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).