[PATCH] PM: sleep: wakeirq: harden dev_pm_clear_wake_irq() against races

[PATCH] PM: sleep: wakeirq: harden dev_pm_clear_wake_irq() against races

[Gui-Dong Han]

dev_pm_clear_wake_irq() currently uses a dangerous pattern where
dev->power.wakeirq is read and checked for NULL outside the lock. If two
callers invoke this function concurrently, both might see a valid
pointer and proceed. This could result in a double-free when the second
caller acquires the lock and tries to release the same object.

Address this by using double-checked locking. This retains the
performance benefit of avoiding the lock when dev->power.wakeirq is
NULL, consistent with the original logic, but adds a necessary re-check
after acquiring dev->power.lock. Additionally, use READ_ONCE() and
WRITE_ONCE() to annotate the shared variable accesses to avoid data races
as defined by the kernel documentation.

Based on a quick scan of current users, I did not find an actual bug as
drivers seem to rely on their own synchronization. However, since
asynchronous usage patterns exist (e.g., in
drivers/net/wireless/ti/wlcore), I believe a race is theoretically
possible if the API is used less carefully in the future. This change
hardens the API to be robust against such cases.

Fixes: 4990d4fe327b ("PM / Wakeirq: Add automated device wake IRQ handling")
Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
---
While studying wakeirq, I noticed dev_pm_clear_wake_irq() handles
sequential re-entry (via the NULL check) but may lead to a double-free
on concurrent calls.

I considered whether we should simply document that concurrent calls are
forbidden. However, since the double-check locking pattern is
straightforward and adds negligible performance overhead (we still skip
the lock in the NULL case), I believe hardening the API is the better
approach.

I also noticed comments for dev_pm_enable_wake_irq_check() and friends
appear outdated. They claim "Caller must hold &dev->power.lock" and
limit usage to rpm_suspend/resume, yet pm_runtime_force_suspend/resume()
now call them lockless. While this usage appears safe due to the specific
context, it conflicts with the comments.

I can submit a follow-up patch to fix this doc drift but am unsure
whether to simply remove the restriction text or complicate it with
exceptions. Guidance is welcome.
---
 drivers/base/power/wakeirq.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
index 8aa28c08b289..acb520626195 100644
--- a/drivers/base/power/wakeirq.c
+++ b/drivers/base/power/wakeirq.c
@@ -30,7 +30,7 @@ static int dev_pm_attach_wake_irq(struct device *dev, struct wake_irq *wirq)
 		return -EEXIST;
 	}
 
-	dev->power.wakeirq = wirq;
+	WRITE_ONCE(dev->power.wakeirq, wirq);
 	device_wakeup_attach_irq(dev, wirq);
 
 	spin_unlock_irqrestore(&dev->power.lock, flags);
