linux-kernel - RE: [PATCH v6 01/10] iommu: Introduce domain attachment handle

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: <BN9PR11MB52767B3B1F441D9AA656AC258CF92@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Wed, 5 Jun 2024 08:02:38 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Lu Baolu <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>, Jason Gunthorpe <jgg@...dia.com>
Subject: RE: [PATCH v6 01/10] iommu: Introduce domain attachment handle

> From: Lu Baolu <baolu.lu@...ux.intel.com>
> Sent: Monday, May 27, 2024 12:05 PM
> 
> @@ -99,7 +99,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device
> *dev, struct mm_struct *mm
> 
>  	/* Search for an existing domain. */
>  	list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) {
> -		ret = iommu_attach_device_pasid(domain, dev, iommu_mm-
> >pasid);
> +		handle->handle.domain = domain;
> +		ret = iommu_attach_device_pasid(domain, dev, iommu_mm-
> >pasid,
> +						&handle->handle);

move the setting of handle.domain into the helper?

> @@ -3382,11 +3383,9 @@ int iommu_attach_device_pasid(struct
> iommu_domain *domain,
>  		}
>  	}
> 
> -	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
> GFP_KERNEL);
> -	if (curr) {
> -		ret = xa_err(curr) ? : -EBUSY;
> +	ret = xa_insert(&group->pasid_array, pasid, handle, GFP_KERNEL);
> +	if (ret)
>  		goto out_unlock;
> -	}

this leads to a slightly different implication comparing to the old code.

the domain pointer was always tracked in the old code but now the handle
is optional.

if iommu core only needs to know whether a pasid has been attached (as
in this helper), it still works as xa_insert() will mark the entry as reserved
if handle==NULL and next xa_insert() at the same index will fail due to
the entry being reserved.

But if certain path (other than iopf) in the iommu core needs to know
the exact domain pointer then this change breaks it.

Anyway some explanation is welcomed here for why this change is safe.

> @@ -3414,7 +3413,7 @@ void iommu_detach_device_pasid(struct
> iommu_domain *domain, struct device *dev,
> 
>  	mutex_lock(&group->mutex);
>  	__iommu_remove_group_pasid(group, pasid, domain);
> -	WARN_ON(xa_erase(&group->pasid_array, pasid) != domain);
> +	xa_erase(&group->pasid_array, pasid);

if the entry is valid do we want to keep the WARN_ON() upon handle->domain?