Re: [PATCH v5 1/2] gpio: shared: detach the proxy device from struct gpio_shared_ref

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] gpio: shared: leak of proxy_dev and IDA ID on partial setup failure in gpiochip_setup_shared()
--

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

gpio: shared: detach the proxy device from struct gpio_shared_ref

This commit moves the auxiliary device structure out of struct
gpio_shared_ref and allocates it separately when setting up the GPIO shared
proxy device, while also moving the IDA allocation and release to match the
proxy device lifecycle.

> diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
> index 8c9ac9d933049..c7893a5ac80a6 100644
> --- a/drivers/gpio/gpiolib-shared.c
> +++ b/drivers/gpio/gpiolib-shared.c

[ ... ]

> @@ -306,31 +301,43 @@ static int gpio_shared_of_scan(void)
>  }
>  #endif /* CONFIG_OF */
>  
> -static void gpio_shared_adev_release(struct device *dev)
> +static void gpio_shared_proxy_dev_release(struct device *dev)
>  {
> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
>  
> +	ida_free(&gpio_shared_ida, adev->id);
> +	kfree(adev);
>  }
>  
> -static int gpio_shared_make_adev(struct gpio_device *gdev,
> -				 struct gpio_shared_entry *entry,
> -				 struct gpio_shared_ref *ref)
> +static int gpio_shared_make_proxy_dev(struct gpio_device *gdev,
> +				      struct gpio_shared_entry *entry,
> +				      struct gpio_shared_ref *ref)
>  {
> -	struct auxiliary_device *adev = &ref->adev;
> -	int ret;
> +	struct auxiliary_device *adev;
> +	int ret, id;
>  
> -	guard(mutex)(&ref->lock);
> +	adev = kzalloc_obj(*adev);
> +	if (!adev)
> +		return -ENOMEM;
>  
> -	memset(adev, 0, sizeof(*adev));
> +	id = ida_alloc(&gpio_shared_ida, GFP_KERNEL);
> +	if (id < 0) {
> +		kfree(adev);
> +		return id;
> +	}
>  
> -	adev->id = ref->dev_id;
> +	adev->id = id;
>  	adev->name = "proxy";
>  	adev->dev.parent = gdev->dev.parent;
>  	adev->dev.platform_data = entry;
> -	adev->dev.release = gpio_shared_adev_release;
> +	adev->dev.release = gpio_shared_proxy_dev_release;
>  
>  	ret = auxiliary_device_init(adev);
> -	if (ret)
> +	if (ret) {
> +		ida_free(&gpio_shared_ida, id);
> +		kfree(adev);
>  		return ret;
> +	}
>  
>  	ret = auxiliary_device_add(adev);
>  	if (ret) {
> @@ -341,6 +348,10 @@ static int gpio_shared_make_adev(struct gpio_device *gdev,
>  	pr_debug("Created an auxiliary GPIO proxy %s for GPIO device %s\n",
>  		 dev_name(&adev->dev), gpio_device_get_label(gdev));
>  
> +	scoped_guard(mutex, &ref->lock) {
> +		ref->proxy_dev = adev;
> +	}
> +
>  	return 0;
>  }

[ ... ]

> @@ -592,7 +604,7 @@ int gpiochip_setup_shared(struct gpio_chip *gc)
>  				 fwnode_get_name(ref->fwnode) ?: "(no fwnode)",
>  				 ref->con_id ?: "(none)");
>  
> -			ret = gpio_shared_make_adev(gdev, entry, ref);
> +			ret = gpio_shared_make_proxy_dev(gdev, entry, ref);
>  			if (ret) {
>  				gpiod_free_commit(desc);
>  				return ret;

[Severity: Medium]
Can this error path leak ref->proxy_dev and its gpio_shared_ida ID when a
subsequent iteration fails?

In gpiochip_setup_shared(), each shared entry has at least two references in
entry->refs:

drivers/gpio/gpiolib-shared.c:gpiochip_setup_shared() {
    ...
		if (list_count_nodes(&entry->refs) <= 1)
			continue;
    ...
		list_for_each_entry(ref, &entry->refs, list) {
			...
			ret = gpio_shared_make_proxy_dev(gdev, entry, ref);
			if (ret) {
				gpiod_free_commit(desc);
				return ret;
			}
		}
}

If gpio_shared_make_proxy_dev() succeeds for the first ref in entry->refs (or
for a previous shared entry on the same chip) and then fails on a later ref,
gpiochip_setup_shared() only calls gpiod_free_commit(desc) and returns the
error.

In the caller gpiochip_add_data_with_key() in drivers/gpio/gpiolib.c:

	ret = gpiochip_setup_shared(gc);
	if (ret)
		goto err_remove_irqchip;

this bypasses gpio_device_teardown_shared(), leaving the already-created
ref->proxy_dev auxiliary devices registered on the auxiliary bus and leaking
their dynamically allocatedadev structures and gpio_shared_ida IDs (as well as
subsequently overwriting ref->proxy_dev if the GPIO controller re-probes).

[ ... ]

>  			}
>  		}
>  	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-gpio-shared-fwnode-lookup-v5-0-f4a2dfc059d7@oss.qualcomm.com?part=1