[PATCH 0/4] Fix hwpt lifetime and detach bugs

[PATCH 0/4] Fix hwpt lifetime and detach bugs

[Jason Gunthorpe]

As discussed here is a small series to address the confusing lifetime
scheme for the hwpt. This was some leftover from prior rework that was
never fully cleaned up.

Make it clear that the ioas and ioas->hwpt_list are associated with the
hwpt during creation and never changed until it is destroyed. A
post-finalize hwpt with a positive reference count is always valid for
device attachment.

This should simplify the nesting and replace series.

Jason Gunthorpe (4):
  iommufd: Do not add the same hwpt to the ioas->hwpt_list twice
  iommufd: Assert devices_lock for iommufd_hw_pagetable_has_group()
  iommufd: Add iommufd_lock_obj() around the auto-domains hwpts
  iommufd: Simplify the lifecycle of a hwpt

 drivers/iommu/iommufd/device.c          | 47 +++++++++++++------------
 drivers/iommu/iommufd/hw_pagetable.c    | 25 +++++++++++++
 drivers/iommu/iommufd/iommufd_private.h |  2 ++
 3 files changed, 52 insertions(+), 22 deletions(-)


base-commit: bed9e516f1183faa0e484479701cc669efd9049a
-- 
2.39.1

[PATCH 1/4] iommufd: Do not add the same hwpt to the ioas->hwpt_list twice

[Jason Gunthorpe]

The hwpt is added to the hwpt_list only during its creation, it is never
added again. This hunk is some missed leftover from rework. Adding it
twice will corrupt the linked list in some cases.

It effects HWPT specific attachment, which is something the test suite
cannot cover until we can create a legitimate struct device with a
non-system iommu "driver" (ie we need the bus removed from the iommu code)

Cc: stable@vger.kernel.org
Fixes: e8d57210035b ("iommufd: Add kAPI toward external drivers for physical devices")
Reported-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/device.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 9f3b9674d72e81..a0c66f47a65ada 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -344,10 +344,6 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id)
 		rc = iommufd_device_do_attach(idev, hwpt);
 		if (rc)
 			goto out_put_pt_obj;
