Re: [PATCH v6 5/5] gpio: shared: convert to using software nodes for GPIO shared proxy lookup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: shared: false-positive NULL fwnode match in gpio_shared_get_proxy()
- [High] gpio: shared: unlocked proxy teardown and NULL fwnode match in lookup
--

commit c9587b1290500bb02f98bc321f73e9b020833797
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

gpio: shared: convert to using software nodes for GPIO shared proxy lookup

This commit replaces platform lookup tables with software nodes for shared
GPIO proxy devices so that shared GPIOs can be looked up via their proxy
device firmware node in gpio_shared_get_proxy().

> diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
> index 3ed25609cae10..71f16105f7a5c 100644
> --- a/drivers/gpio/gpiolib-shared.c
> +++ b/drivers/gpio/gpiolib-shared.c

[ ... ]

> @@ -457,23 +483,19 @@ static bool gpio_shared_dev_is_reset_gpio(struct device *consumer,
>  }
>  #endif /* CONFIG_RESET_GPIO */
>  
> -int gpio_shared_add_proxy_lookup(struct device *consumer, struct fwnode_handle *fwnode,
> -				 const char *con_id, unsigned long lflags)
> +struct gpio_desc *
> +gpio_shared_get_proxy(struct device *consumer, struct fwnode_handle *fwnode,
> +		      const char *con_id)
>  {
>  	struct gpio_shared_entry *entry;
>  	struct gpio_shared_ref *ref;
> -	const char *dev_id;
> -
> -	if (!consumer)
> -		return -EOPNOTSUPP;
> -
> -	dev_id = dev_name(consumer);
>  
>  	list_for_each_entry(entry, &gpio_shared_list, list) {
>  		list_for_each_entry(ref, &entry->refs, list) {
>  			guard(mutex)(&ref->lock);
>  
> -			if (!ref->fwnode && device_is_compatible(consumer, "reset-gpio")) {
> +			if (!ref->fwnode && consumer &&
> +			    device_is_compatible(consumer, "reset-gpio")) {
>  				if (!gpio_shared_dev_is_reset_gpio(consumer, entry, ref))
>  					continue;
>  			} else if (fwnode != ref->fwnode) {
> @@ -484,49 +506,31 @@ int gpio_shared_add_proxy_lookup(struct device *consumer, struct fwnode_handle *
>  			    (con_id && ref->con_id && strcmp(con_id, ref->con_id) != 0))
>  				continue;
>  
> -			/* We've already done that on a previous request. */
> -			if (ref->lookup)
> -				return 0;
> -
> -			char *key __free(kfree) =
> -				kasprintf(GFP_KERNEL,
> -					  KBUILD_MODNAME ".proxy.%u",
> -					  ref->proxy_dev->id);
> -			if (!key)
> -				return -ENOMEM;
> -
> -			struct gpiod_lookup_table *lookup __free(kfree) =
> -				kzalloc_flex(*lookup, table, 2);
> -			if (!lookup)
> -				return -ENOMEM;
> -
> -			pr_debug("Adding machine lookup entry for a shared GPIO for consumer %s, with key '%s' and con_id '%s'\n",
> -				 dev_id, key, ref->con_id ?: "none");
> -
> -			lookup->dev_id = kstrdup(dev_id, GFP_KERNEL);
> -			if (!lookup->dev_id)
> -				return -ENOMEM;
> -
> -			lookup->table[0] = GPIO_LOOKUP(no_free_ptr(key), 0,
> -						       ref->con_id, lflags);
> -
> -			ref->lookup = no_free_ptr(lookup);
> -			gpiod_add_lookup_table(ref->lookup);
> +			struct gpio_device *gdev __free(gpio_device_put) =
> +					gpio_device_find_by_fwnode(ref->proxy_fwnode);
> +			if (!gdev)
> +				/*
> +				 * A matching entry is there but the associated
> +				 * controller has not been registered yet.
> +				 */
> +				return ERR_PTR(-EPROBE_DEFER);
>  
> -			return 0;
> +			return gpio_device_get_desc(gdev, 0);

[Severity: High]
What happens in gpio_shared_get_proxy() if ref->proxy_fwnode is null when
passed to gpio_device_find_by_fwnode()?

In drivers/gpio/gpiolib.c, gpio_device_find_by_fwnode() uses
gpio_chip_match_by_fwnode():

static int gpio_chip_match_by_fwnode(struct gpio_chip *gc, const void *fwnode)
{
	struct device *dev = &gc->gpiodev->dev;
	struct fwnode_handle *node = dev_fwnode(dev);

	if (IS_ERR(fwnode))
		return 0;

	if (device_match_fwnode(dev, fwnode))
		return 1;

	return node && node->secondary == fwnode;
}

When fwnode is null, both IS_ERR(fwnode) and device_match_fwnode(dev, fwnode)
return 0, so the final check evaluates to:

	node && node->secondary == NULL

This matches the first registered gpio_device that has a primary fwnode and
no secondary fwnode, causing gpio_shared_get_proxy() to return descriptor 0
of an unrelated controller instead of returning ERR_PTR(-EPROBE_DEFER).

Because gpio_shared_get_proxy() scans gpio_shared_list matching only
(consumer, fwnode, con_id) without checking that the entry matches the
descriptor or index returned by gpiod_fwnode_lookup(), this can be triggered
when a consumer has a multi-element GPIO property scanned by
gpio_shared_of_traverse(): if the second controller is registered while the
first controller in gpio_shared_list is not yet registered, requesting the
second GPIO matches the first entry where ref->proxy_fwnode is still null.

It can also happen if gpiod_find_and_request() resolves desc in
gpiod_fwnode_lookup() under gpio_devices_srcu while
gpio_device_teardown_shared() -> gpio_shared_remove_proxy_dev() clears
ref->proxy_fwnode.

Should gpio_shared_get_proxy() verify that the entry matches the looked-up
descriptor (or index) and check that ref->proxy_fwnode is non-null before
calling gpio_device_find_by_fwnode()?

>  		}
>  	}
>  
>  	/* We warn here because this can only happen if the programmer borked. */
>  	WARN_ON(1);
> -	return -ENOENT;
> +	return ERR_PTR(-ENOENT);
>  }
>  
>  static void gpio_shared_remove_proxy_dev(struct gpio_shared_ref *ref)
>  {
>  	auxiliary_device_delete(ref->proxy_dev);
>  	auxiliary_device_uninit(ref->proxy_dev);
> +	software_node_unregister(&ref->proxy_swnode);
>  	ref->proxy_dev = NULL;
> +	ref->proxy_fwnode = NULL;
>  }

[Severity: High]
Could gpio_shared_remove_proxy_dev() race with concurrent readers of
ref->proxy_dev and ref->proxy_fwnode?

In struct gpio_shared_ref, ref->lock is documented as protecting proxy_dev
and proxy_fwnode, and both gpio_shared_make_proxy_dev() and
gpio_shared_get_proxy() acquire ref->lock.

However, gpio_shared_remove_proxy_dev() does not acquire ref->lock, and it
destroys ref->proxy_dev and unregisters ref->proxy_swnode (freeing the
backing swnode for ref->proxy_fwnode) before clearing ref->proxy_dev and
ref->proxy_fwnode.

Similarly, gpiochip_setup_shared() reads ref->proxy_dev without holding
ref->lock:

	list_for_each_entry(entry, &gpio_shared_list, list) {
		list_for_each_entry(ref, &entry->refs, list) {
			if (ref->proxy_dev && gdev->dev.parent == &ref->proxy_dev->dev) {

This appears reachable in several ways:

1. In drivers/gpio/gpiolib.c:gpiochip_add_data_with_key(), if
gpiochip_setup_dev() fails after gcdev_register() has registered gdev->dev,
err_teardown_shared runs gpio_device_teardown_shared() ->
gpio_shared_remove_proxy_dev() before err_remove_from_list removes gdev from
gpio_devices and calls synchronize_srcu(&gpio_devices_srcu):

	if (gpiolib_initialized) {
		ret = gpiochip_setup_dev(gc);
		if (ret)
			goto err_teardown_shared;
	}

	return 0;

err_teardown_shared:
	gpio_device_teardown_shared(gdev);
...
err_remove_from_list:
	scoped_guard(mutex, &gpio_devices_lock)
		list_del_rcu(&gdev->list);
	synchronize_srcu(&gpio_devices_srcu);

2. A consumer with multiple shared GPIOs under the same con_id can call
gpio_shared_get_proxy() and visit an entry whose controller is concurrently
being torn down in gpiochip_remove().

3. Another GPIO chip can concurrently run gpiochip_setup_shared() while a
shared GPIO controller is being torn down or unwinding a failed setup.

Should gpio_shared_remove_proxy_dev() and gpiochip_setup_shared() guard
accesses to ref->proxy_dev and ref->proxy_fwnode with ref->lock, and clear
the pointers before releasing the underlying objects?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-gpio-shared-fwnode-lookup-v6-0-6af7d8eefd25@oss.qualcomm.com?part=5