[PATCH] nvmem: Drop a layout's cells when it is unregistered

[Alexey Charkov]

Layout drivers add cells carrying a read_post_process callback, and
sometimes private data, which belong to the layout driver itself.
Since layouts became regular devices, that driver may be a module,
unbound and unloaded independently of the nvmem provider, yet
nvmem_layout_unregister() drops nothing: the cells stay registered in
the core along with their now dangling callback pointer, as do the
sysfs attributes referring to them. Reading such a cell afterwards,
through sysfs for instance, calls into freed module text.

Nothing prevents that unload either. The module reference taken in
of_nvmem_cell_get() is released by nvmem_cell_put() as soon as a
consumer is done with the cell, so once the consumers have probed the
layout module is free to go. Layout drivers cannot clean up after
themselves, as the core exports no way to remove a cell.

Record the layout which populated each cell and drop those cells again
when the layout is unregistered. As the sysfs attributes point at the
cell entries, take the "cells" attribute group down before freeing them
and rebuild it afterwards for the cells which remain. This also stops
the cells of a failed add_cells() from being left behind, and releases
the device_node references those cells hold.

Fixes: fc29fd821d9a ("nvmem: core: Rework layouts to become regular devices")
Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
Closes: https://lore.kernel.org/all/20260901160339.C57051F000E9@xxxxxxxxxxxxxxx/
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Alexey Charkov <alchark@xxxxxxxxxxx>
---
 drivers/nvmem/core.c      | 114 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/nvmem/internals.h |   2 +
 2 files changed, 110 insertions(+), 6 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 0556d140170a..c9a977c52c61 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -35,6 +35,7 @@ struct nvmem_cell_entry {
 	int			nbits;
 	nvmem_cell_post_process_t read_post_process;
 	void			*priv;
+	struct nvmem_layout	*layout;
 	struct device_node	*np;
 	struct nvmem_device	*nvmem;
 	struct list_head	node;
@@ -517,11 +518,42 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
 	if (ret)
 		return ret;
 
+	nvmem->sysfs_cells_attrs = attrs;
+	nvmem->sysfs_cells_pattrs = pattrs;
 	nvmem->sysfs_cells_populated = true;
 
 	return ret;
 }
 
+static void nvmem_destroy_sysfs_cells(struct nvmem_device *nvmem)
+{
+	struct attribute_group group = {
+		.name	= "cells",
+	};
+	unsigned int i;
+
+	guard(mutex)(&nvmem_mutex);
+
+	if (!nvmem->sysfs_cells_populated)
+		return;
+
+	/*
+	 * Removing the group waits for any read in flight, after which nothing
+	 * refers to the cell entries through their attributes any more.
+	 */
+	device_remove_group(&nvmem->dev, &group);
+
+	for (i = 0; nvmem->sysfs_cells_pattrs[i]; i++)
+		devm_kfree(&nvmem->dev, nvmem->sysfs_cells_pattrs[i]->attr.name);
+
+	devm_kfree(&nvmem->dev, nvmem->sysfs_cells_attrs);
+	devm_kfree(&nvmem->dev, nvmem->sysfs_cells_pattrs);
+
+	nvmem->sysfs_cells_attrs = NULL;
+	nvmem->sysfs_cells_pattrs = NULL;
+	nvmem->sysfs_cells_populated = false;
+}
+
 #else /* CONFIG_NVMEM_SYSFS */
 
 static int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
@@ -529,10 +561,20 @@ static int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
 {
 	return -ENOSYS;
 }
+
 static void nvmem_sysfs_remove_compat(struct nvmem_device *nvmem)
 {
 }
 
+static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
+{
+	return 0;
+}
+
+static void nvmem_destroy_sysfs_cells(struct nvmem_device *nvmem)
+{
+}
+
 #endif /* CONFIG_NVMEM_SYSFS */
 
 static void nvmem_release(struct device *dev)
@@ -571,6 +613,15 @@ static void nvmem_device_remove_all_cells(const struct nvmem_device *nvmem)
 		nvmem_cell_entry_drop(cell);
 }
 
+static void nvmem_device_remove_layout_cells(struct nvmem_layout *layout)
+{
+	struct nvmem_cell_entry *cell, *p;
+
+	list_for_each_entry_safe(cell, p, &layout->nvmem->cells, node)
+		if (cell->layout == layout)
+			nvmem_cell_entry_drop(cell);
+}
+
 static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
 {
 	scoped_guard(mutex, &nvmem_mutex)
@@ -844,33 +895,84 @@ static int nvmem_add_cells_from_legacy_of(struct nvmem_device *nvmem)
 	return nvmem_add_cells_from_dt(nvmem, nvmem->dev.of_node);
 }
 
