A memory leak occurs in a scenario where an ACPI SSDT overlay is removed
and that is the trigger for the removal of the acpi_gpio_chip.
This occurs because we use the ACPI_HANDLE of the chip->parent as a
convenient location to tie the 'struct acpi_gpio_chip' to, using
acpi_attach_data().
This is fine in the usual case of removal of the 'struct acpi_gpio_chip'
via a call to acpi_gpiochip_remove() because usually the ACPI data is
still valid.
But in the case of an SSDT overlay removal, the ACPI data has been
marked as invalid before the removal code is triggered (see the setting
of the acpi_device handle to INVALID_ACPI_HANDLE in
acpi_scan_drop_device()). This means that by the time we execute
acpi_gpiochip_remove(), the handles are invalid and we are unable to
retrieve the 'struct acpi_gpio_chip' using acpi_get_data(). Unable to
get our data, we hit the failure case and see the following warning
logged:
Failed to retrieve ACPI GPIO chip
This means we also fail to kfree() the struct at the end of
acpi_gpiochip_remove().
The fix is to no longer tie the acpi_gpio_chip data to the ACPI data,
but instead hang it directly from the 'struct gpio_chip' with a new
field. This decouples the lifecycle of the acpi_gpio_chip from the ACPI
data, and ties it to the gpio_chip. This seems a much better place since
they share a common lifecycle.
The general concept of attaching data to the ACPI objects may also need
revisiting in light of the issue this patch addresses. For instance
i2c_acpi_remove_space_handler() is vulnerable to a similar leak due to
using acpi_bus_get_private_data(), which just wraps acpi_attach_data().
This may need a more widespread change than is addressed in this patch.
Signed-off-by: Hamish Martin <hamish.martin@xxxxxxxxxxxxxxxxxxx>
---
drivers/gpio/gpiolib-acpi.c | 57 ++++++++-----------------------------
include/linux/gpio/driver.h | 4 +++
2 files changed, 16 insertions(+), 45 deletions(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index cd3e9657cc36..14d29902391f 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -180,11 +180,6 @@ static irqreturn_t acpi_gpio_irq_handler_evt(int irq, void *data)
return IRQ_HANDLED;
}
-static void acpi_gpio_chip_dh(acpi_handle handle, void *data)
-{
- /* The address of this function is used as a key. */
-}
-
bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
struct acpi_resource_gpio **agpio)
{
@@ -500,18 +495,17 @@ void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
{
struct acpi_gpio_chip *acpi_gpio;
acpi_handle handle;
- acpi_status status;
bool defer;
if (!chip->parent || !chip->to_irq)
return;
- handle = ACPI_HANDLE(chip->parent);
- if (!handle)
+ acpi_gpio = chip->acpi_gpio;
+ if (!acpi_gpio)
return;
- status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&acpi_gpio);
- if (ACPI_FAILURE(status))
+ handle = ACPI_HANDLE(chip->parent);
+ if (!handle)
return;
if (acpi_quirk_skip_gpio_event_handlers())
@@ -545,18 +539,12 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
{
struct acpi_gpio_chip *acpi_gpio;
struct acpi_gpio_event *event, *ep;
- acpi_handle handle;
- acpi_status status;
if (!chip->parent || !chip->to_irq)
return;
- handle = ACPI_HANDLE(chip->parent);
- if (!handle)
- return;
-
- status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&acpi_gpio);
- if (ACPI_FAILURE(status))
+ acpi_gpio = chip->acpi_gpio;
+ if (!acpi_gpio)
return;
mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
@@ -1218,15 +1206,10 @@ 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)) {
- dev_err(chip->parent,
- "Failed to remove GPIO OpRegion handler\n");
- return;
- }
+ /* Ignore the return status as the handle is likely no longer valid. */
+ acpi_remove_address_space_handler(handle, ACPI_ADR_SPACE_GPIO,
+ acpi_gpio_adr_space_handler);
list_for_each_entry_safe_reverse(conn, tmp, &achip->conns, node) {
gpiochip_free_own_desc(conn->desc);
@@ -1310,7 +1293,6 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
{
struct acpi_gpio_chip *acpi_gpio;
struct acpi_device *adev;
- acpi_status status;
if (!chip || !chip->parent)
return;
@@ -1327,16 +1309,10 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
}
acpi_gpio->chip = chip;
+ chip->acpi_gpio = acpi_gpio;
INIT_LIST_HEAD(&acpi_gpio->events);
INIT_LIST_HEAD(&acpi_gpio->deferred_req_irqs_list_entry);
- status = acpi_attach_data(adev->handle, acpi_gpio_chip_dh, acpi_gpio);
- if (ACPI_FAILURE(status)) {
- dev_err(chip->parent, "Failed to attach ACPI GPIO chip\n");
- kfree(acpi_gpio);
- return;
- }
-
acpi_gpiochip_request_regions(acpi_gpio);
acpi_gpiochip_scan_gpios(acpi_gpio);
acpi_dev_clear_dependencies(adev);
@@ -1345,25 +1321,16 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
void acpi_gpiochip_remove(struct gpio_chip *chip)
{
struct acpi_gpio_chip *acpi_gpio;
- acpi_handle handle;
- acpi_status status;
if (!chip || !chip->parent)
return;
- handle = ACPI_HANDLE(chip->parent);
- if (!handle)
+ acpi_gpio = chip->acpi_gpio;
+ if (!acpi_gpio)
return;
- status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&acpi_gpio);
- if (ACPI_FAILURE(status)) {
- dev_warn(chip->parent, "Failed to retrieve ACPI GPIO chip\n");
- return;
- }
-
acpi_gpiochip_free_regions(acpi_gpio);
- acpi_detach_data(handle, acpi_gpio_chip_dh);
kfree(acpi_gpio);
}
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 7f75c9a51874..698b92b1764c 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -529,6 +529,10 @@ struct gpio_chip {
int (*of_xlate)(struct gpio_chip *gc,
const struct of_phandle_args *gpiospec, u32 *flags);
#endif /* CONFIG_OF_GPIO */
+
+#ifdef CONFIG_GPIO_ACPI
+ struct acpi_gpio_chip *acpi_gpio;
+#endif /* CONFIG_GPIO_ACPI */
};
char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset);
--
2.43.2
