Re: Race condition between driver_probe_device and device_shutdown‏

Re: Race condition between driver_probe_device and device_shutdown‏

[Greg KH]

> Hi,

First off, sorry for missing this, and thanks to Andrew for pointing it
out to me.  You might want to use the tool, scripts/get_maintainer.pl
for who to know to cc: for patches like this, so I don't miss it.

> I'm seeing a driver crash in its shutdown routine because it's
> touching some uninitialized state. It turns out that the driver's
> probe routine was still running [for the same device]. There also
> appears to be an issue in the remove path, where device_shutdown()
> checks the dev->driver pointer and uses it later, with seemingly
> nothing to guarantee that it doesn't change.

What type of driver is having this problem?  What type of bus is it on?
Usually the bus prevents this from happening with its own serialization.

> Shouldn't we synchronize the shutdown routine with probe/remove to
> prevent such races?

Normally, yes, and for some reason, I thought we already were doing
that.

> The patch below should take care of these races.

Does this patch solve your problem?  Care to show me the oops you get
without it?

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index e28ce98..f2c63c6 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1823,6 +1823,9 @@ void device_shutdown(void)
>                 pm_runtime_get_noresume(dev);
>                 pm_runtime_barrier(dev);
> 
> +               if (dev->parent)        /* Needed for USB */
> +                       device_lock(dev->parent);
> +               device_lock(dev);

The parent thing is "interesting", and if we are going to have to start
duplicating this logic in different parts of the driver core, we should
wrap it up in a common function, right?

thanks,

greg k-h

Re: Race condition between driver_probe_device and device_shutdown‏

[Ming Lei]

Hi,

On Fri, May 18, 2012 at 12:59 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> Hi,
>
> First off, sorry for missing this, and thanks to Andrew for pointing it
> out to me.  You might want to use the tool, scripts/get_maintainer.pl
> for who to know to cc: for patches like this, so I don't miss it.
>
>> I'm seeing a driver crash in its shutdown routine because it's
>> touching some uninitialized state. It turns out that the driver's
>> probe routine was still running [for the same device]. There also
>> appears to be an issue in the remove path, where device_shutdown()
>> checks the dev->driver pointer and uses it later, with seemingly
>> nothing to guarantee that it doesn't change.
>
> What type of driver is having this problem?  What type of bus is it on?
> Usually the bus prevents this from happening with its own serialization.

Looks it is a generic problem.

There are two races, one is between .probe and .shutdown, and another
is between .release  and .shutdown, see below:


void device_shutdown(void)

		......
		/* Don't allow any more runtime suspends */
		pm_runtime_get_noresume(dev);
		pm_runtime_barrier(dev);

		if (dev->bus && dev->bus->shutdown) {
			dev_dbg(dev, "shutdown\n");
			dev->bus->shutdown(dev);
		} else if (dev->driver && dev->driver->shutdown) {  /*line-driver*/
			dev_dbg(dev, "shutdown\n");
			dev->driver->shutdown(dev);                              /*line-shut*/
		}
		......

If dev->driver is just set(really_probe) before 'line-driver' and .probe is
not executed before 'line-shut', the .shutdown may touch a uninitialized
device.

Also if dev->driver is just cleared(__device_release_driver) after "line-driver"
and before "line-shut", null pointer will be referenced and oops will
be triggered.


>> Shouldn't we synchronize the shutdown routine with probe/remove to
>> prevent such races?
>
> Normally, yes, and for some reason, I thought we already were doing
> that.

Looks the races are still there.

>> The patch below should take care of these races.
>
> Does this patch solve your problem?  Care to show me the oops you get
> without it?
>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index e28ce98..f2c63c6 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -1823,6 +1823,9 @@ void device_shutdown(void)
>>                 pm_runtime_get_noresume(dev);
>>                 pm_runtime_barrier(dev);
>>
>> +               if (dev->parent)        /* Needed for USB */
>> +                       device_lock(dev->parent);
>> +               device_lock(dev);

Looks the above makes sense to serialize .shutdown with
.probe and .release.


Thanks,
--
Ming Lei

Re: Race condition between driver_probe_device and device_shutdown‏

[Greg KH]

On Sun, May 20, 2012 at 10:08:08PM +0800, Ming Lei wrote:
> Hi,
> 
> On Fri, May 18, 2012 at 12:59 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> Hi,
> >
> > First off, sorry for missing this, and thanks to Andrew for pointing it
> > out to me.  You might want to use the tool, scripts/get_maintainer.pl
> > for who to know to cc: for patches like this, so I don't miss it.
> >
> >> I'm seeing a driver crash in its shutdown routine because it's
> >> touching some uninitialized state. It turns out that the driver's
> >> probe routine was still running [for the same device]. There also
> >> appears to be an issue in the remove path, where device_shutdown()
> >> checks the dev->driver pointer and uses it later, with seemingly
> >> nothing to guarantee that it doesn't change.
> >
> > What type of driver is having this problem?  What type of bus is it on?
> > Usually the bus prevents this from happening with its own serialization.
> 
> Looks it is a generic problem.
> 
> There are two races, one is between .probe and .shutdown, and another
> is between .release  and .shutdown, see below:
> 
> 
> void device_shutdown(void)
> 
> 		......
> 		/* Don't allow any more runtime suspends */
> 		pm_runtime_get_noresume(dev);
> 		pm_runtime_barrier(dev);
> 
> 		if (dev->bus && dev->bus->shutdown) {
> 			dev_dbg(dev, "shutdown\n");
> 			dev->bus->shutdown(dev);
> 		} else if (dev->driver && dev->driver->shutdown) {  /*line-driver*/
> 			dev_dbg(dev, "shutdown\n");
> 			dev->driver->shutdown(dev);                              /*line-shut*/
> 		}
> 		......
> 
> If dev->driver is just set(really_probe) before 'line-driver' and .probe is
> not executed before 'line-shut', the .shutdown may touch a uninitialized
> device.
> 
> Also if dev->driver is just cleared(__device_release_driver) after "line-driver"
> and before "line-shut", null pointer will be referenced and oops will
> be triggered.

