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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

+Cc: Rafael

On Wed, Mar 13, 2024 at 04:02:51PM +1300, Hamish Martin 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 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.

Maybe in this case it's indeed better.

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

But with this it sounds to me that the root cause is like you said in ACPI
removal device code, i.e. acpi_scan_drop_device. Shouldn't that be fixed first
to be more clever?
Or are you stating that basically acpi_bus_get_private_data() and concept of
the private data is weak to the point that mostly become useless?

-- 
With Best Regards,
Andy Shevchenko