The struct sva_iommu represents a bond of an SVA domain and a device.
It is functionally equivalent to the iommu_attach_handle. To avoid
code duplication, replace sva_iommu with the iommu_attach_handle and
remove the code that manages sva_iommu.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h      | 28 +++++------------
 include/linux/uacce.h      |  2 +-
 drivers/dma/idxd/idxd.h    |  2 +-
 drivers/dma/idxd/cdev.c    |  4 +--
 drivers/iommu/iommu-sva.c  | 61 ++++++++++++++++----------------------
 drivers/misc/uacce/uacce.c |  2 +-
 6 files changed, 38 insertions(+), 61 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2e925b5eba53..be9c9a10169d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -39,7 +39,6 @@ struct iommu_domain;
 struct iommu_domain_ops;
 struct iommu_dirty_ops;
 struct notifier_block;
-struct iommu_sva;
 struct iommu_dma_cookie;
 struct iommu_fault_param;
 
@@ -986,20 +985,9 @@ struct iommu_fwspec {
 /* ATS is supported */
 #define IOMMU_FWSPEC_PCI_RC_ATS			(1 << 0)
 
-/**
- * struct iommu_sva - handle to a device-mm bond
- */
-struct iommu_sva {
-	struct device			*dev;
-	struct iommu_domain		*domain;
-	struct list_head		handle_item;
-	refcount_t			users;
-};
-
 struct iommu_mm_data {
 	u32			pasid;
 	struct list_head	sva_domains;
-	struct list_head	sva_handles;
 };
 
 int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
@@ -1527,24 +1515,24 @@ static inline u32 mm_get_enqcmd_pasid(struct mm_struct *mm)
 }
 
 void mm_pasid_drop(struct mm_struct *mm);
-struct iommu_sva *iommu_sva_bind_device(struct device *dev,
-					struct mm_struct *mm);
-void iommu_sva_unbind_device(struct iommu_sva *handle);
-u32 iommu_sva_get_pasid(struct iommu_sva *handle);
+struct iommu_attach_handle *iommu_sva_bind_device(struct device *dev,
+						  struct mm_struct *mm);
+void iommu_sva_unbind_device(struct iommu_attach_handle *handle);
+u32 iommu_sva_get_pasid(struct iommu_attach_handle *handle);
 struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
 					    struct mm_struct *mm);
 #else
-static inline struct iommu_sva *
+static inline struct iommu_attach_handle *
 iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
 {
-	return NULL;
+	return ERR_PTR(-ENODEV);
 }
 
-static inline void iommu_sva_unbind_device(struct iommu_sva *handle)
+static inline void iommu_sva_unbind_device(struct iommu_attach_handle *handle)
 {
 }
 
-static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
+static inline u32 iommu_sva_get_pasid(struct iommu_attach_handle *handle)
 {
 	return IOMMU_PASID_INVALID;
 }