And how can that happen with a real bus?  Don't we have a lock
somewhere per-bus that should be protecting this type of thing (sorry,
can't dig through the code at the moment, on the road...)

> >> Shouldn't we synchronize the shutdown routine with probe/remove to
> >> prevent such races?
> >
> > Normally, yes, and for some reason, I thought we already were doing
> > that.
> 
> Looks the races are still there.

How come no one has ever hit them in the past 10 years?  What am I
missing here?

> >> The patch below should take care of these races.
> >
> > Does this patch solve your problem?  Care to show me the oops you get
> > without it?
> >
> >> diff --git a/drivers/base/core.c b/drivers/base/core.c
> >> index e28ce98..f2c63c6 100644
> >> --- a/drivers/base/core.c
> >> +++ b/drivers/base/core.c
> >> @@ -1823,6 +1823,9 @@ void device_shutdown(void)
> >>                 pm_runtime_get_noresume(dev);
> >>                 pm_runtime_barrier(dev);
> >>
> >> +               if (dev->parent)        /* Needed for USB */
> >> +                       device_lock(dev->parent);
> >> +               device_lock(dev);
> 
> Looks the above makes sense to serialize .shutdown with
> .probe and .release.

Let me look at the code when I get back in a few days, but I really
thought we already had a lock protecting all of this...

thanks,

greg k-h

Re: Race condition between driver_probe_device and device_shutdown‏

[Ming Lei]

Cc pm list because it is related with PM.

Hi Greg,

On Mon, May 21, 2012 at 3:51 AM, Greg KH <gregkh@linuxfoundation.org> wrote:

>
> And how can that happen with a real bus?  Don't we have a lock

The races may be triggered when one device is just probed(triggered
by plug) or released(triggered by unplug) at the same time of running
reboot/poweroff.

> somewhere per-bus that should be protecting this type of thing (sorry,
> can't dig through the code at the moment, on the road...)

device_shutdown is called with only holding reboot_mutex, so I think no
any protection on dev->driver there.

>
> How come no one has ever hit them in the past 10 years?  What am I
> missing here?

The window is so small that maybe it is very very difficult to trigger
the races, :-)
But looks Wedson is luck enough to observe it.

>> Looks the above makes sense to serialize .shutdown with
>> .probe and .release.
>
> Let me look at the code when I get back in a few days, but I really
> thought we already had a lock protecting all of this...

Also the previous patch don't cover the .runtime_resume races with
.probe or .release, so the right fix may be below:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 346be8b..cbc8bd2 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1820,6 +1820,11 @@ void device_shutdown(void)
 		list_del_init(&dev->kobj.entry);
 		spin_unlock(&devices_kset->list_lock);

+		/*hold lock[s] to avoid races with .probe/.release*/
+		if (dev->parent)
+			device_lock(dev->parent);
+		device_lock(dev);
+
 		/* Don't allow any more runtime suspends */
 		pm_runtime_get_noresume(dev);
 		pm_runtime_barrier(dev);
@@ -1831,6 +1836,9 @@ void device_shutdown(void)
 			dev_dbg(dev, "shutdown\n");
 			dev->driver->shutdown(dev);
 		}
+		device_unlock(dev);
+		if (dev->parent)
+			device_unlock(dev->parent);
 		put_device(dev);

 		spin_lock(&devices_kset->list_lock);

Another candidate fix is to register a reboot notifier in driver core to prevent
driver from being bound or unbound from start of reboot/shutdown, but looks
not easy as the way of holding device locks.


Thanks,
--
Ming Lei

Re: Race condition between driver_probe_device and device_shutdown‏

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 2617 bytes --]

On Mon, 21 May 2012, Ming Lei wrote:

> Cc pm list because it is related with PM.
> 
> Hi Greg,
> 
> On Mon, May 21, 2012 at 3:51 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> >
> > And how can that happen with a real bus?  Don't we have a lock
> 
> The races may be triggered when one device is just probed(triggered
> by plug) or released(triggered by unplug) at the same time of running
> reboot/poweroff.
> 
> > somewhere per-bus that should be protecting this type of thing (sorry,
> > can't dig through the code at the moment, on the road...)
> 
> device_shutdown is called with only holding reboot_mutex, so I think no
> any protection on dev->driver there.
> 
> >
> > How come no one has ever hit them in the past 10 years?  What am I
> > missing here?
> 
> The window is so small that maybe it is very very difficult to trigger
> the races, :-)
> But looks Wedson is luck enough to observe it.
> 
> >> Looks the above makes sense to serialize .shutdown with
> >> .probe and .release.
> >
> > Let me look at the code when I get back in a few days, but I really
> > thought we already had a lock protecting all of this...
> 
> Also the previous patch don't cover the .runtime_resume races with
> .probe or .release, so the right fix may be below:
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 346be8b..cbc8bd2 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1820,6 +1820,11 @@ void device_shutdown(void)
>  		list_del_init(&dev->kobj.entry);
>  		spin_unlock(&devices_kset->list_lock);
> 
> +		/*hold lock[s] to avoid races with .probe/.release*/
> +		if (dev->parent)
> +			device_lock(dev->parent);
> +		device_lock(dev);
> +
>  		/* Don't allow any more runtime suspends */
>  		pm_runtime_get_noresume(dev);
>  		pm_runtime_barrier(dev);
> @@ -1831,6 +1836,9 @@ void device_shutdown(void)
>  			dev_dbg(dev, "shutdown\n");
>  			dev->driver->shutdown(dev);
>  		}
> +		device_unlock(dev);
> +		if (dev->parent)
> +			device_unlock(dev->parent);
>  		put_device(dev);
> 
>  		spin_lock(&devices_kset->list_lock);
> 
> Another candidate fix is to register a reboot notifier in driver core to prevent
> driver from being bound or unbound from start of reboot/shutdown, but looks
> not easy as the way of holding device locks.

I'd guess it was done this way so that the shutdown task wouldn't have 
to wait for a buggy driver that didn't want to release the device lock 
(or that crashed while holding the lock).

It's not clear that the reboot notifier approach would work.  What 
about probes that had already started when notifier was called?

Alan Stern

Re: Race condition between driver_probe_device and device_shutdown‏

[Ming Lei]

On Tue, May 22, 2012 at 2:29 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 21 May 2012, Ming Lei wrote:
>> Another candidate fix is to register a reboot notifier in driver core to prevent
>> driver from being bound or unbound from start of reboot/shutdown, but looks
>> not easy as the way of holding device locks.
>
> I'd guess it was done this way so that the shutdown task wouldn't have
> to wait for a buggy driver that didn't want to release the device lock
> (or that crashed while holding the lock).

