From: Jason Gunthorpe <jgg@nvidia.com>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: Alex Williamson <alex.williamson@redhat.com>,
Cornelia Huck <cohuck@redhat.com>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
Qian Cai <cai@lca.pw>, "Rodel, Jorg" <jroedel@suse.de>,
Marek Szyprowski <m.szyprowski@samsung.com>,
Matthew Rosato <mjrosato@linux.ibm.com>
Subject: Re: [PATCH v2] vfio: Follow a strict lifetime for struct iommu_group
Date: Wed, 28 Sep 2022 12:26:14 -0300 [thread overview]
Message-ID: <YzRnlqw/U7xDhL7P@nvidia.com> (raw)
In-Reply-To: <BN9PR11MB5276ED36A2F498D37A18DCF38C549@BN9PR11MB5276.namprd11.prod.outlook.com>
On Wed, Sep 28, 2022 at 03:51:01AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Friday, September 23, 2022 8:06 AM
> >
> > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> > index 56fab31f8e0ff8..039e3208d286fa 100644
> > --- a/drivers/vfio/vfio.h
> > +++ b/drivers/vfio/vfio.h
> > @@ -41,7 +41,15 @@ enum vfio_group_type {
> > struct vfio_group {
> > struct device dev;
> > struct cdev cdev;
> > + /*
> > + * When drivers is non-zero a driver is attached to the struct device
> > + * that provided the iommu_group and thus the iommu_group is a
> > valid
> > + * pointer. When drivers is 0 the driver is being detached. Once users
> > + * reaches 0 then the iommu_group is invalid.
> > + */
> > + refcount_t drivers;
>
> While I agree all this patch is doing, the notation of 'drivers' here sounds
> a bit confusing IMHO.
Maybe, I picked it because we recently had a num_devices here that was
a different thing. "drivers" comes from the idea that it is the number
of drivers that have called 'register' on the group. This also happens
to be the number of vfio_devices of course.
> > refcount_t users;
> > + struct completion users_comp;
>
> Now the only place poking 'users' is when a group is opened/closed,
> while group->opened_file already plays the guard role. From this
> angle 'users' sounds redundant now?
Oh interesting. I did try to get rid of that thing, but I was thinking
to make it "disassociate" so we didn't have to sleep at all, but SPAPR
messed that up.. It is a good followup patch
So like this:
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 039e3208d286fa..78b362a9250113 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -48,8 +48,6 @@ struct vfio_group {
* reaches 0 then the iommu_group is invalid.
*/
refcount_t drivers;
- refcount_t users;
- struct completion users_comp;
unsigned int container_users;
struct iommu_group *iommu_group;
struct vfio_container *container;
@@ -61,6 +59,7 @@ struct vfio_group {
struct rw_semaphore group_rwsem;
struct kvm *kvm;
struct file *opened_file;
+ struct swait_queue_head opened_file_wait;
struct blocking_notifier_head notifier;
};
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index f19171cad9a25f..57a7576a96a61b 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -186,10 +186,9 @@ static struct vfio_group *vfio_group_alloc(struct iommu_group *iommu_group,
cdev_init(&group->cdev, &vfio_group_fops);
group->cdev.owner = THIS_MODULE;
- refcount_set(&group->users, 1);
refcount_set(&group->drivers, 1);
- init_completion(&group->users_comp);
init_rwsem(&group->group_rwsem);
+ init_swait_queue_head(&group->opened_file_wait);
INIT_LIST_HEAD(&group->device_list);
mutex_init(&group->device_lock);
group->iommu_group = iommu_group;
@@ -245,12 +244,6 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
return ret;
}
-static void vfio_group_put(struct vfio_group *group)
-{
- if (refcount_dec_and_test(&group->users))
- complete(&group->users_comp);
-}
-
static void vfio_device_remove_group(struct vfio_device *device)
{
struct vfio_group *group = device->group;
@@ -270,10 +263,6 @@ static void vfio_device_remove_group(struct vfio_device *device)
* cdev_device_add() will fail due to the name aready existing.
*/
cdev_device_del(&group->cdev, &group->dev);
- mutex_unlock(&vfio.group_lock);
-
- /* Matches the get from vfio_group_alloc() */
- vfio_group_put(group);
/*
* Before we allow the last driver in the group to be unplugged the
@@ -281,7 +270,13 @@ static void vfio_device_remove_group(struct vfio_device *device)
* is because the group->iommu_group pointer should only be used so long
* as a device driver is attached to a device in the group.
*/
- wait_for_completion(&group->users_comp);
+ while (group->opened_file) {
+ mutex_unlock(&vfio.group_lock);
+ swait_event_idle_exclusive(group->opened_file_wait,
+ !group->opened_file);
+ mutex_lock(&vfio.group_lock);
+ }
+ mutex_unlock(&vfio.group_lock);
/*
* These data structures all have paired operations that can only be
@@ -906,15 +901,18 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
down_write(&group->group_rwsem);
- /* users can be zero if this races with vfio_device_remove_group() */
- if (!refcount_inc_not_zero(&group->users)) {
+ /*
+ * drivers can be zero if this races with vfio_device_remove_group(), it
+ * will be stable at 0 under the group rwsem
+ */
+ if (refcount_read(&group->drivers) == 0) {
ret = -ENODEV;
- goto err_unlock;
+ goto out_unlock;
}
if (group->type == VFIO_NO_IOMMU && !capable(CAP_SYS_RAWIO)) {
ret = -EPERM;
- goto err_put;
+ goto out_unlock;
}
/*
@@ -922,16 +920,12 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
*/
if (group->opened_file) {
ret = -EBUSY;
- goto err_put;
+ goto out_unlock;
}
group->opened_file = filep;
filep->private_data = group;
-
- up_write(&group->group_rwsem);
- return 0;
-err_put:
- vfio_group_put(group);
-err_unlock:
+ ret = 0;
+out_unlock:
up_write(&group->group_rwsem);
return ret;
}
@@ -952,8 +946,7 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
vfio_group_detach_container(group);
group->opened_file = NULL;
up_write(&group->group_rwsem);
-
- vfio_group_put(group);
+ swake_up_one(&group->opened_file_wait);
return 0;
}
next prev parent reply other threads:[~2022-09-28 15:27 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-23 0:06 [PATCH v2] vfio: Follow a strict lifetime for struct iommu_group Jason Gunthorpe
2022-09-26 17:03 ` Matthew Rosato
2022-09-27 20:05 ` Alex Williamson
2022-10-04 15:19 ` Christian Borntraeger
2022-10-04 15:40 ` Jason Gunthorpe
2022-10-04 15:44 ` Christian Borntraeger
2022-10-04 16:28 ` Jason Gunthorpe
2022-10-04 17:15 ` Christian Borntraeger
2022-10-04 17:22 ` Jason Gunthorpe
2022-10-04 17:36 ` Christian Borntraeger
2022-10-04 17:48 ` Christian Borntraeger
2022-10-04 18:22 ` Matthew Rosato
2022-10-04 18:56 ` Eric Farman
2022-10-05 13:46 ` Matthew Rosato
2022-10-05 13:57 ` Jason Gunthorpe
2022-10-05 14:00 ` Christian Borntraeger
2022-10-05 14:01 ` Jason Gunthorpe
2022-10-05 14:19 ` Christian Borntraeger
2022-10-06 11:55 ` Christian Borntraeger
2022-10-05 14:21 ` Matthew Rosato
2022-10-05 15:40 ` Matthew Rosato
2022-10-05 14:01 ` Matthew Rosato
2022-10-04 19:59 ` Matthew Rosato
2022-10-04 20:19 ` Alex Williamson
2022-10-04 22:30 ` Jason Gunthorpe
2022-09-27 6:34 ` Yi Liu
2022-09-27 13:12 ` Jason Gunthorpe
2022-09-28 3:51 ` Tian, Kevin
2022-09-28 15:26 ` Jason Gunthorpe [this message]
2022-09-28 23:54 ` Tian, Kevin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YzRnlqw/U7xDhL7P@nvidia.com \
--to=jgg@nvidia.com \
--cc=alex.williamson@redhat.com \
--cc=cai@lca.pw \
--cc=cohuck@redhat.com \
--cc=jroedel@suse.de \
--cc=kevin.tian@intel.com \
--cc=kvm@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=mjrosato@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.