[PATCH] PM: Delete timer before removing wakeup source from list

[PATCH] PM: Delete timer before removing wakeup source from list

[Kaushlendra Kumar]

Move timer_delete_sync() before list_del_rcu() in wakeup_source_remove()
to ensure proper cleanup ordering. This prevents the timer callback from
executing after the wakeup source has been removed from the events list.

The previous order could allow the timer callback to access the wakeup
source entry after removal but before timer deletion, potentially causing
use-after-free issues or list corruption.

Deleting the timer first ensures that no callbacks can execute during
the wakeup source removal process, providing safer cleanup semantics.

Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@intel.com>
---
 drivers/base/power/wakeup.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index d1283ff1080b..ae6ec9f04b61 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -189,12 +189,11 @@ static void wakeup_source_remove(struct wakeup_source *ws)
 	if (WARN_ON(!ws))
 		return;
 
+	timer_delete_sync(&ws->timer);
 	raw_spin_lock_irqsave(&events_lock, flags);
 	list_del_rcu(&ws->entry);
 	raw_spin_unlock_irqrestore(&events_lock, flags);
 	synchronize_srcu(&wakeup_srcu);
-
-	timer_delete_sync(&ws->timer);
 	/*
 	 * Clear timer.function to make wakeup_source_not_registered() treat
 	 * this wakeup source as not registered.
-- 
2.34.1

Re: [PATCH] PM: Delete timer before removing wakeup source from list

[Rafael J. Wysocki]

On Sun, Sep 21, 2025 at 6:27 AM Kaushlendra Kumar
<kaushlendra.kumar@intel.com> wrote:
>
> Move timer_delete_sync() before list_del_rcu() in wakeup_source_remove()
> to ensure proper cleanup ordering. This prevents the timer callback from
> executing after the wakeup source has been removed from the events list.
>
> The previous order could allow the timer callback to access the wakeup
> source entry after removal but before timer deletion, potentially causing
> use-after-free issues or list corruption.

How so?  You need to specify the scenario in which that can happen.

> Deleting the timer first ensures that no callbacks can execute during
> the wakeup source removal process, providing safer cleanup semantics.
>
> Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@intel.com>
> ---
>  drivers/base/power/wakeup.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index d1283ff1080b..ae6ec9f04b61 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -189,12 +189,11 @@ static void wakeup_source_remove(struct wakeup_source *ws)
>         if (WARN_ON(!ws))
>                 return;
>
> +       timer_delete_sync(&ws->timer);
>         raw_spin_lock_irqsave(&events_lock, flags);
>         list_del_rcu(&ws->entry);
>         raw_spin_unlock_irqrestore(&events_lock, flags);
>         synchronize_srcu(&wakeup_srcu);
> -
> -       timer_delete_sync(&ws->timer);
>         /*
>          * Clear timer.function to make wakeup_source_not_registered() treat
>          * this wakeup source as not registered.
> --
> 2.34.1
>

RE: [PATCH] PM: Delete timer before removing wakeup source from list

[Kumar, Kaushlendra]

On Sun, Sep 21, 2025 at 6:27 AM, Rafael J. Wysocki wrote:
> On Sun, Sep 21, 2025 at 6:27 AM Kaushlendra Kumar <kaushlendra.kumar@intel.com> wrote:
> >
> > Move timer_delete_sync() before list_del_rcu() in 
> > wakeup_source_remove() to ensure proper cleanup ordering. This 
> > prevents the timer callback from executing after the wakeup source has been removed from the events list.
> >
> > The previous order could allow the timer callback to access the wakeup 
> > source entry after removal but before timer deletion, potentially 
> > causing use-after-free issues or list corruption.
> 
> How so?  You need to specify the scenario in which that can happen.

Hi Rafael,

Thank you for asking for clarification. Here's the specific scenario where the 
it can occur:

**Issue Condition Timeline:**

1. **Thread A** calls wakeup_source_remove():

raw_spin_lock_irqsave(&events_lock, flags); list_del_rcu(&ws->entry); // Remove from events list raw_spin_unlock_irqrestore(&events_lock, flags); synchronize_srcu(&wakeup_srcu); // Wait for RCU readers // *** ISSUE WINDOW HERE *** timer_delete_sync(&ws->timer); // Timer still active!

2. **Thread B** (timer callback) fires during the issue window:

static void pm_wakeup_timer_fn(struct timer_list *t) { struct wakeup_source *ws = from_timer(ws, t, timer);
// Problem: ws->entry was already removed from events list
   // but timer callback still executes

**Specific Issues:**

**Timer accesses removed wakeup_source**: The timer callback 
(pm_wakeup_timer_fn) can execute after list_del_rcu() but before 
timer_delete_sync(), accessing a wakeup_source that's no longer in 
the events list.

**Why moving timer_delete_sync() first fixes this:**

- Ensures timer cannot fire during list removal
- Guarantees no timer callbacks execute after we start cleanup

The fix ensures that once we start wakeup_source_remove(), no timer 
callbacks can execute

Re: [PATCH] PM: Delete timer before removing wakeup source from list

[Rafael J. Wysocki]

On Tue, Sep 23, 2025 at 6:52 AM Kumar, Kaushlendra
<kaushlendra.kumar@intel.com> wrote:
>
> On Sun, Sep 21, 2025 at 6:27 AM, Rafael J. Wysocki wrote:
> > On Sun, Sep 21, 2025 at 6:27 AM Kaushlendra Kumar <kaushlendra.kumar@intel.com> wrote:
> > >
> > > Move timer_delete_sync() before list_del_rcu() in
> > > wakeup_source_remove() to ensure proper cleanup ordering. This
> > > prevents the timer callback from executing after the wakeup source has been removed from the events list.
> > >
> > > The previous order could allow the timer callback to access the wakeup
> > > source entry after removal but before timer deletion, potentially
> > > causing use-after-free issues or list corruption.
> >
> > How so?  You need to specify the scenario in which that can happen.
>
> Hi Rafael,
>
> Thank you for asking for clarification. Here's the specific scenario where the
> it can occur:
>
> **Issue Condition Timeline:**
>
> 1. **Thread A** calls wakeup_source_remove():
>
> raw_spin_lock_irqsave(&events_lock, flags); list_del_rcu(&ws->entry); // Remove from events list raw_spin_unlock_irqrestore(&events_lock, flags); synchronize_srcu(&wakeup_srcu); // Wait for RCU readers // *** ISSUE WINDOW HERE *** timer_delete_sync(&ws->timer); // Timer still active!
>
> 2. **Thread B** (timer callback) fires during the issue window:
>
> static void pm_wakeup_timer_fn(struct timer_list *t) { struct wakeup_source *ws = from_timer(ws, t, timer);
> // Problem: ws->entry was already removed from events list
>    // but timer callback still executes

Why is this a problem?  ws is still there and the timer callback
doesn't do anything with ws->entry AFAICS.

> **Specific Issues:**
>
> **Timer accesses removed wakeup_source**: The timer callback
> (pm_wakeup_timer_fn) can execute after list_del_rcu() but before
> timer_delete_sync(), accessing a wakeup_source that's no longer in
> the events list.

Again, why is this a problem?

> **Why moving timer_delete_sync() first fixes this:**
>
> - Ensures timer cannot fire during list removal
> - Guarantees no timer callbacks execute after we start cleanup
>
> The fix ensures that once we start wakeup_source_remove(), no timer
> callbacks can execute

You may argue that the new code will be cleaner (fair enough), but
unless there is a specific correctness matter here, you can't call it
a fix.

RE: [PATCH] PM: Delete timer before removing wakeup source from list

[Kumar, Kaushlendra]

On Tue, Sep 23, 2025 at 6:52 AM, Rafael J. Wysocki wrote:
> On Tue, Sep 23, 2025 at 6:52 AM Kumar, Kaushlendra <kaushlendra.kumar@intel.com> wrote:
> >
> > On Sun, Sep 21, 2025 at 6:27 AM, Rafael J. Wysocki wrote:
> > > On Sun, Sep 21, 2025 at 6:27 AM Kaushlendra Kumar <kaushlendra.kumar@intel.com> wrote:
> > > >
> > > > Move timer_delete_sync() before list_del_rcu() in
> > > > wakeup_source_remove() to ensure proper cleanup ordering. This 
> > > > prevents the timer callback from executing after the wakeup source has been removed from the events list.
> > > >
> > > > The previous order could allow the timer callback to access the 
> > > > wakeup source entry after removal but before timer deletion, 
> > > > potentially causing use-after-free issues or list corruption.
> > >
> > > How so?  You need to specify the scenario in which that can happen.
> >
> > Hi Rafael,
> >
> > Thank you for asking for clarification. Here's the specific scenario 
> > where the it can occur:
> >
> > **Issue Condition Timeline:**
> >
> > 1. **Thread A** calls wakeup_source_remove():
> >
> > raw_spin_lock_irqsave(&events_lock, flags); list_del_rcu(&ws->entry); // Remove from events list raw_spin_unlock_irqrestore(&events_lock, flags); synchronize_srcu(&wakeup_srcu); // Wait for RCU readers // *** ISSUE WINDOW HERE *** timer_delete_sync(&ws->timer); // Timer still active!
> >
> > 2. **Thread B** (timer callback) fires during the issue window:
> >
> > static void pm_wakeup_timer_fn(struct timer_list *t) { struct 
> > wakeup_source *ws = from_timer(ws, t, timer); // Problem: ws->entry was already removed from events list
> >    // but timer callback still executes
> 
> Why is this a problem?  ws is still there and the timer callback doesn't do anything with ws->entry AFAICS.
> 
> > **Specific Issues:**
> >
> > **Timer accesses removed wakeup_source**: The timer callback
> > (pm_wakeup_timer_fn) can execute after list_del_rcu() but before 
> > timer_delete_sync(), accessing a wakeup_source that's no longer in the 
> > events list.
> 
> Again, why is this a problem?
> 
> > **Why moving timer_delete_sync() first fixes this:**
> >
> > - Ensures timer cannot fire during list removal
> > - Guarantees no timer callbacks execute after we start cleanup
> >
> > The fix ensures that once we start wakeup_source_remove(), no timer 
> > callbacks can execute
> 
> You may argue that the new code will be cleaner (fair enough), but unless there is a specific correctness matter here, you can't call it a fix.

Hi Rafael,

You're absolutely correct, I don't have any evidence in real use cases for this. Given your feedback, I should 
Reframe it as a cleanup/improvement patch rather than a "fix"

If it is fine with you then I can create v2 patch with your suggestions.

Best regards,
Kaushlendra

Re: [PATCH] PM: Delete timer before removing wakeup source from list

[Rafael J. Wysocki]

On Wed, Sep 24, 2025 at 6:15 AM Kumar, Kaushlendra
<kaushlendra.kumar@intel.com> wrote:
>
> On Tue, Sep 23, 2025 at 6:52 AM, Rafael J. Wysocki wrote:
> > On Tue, Sep 23, 2025 at 6:52 AM Kumar, Kaushlendra <kaushlendra.kumar@intel.com> wrote:
> > >
> > > On Sun, Sep 21, 2025 at 6:27 AM, Rafael J. Wysocki wrote:
> > > > On Sun, Sep 21, 2025 at 6:27 AM Kaushlendra Kumar <kaushlendra.kumar@intel.com> wrote:
> > > > >
> > > > > Move timer_delete_sync() before list_del_rcu() in
> > > > > wakeup_source_remove() to ensure proper cleanup ordering. This
> > > > > prevents the timer callback from executing after the wakeup source has been removed from the events list.
> > > > >
> > > > > The previous order could allow the timer callback to access the
> > > > > wakeup source entry after removal but before timer deletion,
> > > > > potentially causing use-after-free issues or list corruption.
> > > >
> > > > How so?  You need to specify the scenario in which that can happen.
> > >
> > > Hi Rafael,
> > >
> > > Thank you for asking for clarification. Here's the specific scenario
> > > where the it can occur:
> > >
> > > **Issue Condition Timeline:**
> > >
> > > 1. **Thread A** calls wakeup_source_remove():
> > >
> > > raw_spin_lock_irqsave(&events_lock, flags); list_del_rcu(&ws->entry); // Remove from events list raw_spin_unlock_irqrestore(&events_lock, flags); synchronize_srcu(&wakeup_srcu); // Wait for RCU readers // *** ISSUE WINDOW HERE *** timer_delete_sync(&ws->timer); // Timer still active!
> > >
> > > 2. **Thread B** (timer callback) fires during the issue window:
> > >
> > > static void pm_wakeup_timer_fn(struct timer_list *t) { struct
> > > wakeup_source *ws = from_timer(ws, t, timer); // Problem: ws->entry was already removed from events list
> > >    // but timer callback still executes
> >
> > Why is this a problem?  ws is still there and the timer callback doesn't do anything with ws->entry AFAICS.
> >
> > > **Specific Issues:**
> > >
> > > **Timer accesses removed wakeup_source**: The timer callback
> > > (pm_wakeup_timer_fn) can execute after list_del_rcu() but before
> > > timer_delete_sync(), accessing a wakeup_source that's no longer in the
> > > events list.
> >
> > Again, why is this a problem?
> >
> > > **Why moving timer_delete_sync() first fixes this:**
> > >
> > > - Ensures timer cannot fire during list removal
> > > - Guarantees no timer callbacks execute after we start cleanup
> > >
> > > The fix ensures that once we start wakeup_source_remove(), no timer
> > > callbacks can execute
> >
> > You may argue that the new code will be cleaner (fair enough), but unless there is a specific correctness matter here, you can't call it a fix.
>
> Hi Rafael,
>
> You're absolutely correct, I don't have any evidence in real use cases for this. Given your feedback, I should
> Reframe it as a cleanup/improvement patch rather than a "fix"
>
> If it is fine with you then I can create v2 patch with your suggestions.

Yes, please!

RE: [PATCH] PM: Delete timer before removing wakeup source from list

[Kumar, Kaushlendra]

On Wed, Sep 24, 2025 at 6:15 AM, Rafael J. Wysocki wrote:
> On Wed, Sep 24, 2025 at 6:15 AM Kumar, Kaushlendra <kaushlendra.kumar@intel.com> wrote:
> >
> > On Tue, Sep 23, 2025 at 6:52 AM, Rafael J. Wysocki wrote:
> > > On Tue, Sep 23, 2025 at 6:52 AM Kumar, Kaushlendra <kaushlendra.kumar@intel.com> wrote:
> > > >
> > > > On Sun, Sep 21, 2025 at 6:27 AM, Rafael J. Wysocki wrote:
> > > > > On Sun, Sep 21, 2025 at 6:27 AM Kaushlendra Kumar <kaushlendra.kumar@intel.com> wrote:
> > > > > >
> > > > > > Move timer_delete_sync() before list_del_rcu() in
> > > > > > wakeup_source_remove() to ensure proper cleanup ordering. This 
> > > > > > prevents the timer callback from executing after the wakeup source has been removed from the events list.
> > > > > >
> > > > > > The previous order could allow the timer callback to access 
> > > > > > the wakeup source entry after removal but before timer 
> > > > > > deletion, potentially causing use-after-free issues or list corruption.
> > > > >
> > > > > How so?  You need to specify the scenario in which that can happen.
> > > >
> > > > Hi Rafael,
> > > >
> > > > Thank you for asking for clarification. Here's the specific 
> > > > scenario where the it can occur:
> > > >
> > > > **Issue Condition Timeline:**
> > > >
> > > > 1. **Thread A** calls wakeup_source_remove():
> > > >
> > > > raw_spin_lock_irqsave(&events_lock, flags); list_del_rcu(&ws->entry); // Remove from events list raw_spin_unlock_irqrestore(&events_lock, flags); synchronize_srcu(&wakeup_srcu); // Wait for RCU readers // *** ISSUE WINDOW HERE *** timer_delete_sync(&ws->timer); // Timer still active!
> > > >
> > > > 2. **Thread B** (timer callback) fires during the issue window:
> > > >
> > > > static void pm_wakeup_timer_fn(struct timer_list *t) { struct 
> > > > wakeup_source *ws = from_timer(ws, t, timer); // Problem: ws->entry was already removed from events list
> > > >    // but timer callback still executes
> > >
> > > Why is this a problem?  ws is still there and the timer callback doesn't do anything with ws->entry AFAICS.
> > >
> > > > **Specific Issues:**
> > > >
> > > > **Timer accesses removed wakeup_source**: The timer callback
> > > > (pm_wakeup_timer_fn) can execute after list_del_rcu() but before 
> > > > timer_delete_sync(), accessing a wakeup_source that's no longer in 
> > > > the events list.
> > >
> > > Again, why is this a problem?
> > >
> > > > **Why moving timer_delete_sync() first fixes this:**
> > > >
> > > > - Ensures timer cannot fire during list removal
> > > > - Guarantees no timer callbacks execute after we start cleanup
> > > >
> > > > The fix ensures that once we start wakeup_source_remove(), no 
> > > > timer callbacks can execute
> > >
> > > You may argue that the new code will be cleaner (fair enough), but unless there is a specific correctness matter here, you can't call it a fix.
> >
> > Hi Rafael,
> >
> > You're absolutely correct, I don't have any evidence in real use cases 
> > for this. Given your feedback, I should Reframe it as a cleanup/improvement patch rather than a "fix"
> >
> > If it is fine with you then I can create v2 patch with your suggestions.
> 
> Yes, please!

Hi Rafael,

Thank you for the approval!.  I have sent  a v2 patch with the proper framing as a code improvement

Best regards,
Kaushlendra