Re: [PATCH] gpiolib: acpi: Move storage of acpi_gpio_chip

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

On Wed, Mar 13, 2024 at 4:03 AM Hamish Martin
<hamish.martin@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 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 is not entirely accurate.

The ACPI handle of the struct acpi_device going away is replaced with
INVALID_ACPI_HANDLE because by the time acpi_device_del_work_fn()
runs, the namespace object identified by this handle may have been
deleted already.

Moreover, all of the data items attached to that object are deleted by
the very acpi_ns_delete_node() call that has invoked
acpi_scan_drop_device().

So acpi_scan_drop_device() must invalidate the ACPI handle in the
struct acpi_device, because going forward it is not associated with a
valid namespace object and all of the data items attached to it via
acpi_attach_data() have been deleted.

> 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().

Indeed, that information has gone away already.

However, acpi_gpio_chip_dh() is called when that happens, so in
principle you can know when it is happening.

> 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.

I can agree with this.

> 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.

acpi_bus_attach_private_data() is only used in 2 places beyond this,
so I'm not worried too much.

But yes, generally speaking, if the ACPI namespace object you attach
data to can vanish from under you as a result of an overlay removal,
you better not attach data to it or use a meaningful data removal
handler.

> 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
>
>
[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux