linux-kernel - Re: [PATCH v1 3/6] iommu/vt-d: Refactor iommu information of each domain

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]

Date:   Fri, 1 Jul 2022 10:35:50 +0800
From:   Lu Baolu <baolu.lu@...ux.intel.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>,
        Joerg Roedel <joro@...tes.org>, Steve Wahl <steve.wahl@....com>
Cc:     baolu.lu@...ux.intel.com, David Woodhouse <dwmw2@...radead.org>,
        Jerry Snitselaar <jsnitsel@...hat.com>,
        Mike Travis <mike.travis@....com>,
        Dimitri Sivanich <sivanich@....com>,
        "Anderson, Russ" <russ.anderson@....com>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 3/6] iommu/vt-d: Refactor iommu information of each
 domain

On 6/30/22 4:28 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@...ux.intel.com>
>> Sent: Saturday, June 25, 2022 8:52 PM
>>
>> +struct iommu_domain_info {
>> +	struct intel_iommu *iommu;
>> +	unsigned int refcnt;
>> +	u16 did;
>> +};
>> +
>>   struct dmar_domain {
>>   	int	nid;			/* node id */
>> -
>> -	unsigned int iommu_refcnt[DMAR_UNITS_SUPPORTED];
>> -					/* Refcount of devices per iommu */
>> -
>> -
>> -	u16		iommu_did[DMAR_UNITS_SUPPORTED];
>> -					/* Domain ids per IOMMU. Use u16
>> since
>> -					 * domain ids are 16 bit wide
>> according
>> -					 * to VT-d spec, section 9.3 */
> 
> It'd make sense to keep the comments when moving above fields.

Sure. Updated.

diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 56c3d1a9e155..fae45bbb0c7f 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -527,8 +527,10 @@ struct context_entry {

  struct iommu_domain_info {
         struct intel_iommu *iommu;
-       unsigned int refcnt;
-       u16 did;
+       unsigned int refcnt;            /* Refcount of devices per iommu */
+       u16 did;                        /* Domain ids per IOMMU. Use u16 
since
+                                        * domain ids are 16 bit wide 
according
+                                        * to VT-d spec, section 9.3 */
  };


> 
>> +	spin_lock(&iommu->lock);
>> +	curr = xa_load(&domain->iommu_array, iommu->seq_id);
>> +	if (curr) {
>> +		curr->refcnt++;
>> +		kfree(info);
>> +		goto success;
> 
> Not a fan of adding a label for success. Just putting two lines (unlock+
> return) here is clearer.

Updated.

> 
>> +	ret = xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
>> +			      info, GFP_ATOMIC));
> 
> check xa_err in separate line.

Sure. Updated as below:

@@ -1778,13 +1780,14 @@ static int domain_attach_iommu(struct 
dmar_domain *domain,
         info->did       = num;
         info->iommu     = iommu;
         domain->nid     = iommu->node;
-       ret = xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
-                             info, GFP_ATOMIC));
-       if (ret)
+       curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
+                         NULL, info, GFP_ATOMIC);
+       if (curr) {
+               ret = xa_err(curr) ? : -EBUSY;
                 goto err_clear;
+       }
         domain_update_iommu_cap(domain);

> 
>>
>>   static void domain_detach_iommu(struct dmar_domain *domain,
>>   				struct intel_iommu *iommu)
>>   {
>> -	int num;
>> +	struct iommu_domain_info *info;
>>
>>   	spin_lock(&iommu->lock);
>> -	domain->iommu_refcnt[iommu->seq_id] -= 1;
>> -	if (domain->iommu_refcnt[iommu->seq_id] == 0) {
>> -		num = domain->iommu_did[iommu->seq_id];
>> -		clear_bit(num, iommu->domain_ids);
>> +	info = xa_load(&domain->iommu_array, iommu->seq_id);
>> +	if (--info->refcnt == 0) {
>> +		clear_bit(info->did, iommu->domain_ids);
>> +		xa_erase(&domain->iommu_array, iommu->seq_id);
>>   		domain_update_iommu_cap(domain);
>> -		domain->iommu_did[iommu->seq_id] = 0;
>> +		kfree(info);
> 
> domain->nid may still point to the node of the removed iommu.

Good catch! I should assign it to NUMA_NO_NODE, so that it could be
updated in the next domain_update_iommu_cap(). Updated as below:

@@ -1806,6 +1809,7 @@ static void domain_detach_iommu(struct dmar_domain 
*domain,
         if (--info->refcnt == 0) {
                 clear_bit(info->did, iommu->domain_ids);
                 xa_erase(&domain->iommu_array, iommu->seq_id);
+               domain->nid = NUMA_NO_NODE;
                 domain_update_iommu_cap(domain);
                 kfree(info);
         }

Best regards,
baolu