+/**
+ * nvmem_layout_register() - Register a layout and populate its cells
+ *
+ * @layout: nvmem layout, as handed to the layout driver's probe callback
+ *
+ * Runs the layout's add_cells() callback and takes ownership of the cells it
+ * adds, so that nvmem_layout_unregister() can drop them again. Meant to be
+ * called by a layout driver from its probe callback.
+ *
+ * Return: 0 on success, a negative error code otherwise.
+ */
 int nvmem_layout_register(struct nvmem_layout *layout)
 {
+	struct nvmem_device *nvmem = layout->nvmem;
+	struct nvmem_cell_entry *cell;
+	struct list_head *pos, *last;
 	int ret;
 
 	if (!layout->add_cells)
 		return -EINVAL;
 
+	scoped_guard(mutex, &nvmem_mutex)
+		last = nvmem->cells.prev;
+
 	/* Populate the cells */
 	ret = layout->add_cells(layout);
-	if (ret)
+
+	/*
+	 * Claim whatever the layout has just appended, including on failure, so
+	 * that it can be dropped again when the layout goes away. Cells added
+	 * by anyone else, before or after this, are left alone.
+	 */
+	scoped_guard(mutex, &nvmem_mutex) {
+		for (pos = last->next; pos != &nvmem->cells; pos = pos->next) {
+			cell = list_entry(pos, struct nvmem_cell_entry, node);
+			cell->layout = layout;
+		}
+	}
+
+	if (ret) {
+		nvmem_device_remove_layout_cells(layout);
 		return ret;
+	}
 
-#ifdef CONFIG_NVMEM_SYSFS
-	ret = nvmem_populate_sysfs_cells(layout->nvmem);
+	ret = nvmem_populate_sysfs_cells(nvmem);
 	if (ret) {
-		nvmem_device_remove_all_cells(layout->nvmem);
+		nvmem_device_remove_all_cells(nvmem);
 		return ret;
 	}
-#endif
 
 	return 0;
 }
 EXPORT_SYMBOL_GPL(nvmem_layout_register);
 
+/**
+ * nvmem_layout_unregister() - Unregister a layout and drop its cells
+ *
+ * @layout: nvmem layout to unregister
+ *
+ * Drops the cells which nvmem_layout_register() has populated, as they refer
+ * to the layout driver and must not outlive it. Meant to be called by a layout
+ * driver from its remove callback.
+ */
 void nvmem_layout_unregister(struct nvmem_layout *layout)
 {
-	/* Keep the API even with an empty stub in case we need it later */
+	struct nvmem_device *nvmem = layout->nvmem;
+
+	/*
+	 * The cells this layout added hold a read_post_process callback, and
+	 * possibly private data, belonging to the layout driver, which may be a
+	 * module on its way out. Drop them, taking their sysfs attributes down
+	 * first as those point at the cells, then publish what remains again.
+	 */
+	nvmem_destroy_sysfs_cells(nvmem);
+	nvmem_device_remove_layout_cells(layout);
+
+	if (nvmem_populate_sysfs_cells(nvmem))
+		dev_warn(&nvmem->dev, "failed to rebuild cell attributes\n");
 }
 EXPORT_SYMBOL_GPL(nvmem_layout_unregister);
 
diff --git a/drivers/nvmem/internals.h b/drivers/nvmem/internals.h
index 4e610deeaa7b..9345364f6ed7 100644
--- a/drivers/nvmem/internals.h
+++ b/drivers/nvmem/internals.h
@@ -36,6 +36,8 @@ struct nvmem_device {
 	struct nvmem_layout	*layout;
 	struct nvmem_operations	*ops;
 	void *priv;
+	const struct bin_attribute *sysfs_cells_attrs;
+	const struct bin_attribute **sysfs_cells_pattrs;
 	bool			sysfs_cells_populated;
 };
 

---
base-commit: 8b72f6626dc39b9e7e82b2721d4f7c3b86286012
change-id: 20260902-nvmem-layout-unreg-2d0ab332395b

Best regards,
--  
Alexey Charkov <alchark@xxxxxxxxxxx>