> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Tuesday, September 4, 2018 5:54 PM
> To: Leon Romanovsky <leon@xxxxxxxxxx>
> Cc: Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky
> <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>;
> Daniel Jurgens <danielj@xxxxxxxxxxxx>; Parav Pandit <parav@xxxxxxxxxxxx>
> Subject: Re: [PATCH rdma-next 1/4] RDMA/uverbs: Fix error cleanup path of
> ib_uverbs_add_one()
>
> On Tue, Aug 28, 2018 at 03:05:07PM +0300, Leon Romanovsky wrote:
> > From: Parav Pandit <parav@xxxxxxxxxxxx>
> >
> > (a) If ib_uverbs_create_uapi() fails, dev_num should be freed.
> > (b) Destroy uapi if fails to create device.
> >
> > Fixes: 7d96c9b17636 ("IB/uverbs: Have the core code create the
> > uverbs_root_spec")
> > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > ---
> > drivers/infiniband/core/uverbs_main.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/uverbs_main.c
> > b/drivers/infiniband/core/uverbs_main.c
> > index 823beca448e1..57495a93017b 100644
> > --- a/drivers/infiniband/core/uverbs_main.c
> > +++ b/drivers/infiniband/core/uverbs_main.c
> > @@ -1050,7 +1050,7 @@ static void ib_uverbs_add_one(struct ib_device
> *device)
> > uverbs_dev->num_comp_vectors = device->num_comp_vectors;
> >
> > if (ib_uverbs_create_uapi(device, uverbs_dev))
> > - goto err;
> > + goto err_uapi;
> >
> > cdev_init(&uverbs_dev->cdev, NULL);
> > uverbs_dev->cdev.owner = THIS_MODULE; @@ -1077,11 +1077,11 @@
> static
> > void ib_uverbs_add_one(struct ib_device *device)
> >
> > err_class:
> > device_destroy(uverbs_class, uverbs_dev->cdev.dev);
> > -
> > err_cdev:
> > + uverbs_destroy_api(uverbs_dev->uapi);
> > cdev_del(&uverbs_dev->cdev);
> > +err_uapi:
> > clear_bit(devnum, dev_map);
> > -
> > err:
> > if (atomic_dec_and_test(&uverbs_dev->refcount))
> > ib_uverbs_comp_dev(uverbs_dev);
>
> Actually, this change to uverbs_destroy_api is wrong, so I'm not going to apply
> this patch..
>
> The control flow looks like this:
>
> static void ib_uverbs_add_one(struct ib_device *device) {
> kobject_init(&uverbs_dev->kobj, &ib_uverbs_dev_ktype);
> if (ib_uverbs_create_uapi(device, uverbs_dev))
> [..]
> if (cdev_add(&uverbs_dev->cdev, base, 1))
> goto err_cdev;
> err_cdev:
> [..]
> kobject_put(&uverbs_dev->kobj);
> return;
>
> And ib_uverbs_release_dev() is called by kobject_put, and it does:
>
> static void ib_uverbs_release_dev(struct kobject *kobj) {
> uverbs_destroy_api(dev->uapi);
>
> So, this is now double freeing uapi if we add another call to uverbs_destroy_api.
>
Right.
> The issue with the devnum is real though, but it is simpler to solve it by moving
> the ib_uverbs_create_uapi up to before the set_bit.
>
No need for more changes.
Just removing uverbs_destroy_api() in error fine is fine. Kobject_put is cleaning up later on.
Sending v1 from Leon's queue.
> Please respin the series..
>
> Jason