-
-		mutex_lock(&hwpt->ioas->mutex);
-		list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
-		mutex_unlock(&hwpt->ioas->mutex);
 		break;
 	}
 	case IOMMUFD_OBJ_IOAS: {
-- 
2.39.1

RE: [PATCH 1/4] iommufd: Do not add the same hwpt to the ioas->hwpt_list twice

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, February 14, 2023 2:03 AM
> 
> The hwpt is added to the hwpt_list only during its creation, it is never
> added again. This hunk is some missed leftover from rework. Adding it
> twice will corrupt the linked list in some cases.
> 
> It effects HWPT specific attachment, which is something the test suite
> cannot cover until we can create a legitimate struct device with a
> non-system iommu "driver" (ie we need the bus removed from the iommu
> code)
> 
> Cc: stable@vger.kernel.org
> Fixes: e8d57210035b ("iommufd: Add kAPI toward external drivers for
> physical devices")
> Reported-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

for stable kernels this kind of fixes an issue in a dead code path
where no uAPI for user to manually create hwpt or return the
hwpt id of auto domain to userspace.

from that angle do we still want a fix tag?

> ---
>  drivers/iommu/iommufd/device.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c
> b/drivers/iommu/iommufd/device.c
> index 9f3b9674d72e81..a0c66f47a65ada 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -344,10 +344,6 @@ int iommufd_device_attach(struct iommufd_device
> *idev, u32 *pt_id)
>  		rc = iommufd_device_do_attach(idev, hwpt);
>  		if (rc)
>  			goto out_put_pt_obj;
> -
> -		mutex_lock(&hwpt->ioas->mutex);
> -		list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
> -		mutex_unlock(&hwpt->ioas->mutex);
>  		break;
>  	}

Re: [PATCH 1/4] iommufd: Do not add the same hwpt to the ioas->hwpt_list twice

[Jason Gunthorpe]

On Tue, Feb 14, 2023 at 05:51:13AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Tuesday, February 14, 2023 2:03 AM
> > 
> > The hwpt is added to the hwpt_list only during its creation, it is never
> > added again. This hunk is some missed leftover from rework. Adding it
> > twice will corrupt the linked list in some cases.
> > 
> > It effects HWPT specific attachment, which is something the test suite
> > cannot cover until we can create a legitimate struct device with a
> > non-system iommu "driver" (ie we need the bus removed from the iommu
> > code)
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: e8d57210035b ("iommufd: Add kAPI toward external drivers for
> > physical devices")
> > Reported-by: Kevin Tian <kevin.tian@intel.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> for stable kernels this kind of fixes an issue in a dead code path
> where no uAPI for user to manually create hwpt or return the
> hwpt id of auto domain to userspace.
>
> from that angle do we still want a fix tag?

I always put fixes tags, no matter how minor.

The cc stable is the thoughtful one.

In this case though there is no way to formall access this API the
HWPT ID does exist and a hostile userspace can guess it to trigger a
kernel bug. So it should be fixed.

Jason

RE: [PATCH 1/4] iommufd: Do not add the same hwpt to the ioas->hwpt_list twice

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, February 14, 2023 11:15 PM
> 
> On Tue, Feb 14, 2023 at 05:51:13AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Tuesday, February 14, 2023 2:03 AM
> > >
> > > The hwpt is added to the hwpt_list only during its creation, it is never
> > > added again. This hunk is some missed leftover from rework. Adding it
> > > twice will corrupt the linked list in some cases.
> > >
> > > It effects HWPT specific attachment, which is something the test suite
> > > cannot cover until we can create a legitimate struct device with a
> > > non-system iommu "driver" (ie we need the bus removed from the
> iommu
> > > code)
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: e8d57210035b ("iommufd: Add kAPI toward external drivers for
> > > physical devices")
> > > Reported-by: Kevin Tian <kevin.tian@intel.com>
> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >
> > for stable kernels this kind of fixes an issue in a dead code path
> > where no uAPI for user to manually create hwpt or return the
> > hwpt id of auto domain to userspace.
> >
> > from that angle do we still want a fix tag?
> 
> I always put fixes tags, no matter how minor.
> 
> The cc stable is the thoughtful one.
> 
> In this case though there is no way to formall access this API the
> HWPT ID does exist and a hostile userspace can guess it to trigger a
> kernel bug. So it should be fixed.
> 

this is fair. So,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

[PATCH 2/4] iommufd: Assert devices_lock for iommufd_hw_pagetable_has_group()

[Jason Gunthorpe]

The hwpt->devices list is locked by this, make it clearer.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index a0c66f47a65ada..dcfaf6567420e0 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -186,6 +186,8 @@ static bool iommufd_hw_pagetable_has_group(struct iommufd_hw_pagetable *hwpt,
 {
 	struct iommufd_device *cur_dev;
 
+	lockdep_assert_held(&hwpt->devices_lock);
+
 	list_for_each_entry(cur_dev, &hwpt->devices, devices_item)
 		if (cur_dev->group == group)
 			return true;
-- 
2.39.1

RE: [PATCH 2/4] iommufd: Assert devices_lock for iommufd_hw_pagetable_has_group()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, February 14, 2023 2:03 AM
> 
> The hwpt->devices list is locked by this, make it clearer.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

[PATCH 3/4] iommufd: Add iommufd_lock_obj() around the auto-domains hwpts

[Jason Gunthorpe]

A later patch will require this locking - currently under the ioas mutex
the hwpt can not have a 0 reference and be on the list.

Optimize this a bit more by having iommufd_device_do_attach() consume the
reference that most of the callers already have.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/device.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index dcfaf6567420e0..7b687d002c5c81 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -194,6 +194,7 @@ static bool iommufd_hw_pagetable_has_group(struct iommufd_hw_pagetable *hwpt,
 	return false;
 }
 
+/* On success this consumes a hwpt reference from the caller */
 static int iommufd_device_do_attach(struct iommufd_device *idev,
 				    struct iommufd_hw_pagetable *hwpt)
 {
@@ -247,7 +248,7 @@ static int iommufd_device_do_attach(struct iommufd_device *idev,
 	}
 
 	idev->hwpt = hwpt;
-	refcount_inc(&hwpt->obj.users);
+	/* The HWPT reference from the caller is moved to this list */
 	list_add(&idev->devices_item, &hwpt->devices);
 	mutex_unlock(&hwpt->devices_lock);
 	return 0;
@@ -282,7 +283,15 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
 		if (!hwpt->auto_domain)
 			continue;
 
+		if (!iommufd_lock_obj(&hwpt->obj))
+			continue;
 		rc = iommufd_device_do_attach(idev, hwpt);
+		if (rc) {
+			iommufd_put_object(&hwpt->obj);
+		} else {
+			/* Our reference was passed into iommufd_device_do_attach() */
+			iommufd_ref_to_users(&hwpt->obj);
+		}
 
 		/*
 		 * -EINVAL means the domain is incompatible with the device.
@@ -301,9 +310,13 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
 	}
 	hwpt->auto_domain = true;
 
+	refcount_inc(&hwpt->obj.users);
 	rc = iommufd_device_do_attach(idev, hwpt);
-	if (rc)
+	if (rc) {
+		refcount_dec(&hwpt->obj.users);
 		goto out_abort;
+	}
+
 	list_add_tail(&hwpt->hwpt_item, &ioas->hwpt_list);
 
 	mutex_unlock(&ioas->mutex);
@@ -346,7 +359,11 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id)
 		rc = iommufd_device_do_attach(idev, hwpt);
 		if (rc)
 			goto out_put_pt_obj;
-		break;
+
+		/* Our reference was passed into iommufd_device_do_attach() */
+		iommufd_ref_to_users(pt_obj);
+		refcount_inc(&idev->obj.users);
+		return 0;
 	}
 	case IOMMUFD_OBJ_IOAS: {
 		struct iommufd_ioas *ioas =
@@ -355,17 +372,15 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id)
 		rc = iommufd_device_auto_get_domain(idev, ioas);
 		if (rc)
 			goto out_put_pt_obj;
-		break;
+		*pt_id = idev->hwpt->obj.id;
+		refcount_inc(&idev->obj.users);
+		goto out_put_pt_obj;
 	}
 	default:
 		rc = -EINVAL;
 		goto out_put_pt_obj;
 	}
 
-	refcount_inc(&idev->obj.users);
-	*pt_id = idev->hwpt->obj.id;
-	rc = 0;
-
 out_put_pt_obj:
 	iommufd_put_object(pt_obj);
 	return rc;
-- 
2.39.1

RE: [PATCH 3/4] iommufd: Add iommufd_lock_obj() around the auto-domains hwpts

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, February 14, 2023 2:03 AM
> 
> A later patch will require this locking - currently under the ioas mutex
> the hwpt can not have a 0 reference and be on the list.
> 
> Optimize this a bit more by having iommufd_device_do_attach() consume
> the
> reference that most of the callers already have.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

[PATCH 4/4] iommufd: Simplify the lifecycle of a hwpt

[Jason Gunthorpe]

The HWPT is connected to the ioas when it is finished being created and
should remain connected until it is destroyed.

Instead of trying to disconnect the HWPT manually during
iommufd_device_detach() simply use the object reference counting mechanism
and free it when the refcount reaches 0.

This means the ioas->hwpt_list can have an item with a 0 refcount during
races, a prior patch made sure this is safe.

Add a new API iommufd_hw_pagetable_finalize() to formalize this contract.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/device.c          | 14 ++------------
 drivers/iommu/iommufd/hw_pagetable.c    | 25 +++++++++++++++++++++++++
 drivers/iommu/iommufd/iommufd_private.h |  2 ++
 3 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 7b687d002c5c81..cc1452f9e21024 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -317,10 +317,8 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
 		goto out_abort;
 	}
 
-	list_add_tail(&hwpt->hwpt_item, &ioas->hwpt_list);
-
+	iommufd_hw_pagetable_finalize(idev->ictx, hwpt);
 	mutex_unlock(&ioas->mutex);
-	iommufd_object_finalize(idev->ictx, &hwpt->obj);
 	return 0;
 
 out_abort:
@@ -398,20 +396,12 @@ void iommufd_device_detach(struct iommufd_device *idev)
 {
 	struct iommufd_hw_pagetable *hwpt = idev->hwpt;
 
-	mutex_lock(&hwpt->ioas->mutex);
 	mutex_lock(&hwpt->devices_lock);
 	list_del(&idev->devices_item);
-	if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) {
-		if (list_empty(&hwpt->devices)) {
-			iopt_table_remove_domain(&hwpt->ioas->iopt,
-						 hwpt->domain);
-			list_del(&hwpt->hwpt_item);
-		}
+	if (!iommufd_hw_pagetable_has_group(hwpt, idev->group))
 		iommu_detach_group(hwpt->domain, idev->group);
-	}
 	iopt_remove_reserved_iova(&hwpt->ioas->iopt, idev->dev);
 	mutex_unlock(&hwpt->devices_lock);
-	mutex_unlock(&hwpt->ioas->mutex);
 
 	if (hwpt->auto_domain)
 		iommufd_object_destroy_user(idev->ictx, &hwpt->obj);
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 43d473989a0667..39c9a7d6ea3da9 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -10,9 +10,18 @@ void iommufd_hw_pagetable_destroy(struct iommufd_object *obj)
 {
 	struct iommufd_hw_pagetable *hwpt =
 		container_of(obj, struct iommufd_hw_pagetable, obj);
+	bool finalize_done;
 
 	WARN_ON(!list_empty(&hwpt->devices));
 
+	mutex_lock(&hwpt->ioas->mutex);
+	finalize_done = !list_empty(&hwpt->hwpt_item);
+	list_del(&hwpt->hwpt_item);
+	mutex_unlock(&hwpt->ioas->mutex);
+
+	if (finalize_done)
+		iopt_table_remove_domain(&hwpt->ioas->iopt, hwpt->domain);
+
 	iommu_domain_free(hwpt->domain);
 	refcount_dec(&hwpt->ioas->obj.users);
 	mutex_destroy(&hwpt->devices_lock);
@@ -25,6 +34,8 @@ void iommufd_hw_pagetable_destroy(struct iommufd_object *obj)
  * @dev: Device to get an iommu_domain for
  *
  * Allocate a new iommu_domain and return it as a hw_pagetable.
+ * iommufd_hw_pagetable_finalize() must be called to successfully complete the
+ * allocation, otherwise iommufd_object_abort_and_destroy() should be called.
  */
 struct iommufd_hw_pagetable *
 iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
@@ -55,3 +66,17 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 	iommufd_object_abort(ictx, &hwpt->obj);
 	return ERR_PTR(rc);
 }
+
+void iommufd_hw_pagetable_finalize(struct iommufd_ctx *ictx,
+				   struct iommufd_hw_pagetable *hwpt)
+{
+	lockdep_assert_held(&hwpt->ioas->mutex);
+
+	/*
+	 * Once the hwpt is on this list it can become attached through the
+	 * auto_domains mechanism so this must be called after
+	 * iopt_table_add_domain().
+	 */
+	list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
+	iommufd_object_finalize(ictx, &hwpt->obj);
+}
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 9d7f71510ca1bc..2837044e8431be 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -255,6 +255,8 @@ struct iommufd_hw_pagetable {
 struct iommufd_hw_pagetable *
 iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 			   struct device *dev);
+void iommufd_hw_pagetable_finalize(struct iommufd_ctx *ictx,
+				   struct iommufd_hw_pagetable *hwpt);
 void iommufd_hw_pagetable_destroy(struct iommufd_object *obj);
 
 void iommufd_device_destroy(struct iommufd_object *obj);
-- 
2.39.1

RE: [PATCH 4/4] iommufd: Simplify the lifecycle of a hwpt

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, February 14, 2023 2:03 AM
> 
> The HWPT is connected to the ioas when it is finished being created and
> should remain connected until it is destroyed.
> 
> Instead of trying to disconnect the HWPT manually during
> iommufd_device_detach() simply use the object reference counting
> mechanism
> and free it when the refcount reaches 0.
> 
> This means the ioas->hwpt_list can have an item with a 0 refcount during
> races, a prior patch made sure this is safe.
> 
> Add a new API iommufd_hw_pagetable_finalize() to formalize this contract.

Can you add a few words how you expect this to be called in manually
created hwpt case? the new API requires it to be called after
iopt_table_add_domain() which is part of iommufd_device_do_attach()
now. Do you intend to move iopt_table_add_domain() into hwpt
creation path so this finalize API is also called there or leave it as today
which means again need to track first device in the group to finalize?
 
> @@ -10,9 +10,18 @@ void iommufd_hw_pagetable_destroy(struct
> iommufd_object *obj)
>  {
>  	struct iommufd_hw_pagetable *hwpt =
>  		container_of(obj, struct iommufd_hw_pagetable, obj);
> +	bool finalize_done;
> 

'finalized'

Re: [PATCH 4/4] iommufd: Simplify the lifecycle of a hwpt

[Nicolin Chen]

On Tue, Feb 14, 2023 at 06:14:07AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Tuesday, February 14, 2023 2:03 AM
> >
> > The HWPT is connected to the ioas when it is finished being created and
> > should remain connected until it is destroyed.
> >
> > Instead of trying to disconnect the HWPT manually during
> > iommufd_device_detach() simply use the object reference counting
> > mechanism
> > and free it when the refcount reaches 0.
> >
> > This means the ioas->hwpt_list can have an item with a 0 refcount during
> > races, a prior patch made sure this is safe.
> >
> > Add a new API iommufd_hw_pagetable_finalize() to formalize this contract.
> 
> Can you add a few words how you expect this to be called in manually
> created hwpt case? the new API requires it to be called after
> iopt_table_add_domain() which is part of iommufd_device_do_attach()
> now. Do you intend to move iopt_table_add_domain() into hwpt
> creation path so this finalize API is also called there or leave it as today
> which means again need to track first device in the group to finalize?

I think we could probably call iommufd_hw_pagetable_finalize()
in iommufd_device_do_attach(), behind iopt_table_add_domain().

New HWPT_ALLOC ioctl handler can't do iopt_table_add_domain(),
because it might go for the ops->domain_alloc() fallback path.
So the iopt_table_add_domain() in iommufd_device_do_attach() is
necessary for the that path too.

Thanks
Nic

Re: [PATCH 4/4] iommufd: Simplify the lifecycle of a hwpt

[Jason Gunthorpe]

On Tue, Feb 14, 2023 at 06:14:07AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Tuesday, February 14, 2023 2:03 AM
> > 
> > The HWPT is connected to the ioas when it is finished being created and
> > should remain connected until it is destroyed.
> > 
> > Instead of trying to disconnect the HWPT manually during
> > iommufd_device_detach() simply use the object reference counting
> > mechanism
> > and free it when the refcount reaches 0.
> > 
> > This means the ioas->hwpt_list can have an item with a 0 refcount during
> > races, a prior patch made sure this is safe.
> > 
> > Add a new API iommufd_hw_pagetable_finalize() to formalize this contract.
> 
> Can you add a few words how you expect this to be called in manually
> created hwpt case? the new API requires it to be called after
> iopt_table_add_domain() which is part of iommufd_device_do_attach()
> now. Do you intend to move iopt_table_add_domain() into hwpt
> creation path so this finalize API is also called there or leave it as today
> which means again need to track first device in the group to finalize?

It is all still pretty messy, let me try again.

Jason

Re: [PATCH 0/4] Fix hwpt lifetime and detach bugs

[Jason Gunthorpe]

On Mon, Feb 13, 2023 at 02:02:41PM -0400, Jason Gunthorpe wrote:
> As discussed here is a small series to address the confusing lifetime
> scheme for the hwpt. This was some leftover from prior rework that was
> never fully cleaned up.
> 
> Make it clear that the ioas and ioas->hwpt_list are associated with the
> hwpt during creation and never changed until it is destroyed. A
> post-finalize hwpt with a positive reference count is always valid for
> device attachment.
> 
> This should simplify the nesting and replace series.
> 
> Jason Gunthorpe (4):
>   iommufd: Do not add the same hwpt to the ioas->hwpt_list twice

I've applied this patch to the tree

And my v2 of this series is here now:

https://github.com/jgunthorpe/linux/commits/iommufd_hwpt

Which is completely redone

I will try to make the patches to do replace and alloc hwpt on top of
this, I think it fixes all the locking and consistency problems.

Jason

RE: [PATCH 0/4] Fix hwpt lifetime and detach bugs

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, February 16, 2023 9:37 AM
> 
> On Mon, Feb 13, 2023 at 02:02:41PM -0400, Jason Gunthorpe wrote:
> > As discussed here is a small series to address the confusing lifetime
> > scheme for the hwpt. This was some leftover from prior rework that was
> > never fully cleaned up.
> >
> > Make it clear that the ioas and ioas->hwpt_list are associated with the
> > hwpt during creation and never changed until it is destroyed. A
> > post-finalize hwpt with a positive reference count is always valid for
> > device attachment.
> >
> > This should simplify the nesting and replace series.
> >
> > Jason Gunthorpe (4):
> >   iommufd: Do not add the same hwpt to the ioas->hwpt_list twice
> 
> I've applied this patch to the tree
> 
> And my v2 of this series is here now:
> 
> https://github.com/jgunthorpe/linux/commits/iommufd_hwpt
> 
> Which is completely redone
> 
> I will try to make the patches to do replace and alloc hwpt on top of
> this, I think it fixes all the locking and consistency problems.
> 

with group now explicitly tracked in iommufd presumably iommufd
can continue to use iommu_attach_group() instead of switching to
iommu_attach_device() as previously planned?

Re: [PATCH 0/4] Fix hwpt lifetime and detach bugs

[Jason Gunthorpe]

On Wed, Feb 22, 2023 at 02:14:42AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, February 16, 2023 9:37 AM
> > 
> > On Mon, Feb 13, 2023 at 02:02:41PM -0400, Jason Gunthorpe wrote:
> > > As discussed here is a small series to address the confusing lifetime
> > > scheme for the hwpt. This was some leftover from prior rework that was
> > > never fully cleaned up.
> > >
> > > Make it clear that the ioas and ioas->hwpt_list are associated with the
> > > hwpt during creation and never changed until it is destroyed. A
> > > post-finalize hwpt with a positive reference count is always valid for
> > > device attachment.
> > >
> > > This should simplify the nesting and replace series.
> > >
> > > Jason Gunthorpe (4):
> > >   iommufd: Do not add the same hwpt to the ioas->hwpt_list twice
> > 
> > I've applied this patch to the tree
> > 
> > And my v2 of this series is here now:
> > 
> > https://github.com/jgunthorpe/linux/commits/iommufd_hwpt
> > 
> > Which is completely redone
> > 
> > I will try to make the patches to do replace and alloc hwpt on top of
> > this, I think it fixes all the locking and consistency problems.
> > 
> 
> with group now explicitly tracked in iommufd presumably iommufd
> can continue to use iommu_attach_group() instead of switching to
> iommu_attach_device() as previously planned?

Yes, it seems necessary.

The only other option I could think of was to expose more of the
core's iommu_group struct for owner usage.

Jason