Maybe, so I understand you agree on adding the device lock as did
in the patch, don't I?

Also we can add below line before acquiring device lock to help
toubleshoot buggy driver.

               dev_dbg(dev, "acquiring device lock for shutdown\n");

>
> It's not clear that the reboot notifier approach would work.  What
> about probes that had already started when notifier was called?

Looks holding device lock is still needed in the notifier callback for
handling the case if reboot notifier approach is taken.

IMO, the way of holding device lock is better than reboot notifier approach.

Thanks,
--
Ming Lei

Re: Race condition between driver_probe_device and device_shutdown‏

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1512 bytes --]

On Tue, 22 May 2012, Ming Lei wrote:

> On Tue, May 22, 2012 at 2:29 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Mon, 21 May 2012, Ming Lei wrote:
> >> Another candidate fix is to register a reboot notifier in driver core to prevent
> >> driver from being bound or unbound from start of reboot/shutdown, but looks
> >> not easy as the way of holding device locks.
> >
> > I'd guess it was done this way so that the shutdown task wouldn't have
> > to wait for a buggy driver that didn't want to release the device lock
> > (or that crashed while holding the lock).
> 
> Maybe, so I understand you agree on adding the device lock as did
> in the patch, don't I?

I don't know.  It depends on the intention behind the shutdown 
callback.  Maybe each driver is supposed to be responsible for doing 
its own locking.

Do you think that a buggy driver should be able to prevent the system 
from shutting down?

> Also we can add below line before acquiring device lock to help
> toubleshoot buggy driver.
> 
>                dev_dbg(dev, "acquiring device lock for shutdown\n");
> 
> >
> > It's not clear that the reboot notifier approach would work.  What
> > about probes that had already started when notifier was called?
> 
> Looks holding device lock is still needed in the notifier callback for
> handling the case if reboot notifier approach is taken.
> 
> IMO, the way of holding device lock is better than reboot notifier approach.

Yes, it is better.  The question is, is it right?

Alan Stern

Re: Race condition between driver_probe_device and device_shutdown‏

[Eric W. Biederman]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Tue, 22 May 2012, Ming Lei wrote:
>
>> On Tue, May 22, 2012 at 2:29 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Mon, 21 May 2012, Ming Lei wrote:
>> >> Another candidate fix is to register a reboot notifier in driver core to prevent
>> >> driver from being bound or unbound from start of reboot/shutdown, but looks
>> >> not easy as the way of holding device locks.

You might also be able to look at system_state and simply not do any
hotplug work if the state is neither SYSTEM_BOOTING or SYSTEM_RUNNING.

>> > I'd guess it was done this way so that the shutdown task wouldn't have
>> > to wait for a buggy driver that didn't want to release the device lock
>> > (or that crashed while holding the lock).
>> 
>> Maybe, so I understand you agree on adding the device lock as did
>> in the patch, don't I?
>
> I don't know.  It depends on the intention behind the shutdown 
> callback.  Maybe each driver is supposed to be responsible for doing 
> its own locking.
>
> Do you think that a buggy driver should be able to prevent the system 
> from shutting down?

The original intent of the shutdown callback was to just the hardware
part of the device shutdown and not do muck with kernel data structures
because just the device portion should be more reliable and was all
that is needed.

Given the current minimal usage of the device shutdown callback I'm not
convinced the original reasoning made sense but shrug.  So we might
want to reorg things and merge remove and shutdown.  I missed the start
of this thread so I don't know how ambitious everyone is.

Eric

Re: Race condition between driver_probe_device and device_shutdown‏

[Ming Lei]

On Wed, May 23, 2012 at 3:16 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Alan Stern <stern@rowland.harvard.edu> writes:
>
>> On Tue, 22 May 2012, Ming Lei wrote:
>>
>>> On Tue, May 22, 2012 at 2:29 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>> > On Mon, 21 May 2012, Ming Lei wrote:
>>> >> Another candidate fix is to register a reboot notifier in driver core to prevent
>>> >> driver from being bound or unbound from start of reboot/shutdown, but looks
>>> >> not easy as the way of holding device locks.
>
> You might also be able to look at system_state and simply not do any
> hotplug work if the state is neither SYSTEM_BOOTING or SYSTEM_RUNNING.

As Alan pointed, we need to handle the case of device probing or releasing
at the same time of starting shutdown, so maybe waiting for completing of
device probe or release is needed if device lock is to be avoided.

>
>>> > I'd guess it was done this way so that the shutdown task wouldn't have
>>> > to wait for a buggy driver that didn't want to release the device lock
>>> > (or that crashed while holding the lock).
>>>
>>> Maybe, so I understand you agree on adding the device lock as did
>>> in the patch, don't I?
>>
>> I don't know.  It depends on the intention behind the shutdown
>> callback.  Maybe each driver is supposed to be responsible for doing
>> its own locking.
>>
>> Do you think that a buggy driver should be able to prevent the system
>> from shutting down?

IMO, the buggy driver should be fixed first, not only in .probe or .release, but
also in .runtime_resume, all may affect shutting down.

>
> The original intent of the shutdown callback was to just the hardware
> part of the device shutdown and not do muck with kernel data structures
> because just the device portion should be more reliable and was all
> that is needed.

The .shutdown callback pointer is got from device->driver, which is
changed in probe and release path. Also runtime PM thing has been
involved into shutting down recently, so looks not only hardware parts
are involved now.

>
> Given the current minimal usage of the device shutdown callback I'm not
> convinced the original reasoning made sense but shrug.  So we might
> want to reorg things and merge remove and shutdown.  I missed the start
> of this thread so I don't know how ambitious everyone is.

Generally remove/release may take much more time than shutdown only, which
may slow down the 'power off'. Also some device may require special handling
in .shutdown, such as, network driver may need to enable WoL or change power
state in .shutdown, so it is not easy to merge remove with shutdown safely.


Thanks,
--
Ming Lei

Re: Race condition between driver_probe_device and device_shutdown‏

[Alan Stern]

On Wed, 23 May 2012, Ming Lei wrote:

