linux-kernel - Re: [PATCH v2] iommu/vt-d: fix system hang on reboot -f

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <0691a295-0883-47b3-84a6-47d9a94af69a@linux.intel.com>
Date: Tue, 25 Feb 2025 15:01:38 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: Yunhui Cui <cuiyunhui@...edance.com>, dwmw2@...radead.org,
 joro@...tes.org, will@...nel.org, robin.murphy@....com,
 iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Cc: baolu.lu@...ux.intel.com, Ethan Zhao <haifeng.zhao@...ux.intel.com>
Subject: Re: [PATCH v2] iommu/vt-d: fix system hang on reboot -f

On 2025/2/25 14:48, Yunhui Cui wrote:
> We found that executing the command ./a.out &;reboot -f (where a.out is a
> program that only executes a while(1) infinite loop) can probabilistically
> cause the system to hang in the intel_iommu_shutdown() function, rendering
> it unresponsive. Through analysis, we identified that the factors
> contributing to this issue are as follows:
> 
> 1. The reboot -f command does not prompt the kernel to notify the
> application layer to perform cleanup actions, allowing the application to
> continue running.
> 
> 2. When the kernel reaches the intel_iommu_shutdown() function, only the
> BSP (Bootstrap Processor) CPU is operational in the system.
> 
> 3. During the execution of intel_iommu_shutdown(), the function down_write
> (&dmar_global_lock) causes the process to sleep and be scheduled out.
> 
> 4. At this point, though the processor's interrupt flag is not cleared,
>   allowing interrupts to be accepted. However, only legacy devices and NMI
> (Non-Maskable Interrupt) interrupts could come in, as other interrupts
> routing have already been disabled. If no legacy or NMI interrupts occur
> at this stage, the scheduler will not be able to run.
> 
> 5. If the application got scheduled at this time is executing a while(1)-
> type loop, it will be unable to be preempted, leading to an infinite loop
> and causing the system to become unresponsive.
> 
> To resolve this issue, the intel_iommu_shutdown() function should not
> execute down_write(), which can potentially cause the process to be
> scheduled out. Furthermore, since only the BSP is running during the later
> stages of the reboot, there is no need for protection against parallel
> access to the DMAR (DMA Remapping) unit. Therefore, the following lines
> could be removed:

Good summary! Thank you!

> 
> down_write(&dmar_global_lock);
> up_write(&dmar_global_lock);
> 
> After testing, the issue has been resolved.
> 
> Fixes: 6c3a44ed3c55 ("iommu/vt-d: Turn off translations at shutdown")
> Co-developed-by: Ethan Zhao <haifeng.zhao@...ux.intel.com>
> Signed-off-by: Ethan Zhao <haifeng.zhao@...ux.intel.com>
> Signed-off-by: Yunhui Cui <cuiyunhui@...edance.com>
> ---
>   drivers/iommu/intel/iommu.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index cc46098f875b..6d9f2e56ce88 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -2871,16 +2871,12 @@ void intel_iommu_shutdown(void)
>   	if (no_iommu || dmar_disabled)
>   		return;
>   
> -	down_write(&dmar_global_lock);
> -
>   	/* Disable PMRs explicitly here. */
>   	for_each_iommu(iommu, drhd)

Removing the locking for for_each_iommu() will trigger a suspicious RCU
usage splat. You need to replace this helper with a raw
list_for_each_entry() with some comments around it to explain why it is
safe.

>   		iommu_disable_protect_mem_regions(iommu);
>   
>   	/* Make sure the IOMMUs are switched off */
>   	intel_disable_iommus();
> -
> -	up_write(&dmar_global_lock);
>   }
>   
>   static struct intel_iommu *dev_to_intel_iommu(struct device *dev)

Thanks,
baolu