| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8105997.7zQ9mSfecG@vostro.rjw.lan>
Date: Thu, 28 Aug 2014 01:37:57 +0200
From: "Rafael J. Wysocki" <rjw@...ysocki.net>
To: Lan Tianyu <tianyu.lan@...el.com>
Cc: lenb@...nel.org, fengguang.wu@...el.com,
linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
All applicable <stable@...r.kernel.org>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
Subject: Re: [PATCH V3] ACPI / OSL: Make acpi_os_map_cleanup() use call_rcu() to avoid deadlocks
On Wednesday, August 27, 2014 03:11:29 PM Lan Tianyu wrote:
> Deadlock is possible when CPU hotplug and evaluating ACPI method happen
> at the same time.
>
> During CPU hotplug, acpi_cpu_soft_notify() is called under the CPU hotplug
> lock. Then, acpi_cpu_soft_notify() calls acpi_bus_get_device() to obtain
> the struct acpi_device attached to the given ACPI handle. The ACPICA's
> namespace lock will be acquired by acpi_bus_get_device() in the process.
> Thus it is possible to hold the ACPICA's namespace lock under the CPU
> hotplug lock.
>
> Evaluating an ACPI method may involve accessing an operation region in
> system memory and the associated address space will be unmapped under
> the ACPICA's namespace lock after completing the access. Currently, osl.c
> uses RCU to protect memory ranges used by AML. When unmapping them it
> calls synchronize_rcu() in acpi_os_map_cleanup(), but that blocks
> CPU hotplug by acquiring the CPU hotplug lock. Thus it is possible to
> hold the CPU hotplug lock under the ACPICA's namespace lock.
>
> This leads to deadlocks like the following one if AML accessing operation
> regions in memory is executed in parallel with CPU hotplug.
[cut]
> To avoid such deadlocks, modify acpi_os_map_cleanup() to use call_rcu()
> to schedule acpi_os_async_umap() asynchronously to umap memory regions
> that aren't used any more. The umap operation can't be done in the
> call_rcu()'s callback directly because the callback will be called in the
> soft irq context and acpi_unmap() holds mutex lock inside.
>
> Signed-off-by: Lan Tianyu <tianyu.lan@...el.com>
> [rjw: Subject and changelog.]
> Cc: All applicable <stable@...r.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> Signed-off-by: Lan Tianyu <tianyu.lan@...el.com>
> ---
> drivers/acpi/osl.c | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 3abe9b2..9baef71 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -40,6 +40,7 @@
> #include <linux/nmi.h>
> #include <linux/acpi.h>
> #include <linux/efi.h>
> +#include <linux/async.h>
> #include <linux/ioport.h>
> #include <linux/list.h>
> #include <linux/jiffies.h>
> @@ -94,6 +95,7 @@ struct acpi_ioremap {
> acpi_physical_address phys;
> acpi_size size;
> unsigned long refcount;
> + struct rcu_head rcu;
> };
>
> static LIST_HEAD(acpi_ioremaps);
> @@ -423,13 +425,25 @@ static void acpi_os_drop_map_ref(struct acpi_ioremap *map)
> list_del_rcu(&map->list);
> }
>
> +static void acpi_os_async_umap(void *data, async_cookie_t cookie)
> +{
> + struct acpi_ioremap *map = data;
> +
> + acpi_unmap(map->phys, map->virt);
> + kfree(map);
> +}
> +
> +static void acpi_os_map_reclaim(struct rcu_head *rcu)
> +{
> + struct acpi_ioremap *map = container_of(rcu, struct acpi_ioremap, rcu);
> +
> + async_schedule(acpi_os_async_umap, map);
> +}
> +
> static void acpi_os_map_cleanup(struct acpi_ioremap *map)
> {
> - if (!map->refcount) {
> - synchronize_rcu();
> - acpi_unmap(map->phys, map->virt);
> - kfree(map);
> - }
> + if (!map->refcount)
> + call_rcu(&map->rcu, acpi_os_map_reclaim);
> }
>
> void __ref acpi_os_unmap_iomem(void __iomem *virt, acpi_size size)
>
This goes a bit too far. First, if you need to start an async thread,
you can simply do synchronize_rcu() from there. Second, though, perhaps
we can address the whole deadlock in a different way.
For example, if we do something like the patch below (which I haven't
tested, but it should work if I'm not missing something horribly), we
won't be taking the ACPI namespace lock under the CPU hotplug lock
in acpi_cpu_soft_notify() any more.
Rafael
---
drivers/acpi/processor_driver.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Index: linux-pm/drivers/acpi/processor_driver.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_driver.c
+++ linux-pm/drivers/acpi/processor_driver.c
@@ -129,7 +129,11 @@ static int acpi_cpu_soft_notify(struct n
if (action == CPU_STARTING || action == CPU_DYING)
return NOTIFY_DONE;
- if (!pr || acpi_bus_get_device(pr->handle, &device))
+ if (!pr || !pr->dev)
+ return NOTIFY_DONE;
+
+ device = ACPI_COMPANION(pr->dev);
+ if (!device)
return NOTIFY_DONE;
if (action == CPU_ONLINE) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists