[PATCH v2] driver core: faux: Add sysfs groups after probing

[Kurt Borja]

Manually add sysfs groups after the faux_device_ops's probe succeeds.
Likewise remove these groups just before calling the faux_devices_ops's
remove callback. This approach approximates the order in which the
driver core adds and removes the driver's .dev_groups of a device to
avoid lifetime issues.

This is done specifically to avoid using the device's .groups member,
which adds groups before the device is even registered to the bus.

This lets consumers of this API, initialize resources on the .probe
callback and then use them inside is_visible/show/store methods, through
dev_get_drvdata() without races.

Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>
Cc: Danilo Krummrich <dakr@xxxxxxxxxx>
Signed-off-by: Kurt Borja <kuurtb@xxxxxxxxx>
---
Hi all,

Currently, groups are added automatically to the device using it's
`.groups` member. I think this is problematic because as you can see
here [1], these groups are added way before the bus's probe is called.

This makes the seemingly innocuous pattern of setting drvdata in the
probe and dereferincing it in a group is_visible callback, result in a
NULL pointer dereference.

I believe this isn't a problem yet, but it might be in the future if a
developer doesn't know this very counter-intuitive detail.

I went ahead and sent a v2 with modifications to the commit message in
case the original patch got lost in the list.

This is based on the driver-core-next branch of the driver-core tree.

[1] https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/base/core.c#L3620
---
Changes in v2:
- Slightly reword the commit message
- Link to v1: https://lore.kernel.org/r/20250225170318.3826-1-kuurtb@xxxxxxxxx
---
 drivers/base/faux.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/base/faux.c b/drivers/base/faux.c
index 407c1d1aad50b7969a6dab9d2027d8beab66a754..9054d346bd7fe89575b95c1491467850bcd0393a 100644
--- a/drivers/base/faux.c
+++ b/drivers/base/faux.c
@@ -25,6 +25,7 @@
 struct faux_object {
 	struct faux_device faux_dev;
 	const struct faux_device_ops *faux_ops;
+	const struct attribute_group **groups;
 };
 #define to_faux_object(dev) container_of_const(dev, struct faux_object, faux_dev.dev)
 
@@ -43,10 +44,21 @@ static int faux_probe(struct device *dev)
 	struct faux_object *faux_obj = to_faux_object(dev);
 	struct faux_device *faux_dev = &faux_obj->faux_dev;
 	const struct faux_device_ops *faux_ops = faux_obj->faux_ops;
-	int ret = 0;
+	int ret;
 
-	if (faux_ops && faux_ops->probe)
+	if (faux_ops && faux_ops->probe) {
 		ret = faux_ops->probe(faux_dev);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Add groups after the probe succeeds to ensure resources are
+	 * initialized correctly
+	 */
+	ret = device_add_groups(dev, faux_obj->groups);
+	if (ret && faux_ops && faux_ops->remove)
+		faux_ops->remove(faux_dev);
 
 	return ret;
 }
@@ -57,6 +69,8 @@ static void faux_remove(struct device *dev)
 	struct faux_device *faux_dev = &faux_obj->faux_dev;
 	const struct faux_device_ops *faux_ops = faux_obj->faux_ops;
 
+	device_remove_groups(dev, faux_obj->groups);
+
 	if (faux_ops && faux_ops->remove)
 		faux_ops->remove(faux_dev);
 }
@@ -124,8 +138,9 @@ struct faux_device *faux_device_create_with_groups(const char *name,
 	if (!faux_obj)
 		return NULL;
 
-	/* Save off the callbacks so we can use them in the future */
+	/* Save off the callbacks and groups so we can use them in the future */
 	faux_obj->faux_ops = faux_ops;
+	faux_obj->groups = groups;
 
 	/* Initialize the device portion and register it with the driver core */
 	faux_dev = &faux_obj->faux_dev;
@@ -138,7 +153,6 @@ struct faux_device *faux_device_create_with_groups(const char *name,
 	else
 		dev->parent = &faux_bus_root;
 	dev->bus = &faux_bus_type;
-	dev->groups = groups;
 	dev_set_name(dev, "%s", name);
 
 	ret = device_add(dev);

---
base-commit: 51d0de7596a458096756c895cfed6bc4a7ecac10
change-id: 20250327-faux-groups-3e0c4cffe9c6

Best regards,
-- 
 ~ Kurt