linux-kernel - Re: [PATCH v4 07/13] iommu: Add iommu_at[de]tach_device

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: <254d6e52-0644-6600-8f30-5331ed961298@linux.intel.com>
Date:   Fri, 24 Dec 2021 14:44:24 +0800
From:   Lu Baolu <baolu.lu@...ux.intel.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     baolu.lu@...ux.intel.com, Robin Murphy <robin.murphy@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Joerg Roedel <joro@...tes.org>,
        Alex Williamson <alex.williamson@...hat.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Christoph Hellwig <hch@...radead.org>,
        Kevin Tian <kevin.tian@...el.com>,
        Ashok Raj <ashok.raj@...el.com>, Will Deacon <will@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>, rafael@...nel.org,
        Diana Craciun <diana.craciun@....nxp.com>,
        Cornelia Huck <cohuck@...hat.com>,
        Eric Auger <eric.auger@...hat.com>,
        Liu Yi L <yi.l.liu@...el.com>,
        Jacob jun Pan <jacob.jun.pan@...el.com>,
        Chaitanya Kulkarni <kch@...dia.com>,
        Stuart Yoder <stuyoder@...il.com>,
        Laurentiu Tudor <laurentiu.tudor@....com>,
        Thierry Reding <thierry.reding@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Li Yang <leoyang.li@....com>,
        Dmitry Osipenko <digetx@...il.com>,
        iommu@...ts.linux-foundation.org, linux-pci@...r.kernel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 07/13] iommu: Add iommu_at[de]tach_device_shared() for
 multi-device groups

Hi Jason,

On 2021/12/24 10:50, Jason Gunthorpe wrote:
> On Fri, Dec 24, 2021 at 09:30:17AM +0800, Lu Baolu wrote:
>> Hi Jason,
>>
>> On 12/23/21 10:03 PM, Jason Gunthorpe wrote:
>>>>> I think it would be clear why iommu_group_set_dma_owner(), which
>>>>> actually does detatch, is not the same thing as iommu_attach_device().
>>>> iommu_device_set_dma_owner() will eventually call
>>>> iommu_group_set_dma_owner(). I didn't get why
>>>> iommu_group_set_dma_owner() is special and need to keep.
>>> Not quite, they would not call each other, they have different
>>> implementations:
>>>
>>> int iommu_device_use_dma_api(struct device *device)
>>> {
>>> 	struct iommu_group *group = device->iommu_group;
>>>
>>> 	if (!group)
>>> 		return 0;
>>>
>>> 	mutex_lock(&group->mutex);
>>> 	if (group->owner_cnt != 0 ||
>>> 	    group->domain != group->default_domain) {
>>> 		mutex_unlock(&group->mutex);
>>> 		return -EBUSY;
>>> 	}
>>> 	group->owner_cnt = 1;
>>> 	group->owner = NULL;
>>> 	mutex_unlock(&group->mutex);
>>> 	return 0;
>>> }
>> It seems that this function doesn't work for multi-device groups. When
>> the user unbinds all native drivers from devices in the group and start
>> to bind them with vfio-pci and assign them to user, how could iommu know
>> whether the group is viable for user?
> It is just a mistake, I made this very fast. It should work as your
> patch had it with a ++. More like this:
> 
> int iommu_device_use_dma_api(struct device *device)
> {
> 	struct iommu_group *group = device->iommu_group;
> 
> 	if (!group)
> 		return 0;
> 
> 	mutex_lock(&group->mutex);
> 	if (group->owner_cnt != 0) {
> 		if (group->domain != group->default_domain ||
> 		    group->owner != NULL) {
> 			mutex_unlock(&group->mutex);
> 			return -EBUSY;
> 		}
> 	}
> 	group->owner_cnt++;
> 	mutex_unlock(&group->mutex);
> 	return 0;
> }
> 
>>> See, we get rid of the enum as a multiplexor parameter, each API does
>>> only wnat it needs, they don't call each other.
>> I like the idea of removing enum parameter and make the API name
>> specific. But I didn't get why they can't call each other even the
>> data in group is the same.
> Well, I think when you type them out you'll find they don't work the
> same. Ie the iommu_group_set_dma_owner() does __iommu_detach_group()
> which iommu_device_use_dma_api() definately doesn't want to
> do. iommu_device_use_dma_api() checks the domain while
> iommu_group_set_dma_owner() must not.
> 
> This is basically the issue, all the places touching ownercount are
> superficially the same but each use different predicates. Given the
> predicate is more than half the code I wouldn't try to share the rest
> of it. But maybe when it is all typed in something will become
> obvious?
> 

Get you and agree with you. For the remaining comments, let me wait and
listen what Robin will comment.

Best regards,
baolu