> >> Do you think that a buggy driver should be able to prevent the system
> >> from shutting down?
> 
> IMO, the buggy driver should be fixed first, not only in .probe or .release, but
> also in .runtime_resume, all may affect shutting down.
> 
> >
> > The original intent of the shutdown callback was to just the hardware
> > part of the device shutdown and not do muck with kernel data structures
> > because just the device portion should be more reliable and was all
> > that is needed.
> 
> The .shutdown callback pointer is got from device->driver, which is
> changed in probe and release path. Also runtime PM thing has been
> involved into shutting down recently, so looks not only hardware parts
> are involved now.

This is a tricky question.  Overall I think you're probably right.  

It's certainly true that holding the device lock across the shutdown
callback is the easiest and most reliable way to prevent these races.

Alan Stern

Re: Race condition between driver_probe_device and device_shutdown‏

[Ming Lei]

On Wed, May 23, 2012 at 11:06 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 23 May 2012, Ming Lei wrote:
>> The .shutdown callback pointer is got from device->driver, which is
>> changed in probe and release path. Also runtime PM thing has been
>> involved into shutting down recently, so looks not only hardware parts
>> are involved now.
>
> This is a tricky question.  Overall I think you're probably right.
>
> It's certainly true that holding the device lock across the shutdown
> callback is the easiest and most reliable way to prevent these races.

But holding device lock across .shutdown is very inefficient because
most of devices' driver have not shutdown callback, so I think it is better
to fix the race by prevent driver core from probing or releasing once
shutdown is started.

How about the below patch?

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 346be8b..fc70930 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1803,6 +1803,18 @@ void device_shutdown(void)
 {
 	struct device *dev;

+	/*wait for completing of device probing and releasing*/
+	printk("%s: wait for completing of devices' probing and"
+			"releasing...");
+
+	/*
+	 * order between writing system_state and read probe/release
+	 * counter.
+	 */
+	smp_mb();
+	wait_for_device_probe_release();
+	printk("OK\n");
+
 	spin_lock(&devices_kset->list_lock);
 	/*
 	 * Walk the devices list backward, shutting down each in turn.
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1b1cbb5..8328383 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -245,6 +245,7 @@ int device_bind_driver(struct device *dev)
 EXPORT_SYMBOL_GPL(device_bind_driver);

 static atomic_t probe_count = ATOMIC_INIT(0);
+static atomic_t release_count = ATOMIC_INIT(0);
 static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);

 static int really_probe(struct device *dev, struct device_driver *drv)
@@ -252,6 +253,10 @@ static int really_probe(struct device *dev,
struct device_driver *drv)
 	int ret = 0;

 	atomic_inc(&probe_count);
+	smp_mb();
+	if (system_in_shutdown())
+		goto done;
+
 	pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
 		 drv->bus->name, __func__, drv->name, dev_name(dev));
 	WARN_ON(!list_empty(&dev->devres_head));
@@ -336,6 +341,19 @@ void wait_for_device_probe(void)
 EXPORT_SYMBOL_GPL(wait_for_device_probe);

 /**
+ * wait_for_device_probe_release
+ * Wait for device probing and releasing to be completed.
+ */
+void wait_for_device_probe_release(void)
+{
+	/* wait for complete devices' probing and releasing*/
+	wait_event(probe_waitqueue, atomic_read(&probe_count) == 0 &&
+			atomic_read(&release_count) == 0);
+	async_synchronize_full();
+}
+EXPORT_SYMBOL_GPL(wait_for_device_probe_release);
+
+/**
  * driver_probe_device - attempt to bind device & driver together
  * @drv: driver to bind a device to
  * @dev: device to try to bind to the driver
@@ -470,6 +488,11 @@ static void __device_release_driver(struct device *dev)

 	drv = dev->driver;
 	if (drv) {
+		atomic_inc(&release_count);
+		smp_mb();
+		if (system_in_shutdown())
+			goto exit;
+
 		pm_runtime_get_sync(dev);

 		driver_sysfs_remove(dev);
@@ -492,7 +515,9 @@ static void __device_release_driver(struct device *dev)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBOUND_DRIVER,
 						     dev);
-
+exit:
+		atomic_dec(&release_count);
+		wake_up(&probe_waitqueue);
 	}
 }

diff --git a/include/linux/device.h b/include/linux/device.h
index 161d962..3fdf782 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -243,6 +243,7 @@ extern struct device_driver *driver_find(const char *name,
 					 struct bus_type *bus);
 extern int driver_probe_done(void);
 extern void wait_for_device_probe(void);
+extern void wait_for_device_probe_release(void);


 /* sysfs interface for exporting driver attributes */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 64bdeeb8..3cf4f36 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -378,6 +378,12 @@ extern enum system_states {
 	SYSTEM_SUSPEND_DISK,
 } system_state;

+static inline int system_in_shutdown(void)
+{
+	return (system_state >= SYSTEM_HALT) &&
+		(system_state <= SYSTEM_RESTART);
+}
+
 #define TAINT_PROPRIETARY_MODULE	0
 #define TAINT_FORCED_MODULE		1
 #define TAINT_UNSAFE_SMP		2



Thanks,
--
Ming Lei

Re: Race condition between driver_probe_device and device_shutdown‏

[Greg KH]

On Thu, May 24, 2012 at 09:39:46AM +0800, Ming Lei wrote:
> On Wed, May 23, 2012 at 11:06 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 23 May 2012, Ming Lei wrote:
> >> The .shutdown callback pointer is got from device->driver, which is
> >> changed in probe and release path. Also runtime PM thing has been
> >> involved into shutting down recently, so looks not only hardware parts
> >> are involved now.
> >
> > This is a tricky question.  Overall I think you're probably right.
> >
> > It's certainly true that holding the device lock across the shutdown
> > callback is the easiest and most reliable way to prevent these races.
> 
> But holding device lock across .shutdown is very inefficient because
> most of devices' driver have not shutdown callback, so I think it is better
> to fix the race by prevent driver core from probing or releasing once
> shutdown is started.
> 
> How about the below patch?

How about waiting for the original poster to respond as to exactly how
they are hitting this race before doing anything?

greg k-h

Re: Race condition between driver_probe_device and device_shutdown‏

[Ming Lei]

On Thu, May 24, 2012 at 10:14 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> How about waiting for the original poster to respond as to exactly how
> they are hitting this race before doing anything?

Sure, :-)


