[PATCH] gpio: sim: snapshot configfs leaf items before depending on them

[PATCH] gpio: sim: snapshot configfs leaf items before depending on them

[Yu Zhang]

gpio_sim_device_lockup_configfs() walks dev->bank_list and each
bank->line_list to depend (or undepend) the leaf configfs items when a
simulated device is activated or deactivated.  The walk runs without
holding dev->lock:

	list_for_each_entry(bank, &dev->bank_list, siblings) {
		list_for_each_entry(line, &bank->line_list, siblings) {
			...
			configfs_depend_item_unlocked(subsys, item);

Every other traversal of these lists holds dev->lock, and the lists are
mutated under it: gpio_sim_line_config_group_release() and
gpio_sim_bank_config_group_release() both do list_del() under dev->lock
and then free the line/bank.  A line or bank can be removed by rmdir(2)
while it is not yet (or no longer) depended on, i.e. in the window around
activation before configfs_depend_item_unlocked() has pinned it, and in
the window around deactivation after configfs_undepend_item_unlocked()
has released it.  The lockless walk can then dereference a line or bank
that a concurrent release is freeing, or follow a ->siblings pointer into
a freed node, resulting in a use-after-free / list corruption.

The walk cannot simply be moved under dev->lock, because
configfs_[un]depend_item_unlocked() take configfs-internal locks and
holding dev->lock across them would invert the lock order against the
configfs rmdir path, which calls the release callbacks (taking dev->lock)
with those configfs locks held.

Take a pinned snapshot of the leaf items under dev->lock instead.  The
list pointers are now only followed with the lock held; each item is
pinned with config_item_get_unless_zero() so it cannot be freed once the
lock is dropped, and items already being torn down (get returns NULL) are
skipped, since they must not be depended on anyway.  The snapshot is then
depended/undepended with the lock dropped - preserving the required lock
order - and each item is put afterwards.

Fixes: 8bd76b3d3f3a ("gpio: sim: lock up configfs that an instantiated device depends on")
Cc: stable@vger.kernel.org
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/gpio/gpio-sim.c | 56 +++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 0c73bec..562f305 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -978,27 +978,61 @@ static void
 gpio_sim_device_lockup_configfs(struct gpio_sim_device *dev, bool lock)
 {
 	struct configfs_subsystem *subsys = dev->group.cg_subsys;
+	struct config_item **items = NULL, *item;
 	struct gpio_sim_bank *bank;
 	struct gpio_sim_line *line;
-	struct config_item *item;
+	size_t i, n;
 
 	/*
 	 * The device only needs to depend on leaf entries. This is
 	 * sufficient to lock up all the configfs entries that the
 	 * instantiated, alive device depends on.
+	 *
+	 * The bank and line lists are only stable under dev->lock, but
+	 * configfs_[un]depend_item_unlocked() must not be called under it:
+	 * they take configfs-internal locks and that would invert the lock
+	 * order against the configfs rmdir path, which removes lines and
+	 * banks from these lists and frees them under dev->lock. So take a
+	 * pinned snapshot of the leaf items under the lock - skipping any
+	 * that are already being torn down - and then depend/undepend the
+	 * snapshot with the lock dropped.
 	 */
-	list_for_each_entry(bank, &dev->bank_list, siblings) {
-		list_for_each_entry(line, &bank->line_list, siblings) {
-			item = line->hog ? &line->hog->item
-					 : &line->group.cg_item;
-
-			if (lock)
-				WARN_ON(configfs_depend_item_unlocked(subsys,
-								      item));
-			else
-				configfs_undepend_item_unlocked(item);
+	scoped_guard(mutex, &dev->lock) {
+		n = 0;
+		list_for_each_entry(bank, &dev->bank_list, siblings)
+			list_for_each_entry(line, &bank->line_list, siblings)
+				n++;
+
+		if (!n)
+			return;
+
+		items = kcalloc(n, sizeof(*items), GFP_KERNEL);
+		if (!items)
+			return;
+
+		i = 0;
+		list_for_each_entry(bank, &dev->bank_list, siblings) {
+			list_for_each_entry(line, &bank->line_list, siblings) {
+				item = line->hog ? &line->hog->item
+						 : &line->group.cg_item;
+				item = config_item_get_unless_zero(item);
+				if (item)
+					items[i++] = item;
+			}
 		}
+		n = i;
 	}
+
+	for (i = 0; i < n; i++) {
+		if (lock)
+			WARN_ON(configfs_depend_item_unlocked(subsys,
+							      items[i]));
+		else
+			configfs_undepend_item_unlocked(items[i]);
+		config_item_put(items[i]);
+	}
+
+	kfree(items);
 }
 
 static ssize_t
-- 
2.43.0

Re: [PATCH] gpio: sim: snapshot configfs leaf items before depending on them

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] gpio: sim: race with mkdir/rmdir in gpio_sim_device_lockup_configfs()
- [Critical] gpio: sim: extra leaf ref in gpio_sim_device_lockup_configfs() UAFs parent
- [High] gpio: sim: unchecked kcalloc() failure in gpio_sim_device_lockup_configfs()
--

commit 2fe24a96c3fb83559b6e4860a61df83ca81d16bb
Author: Yu Zhang <yuz08559@gmail.com>

gpio: sim: snapshot configfs leaf items before depending on them

This commit takes a reference-counted snapshot of leaf configfs items in
gpio_sim_device_lockup_configfs() while holding dev->lock, then calls
configfs_depend_item_unlocked() or configfs_undepend_item_unlocked() after
dropping the lock to avoid lock-order inversion with configfs rmdir.

> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
> index 898638e2794cb..e4d1318b3019c 100644
> --- a/drivers/gpio/gpio-sim.c
> +++ b/drivers/gpio/gpio-sim.c
> @@ -978,27 +978,61 @@ static void
>  gpio_sim_device_lockup_configfs(struct gpio_sim_device *dev, bool lock)
>  {
>  	struct configfs_subsystem *subsys = dev->group.cg_subsys;
> +	struct config_item **items = NULL, *item;
>  	struct gpio_sim_bank *bank;
>  	struct gpio_sim_line *line;
> -	struct config_item *item;
> +	size_t i, n;
>  
>  	/*
>  	 * The device only needs to depend on leaf entries. This is
>  	 * sufficient to lock up all the configfs entries that the
>  	 * instantiated, alive device depends on.
> +	 *
> +	 * The bank and line lists are only stable under dev->lock, but
> +	 * configfs_[un]depend_item_unlocked() must not be called under it:
> +	 * they take configfs-internal locks and that would invert the lock
> +	 * order against the configfs rmdir path, which removes lines and
> +	 * banks from these lists and frees them under dev->lock. So take a
> +	 * pinned snapshot of the leaf items under the lock - skipping any
> +	 * that are already being torn down - and then depend/undepend the
> +	 * snapshot with the lock dropped.
>  	 */
> -	list_for_each_entry(bank, &dev->bank_list, siblings) {
> -		list_for_each_entry(line, &bank->line_list, siblings) {
> -			item = line->hog ? &line->hog->item
> -					 : &line->group.cg_item;
> -
> -			if (lock)
> -				WARN_ON(configfs_depend_item_unlocked(subsys,
> -								      item));
> -			else
> -				configfs_undepend_item_unlocked(item);
> +	scoped_guard(mutex, &dev->lock) {
> +		n = 0;
> +		list_for_each_entry(bank, &dev->bank_list, siblings)
> +			list_for_each_entry(line, &bank->line_list, siblings)
> +				n++;
> +
> +		if (!n)
> +			return;
> +
> +		items = kcalloc(n, sizeof(*items), GFP_KERNEL);
> +		if (!items)
> +			return;

[Severity: High]
What happens if kcalloc() fails here when called from
gpio_sim_device_config_live_store()?

drivers/gpio/gpio-sim.c:gpio_sim_device_config_live_store() {
    ...
	if (live)
		gpio_sim_device_lockup_configfs(dev, true);

	scoped_guard(mutex, &dev->lock) {
		if (live == gpio_sim_device_is_live(dev))
			ret = -EPERM;
		else if (live)
			ret = gpio_sim_device_activate(dev);
		else
			gpio_sim_device_deactivate(dev);
	}

	if (live == !!ret)
		gpio_sim_device_lockup_configfs(dev, false);
}

Because gpio_sim_device_lockup_configfs() returns void, an allocation failure
on the lock == true path is silently ignored without depending on any leaf
items. gpio_sim_device_config_live_store() still proceeds, and when
gpio_sim_device_lockup_configfs(dev, false) later runs (either immediately on
activation failure / -EPERM or later on deactivation) and its kcalloc()
succeeds, configfs_undepend_item_unlocked() -> configfs_undepend_item() hits:

fs/configfs/dir.c:configfs_undepend_item() {
    ...
	sd = target->ci_dentry->d_fsdata;
	BUG_ON(sd->s_dependent_count < 1);

	sd->s_dependent_count -= 1;
    ...
}

Conversely, if kcalloc() fails on the lock == false path during deactivation,
gpio_sim_device_deactivate() has already torn down the device, so the
s_dependent_count references on all leaf items are permanently leaked and the
configfs directories can never be removed.

Should gpio_sim_device_lockup_configfs() propagate allocation errors, or can
the items be tracked without a dynamic allocation on the undepend path?

> +
> +		i = 0;
> +		list_for_each_entry(bank, &dev->bank_list, siblings) {
> +			list_for_each_entry(line, &bank->line_list, siblings) {
> +				item = line->hog ? &line->hog->item
> +						 : &line->group.cg_item;
> +				item = config_item_get_unless_zero(item);
> +				if (item)
> +					items[i++] = item;
> +			}
>  		}
> +		n = i;
>  	}

[Severity: Critical]
Can this snapshot logic still race with concurrent configfs mkdir and rmdir
operations in three ways?

First, when a hog directory is removed via configfs_rmdir(), the final
config_item_put() drops hog->item.ci_kref to 0 and calls
gpio_sim_hog_config_item_release(), which waits on dev->lock before clearing
line->hog:

drivers/gpio/gpio-sim.c:gpio_sim_hog_config_item_release() {
    ...
	scoped_guard(mutex, &dev->lock)
		line->hog = NULL;
    ...
}

If gpio_sim_device_lockup_configfs(dev, true) holds dev->lock at that point,
it sees line->hog != NULL, calls config_item_get_unless_zero(&line->hog->item)
which returns NULL, and skips the line completely without falling back to
&line->group.cg_item. Once dev->lock is dropped, line->hog becomes NULL, so a
subsequent gpio_sim_device_lockup_configfs(dev, false) snapshots
&line->group.cg_item and triggers BUG_ON(sd->s_dependent_count < 1) in
configfs_undepend_item().

Second, in configfs_rmdir(), unlink_obj() clears item->ci_group = NULL before
the final config_item_put() drops ci_kref to 0 and runs the release callback:

fs/configfs/dir.c:unlink_obj() {
    ...
	item->ci_group = NULL;
	item->ci_parent = NULL;

	/* Drop the reference for ci_entry */
	config_item_put(item);
}

An item can therefore be snapshotted via config_item_get_unless_zero() after
unlink_obj() has already cleared ci_group (or a concurrent configfs_rmdir()
can run unlink_obj() after dev->lock is dropped but before
configfs_depend_item_unlocked() runs). When configfs_depend_item_unlocked()
then inspects the snapshotted item, target->ci_group is NULL and
parent->cg_subsys dereferences a NULL pointer:

fs/configfs/dir.c:configfs_depend_item_unlocked() {
    ...
	parent = target->ci_group;
	if (configfs_is_root(&parent->cg_item)) {
		target_subsys = to_configfs_subsystem(to_config_group(target));
		root = parent;
	} else {
		target_subsys = parent->cg_subsys;
    ...
}

Third, in configfs_mkdir(), gpio_sim_bank_config_make_line_group() and
gpio_sim_line_config_make_hog_item() add the new item to bank->line_list or
line->hog with ci_kref == 1 under dev->lock before link_obj() sets
item->ci_group and before configfs_dir_set_ready() clears
CONFIGFS_USET_CREATING. Snapshotting the item in that window either
dereferences NULL target->ci_group in configfs_depend_item_unlocked() or fails
with -ENOENT in configfs_depend_prep() (leaving s_dependent_count == 0 and
later hitting BUG_ON(sd->s_dependent_count < 1) on undepend).

> +
> +	for (i = 0; i < n; i++) {
> +		if (lock)
> +			WARN_ON(configfs_depend_item_unlocked(subsys,
> +							      items[i]));
> +		else
> +			configfs_undepend_item_unlocked(items[i]);
> +		config_item_put(items[i]);
> +	}

[Severity: Critical]
Does holding an extra ci_kref reference on items[i] across the unlocked window
allow a child's release callback to run after its parent has already been
freed?

In configfs, unlink_obj() drops the child's reference on its parent
config_group during configfs_rmdir(child) via config_group_put(group), rather
than when the child's ci_kref reaches 0.

If a concurrent rmdir removes the leaf item (for example, right after
configfs_undepend_item_unlocked(items[i]) on deactivation, or before
configfs_depend_item_unlocked() on activation) while items[i] still holds a
reference from config_item_get_unless_zero(), configfs_rmdir(child) finishes
and drops the reference on the parent directory while child->ci_kref remains
1. Userspace can then immediately rmdir the now-empty parent line or bank
directory, freeing struct gpio_sim_line or struct gpio_sim_bank.

When config_item_put(items[i]) then drops the last reference on the child,
gpio_sim_hog_config_item_release() or gpio_sim_line_config_group_release()
dereferences the uncounted hog->parent or line->parent pointer into the freed
parent structure:

drivers/gpio/gpio-sim.c:gpio_sim_hog_config_item_release() {
	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
	struct gpio_sim_line *line = hog->parent;
	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);

	scoped_guard(mutex, &dev->lock)
		line->hog = NULL;
    ...
}

drivers/gpio/gpio-sim.c:gpio_sim_line_config_group_release() {
	struct gpio_sim_line *line = to_gpio_sim_line(item);
	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);

	scoped_guard(mutex, &dev->lock)
		list_del(&line->siblings);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026060049.60599-1-yuz08559@gmail.com?part=1

[PATCH v2] gpio: sim: fix the configfs lockup racing with mkdir/rmdir

[Yu Zhang]

gpio_sim_device_lockup_configfs() depends (on activation) and undepends
(on deactivation) the leaf configfs entries of a device. It walks
dev->bank_list and bank->line_list without dev->lock, while the lists
are modified and their nodes freed under dev->lock by the configfs
make/release callbacks. It also walks the lists twice, once to depend
and once to undepend, and the two walks can see different entries:

 - A hog created while the device is live is never depended on, but
   it is undepended on deactivation, hitting
   BUG_ON(sd->s_dependent_count < 1) in configfs_undepend_item().
   The following is enough to trigger it:

	mkdir $G/dev $G/dev/bank0 $G/dev/bank0/line0
	echo 1 > $G/dev/live
	mkdir $G/dev/bank0/line0/hog
	echo 0 > $G/dev/live

 - An entry that is being created or removed when the device goes live
   cannot be depended on (the WARN_ON() fires), but it is undepended
   later, hitting the same BUG_ON(). If it has already been unlinked,
   configfs_depend_item_unlocked() dereferences its NULL ci_group.

Toggling "live" while concurrently creating and removing line and hog
directories hits the WARN_ON() and then the BUG_ON() within a second.

Hold the configfs subsystem mutex and dev->lock across the whole live
transition. The subsystem mutex is what configfs holds when it links
and unlinks entries and when it calls the make_group()/make_item()
callbacks, which then take dev->lock, so the lock order is the same.
Record the entries that were actually depended on and undepend exactly
those. If an entry cannot be depended on because it is being created
or removed, undo the dependencies already taken and fail with -EBUSY
instead of warning.

configfs_depend_item_unlocked() and configfs_undepend_item_unlocked()
only take configfs_dirent_lock for an item of the caller's own
subsystem, so calling them under these locks is fine.

Fixes: 8bd76b3d3f3a ("gpio: sim: lock up configfs that an instantiated device depends on")
Cc: stable@vger.kernel.org
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/gpio/gpio-sim.c | 109 +++++++++++++++++++++++++++++++---------
 1 file changed, 85 insertions(+), 24 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 0c73bec..ad4affc 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -560,6 +560,10 @@ struct gpio_sim_device {
 	struct mutex lock;
 
 	struct list_head bank_list;
+
+	/* Leaf entries depended on while the device is live. */
+	struct config_item **deps;
+	unsigned int num_deps;
 };
 
 static struct gpio_sim_device *to_gpio_sim_device(struct config_item *item)
@@ -974,31 +978,81 @@ static void gpio_sim_device_deactivate(struct gpio_sim_device *dev)
 	gpio_sim_remove_swnode_recursive(swnode);
 }
 
-static void
-gpio_sim_device_lockup_configfs(struct gpio_sim_device *dev, bool lock)
+static void gpio_sim_device_unlock_configfs(struct gpio_sim_device *dev)
+{
+	unsigned int i;
+
+	lockdep_assert_held(&dev->lock);
+
+	for (i = 0; i < dev->num_deps; i++)
+		configfs_undepend_item_unlocked(dev->deps[i]);
+
+	kfree(dev->deps);
+	dev->deps = NULL;
+	dev->num_deps = 0;
+}
+
+static int gpio_sim_device_lockup_configfs(struct gpio_sim_device *dev)
 {
 	struct configfs_subsystem *subsys = dev->group.cg_subsys;
 	struct gpio_sim_bank *bank;
 	struct gpio_sim_line *line;
 	struct config_item *item;
+	unsigned int num = 0;
+	int ret;
+
+	lockdep_assert_held(&subsys->su_mutex);
+	lockdep_assert_held(&dev->lock);
 
 	/*
 	 * The device only needs to depend on leaf entries. This is
 	 * sufficient to lock up all the configfs entries that the
 	 * instantiated, alive device depends on.
+	 *
+	 * dev->lock keeps the bank and line lists stable and the subsystem
+	 * mutex keeps the entries' ci_group stable. An entry that is still
+	 * being created or removed cannot be depended on, in which case the
+	 * device is not brought up. The entries that were actually depended
+	 * on are recorded, so that exactly those are undepended when the
+	 * device goes down.
 	 */
+	list_for_each_entry(bank, &dev->bank_list, siblings)
+		list_for_each_entry(line, &bank->line_list, siblings)
+			num++;
+
+	if (!num)
+		return 0;
+
+	dev->deps = kzalloc_objs(*dev->deps, num);
+	if (!dev->deps)
+		return -ENOMEM;
+
 	list_for_each_entry(bank, &dev->bank_list, siblings) {
 		list_for_each_entry(line, &bank->line_list, siblings) {
 			item = line->hog ? &line->hog->item
 					 : &line->group.cg_item;
 
-			if (lock)
-				WARN_ON(configfs_depend_item_unlocked(subsys,
-								      item));
-			else
-				configfs_undepend_item_unlocked(item);
+			/* Already unlinked by a concurrent rmdir(). */
+			if (!item->ci_group) {
+				ret = -EBUSY;
+				goto err_unlock;
+			}
+
+			ret = configfs_depend_item_unlocked(subsys, item);
+			if (ret) {
+				ret = -EBUSY;
+				goto err_unlock;
+			}
+
+			dev->deps[dev->num_deps++] = item;
 		}
 	}
+
+	return 0;
+
+err_unlock:
+	gpio_sim_device_unlock_configfs(dev);
+	return ret;
 }
 
 static ssize_t
@@ -1013,26 +1067,32 @@ gpio_sim_device_config_live_store(struct config_item *item,
 	if (ret)
 		return ret;
 
-	if (live)
-		gpio_sim_device_lockup_configfs(dev, true);
-
-	scoped_guard(mutex, &dev->lock) {
-		if (live == gpio_sim_device_is_live(dev))
-			ret = -EPERM;
-		else if (live)
-			ret = gpio_sim_device_activate(dev);
-		else
-			gpio_sim_device_deactivate(dev);
-	}
-
 	/*
-	 * Undepend is required only if device disablement (live == 0)
-	 * succeeds or if device enablement (live == 1) fails.
+	 * Lock order: subsystem mutex, then dev->lock. This is the same
+	 * order as in the make_group()/make_item() callbacks.
 	 */
-	if (live == !!ret)
-		gpio_sim_device_lockup_configfs(dev, false);
+	guard(mutex)(&dev->group.cg_subsys->su_mutex);
+	guard(mutex)(&dev->lock);
+
+	if (live == gpio_sim_device_is_live(dev))
+		return -EPERM;
+
+	if (live) {
+		ret = gpio_sim_device_lockup_configfs(dev);
+		if (ret)
+			return ret;
 
-	return ret ?: count;
+		ret = gpio_sim_device_activate(dev);
+		if (ret) {
+			gpio_sim_device_unlock_configfs(dev);
+			return ret;
+		}
+	} else {
+		gpio_sim_device_deactivate(dev);
+		gpio_sim_device_unlock_configfs(dev);
+	}
+
+	return count;
 }
 
 CONFIGFS_ATTR(gpio_sim_device_config_, live);
@@ -1546,6 +1606,7 @@ static void gpio_sim_device_config_group_release(struct config_item *item)
 
 	mutex_destroy(&dev->lock);
 	ida_free(&gpio_sim_ida, dev->id);
+	kfree(dev->deps);
 	kfree(dev);
 }
 
-- 
2.43.0

Re: [PATCH v2] gpio: sim: fix the configfs lockup racing with mkdir/rmdir

[Bartosz Golaszewski]

On Tue, 27 Oct 2026 09:56:53 +0100, Yu Zhang <yuz08559@gmail.com> said:
> gpio_sim_device_lockup_configfs() depends (on activation) and undepends
> (on deactivation) the leaf configfs entries of a device. It walks
> dev->bank_list and bank->line_list without dev->lock, while the lists
> are modified and their nodes freed under dev->lock by the configfs
> make/release callbacks. It also walks the lists twice, once to depend
> and once to undepend, and the two walks can see different entries:
>
>  - A hog created while the device is live is never depended on, but
>    it is undepended on deactivation, hitting
>    BUG_ON(sd->s_dependent_count < 1) in configfs_undepend_item().
>    The following is enough to trigger it:
>
> 	mkdir $G/dev $G/dev/bank0 $G/dev/bank0/line0
> 	echo 1 > $G/dev/live
> 	mkdir $G/dev/bank0/line0/hog
> 	echo 0 > $G/dev/live
>
>  - An entry that is being created or removed when the device goes live
>    cannot be depended on (the WARN_ON() fires), but it is undepended
>    later, hitting the same BUG_ON(). If it has already been unlinked,
>    configfs_depend_item_unlocked() dereferences its NULL ci_group.
>
> Toggling "live" while concurrently creating and removing line and hog
> directories hits the WARN_ON() and then the BUG_ON() within a second.
>
> Hold the configfs subsystem mutex and dev->lock across the whole live
> transition. The subsystem mutex is what configfs holds when it links
> and unlinks entries and when it calls the make_group()/make_item()
> callbacks, which then take dev->lock, so the lock order is the same.
> Record the entries that were actually depended on and undepend exactly
> those. If an entry cannot be depended on because it is being created
> or removed, undo the dependencies already taken and fail with -EBUSY
> instead of warning.
>
> configfs_depend_item_unlocked() and configfs_undepend_item_unlocked()
> only take configfs_dirent_lock for an item of the caller's own
> subsystem, so calling them under these locks is fine.
>
> Fixes: 8bd76b3d3f3a ("gpio: sim: lock up configfs that an instantiated device depends on")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yu Zhang <yuz08559@gmail.com>
> ---

Is this in any way linked to your other patch[1] for gpio-sim? If so, and if
this one is in response to sashiko reviews, please indicate it by keeping the
series versioning, even if you changed the approach, otherwise, I can't know
if the other one should be dropped or not?

>  drivers/gpio/gpio-sim.c | 109 +++++++++++++++++++++++++++++++---------
>  1 file changed, 85 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
> index 0c73bec..ad4affc 100644
> --- a/drivers/gpio/gpio-sim.c
> +++ b/drivers/gpio/gpio-sim.c
> @@ -560,6 +560,10 @@ struct gpio_sim_device {
>  	struct mutex lock;
>
>  	struct list_head bank_list;
> +
> +	/* Leaf entries depended on while the device is live. */
> +	struct config_item **deps;
> +	unsigned int num_deps;
>  };
>
>  static struct gpio_sim_device *to_gpio_sim_device(struct config_item *item)
> @@ -974,31 +978,81 @@ static void gpio_sim_device_deactivate(struct gpio_sim_device *dev)
>  	gpio_sim_remove_swnode_recursive(swnode);
>  }
>
> -static void
> -gpio_sim_device_lockup_configfs(struct gpio_sim_device *dev, bool lock)
> +static void gpio_sim_device_unlock_configfs(struct gpio_sim_device *dev)
> +{
> +	unsigned int i;
> +
> +	lockdep_assert_held(&dev->lock);
> +
> +	for (i = 0; i < dev->num_deps; i++)
> +		configfs_undepend_item_unlocked(dev->deps[i]);
> +
> +	kfree(dev->deps);
> +	dev->deps = NULL;
> +	dev->num_deps = 0;
> +}
> +
> +static int gpio_sim_device_lockup_configfs(struct gpio_sim_device *dev)
>  {
>  	struct configfs_subsystem *subsys = dev->group.cg_subsys;
>  	struct gpio_sim_bank *bank;
>  	struct gpio_sim_line *line;
>  	struct config_item *item;
> +	unsigned int num = 0;
> +	int ret;
> +
> +	lockdep_assert_held(&subsys->su_mutex);
> +	lockdep_assert_held(&dev->lock);
>
>  	/*
>  	 * The device only needs to depend on leaf entries. This is
>  	 * sufficient to lock up all the configfs entries that the
>  	 * instantiated, alive device depends on.
> +	 *
> +	 * dev->lock keeps the bank and line lists stable and the subsystem
> +	 * mutex keeps the entries' ci_group stable. An entry that is still
> +	 * being created or removed cannot be depended on, in which case the
> +	 * device is not brought up. The entries that were actually depended
> +	 * on are recorded, so that exactly those are undepended when the
> +	 * device goes down.
>  	 */
> +	list_for_each_entry(bank, &dev->bank_list, siblings)
> +		list_for_each_entry(line, &bank->line_list, siblings)
> +			num++;

Maybe:
		num += list_count_nodes(&bank->line_list)

> +
> +	if (!num)
> +		return 0;
> +
> +	dev->deps = kzalloc_objs(*dev->deps, num);
> +	if (!dev->deps)
> +		return -ENOMEM;
> +
>  	list_for_each_entry(bank, &dev->bank_list, siblings) {
>  		list_for_each_entry(line, &bank->line_list, siblings) {
>  			item = line->hog ? &line->hog->item
>  					 : &line->group.cg_item;
>
> -			if (lock)
> -				WARN_ON(configfs_depend_item_unlocked(subsys,
> -								      item));
> -			else
> -				configfs_undepend_item_unlocked(item);
> +			/* Already unlinked by a concurrent rmdir(). */
> +			if (!item->ci_group) {
> +				ret = -EBUSY;
> +				goto err_unlock;
> +			}
> +
> +			ret = configfs_depend_item_unlocked(subsys, item);
> +			if (ret) {
> +				ret = -EBUSY;
> +				goto err_unlock;
> +			}
> +
> +			dev->deps[dev->num_deps++] = item;
>  		}
>  	}
> +
> +	return 0;
> +
> +err_unlock:
> +	gpio_sim_device_unlock_configfs(dev);
> +	return ret;
>  }
>
>  static ssize_t
> @@ -1013,26 +1067,32 @@ gpio_sim_device_config_live_store(struct config_item *item,
>  	if (ret)
>  		return ret;
>
> -	if (live)
> -		gpio_sim_device_lockup_configfs(dev, true);
> -
> -	scoped_guard(mutex, &dev->lock) {
> -		if (live == gpio_sim_device_is_live(dev))
> -			ret = -EPERM;
> -		else if (live)
> -			ret = gpio_sim_device_activate(dev);
> -		else
> -			gpio_sim_device_deactivate(dev);
> -	}
> -
>  	/*
> -	 * Undepend is required only if device disablement (live == 0)
> -	 * succeeds or if device enablement (live == 1) fails.
> +	 * Lock order: subsystem mutex, then dev->lock. This is the same
> +	 * order as in the make_group()/make_item() callbacks.
>  	 */
> -	if (live == !!ret)
> -		gpio_sim_device_lockup_configfs(dev, false);
> +	guard(mutex)(&dev->group.cg_subsys->su_mutex);
> +	guard(mutex)(&dev->lock);
> +
> +	if (live == gpio_sim_device_is_live(dev))
> +		return -EPERM;
> +
> +	if (live) {
> +		ret = gpio_sim_device_lockup_configfs(dev);
> +		if (ret)
> +			return ret;
>
> -	return ret ?: count;
> +		ret = gpio_sim_device_activate(dev);
> +		if (ret) {
> +			gpio_sim_device_unlock_configfs(dev);
> +			return ret;
> +		}
> +	} else {
> +		gpio_sim_device_deactivate(dev);
> +		gpio_sim_device_unlock_configfs(dev);
> +	}
> +
> +	return count;
>  }
>
>  CONFIGFS_ATTR(gpio_sim_device_config_, live);
> @@ -1546,6 +1606,7 @@ static void gpio_sim_device_config_group_release(struct config_item *item)
>
>  	mutex_destroy(&dev->lock);
>  	ida_free(&gpio_sim_ida, dev->id);
> +	kfree(dev->deps);
>  	kfree(dev);
>  }
>
> --
> 2.43.0
>
>

I'd prefer to avoid the allocation for deps but I don't have a better idea
because struct gpio_sim_line and struct gpio_sim_hog which are the ones that
can be leaves are quite different so I don't know how to generalize them.

Let's keep it for now.

Bart

[1] https://lore.kernel.org/all/20261026060049.60599-1-yuz08559@gmail.com/

Re: [PATCH v2] gpio: sim: fix the configfs lockup racing with mkdir/rmdir

[Yu Zhang]

On Wed, Oct 28, 2026 at 03:07:55AM -0700, Bartosz Golaszewski wrote:
> Is this in any way linked to your other patch[1] for gpio-sim? If so, and if
> this one is in response to sashiko reviews, please indicate it by keeping the
> series versioning, even if you changed the approach, otherwise, I can't know
> if the other one should be dropped or not?

Yes, v2 replaces [1], please drop it. Sashiko was right that v1 still
raced with mkdir/rmdir (it depended on the entries after dropping
dev->lock), so I redid the fix and changed the subject, but forgot to
put a changelog under the ---. Sorry about that.

> Maybe:
> 		num += list_count_nodes(&bank->line_list)

Done in v3, with the full history under the ---.

Thanks,
Yu

[PATCH v3] gpio: sim: fix the configfs lockup racing with mkdir/rmdir

[Yu Zhang]

gpio_sim_device_lockup_configfs() depends (on activation) and undepends
(on deactivation) the leaf configfs entries of a device. It walks
dev->bank_list and bank->line_list without dev->lock, while the lists
are modified and their nodes freed under dev->lock by the configfs
make/release callbacks. It also walks the lists twice, once to depend
and once to undepend, and the two walks can see different entries:

 - A hog created while the device is live is never depended on, but
   it is undepended on deactivation, hitting
   BUG_ON(sd->s_dependent_count < 1) in configfs_undepend_item().
   The following is enough to trigger it:

	mkdir $G/dev $G/dev/bank0 $G/dev/bank0/line0
	echo 1 > $G/dev/live
	mkdir $G/dev/bank0/line0/hog
	echo 0 > $G/dev/live

 - An entry that is being created or removed when the device goes live
   cannot be depended on (the WARN_ON() fires), but it is undepended
   later, hitting the same BUG_ON(). If it has already been unlinked,
   configfs_depend_item_unlocked() dereferences its NULL ci_group.

Toggling "live" while concurrently creating and removing line and hog
directories hits the WARN_ON() and then the BUG_ON() within a second.

Hold the configfs subsystem mutex and dev->lock across the whole live
transition. The subsystem mutex is what configfs holds when it links
and unlinks entries and when it calls the make_group()/make_item()
callbacks, which then take dev->lock, so the lock order is the same.
Record the entries that were actually depended on and undepend exactly
those. If an entry cannot be depended on because it is being created
or removed, undo the dependencies already taken and fail with -EBUSY
instead of warning.

configfs_depend_item_unlocked() and configfs_undepend_item_unlocked()
only take configfs_dirent_lock for an item of the caller's own
subsystem, so calling them under these locks is fine.

Fixes: 8bd76b3d3f3a ("gpio: sim: lock up configfs that an instantiated device depends on")
Cc: stable@vger.kernel.org
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
v3:
 - Count the lines with list_count_nodes() (Bartosz).
v2: https://lore.kernel.org/all/20261027085653.90471-1-yuz08559@gmail.com/
 - Supersedes v1 "gpio: sim: snapshot configfs leaf items before
   depending on them". The v1 approach of taking references on the
   entries and depending on them after dropping dev->lock still raced
   with mkdir/rmdir, as the sashiko review pointed out, so the fix
   was redone: hold the subsystem mutex and dev->lock across the whole
   live transition, record the entries that were depended on and fail
   with -EBUSY instead of warning.
v1: https://lore.kernel.org/all/20261026060049.60599-1-yuz08559@gmail.com/
---
 drivers/gpio/gpio-sim.c | 108 +++++++++++++++++++++++++++++++---------
 1 file changed, 84 insertions(+), 24 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 0c73bec..abc73bb 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -560,6 +560,10 @@ struct gpio_sim_device {
 	struct mutex lock;
 
 	struct list_head bank_list;
+
+	/* Leaf entries depended on while the device is live. */
+	struct config_item **deps;
+	unsigned int num_deps;
 };
 
 static struct gpio_sim_device *to_gpio_sim_device(struct config_item *item)
@@ -974,31 +978,80 @@ static void gpio_sim_device_deactivate(struct gpio_sim_device *dev)
 	gpio_sim_remove_swnode_recursive(swnode);
 }
 
-static void
-gpio_sim_device_lockup_configfs(struct gpio_sim_device *dev, bool lock)
+static void gpio_sim_device_unlock_configfs(struct gpio_sim_device *dev)
+{
+	unsigned int i;
+
+	lockdep_assert_held(&dev->lock);
+
+	for (i = 0; i < dev->num_deps; i++)
+		configfs_undepend_item_unlocked(dev->deps[i]);
+
+	kfree(dev->deps);
+	dev->deps = NULL;
+	dev->num_deps = 0;
+}
+
+static int gpio_sim_device_lockup_configfs(struct gpio_sim_device *dev)
 {
 	struct configfs_subsystem *subsys = dev->group.cg_subsys;
 	struct gpio_sim_bank *bank;
 	struct gpio_sim_line *line;
 	struct config_item *item;
+	unsigned int num = 0;
+	int ret;
+
+	lockdep_assert_held(&subsys->su_mutex);
+	lockdep_assert_held(&dev->lock);
 
 	/*
 	 * The device only needs to depend on leaf entries. This is
 	 * sufficient to lock up all the configfs entries that the
 	 * instantiated, alive device depends on.
+	 *
+	 * dev->lock keeps the bank and line lists stable and the subsystem
+	 * mutex keeps the entries' ci_group stable. An entry that is still
+	 * being created or removed cannot be depended on, in which case the
+	 * device is not brought up. The entries that were actually depended
+	 * on are recorded, so that exactly those are undepended when the
+	 * device goes down.
 	 */
+	list_for_each_entry(bank, &dev->bank_list, siblings)
+		num += list_count_nodes(&bank->line_list);
+
+	if (!num)
+		return 0;
+
+	dev->deps = kzalloc_objs(*dev->deps, num);
+	if (!dev->deps)
+		return -ENOMEM;
+
 	list_for_each_entry(bank, &dev->bank_list, siblings) {
 		list_for_each_entry(line, &bank->line_list, siblings) {
 			item = line->hog ? &line->hog->item
 					 : &line->group.cg_item;
 
-			if (lock)
-				WARN_ON(configfs_depend_item_unlocked(subsys,
-								      item));
-			else
-				configfs_undepend_item_unlocked(item);
+			/* Already unlinked by a concurrent rmdir(). */
+			if (!item->ci_group) {
+				ret = -EBUSY;
+				goto err_unlock;
+			}
+
+			ret = configfs_depend_item_unlocked(subsys, item);
+			if (ret) {
+				ret = -EBUSY;
+				goto err_unlock;
+			}
+
+			dev->deps[dev->num_deps++] = item;
 		}
 	}
+
+	return 0;
+
+err_unlock:
+	gpio_sim_device_unlock_configfs(dev);
+	return ret;
 }
 
 static ssize_t
@@ -1013,26 +1066,32 @@ gpio_sim_device_config_live_store(struct config_item *item,
 	if (ret)
 		return ret;
 
-	if (live)
-		gpio_sim_device_lockup_configfs(dev, true);
-
-	scoped_guard(mutex, &dev->lock) {
-		if (live == gpio_sim_device_is_live(dev))
-			ret = -EPERM;
-		else if (live)
-			ret = gpio_sim_device_activate(dev);
-		else
-			gpio_sim_device_deactivate(dev);
-	}
-
 	/*
-	 * Undepend is required only if device disablement (live == 0)
-	 * succeeds or if device enablement (live == 1) fails.
+	 * Lock order: subsystem mutex, then dev->lock. This is the same
+	 * order as in the make_group()/make_item() callbacks.
 	 */
-	if (live == !!ret)
-		gpio_sim_device_lockup_configfs(dev, false);
+	guard(mutex)(&dev->group.cg_subsys->su_mutex);
+	guard(mutex)(&dev->lock);
+
+	if (live == gpio_sim_device_is_live(dev))
+		return -EPERM;
+
+	if (live) {
+		ret = gpio_sim_device_lockup_configfs(dev);
+		if (ret)
+			return ret;
 
-	return ret ?: count;
+		ret = gpio_sim_device_activate(dev);
+		if (ret) {
+			gpio_sim_device_unlock_configfs(dev);
+			return ret;
+		}
+	} else {
+		gpio_sim_device_deactivate(dev);
+		gpio_sim_device_unlock_configfs(dev);
+	}
+
+	return count;
 }
 
 CONFIGFS_ATTR(gpio_sim_device_config_, live);
@@ -1546,6 +1605,7 @@ static void gpio_sim_device_config_group_release(struct config_item *item)
 
 	mutex_destroy(&dev->lock);
 	ida_free(&gpio_sim_ida, dev->id);
+	kfree(dev->deps);
 	kfree(dev);
 }
 
