Re: [PATCH v3 2/2] Fixes: gpiolib: acpi: resource leak in OpRegion

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, May 20, 2026 at 09:45:47AM +0200, Marco Scardovi (scardracs) wrote:
> If acpi_remove_address_space_handler() fails, the cleanup function
> acpi_gpiochip_free_regions() previously returned early. This leaks
> the connections list and all requested GPIO descriptors.
> 
> Similarly, if acpi_gpio_adr_space_handler() fails to allocate a connection
> or request a GPIO descriptor during a multi-pin transaction, it exits
> without freeing the descriptors and connections that were already allocated
> in the same call.
> 
> To avoid leaks, introduce a localized new_conns list inside the handler to
> track the new connections requested during the current transaction. On
> error, roll back only the connections in the new_conns list (avoiding
> deadlock on the non-recursive conn_lock and preventing over-cleanup of
> historic connections). On success, splice the new_conns list into the
> global achip->conns list under the conn_lock.
> 
> Additionally, rename the global connection cleanup function to
> acpi_gpiochip_free_all_connections() and call it in the GPIO chip teardown
> path.

...

> +static void acpi_gpiochip_free_connection_list(struct list_head *list)
> +{
> +	struct acpi_gpio_connection *conn;
> +	struct acpi_gpio_connection *tmp;

Make it a single line.

> +	list_for_each_entry_safe_reverse(conn, tmp, list, node) {
> +		gpiochip_free_own_desc(conn->desc);
> +		list_del(&conn->node);
> +		kfree(conn);
> +	}
> +}

...

> +static struct acpi_gpio_connection *
> +acpi_gpiochip_find_conn(struct acpi_gpio_chip *achip,
> +			struct list_head *new_conns, unsigned int pin)

We allow longer lines, so

static struct acpi_gpio_connection *
acpi_gpiochip_find_conn(struct acpi_gpio_chip *achip, struct list_head *new_conns,
			unsigned int pin)

is fine

> +{
> +	struct acpi_gpio_connection *conn;
> +
> +	list_for_each_entry(conn, &achip->conns, node) {
> +		if (conn->pin == pin)
> +			return conn;
> +	}

> +	if (new_conns) {

Is it ever called with new_conns == NULL?

> +		list_for_each_entry(conn, new_conns, node) {
> +			if (conn->pin == pin)
> +				return conn;
> +		}
> +	}
> +
> +	return NULL;
> +}

...

Before fixing the issue, introduce helpers and use them in the existing code
first. Then in the follow up fix the issue.

...

>  		struct gpio_desc *desc;
> -		u16 word, shift;
> -		bool found;
> +		bool found = false;

Do you still need it? Wouldn't 'conn == NULL' be equal to 'false'?

...

-- 
With Best Regards,
Andy Shevchenko