linux-kernel - RE: [PATCH v6 02/10] iommu: Remove sva handle list

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: <BN9PR11MB527693E470478D92564A31718CFB2@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Fri, 7 Jun 2024 09:35:23 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>, Jason Gunthorpe <jgg@...pe.ca>,
	"Joerg Roedel" <joro@...tes.org>, Will Deacon <will@...nel.org>, Robin Murphy
	<robin.murphy@....com>, Jean-Philippe Brucker <jean-philippe@...aro.org>,
	Nicolin Chen <nicolinc@...dia.com>, "Liu, Yi L" <yi.l.liu@...el.com>, "Jacob
 Pan" <jacob.jun.pan@...ux.intel.com>, Joel Granados <j.granados@...sung.com>
CC: "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"virtualization@...ts.linux-foundation.org"
	<virtualization@...ts.linux-foundation.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v6 02/10] iommu: Remove sva handle list

> From: Baolu Lu <baolu.lu@...ux.intel.com>
> Sent: Thursday, June 6, 2024 2:07 PM
> 
> On 6/5/24 4:15 PM, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@...ux.intel.com>
> >> Sent: Monday, May 27, 2024 12:05 PM
> >>
> >> -	list_for_each_entry(handle, &mm->iommu_mm->sva_handles,
> >> handle_item) {
> >> -		if (handle->dev == dev) {
> >> -			refcount_inc(&handle->users);
> >> -			mutex_unlock(&iommu_sva_lock);
> >> -			return handle;
> >> -		}
> >> +	/* A bond already exists, just take a reference`. */
> >> +	attach_handle = iommu_attach_handle_get(group, iommu_mm-
> >>> pasid, IOMMU_DOMAIN_SVA);
> >> +	if (!IS_ERR(attach_handle)) {
> >> +		handle = container_of(attach_handle, struct iommu_sva,
> >> handle);
> >> +		refcount_inc(&handle->users);
> >> +		mutex_unlock(&iommu_sva_lock);
> >> +		return handle;
> >>   	}
> >
> > It's counter-intuitive to move forward when an error is returned.
> >
> > e.g. if it's -EBUSY indicating the pasid already used for another type then
> > following attempts shouldn't been tried.
> >
> > probably we should have iommu_attach_handle_get() return NULL
> > instead of -ENOENT when the entry is free? then:
> >
> > 	attach_handle = iommu_attach_handle_get();
> > 	if (IS_ERR(attach_handle)) {
> > 		ret = PTR_ERR(attach_handle);
> > 		goto out_unlock;
> > 	} else if (attach_handle) {
> > 		/* matched and increase handle->users */
> > 	}
> >
> > 	/* free entry falls through */
> > But then there is one potential issue with the design that 'handle'
> > can be optional in iommu_attach_device_pasid(). In that case
> > xa_load returns NULL then we cannot differentiate a real unused
> > PASID vs. one which has been attached w/o an handle.
> 
> The PASID should be allocated exclusively. This means that once a PASID
> is assigned to A, it shouldn't be assigned to B at the same time. If a
> single PASID is used for multiple purposes, it's likely a bug in the
> system.

yes there is a bug but catching it here would make diagnostic easier.

> 
> So the logic of iommu_attach_handle_get() here is: has an SVA domain
> already been installed for this PASID? If so, just reuse it. Otherwise,
> try to install a new SVA domain.
> 
> > Does it suggest that having the caller to always provide a handle
> > makes more sense?
> 
> I'm neutral on this since only sva bind and iopf path delivery currently
> require an attach handle.
> 

let's hear Jason's opinion.