diff --git a/include/linux/uacce.h b/include/linux/uacce.h
index e290c0269944..1548119c89ae 100644
--- a/include/linux/uacce.h
+++ b/include/linux/uacce.h
@@ -97,7 +97,7 @@ struct uacce_queue {
 	struct mutex mutex;
 	enum uacce_q_state state;
 	u32 pasid;
-	struct iommu_sva *handle;
+	struct iommu_attach_handle *handle;
 	struct address_space *mapping;
 };
 
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index a4099a1e2340..3ee89e9cb049 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -335,7 +335,7 @@ struct idxd_device {
 	struct idxd_wq **wqs;
 	struct idxd_engine **engines;
 
-	struct iommu_sva *sva;
+	struct iommu_attach_handle *sva;
 	unsigned int pasid;
 
 	int num_groups;
diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index 8078ab9acfbc..a029bda92615 100644
--- a/drivers/dma/idxd/cdev.c
+++ b/drivers/dma/idxd/cdev.c
@@ -45,7 +45,7 @@ struct idxd_user_context {
 	unsigned int pasid;
 	struct mm_struct *mm;
 	unsigned int flags;
-	struct iommu_sva *sva;
+	struct iommu_attach_handle *sva;
 	struct idxd_dev idxd_dev;
 	u64 counters[COUNTER_MAX];
 	int id;
@@ -225,7 +225,7 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
 	struct idxd_wq *wq;
 	struct device *dev, *fdev;
 	int rc = 0;
-	struct iommu_sva *sva;
+	struct iommu_attach_handle *sva;
 	unsigned int pasid;
 	struct idxd_cdev *idxd_cdev;
 
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 640acc804e8c..35ac2e4836e9 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -41,7 +41,6 @@ static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct de
 	}
 	iommu_mm->pasid = pasid;
 	INIT_LIST_HEAD(&iommu_mm->sva_domains);
-	INIT_LIST_HEAD(&iommu_mm->sva_handles);
 	/*
 	 * Make sure the write to mm->iommu_mm is not reordered in front of
 	 * initialization to iommu_mm fields. If it does, readers may see a
@@ -67,13 +66,17 @@ static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct de
  *
  * On error, returns an ERR_PTR value.
  */
-struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
+struct iommu_attach_handle *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
 {
+	struct iommu_group *group = dev->iommu_group;
+	struct iommu_attach_handle *handle;
 	struct iommu_mm_data *iommu_mm;
 	struct iommu_domain *domain;
-	struct iommu_sva *handle;
 	int ret;
 
+	if (!group)
+		return ERR_PTR(-ENODEV);
+
 	mutex_lock(&iommu_sva_lock);
 
 	/* Allocate mm->pasid if necessary. */
@@ -83,18 +86,11 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 		goto out_unlock;
 	}
 
-	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;
-		}
-	}
-
-	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
-	if (!handle) {
-		ret = -ENOMEM;
-		goto out_unlock;
+	/* A bond already exists, just take a reference`. */
+	handle = iommu_attach_handle_get(group, iommu_mm->pasid);
+	if (handle) {
+		mutex_unlock(&iommu_sva_lock);
+		return handle;
 	}
 
 	/* Search for an existing domain. */
@@ -110,7 +106,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 	domain = iommu_sva_domain_alloc(dev, mm);
 	if (!domain) {
 		ret = -ENOMEM;
-		goto out_free_handle;
+		goto out_unlock;
 	}
 
 	ret = iommu_attach_device_pasid(domain, dev, iommu_mm->pasid);
@@ -120,17 +116,14 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 	list_add(&domain->next, &mm->iommu_mm->sva_domains);
 
 out:
-	refcount_set(&handle->users, 1);
-	list_add(&handle->handle_item, &mm->iommu_mm->sva_handles);
+	handle = iommu_attach_handle_get(group, iommu_mm->pasid);
 	mutex_unlock(&iommu_sva_lock);
-	handle->dev = dev;
-	handle->domain = domain;
+	handle->priv = dev;
+
 	return handle;
 
 out_free_domain:
 	iommu_domain_free(domain);
-out_free_handle:
-	kfree(handle);
 out_unlock:
 	mutex_unlock(&iommu_sva_lock);
 	return ERR_PTR(ret);
@@ -145,30 +138,26 @@ EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
  * not be issuing any more transaction for this PASID. All outstanding page
  * requests for this PASID must have been flushed to the IOMMU.
  */
-void iommu_sva_unbind_device(struct iommu_sva *handle)
+void iommu_sva_unbind_device(struct iommu_attach_handle *handle)
 {
 	struct iommu_domain *domain = handle->domain;
 	struct iommu_mm_data *iommu_mm = domain->mm->iommu_mm;
-	struct device *dev = handle->dev;
+	struct device *dev = handle->priv;
 
 	mutex_lock(&iommu_sva_lock);
-	if (!refcount_dec_and_test(&handle->users)) {
-		mutex_unlock(&iommu_sva_lock);
-		return;
-	}
-	list_del(&handle->handle_item);
-
-	iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
-	if (--domain->users == 0) {
-		list_del(&domain->next);
-		iommu_domain_free(domain);
+	iommu_attach_handle_put(handle);
+	if (refcount_read(&handle->users) == 1) {
+		iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
+		if (--domain->users == 0) {
+			list_del(&domain->next);
+			iommu_domain_free(domain);
+		}
 	}
 	mutex_unlock(&iommu_sva_lock);
-	kfree(handle);
 }
 EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);
 
-u32 iommu_sva_get_pasid(struct iommu_sva *handle)
+u32 iommu_sva_get_pasid(struct iommu_attach_handle *handle)
 {
 	struct iommu_domain *domain = handle->domain;
 
diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index bdc2e6fda782..b325097421c1 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -106,7 +106,7 @@ static long uacce_fops_compat_ioctl(struct file *filep,
 static int uacce_bind_queue(struct uacce_device *uacce, struct uacce_queue *q)
 {
 	u32 pasid;
-	struct iommu_sva *handle;
+	struct iommu_attach_handle *handle;
 
 	if (!(uacce->flags & UACCE_DEV_SVA))
 		return 0;
-- 
2.34.1

On Wed, Apr 03, 2024 at 09:15:12AM +0800, Lu Baolu wrote:
> +	/* A bond already exists, just take a reference`. */
> +	handle = iommu_attach_handle_get(group, iommu_mm->pasid);
> +	if (handle) {
> +		mutex_unlock(&iommu_sva_lock);
> +		return handle;
>  	}

At least in this context this is not enough we need to ensure that the
domain on the PASID is actually an SVA domain and it was installed by
this mechanism, not an iommufd domain for instance.

ie you probably need a type field in the iommu_attach_handle to tell
what the priv is.

Otherwise this seems like a great idea!

> -	iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
> -	if (--domain->users == 0) {
> -		list_del(&domain->next);
> -		iommu_domain_free(domain);
> +	iommu_attach_handle_put(handle);
> +	if (refcount_read(&handle->users) == 1) {
> +		iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
> +		if (--domain->users == 0) {
> +			list_del(&domain->next);
> +			iommu_domain_free(domain);
> +		}
>  	}

Though I'm not convinced the refcount should be elevated into the core
structure. The prior patch I showed you where the caller can provide
the memory for the handle and we don't have a priv would make it easy
to put the refcount in a SVA dervied handle struct without more
allocation. Then we don't need this weirdness.

>  	mutex_unlock(&iommu_sva_lock);
> -	kfree(handle);

Also do we need iommu_sva_lock here anymore? I wonder if the group
mutex would be sufficient..

Jason

On 4/3/24 7:59 PM, Jason Gunthorpe wrote:
>> -	iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
>> -	if (--domain->users == 0) {
>> -		list_del(&domain->next);
>> -		iommu_domain_free(domain);
>> +	iommu_attach_handle_put(handle);
>> +	if (refcount_read(&handle->users) == 1) {
>> +		iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
>> +		if (--domain->users == 0) {
>> +			list_del(&domain->next);
>> +			iommu_domain_free(domain);
>> +		}
>>   	}
> Though I'm not convinced the refcount should be elevated into the core
> structure. The prior patch I showed you where the caller can provide
> the memory for the handle and we don't have a priv would make it easy
> to put the refcount in a SVA dervied handle struct without more
> allocation. Then we don't need this weirdness.

It's fine to move the refcount out of the core and allow the caller to
specify and manage its own attach handler. The refcount would then be
managed by the SVA code.

For the IOMMUFD case, we've discussed that all outstanding iopf's
should be automatically responded in the detach process. This ensures
the attach handle won't be used once the detach process completes.
Therefore, if this is true, there appears to be no need for a refcount
for IOMMUFD.

> 
>>   	mutex_unlock(&iommu_sva_lock);
>> -	kfree(handle);
> Also do we need iommu_sva_lock here anymore? I wonder if the group
> mutex would be sufficient..

The iommu_sva_lock protects the whole process of a mm binding, from
pasid allocation to domain attachment. While the group mutex only
protects the data within it structure. I don't think we could replace
iommu_sva_lock with group mutex in this patch. Or any misunderstanding?

Best regards,
baolu

On 4/3/24 7:59 PM, Jason Gunthorpe wrote:
> On Wed, Apr 03, 2024 at 09:15:12AM +0800, Lu Baolu wrote:
>> +	/* A bond already exists, just take a reference`. */
>> +	handle = iommu_attach_handle_get(group, iommu_mm->pasid);
>> +	if (handle) {
>> +		mutex_unlock(&iommu_sva_lock);
>> +		return handle;
>>   	}
> At least in this context this is not enough we need to ensure that the
> domain on the PASID is actually an SVA domain and it was installed by
> this mechanism, not an iommufd domain for instance.
> 
> ie you probably need a type field in the iommu_attach_handle to tell
> what the priv is.
> 
> Otherwise this seems like a great idea!

Yes, you are right. For the SVA case, I will add the following changes.
The IOMMUFD path will also need such enhancement. I will update it in
the next version.

diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index 08c0667cef54..9aee70f87a21 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -28,9 +28,22 @@ void iommu_device_unregister_bus(struct iommu_device 
*iommu,
  				 const struct bus_type *bus,
  				 struct notifier_block *nb);

+enum attach_handle_type {
+	ATTACH_HANDLE_TYPE_DEFAULT = 0,
+	ATTACH_HANDLE_TYPE_SVA,
+	ATTACH_HANDLE_TYPE_IOMMUFD,
+};
+
  struct iommu_attach_handle {
  	struct iommu_domain		*domain;
  	refcount_t			users;
+
+	/*
+	 * Set by the attach interface callers. The type field could be used
+	 * by the caller to identify whether the priv field was installed by
+	 * them.
+	 */
+	enum attach_handle_type		type;
  	void				*priv;
  };

diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index c66cf26137bf..3eb664cc3f3a 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -90,7 +90,11 @@ struct iommu_attach_handle 
*iommu_sva_bind_device(struct device *dev, struct mm_
  	handle = iommu_attach_handle_get(group, iommu_mm->pasid);
  	if (handle) {
  		mutex_unlock(&iommu_sva_lock);
-		return handle;
+		if (handle->type == ATTACH_HANDLE_TYPE_SVA)
+			return handle;
+
+		iommu_attach_handle_put(handle);
+		return ERR_PTR(-EBUSY);
  	}

  	/* Search for an existing domain. */
@@ -118,6 +122,7 @@ struct iommu_attach_handle 
*iommu_sva_bind_device(struct device *dev, struct mm_
  out:
  	handle = iommu_attach_handle_get(group, iommu_mm->pasid);
  	mutex_unlock(&iommu_sva_lock);
+	handle->type = ATTACH_HANDLE_TYPE_SVA;
  	handle->priv = dev;

  	return handle;

Best regards,
baolu

On Sat, Apr 06, 2024 at 02:09:34PM +0800, Baolu Lu wrote:
> On 4/3/24 7:59 PM, Jason Gunthorpe wrote:
> > On Wed, Apr 03, 2024 at 09:15:12AM +0800, Lu Baolu wrote:
> > > +	/* A bond already exists, just take a reference`. */
> > > +	handle = iommu_attach_handle_get(group, iommu_mm->pasid);
> > > +	if (handle) {
> > > +		mutex_unlock(&iommu_sva_lock);
> > > +		return handle;
> > >   	}
> > At least in this context this is not enough we need to ensure that the
> > domain on the PASID is actually an SVA domain and it was installed by
> > this mechanism, not an iommufd domain for instance.
> > 
> > ie you probably need a type field in the iommu_attach_handle to tell
> > what the priv is.
> > 
> > Otherwise this seems like a great idea!
> 
> Yes, you are right. For the SVA case, I will add the following changes.
> The IOMMUFD path will also need such enhancement. I will update it in
> the next version.

The only use for this is the PRI callbacks right? Maybe instead of
adding a handle type let's just check domain->iopf_handler  ?

Ie SVA will pass &ommu_sva_iopf_handler as its "type"

Jason

On 4/8/24 10:19 PM, Jason Gunthorpe wrote:
> On Sat, Apr 06, 2024 at 02:09:34PM +0800, Baolu Lu wrote:
>> On 4/3/24 7:59 PM, Jason Gunthorpe wrote:
>>> On Wed, Apr 03, 2024 at 09:15:12AM +0800, Lu Baolu wrote:
>>>> +	/* A bond already exists, just take a reference`. */
>>>> +	handle = iommu_attach_handle_get(group, iommu_mm->pasid);
>>>> +	if (handle) {
>>>> +		mutex_unlock(&iommu_sva_lock);
>>>> +		return handle;
>>>>    	}
>>> At least in this context this is not enough we need to ensure that the
>>> domain on the PASID is actually an SVA domain and it was installed by
>>> this mechanism, not an iommufd domain for instance.
>>>
>>> ie you probably need a type field in the iommu_attach_handle to tell
>>> what the priv is.
>>>
>>> Otherwise this seems like a great idea!
>> Yes, you are right. For the SVA case, I will add the following changes.
>> The IOMMUFD path will also need such enhancement. I will update it in
>> the next version.
> The only use for this is the PRI callbacks right? Maybe instead of
> adding a handle type let's just check domain->iopf_handler  ?
> 
> Ie SVA will pass &ommu_sva_iopf_handler as its "type"

Sorry that I don't fully understand the proposal here.

We need to get the attach handle at least in below cases:

1. In the iommu_sva_bind_device() path so that the existing bind could
    be reused.

2. In the iommu_report_device_fault() path so that the context-specific
    data could be used in the fault handler.

The problem is that the context code (SVA, IOMMUFD, etc.) needs to make
sure that the attach handle is really what it has installed during
domain attachment. The context code needs some mechanism to include some
kind of "owner cookie" in the attach handle, so that it could check
against it later for valid use.

Best regards,
baolu

On Tue, Apr 09, 2024 at 10:11:28AM +0800, Baolu Lu wrote:
> On 4/8/24 10:19 PM, Jason Gunthorpe wrote:
> > On Sat, Apr 06, 2024 at 02:09:34PM +0800, Baolu Lu wrote:
> > > On 4/3/24 7:59 PM, Jason Gunthorpe wrote:
> > > > On Wed, Apr 03, 2024 at 09:15:12AM +0800, Lu Baolu wrote:
> > > > > +	/* A bond already exists, just take a reference`. */
> > > > > +	handle = iommu_attach_handle_get(group, iommu_mm->pasid);
> > > > > +	if (handle) {
> > > > > +		mutex_unlock(&iommu_sva_lock);
> > > > > +		return handle;
> > > > >    	}
> > > > At least in this context this is not enough we need to ensure that the
> > > > domain on the PASID is actually an SVA domain and it was installed by
> > > > this mechanism, not an iommufd domain for instance.
> > > > 
> > > > ie you probably need a type field in the iommu_attach_handle to tell
> > > > what the priv is.
> > > > 
> > > > Otherwise this seems like a great idea!
> > > Yes, you are right. For the SVA case, I will add the following changes.
> > > The IOMMUFD path will also need such enhancement. I will update it in
> > > the next version.
> > The only use for this is the PRI callbacks right? Maybe instead of
> > adding a handle type let's just check domain->iopf_handler  ?
> > 
> > Ie SVA will pass &ommu_sva_iopf_handler as its "type"
> 
> Sorry that I don't fully understand the proposal here.

I was talking specifically about the type field you suggested adding
to the handle struct.

Instead of adding a type field check the domain->iopf_handler to
determine the domain and thus handle type.

> The problem is that the context code (SVA, IOMMUFD, etc.) needs to make
> sure that the attach handle is really what it has installed during
> domain attachment. The context code needs some mechanism to include some
> kind of "owner cookie" in the attach handle, so that it could check
> against it later for valid use.

Right, you have a derived struct for each user and you need a way to
check if casting from the general handle struct to the derived struct
is OK.

I'm suggesting using domain->iopf_handle as the type key.

Jason

On 2024/4/10 7:48, Jason Gunthorpe wrote:
> On Tue, Apr 09, 2024 at 10:11:28AM +0800, Baolu Lu wrote:
>> On 4/8/24 10:19 PM, Jason Gunthorpe wrote:
>>> On Sat, Apr 06, 2024 at 02:09:34PM +0800, Baolu Lu wrote:
>>>> On 4/3/24 7:59 PM, Jason Gunthorpe wrote:
>>>>> On Wed, Apr 03, 2024 at 09:15:12AM +0800, Lu Baolu wrote:
>>>>>> +	/* A bond already exists, just take a reference`. */
>>>>>> +	handle = iommu_attach_handle_get(group, iommu_mm->pasid);
>>>>>> +	if (handle) {
>>>>>> +		mutex_unlock(&iommu_sva_lock);
>>>>>> +		return handle;
>>>>>>     	}
>>>>> At least in this context this is not enough we need to ensure that the
>>>>> domain on the PASID is actually an SVA domain and it was installed by
>>>>> this mechanism, not an iommufd domain for instance.
>>>>>
>>>>> ie you probably need a type field in the iommu_attach_handle to tell
>>>>> what the priv is.
>>>>>
>>>>> Otherwise this seems like a great idea!
>>>> Yes, you are right. For the SVA case, I will add the following changes.
>>>> The IOMMUFD path will also need such enhancement. I will update it in
>>>> the next version.
>>> The only use for this is the PRI callbacks right? Maybe instead of
>>> adding a handle type let's just check domain->iopf_handler  ?
>>>
>>> Ie SVA will pass &ommu_sva_iopf_handler as its "type"
>> Sorry that I don't fully understand the proposal here.
> I was talking specifically about the type field you suggested adding
> to the handle struct.
> 
> Instead of adding a type field check the domain->iopf_handler to
> determine the domain and thus handle type.
> 
>> The problem is that the context code (SVA, IOMMUFD, etc.) needs to make
>> sure that the attach handle is really what it has installed during
>> domain attachment. The context code needs some mechanism to include some
>> kind of "owner cookie" in the attach handle, so that it could check
>> against it later for valid use.
> Right, you have a derived struct for each user and you need a way to
> check if casting from the general handle struct to the derived struct
> is OK.
> 
> I'm suggesting using domain->iopf_handle as the type key.

After removing the refcount from the attach handle, I am trying to make
the code look like this,

         /* A bond already exists, just take a reference`. */
         handle = iommu_attach_handle_get(group, iommu_mm->pasid);
         if (handle) {
                 if (handle->domain->iopf_handler != 
iommu_sva_iopf_handler) {
                         ret = -EBUSY;
                         goto out_unlock;
                 }

                 refcount_inc(&handle->users);
                 mutex_unlock(&iommu_sva_lock);
                 return handle;
         }

But it appears that this code is not lock safe. If the domain on the
PASID is not a SVA domain, the check of "handle->domain->iopf_handler !=
iommu_sva_iopf_handler" could result in a use-after-free issue as the
other thread might detach the domain in between the fetch and check
lines.

Probably we still need to keep the refcount in the attach handle?

Best regards,
baolu

On Sun, Apr 28, 2024 at 06:22:28PM +0800, Baolu Lu wrote:

>         /* A bond already exists, just take a reference`. */
>         handle = iommu_attach_handle_get(group, iommu_mm->pasid);
>         if (handle) {
>                 if (handle->domain->iopf_handler != iommu_sva_iopf_handler)
> {
>                         ret = -EBUSY;
>                         goto out_unlock;
>                 }
> 
>                 refcount_inc(&handle->users);
>                 mutex_unlock(&iommu_sva_lock);
>                 return handle;
>         }
> 
> But it appears that this code is not lock safe. If the domain on the
> PASID is not a SVA domain, the check of "handle->domain->iopf_handler !=
> iommu_sva_iopf_handler" could result in a use-after-free issue as the
> other thread might detach the domain in between the fetch and check
> lines.

For the above you just need to pass in the iommu_sva_iopf_handler as
an argument to attach_handle_get() and have it check it under the
xa_lock.

The whole thing is already protected under the ugly sva_lock.

Ideally it would be protected by the group mutex..

Jason

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Sunday, April 28, 2024 6:22 PM
> 
> On 2024/4/10 7:48, Jason Gunthorpe wrote:
> > On Tue, Apr 09, 2024 at 10:11:28AM +0800, Baolu Lu wrote:
> >> On 4/8/24 10:19 PM, Jason Gunthorpe wrote:
> >>> On Sat, Apr 06, 2024 at 02:09:34PM +0800, Baolu Lu wrote:
> >>>> On 4/3/24 7:59 PM, Jason Gunthorpe wrote:
> >>>>> On Wed, Apr 03, 2024 at 09:15:12AM +0800, Lu Baolu wrote:
> >>>>>> +	/* A bond already exists, just take a reference`. */
> >>>>>> +	handle = iommu_attach_handle_get(group, iommu_mm-
> >pasid);
> >>>>>> +	if (handle) {
> >>>>>> +		mutex_unlock(&iommu_sva_lock);
> >>>>>> +		return handle;
> >>>>>>     	}
> >>>>> At least in this context this is not enough we need to ensure that the
> >>>>> domain on the PASID is actually an SVA domain and it was installed by
> >>>>> this mechanism, not an iommufd domain for instance.
> >>>>>
> >>>>> ie you probably need a type field in the iommu_attach_handle to tell
> >>>>> what the priv is.
> >>>>>
> >>>>> Otherwise this seems like a great idea!
> >>>> Yes, you are right. For the SVA case, I will add the following changes.
> >>>> The IOMMUFD path will also need such enhancement. I will update it in
> >>>> the next version.
> >>> The only use for this is the PRI callbacks right? Maybe instead of
> >>> adding a handle type let's just check domain->iopf_handler  ?
> >>>
> >>> Ie SVA will pass &ommu_sva_iopf_handler as its "type"
> >> Sorry that I don't fully understand the proposal here.
> > I was talking specifically about the type field you suggested adding
> > to the handle struct.
> >
> > Instead of adding a type field check the domain->iopf_handler to
> > determine the domain and thus handle type.
> >
> >> The problem is that the context code (SVA, IOMMUFD, etc.) needs to
> make
> >> sure that the attach handle is really what it has installed during
> >> domain attachment. The context code needs some mechanism to include
> some
> >> kind of "owner cookie" in the attach handle, so that it could check
> >> against it later for valid use.
> > Right, you have a derived struct for each user and you need a way to
> > check if casting from the general handle struct to the derived struct
> > is OK.
> >
> > I'm suggesting using domain->iopf_handle as the type key.
> 
> After removing the refcount from the attach handle, I am trying to make
> the code look like this,
> 
>          /* A bond already exists, just take a reference`. */
>          handle = iommu_attach_handle_get(group, iommu_mm->pasid);
>          if (handle) {
>                  if (handle->domain->iopf_handler !=
> iommu_sva_iopf_handler) {
>                          ret = -EBUSY;
>                          goto out_unlock;
>                  }
> 
>                  refcount_inc(&handle->users);
>                  mutex_unlock(&iommu_sva_lock);
>                  return handle;
>          }
> 
> But it appears that this code is not lock safe. If the domain on the
> PASID is not a SVA domain, the check of "handle->domain->iopf_handler !=
> iommu_sva_iopf_handler" could result in a use-after-free issue as the
> other thread might detach the domain in between the fetch and check
> lines.
> 
> Probably we still need to keep the refcount in the attach handle?
> 

What about Jason's another comment in his original replies?

"
Though I'm not convinced the refcount should be elevated into the core
structure. The prior patch I showed you where the caller can provide
the memory for the handle and we don't have a priv would make it easy
to put the refcount in a SVA dervied handle struct without more
allocation. Then we don't need this weirdness.
"

That sounds like we'll need a iommu_sva like structure to hold
its own refcnt. Then we don't need this type check and refcnt
in the core.

On 4/29/24 10:39 AM, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Sunday, April 28, 2024 6:22 PM
>>
>> On 2024/4/10 7:48, Jason Gunthorpe wrote:
>>> On Tue, Apr 09, 2024 at 10:11:28AM +0800, Baolu Lu wrote:
>>>> On 4/8/24 10:19 PM, Jason Gunthorpe wrote:
>>>>> On Sat, Apr 06, 2024 at 02:09:34PM +0800, Baolu Lu wrote:
>>>>>> On 4/3/24 7:59 PM, Jason Gunthorpe wrote:
>>>>>>> On Wed, Apr 03, 2024 at 09:15:12AM +0800, Lu Baolu wrote:
>>>>>>>> +	/* A bond already exists, just take a reference`. */
>>>>>>>> +	handle = iommu_attach_handle_get(group, iommu_mm-
>>> pasid);
>>>>>>>> +	if (handle) {
>>>>>>>> +		mutex_unlock(&iommu_sva_lock);
>>>>>>>> +		return handle;
>>>>>>>>      	}
>>>>>>> At least in this context this is not enough we need to ensure that the
>>>>>>> domain on the PASID is actually an SVA domain and it was installed by
>>>>>>> this mechanism, not an iommufd domain for instance.
>>>>>>>
>>>>>>> ie you probably need a type field in the iommu_attach_handle to tell
>>>>>>> what the priv is.
>>>>>>>
>>>>>>> Otherwise this seems like a great idea!
>>>>>> Yes, you are right. For the SVA case, I will add the following changes.
>>>>>> The IOMMUFD path will also need such enhancement. I will update it in
>>>>>> the next version.
>>>>> The only use for this is the PRI callbacks right? Maybe instead of
>>>>> adding a handle type let's just check domain->iopf_handler  ?
>>>>>
>>>>> Ie SVA will pass &ommu_sva_iopf_handler as its "type"
>>>> Sorry that I don't fully understand the proposal here.
>>> I was talking specifically about the type field you suggested adding
>>> to the handle struct.
>>>
>>> Instead of adding a type field check the domain->iopf_handler to
>>> determine the domain and thus handle type.
>>>
>>>> The problem is that the context code (SVA, IOMMUFD, etc.) needs to
>> make
>>>> sure that the attach handle is really what it has installed during
>>>> domain attachment. The context code needs some mechanism to include
>> some
>>>> kind of "owner cookie" in the attach handle, so that it could check
>>>> against it later for valid use.
>>> Right, you have a derived struct for each user and you need a way to
>>> check if casting from the general handle struct to the derived struct
>>> is OK.
>>>
>>> I'm suggesting using domain->iopf_handle as the type key.
>>
>> After removing the refcount from the attach handle, I am trying to make
>> the code look like this,
>>
>>           /* A bond already exists, just take a reference`. */
>>           handle = iommu_attach_handle_get(group, iommu_mm->pasid);
>>           if (handle) {
>>                   if (handle->domain->iopf_handler !=
>> iommu_sva_iopf_handler) {
>>                           ret = -EBUSY;
>>                           goto out_unlock;
>>                   }
>>
>>                   refcount_inc(&handle->users);
>>                   mutex_unlock(&iommu_sva_lock);
>>                   return handle;
>>           }
>>
>> But it appears that this code is not lock safe. If the domain on the
>> PASID is not a SVA domain, the check of "handle->domain->iopf_handler !=
>> iommu_sva_iopf_handler" could result in a use-after-free issue as the
>> other thread might detach the domain in between the fetch and check
>> lines.
>>
>> Probably we still need to keep the refcount in the attach handle?
>>
> 
> What about Jason's another comment in his original replies?
> 
> "
> Though I'm not convinced the refcount should be elevated into the core
> structure. The prior patch I showed you where the caller can provide
> the memory for the handle and we don't have a priv would make it easy
> to put the refcount in a SVA dervied handle struct without more
> allocation. Then we don't need this weirdness.
> "
> 
> That sounds like we'll need a iommu_sva like structure to hold
> its own refcnt. Then we don't need this type check and refcnt
> in the core.

The problem I'm facing isn't about who allocates the handle memory.
Instead, there's no mechanism to synchronize access between two threads.
One thread might remove the handle while another fetches and reads a
member of its structure.

A similar issue exists with iommu_get_domain_for_dev_pasid(). It fetches
and returns a domain, but there's no guarantee that the domain will
*not* be freed while the caller is still using it.

One reason I introduced the reference count for attach handles is to
potentially replace iommu_get_domain_for_dev_pasid(), allowing the
domain to be accessible without any potential UAF issue.

Best regards,
baolu

On 4/10/24 7:48 AM, Jason Gunthorpe wrote:
> On Tue, Apr 09, 2024 at 10:11:28AM +0800, Baolu Lu wrote:
>> On 4/8/24 10:19 PM, Jason Gunthorpe wrote:
>>> On Sat, Apr 06, 2024 at 02:09:34PM +0800, Baolu Lu wrote:
>>>> On 4/3/24 7:59 PM, Jason Gunthorpe wrote:
>>>>> On Wed, Apr 03, 2024 at 09:15:12AM +0800, Lu Baolu wrote:
>>>>>> +	/* A bond already exists, just take a reference`. */
>>>>>> +	handle = iommu_attach_handle_get(group, iommu_mm->pasid);
>>>>>> +	if (handle) {
>>>>>> +		mutex_unlock(&iommu_sva_lock);
>>>>>> +		return handle;
>>>>>>     	}
>>>>> At least in this context this is not enough we need to ensure that the
>>>>> domain on the PASID is actually an SVA domain and it was installed by
>>>>> this mechanism, not an iommufd domain for instance.
>>>>>
>>>>> ie you probably need a type field in the iommu_attach_handle to tell
>>>>> what the priv is.
>>>>>
>>>>> Otherwise this seems like a great idea!
>>>> Yes, you are right. For the SVA case, I will add the following changes.
>>>> The IOMMUFD path will also need such enhancement. I will update it in
>>>> the next version.
>>> The only use for this is the PRI callbacks right? Maybe instead of
>>> adding a handle type let's just check domain->iopf_handler  ?
>>>
>>> Ie SVA will pass &ommu_sva_iopf_handler as its "type"
>> Sorry that I don't fully understand the proposal here.
> I was talking specifically about the type field you suggested adding
> to the handle struct.
> 
> Instead of adding a type field check the domain->iopf_handler to
> determine the domain and thus handle type.
> 
>> The problem is that the context code (SVA, IOMMUFD, etc.) needs to make
>> sure that the attach handle is really what it has installed during
>> domain attachment. The context code needs some mechanism to include some
>> kind of "owner cookie" in the attach handle, so that it could check
>> against it later for valid use.
> Right, you have a derived struct for each user and you need a way to
> check if casting from the general handle struct to the derived struct
> is OK.
> 
> I'm suggesting using domain->iopf_handle as the type key.

Oh, I see. It works. Thanks!

Best regards,
baolu