Thanks,
--
Ming Lei

Re: Race condition between driver_probe_device and device_shutdown‏

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1564 bytes --]

On Wed, 23 May 2012, Greg KH wrote:

> On Thu, May 24, 2012 at 09:39:46AM +0800, Ming Lei wrote:
> > On Wed, May 23, 2012 at 11:06 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Wed, 23 May 2012, Ming Lei wrote:
> > >> The .shutdown callback pointer is got from device->driver, which is
> > >> changed in probe and release path. Also runtime PM thing has been
> > >> involved into shutting down recently, so looks not only hardware parts
> > >> are involved now.
> > >
> > > This is a tricky question.  Overall I think you're probably right.
> > >
> > > It's certainly true that holding the device lock across the shutdown
> > > callback is the easiest and most reliable way to prevent these races.
> > 
> > But holding device lock across .shutdown is very inefficient because
> > most of devices' driver have not shutdown callback, so I think it is better

The code there is racy already.  It does:

		} else if (dev->driver && dev->driver->shutdown) {

without any locking protection.  If the driver is unbound while this 
statement runs then dev->driver could be non-NULL for the first test 
and NULL for the second.

> > to fix the race by prevent driver core from probing or releasing once
> > shutdown is started.
> > 
> > How about the below patch?
> 
> How about waiting for the original poster to respond as to exactly how
> they are hitting this race before doing anything?

In addition, the patch is too complicated.  For this type of
synchronization you should use SRCU.  See
Documentation/RCU/whatisRCU.txt and related files.

Alan Stern

Re: Race condition between driver_probe_device and device_shutdown‏

[Ming Lei]

On Thu, May 24, 2012 at 10:37 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> The code there is racy already.  It does:
>
>                } else if (dev->driver && dev->driver->shutdown) {
>
> without any locking protection.  If the driver is unbound while this
> statement runs then dev->driver could be non-NULL for the first test
> and NULL for the second.

Yes,  I missed this one, :-)

>
>> > to fix the race by prevent driver core from probing or releasing once
>> > shutdown is started.
>> >
>> > How about the below patch?
>>
>> How about waiting for the original poster to respond as to exactly how
>> they are hitting this race before doing anything?
>
> In addition, the patch is too complicated.  For this type of
> synchronization you should use SRCU.  See
> Documentation/RCU/whatisRCU.txt and related files.

Yes, the synchronization should be a many reader vs. one
writer problem, RCU should be suitable.

Looks we think alike, :-)

I have studied RCU yesterday, but was afraid that may introduce
much more code, so not applied it in the patch. Will study it further
to figure out a new version.


Thanks,
--
Ming Lei

Re: Race condition between driver_probe_device and device_shutdown

[Wedson Almeida Filho]

[Sorry for taking so long to respond, after a week of silence I
assumed I wouldn't get any responses, plus I had moved on to other
things.]

I happen to still have the logs, the relevant part is pasted at the end.

Answering some of the questions: the driver is on the platform bus, in
fact, it's drivers/usb/host/ehci-tegra.c; after seeing the oops below,
I added printks when entering and exiting tegra_ehci_probe to try to
understand it better.

Note that in the log we see some thread entering tegra_ehci_probe, but
nothing indicates that it has exited before we get the oops.

As for how to reproduce this, I was running a "reboot stress" test; I
would boot the device and reboot it as soon as possible. It appears
that drivers were still being loaded when I managed to start the
reboot. Given that it's a race condition, it wouldn't always
reproduce, but it happened often enough that caught my attention.

With the patch I sent out in my first email I wouldn't run into this at all.

