linux-kernel - Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fgl4w5uhxci7rrbdigtni72vveb2gqemh6iccz4qruqkek5rja@rzwkcjg6hkid>
Date: Wed, 21 May 2025 07:57:58 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Claudiu Beznea <claudiu.beznea@...on.dev>, 
	Jonathan Cameron <jic23@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, dakr@...nel.org, linux-kernel@...r.kernel.org, 
	linux-pm@...r.kernel.org, linux-renesas-soc@...r.kernel.org, geert@...ux-m68k.org, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	linux-iio@...r.kernel.org, bhelgaas@...gle.com
Subject: Re: [PATCH] driver core: platform: Use devres group to free driver
 probe resources

On Wed, May 21, 2025 at 02:37:08PM +0200, Ulf Hansson wrote:
> On Wed, 21 May 2025 at 07:41, Claudiu Beznea <claudiu.beznea@...on.dev> wrote:
> >
> > Hi, Ulf,
> >
> > On 20.05.2025 15:09, Ulf Hansson wrote:
> > > For example, even if the order is made correctly, suppose a driver's
> > > ->remove() callback completes by turning off the resources for its
> > > device and leaves runtime PM enabled, as it relies on devres to do it
> > > some point later. Beyond this point, nothing would prevent userspace
> > > for runtime resuming/suspending the device via sysfs.
> >
> > If I'm not wrong, that can't happen? The driver_sysfs_remove() is called
> > before device_remove() (which calls the driver remove) is called, this
> > being the call path:
> >
> > device_driver_detach() ->
> >   device_release_driver_internal() ->
> >     __device_release_driver() ->
> >       driver_sysfs_remove()
> >       // ...
> >       device_remove()
> >
> > And the driver_sysfs_remove() calls in the end __kernfs_remove() which
> > looks to me like the place that actually drops the entries from sysfs, this
> > being a call path for it:
> >
> > driver_sysfs_remove() ->
> >   sysfs_remove_link() ->
> >     kernfs_remove_by_name() ->
> >       kernfs_remove_by_name_ns() ->
> >         __kernfs_remove() ->
> >
> > activating the following line in __kernfs_remove():
> >
> > pr_debug("kernfs %s: removing\n", kernfs_rcu_name(kn));
> >
> > leads to the following prints when unbinding the watchdog device from its
> > watchdog driver (attached to platform bus) on my board:
> > https://p.fr33tux.org/935252
> 
> Indeed this is a very good point you make! I completely overlooked
> this fact, thanks a lot for clarifying this!
> 
> However, my main point still stands.
> 
> In the end, there is nothing preventing rpm_suspend|resume|idle() in
> drivers/base/power/runtime.c from running (don't forget runtime PM is
> asynchronous too) for the device in question. This could lead to that
> a ->runtime_suspend|resume|idle() callback becomes executed at any
> point in time, as long as we haven't called pm_runtime_disable() for
> the device.

So exactly the same may happen if you enter driver->remove() and
something calls runtime API before pm_runtime_disable() is called.
The driver has (as they should be doing currently) be prepared for this.

> 
> That's why the devm_pm_runtime_enable() should be avoided as it simply
> introduces a race-condition. Drivers need to be more careful and use
> pm_runtime_enable|disable() explicitly to control the behaviour.

You make it sound like we are dealing with some non-deterministic
process, like garbage collector, where runtime disable done by devm
happens at some unspecified point in the future. However we are dealing
with very well defined order of operations, all happening within
__device_release_driver() call. It is the same scope as when using
manual pm_runtime_disable(). Just the order is wrong, that is it.

Thanks.

-- 
Dmitry