On Thu, Mar 02, 2023 at 07:55:12AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > 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