[   58.759906] tegra_ehci_probe instance 1
[   58.764958] [USBHHCD] : usb_create_hcd start
[   58.769342] [USBHHCD] : usb_bus_init start
[   58.772507] Entering device_shutdown
[   58.772516] [USBHv2] tegra_ehci_hcd_shutdown
[   58.772534] Unable to handle kernel paging request at virtual
address ffffffa8
[   58.772541] pgd = ef1e0000
[   58.772545] [ffffffa8] *pgd=af7fe021, *pte=00000000, *ppte=00000000
[   58.772557] Internal error: Oops: 17 [#1] PREEMPT SMP
[   58.772563] last sysfs file:
/sys/devices/platform/tegra-ehci.1/usb1/1-1/bbusb_ioctl
[   58.772588] CPU: 0    Tainted: G        WC
(2.6.36.3-02116-gb523cbe-dirty #3)
[   58.772610] PC is at tegra_ehci_hcd_shutdown+0x28/0x64
[   58.772617] LR is at tegra_ehci_hcd_shutdown+0x28/0x64
[   58.772624] pc : [<c04e3c04>]    lr : [<c04e3c04>]    psr: 60000013
[   58.772628] sp : e5ea3e50  ip : 00010135  fp : 00000001
[   58.772634] r10: 40819060  r9 : e5ea2000  r8 : c023c284
[   58.772639] r7 : 4081b090  r6 : c09326d0  r5 : fffffef8  r4 : 00000000
[   58.772645] r3 : 00000000  r2 : 00000001  r1 : 60000093  r0 : 00000036
[   58.772652] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
[   58.772659] Control: 10c5387d  Table: af1e004a  DAC: 00000015
[   58.772664]
[   58.772666] PC: 0xc04e3b84:
[   58.772669] 3b84  10800003 eaffffd3 e591c000 e59c6008 e1120006
15d1302b 10800003 e1140006
[   58.772680] 3ba4  15d1302d e2811008 10800003 eaffffc9 e2803f42
e92d4010 e5932004 e592000c
[   58.772691] 3bc4  f57ff05f e5931024 e1a001a0 ebfcc5b7 e1a00001
e8bd8010 e92d4070 e2800008
[   58.772702] 3be4  ebfe615a e3500000 08bd8070 e5904000 e59f1038
e59f0038 e2445f42 eb07d6b4
[   58.772713] 3c04  e5143058 e5933028 e3530000 08bd8070 e59f1018
e59f001c eb07d6ad e5143058
[   58.772723] 3c24  e1a00005 e1a0e00f e593f028 e8bd8070 c07098f8
c07ec784 c07ec794 e92d41f0
[   58.772734] 3c44  e59d4018 e1a05001 e1a06002 e1a07003 e5952000
f57ff05f e3720001 e0021006
[   58.772745] 3c64  e2444001 e3a00001 0a000006 e1510007 0a000006
ebf5c2ba e3540000 cafffff3
[   58.772757]
[   58.772758] LR: 0xc04e3b84:
[   58.772761] 3b84  10800003 eaffffd3 e591c000 e59c6008 e1120006
15d1302b 10800003 e1140006
[   58.772772] 3ba4  15d1302d e2811008 10800003 eaffffc9 e2803f42
e92d4010 e5932004 e592000c
[   58.772783] 3bc4  f57ff05f e5931024 e1a001a0 ebfcc5b7 e1a00001
e8bd8010 e92d4070 e2800008
[   58.772793] 3be4  ebfe615a e3500000 08bd8070 e5904000 e59f1038
e59f0038 e2445f42 eb07d6b4
[   58.772804] 3c04  e5143058 e5933028 e3530000 08bd8070 e59f1018
e59f001c eb07d6ad e5143058
[   58.772814] 3c24  e1a00005 e1a0e00f e593f028 e8bd8070 c07098f8
c07ec784 c07ec794 e92d41f0
[   58.772825] 3c44  e59d4018 e1a05001 e1a06002 e1a07003 e5952000
f57ff05f e3720001 e0021006
[   58.772836] 3c64  e2444001 e3a00001 0a000006 e1510007 0a000006
ebf5c2ba e3540000 cafffff3
[   58.772847]
[   58.772849] SP: 0xe5ea3dd0:
[   58.772852] 3dd0  58b8609a 2020205b 372e3835 31353237 00205d36
00000000 3b9aca00 10624dd3
[   58.772863] 3df0  60000013 ffffffff e5ea3e3c c09326d0 4081b090
c023bbac 00000036 60000093
[   58.772874] 3e10  00000001 00000000 00000000 fffffef8 c09326d0
4081b090 c023c284 e5ea2000
[   58.772884] 3e30  40819060 00000001 00010135 e5ea3e50 c04e3c04
c04e3c04 60000013 ffffffff
[   58.772895] 3e50  ef044a08 ef044a14 c09326d0 c047d500 ef044a08
c047908c c07b89b8 e5ea3e90
[   58.772905] 3e70  00000000 c02b27f4 e5ea3e90 e5ea3e90 00000000
c02b28a4 00000000 c02b2a80
[   58.772916] 3e90  e5ea2000 403adc40 00000000 403adc48 00000100
00000000 00000000 e5ea3fb0
[   58.772927] 3eb0  e5ea2000 eb1f0080 e5ea2000 c02ae37c e5ea3fb0
c023eeb0 00000011 c11cbf14
[   58.772938]
[   58.772940] R5: 0xfffffe78:
[   58.772943] fe78  ******** ******** ******** ******** ********
******** ******** ********
[   58.772957] fe98  ******** ******** ******** ******** ********
******** ******** ********
[   58.772968] feb8  ******** ******** ******** ******** ********
******** ******** ********
[   58.772979] fed8  ******** ******** ******** ******** ********
******** ******** ********
[   58.772990] fef8  ******** ******** ******** ******** ********
******** ******** ********
[   58.773000] ff18  ******** ******** ******** ******** ********
******** ******** ********
[   58.773011] ff38  ******** ******** ******** ******** ********
******** ******** ********
[   58.773022] ff58  ******** ******** ******** ******** ********
******** ******** ********
[   58.773033]
[   58.773035] R6: 0xc0932650:
[   58.773038] 2650  00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   58.773049] 2670  00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   58.773059] 2690  00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   58.773069] 26b0  00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   58.773079] 26d0  ef015420 ef096c60 ef0153e0 ef0153a0 ef015360
00000000 00000000 ef0151e0
[   58.773090] 26f0  00000000 00000000 ef015320 00000001 ef0152e0
00000000 ef0152a0 00000000
[   58.773100] 2710  00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   58.773110] 2730  00000000 00000000 00000000 00000000 ef3f5bc0
00000000 00000000 00000000
[   58.773121]
[   58.773123] R8: 0xc023c204:
[   58.773126] c204  e31c0c01 1a000008 e3570f5d e24fef46 3798f107
e28d1008 e3a08000 e357080f
[   58.773137] c224  e2270000 2a000f9a ea01fd5c e1a02007 e28d1008
e3a00000 eb000583 e28fe014
[   58.773147] c244  e1a07000 e28d1008 e3570f5d 3891000f 3798f107
eaffffef e5ad0008 e1a02007
[   58.773158] c264  e1a0100d e3a00001 eb000577 eaffffba e320f000
e320f000 e320f000 c088360c
[   58.773169] c284  c02ad098 c02a5208 c023c87c c031ebf8 c031e9c4
c031c940 c031c6f8 c02bb7a4
[   58.773179] c2a4  c031c958 c032a680 c0329fe8 c023c88c c031d2c4
c02bb7a4 c032a3dc c031d1b8
[   58.773191] c2c4  c02cf7ac c02bb7a4 c02bb7a4 c031db00 c02ac334
c03381b8 c02bb7a4 c02cf744
[   58.773201] c2e4  c02cf4e8 c02bb7a4 c02ab000 c02bb7a4 c02bb7a4
c02ad218 c02bb7a4 c02bb7a4
[   58.773213]
[   58.773215] R9: 0xe5ea1f80:
[   58.773217] 1f80  00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   58.773228] 1fa0  00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   58.773238] 1fc0  00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   58.773248] 1fe0  00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   58.773258] 2000  00000000 00000002 00000000 eb1f0080 c08a5268
00000000 00000015 eb1f0080
[   58.773269] 2020  c11bcb00 af1765a8 0000000d ee93da20 c08827e0
00000000 e5ea3bb4 e5ea3b18
[   58.773279] 2040  c06d9f30 00000000 00000000 00000000 00000000
00000000 01000000 00000000
[   58.773289] 2060  403adf00 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[   58.773303] Process adbd (pid: 662, stack limit = 0xe5ea22f0)
[   58.773310] Stack: (0xe5ea3e50 to 0xe5ea4000)
[   58.773317] 3e40:                                     ef044a08
ef044a14 c09326d0 c047d500
[   58.773326] 3e60: ef044a08 c047908c c07b89b8 e5ea3e90 00000000
c02b27f4 e5ea3e90 e5ea3e90
[   58.773335] 3e80: 00000000 c02b28a4 00000000 c02b2a80 e5ea2000
403adc40 00000000 403adc48
[   58.773344] 3ea0: 00000100 00000000 00000000 e5ea3fb0 e5ea2000
eb1f0080 e5ea2000 c02ae37c
[   58.773354] 3ec0: e5ea3fb0 c023eeb0 00000011 c11cbf14 c08a8100
60000013 00000000 00000011
[   58.773363] 3ee0: 00000011 00000000 00040001 0000029b 00000000
00000000 00000000 00000001
[   58.773372] 3f00: 00000001 00000000 00000001 ee2064e0 e5ea3f60
ee856bd4 e5ea3f78 c02baa78
[   58.773381] 3f20: ee01db78 e5ea3f60 ee01dda8 c02a4890 00000001
e5ea3f78 00000000 00000005
[   58.773390] 3f40: 403adb4c 00000000 00000003 00000000 e5ea2000
00000000 e5ea2000 00000000
[   58.773399] 3f60: e5ea2000 e5ea21b0 00000100 00000000 403adb58
403add58 00000000 00000000
[   58.773408] 3f80: e5ea3fb0 e5ea2000 0000001b 00000077 00000000
000374d0 4081b090 0000001b
[   58.773417] 3fa0: 00000058 c023c100 000374d0 4081b090 fee1dead
28121969 a1b2c3d4 4081b090
[   58.773426] 3fc0: 000374d0 4081b090 0000001b 00000058 0000ee29
00100000 40819060 00000001
[   58.773436] 3fe0: 403adffc 403ade48 0000f1f7 000084fc 20000010
fee1dead ffffffff ffffffff
[   58.773468] [<c04e3c04>] (tegra_ehci_hcd_shutdown+0x28/0x64) from
[<c047d500>] (platform_drv_shutdown+0x18/0x1c)
[   58.773485] [<c047d500>] (platform_drv_shutdown+0x18/0x1c) from
[<c047908c>] (device_shutdown+0x78/0xd0)
[   58.773503] [<c047908c>] (device_shutdown+0x78/0xd0) from
[<c02b27f4>] (kernel_restart_prepare+0x58/0x70)
[   58.773516] [<c02b27f4>] (kernel_restart_prepare+0x58/0x70) from
[<c02b28a4>] (kernel_restart+0x2c/0x7c)
[   58.773527] [<c02b28a4>] (kernel_restart+0x2c/0x7c) from
[<c02b2a80>] (sys_reboot+0x184/0x1cc)
[   58.773540] [<c02b2a80>] (sys_reboot+0x184/0x1cc) from [<c023c100>]
(ret_fast_syscall+0x0/0x30)
[   58.773551] Code: e59f1038 e59f0038 e2445f42 eb07d6b4 (e5143058)
[   58.773559] ---[ end trace 1b75b31a2719ed32 ]---

