Re: [PATCH v6 06/10] iommufd: Add iommufd fault object

[Jason Gunthorpe <jgg@ziepe.ca>]

On Sat, Jun 08, 2024 at 05:58:34PM +0800, Baolu Lu wrote:

> > > +static int iommufd_fault_fops_release(struct inode *inode, struct file *filep)
> > > +{
> > > +	struct iommufd_fault *fault = filep->private_data;
> > > +
> > > +	iommufd_ctx_put(fault->ictx);
> > > +	refcount_dec(&fault->obj.users);
> > > +	return 0;


This is in the wrong order, dec users before ctx_put.

> > hmm this doesn't sound correct. the context and refcount are
> > acquired in iommufd_fault_alloc() but here they are reverted when
> > the fd is closed...
> 
> These two refcounts were requested when the fault object was installed
> to the fault FD.
> 
>        filep = anon_inode_getfile("[iommufd-pgfault]", &iommufd_fault_fops,
>                                    fault, O_RDWR);
>         if (IS_ERR(filep)) {
>                 rc = PTR_ERR(filep);
>                 goto out_abort;
>         }
> 
>         refcount_inc(&fault->obj.users);
>         iommufd_ctx_get(fault->ictx);
>         fault->filep = filep;
> 
> These refcounts must then be released when the FD is released.

Yes

The ctx refcount is to avoid destroying the ctx FD, which can't work,
while the fault FD has an object pinned. This is also why the above
order is backwards.

Jason