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
| ||
|
Date: Mon, 11 Apr 2022 12:35:35 +0200 From: Ulf Hansson <ulf.hansson@...aro.org> To: "Rafael J. Wysocki" <rafael@...nel.org> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, Alan Stern <stern@...land.harvard.edu> Subject: Re: [PATCH v1] PM: runtime: Avoid device usage count underflows On Fri, 8 Apr 2022 at 19:05, Rafael J. Wysocki <rafael@...nel.org> wrote: > > On Fri, Apr 8, 2022 at 4:05 PM Ulf Hansson <ulf.hansson@...aro.org> wrote: > > > > On Wed, 6 Apr 2022 at 21:03, Rafael J. Wysocki <rjw@...ysocki.net> wrote: > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com> > > > > > > A PM-runtime device usage count underflow is potentially critical, > > > because it may cause a device to be suspended when it is expected to > > > be operational. > > > > I get the point. Although, perhaps we should also state that it's a > > programming problem that we would like to catch and warn about? > > OK, I can add that to the changelog. > > > > > > > For this reason, (1) make rpm_check_suspend_allowed() return an error > > > when the device usage count is negative to prevent devices from being > > > suspended in that case, (2) introduce rpm_drop_usage_count() that will > > > detect device usage count underflows, warn about them and fix them up, > > > and (3) use it to drop the usage count in a few places instead of > > > atomic_dec_and_test(). > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com> > > > --- > > > drivers/base/power/runtime.c | 44 ++++++++++++++++++++++++++++++++++++------- > > > 1 file changed, 37 insertions(+), 7 deletions(-) > > > > > > Index: linux-pm/drivers/base/power/runtime.c > > > =================================================================== > > > --- linux-pm.orig/drivers/base/power/runtime.c > > > +++ linux-pm/drivers/base/power/runtime.c > > > @@ -263,7 +263,7 @@ static int rpm_check_suspend_allowed(str > > > retval = -EINVAL; > > > else if (dev->power.disable_depth > 0) > > > retval = -EACCES; > > > - else if (atomic_read(&dev->power.usage_count) > 0) > > > + else if (atomic_read(&dev->power.usage_count)) > > > retval = -EAGAIN; > > > else if (!dev->power.ignore_children && > > > atomic_read(&dev->power.child_count)) > > > @@ -1039,13 +1039,33 @@ int pm_schedule_suspend(struct device *d > > > } > > > EXPORT_SYMBOL_GPL(pm_schedule_suspend); > > > > > > +static int rpm_drop_usage_count(struct device *dev) > > > +{ > > > + int ret; > > > + > > > + ret = atomic_sub_return(1, &dev->power.usage_count); > > > + if (ret >= 0) > > > + return ret; > > > + > > > + /* > > > + * Because rpm_resume() does not check the usage counter, it will resume > > > + * the device even if the usage counter is 0 or negative, so it is > > > + * sufficient to increment the usage counter here to reverse the change > > > + * made above. > > > + */ > > > + atomic_inc(&dev->power.usage_count); > > > > Rather than this two-step process, couldn't we just do an > > "atomic_add_unless(&dev->power.usage_count, -1, 0)" - and check the > > return value? > > No, we couldn't, because atomic_add_unless() returns a bool and we > need to know the new counter value (and in particular whether or not > it is 0). atomic_add_unless(&dev->power.usage_count, -1, 0) would return true as long as the counter value is greater than 0. If the counter has become 0, atomic_add_unless() would return false and not continue to decrease the value below zero. Isn't this exactly what we want? > > I thought that it would be better to do the extra access in the > failing case only. > > > > + dev_warn(dev, "Runtime PM usage count underflow!\n"); > > > + return -EINVAL; > > > +} > > > + > > > > [...] Kind regards Uffe
Powered by blists - more mailing lists