[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

[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