@@ -83,15 +83,21 @@ EXPORT_SYMBOL_GPL(dev_pm_set_wake_irq);
  */
 void dev_pm_clear_wake_irq(struct device *dev)
 {
-	struct wake_irq *wirq = dev->power.wakeirq;
+	struct wake_irq *wirq = READ_ONCE(dev->power.wakeirq);
 	unsigned long flags;
 
 	if (!wirq)
 		return;
 
 	spin_lock_irqsave(&dev->power.lock, flags);
+	wirq = dev->power.wakeirq;
+	if (!wirq) {
+		spin_unlock_irqrestore(&dev->power.lock, flags);
+		return;
+	}
+
 	device_wakeup_detach_irq(dev);
-	dev->power.wakeirq = NULL;
+	WRITE_ONCE(dev->power.wakeirq, NULL);
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 
 	if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED) {
-- 
2.43.0

Re: [PATCH] PM: sleep: wakeirq: harden dev_pm_clear_wake_irq() against races

[Markus Elfring]

…
> Address this by using double-checked locking. This retains the
> performance benefit of avoiding the lock when dev->power.wakeirq is
> NULL, consistent with the original logic, but adds a necessary re-check
> after acquiring dev->power.lock. Additionally, use READ_ONCE() and
> WRITE_ONCE() to annotate the shared variable accesses to avoid data races
> as defined by the kernel documentation.
…

How do you think about to apply lock guards in the affected function implementations?

Regards,
Markus

Re: [PATCH] PM: sleep: wakeirq: harden dev_pm_clear_wake_irq() against races

[Rafael J. Wysocki]

On Sat, Jan 31, 2026 at 11:13 AM Gui-Dong Han <hanguidong02@gmail.com> wrote:
>
> dev_pm_clear_wake_irq() currently uses a dangerous pattern where
> dev->power.wakeirq is read and checked for NULL outside the lock. If two
> callers invoke this function concurrently, both might see a valid
> pointer and proceed. This could result in a double-free when the second
> caller acquires the lock and tries to release the same object.
>
> Address this by using double-checked locking. This retains the
> performance benefit of avoiding the lock when dev->power.wakeirq is
> NULL, consistent with the original logic, but adds a necessary re-check
> after acquiring dev->power.lock. Additionally, use READ_ONCE() and
> WRITE_ONCE() to annotate the shared variable accesses to avoid data races
> as defined by the kernel documentation.
>
> Based on a quick scan of current users, I did not find an actual bug as
> drivers seem to rely on their own synchronization. However, since
> asynchronous usage patterns exist (e.g., in
> drivers/net/wireless/ti/wlcore), I believe a race is theoretically
> possible if the API is used less carefully in the future. This change
> hardens the API to be robust against such cases.
>
> Fixes: 4990d4fe327b ("PM / Wakeirq: Add automated device wake IRQ handling")
> Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
> ---
> While studying wakeirq, I noticed dev_pm_clear_wake_irq() handles
> sequential re-entry (via the NULL check) but may lead to a double-free
> on concurrent calls.
>
> I considered whether we should simply document that concurrent calls are
> forbidden. However, since the double-check locking pattern is
> straightforward and adds negligible performance overhead (we still skip
> the lock in the NULL case), I believe hardening the API is the better
> approach.
>
> I also noticed comments for dev_pm_enable_wake_irq_check() and friends
> appear outdated. They claim "Caller must hold &dev->power.lock" and
> limit usage to rpm_suspend/resume, yet pm_runtime_force_suspend/resume()
> now call them lockless. While this usage appears safe due to the specific
> context, it conflicts with the comments.
>
> I can submit a follow-up patch to fix this doc drift but am unsure
> whether to simply remove the restriction text or complicate it with
> exceptions. Guidance is welcome.
> ---
>  drivers/base/power/wakeirq.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
> index 8aa28c08b289..acb520626195 100644
> --- a/drivers/base/power/wakeirq.c
> +++ b/drivers/base/power/wakeirq.c
> @@ -30,7 +30,7 @@ static int dev_pm_attach_wake_irq(struct device *dev, struct wake_irq *wirq)
>                 return -EEXIST;
>         }
>
> -       dev->power.wakeirq = wirq;
> +       WRITE_ONCE(dev->power.wakeirq, wirq);
>         device_wakeup_attach_irq(dev, wirq);
>
>         spin_unlock_irqrestore(&dev->power.lock, flags);
> @@ -83,15 +83,21 @@ EXPORT_SYMBOL_GPL(dev_pm_set_wake_irq);
>   */
>  void dev_pm_clear_wake_irq(struct device *dev)
>  {
> -       struct wake_irq *wirq = dev->power.wakeirq;
> +       struct wake_irq *wirq = READ_ONCE(dev->power.wakeirq);

Just remove the dev->power.wakeirq access from here.

>         unsigned long flags;
>
>         if (!wirq)
>                 return;

Along with the check above because it is still racy.

>         spin_lock_irqsave(&dev->power.lock, flags);
> +       wirq = dev->power.wakeirq;
> +       if (!wirq) {
> +               spin_unlock_irqrestore(&dev->power.lock, flags);
> +               return;
> +       }

And just do the above.

WRITE_ONCE() will not be needed then.

> +
>         device_wakeup_detach_irq(dev);
> -       dev->power.wakeirq = NULL;
> +       WRITE_ONCE(dev->power.wakeirq, NULL);
>         spin_unlock_irqrestore(&dev->power.lock, flags);
>
>         if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED) {
> --