linux-kernel - Re: [PATCH v6 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: <05a68e1e-8e18-5914-ebe7-d7b1a4aaa2ec@linux.intel.com>
Date:   Thu, 12 May 2022 11:02:39 +0800
From:   Baolu Lu <baolu.lu@...ux.intel.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     baolu.lu@...ux.intel.com, Joerg Roedel <joro@...tes.org>,
        Christoph Hellwig <hch@...radead.org>,
        Kevin Tian <kevin.tian@...el.com>,
        Ashok Raj <ashok.raj@...el.com>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Jean-Philippe Brucker <jean-philippe@...aro.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Vinod Koul <vkoul@...nel.org>,
        Eric Auger <eric.auger@...hat.com>,
        Liu Yi L <yi.l.liu@...el.com>,
        Jacob jun Pan <jacob.jun.pan@...el.com>,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA
 interfaces

On 2022/5/11 22:53, Jason Gunthorpe wrote:
>>> Assuming we leave room for multi-device groups this logic should just
>>> be
>>>
>>> 	group = iommu_group_get(dev);
>>> 	if (!group)
>>> 		return -ENODEV;
>>>
>>> 	mutex_lock(&group->mutex);
>>> 	domain = xa_load(&group->pasid_array, mm->pasid);
>>> 	if (!domain || domain->type != IOMMU_DOMAIN_SVA || domain->mm != mm)
>>> 		domain = iommu_sva_alloc_domain(dev, mm);
>>>
>>> ?
>> Agreed. As a helper in iommu core, how about making it more generic like
>> below?
> IDK, is there more users of this? AFAIK SVA is the only place that
> will be auto-sharing?

The generic thing is that components, like SVA, want to fetch the
attached domain from the iommu core.

> 
>> +       mutex_lock(&group->mutex);
>> +       domain = xa_load(&group->pasid_array, pasid);
>> +       if (domain && domain->type != type)
>> +               domain = NULL;
>> +       mutex_unlock(&group->mutex);
>> +       iommu_group_put(group);
>> +
>> +       return domain;
> This is bad locking, group->pasid_array values cannot be taken outside
> the lock.

It's not iommu core, but SVA (or other feature components) that manage
the life cycle of a domain. The iommu core only provides a place to
store the domain pointer. The feature components are free to fetch their
domain pointers from iommu core as long as they are sure that the domain
is alive during use.

> 
>>> And stick the refcount in the sva_domain
>>>
>>> Also, given the current arrangement it might make sense to have a
>>> struct iommu_domain_sva given that no driver is wrappering this in
>>> something else.
>> Fair enough. How about below wrapper?
>>
>> +struct iommu_sva_domain {
>> +       /*
>> +        * Common iommu domain header,*must*  be put at the top
>> +        * of the structure.
>> +        */
>> +       struct iommu_domain domain;
>> +       struct mm_struct *mm;
>> +       struct iommu_sva bond;
>> +}
>>
>> The refcount is wrapped in bond.
> I'm still not sure that bond is necessary

"bond" is the sva handle that the device drivers get through calling
iommu_sva_bind().

> 
> But yes, something like that

Best regards,
baolu