On Thu, May 24, 2012 at 5:33 PM, Ming Lei <ming.lei@canonical.com> wrote:
> On Thu, May 24, 2012 at 10:37 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>
>> The code there is racy already.  It does:
>>
>>                } else if (dev->driver && dev->driver->shutdown) {
>>
>> without any locking protection.  If the driver is unbound while this
>> statement runs then dev->driver could be non-NULL for the first test
>> and NULL for the second.
>
> Yes,  I missed this one, :-)
>
>>
>>> > to fix the race by prevent driver core from probing or releasing once
>>> > shutdown is started.
>>> >
>>> > How about the below patch?
>>>
>>> How about waiting for the original poster to respond as to exactly how
>>> they are hitting this race before doing anything?
>>
>> In addition, the patch is too complicated.  For this type of
>> synchronization you should use SRCU.  See
>> Documentation/RCU/whatisRCU.txt and related files.
>
> Yes, the synchronization should be a many reader vs. one
> writer problem, RCU should be suitable.
>
> Looks we think alike, :-)
>
> I have studied RCU yesterday, but was afraid that may introduce
> much more code, so not applied it in the patch. Will study it further
> to figure out a new version.
>
>
> Thanks,
> --
> Ming Lei

Re: Race condition between driver_probe_device and device_shutdown

[Ming Lei]

On Thu, Dec 6, 2012 at 5:11 PM, Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
> [Sorry for taking so long to respond, after a week of silence I
> assumed I wouldn't get any responses, plus I had moved on to other
> things.]
>
> I happen to still have the logs, the relevant part is pasted at the end.
>
> Answering some of the questions: the driver is on the platform bus, in
> fact, it's drivers/usb/host/ehci-tegra.c; after seeing the oops below,
> I added printks when entering and exiting tegra_ehci_probe to try to
> understand it better.
>
> Note that in the log we see some thread entering tegra_ehci_probe, but
> nothing indicates that it has exited before we get the oops.

The commit d1c6c030fcec6f860d9bb6c632a3ebe62e28440b(driver core:
fix shutdown races with probe/remove(v3)) has been merged to v3.6, so
device_shutdown will wait for completing of probe.

Could you test kernel 3.6 or the latest kernel to see if the problem can be
reproduced?

Thanks,
--
Ming Lei

Re: Race condition between driver_probe_device and device_shutdown

[Wedson Almeida Filho]

I don't have the setup anymore, I'll give it a shot if I ever get back it back.

I have a question though: why do we need to get a reference to the
parent? If the assumption is that the callee can release its reference
to the parent, then one may also expect callees to reassign
dev->parent (e.g., by calling device_move), so it wouldn't be safe to
later call device_unlock on the potentially different dev->parent.

If we expect dev->parent to remain unchanged, then there's no need to
get an extra reference to the parent as the device itself already
holds one (and we hold an extra one on the device).

What am I missing?

Thanks

