linux-kernel - Re: [PATCH RFC v3 08/12] iommu/sva: Use attach/detach_pasid

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: <1289f780-143e-004c-41ac-d95b6f18d63c@linux.intel.com>
Date:   Tue, 12 Apr 2022 20:53:11 +0800
From:   Lu Baolu <baolu.lu@...ux.intel.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>,
        Joerg Roedel <joro@...tes.org>,
        Jason Gunthorpe <jgg@...dia.com>,
        Christoph Hellwig <hch@...radead.org>,
        "Raj, Ashok" <ashok.raj@...el.com>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Jean-Philippe Brucker <jean-philippe@...aro.com>
Cc:     baolu.lu@...ux.intel.com, Eric Auger <eric.auger@...hat.com>,
        "Liu, Yi L" <yi.l.liu@...el.com>,
        "Pan, Jacob jun" <jacob.jun.pan@...el.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC v3 08/12] iommu/sva: Use attach/detach_pasid_dev in
 SVA interfaces

On 2022/4/12 15:19, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@...ux.intel.com>
>> Sent: Sunday, April 10, 2022 6:25 PM
>> +struct iommu_sva *
>> +iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void
>> *drvdata)
>> +{
>> +	int ret = -EINVAL;
>> +	struct iommu_sva *handle;
>> +	struct iommu_domain *domain;
>> +	struct iommu_sva_ioas *ioas;
>> +
>> +	/*
>> +	 * TODO: Remove the drvdata parameter after kernel PASID support
>> is
>> +	 * enabled for the idxd driver.
>> +	 */
>> +	if (drvdata)
>> +		return ERR_PTR(-EOPNOTSUPP);
>> +
>> +	/* Allocate mm->pasid if necessary. */
>> +	ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits)
>> - 1);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	mutex_lock(&iommu_sva_lock);
>> +	ioas = iommu_sva_ioas_get(mm, mm->pasid);
>> +	if (!ioas) {
>> +		ret = -ENOMEM;
> 
> ioas can be NULL for multiple reasons, e.g. :
> 
> 1) ioas->mm != mm;
> 2) kzalloc failure;
> 3) xa_store failure;
> 
> It's more sensible to return error from iommu_sva_ioas_get() directly.

Fair enough.

> 
>> +		goto out_unlock;
>> +	}
>> +
>> +	/* Search for an existing bond. */
>> +	list_for_each_entry(handle, &ioas->bonds, node) {
>> +		if (handle->dev == dev) {
>> +			refcount_inc(&handle->users);
>> +			/* No new bond, drop the counter. */
>> +			iommu_sva_ioas_put(ioas);
>> +			goto out_success;
>> +		}
>> +	}
>> +
>> +	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> 
> s/handle/bond/?

"handle" is used in the previous implementation but "bond" looks better
to read. :-)

> 
>> +	if (!handle) {
>> +		ret = -ENOMEM;
>> +		goto out_put_ioas;
>> +	}
>> +
>> +	/* The reference to ioas will be kept until domain free. */
>> +	domain = iommu_sva_alloc_domain(dev, ioas);
> 
> Shouldn't we first try whether existing domains are compatible to this
> device?

If we think that here domain represents a hardware pagetable actually
used by IOMMU for a {device, pasid}, we are able to use per-{device,
pasid} domain without checking compatibility. Sharing a domain among
devices under the same IOMMU may be an optimization. That could be done
in the IOMMU driver just like what vt-d driver is doing for pass-through
DMA domains.

> 
>> @@ -1952,6 +1954,7 @@ EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>>   void iommu_domain_free(struct iommu_domain *domain)
>>   {
>>   	iommu_put_dma_cookie(domain);
>> +	iommu_sva_ioas_put(domain->sva_ioas);
>>   	domain->ops->free(domain);
>>   }
> 
> is it good to have general iommu_domain_free() to always call a put()
> function for a specific domain type? Why cannot it be done before
> calling iommu_domain_free() in sva-lib.c?

It's better to have a generic free() helper since an sva domain could be
freed outside of iommu sva code as you can see in the following patches.

Best regards,
baolu