Re: [PATCH 2/2] PM: cpu: Restore synchron ize_rcu() to cpu_pm_unregister_notifier()
Bradley Morgan <[email protected]>
| Newsgroups | org.kernel.vger.linux-pm,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
On 20 August 2026 22:24:51 BST, "Rafael J. Wysocki (Intel)" <[email protected]> wrote: >On Thu, Aug 13, 2026 at 9:43 AM Bradley Morgan <[email protected]> wrote: >> >> cpu_pm_notify() walks the notifier chain lockless under only >> rcu_read_lock(). That only works if a removed notifier block is not >> freed until every concurrent walker is done with it. >> >> The chain used to be an atomic_notifier, whose unregister ends in >> synchronize_rcu(). Commit b2f6662ac08d ("PM: cpu: Make notifier chain >> use a raw_spinlock_t") switched it over to a raw_notifier, and in >> doing so replaced that with raw_notifier_chain_unregister(), which >> does not synchronize. The grace period quietly went away, so a driver >> that frees the memory holding its notifier block right after >> cpu_pm_unregister_notifier() returns can race with a concurrent >> walker: >> >> cpu1 (idle exit) cpu2 (driver remove) >> ---------------- -------------------- >> cpu_pm_notify(CPU_PM_EXIT) >> rcu_read_lock() >> nb = rcu_dereference_raw(*nl) >> cpu_pm_unregister_notifier(&od->nb) >> unlink, no grace period >> remove() frees od (devm) >> nb->notifier_call(nb, ...) >> /* use after free */ >> >> The rcu_read_lock() on cpu1 does not stop cpu2 from freeing the >> block, and nothing else does. The read side is the idle exit path, >> so this can hit on any system where a driver with a cpu_pm notifier >> gets unbound. >> >> Add the grace period back. It is only needed when a notifier was >> actually removed, so wait on success and return -ENOENT without >> waiting otherwise. The kerneldoc gets back the "may sleep" note that >> the same commit dropped. >> >> Fixes: b2f6662ac08d ("PM: cpu: Make notifier chain use a >raw_spinlock_t") >> Cc: [email protected] >> Signed-off-by: Bradley Morgan <[email protected]> > >Acked-by: Rafael J. Wysocki (Intel) <[email protected]> > >or if you want me to pick up this series, I need an ACK on the first >patch. > Waiting game... (And I can't take it! I'm not maintainer of anything sadly, one day, one day, I'm manifesting it.. one day) >> --- >> kernel/cpu_pm.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c >> index 7481fbb947d3..a2a598ad6e6d 100644 >> --- a/kernel/cpu_pm.c >> +++ b/kernel/cpu_pm.c >> @@ -10,6 +10,7 @@ >> #include <linux/cpu_pm.h> >> #include <linux/module.h> >> #include <linux/notifier.h> >> +#include <linux/rcupdate.h> >> #include <linux/spinlock.h> >> #include <linux/syscore_ops.h> >> >> @@ -76,7 +77,8 @@ EXPORT_SYMBOL_GPL(cpu_pm_register_notifier); >> * >> * Remove a driver from the CPU PM notifier list. >> * >> - * This function has the same return conditions as >raw_notifier_chain_unregister. >> + * This function may sleep, and has the same return conditions as >> + * raw_notifier_chain_unregister. >> */ >> int cpu_pm_unregister_notifier(struct notifier_block *nb) >> { >> @@ -86,6 +88,11 @@ int cpu_pm_unregister_notifier(struct notifier_block >*nb) >> raw_spin_lock_irqsave(&cpu_pm_notifier.lock, flags); >> ret = raw_notifier_chain_unregister(&cpu_pm_notifier.chain, nb); >> raw_spin_unlock_irqrestore(&cpu_pm_notifier.lock, flags); >> + >> + /* Wait for the rcu_read_lock() walkers in cpu_pm_notify(). */ >> + if (!ret) >> + synchronize_rcu(); >> + >> return ret; >> } >> EXPORT_SYMBOL_GPL(cpu_pm_unregister_notifier); >> -- >> 2.47.3 >> > Thanks!