[PATCH] driver core: fix shutdown races with probe/remove(v1)

[PATCH] driver core: fix shutdown races with probe/remove(v1)

[Ming Lei]

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 <stern@rowland.harvard.edu>
Cc: stable@vger.kernel.org
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 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

Re: [PATCH] driver core: fix shutdown races with probe/remove(v1)

[Sergei Shtylyov]

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 <stern@rowland.harvard.edu>
> Cc: stable@vger.kernel.org
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>   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

Re: [PATCH] driver core: fix shutdown races with probe/remove(v1)

[Alan Stern]

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 <stern@rowland.harvard.edu>
> Cc: stable@vger.kernel.org
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  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

Re: [PATCH] driver core: fix shutdown races with probe/remove(v1)

[Ming Lei]

On Fri, Jun 8, 2012 at 9:40 PM, Alan Stern <stern@rowland.harvard.edu> 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