Re: [PATCH] ACPI / hotplug: Fix concurrency issues and memory leaks

Yasuaki Ishimatsu
2013-02-13T03:31:40Z

Hi Rafael,

I have another comment at container.c.

2013/02/13 12:08, Yasuaki Ishimatsu wrote:
> Hi Rafael,
>
> The patch seems good.
> There is a comment below.
>
> 2013/02/13 9:19, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> This changeset is aimed at fixing a few different but related
>> problems in the ACPI hotplug infrastructure.
>>
>> First of all, since notify handlers may be run in parallel with
>> acpi_bus_scan(), acpi_bus_trim() and acpi_bus_hot_remove_device()
>> and some of them are installed for ACPI handles that have no struct
>> acpi_device objects attached (i.e. before those objects are created),
>> those notify handlers have to take acpi_scan_lock to prevent races
>> from taking place (e.g. a struct acpi_device is found to be present
>> for the given ACPI handle, but right after that it is removed by
>> acpi_bus_trim() running in parallel to the given notify handler).
>> Moreover, since some of them call acpi_bus_scan() and
>> acpi_bus_trim(), this leads to the conclusion that acpi_scan_lock
>> should be acquired by the callers of these two funtions rather by
>> these functions themselves.
>>
>> For these reasons, make all notify handlers that can handle device
>> addition and eject events take acpi_scan_lock and remove the
>> acpi_scan_lock locking from acpi_bus_scan() and acpi_bus_trim().
>> Accordingly, update all of their users to make sure that they
>> are always called under acpi_scan_lock.
>>
>> Furthermore, since eject operations are carried out asynchronously
>> with respect to the notify events that trigger them, with the help
>> of acpi_bus_hot_remove_device(), even if notify handlers take the
>> ACPI scan lock, it still is possible that, for example,
>> acpi_bus_trim() will run between acpi_bus_hot_remove_device() and
>> the notify handler that scheduled its execution and that
>> acpi_bus_trim() will remove the device node passed to
>> acpi_bus_hot_remove_device() for ejection. In that case, the struct
>> acpi_device object obtained by acpi_bus_hot_remove_device() will be
>> invalid and not-so-funny things will ensue. To protect agaist that,
>> make the users of acpi_bus_hot_remove_device() run get_device() on
>> ACPI device node objects that are about to be passed to it and make
>> acpi_bus_hot_remove_device() run put_device() on them and check if
>> their ACPI handles are not NULL (make acpi_device_unregister() clear
>> the device nodes' ACPI handles for that check to work).
>>
>> Finally, observe that acpi_os_hotplug_execute() actually can fail,
>> in which case its caller ought to free memory allocated for the
>> context object to prevent leaks from happening. It also needs to
>> run put_device() on the device node that it ran get_device() on
>> previously in that case. Modify the code accordingly.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>
>> On top of linux-pm.git/linux-next.
>>
>> Thanks,
>> Rafael
>>
>> ---
>> drivers/acpi/acpi_memhotplug.c | 56 +++++++++++++++++++-----------
>> drivers/acpi/container.c | 10 +++--
>> drivers/acpi/dock.c | 19 ++++++++--
>> drivers/acpi/processor_driver.c | 24 +++++++++---
>> drivers/acpi/scan.c | 69 +++++++++++++++++++++++++------------
>> drivers/pci/hotplug/acpiphp_glue.c | 6 +++
>> drivers/pci/hotplug/sgi_hotplug.c | 5 ++
>> include/acpi/acpi_bus.h | 3 +
>> 8 files changed, 138 insertions(+), 54 deletions(-)
>>
>> Index: test/drivers/acpi/scan.c
>> ===================================================================
>> --- test.orig/drivers/acpi/scan.c
>> +++ test/drivers/acpi/scan.c
>> @@ -42,6 +42,18 @@ struct acpi_device_bus_id{
>> struct list_head node;
>> };
>>
>> +void acpi_scan_lock_acquire(void)
>> +{
>> + mutex_lock(&acpi_scan_lock);
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_scan_lock_acquire);
>> +
>> +void acpi_scan_lock_release(void)
>> +{
>> + mutex_unlock(&acpi_scan_lock);
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_scan_lock_release);
>> +
>> int acpi_scan_add_handler(struct acpi_scan_handler *handler)
>> {
>> if (!handler || !handler->attach)
>> @@ -95,8 +107,6 @@ acpi_device_modalias_show(struct device
>> }
>> static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
>>
>> -static void __acpi_bus_trim(struct acpi_device *start);
>> -
>> /**
>> * acpi_bus_hot_remove_device: hot-remove a device and its children
>> * @context: struct acpi_eject_event pointer (freed in this func)
>> @@ -107,7 +117,7 @@ static void __acpi_bus_trim(struct acpi_
>> */
>> void acpi_bus_hot_remove_device(void *context)
>> {
>> - struct acpi_eject_event *ej_event = (struct acpi_eject_event *) context;
>> + struct acpi_eject_event *ej_event = context;
>> struct acpi_device *device = ej_event->device;
>> acpi_handle handle = device->handle;
>> acpi_handle temp;
>> @@ -118,11 +128,19 @@ void acpi_bus_hot_remove_device(void *co
>>
>> mutex_lock(&acpi_scan_lock);
>>
>> + /* If there is no handle, the device node has been unregistered. */
>> + if (!device->handle) {
>> + dev_dbg(&device->dev, "ACPI handle missing\n");
>> + put_device(&device->dev);
>> + goto out;
>> + }
>> +
>> ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>> "Hot-removing device %s...\n", dev_name(&device->dev)));
>>
>> - __acpi_bus_trim(device);
>> - /* Device node has been released. */
>> + acpi_bus_trim(device);
>> + /* Device node has been unregistered. */
>> + put_device(&device->dev);
>> device = NULL;
>>
>> if (ACPI_SUCCESS(acpi_get_handle(handle, "_LCK", &temp))) {
>> @@ -151,6 +169,7 @@ void acpi_bus_hot_remove_device(void *co
>> ost_code, NULL);
>> }
>>
>> + out:
>> mutex_unlock(&acpi_scan_lock);
>> kfree(context);
>> return;
>> @@ -212,6 +231,7 @@ acpi_eject_store(struct device *d, struc
>> goto err;
>> }
>>
>> + get_device(&acpi_device->dev);
>> ej_event->device = acpi_device;
>> if (acpi_device->flags.eject_pending) {
>> /* event originated from ACPI eject notification */
>> @@ -224,7 +244,11 @@ acpi_eject_store(struct device *d, struc
>> ej_event->event, ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
>> }
>>
>> - acpi_os_hotplug_execute(acpi_bus_hot_remove_device, (void *)ej_event);
>> + status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device, ej_event);
>> + if (ACPI_FAILURE(status)) {
>> + put_device(&acpi_device->dev);
>> + kfree(ej_event);
>> + }
>> err:
>> return ret;
>> }
>> @@ -779,6 +803,7 @@ static void acpi_device_unregister(struc
>> * no more references.
>> */
>> acpi_device_set_power(device, ACPI_STATE_D3_COLD);
>> + device->handle = NULL;
>> put_device(&device->dev);
>> }
>>
>> @@ -1626,14 +1651,14 @@ static acpi_status acpi_bus_device_attac
>> * there has been a real error. There just have been no suitable ACPI objects
>> * in the table trunk from which the kernel could create a device and add an
>> * appropriate driver.
>> + *
>> + * Must be called under acpi_scan_lock.
>> */
>> int acpi_bus_scan(acpi_handle handle)
>> {
>> void *device = NULL;
>> int error = 0;
>>
>> - mutex_lock(&acpi_scan_lock);
>> -
>> if (ACPI_SUCCESS(acpi_bus_check_add(handle, 0, NULL, &device)))
>> acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
>> acpi_bus_check_add, NULL, NULL, &device);
>> @@ -1644,7 +1669,6 @@ int acpi_bus_scan(acpi_handle handle)
>> acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
>> acpi_bus_device_attach, NULL, NULL, NULL);
>>
>> - mutex_unlock(&acpi_scan_lock);
>> return error;
>> }
>> EXPORT_SYMBOL(acpi_bus_scan);
>> @@ -1681,7 +1705,13 @@ static acpi_status acpi_bus_remove(acpi_
>> return AE_OK;
>> }
>>
>> -static void __acpi_bus_trim(struct acpi_device *start)
>> +/**
>> + * acpi_bus_trim - Remove ACPI device node and all of its descendants
>> + * @start: Root of the ACPI device nodes subtree to remove.
>> + *
>> + * Must be called under acpi_scan_lock.
>> + */
>> +void acpi_bus_trim(struct acpi_device *start)
>> {
>> /*
>> * Execute acpi_bus_device_detach() as a post-order callback to detach
>> @@ -1698,13 +1728,6 @@ static void __acpi_bus_trim(struct acpi_
>> acpi_bus_remove, NULL, NULL);
>> acpi_bus_remove(start->handle, 0, NULL, NULL);
>> }
>> -
>> -void acpi_bus_trim(struct acpi_device *start)
>> -{
>> - mutex_lock(&acpi_scan_lock);
>> - __acpi_bus_trim(start);
>> - mutex_unlock(&acpi_scan_lock);
>> -}
>> EXPORT_SYMBOL_GPL(acpi_bus_trim);
>>
>> static int acpi_bus_scan_fixed(void)
>> @@ -1762,23 +1785,27 @@ int __init acpi_scan_init(void)
>> acpi_csrt_init();
>> acpi_container_init();
>>
>> + mutex_lock(&acpi_scan_lock);
>> /*
>> * Enumerate devices in the ACPI namespace.
>> */
>> result = acpi_bus_scan(ACPI_ROOT_OBJECT);
>> if (result)
>> - return result;
>> + goto out;
>>
>> result = acpi_bus_get_device(ACPI_ROOT_OBJECT, &acpi_root);
>> if (result)
>> - return result;
>> + goto out;
>>
>> result = acpi_bus_scan_fixed();
>> if (result) {
>> acpi_device_unregister(acpi_root);
>> - return result;
>> + goto out;
>> }
>>
>> acpi_update_all_gpes();
>> - return 0;
>> +
>> + out:
>> + mutex_unlock(&acpi_scan_lock);
>> + return result;
>> }
>> Index: test/include/acpi/acpi_bus.h
>> ===================================================================
>> --- test.orig/include/acpi/acpi_bus.h
>> +++ test/include/acpi/acpi_bus.h
>> @@ -395,6 +395,9 @@ int acpi_bus_receive_event(struct acpi_b
>> static inline int acpi_bus_generate_proc_event(struct acpi_device *device, u8 type, int data)
>> { return 0; }
>> #endif
>> +
>> +void acpi_scan_lock_acquire(void);
>> +void acpi_scan_lock_release(void);
>> int acpi_scan_add_handler(struct acpi_scan_handler *handler);
>> int acpi_bus_register_driver(struct acpi_driver *driver);
>> void acpi_bus_unregister_driver(struct acpi_driver *driver);
>> Index: test/drivers/acpi/acpi_memhotplug.c
>> ===================================================================
>> --- test.orig/drivers/acpi/acpi_memhotplug.c
>> +++ test/drivers/acpi/acpi_memhotplug.c
>> @@ -153,14 +153,16 @@ acpi_memory_get_device_resources(struct
>> return 0;
>> }
>>
>> -static int
>> -acpi_memory_get_device(acpi_handle handle,
>> - struct acpi_memory_device **mem_device)
>> +static int acpi_memory_get_device(acpi_handle handle,
>> + struct acpi_memory_device **mem_device)
>> {
>> struct acpi_device *device = NULL;
>> - int result;
>> + int result = 0;
>> +
>> + acpi_scan_lock_acquire();
>>
>> - if (!acpi_bus_get_device(handle, &device) && device)
>> + acpi_bus_get_device(handle, &device);
>> + if (device)
>> goto end;
>>
>> /*
>> @@ -169,23 +171,28 @@ acpi_memory_get_device(acpi_handle handl
>> */
>> result = acpi_bus_scan(handle);
>> if (result) {
>> - acpi_handle_warn(handle, "Cannot add acpi bus\n");
>> - return -EINVAL;
>> + acpi_handle_warn(handle, "ACPI namespace scan failed\n");
>> + result = -EINVAL;
>> + goto out;
>> }
>> result = acpi_bus_get_device(handle, &device);
>> if (result) {
>> acpi_handle_warn(handle, "Missing device object\n");
>> - return -EINVAL;
>> + result = -EINVAL;
>> + goto out;
>> }
>>
>> - end:
>> + end:
>> *mem_device = acpi_driver_data(device);
>> if (!(*mem_device)) {
>> dev_err(&device->dev, "driver data not found\n");
>> - return -ENODEV;
>> + result = -ENODEV;
>> + goto out;
>> }
>>
>> - return 0;
>> + out:
>> + acpi_scan_lock_release();
>> + return result;
>> }
>>
>> static int acpi_memory_check_device(struct acpi_memory_device *mem_device)
>> @@ -305,6 +312,7 @@ static void acpi_memory_device_notify(ac
>> struct acpi_device *device;
>> struct acpi_eject_event *ej_event = NULL;
>> u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
>> + acpi_status status;
>>
>> switch (event) {
>> case ACPI_NOTIFY_BUS_CHECK:
>> @@ -327,29 +335,40 @@ static void acpi_memory_device_notify(ac
>> ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>> "\nReceived EJECT REQUEST notification for device\n"));
>>
>> + status = AE_ERROR;
>> + acpi_scan_lock_acquire();
>> +
>> if (acpi_bus_get_device(handle, &device)) {
>> acpi_handle_err(handle, "Device doesn't exist\n");
>> - break;
>> + goto unlock;
>> }
>> mem_device = acpi_driver_data(device);
>> if (!mem_device) {
>> acpi_handle_err(handle, "Driver Data is NULL\n");
>> - break;
>> + goto unlock;
>> }
>>
>> ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
>> if (!ej_event) {
>> pr_err(PREFIX "No memory, dropping EJECT\n");
>> - break;
>> + goto unlock;
>> }
>>
>> + get_device(&device->dev);
>> ej_event->device = device;
>> ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
>> - acpi_os_hotplug_execute(acpi_bus_hot_remove_device,
>> - (void *)ej_event);
>> + /* The eject is carried out asynchronously. */
>> + status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device,
>> + ej_event);
>> + if (ACPI_FAILURE(status)) {
>> + put_device(&device->dev);
>> + kfree(ej_event);
>> + }
>>
>> - /* eject is performed asynchronously */
>> - return;
>> + unlock:
>> + acpi_scan_lock_release();
>> + if (ACPI_SUCCESS(status))
>> + return;
>> default:
>> ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>> "Unsupported event [0x%x]\n", event));
>> @@ -360,7 +379,6 @@ static void acpi_memory_device_notify(ac
>>
>> /* Inform firmware that the hotplug operation has completed */
>> (void) acpi_evaluate_hotplug_ost(handle, event, ost_code, NULL);
>> - return;
>> }
>>
>> static void acpi_memory_device_free(struct acpi_memory_device *mem_device)
>> Index: test/drivers/acpi/processor_driver.c
>> ===================================================================
>> --- test.orig/drivers/acpi/processor_driver.c
>> +++ test/drivers/acpi/processor_driver.c
>> @@ -683,8 +683,11 @@ static void acpi_processor_hotplug_notif
>> struct acpi_device *device = NULL;
>> struct acpi_eject_event *ej_event = NULL;
>> u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
>> + acpi_status status;
>> int result;
>>
>> + acpi_scan_lock_acquire();
>> +
>> switch (event) {
>> case ACPI_NOTIFY_BUS_CHECK:
>> case ACPI_NOTIFY_DEVICE_CHECK:
>> @@ -733,25 +736,32 @@ static void acpi_processor_hotplug_notif
>> break;
>> }
>>
>> + get_device(&device->dev);
>> ej_event->device = device;
>> ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
>> - acpi_os_hotplug_execute(acpi_bus_hot_remove_device,
>> - (void *)ej_event);
>> -
>> - /* eject is performed asynchronously */
>> - return;
>> + /* The eject is carried out asynchronously. */
>> + status = acpi_os_hotplug_execute(acpi_bus_hot_remove_device,
>> + ej_event);
>> + if (ACPI_FAILURE(status)) {
>> + put_device(&device->dev);
>> + kfree(ej_event);
>> + break;
>> + }
>> + goto out;
>>
>> default:
>> ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>> "Unsupported event [0x%x]\n", event));
>>
>> /* non-hotplug event; possibly handled by other handler */
>> - return;
>> + goto out;
>> }
>>
>> /* Inform firmware that the hotplug operation has completed */
>> (void) acpi_evaluate_hotplug_ost(handle, event, ost_code, NULL);
>> - return;
>> +
>> + out:
>
>> + acpi_scan_lock_release();;
>
> extra ";"
>
> Thanks,
> Yasuaki Ishimatsu
>
>> }
>>
>> static acpi_status is_processor_device(acpi_handle handle)
>> Index: test/drivers/acpi/container.c
>> ===================================================================
>> --- test.orig/drivers/acpi/container.c
>> +++ test/drivers/acpi/container.c
>> @@ -88,6 +88,8 @@ static void container_notify_cb(acpi_han
>> acpi_status status;
>> u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
>>
>> + acpi_scan_lock_acquire();
>> +
>> switch (type) {
>> case ACPI_NOTIFY_BUS_CHECK:
>> /* Fall through */

101 present = is_device_present(handle);
102 status = acpi_bus_get_device(handle, &device);
103 if (!present) {
104 if (ACPI_SUCCESS(status)) {
105 /* device exist and this is a remove request */
106 device->flags.eject_pending = 1;
107 kobject_uevent(&device->dev.kobj, KOBJ_OFLINE);

108 return;

It should use "goto out" instead of return.

109 }
110 break;
111 }

Thanks,
Yasuaki Ishimatsu

>> @@ -130,18 +132,20 @@ static void container_notify_cb(acpi_han
>> if (!acpi_bus_get_device(handle, &device) && device) {
>> device->flags.eject_pending = 1;
>> kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
>> - return;
>> + goto out;
>> }
>> break;
>>
>> default:
>> /* non-hotplug event; possibly handled by other handler */
>> - return;
>> + goto out;
>> }
>>
>> /* Inform firmware that the hotplug operation has completed */
>> (void) acpi_evaluate_hotplug_ost(handle, type, ost_code, NULL);
>> - return;
>> +
>> + out:
>> + acpi_scan_lock_release();
>> }
>>
>> static bool is_container(acpi_handle handle)
>> Index: test/drivers/acpi/dock.c
>> ===================================================================
>> --- test.orig/drivers/acpi/dock.c
>> +++ test/drivers/acpi/dock.c
>> @@ -744,7 +744,9 @@ static void acpi_dock_deferred_cb(void *
>> {
>> struct dock_data *data = context;
>>
>> + acpi_scan_lock_acquire();
>> dock_notify(data->handle, data->event, data->ds);
>> + acpi_scan_lock_release();
>> kfree(data);
>> }
>>
>> @@ -757,20 +759,31 @@ static int acpi_dock_notifier_call(struc
>> if (event != ACPI_NOTIFY_BUS_CHECK && event != ACPI_NOTIFY_DEVICE_CHECK
>> && event != ACPI_NOTIFY_EJECT_REQUEST)
>> return 0;
>> +
>> + acpi_scan_lock_acquire();
>> +
>> list_for_each_entry(dock_station, &dock_stations, sibling) {
>> if (dock_station->handle == handle) {
>> struct dock_data *dd;
>> + acpi_status status;
>>
>> dd = kmalloc(sizeof(*dd), GFP_KERNEL);
>> if (!dd)
>> - return 0;
>> + break;
>> +
>> dd->handle = handle;
>> dd->event = event;
>> dd->ds = dock_station;
>> - acpi_os_hotplug_execute(acpi_dock_deferred_cb, dd);
>> - return 0 ;
>> + status = acpi_os_hotplug_execute(acpi_dock_deferred_cb,
>> + dd);
>> + if (ACPI_FAILURE(status))
>> + kfree(dd);
>> +
>> + break;
>> }
>> }
>> +
>> + acpi_scan_lock_release();
>> return 0;
>> }
>>
>> Index: test/drivers/pci/hotplug/acpiphp_glue.c
>> ===================================================================
>> --- test.orig/drivers/pci/hotplug/acpiphp_glue.c
>> +++ test/drivers/pci/hotplug/acpiphp_glue.c
>> @@ -1218,6 +1218,8 @@ static void _handle_hotplug_event_bridge
>> handle = hp_work->handle;
>> type = hp_work->type;
>>
>> + acpi_scan_lock_acquire();
>> +
>> if (acpi_bus_get_device(handle, &device)) {
>> /* This bridge must have just been physically inserted */
>> handle_bridge_insertion(handle, type);
>> @@ -1295,6 +1297,7 @@ static void _handle_hotplug_event_bridge
>> }
>>
>> out:
>> + acpi_scan_lock_release();
>> kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
>> }
>>
>> @@ -1341,6 +1344,8 @@ static void _handle_hotplug_event_func(s
>>
>> func = (struct acpiphp_func *)context;
>>
>> + acpi_scan_lock_acquire();
>> +
>> switch (type) {
>> case ACPI_NOTIFY_BUS_CHECK:
>> /* bus re-enumerate */
>> @@ -1371,6 +1376,7 @@ static void _handle_hotplug_event_func(s
>> break;
>> }
>>
>> + acpi_scan_lock_release();
>> kfree(hp_work); /* allocated in handle_hotplug_event_func */
>> }
>>
>> Index: test/drivers/pci/hotplug/sgi_hotplug.c
>> ===================================================================
>> --- test.orig/drivers/pci/hotplug/sgi_hotplug.c
>> +++ test/drivers/pci/hotplug/sgi_hotplug.c
>> @@ -425,6 +425,7 @@ static int enable_slot(struct hotplug_sl
>> pdevice = NULL;
>> }
>>
>> + acpi_scan_lock_acquire();
>> /*
>> * Walk the rootbus node's immediate children looking for
>> * the slot's device node(s). There can be more than
>> @@ -458,6 +459,7 @@ static int enable_slot(struct hotplug_sl
>> }
>> }
>> }
>> + acpi_scan_lock_release();
>> }
>>
>> /* Call the driver for the new device */
>> @@ -508,6 +510,7 @@ static int disable_slot(struct hotplug_s
>> /* Get the rootbus node pointer */
>> phandle = PCI_CONTROLLER(slot->pci_bus)->acpi_handle;
>>
>> + acpi_scan_lock_acquire();
>> /*
>> * Walk the rootbus node's immediate children looking for
>> * the slot's device node(s). There can be more than
>> @@ -538,7 +541,7 @@ static int disable_slot(struct hotplug_s
>> acpi_bus_trim(device);
>> }
>> }
>> -
>> + acpi_scan_lock_release();
>> }
>>
>> /* Free the SN resources assigned to the Linux device.*/
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html