Re: [PATCH v4 2/4] gpiolib: acpi: fix resource leak in OpRegion

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Sun, May 31, 2026 at 02:03:09PM +0200, Marco Scardovi wrote:
> From: "Marco Scardovi (scardracs)" <mscardovi95@gmail.com>
> 
> 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.
> 
> Fixes: 473ed7be0da0 ("gpio / ACPI: Add support for ACPI GPIO operation regions")
> Assisted-by: Antigravity:gemini-3.5-flash
> Signed-off-by: Marco Scardovi <mscardovi95@gmail.com>
> ---
>  drivers/gpio/gpiolib-acpi-core.c | 142 ++++++++++++++++++++++++-------
>  1 file changed, 111 insertions(+), 31 deletions(-)

This is really complex solution to a problem I'm not sure that even exists.

> diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
> index a6d78dad299e..e3bc4677b51d 100644
> --- a/drivers/gpio/gpiolib-acpi-core.c
> +++ b/drivers/gpio/gpiolib-acpi-core.c
> @@ -1094,6 +1094,51 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id,
>  }
>  EXPORT_SYMBOL_GPL(acpi_dev_gpio_irq_wake_get_by);
>  
> +static void acpi_gpiochip_free_connection_list(struct list_head *list)
> +{
> +	struct acpi_gpio_connection *conn;
> +	struct acpi_gpio_connection *tmp;
> +
> +	list_for_each_entry_safe_reverse(conn, tmp, list, node) {
> +		gpiochip_free_own_desc(conn->desc);
> +		list_del(&conn->node);
> +		kfree(conn);
> +	}
> +}
> +
> +static void acpi_gpiochip_free_all_connections(struct acpi_gpio_chip *achip)
> +{
> +	guard(mutex)(&achip->conn_lock);
> +
> +	acpi_gpiochip_free_connection_list(&achip->conns);
> +}
> +
> +/*
> + * Find a connection in the global or local list.
> + * The caller must hold achip->conn_lock to protect the global list.
> + * The local list new_conns is private to the calling thread.
> + */
> +static struct acpi_gpio_connection *
> +acpi_gpiochip_find_conn(struct acpi_gpio_chip *achip,
> +			struct list_head *new_conns, unsigned int pin)
> +{
> +	struct acpi_gpio_connection *conn;
> +
> +	list_for_each_entry(conn, &achip->conns, node) {
> +		if (conn->pin == pin)
> +			return conn;
> +	}
> +
> +	if (new_conns) {
> +		list_for_each_entry(conn, new_conns, node) {
> +			if (conn->pin == pin)
> +				return conn;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
>  static acpi_status
>  acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  			    u32 bits, u64 *value, void *handler_context,
> @@ -1101,11 +1146,16 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  {
>  	struct acpi_gpio_chip *achip = region_context;
>  	struct gpio_chip *chip = achip->chip;
> +	struct acpi_gpio_connection *other;
> +	struct acpi_gpio_connection *conn;
>  	struct acpi_resource_gpio *agpio;
>  	struct acpi_resource *ares;
>  	u16 pin_index = address;
> +	LIST_HEAD(new_conns);
>  	acpi_status status;
>  	int length;
> +	u16 shift;
> +	u16 word;
>  	int i;
>  
>  	status = acpi_buffer_to_resource(achip->conn_info.connection,
> @@ -1129,20 +1179,15 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  	length = min(agpio->pin_table_length, pin_index + bits);
>  	for (i = pin_index; i < length; ++i) {
>  		unsigned int pin = agpio->pin_table[i];
> -		struct acpi_gpio_connection *conn;
>  		struct gpio_desc *desc;
> -		u16 word, shift;
> -		bool found;
> +		bool found = false;
>  
>  		mutex_lock(&achip->conn_lock);
>  
> -		found = false;
> -		list_for_each_entry(conn, &achip->conns, node) {
> -			if (conn->pin == pin) {
> -				found = true;
> -				desc = conn->desc;
> -				break;
> -			}
> +		conn = acpi_gpiochip_find_conn(achip, &new_conns, pin);
> +		if (conn) {
> +			desc = conn->desc;
> +			found = true;
>  		}
>  
>  		/*
> @@ -1163,34 +1208,66 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  			}
>  		}
>  
> +		mutex_unlock(&achip->conn_lock);
> +
>  		if (!found) {
>  			desc = acpi_request_own_gpiod(chip, agpio, i, "ACPI:OpRegion");

If any of the below fails we clean up the descriptor while the prior are
already in the list. Once the GPIO chip is released the list is released as
well.

> -				mutex_unlock(&achip->conn_lock);
> +				/*
> +				 * In case of concurrent handler execution, another
> +				 * thread may have already requested the same GPIO,
> +				 * causing acpi_request_own_gpiod() to fail with -EBUSY.
> +				 * Re-check the visible connection lists and reuse an
> +				 * existing connection if one is found.
> +				 */
> +				if (PTR_ERR(desc) == -EBUSY) {
> +					mutex_lock(&achip->conn_lock);
> +					conn = acpi_gpiochip_find_conn(achip, &new_conns, pin);
> +					if (conn) {
> +						desc = conn->desc;
> +						found = true;
> +					}
> +					mutex_unlock(&achip->conn_lock);
> +					if (found)
> +						goto use_existing;
> +				}
>  				status = AE_ERROR;
> -				goto out;
> +				goto err_free_new_conns;
>  			}
>  
>  			conn = kzalloc_obj(*conn);
>  			if (!conn) {
>  				gpiochip_free_own_desc(desc);
> -				mutex_unlock(&achip->conn_lock);
>  				status = AE_NO_MEMORY;
> -				goto out;
> +				goto err_free_new_conns;
>  			}
>  
>  			conn->pin = pin;
>  			conn->desc = desc;
> -			list_add_tail(&conn->node, &achip->conns);
> -		}
>  
> -		mutex_unlock(&achip->conn_lock);
> +			mutex_lock(&achip->conn_lock);
> +			/*
> +			 * Re-check both lists again before adding to local
> +			 * new_conns, as another thread could have completed
> +			 * and spliced its list while conn_lock was released.
> +			 */
> +			other = acpi_gpiochip_find_conn(achip, &new_conns, pin);
> +			if (other) {
> +				struct gpio_desc *desc_to_free = conn->desc;
> +				struct acpi_gpio_connection *c_to_free = conn;
>  
> -		/*
> -		 * For the cases when OperationRegion() consists of more than
> -		 * 64 bits calculate the word and bit shift to use that one to
> -		 * access the value.
> -		 */
> +				desc = other->desc;
> +				mutex_unlock(&achip->conn_lock);
> +
> +				gpiochip_free_own_desc(desc_to_free);
> +				kfree(c_to_free);
> +			} else {
> +				list_add_tail(&conn->node, &new_conns);
> +				mutex_unlock(&achip->conn_lock);
> +			}
> +		}
> +
> +use_existing:
>  		word = i / 64;
>  		shift = i % 64;
>  
> @@ -1204,9 +1281,19 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  		}
>  	}
>  
> +	mutex_lock(&achip->conn_lock);
> +	list_splice_tail(&new_conns, &achip->conns);
> +	mutex_unlock(&achip->conn_lock);
> +
> +	status = AE_OK;
> +
>  out:
>  	ACPI_FREE(ares);
>  	return status;
> +
> +err_free_new_conns:
> +	acpi_gpiochip_free_connection_list(&new_conns);
> +	goto out;
>  }
>  
>  static void acpi_gpiochip_request_regions(struct acpi_gpio_chip *achip)
> @@ -1229,22 +1316,15 @@ static void acpi_gpiochip_free_regions(struct acpi_gpio_chip *achip)
>  {
>  	struct gpio_chip *chip = achip->chip;
>  	acpi_handle handle = ACPI_HANDLE(chip->parent);
> -	struct acpi_gpio_connection *conn, *tmp;
>  	acpi_status status;
>  
>  	status = acpi_remove_address_space_handler(handle, ACPI_ADR_SPACE_GPIO,
>  						   acpi_gpio_adr_space_handler);
> -	if (ACPI_FAILURE(status)) {
> +	if (ACPI_FAILURE(status))
>  		dev_err(chip->parent,
>  			"Failed to remove GPIO OpRegion handler\n");
> -		return;
> -	}

If we fail to remove the OpRegion handler and continue to release the
structures does that mean any AML code that accesses the GPIOs now are
accessing already removed (below) objects? Here _REG is not called (we
failed to remove the OpRegion handler) so the AML does not know that it is
gone so it can happily still access the region. So IMHO it is safer to keep
the connections instead of removing them.

Of course it is possible that I'm missing something. It has been a while
since I looked at any of this stuff.

>  
> -	list_for_each_entry_safe_reverse(conn, tmp, &achip->conns, node) {
> -		gpiochip_free_own_desc(conn->desc);
> -		list_del(&conn->node);
> -		kfree(conn);
> -	}
> +	acpi_gpiochip_free_all_connections(achip);
>  }
>  
>  void acpi_gpiochip_add(struct gpio_chip *chip)
> -- 
> 2.54.0