From: Wen Yang <redacted>
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@redhat.com>
Signed-off-by: Wen Yang <redacted>
---
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