On Thu, Mar 02, 2023 at 07:55:12AM +0000, Tian, Kevin wrote:
From: Jason Gunthorpe jgg@nvidia.com Sent: Saturday, February 25, 2023 8:28 AM
- /*
* All objects using a group reference must put it before their destroy* completes*/- new_igroup->ictx = ictx;
Looks the comment is not related to the code. Probably put it in the destroy function?
It explains why we don't take a reference on ictx here.
- cur_igroup = NULL;
- xa_lock(&ictx->groups);
- while (true) {
igroup = __xa_cmpxchg(&ictx->groups, id, cur_igroup,new_igroup,
GFP_KERNEL);if (xa_is_err(igroup)) {xa_unlock(&ictx->groups);iommufd_put_group(new_igroup);return ERR_PTR(xa_err(igroup));}/* new_group was successfully installed */if (cur_igroup == igroup) {xa_unlock(&ictx->groups);return new_igroup;}/* Check again if the current group is any good */if (igroup && igroup->group == group &&kref_get_unless_zero(&igroup->ref)) {xa_unlock(&ictx->groups);iommufd_put_group(new_igroup);return igroup;}cur_igroup = igroup;- }
Add a WARN_ON(igroup->group != group). The only valid race should be when an existing group which is created by another device in the same iommu group is being destroyed then we want another try hoping that object will be removed from xarray soon. But there should not be a case with the same slot pointing to a different iommu group.
Yeah, that is the case, both the group checks are WARN_ON's because we hold an iommu_group reference as long as the xarray entry is popoulated so it should be impossible for another iommu_group pointer to have the same ID.
@@ -98,7 +195,7 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx, /* The calling driver is a user until iommufd_device_unbind() */ refcount_inc(&idev->obj.users); /* group refcount moves into iommufd_device */
- idev->group = group;
- idev->igroup = igroup;
the comment about group refcount is stale now.
You mean it should say 'igroup refcount' ?
Jason