linux-kernel - Re: [PATCH] iommu/vt-d: Check for non-NULL domain on device release

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 31 Jan 2024 15:10:53 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: Jason Gunthorpe <jgg@...pe.ca>, Eric Badger <ebadger@...estorage.com>
Cc: baolu.lu@...ux.intel.com, David Woodhouse <dwmw2@...radead.org>,
 Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
 Robin Murphy <robin.murphy@....com>,
 "open list:INTEL IOMMU (VT-d)" <iommu@...ts.linux.dev>,
 open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iommu/vt-d: Check for non-NULL domain on device release

On 2024/1/16 23:22, Jason Gunthorpe wrote:
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 6fb5f6fceea1..26e450259889 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -3750,7 +3750,6 @@ static void domain_context_clear(struct device_domain_info *info)
>>   static void dmar_remove_one_dev_info(struct device *dev)
>>   {
>>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
>> -	struct dmar_domain *domain = info->domain;
>>   	struct intel_iommu *iommu = info->iommu;
>>   	unsigned long flags;
>>   
>> @@ -3763,11 +3762,14 @@ static void dmar_remove_one_dev_info(struct device *dev)
>>   		domain_context_clear(info);
>>   	}
>>   
>> -	spin_lock_irqsave(&domain->lock, flags);
>> +	if (!info->domain)
>> +		return;
>> +
>> +	spin_lock_irqsave(&info->domain->lock, flags);
>>   	list_del(&info->link);
>> -	spin_unlock_irqrestore(&domain->lock, flags);
>> +	spin_unlock_irqrestore(&info->domain->lock, flags);
>>   
>> -	domain_detach_iommu(domain, iommu);
>> +	domain_detach_iommu(info->domain, iommu);
>>   	info->domain = NULL;
>>   }
> This function is almost a copy of device_block_translation(), with
> some bugs added :\
> 
> Lu can they be made the same? It seems to me BLOCKED could always
> clear the domain context, even in scalable mode?

Yes. A part of dmar_remove_one_dev_info() is duplicated with
device_block_translation().

> 
> Anyhow, my suggestion is more like:

I like this suggestion.

Currently, the device_release callback for an iommu driver does the
following:

a) Silent IOMMU DMA translation. This is done by detaching the existing
    domain from the IOMMU and bringing the IOMMU into a blocking state
    (some drivers might be in identity state);
b) Releases the resources allocated in the probe callback and restores
    the device to its previous state before the probe.

 From my understanding of your suggestion, we should move a) out of the
release callback and make it a special domain, which could be a blocking
domain or identity domain, depending on the iommu hardware.

In the end, the release_device callback of an iommu driver should focus
on the opposite operation of device_probe. This makes the concept
clearer.

Best regards,
baolu