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

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

On Wed, Mar 13, 2024 at 01:21:25PM +0200, Andy Shevchenko wrote:
> 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?

Another Q is how does the OF case survive similar flow (DT overlay removal)?

-- 
With Best Regards,
Andy Shevchenko