Re: [PATCH 5/6] vfio: Simplify the life cycle of the group FD

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, May 10, 2022 at 01:59:59PM -0600, Alex Williamson wrote:
> On Thu,  5 May 2022 21:25:05 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > Once userspace opens a group FD it is prevented from opening another
> > instance of that same group FD until all the prior group FDs and users of
> > the container are done.
> > 
> > The first is done trivially by checking the group->owned during group FD
> > open.
> > 
> > However, things get a little weird if userspace creates a device FD and
> > then closes the group FD. The group FD still cannot be re-opened, but this
> > time it is because the group->container is still set and container_users
> > is elevated by the device FD.
> > 
> > Due to this mismatched lifecycle we have the
> > vfio_group_try_dissolve_container() which tries to auto-free a container
> > after the group FD is closed but the device FD remains open.
> > 
> > Instead have the device FD hold onto a reference to the single group
> > FD. This directly prevents vfio_group_fops_release() from being called
> > when any device FD exists and makes the lifecycle model more
> > understandable.
> > 
> > vfio_group_try_dissolve_container() is removed as the only place a
> > container is auto-deleted is during vfio_group_fops_release(). At this
> > point the container_users is either 1 or 0 since all device FDs must be
> > closed.
> > 
> > Change group->owner to group->singleton_filep which points to the single
> > struct file * that is open for the group. If the group->singleton_filep is
> > NULL then group->container == NULL.
> > 
> > If all device FDs have closed then the group's notifier list must be
> > empty.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  drivers/vfio/vfio.c | 49 +++++++++++++++++++--------------------------
> >  1 file changed, 21 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index 63f7fa872eae60..94ab415190011d 100644
> > +++ b/drivers/vfio/vfio.c
> > @@ -73,12 +73,12 @@ struct vfio_group {
> >  	struct mutex			device_lock;
> >  	struct list_head		vfio_next;
> >  	struct list_head		container_next;
> > -	bool				opened;
> >  	wait_queue_head_t		container_q;
> >  	enum vfio_group_type		type;
> >  	unsigned int			dev_counter;
> >  	struct rw_semaphore		group_rwsem;
> >  	struct kvm			*kvm;
> > +	struct file			*singleton_file;
> 
> I'm not really a fan of this name, if we have a single struct file
> pointer on the group, it's necessarily singleton.  Maybe just
> "opened_file"?

Sure

> > @@ -1315,10 +1304,14 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
> >  
> >  	filep->private_data = NULL;
> >  
> > -	vfio_group_try_dissolve_container(group);
> > -
> >  	down_write(&group->group_rwsem);
> > -	group->opened = false;
> > +	/* All device FDs must be released before the group fd releases. */
> 
> This sounds more like a user directive as it's phrased, maybe something
> like:
> 
> 	/*
> 	 * Device FDs hold a group file reference, therefore the group
> 	 * release is only called when there are no open devices.
> 	 */

OK

What do you want to do with this series?

As-posted it requires the iommu series, and Joerg has vanished again
so I don't know what will happen there.

However, it doesn't require it, there are just some textual conflicts.

It does need the KVM series though, which I expect we will just go
ahead with unacked. Oh well.

Do you want to still try to get it in, or just give up for this cycle?
If yes, which base should I use :)

Thanks,
Jason