| Message ID | 8-v1-7dedf20b2b75+4f785-vfio2_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make vfio_mdev type safe | expand |
> up_read(&parent->unreg_sem); > - put_device(&mdev->dev); > mdev_fail: > > > > - mdev_put_parent(parent); > + put_device(&mdev->dev); That mdev_fail label is not very descriptive, what about free_device instead? Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, March 24, 2021 1:55 AM > > Once the memory for the struct mdev_device is allocated it should > immediately be device_initialize()'d and filled in so that put_device() > can always be used to undo the allocation. > > Place the mdev_get/put_parent() so that they are clearly protecting the > mdev->parent pointer. Move the final put to the release function so that > the lifetime rules are trivial to understand. > > Remove mdev_device_free() as the release function via device_put() is now > usable in all cases. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > drivers/vfio/mdev/mdev_core.c | 46 +++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 26 deletions(-) > > diff --git a/drivers/vfio/mdev/mdev_core.c > b/drivers/vfio/mdev/mdev_core.c > index 7ec21c907397a5..517b6fd351b63a 100644 > --- a/drivers/vfio/mdev/mdev_core.c > +++ b/drivers/vfio/mdev/mdev_core.c > @@ -71,7 +71,6 @@ static void mdev_device_remove_common(struct > mdev_device *mdev) > > /* Balances with device_initialize() */ > put_device(&mdev->dev); > - mdev_put_parent(parent); > } > > static int mdev_device_remove_cb(struct device *dev, void *data) > @@ -208,8 +207,13 @@ void mdev_unregister_device(struct device *dev) > } > EXPORT_SYMBOL(mdev_unregister_device); > > -static void mdev_device_free(struct mdev_device *mdev) > +static void mdev_device_release(struct device *dev) > { > + struct mdev_device *mdev = to_mdev_device(dev); > + > + /* Pairs with the get in mdev_device_create() */ > + mdev_put_parent(mdev->parent); > + > mutex_lock(&mdev_list_lock); > list_del(&mdev->next); > mutex_unlock(&mdev_list_lock); > @@ -218,59 +222,50 @@ static void mdev_device_free(struct mdev_device > *mdev) > kfree(mdev); > } > > -static void mdev_device_release(struct device *dev) > -{ > - struct mdev_device *mdev = to_mdev_device(dev); > - > - mdev_device_free(mdev); > -} > - > int mdev_device_create(struct mdev_type *type, const guid_t *uuid) > { > int ret; > struct mdev_device *mdev, *tmp; > struct mdev_parent *parent = type->parent; > > - mdev_get_parent(parent); > mutex_lock(&mdev_list_lock); > > /* Check for duplicate */ > list_for_each_entry(tmp, &mdev_list, next) { > if (guid_equal(&tmp->uuid, uuid)) { > mutex_unlock(&mdev_list_lock); > - ret = -EEXIST; > - goto mdev_fail; > + return -EEXIST; > } > } > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); > if (!mdev) { > mutex_unlock(&mdev_list_lock); > - ret = -ENOMEM; > - goto mdev_fail; > + return -ENOMEM; > } > > + device_initialize(&mdev->dev); > + mdev->dev.parent = parent->dev; > + mdev->dev.bus = &mdev_bus_type; > + mdev->dev.release = mdev_device_release; > + mdev->dev.groups = parent->ops->mdev_attr_groups; > + mdev->type = type; > + mdev->parent = parent; > + /* Pairs with the put in mdev_device_release() */ > + mdev_get_parent(parent); > + > guid_copy(&mdev->uuid, uuid); > list_add(&mdev->next, &mdev_list); > mutex_unlock(&mdev_list_lock); > > - mdev->parent = parent; > + dev_set_name(&mdev->dev, "%pUl", uuid); > > /* Check if parent unregistration has started */ > if (!down_read_trylock(&parent->unreg_sem)) { > - mdev_device_free(mdev); > ret = -ENODEV; > goto mdev_fail; > } > > - device_initialize(&mdev->dev); > - mdev->dev.parent = parent->dev; > - mdev->dev.bus = &mdev_bus_type; > - mdev->dev.release = mdev_device_release; > - dev_set_name(&mdev->dev, "%pUl", uuid); > - mdev->dev.groups = parent->ops->mdev_attr_groups; > - mdev->type = type; > - > ret = parent->ops->create(&type->kobj, mdev); > if (ret) > goto ops_create_fail; > @@ -295,9 +290,8 @@ int mdev_device_create(struct mdev_type *type, > const guid_t *uuid) > parent->ops->remove(mdev); > ops_create_fail: > up_read(&parent->unreg_sem); > - put_device(&mdev->dev); > mdev_fail: > - mdev_put_parent(parent); > + put_device(&mdev->dev); > return ret; > } > > -- > 2.31.0
> From: Christoph Hellwig <hch@lst.de> > Sent: Wednesday, March 24, 2021 3:21 AM > > > up_read(&parent->unreg_sem); > > - put_device(&mdev->dev); > > mdev_fail: > > > > > > > > - mdev_put_parent(parent); > > + put_device(&mdev->dev); > > That mdev_fail label is not very descriptive, what about free_device > instead? > There is only one goto to this place after this patch. Possibly just call it trylock_fail.
> From: Tian, Kevin <kevin.tian@intel.com> > Sent: Friday, March 26, 2021 11:34 AM > > > From: Christoph Hellwig <hch@lst.de> > > Sent: Wednesday, March 24, 2021 3:21 AM > > > > > up_read(&parent->unreg_sem); > > > - put_device(&mdev->dev); > > > mdev_fail: > > > > > > > > > > > > - mdev_put_parent(parent); > > > + put_device(&mdev->dev); > > > > That mdev_fail label is not very descriptive, what about free_device > > instead? > > > > There is only one goto to this place after this patch. Possibly just > call it trylock_fail. oops, please forget this comment as another goto comes in next patch...
On Tue, 23 Mar 2021 14:55:25 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > Once the memory for the struct mdev_device is allocated it should > immediately be device_initialize()'d and filled in so that put_device() > can always be used to undo the allocation. > > Place the mdev_get/put_parent() so that they are clearly protecting the > mdev->parent pointer. Move the final put to the release function so that > the lifetime rules are trivial to understand. > > Remove mdev_device_free() as the release function via device_put() is now > usable in all cases. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/mdev/mdev_core.c | 46 +++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 26 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On Tue, Mar 23, 2021 at 08:20:40PM +0100, Christoph Hellwig wrote: > > up_read(&parent->unreg_sem); > > - put_device(&mdev->dev); > > mdev_fail: > > > > > > > > - mdev_put_parent(parent); > > + put_device(&mdev->dev); > > That mdev_fail label is not very descriptive, what about free_device > instead? It is all a bit off normal, lets just fix it all in this patch too, there is alot more changing in this function in later patches that will all read better: diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index 517b6fd351b63a..f7559835b0610f 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -263,20 +263,20 @@ int mdev_device_create(struct mdev_type *type, const guid_t *uuid) /* Check if parent unregistration has started */ if (!down_read_trylock(&parent->unreg_sem)) { ret = -ENODEV; - goto mdev_fail; + goto out_put_device; } ret = parent->ops->create(&type->kobj, mdev); if (ret) - goto ops_create_fail; + goto out_unlock; ret = device_add(&mdev->dev); if (ret) - goto add_fail; + goto out_remove; ret = mdev_create_sysfs_files(mdev); if (ret) - goto sysfs_fail; + goto out_del; mdev->active = true; dev_dbg(&mdev->dev, "MDEV: created\n"); @@ -284,13 +284,13 @@ int mdev_device_create(struct mdev_type *type, const guid_t *uuid) return 0; -sysfs_fail: +out_del: device_del(&mdev->dev); -add_fail: +out_remove: parent->ops->remove(mdev); -ops_create_fail: +out_unlock: up_read(&parent->unreg_sem); -mdev_fail: +out_put_device: put_device(&mdev->dev); return ret; } Thanks, Jason
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index 7ec21c907397a5..517b6fd351b63a 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -71,7 +71,6 @@ static void mdev_device_remove_common(struct mdev_device *mdev) /* Balances with device_initialize() */ put_device(&mdev->dev); - mdev_put_parent(parent); } static int mdev_device_remove_cb(struct device *dev, void *data) @@ -208,8 +207,13 @@ void mdev_unregister_device(struct device *dev) } EXPORT_SYMBOL(mdev_unregister_device); -static void mdev_device_free(struct mdev_device *mdev) +static void mdev_device_release(struct device *dev) { + struct mdev_device *mdev = to_mdev_device(dev); + + /* Pairs with the get in mdev_device_create() */ + mdev_put_parent(mdev->parent); + mutex_lock(&mdev_list_lock); list_del(&mdev->next); mutex_unlock(&mdev_list_lock); @@ -218,59 +222,50 @@ static void mdev_device_free(struct mdev_device *mdev) kfree(mdev); } -static void mdev_device_release(struct device *dev) -{ - struct mdev_device *mdev = to_mdev_device(dev); - - mdev_device_free(mdev); -} - int mdev_device_create(struct mdev_type *type, const guid_t *uuid) { int ret; struct mdev_device *mdev, *tmp; struct mdev_parent *parent = type->parent; - mdev_get_parent(parent); mutex_lock(&mdev_list_lock); /* Check for duplicate */ list_for_each_entry(tmp, &mdev_list, next) { if (guid_equal(&tmp->uuid, uuid)) { mutex_unlock(&mdev_list_lock); - ret = -EEXIST; - goto mdev_fail; + return -EEXIST; } } mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); if (!mdev) { mutex_unlock(&mdev_list_lock); - ret = -ENOMEM; - goto mdev_fail; + return -ENOMEM; } + device_initialize(&mdev->dev); + mdev->dev.parent = parent->dev; + mdev->dev.bus = &mdev_bus_type; + mdev->dev.release = mdev_device_release; + mdev->dev.groups = parent->ops->mdev_attr_groups; + mdev->type = type; + mdev->parent = parent; + /* Pairs with the put in mdev_device_release() */ + mdev_get_parent(parent); + guid_copy(&mdev->uuid, uuid); list_add(&mdev->next, &mdev_list); mutex_unlock(&mdev_list_lock); - mdev->parent = parent; + dev_set_name(&mdev->dev, "%pUl", uuid); /* Check if parent unregistration has started */ if (!down_read_trylock(&parent->unreg_sem)) { - mdev_device_free(mdev); ret = -ENODEV; goto mdev_fail; } - device_initialize(&mdev->dev); - mdev->dev.parent = parent->dev; - mdev->dev.bus = &mdev_bus_type; - mdev->dev.release = mdev_device_release; - dev_set_name(&mdev->dev, "%pUl", uuid); - mdev->dev.groups = parent->ops->mdev_attr_groups; - mdev->type = type; - ret = parent->ops->create(&type->kobj, mdev); if (ret) goto ops_create_fail; @@ -295,9 +290,8 @@ int mdev_device_create(struct mdev_type *type, const guid_t *uuid) parent->ops->remove(mdev); ops_create_fail: up_read(&parent->unreg_sem); - put_device(&mdev->dev); mdev_fail: - mdev_put_parent(parent); + put_device(&mdev->dev); return ret; }
Once the memory for the struct mdev_device is allocated it should immediately be device_initialize()'d and filled in so that put_device() can always be used to undo the allocation. Place the mdev_get/put_parent() so that they are clearly protecting the mdev->parent pointer. Move the final put to the release function so that the lifetime rules are trivial to understand. Remove mdev_device_free() as the release function via device_put() is now usable in all cases. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/mdev/mdev_core.c | 46 +++++++++++++++-------------------- 1 file changed, 20 insertions(+), 26 deletions(-)