[PATCH] soc: xilinx: xlnx_event_manager: protect the registration table

DORMANTno replies

From: Jaidev Shastri via B4 Relay <devnull+jaidevshastri.vt.edu@kernel.org>
Date: 2026-09-22 00:55:19
Also in: b4-sent, lkml
Subsystem: the rest, xilinx event management driver · Maintainers: Linus Torvalds, Michal Simek

From: Jaidev Shastri <redacted>

xlnx_register_event() and xlnx_unregister_event() modify reg_driver_map
and the per-event callback lists with hash_add(), list_add(), hash_del()
and list_del(), taking no lock. xlnx_event_handler() walks the same
table and lists from the SGI, on whichever CPU the firmware raised it,
also without a lock, and calls the registered callbacks.

Once the first event is registered with the firmware the SGI can arrive
at any time, including while versal_edac or zynqmp_power registers
further events. hlist_add_head() and list_add() are plain stores, so the
handler can see a node before its key, flags, list head or callback
pointer are visible, and call through a pointer it reads as NULL. Two
registrations hashing to the same bucket corrupt it outright.

Serialise the writers with a spinlock and walk the table and the lists
under RCU in the handler. hash_add_rcu() and list_add_rcu() publish the
entries, removed entries are freed with kfree_rcu(), and the allocations
move in front of the critical section so that they keep using
GFP_KERNEL. The lock is taken with interrupts disabled because the
handler removes entries when re-registration with the firmware fails.

Found with MBCheck, a static herd7-based memory consistency checker.

Signed-off-by: Jaidev Shastri <redacted>
---
 drivers/soc/xilinx/xlnx_event_manager.c | 183 ++++++++++++++++++++------------
 1 file changed, 116 insertions(+), 67 deletions(-)

diff --git a/drivers/soc/xilinx/xlnx_event_manager.c b/drivers/soc/xilinx/xlnx_event_manager.c
index f733dc42b..157051e45 100644
--- a/drivers/soc/xilinx/xlnx_event_manager.c
+++ b/drivers/soc/xilinx/xlnx_event_manager.c

@@ -18,7 +18,9 @@
 #include <linux/module.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
+#include <linux/rcupdate.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 
 static DEFINE_PER_CPU_READ_MOSTLY(int, dummy_cpu_number);

@@ -39,6 +41,11 @@ static int event_manager_availability = -EACCES;
 #define REGISTER_NOTIFIER_FIRMWARE_VERSION	(2U)
 
 static DEFINE_HASHTABLE(reg_driver_map, REGISTERED_DRIVER_MAX_ORDER);
+/*
+ * Serialises insertions into and removals from reg_driver_map and the
+ * per-event callback lists. The SGI handler walks both under RCU.
+ */
+static DEFINE_SPINLOCK(reg_driver_map_lock);
 static int sgi_num = XLNX_EVENT_SGI_NUM;
 
 static bool is_need_to_unregister;

@@ -48,11 +55,13 @@ static bool is_need_to_unregister;
  * @agent_data:		Data passed back to handler function.
  * @eve_cb:		Function pointer to store the callback function.
  * @list:		member to create list.
