[PATCH v8 04/14] rv: Disarm HA timers on the storage free path

[wen . yang]

From: Wen Yang <wen.yang@xxxxxxxxx>

da_destroy_storage() cancels the HA timer through da_monitor_reset_hook()
and only then unlinks the storage with hash_del_rcu().  A handler on
another CPU that looked the storage up before the unlink can still call
ha_start_timer_*() after the cancel.  The timer then stays queued on a
storage that kfree_rcu() frees a grace period later, and fires on freed
memory, reporting a violation for an object that no longer exists.

A monitor cannot close this from its own handlers: any cancel it issues
precedes the unlink.  The first point no reader can race with is the
RCU callback that frees the storage.

Add da_monitor_free_hook(), called right before a per-object storage is
freed, from the RCU callback and from da_monitor_destroy().  HA monitors
implement it by calling ha_cancel_timer_sync() if da_monitoring() is true.
Replace kfree_rcu() with call_rcu() so there is a callback to hook.

Cancelling synchronously from the RCU callback is safe: HA hrtimers are
always armed in HARD mode, so hrtimer_cancel() only spins, and on
PREEMPT_RT, where timer_delete_sync() may sleep, RCU callbacks run in
the rcuc kthread.

Storages whose monitor never started are skipped via da_monitoring().

Fixes: 4a24127bd6cb ("rv: Add support for per-object monitors in DA/HA")
Suggested-by: Gabriele Monaco <gmonaco@xxxxxxxxxx>
Signed-off-by: Wen Yang <wen.yang@xxxxxxxxx>
---
 include/rv/da_monitor.h | 27 ++++++++++++++++++++++++++-
 include/rv/ha_monitor.h |  8 ++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h
index e3cf85c9ce55..c7f9733fb8a4 100644
--- a/include/rv/da_monitor.h
+++ b/include/rv/da_monitor.h
@@ -67,6 +67,17 @@ static struct rv_monitor rv_this;
 #define da_monitor_sync_hook()
 #endif
 
+/*
+ * Called on the last reference to a storage, right before it is freed.
+ * Monitors embedding a timer disarm it here: an earlier point races with a
+ * reader that resolved the storage before hash_del_rcu(), and
+ * tracepoint_synchronize_unregister() only covers tracepoint probes.
+ * Default: no-op.
+ */
+#ifndef da_monitor_free_hook
+#define da_monitor_free_hook(da_mon)
+#endif
+
 /*
  * Type for the target id, default to int but can be overridden.
  * A long type can work as hash table key (PER_OBJ) but will be downgraded to
@@ -543,6 +554,15 @@ static inline monitor_target da_get_target_by_id(da_id_type id)
 	return mon_storage->target;
 }
 
+static void da_kfree_cb(struct rcu_head *head)
+{
+	struct da_monitor_storage *ms =
+		container_of(head, struct da_monitor_storage, rcu);
+
+	da_monitor_free_hook(&ms->rv.da_mon);
+	kfree(ms);
+}
+
 /*
  * da_destroy_storage - destroy the per-object storage
  *
@@ -564,7 +584,11 @@ static inline void da_destroy_storage(da_id_type id)
 		return;
 	da_monitor_reset_hook(&mon_storage->rv.da_mon);
 	hash_del_rcu(&mon_storage->node);
-	kfree_rcu(mon_storage, rcu);
+	/*
+	 * A reader from before hash_del_rcu() may still arm; the free
+	 * callback runs past it, so the hook disarms there.
+	 */
+	call_rcu(&mon_storage->rcu, da_kfree_cb);
 }
 
 static void __da_monitor_reset_all(void (*reset)(struct da_monitor *))
@@ -607,6 +631,7 @@ static inline void da_monitor_destroy(void)
 	 * pending, we can safely assume no concurrent user.
 	 */
 	hash_for_each_safe(da_monitor_ht, bkt, tmp, mon_storage, node) {
+		da_monitor_free_hook(&mon_storage->rv.da_mon);
 		hash_del_rcu(&mon_storage->node);
 		kfree(mon_storage);
 	}
diff --git a/include/rv/ha_monitor.h b/include/rv/ha_monitor.h
index ebb7f91e3447..3bf1dd15544c 100644
--- a/include/rv/ha_monitor.h
+++ b/include/rv/ha_monitor.h
@@ -29,6 +29,7 @@ static inline void ha_monitor_reset_env(struct da_monitor *da_mon);
 static inline void ha_setup_timer(struct ha_monitor *ha_mon);
 static inline bool ha_cancel_timer(struct ha_monitor *ha_mon);
 static inline void ha_cancel_timer_sync(struct ha_monitor *ha_mon);
+static inline void ha_monitor_free_env(struct da_monitor *da_mon);
 static bool ha_monitor_handle_constraint(struct da_monitor *da_mon,
 					 enum states curr_state,
 					 enum events event,
@@ -38,6 +39,7 @@ static bool ha_monitor_handle_constraint(struct da_monitor *da_mon,
 #define da_monitor_init_hook ha_monitor_init_env
 #define da_monitor_reset_hook ha_monitor_reset_env
 #define da_monitor_sync_hook() synchronize_rcu()
+#define da_monitor_free_hook ha_monitor_free_env
 
 #if !defined(HA_SKIP_AUTO_CLEANUP) && RV_MON_TYPE == RV_MON_PER_TASK
 /*
@@ -551,6 +553,12 @@ static inline bool ha_cancel_timer(struct ha_monitor *ha_mon)
 static inline void ha_cancel_timer_sync(struct ha_monitor *ha_mon) { }
 #endif
 
+static inline void ha_monitor_free_env(struct da_monitor *da_mon)
+{
+	if (da_monitoring(da_mon))
+		ha_cancel_timer_sync(to_ha_monitor(da_mon));
+}
+
 #if IS_ENABLED(CONFIG_RV_MONITORS_KUNIT_TEST)
 #ifdef RV_MON_OPS_INIT
 #undef RV_MON_OPS_INIT
-- 
2.25.1