| Message ID | 1483955769-9579-1-git-send-email-lv.zheng@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Jan 9, 2017 at 10:56 AM, Lv Zheng <lv.zheng@intel.com> wrote: > The rcu synchronization logic is originally provided to protect > apei_read()/apei_write() as in the APEI drivers, there is NMI event source > requiring non spinlock based synchronization mechanism. > > After that, ACPI developers think FADT registers may also require same > facility, so they moved the RCU stuffs to generic ACPI layer. > > So now non-task-context ACPI map lookup is only protected by RCU. > > This triggers problem as acpi_os_map_memory()/acpi_os_unmap_memory() can be > used to map/unmap tables as long as to map/unmap ACPI registers. When it is > used for the ACPI tables, the caller could invoke this very early. When it > is invoked earlier than workqueue_init() and later than > check_early_ioremp_leak(), invoking synchronize_rcu_expedited() can cause a > kernel hang. > > Actually this facility is only used to protect non-task-context ACPI map > lookup, That doesn't sound quite right. acpi_os_read/write_memory() use RCU-protected list lookups, so it's not just non-task-context AFAICS. > and such mappings are only introduced by > acpi_os_map_generic_address(). So before it is invoked, there is no need to > invoke synchronize_rcu_expedited(). That said it may be fine to start actually synchronize RCU after acpi_os_map_generic_address() has been called for the first time. I need a better (or more detailed) explanation why it is fine, though. Thanks, Rafael -- 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
Hi, Rafael > From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J. Wysocki > Sent: Tuesday, January 10, 2017 7:35 AM > Subject: Re: [PATCH] ACPI / OSL: Fix rcu synchronization logic > > On Mon, Jan 9, 2017 at 10:56 AM, Lv Zheng <lv.zheng@intel.com> wrote: > > The rcu synchronization logic is originally provided to protect > > apei_read()/apei_write() as in the APEI drivers, there is NMI event source > > requiring non spinlock based synchronization mechanism. > > > > After that, ACPI developers think FADT registers may also require same > > facility, so they moved the RCU stuffs to generic ACPI layer. > > > > So now non-task-context ACPI map lookup is only protected by RCU. > > > > This triggers problem as acpi_os_map_memory()/acpi_os_unmap_memory() can be > > used to map/unmap tables as long as to map/unmap ACPI registers. When it is > > used for the ACPI tables, the caller could invoke this very early. When it > > is invoked earlier than workqueue_init() and later than > > check_early_ioremp_leak(), invoking synchronize_rcu_expedited() can cause a > > kernel hang. > > > > Actually this facility is only used to protect non-task-context ACPI map > > lookup, > > That doesn't sound quite right. > > acpi_os_read/write_memory() use RCU-protected list lookups, so it's > not just non-task-context AFAICS. Yes, you are right. > > > and such mappings are only introduced by > > acpi_os_map_generic_address(). So before it is invoked, there is no need to > > invoke synchronize_rcu_expedited(). > > That said it may be fine to start actually synchronize RCU after > acpi_os_map_generic_address() has been called for the first time. I > need a better (or more detailed) explanation why it is fine, though. > The reason is wrong. As list lookups are only protected by RCU. Thanks Lv
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index a404ff4..3d93633 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -77,6 +77,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a, static bool acpi_os_initialized; unsigned int acpi_sci_irq = INVALID_ACPI_IRQ; bool acpi_permanent_mmap = false; +bool acpi_synchronize_rcu = false; /* * This list of permanent mappings is for memory that may be accessed from @@ -378,7 +379,8 @@ static void acpi_os_drop_map_ref(struct acpi_ioremap *map) static void acpi_os_map_cleanup(struct acpi_ioremap *map) { if (!map->refcount) { - synchronize_rcu_expedited(); + if (acpi_synchronize_rcu) + synchronize_rcu_expedited(); acpi_unmap(map->phys, map->virt); kfree(map); } @@ -444,6 +446,7 @@ int acpi_os_map_generic_address(struct acpi_generic_address *gas) if (!virt) return -EIO; + acpi_synchronize_rcu = true; return 0; } EXPORT_SYMBOL(acpi_os_map_generic_address);
The rcu synchronization logic is originally provided to protect apei_read()/apei_write() as in the APEI drivers, there is NMI event source requiring non spinlock based synchronization mechanism. After that, ACPI developers think FADT registers may also require same facility, so they moved the RCU stuffs to generic ACPI layer. So now non-task-context ACPI map lookup is only protected by RCU. This triggers problem as acpi_os_map_memory()/acpi_os_unmap_memory() can be used to map/unmap tables as long as to map/unmap ACPI registers. When it is used for the ACPI tables, the caller could invoke this very early. When it is invoked earlier than workqueue_init() and later than check_early_ioremp_leak(), invoking synchronize_rcu_expedited() can cause a kernel hang. Actually this facility is only used to protect non-task-context ACPI map lookup, and such mappings are only introduced by acpi_os_map_generic_address(). So before it is invoked, there is no need to invoke synchronize_rcu_expedited(). Suggested-by: Huang Ying <ying.huang@intel.com> Signed-off-by: Lv Zheng <lv.zheng@intel.com> Cc: Huang Ying <ying.huang@intel.com> Cc: Borislav Petkov <bp@alien8.de> --- drivers/acpi/osl.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)