| 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
| ||
|
Date: Tue, 20 Sep 2022 13:16:32 +0100
From: Robin Murphy <robin.murphy@....com>
To: Thorsten Leemhuis <regressions@...mhuis.info>,
Lu Baolu <baolu.lu@...ux.intel.com>, iommu@...ts.linux.dev
Cc: Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
Kevin Tian <kevin.tian@...el.com>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
George Hilliard <thirtythreeforty@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] Revert "iommu/vt-d: Fix possible recursive locking in
intel_iommu_init()"
On 2022-09-20 12:58, Thorsten Leemhuis wrote:
> On 20.09.22 10:17, Lu Baolu wrote:
>> This reverts commit 9cd4f1434479f1ac25c440c421fbf52069079914.
>
> Thx for taking care of this.
>
>> Some issues were reported on the original commit. Some thunderbolt devices
>> don't work anymore due to the following DMA fault.
>>
>> DMAR: DRHD: handling fault status reg 2
>> DMAR: [INTR-REMAP] Request device [09:00.0] fault index 0x8080
>> [fault reason 0x25]
>> Blocked a compatibility format interrupt request
>>
>> Bring it back for now to avoid functional regression.
>>
>> Fixes: 9cd4f1434479f ("iommu/vt-d: Fix possible recursive locking in intel_iommu_init()")
>> Link: https://lore.kernel.org/linux-iommu/485A6EA5-6D58-42EA-B298-8571E97422DE@getmailspring.com/
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216497
>
> Both those reports were against 5.19.y, so this afaics should have a
>
> Cc: <stable@...r.kernel.org> # 5.19.x
>
> to ensure it's backported.
>
> Speaking of which: Joerg/Will/Robin, it seems quite a few people are
> running into this, it hence would be great to get this quickly mainlined
> (maybe by letting Linus pick it up straight from the list once ready?)
> so stable can pick it up.
As a heads-up, a straight revert is likely to lead to people reporting
lockdep warnings against -next, for the patches queued there which
exposed this dodgy locking in the first place.
Does it work to just move the dmar_register_bus_notifier() call back to
where it was, without undoing the rest of the patch? That seems like the
change that's overwhelmingly likely to have broken IRQ remapping, and
TBH it wasn't clear to me why the original patch moved it to begin with.
Thanks,
Robin.
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
>
> P.S.: As the Linux kernel's regression tracker I deal with a lot of
> reports and sometimes miss something important when writing mails like
> this. If that's the case here, don't hesitate to tell me in a public
> reply, it's in everyone's interest to set the public record straight.
>
>> Cc: Mika Westerberg <mika.westerberg@...ux.intel.com>
>> Reported-and-tested-by: George Hilliard <thirtythreeforty@...il.com>
>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>> ---
>> include/linux/dmar.h | 4 +---
>> drivers/iommu/intel/dmar.c | 7 -------
>> drivers/iommu/intel/iommu.c | 27 +++++++++++++++++++++++++--
>> 3 files changed, 26 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
>> index 8917a32173c4..d81a51978d01 100644
>> --- a/include/linux/dmar.h
>> +++ b/include/linux/dmar.h
>> @@ -65,7 +65,6 @@ struct dmar_pci_notify_info {
>>
>> extern struct rw_semaphore dmar_global_lock;
>> extern struct list_head dmar_drhd_units;
>> -extern int intel_iommu_enabled;
>>
>> #define for_each_drhd_unit(drhd) \
>> list_for_each_entry_rcu(drhd, &dmar_drhd_units, list, \
>> @@ -89,8 +88,7 @@ extern int intel_iommu_enabled;
>> static inline bool dmar_rcu_check(void)
>> {
>> return rwsem_is_locked(&dmar_global_lock) ||
>> - system_state == SYSTEM_BOOTING ||
>> - (IS_ENABLED(CONFIG_INTEL_IOMMU) && !intel_iommu_enabled);
>> + system_state == SYSTEM_BOOTING;
>> }
>>
>> #define dmar_rcu_dereference(p) rcu_dereference_check((p), dmar_rcu_check())
>> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
>> index 04f7991734a9..3528058d253e 100644
>> --- a/drivers/iommu/intel/dmar.c
>> +++ b/drivers/iommu/intel/dmar.c
>> @@ -2356,13 +2356,6 @@ static int dmar_device_hotplug(acpi_handle handle, bool insert)
>> if (!dmar_in_use())
>> return 0;
>>
>> - /*
>> - * It's unlikely that any I/O board is hot added before the IOMMU
>> - * subsystem is initialized.
>> - */
>> - if (IS_ENABLED(CONFIG_INTEL_IOMMU) && !intel_iommu_enabled)
>> - return -EOPNOTSUPP;
>> -
>> if (dmar_detect_dsm(handle, DMAR_DSM_FUNC_DRHD)) {
>> tmp = handle;
>> } else {
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index c20f3e060bac..5bab71a05830 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -3029,7 +3029,13 @@ static int __init init_dmars(void)
>>
>> #ifdef CONFIG_INTEL_IOMMU_SVM
>> if (pasid_supported(iommu) && ecap_prs(iommu->ecap)) {
>> + /*
>> + * Call dmar_alloc_hwirq() with dmar_global_lock held,
>> + * could cause possible lock race condition.
>> + */
>> + up_write(&dmar_global_lock);
>> ret = intel_svm_enable_prq(iommu);
>> + down_write(&dmar_global_lock);
>> if (ret)
>> goto free_iommu;
>> }
>> @@ -3941,6 +3947,7 @@ int __init intel_iommu_init(void)
>> force_on = (!intel_iommu_tboot_noforce && tboot_force_iommu()) ||
>> platform_optin_force_iommu();
>>
>> + down_write(&dmar_global_lock);
>> if (dmar_table_init()) {
>> if (force_on)
>> panic("tboot: Failed to initialize DMAR table\n");
>> @@ -3953,6 +3960,16 @@ int __init intel_iommu_init(void)
>> goto out_free_dmar;
>> }
>>
>> + up_write(&dmar_global_lock);
>> +
>> + /*
>> + * The bus notifier takes the dmar_global_lock, so lockdep will
>> + * complain later when we register it under the lock.
>> + */
>> + dmar_register_bus_notifier();
>> +
>> + down_write(&dmar_global_lock);
>> +
>> if (!no_iommu)
>> intel_iommu_debugfs_init();
>>
>> @@ -3997,9 +4014,11 @@ int __init intel_iommu_init(void)
>> pr_err("Initialization failed\n");
>> goto out_free_dmar;
>> }
>> + up_write(&dmar_global_lock);
>>
>> init_iommu_pm_ops();
>>
>> + down_read(&dmar_global_lock);
>> for_each_active_iommu(iommu, drhd) {
>> /*
>> * The flush queue implementation does not perform
>> @@ -4017,10 +4036,12 @@ int __init intel_iommu_init(void)
>> "%s", iommu->name);
>> iommu_device_register(&iommu->iommu, &intel_iommu_ops, NULL);
>> }
>> + up_read(&dmar_global_lock);
>>
>> if (si_domain && !hw_pass_through)
>> register_memory_notifier(&intel_iommu_memory_nb);
>>
>> + down_read(&dmar_global_lock);
>> if (probe_acpi_namespace_devices())
>> pr_warn("ACPI name space devices didn't probe correctly\n");
>>
>> @@ -4031,15 +4052,17 @@ int __init intel_iommu_init(void)
>>
>> iommu_disable_protect_mem_regions(iommu);
>> }
>> + up_read(&dmar_global_lock);
>>
>> - intel_iommu_enabled = 1;
>> - dmar_register_bus_notifier();
>> pr_info("Intel(R) Virtualization Technology for Directed I/O\n");
>>
>> + intel_iommu_enabled = 1;
>> +
>> return 0;
>>
>> out_free_dmar:
>> intel_iommu_free_dmars();
>> + up_write(&dmar_global_lock);
>> return ret;
>> }
>>
Powered by blists - more mailing lists