Firstly, .shutdown callback may touch a uninitialized hardware
if dev->driver is set and .probe is not completed.
Secondly, device_shutdown() may dereference a null pointer to cause
oops when dev->driver is cleared after it is checked in
device_shutdown().
So just hold device lock and its parent lock if it has to fix the
races.
Cc: Alan Stern <[email protected]>
Cc: [email protected]
Signed-off-by: Ming Lei <[email protected]>
---
drivers/base/core.c | 8 ++++++++
1 file changed, 8 insertions(+)
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);
--
1.7.9.5
Hello.
On 08-06-2012 9:38, Ming Lei wrote:
> Firstly, .shutdown callback may touch a uninitialized hardware
> if dev->driver is set and .probe is not completed.
> Secondly, device_shutdown() may dereference a null pointer to cause
> oops when dev->driver is cleared after it is checked in
> device_shutdown().
> So just hold device lock and its parent lock if it has to fix the
> races.
> Cc: Alan Stern <[email protected]>
> Cc: [email protected]
> Signed-off-by: Ming Lei <[email protected]>
> ---
> drivers/base/core.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
> 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*/
Please add spaces after /* and before */. Or the applier please do it...
WBR, Sergei
On Fri, 8 Jun 2012, Ming Lei wrote:
> Firstly, .shutdown callback may touch a uninitialized hardware
> if dev->driver is set and .probe is not completed.
>
> Secondly, device_shutdown() may dereference a null pointer to cause
> oops when dev->driver is cleared after it is checked in
> device_shutdown().
>
> So just hold device lock and its parent lock if it has to fix the
> races.
>
> Cc: Alan Stern <[email protected]>
> Cc: [email protected]
> Signed-off-by: Ming Lei <[email protected]>
> ---
> drivers/base/core.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> 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);
Would you prefer to use device_trylock in a loop? I guess this comes
down to which you prefer: a hang during shutdown, or a crash. :-)
Alan Stern
On Fri, Jun 8, 2012 at 9:40 PM, Alan Stern <[email protected]> wrote:
>
> Would you prefer to use device_trylock in a loop? ?I guess this comes
Yes, looks trylock in loop is safer than locking simply, and another
advantage is that the buggy device or driver can be logged.
So will do the v2 using trylock.
> down to which you prefer: a hang during shutdown, or a crash. ?:-)
Considered that the device or driver can be logged, either hang or
crash will be fixed later by someone, :-)
Thanks,
--
Ming Lei