linux-kernel - Re: [PATCH v1] PM: runtime: Avoid device usage count underflows

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: <CAPDyKFoop_mtWV0i+fH8GSmaxbPCyCPmOeNi6GuK5Qrj7ZVo1g@mail.gmail.com>
Date:   Fri, 8 Apr 2022 16:04:41 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:     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 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?

>
> 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?

> +       dev_warn(dev, "Runtime PM usage count underflow!\n");
> +       return -EINVAL;
> +}
> +

[...]

Kind regards
Uffe