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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <SJ1PR11MB612990255BD8D39C5F856C30B9DF2@SJ1PR11MB6129.namprd11.prod.outlook.com>
Date: Mon, 17 Mar 2025 05:54:49 +0000
From: "Borah, Chaitanya Kumar" <chaitanya.kumar.borah@...el.com>
To: Lu Baolu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>, "Will
 Deacon" <will@...nel.org>, Robin Murphy <robin.murphy@....com>, "Tian, Kevin"
	<kevin.tian@...el.com>
CC: "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH 1/1] iommu/vt-d: Fix possible circular locking dependency



> -----Original Message-----
> From: Lu Baolu <baolu.lu@...ux.intel.com>
> Sent: Monday, March 17, 2025 9:27 AM
> To: Joerg Roedel <joro@...tes.org>; Will Deacon <will@...nel.org>; Robin
> Murphy <robin.murphy@....com>; Tian, Kevin <kevin.tian@...el.com>;
> Borah, Chaitanya Kumar <chaitanya.kumar.borah@...el.com>
> Cc: iommu@...ts.linux.dev; linux-kernel@...r.kernel.org; Lu Baolu
> <baolu.lu@...ux.intel.com>; stable@...r.kernel.org
> Subject: [PATCH 1/1] iommu/vt-d: Fix possible circular locking dependency
> 
> We have recently seen report of lockdep circular lock dependency warnings
> on platforms like skykale and kabylake:
> 
>  ======================================================
>  WARNING: possible circular locking dependency detected  6.14.0-rc6-
> CI_DRM_16276-gca2c04fe76e8+ #1 Not tainted
>  ------------------------------------------------------
>  swapper/0/1 is trying to acquire lock:
>  ffffffff8360ee48 (iommu_probe_device_lock){+.+.}-{3:3},
>    at: iommu_probe_device+0x1d/0x70
> 
>  but task is already holding lock:
>  ffff888102c7efa8 (&device->physical_node_lock){+.+.}-{3:3},
>    at: intel_iommu_init+0xe75/0x11f0
> 
>  which lock already depends on the new lock.
> 
>  the existing dependency chain (in reverse order) is:
> 
>  -> #6 (&device->physical_node_lock){+.+.}-{3:3}:
>         __mutex_lock+0xb4/0xe40
>         mutex_lock_nested+0x1b/0x30
>         intel_iommu_init+0xe75/0x11f0
>         pci_iommu_init+0x13/0x70
>         do_one_initcall+0x62/0x3f0
>         kernel_init_freeable+0x3da/0x6a0
>         kernel_init+0x1b/0x200
>         ret_from_fork+0x44/0x70
>         ret_from_fork_asm+0x1a/0x30
> 
>  -> #5 (dmar_global_lock){++++}-{3:3}:
>         down_read+0x43/0x1d0
>         enable_drhd_fault_handling+0x21/0x110
>         cpuhp_invoke_callback+0x4c6/0x870
>         cpuhp_issue_call+0xbf/0x1f0
>         __cpuhp_setup_state_cpuslocked+0x111/0x320
>         __cpuhp_setup_state+0xb0/0x220
>         irq_remap_enable_fault_handling+0x3f/0xa0
>         apic_intr_mode_init+0x5c/0x110
>         x86_late_time_init+0x24/0x40
>         start_kernel+0x895/0xbd0
>         x86_64_start_reservations+0x18/0x30
>         x86_64_start_kernel+0xbf/0x110
>         common_startup_64+0x13e/0x141
> 
>  -> #4 (cpuhp_state_mutex){+.+.}-{3:3}:
>         __mutex_lock+0xb4/0xe40
>         mutex_lock_nested+0x1b/0x30
>         __cpuhp_setup_state_cpuslocked+0x67/0x320
>         __cpuhp_setup_state+0xb0/0x220
>         page_alloc_init_cpuhp+0x2d/0x60
>         mm_core_init+0x18/0x2c0
>         start_kernel+0x576/0xbd0
>         x86_64_start_reservations+0x18/0x30
>         x86_64_start_kernel+0xbf/0x110
>         common_startup_64+0x13e/0x141
> 
>  -> #3 (cpu_hotplug_lock){++++}-{0:0}:
>         __cpuhp_state_add_instance+0x4f/0x220
>         iova_domain_init_rcaches+0x214/0x280
>         iommu_setup_dma_ops+0x1a4/0x710
>         iommu_device_register+0x17d/0x260
>         intel_iommu_init+0xda4/0x11f0
>         pci_iommu_init+0x13/0x70
>         do_one_initcall+0x62/0x3f0
>         kernel_init_freeable+0x3da/0x6a0
>         kernel_init+0x1b/0x200
>         ret_from_fork+0x44/0x70
>         ret_from_fork_asm+0x1a/0x30
> 
>  -> #2 (&domain->iova_cookie->mutex){+.+.}-{3:3}:
>         __mutex_lock+0xb4/0xe40
>         mutex_lock_nested+0x1b/0x30
>         iommu_setup_dma_ops+0x16b/0x710
>         iommu_device_register+0x17d/0x260
>         intel_iommu_init+0xda4/0x11f0
>         pci_iommu_init+0x13/0x70
>         do_one_initcall+0x62/0x3f0
>         kernel_init_freeable+0x3da/0x6a0
>         kernel_init+0x1b/0x200
>         ret_from_fork+0x44/0x70
>         ret_from_fork_asm+0x1a/0x30
> 
>  -> #1 (&group->mutex){+.+.}-{3:3}:
>         __mutex_lock+0xb4/0xe40
>         mutex_lock_nested+0x1b/0x30
>         __iommu_probe_device+0x24c/0x4e0
>         probe_iommu_group+0x2b/0x50
>         bus_for_each_dev+0x7d/0xe0
>         iommu_device_register+0xe1/0x260
>         intel_iommu_init+0xda4/0x11f0
>         pci_iommu_init+0x13/0x70
>         do_one_initcall+0x62/0x3f0
>         kernel_init_freeable+0x3da/0x6a0
>         kernel_init+0x1b/0x200
>         ret_from_fork+0x44/0x70
>         ret_from_fork_asm+0x1a/0x30
> 
>  -> #0 (iommu_probe_device_lock){+.+.}-{3:3}:
>         __lock_acquire+0x1637/0x2810
>         lock_acquire+0xc9/0x300
>         __mutex_lock+0xb4/0xe40
>         mutex_lock_nested+0x1b/0x30
>         iommu_probe_device+0x1d/0x70
>         intel_iommu_init+0xe90/0x11f0
>         pci_iommu_init+0x13/0x70
>         do_one_initcall+0x62/0x3f0
>         kernel_init_freeable+0x3da/0x6a0
>         kernel_init+0x1b/0x200
>         ret_from_fork+0x44/0x70
>         ret_from_fork_asm+0x1a/0x30
> 
>  other info that might help us debug this:
> 
>  Chain exists of:
>    iommu_probe_device_lock --> dmar_global_lock -->
>      &device->physical_node_lock
> 
>   Possible unsafe locking scenario:
> 
>         CPU0                    CPU1
>         ----                    ----
>    lock(&device->physical_node_lock);
>                                 lock(dmar_global_lock);
>                                 lock(&device->physical_node_lock);
>    lock(iommu_probe_device_lock);
> 
>   *** DEADLOCK ***
> 
> This driver uses a global lock to protect the list of enumerated DMA
> remapping units. It is necessary due to the driver's support for dynamic
> addition and removal of remapping units at runtime.
> 
> Two distinct code paths require iteration over this remapping unit list:
> 
> - Device registration and probing: the driver iterates the list to
>   register each remapping unit with the upper layer IOMMU framework
>   and subsequently probe the devices managed by that unit.
> - Global configuration: Upper layer components may also iterate the list
>   to apply configuration changes.
> 
> The lock acquisition order between these two code paths was reversed. This
> caused lockdep warnings, indicating a risk of deadlock. Fix this warning by
> releasing the global lock before invoking upper layer interfaces for device
> registration.
> 


Tested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@...el.com>


> Fixes: b150654f74bf ("iommu/vt-d: Fix suspicious RCU usage")
> Closes: https://lore.kernel.org/linux-
> iommu/SJ1PR11MB612953431F94F18C954C4A9CB9D32@...PR11MB6129.na
> mprd11.prod.outlook.com/
> Cc: stable@...r.kernel.org
> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 85aa66ef4d61..ec2f385ae25b 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3049,6 +3049,7 @@ static int __init
> probe_acpi_namespace_devices(void)
>  			if (dev->bus != &acpi_bus_type)
>  				continue;
> 
> +			up_read(&dmar_global_lock);
>  			adev = to_acpi_device(dev);
>  			mutex_lock(&adev->physical_node_lock);
>  			list_for_each_entry(pn,
> @@ -3058,6 +3059,7 @@ static int __init
> probe_acpi_namespace_devices(void)
>  					break;
>  			}
>  			mutex_unlock(&adev->physical_node_lock);
> +			down_read(&dmar_global_lock);
> 
>  			if (ret)
>  				return ret;
> --
> 2.43.0


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