+ * @rcu:		used to free the callback after a grace period.
  */
 struct agent_cb {
 	void *agent_data;
 	event_cb_func_t eve_cb;
 	struct list_head list;
+	struct rcu_head rcu;
 };
 
 /**

@@ -66,6 +75,7 @@ struct agent_cb {
  * @cb_list_head:	Head of call back data list which contain the information
  *			about registered handler and private data.
  * @hentry:		hlist_node that hooks this entry into hashtable.
+ * @rcu:		used to free the entry after a grace period.
  */
 struct registered_event_data {
 	u64 key;

@@ -73,6 +83,7 @@ struct registered_event_data {
 	bool wake;
 	struct list_head cb_list_head;
 	struct hlist_node hentry;
+	struct rcu_head rcu;
 };
 
 static bool xlnx_is_error_event(const u32 node_id)

@@ -104,14 +115,27 @@ static bool xlnx_is_error_event(const u32 node_id)
 static int xlnx_add_cb_for_notify_event(const u32 node_id, const u32 event, const bool wake,
 					event_cb_func_t cb_fun,	void *data)
 {
-	u64 key = 0;
+	u64 key = ((u64)node_id << 32U) | (u64)event;
 	bool present_in_hash = false;
-	struct registered_event_data *eve_data;
+	struct registered_event_data *eve_data, *new_eve_data;
 	struct agent_cb *cb_data;
 	struct agent_cb *cb_pos;
-	struct agent_cb *cb_next;
+	unsigned long flags;
+
+	/* Allocate up front: the hash table is updated under a spinlock */
+	new_eve_data = kmalloc_obj(*new_eve_data);
+	if (!new_eve_data)
+		return -ENOMEM;
+
+	cb_data = kmalloc_obj(*cb_data);
+	if (!cb_data) {
+		kfree(new_eve_data);
+		return -ENOMEM;
+	}
+	cb_data->eve_cb = cb_fun;
+	cb_data->agent_data = data;
 
-	key = ((u64)node_id << 32U) | (u64)event;
+	spin_lock_irqsave(&reg_driver_map_lock, flags);
 	/* Check for existing entry in hash table for given key id */
 	hash_for_each_possible(reg_driver_map, eve_data, hentry, key) {
 		if (eve_data->key == key) {

@@ -122,85 +146,94 @@ static int xlnx_add_cb_for_notify_event(const u32 node_id, const u32 event, cons
 
 	if (!present_in_hash) {
 		/* Add new entry if not present in HASH table */
-		eve_data = kmalloc_obj(*eve_data);
-		if (!eve_data)
-			return -ENOMEM;
+		eve_data = new_eve_data;
+		new_eve_data = NULL;
 		eve_data->key = key;
 		eve_data->cb_type = PM_NOTIFY_CB;
 		eve_data->wake = wake;
 		INIT_LIST_HEAD(&eve_data->cb_list_head);
 
-		cb_data = kmalloc_obj(*cb_data);
-		if (!cb_data) {
-			kfree(eve_data);
-			return -ENOMEM;
-		}
-		cb_data->eve_cb = cb_fun;
-		cb_data->agent_data = data;
-
 		/* Add into callback list */
-		list_add(&cb_data->list, &eve_data->cb_list_head);
-
-		/* Add into HASH table */
-		hash_add(reg_driver_map, &eve_data->hentry, key);
+		list_add_rcu(&cb_data->list, &eve_data->cb_list_head);
+		cb_data = NULL;
+
+		/*
+		 * Add into HASH table. hash_add_rcu() publishes the entry with
+		 * release semantics, so the SGI handler, which walks the table
+		 * without the lock, sees the key, the flags and the callback
+		 * list only once they have been written.
+		 */
+		hash_add_rcu(reg_driver_map, &eve_data->hentry, key);
 	} else {
 		/* Search for callback function and private data in list */
-		list_for_each_entry_safe(cb_pos, cb_next, &eve_data->cb_list_head, list) {
+		list_for_each_entry(cb_pos, &eve_data->cb_list_head, list) {
 			if (cb_pos->eve_cb == cb_fun &&
-			    cb_pos->agent_data == data) {
-				return 0;
-			}
+			    cb_pos->agent_data == data)
+				goto out;
 		}
 
 		/* Add multiple handler and private data in list */
-		cb_data = kmalloc_obj(*cb_data);
-		if (!cb_data)
-			return -ENOMEM;
-		cb_data->eve_cb = cb_fun;
-		cb_data->agent_data = data;
-
-		list_add(&cb_data->list, &eve_data->cb_list_head);
+		list_add_rcu(&cb_data->list, &eve_data->cb_list_head);
+		cb_data = NULL;
 	}
 
+out:
+	spin_unlock_irqrestore(&reg_driver_map_lock, flags);
+	kfree(new_eve_data);
+	kfree(cb_data);
+
 	return 0;
 }
 
 static int xlnx_add_cb_for_suspend(event_cb_func_t cb_fun, void *data)
 {
-	struct registered_event_data *eve_data;
+	struct registered_event_data *eve_data, *new_eve_data;
 	struct agent_cb *cb_data;
+	unsigned long flags;
+	int ret = 0;
 
+	/* Allocate up front: the hash table is updated under a spinlock */
+	new_eve_data = kmalloc_obj(*new_eve_data);
+	if (!new_eve_data)
+		return -ENOMEM;
+
+	cb_data = kmalloc_obj(*cb_data);
+	if (!cb_data) {
+		kfree(new_eve_data);
+		return -ENOMEM;
+	}
+	cb_data->eve_cb = cb_fun;
+	cb_data->agent_data = data;
+
+	spin_lock_irqsave(&reg_driver_map_lock, flags);
 	/* Check for existing entry in hash table for given cb_type */
 	hash_for_each_possible(reg_driver_map, eve_data, hentry, PM_INIT_SUSPEND_CB) {
 		if (eve_data->cb_type == PM_INIT_SUSPEND_CB) {
 			pr_err("Found as already registered\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 	}
 
 	/* Add new entry if not present */
-	eve_data = kmalloc_obj(*eve_data);
-	if (!eve_data)
-		return -ENOMEM;
-
+	eve_data = new_eve_data;
+	new_eve_data = NULL;
 	eve_data->key = 0;
 	eve_data->cb_type = PM_INIT_SUSPEND_CB;
 	INIT_LIST_HEAD(&eve_data->cb_list_head);
 
-	cb_data = kmalloc_obj(*cb_data);
-	if (!cb_data) {
-		kfree(eve_data);
-		return -ENOMEM;
-	}
-	cb_data->eve_cb = cb_fun;
-	cb_data->agent_data = data;
-
 	/* Add into callback list */
-	list_add(&cb_data->list, &eve_data->cb_list_head);
+	list_add_rcu(&cb_data->list, &eve_data->cb_list_head);
+	cb_data = NULL;
 
-	hash_add(reg_driver_map, &eve_data->hentry, PM_INIT_SUSPEND_CB);
+	hash_add_rcu(reg_driver_map, &eve_data->hentry, PM_INIT_SUSPEND_CB);
 
-	return 0;
+out:
+	spin_unlock_irqrestore(&reg_driver_map_lock, flags);
+	kfree(new_eve_data);
+	kfree(cb_data);
+
+	return ret;
 }
 
 static int xlnx_remove_cb_for_suspend(event_cb_func_t cb_fun)

@@ -210,9 +243,11 @@ static int xlnx_remove_cb_for_suspend(event_cb_func_t cb_fun)
 	struct agent_cb *cb_pos;
 	struct agent_cb *cb_next;
 	struct hlist_node *tmp;
+	unsigned long flags;
 
 	is_need_to_unregister = false;
 
+	spin_lock_irqsave(&reg_driver_map_lock, flags);
 	/* Check for existing entry in hash table for given cb_type */
 	hash_for_each_possible_safe(reg_driver_map, eve_data, tmp, hentry, PM_INIT_SUSPEND_CB) {
 		if (eve_data->cb_type == PM_INIT_SUSPEND_CB) {

@@ -220,16 +255,17 @@ static int xlnx_remove_cb_for_suspend(event_cb_func_t cb_fun)
 			list_for_each_entry_safe(cb_pos, cb_next, &eve_data->cb_list_head, list) {
 				if (cb_pos->eve_cb == cb_fun) {
 					is_callback_found = true;
-					list_del_init(&cb_pos->list);
-					kfree(cb_pos);
+					list_del_rcu(&cb_pos->list);
+					kfree_rcu(cb_pos, rcu);
 				}
 			}
 			/* remove an object from a hashtable */
-			hash_del(&eve_data->hentry);
-			kfree(eve_data);
+			hash_del_rcu(&eve_data->hentry);
+			kfree_rcu(eve_data, rcu);
 			is_need_to_unregister = true;
 		}
 	}
+	spin_unlock_irqrestore(&reg_driver_map_lock, flags);
 	if (!is_callback_found) {
 		pr_warn("Didn't find any registered callback for suspend event\n");
 		return -EINVAL;

@@ -247,9 +283,11 @@ static int xlnx_remove_cb_for_notify_event(const u32 node_id, const u32 event,
 	struct agent_cb *cb_pos;
 	struct agent_cb *cb_next;
 	struct hlist_node *tmp;
+	unsigned long flags;
 
 	is_need_to_unregister = false;
 
+	spin_lock_irqsave(&reg_driver_map_lock, flags);
 	/* Check for existing entry in hash table for given key id */
 	hash_for_each_possible_safe(reg_driver_map, eve_data, tmp, hentry, key) {
 		if (eve_data->key == key) {

@@ -258,20 +296,21 @@ static int xlnx_remove_cb_for_notify_event(const u32 node_id, const u32 event,
 				if (cb_pos->eve_cb == cb_fun &&
 				    cb_pos->agent_data == data) {
 					is_callback_found = true;
-					list_del_init(&cb_pos->list);
-					kfree(cb_pos);
+					list_del_rcu(&cb_pos->list);
+					kfree_rcu(cb_pos, rcu);
 				}
 			}
 
 			/* Remove HASH table if callback list is empty */
 			if (list_empty(&eve_data->cb_list_head)) {
 				/* remove an object from a HASH table */
-				hash_del(&eve_data->hentry);
-				kfree(eve_data);
+				hash_del_rcu(&eve_data->hentry);
+				kfree_rcu(eve_data, rcu);
 				is_need_to_unregister = true;
 			}
 		}
 	}
+	spin_unlock_irqrestore(&reg_driver_map_lock, flags);
 	if (!is_callback_found) {
 		pr_warn("Didn't find any registered callback for 0x%x 0x%x\n",
 			node_id, event);

@@ -445,17 +484,18 @@ static void xlnx_call_suspend_cb_handler(const u32 *payload)
 	struct registered_event_data *eve_data;
 	u32 cb_type = payload[0];
 	struct agent_cb *cb_pos;
-	struct agent_cb *cb_next;
 
 	/* Check for existing entry in hash table for given cb_type */
-	hash_for_each_possible(reg_driver_map, eve_data, hentry, cb_type) {
+	rcu_read_lock();
+	hash_for_each_possible_rcu(reg_driver_map, eve_data, hentry, cb_type) {
 		if (eve_data->cb_type == cb_type) {
-			list_for_each_entry_safe(cb_pos, cb_next, &eve_data->cb_list_head, list) {
+			list_for_each_entry_rcu(cb_pos, &eve_data->cb_list_head, list) {
 				cb_pos->eve_cb(&payload[0], cb_pos->agent_data);
 				is_callback_found = true;
 			}
 		}
 	}
+	rcu_read_unlock();
 	if (!is_callback_found)
 		pr_warn("Didn't find any registered callback for suspend event\n");
 }

@@ -467,12 +507,18 @@ static void xlnx_call_notify_cb_handler(const u32 *payload)
 	u64 key = ((u64)payload[1] << 32U) | (u64)payload[2];
 	int ret;
 	struct agent_cb *cb_pos;
-	struct agent_cb *cb_next;
 
+	/*
+	 * Registration may run on another CPU while this SGI is handled. The
+	 * table and the callback lists are walked under RCU; the writers
+	 * publish entries with hash_add_rcu()/list_add_rcu() and free them
+	 * after a grace period, so an entry is never seen before its fields.
+	 */
+	rcu_read_lock();
 	/* Check for existing entry in hash table for given key id */
-	hash_for_each_possible(reg_driver_map, eve_data, hentry, key) {
+	hash_for_each_possible_rcu(reg_driver_map, eve_data, hentry, key) {
 		if (eve_data->key == key) {
-			list_for_each_entry_safe(cb_pos, cb_next, &eve_data->cb_list_head, list) {
+			list_for_each_entry_rcu(cb_pos, &eve_data->cb_list_head, list) {
 				cb_pos->eve_cb(&payload[0], cb_pos->agent_data);
 				is_callback_found = true;
 			}

@@ -483,8 +529,7 @@ static void xlnx_call_notify_cb_handler(const u32 *payload)
 			if (ret) {
 				pr_err("%s() failed for 0x%x and 0x%x: %d\r\n", __func__,
 				       payload[1], payload[2], ret);
-				list_for_each_entry_safe(cb_pos, cb_next, &eve_data->cb_list_head,
-							 list) {
+				list_for_each_entry_rcu(cb_pos, &eve_data->cb_list_head, list) {
 					/* Remove already registered event from hash table */
 					xlnx_remove_cb_for_notify_event(payload[1], payload[2],
 									cb_pos->eve_cb,

@@ -493,6 +538,7 @@ static void xlnx_call_notify_cb_handler(const u32 *payload)
 			}
 		}
 	}
+	rcu_read_unlock();
 	if (!is_callback_found)
 		pr_warn("Unhandled SGI node 0x%x event 0x%x. Expected with Xen hypervisor\n",
 			payload[1], payload[2]);

@@ -690,15 +736,18 @@ static void xlnx_event_manager_remove(struct platform_device *pdev)
 	int ret;
 	struct agent_cb *cb_pos;
 	struct agent_cb *cb_next;
+	unsigned long flags;
 
+	spin_lock_irqsave(&reg_driver_map_lock, flags);
 	hash_for_each_safe(reg_driver_map, i, tmp, eve_data, hentry) {
 		list_for_each_entry_safe(cb_pos, cb_next, &eve_data->cb_list_head, list) {
-			list_del_init(&cb_pos->list);
-			kfree(cb_pos);
+			list_del_rcu(&cb_pos->list);
+			kfree_rcu(cb_pos, rcu);
 		}
-		hash_del(&eve_data->hentry);
-		kfree(eve_data);
+		hash_del_rcu(&eve_data->hentry);
+		kfree_rcu(eve_data, rcu);
 	}
+	spin_unlock_irqrestore(&reg_driver_map_lock, flags);
 
 	ret = zynqmp_pm_register_sgi(0, 1);
 	if (ret)

---
base-commit: 93f51579e7df248780214094418f205253383cc5
change-id: 20260921-mb-xlnx-event-8e2b4b380bb9

Best regards,
--  
Jaidev Shastri [off-list ref]

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help