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

[sashiko-bot@kernel.org]

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