-- 
2.43.0

Re: [PATCH v3] gpio: sim: fix the configfs lockup racing with mkdir/rmdir

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] gpio: sim: missing leaf bank dependency in gpio_sim_device_lockup_configfs()
--

commit e531f54ece1332b506f6748423ca9ef1c7ee7f17
Author: Yu Zhang <yuz08559@gmail.com>

gpio: sim: fix the configfs lockup racing with mkdir/rmdir

This commit fixes races between activating/deactivating a simulated GPIO
device and concurrent configfs mkdir/rmdir operations by holding the configfs
subsystem mutex and dev->lock across live transitions and recording the
depended leaf items.

> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
> index 34b45d0d35750..041bbdc5187b6 100644
> --- a/drivers/gpio/gpio-sim.c
> +++ b/drivers/gpio/gpio-sim.c

[ ... ]

> @@ -974,31 +978,80 @@ static void gpio_sim_device_deactivate(struct gpio_sim_device *dev)
>  	gpio_sim_remove_swnode_recursive(swnode);
>  }
>  
> -static void
> -gpio_sim_device_lockup_configfs(struct gpio_sim_device *dev, bool lock)
> +static void gpio_sim_device_unlock_configfs(struct gpio_sim_device *dev)
> +{
> +	unsigned int i;
> +
> +	lockdep_assert_held(&dev->lock);
> +
> +	for (i = 0; i < dev->num_deps; i++)
> +		configfs_undepend_item_unlocked(dev->deps[i]);
> +
> +	kfree(dev->deps);
> +	dev->deps = NULL;
> +	dev->num_deps = 0;
> +}
> +
> +static int gpio_sim_device_lockup_configfs(struct gpio_sim_device *dev)
>  {
>  	struct configfs_subsystem *subsys = dev->group.cg_subsys;
>  	struct gpio_sim_bank *bank;
>  	struct gpio_sim_line *line;
>  	struct config_item *item;
> +	unsigned int num = 0;
> +	int ret;
> +
> +	lockdep_assert_held(&subsys->su_mutex);
> +	lockdep_assert_held(&dev->lock);
>  
>  	/*
>  	 * The device only needs to depend on leaf entries. This is
>  	 * sufficient to lock up all the configfs entries that the
>  	 * instantiated, alive device depends on.
> +	 *
> +	 * dev->lock keeps the bank and line lists stable and the subsystem
> +	 * mutex keeps the entries' ci_group stable. An entry that is still
> +	 * being created or removed cannot be depended on, in which case the
> +	 * device is not brought up. The entries that were actually depended
> +	 * on are recorded, so that exactly those are undepended when the
> +	 * device goes down.
>  	 */
> +	list_for_each_entry(bank, &dev->bank_list, siblings)
> +		num += list_count_nodes(&bank->line_list);
> +
> +	if (!num)
> +		return 0;
> +
> +	dev->deps = kzalloc_objs(*dev->deps, num);
> +	if (!dev->deps)
> +		return -ENOMEM;
> +
>  	list_for_each_entry(bank, &dev->bank_list, siblings) {
>  		list_for_each_entry(line, &bank->line_list, siblings) {

[Severity: Medium]
What happens in gpio_sim_device_lockup_configfs() when a bank has no line
subdirectories and bank->line_list is empty?

In gpio_sim_device_config_make_bank_group(), a bank is initialized with
bank->num_lines = 1 and an empty bank->line_list, so creating line<offset>
subdirectories is optional. When bank->line_list is empty,
&bank->group.cg_item is itself the leaf configfs entry for that bank.

Because gpio_sim_device_lockup_configfs() only counts and iterates over
bank->line_list, any bank with an empty line_list is skipped:

1. If a bank has no line subdirectories when the device is activated,
   &bank->group.cg_item is never depended on. While the device is live,
   userspace can rmdir the bank directory (causing
   gpio_sim_bank_config_group_release() to unlink and free struct
   gpio_sim_bank) and even rmdir the live device directory itself.

2. A bank directory in the middle of configfs_mkdir() or configfs_rmdir()
   also has an empty bank->line_list. During configfs_rmdir(), for example,
   subsys->su_mutex is unlocked before config_item_put() invokes
   gpio_sim_bank_config_group_release() to remove bank->siblings from
   dev->bank_list under dev->lock:

   fs/configfs/dir.c:configfs_rmdir() {
       ...
       unlink_group(group);
       client_drop_item(parent_item, item);
       mutex_unlock(&subsys->su_mutex);

       /* Drop our reference from above */
       config_item_put(item);
   }

   If gpio_sim_device_config_live_store() acquires subsys->su_mutex and
   dev->lock in that window (or the similar window in configfs_mkdir() before
   configfs_attach_group() finishes), gpio_sim_device_lockup_configfs() skips
   the empty bank instead of checking &bank->group.cg_item and returning
   -EBUSY, allowing gpio_sim_device_activate() to use a bank that is still
   being created or is about to be freed.

Should gpio_sim_device_lockup_configfs() count and depend on
&bank->group.cg_item whenever list_empty(&bank->line_list) is true?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031085821.23385-1-yuz08559@gmail.com?part=1