On Thu, Dec 6, 2012 at 2:52 AM, Ming Lei <ming.lei@canonical.com> wrote:
> On Thu, Dec 6, 2012 at 5:11 PM, Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>> [Sorry for taking so long to respond, after a week of silence I
>> assumed I wouldn't get any responses, plus I had moved on to other
>> things.]
>>
>> I happen to still have the logs, the relevant part is pasted at the end.
>>
>> Answering some of the questions: the driver is on the platform bus, in
>> fact, it's drivers/usb/host/ehci-tegra.c; after seeing the oops below,
>> I added printks when entering and exiting tegra_ehci_probe to try to
>> understand it better.
>>
>> Note that in the log we see some thread entering tegra_ehci_probe, but
>> nothing indicates that it has exited before we get the oops.
>
> The commit d1c6c030fcec6f860d9bb6c632a3ebe62e28440b(driver core:
> fix shutdown races with probe/remove(v3)) has been merged to v3.6, so
> device_shutdown will wait for completing of probe.
>
> Could you test kernel 3.6 or the latest kernel to see if the problem can be
> reproduced?
>
> Thanks,
> --
> Ming Lei

Re: Race condition between driver_probe_device and device_shutdown

[Ming Lei]

On Fri, Dec 7, 2012 at 8:09 AM, Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
> I don't have the setup anymore, I'll give it a shot if I ever get back it back.
>
> I have a question though: why do we need to get a reference to the
> parent? If the assumption is that the callee can release its reference

Because device_del() will put reference count of the parent, and the patch
only focuses on race between probe/release and shutdown.

> to the parent, then one may also expect callees to reassign
> dev->parent (e.g., by calling device_move), so it wouldn't be safe to
> later call device_unlock on the potentially different dev->parent.

As far as device_move() concerned, looks it might be a problem.
The problem even exits on driver attach vs. device open/release,
if device_move is called in open() and open() happens before driver
attach completes.

> If we expect dev->parent to remain unchanged, then there's no need to
> get an extra reference to the parent as the device itself already
> holds one (and we hold an extra one on the device).

As I said above, device_del() will put reference count of parent, but I forget
why the parent's lock has to be held in this patch.

> What am I missing?

Your concern on device_remove() might be correct. Also, I am wondering
if we can walk the 'dpm_list' backwards for device shutdown, which should
be simpler and more reasonable.


Thanks,
--
Ming Lei

Re: Race condition between driver_probe_device and device_shutdown

[Wedson Almeida Filho]

> Because device_del() will put reference count of the parent, and the patch
> only focuses on race between probe/release and shutdown.

Right. device_del() puts the reference count of the parent -- is it
guaranteed that device_del() won't ever reassign dev->parent though
(e.g., to NULL)? I don't think it is, so I think that patch should
also save the pointer to the parent and use it (instead of what
happens to be in than dev->parent) to release the lock and put the
ref.

> As far as device_move() concerned, looks it might be a problem.
> The problem even exits on driver attach vs. device open/release,
> if device_move is called in open() and open() happens before driver
> attach completes.

Yeah, the pattern of locking the parent followed by the device occurs
in a few places. It looks like they were added by Alan with commit
bf74ad5bc41727d5f2f1c6bedb2c1fac394de731. (And as Greg mentioned,
might be occurring often enough to merit being moved into a common
function.)

I guess the question is whether the callee is allowed to call
device_move(), if not, we're good.

> Your concern on device_remove() might be correct. Also, I am wondering
> if we can walk the 'dpm_list' backwards for device shutdown, which should
> be simpler and more reasonable.

How would that help?

Re: Race condition between driver_probe_device and device_shutdown

[Ming Lei]

On Fri, Dec 7, 2012 at 8:16 PM, Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>> Because device_del() will put reference count of the parent, and the patch
>> only focuses on race between probe/release and shutdown.
>
> Right. device_del() puts the reference count of the parent -- is it
> guaranteed that device_del() won't ever reassign dev->parent though
> (e.g., to NULL)? I don't think it is, so I think that patch should
> also save the pointer to the parent and use it (instead of what
> happens to be in than dev->parent) to release the lock and put the
> ref.
>
>> As far as device_move() concerned, looks it might be a problem.
>> The problem even exits on driver attach vs. device open/release,
>> if device_move is called in open() and open() happens before driver
>> attach completes.
>
> Yeah, the pattern of locking the parent followed by the device occurs
> in a few places. It looks like they were added by Alan with commit
> bf74ad5bc41727d5f2f1c6bedb2c1fac394de731. (And as Greg mentioned,
> might be occurring often enough to merit being moved into a common
> function.)

In fact, there is no shutdown callback defined for usb driver, so holding
the parent lock in device_shutdown() might be removed.

>
> I guess the question is whether the callee is allowed to call
> device_move(), if not, we're good.

Not only the callee, and other contexts can change device->parent
too. Looks rfcomm_tty_open() which calls device_move() is called
in open() context, so the parent might be changed before unlock(dev->parent)
in __driver_attach().

>
>> Your concern on device_remove() might be correct. Also, I am wondering
>> if we can walk the 'dpm_list' backwards for device shutdown, which should
>> be simpler and more reasonable.
>
> How would that help?

device_pm_lock() can prevent device_move() from being running.

Thanks,
--
Ming Lei

Re: Race condition between driver_probe_device and device_shutdown

[Alan Stern]

On Fri, 7 Dec 2012, Ming Lei wrote:

> > I guess the question is whether the callee is allowed to call
> > device_move(), if not, we're good.
> 
> Not only the callee, and other contexts can change device->parent
> too. Looks rfcomm_tty_open() which calls device_move() is called
> in open() context, so the parent might be changed before unlock(dev->parent)
> in __driver_attach().
> 
> >
> >> Your concern on device_remove() might be correct. Also, I am wondering
> >> if we can walk the 'dpm_list' backwards for device shutdown, which should
> >> be simpler and more reasonable.
> >
> > How would that help?
> 
> device_pm_lock() can prevent device_move() from being running.

That wouldn't prevent problems during unbinding.  Wedson is right; the 
places that lock dev->parent must save a local copy of dev->parent.

Alan Stern

Re: Race condition between driver_probe_device and device_shutdown

[Ming Lei]

On Fri, Dec 7, 2012 at 11:25 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 7 Dec 2012, Ming Lei wrote:
>> device_pm_lock() can prevent device_move() from being running.
>
> That wouldn't prevent problems during unbinding.  Wedson is right; the

Right.

> places that lock dev->parent must save a local copy of dev->parent.

One simple fix is to remove acquiring/releasing dev->parent lock
and get/put the parent reference, because holding the device lock only
may avoid the race between probe/remove and shutdown.

But __driver_attach and __device_release_driver need
save the local copy of dev->parent.

Thanks,
--
Ming Lei