Re: [PATCH] ACPICA: fix a memleak issue related to ACPI/GPIO

["chenxiang (M)" <chenxiang66@xxxxxxxxxxxxx>]

Hi Erik,


在 2021/5/19 5:35, Kaneda, Erik 写道:
> > -----Original Message-----
> > From: chenxiang (M) <chenxiang66@xxxxxxxxxxxxx>
> > Sent: Monday, May 17, 2021 7:02 PM
> > To: Kaneda, Erik <erik.kaneda@xxxxxxxxx>; Moore, Robert
> > <robert.moore@xxxxxxxxx>; Wysocki, Rafael J <rafael.j.wysocki@xxxxxxxxx>;
> > hoan@xxxxxxxxxxxxxxxxxxxxxx; fancer.lancer@xxxxxxxxx
> > Cc: linux-acpi@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx;
> > linuxarm@xxxxxxxxxx
> > Subject: Re: [PATCH] ACPICA: fix a memleak issue related to ACPI/GPIO
> >
> > Hi Erik,
> >
> >
> > 在 2021/5/18 2:54, Kaneda, Erik 写道:
> > > > -----Original Message-----
> > > > From: chenxiang <chenxiang66@xxxxxxxxxxxxx>
> > > > Sent: Tuesday, May 11, 2021 8:30 PM
> > > > To: Moore, Robert <robert.moore@xxxxxxxxx>; Kaneda, Erik
> > > > <erik.kaneda@xxxxxxxxx>; Wysocki, Rafael J
> > <rafael.j.wysocki@xxxxxxxxx>;
> > > > hoan@xxxxxxxxxxxxxxxxxxxxxx; fancer.lancer@xxxxxxxxx
> > > > Cc: linux-acpi@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx;
> > > > linuxarm@xxxxxxxxxx; Xiang Chen <chenxiang66@xxxxxxxxxxxxx>
> > > > Subject: [PATCH] ACPICA: fix a memleak issue related to ACPI/GPIO
> > > >
> > > > From: Xiang Chen <chenxiang66@xxxxxxxxxxxxx>
> > > >
> > > > There is a memleak reported as follows:
> > > >
> > > > unreferenced object 0xffff00208ff85a00 (size 128):
> > > >     comm "swapper/0", pid 1, jiffies 4294892588 (age 887.572s)
> > > >     hex dump (first 32 bytes):
> > > >       00 00 00 00 02 00 00 00 08 5a f8 8f 20 00 ff ff  .........Z.. ...
> > > >       08 5a f8 8f 20 00 ff ff 00 00 00 00 00 00 00 00  .Z.. ...........
> > > > backtrace:
> > > >       [<00000000bc25bad8>] slab_post_alloc_hook+0x80/0x2e0
> > > >       [<000000008d547074>] kmem_cache_alloc+0x194/0x2c0
> > > >       [<00000000b08da9ad>] acpi_os_create_semaphore+0x3c/0x78
> > > >       [<0000000024816c0a>] acpi_ev_install_space_handler+0x214/0x274
> > > >       [<00000000d93a5ac2>] acpi_install_address_space_handler+0x64/0xb0
> > > >       [<0000000098c37a45>] acpi_gpiochip_add+0x130/0x348
> > > >       [<00000000c1cf4b42>] gpiochip_add_data_with_key+0x79c/0xdd0
> > > >       [<000000005ce539e9>]
> > devm_gpiochip_add_data_with_key+0x30/0x90
> > > >       [<00000000a3038b8d>] dwapb_gpio_probe+0x3e4/0x7e8
> > > >       [<0000000047a03eba>] platform_probe+0x68/0xe0
> > > >       [<00000000dc15c501>] really_probe+0x17c/0x4a0
> > > >       [<00000000aa1f123d>] driver_probe_device+0x68/0xd0
> > > >       [<00000000d97646e0>] device_driver_attach+0x74/0x80
> > > >       [<0000000073d5b3e5>] __driver_attach+0x8c/0xe0
> > > >       [<00000000ff60d118>] bus_for_each_dev+0x7c/0xd8
> > > >       [<00000000b018393d>] driver_attach+0x24/0x30
> > > >
> > > > It requires to delete the handler object in function
> > > > acpi_remove_address_space_handler() but it just up the sem with
> > function
> > > > acpi_os_release_mutex(), so use acpi_os_delete_mutex() instead of
> > > > acpi_os_release_mutex() in function
> > > > acpi_remove_address_space_handler().
> > > >
> > > > Signed-off-by: Xiang Chen <chenxiang66@xxxxxxxxxxxxx>
> > > > ---
> > > >    drivers/acpi/acpica/evxfregn.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/acpi/acpica/evxfregn.c
> > b/drivers/acpi/acpica/evxfregn.c
> > > > index b1ff0a8..4db0bec 100644
> > > > --- a/drivers/acpi/acpica/evxfregn.c
> > > > +++ b/drivers/acpi/acpica/evxfregn.c
> > > > @@ -201,7 +201,7 @@
> > acpi_remove_address_space_handler(acpi_handle
> > > > device,
> > > >
> > > >    			/* Now we can delete the handler object */
> > > Hi Xiang,
> > >
> > > > -			acpi_os_release_mutex(handler_obj-
> > > > > address_space.
> > > > +			acpi_os_delete_mutex(handler_obj->address_space.
> > > >    					      context_mutex);
> > > Thanks for this suggestion! Instead of acpi_os_delete_mutex, could you try
> > using acpi_ut_remove_reference instead?
> > > I believe this will is a safer option. Please test this and see if it fixes the
> > memory leak.
> Hi,
>
> > But there is already acpi_ut_remove_reference(handler_obj) behind it.
> The delete mutex could result in unexpected behavior because it's not always the case that acpi_ut_remove_reference will clean up the object. This function cleans up the object if the reference count is 0 so we should add the delete mutex during the deletion instead.
>
> Could you try this code to see if it fixes the leak?

I have tested the change, and it fixes the leak, and so please feel free 
to add:
Tested-by: Xiang Chen <chenxiang66@xxxxxxxxxxxxx>

> diff --git a/drivers/acpi/acpica/utdelete.c b/drivers/acpi/acpica/utdelete.c
> index 624a26794d55..e5ba9795ec69 100644
> --- a/drivers/acpi/acpica/utdelete.c
> +++ b/drivers/acpi/acpica/utdelete.c
> @@ -285,6 +285,14 @@ static void acpi_ut_delete_internal_obj(union acpi_operand_object *object)
>                  }
>                  break;
>
> +       case ACPI_TYPE_LOCAL_ADDRESS_HANDLER:
> +
> +               ACPI_DEBUG_PRINT((ACPI_DB_ALLOCATIONS,
> +                                 "***** Address handler %p\n", object));
> +
> +               acpi_os_delete_mutex(object->address_space.context_mutex);
> +               break;
> +
>          default:
>
>                  break;
>
> > > Thanks,
> > > Erik
> > >
> > > >    			acpi_ut_remove_reference(handler_obj);
> > > >    			goto unlock_and_exit;
> > > > --
> > > > 2.8.1
> > > .