On Thu, Jun 05, 2025 at 04:40:34PM -0300, Jason Gunthorpe wrote:
> On Thu, Jun 05, 2025 at 10:04:35AM -0700, Nicolin Chen wrote:
> > On Thu, Jun 05, 2025 at 12:16:48PM -0300, Jason Gunthorpe wrote:
> > > On Wed, Jun 04, 2025 at 09:11:07PM -0700, Nicolin Chen wrote:
> > >
> > > > I found the entire ictx would be locked by iommufd_access_create(),
> > > > then the release fop couldn't even get invoked to destroy objects.
> > >
> > > Yes, that makes sense..
> > >
> > > It looks to me like you can safely leave ictx as NULL instead of
> > > adding a flag? That would be nicer than leaving a unrefcounted
> > > pointer floating around..
> >
> > Hmm, there are a few iommufd_get_object calls using access->ictx
> > in iommufd_access_attach() and iommufd_access_destroy().
>
> I counted:
>
> iommufd_access_change_ioas_id
> * Don't call this
> iommufd_access_destroy_object
> * Don't put if null
> iommufd_access_create
> * Don't set it
> iommufd_access_destroy
> * Call iommufd_object_destroy_user directly
> iommufd_access_notify_unmap
> * Check for null access->ops->unmap and skip the lock_obj/put_obj
Yea. And I added a set of lighter "_internal" helpers so the caller
side looks consistent:
access = iommufd_access_create_internal(viommu->ictx);
rc = iommufd_access_attach_internal(access, viommu->hwpt->ioas);
...
iommufd_access_detach_internal(access);
iommufd_access_destroy_internal(ictx, access);
Thanks
Nicolin
