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

[Toshi Kani <toshi.kani@hp.com>]

On Wed, 2013-02-13 at 21:52 +0100, Rafael J. Wysocki wrote:
> On Wednesday, February 13, 2013 10:43:58 AM Toshi Kani wrote:
> > On Wed, 2013-02-13 at 14:16 +0100, 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.
> > 
> > I am concerned with this approach.  ACPICA calls notify handlers through
> > kacpi_notify_wq, which has the max active set to 1.  We then use
> > kacpi_hotplug_wq (which also has the max active set to 1) so that a
> > hotplug procedure does not block the notify handlers since they can be
> > used for non-hotplug events as well.
> 
> In fact we use kacpi_hotplug_wq for a different reason.  Please read the
> comment in __acpi_os_execute() for more details.

Yes, I am aware of the issue as well.

> > Acquiring the scan lock in a notify handler means that a hotplug procedure
> > can block any notify events.
> 
> Yes, it can.
> 
> > So, I'd prefer the following approach.
> > 
> >  - Change all hot-plug procedures (i.e. both add and delete) to proceed
> > under kacpi_hotplug_wq by calling acpi_os_hotplug_execute(). This
> > serializes all hotplug procedures, and prevents blocking other notify
> > events.
> 
> Yes, we can do that.  I was thinking about doing that change, but not in v3.9.
> There are simply too many notify handlers already there to do that so late in
> the cycle.  And doing that for acpiphp, for example, won't be straightforward
> at all.

Right.  I was not suggesting this approach for v3.9.

> Please think about the $subject patch as a temporary measure until we can do
> something better (which we need to do anyway to reduce code duplication among
> other things).

I am fine with the scan lock as long as it is internal.  This patch
publishes the locking interfaces to other modules, which made me worried
that this might become a long term solution.  If we need to fix this
issue for v3.9, I am OK with it as you clarified this as a temporary
solution.

> > (Ideally, we should also run all online/offline procedures
> > under a same work-queue, just like my RFC patchset did, but this is a
> > different topic for now.)
> 
> No, I don't think it is appropriate to run online/offline from _any_
> workqueue.  In my opinion they should be run from user space.

I think there are pros and cons for this.  If we use a user thread to
run online/offline procedure, we can return a result directly.  However,
if an operation takes a long time, it will block the user thread until
it is done.  In addition, we have race conditions between hotplug and
online/offline operations.  So, we may need to come up with other type
of locking if we do not use a workqueue to address it.  Having both the
scan lock and other lock in the callers would not be good.

> >  - Revert 5993c4670 unless this change is absolutely necessary.  From
> > the change log, it is not clear to me why this change was needed.  It
> > changed acpi_bus_hot_remove_device() to take an acpi_device, instead of
> > an acpi_handle, which introduced a race condition with acpi_device.
> > acpi_bus_hot_remove_device() should take an acpi_handle, and then obtain
> > its acpi_device from the acpi_handle since this function is serialized.
> 
> I thought about that, but actually there's no guarantee that the handle
> will be valid after _EJ0 as far as I can say.  So the race condition is
> going to be there anyway and using struct acpi_device just makes it easier
> to avoid it.

In theory, yes, a stale handle could be a problem, if _EJ0 performs
unload table and if ACPICA frees up its internal data structure pointed
by the handle as a result.  But we should not see such issue now since
we do not support dynamic ACPI namespace yet.

> >  - Remove sanity checks with an acpi_device in the notify handlers,
> > which have a race condition with acpi_device.  These type-specific
> > checks will need to be removed when we have a common notify handler
> > anyway.  The notify handler can continue to check the status of ACPI
> > device object with an acpi_handle.  Type-specific sanity checks /
> > validations can be performed within a hotplug procedure, instead.
> 
> Well, the sanest approach here would be to queue up a work item on
> kacpi_hotplug_wq if the event is of a "hotplug" type and let that work
> item do all checks, run acpi_bus_scan() etc.  But not in v3.9.

Agreed, and that's what I meant.

> For v3.9, the most straightforward and least intrusive change we can do
> is the $subject patch as far as I can say.  If you can suggest something
> less intrusive and more straightforward, please do.

My suggestion is to keep the scan lock internal for v3.9 and implement a
new hotplug framework (i.e. the one with user space approach) for v3.10
with a proper locking mechanism.  But, since you clarified this as a
temporary solution, I am OK with it if we need to fix it now.

Thanks,
-Toshi