[PATCH 0/2] PM: cpu: Restore the RCU grace period to cpu_pm_unregister_notifier()

[PATCH 0/2] PM: cpu: Restore the RCU grace period to cpu_pm_unregister_notifier()

[Bradley Morgan]

Since commit b2f6662ac08d ("PM: cpu: Make notifier chain use a
raw_spinlock_t") cpu_pm_unregister_notifier() no longer waits for an
RCU grace period, but cpu_pm_notify() still walks the chain with only
rcu_read_lock() held. A driver that frees its notifier block right
after unregistering can race with a concurrent walker.

Patch 1 moves the one caller that unregisters from atomic context out
from under its spinlock, patch 2 restores the grace period.

Bradley Morgan (2):
  clocksource/drivers/timer-ti-dm: Unregister CPU PM notifier outside of
    the timer lock
  PM: cpu: Restore synchronize_rcu() to cpu_pm_unregister_notifier()

 drivers/clocksource/timer-ti-dm.c | 9 ++++++---
 kernel/cpu_pm.c                   | 9 ++++++++-
 2 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.47.3

[PATCH 1/2] clocksource/drivers/timer-ti-dm: Unregister CPU PM notifier outside of the timer lock

[Bradley Morgan]

omap_dm_timer_remove() calls cpu_pm_unregister_notifier() with
dm_timer_lock held and interrupts disabled. Nothing sleeps in there
today, but it pins the helper into a context where it can never be
allowed to sleep, which is in the way of restoring the RCU grace
period on the cpu_pm notifier chain.

Do the list lookup under the lock and move the unregister after the
unlock. Nothing can race it at that point: remove() owns the device,
and once the timer is off the list nobody can reach it anymore.

Signed-off-by: Bradley Morgan <include@grrlz.net>
---
 drivers/clocksource/timer-ti-dm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/timer-ti-dm.c b/drivers/clocksource/timer-ti-dm.c
index bd06afb7d522..032e102bdd0a 100644
--- a/drivers/clocksource/timer-ti-dm.c
+++ b/drivers/clocksource/timer-ti-dm.c
@@ -1530,7 +1530,7 @@ static int omap_dm_timer_probe(struct platform_device *pdev)
  */
 static void omap_dm_timer_remove(struct platform_device *pdev)
 {
-	struct dmtimer *timer;
+	struct dmtimer *timer, *found = NULL;
 	unsigned long flags;
 	int ret = -EINVAL;
 
@@ -1538,14 +1538,17 @@ static void omap_dm_timer_remove(struct platform_device *pdev)
 	list_for_each_entry(timer, &omap_timer_list, node)
 		if (!strcmp(dev_name(&timer->pdev->dev),
 			    dev_name(&pdev->dev))) {
-			if (!(timer->capability & OMAP_TIMER_ALWON))
-				cpu_pm_unregister_notifier(&timer->nb);
 			list_del(&timer->node);
+			found = timer;
 			ret = 0;
 			break;
 		}
 	spin_unlock_irqrestore(&dm_timer_lock, flags);
 
+	/* Unregister outside the lock: cpu_pm_unregister_notifier() may sleep. */
+	if (found && !(found->capability & OMAP_TIMER_ALWON))
+		cpu_pm_unregister_notifier(&found->nb);
+
 	pm_runtime_disable(&pdev->dev);
 
 	if (ret)
-- 
2.47.3

[PATCH 2/2] PM: cpu: Restore synchronize_rcu() to cpu_pm_unregister_notifier()

[Bradley Morgan]

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: stable@vger.kernel.org
Signed-off-by: Bradley Morgan <include@grrlz.net>
---
 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