linux-kernel - Re: Race condition between driver_probe_device and device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 17 May 2012 21:59:07 -0700
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Wedson Almeida Filho <wedsonaf@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: Race condition between driver_probe_device and device_shutdown‏

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/