linux-kernel - Re: [PATCH v1] PM: sleep: core: Restrict power.set

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: <CAPDyKFqEK3jBQxmuGTRHGHgyNUY+veE+iiujgcJpyOuLjw0vBg@mail.gmail.com>
Date: Mon, 10 Feb 2025 12:31:34 +0100
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>, Bjorn Helgaas <helgaas@...nel.org>, 
	Linux PCI <linux-pci@...r.kernel.org>, Johan Hovold <johan@...nel.org>, 
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, Kevin Xie <kevin.xie@...rfivetech.com>, 
	Jon Hunter <jonathanh@...dia.com>
Subject: Re: [PATCH v1] PM: sleep: core: Restrict power.set_active propagation

On Sat, 8 Feb 2025 at 18:54, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> Commit 3775fc538f53 ("PM: sleep: core: Synchronize runtime PM status of
> parents and children") exposed an issue related to simple_pm_bus_pm_ops
> that uses pm_runtime_force_suspend() and pm_runtime_force_resume() as
> bus type PM callbacks for the noirq phases of system-wide suspend and
> resume.
>
> The problem is that pm_runtime_force_suspend() does not distinguish
> runtime-suspended devices from devices for which runtime PM has never
> been enabled, so if it sees a device with runtime PM status set to
> RPM_ACTIVE, it will assume that runtime PM is enabled for that device
> and so it will attempt to suspend it with the help of its runtime PM
> callbacks which may not be ready for that.  As it turns out, this
> causes simple_pm_bus_runtime_suspend() to crash due to a NULL pointer
> dereference.
>
> Another problem related to the above commit and simple_pm_bus_pm_ops is
> that setting runtime PM status of a device handled by the latter to
> RPM_ACTIVE will actually prevent it from being resumed because
> pm_runtime_force_resume() only resumes devices with runtime PM status
> set to RPM_SUSPENDED.
>
> To mitigate these issues, do not allow power.set_active to propagate
> beyond the parent of the device with DPM_FLAG_SMART_SUSPEND set that
> will need to be resumed, which should be a sufficient stop-gap for the
> time being, but they will need to be properly addressed in the future
> because in general during system-wide resume it is necessary to resume
> all devices in a dependency chain in which at least one device is going
> to be resumed.
>
> Fixes: 3775fc538f53 ("PM: sleep: core: Synchronize runtime PM status of parents and children")
> Closes: https://lore.kernel.org/linux-pm/1c2433d4-7e0f-4395-b841-b8eac7c25651@nvidia.com/
> Reported-by: Jon Hunter <jonathanh@...dia.com>
> Tested-by: Johan Hovold <johan+linaro@...nel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>

Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>

Kind regards
Uffe

> ---
>  drivers/base/power/main.c |   21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1191,24 +1191,18 @@
>         return PMSG_ON;
>  }
>
> -static void dpm_superior_set_must_resume(struct device *dev, bool set_active)
> +static void dpm_superior_set_must_resume(struct device *dev)
>  {
>         struct device_link *link;
>         int idx;
>
> -       if (dev->parent) {
> +       if (dev->parent)
>                 dev->parent->power.must_resume = true;
> -               if (set_active)
> -                       dev->parent->power.set_active = true;
> -       }
>
>         idx = device_links_read_lock();
>
> -       list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) {
> +       list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node)
>                 link->supplier->power.must_resume = true;
> -               if (set_active)
> -                       link->supplier->power.set_active = true;
> -       }
>
>         device_links_read_unlock(idx);
>  }
> @@ -1287,9 +1281,12 @@
>                 dev->power.must_resume = true;
>
>         if (dev->power.must_resume) {
> -               dev->power.set_active = dev->power.set_active ||
> -                       dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND);
> -               dpm_superior_set_must_resume(dev, dev->power.set_active);
> +               if (dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND)) {
> +                       dev->power.set_active = true;
> +                       if (dev->parent && !dev->parent->power.ignore_children)
> +                               dev->parent->power.set_active = true;
> +               }
> +               dpm_superior_set_must_resume(dev);
>         }
>
>  Complete:
>
>
>