> -----Original Message-----
> From: Leon Romanovsky <leon@xxxxxxxxxx>
> Sent: Tuesday, September 18, 2018 2:46 AM
> To: Parav Pandit <parav@xxxxxxxxxxxx>
> Cc: Jason Gunthorpe <jgg@xxxxxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] RDMA/uverbs: Fix error unwind in ib_uverbs_add_one
>
> On Tue, Sep 18, 2018 at 06:41:30AM +0000, Parav Pandit wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jason Gunthorpe
> > > Sent: Monday, September 17, 2018 4:45 PM
> > > To: linux-rdma@xxxxxxxxxxxxxxx; Parav Pandit <parav@xxxxxxxxxxxx>
> > > Subject: [PATCH] RDMA/uverbs: Fix error unwind in ib_uverbs_add_one
> > >
> > > The error path has several mistakes
> > >
> > > - cdev_del should not be called if cdev_device_add fails
> > > - We must call put_device on all the goto exit paths
> > > as that is what frees the uapi, SRCU and the struct itself.
> > >
> > > Fixes: c5c4d92e70f3 ("RDMA/uverbs: Use cdev_device_add() instead of
> > > cdev_add()")
> > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > > ---
> > > drivers/infiniband/core/uverbs_main.c | 15 +++++++--------
> > > 1 file changed, 7 insertions(+), 8 deletions(-)
> > >
> > > Parav, yes?
> > >
> > Looks good.
> >
> > > diff --git a/drivers/infiniband/core/uverbs_main.c
> > > b/drivers/infiniband/core/uverbs_main.c
> > > index 16e5f714ca536e..ed8db441adb90e 100644
> > > --- a/drivers/infiniband/core/uverbs_main.c
> > > +++ b/drivers/infiniband/core/uverbs_main.c
> > > @@ -1031,6 +1031,11 @@ static void ib_uverbs_add_one(struct
> ib_device
> > > *device)
> > > return;
> > > }
> > >
> > > + device_initialize(&uverbs_dev->dev);
> > > + uverbs_dev->dev.class = uverbs_class;
> > > + uverbs_dev->dev.parent = device->dev.parent;
> > > + uverbs_dev->dev.release = ib_uverbs_release_dev;
> > > +
> > Can you please move below two lines also here so that most static fields
> gets initialized together and its easy to read.
> > uverbs_dev->groups[0] = &dev_attr_group;
> > uverbs_dev->dev.groups = uverbs_dev->groups;
> >
> > > atomic_set(&uverbs_dev->refcount, 1);
> > > init_completion(&uverbs_dev->comp);
> > > uverbs_dev->xrcd_tree = RB_ROOT;
> > > @@ -1055,11 +1060,7 @@ static void ib_uverbs_add_one(struct
> ib_device
> > > *device)
> > > if (ib_uverbs_create_uapi(device, uverbs_dev))
> > > goto err_uapi;
> > >
> > > - device_initialize(&uverbs_dev->dev);
> > > - uverbs_dev->dev.class = uverbs_class;
> > > - uverbs_dev->dev.parent = device->dev.parent;
> > > uverbs_dev->dev.devt = base;
> > > - uverbs_dev->dev.release = ib_uverbs_release_dev;
> > > uverbs_dev->groups[0] = &dev_attr_group;
> > > uverbs_dev->dev.groups = uverbs_dev->groups;
> > > dev_set_name(&uverbs_dev->dev, "uverbs%d", uverbs_dev-
> > > >devnum); @@ -1070,20 +1071,18 @@ static void
> ib_uverbs_add_one(struct
> > > ib_device *device)
> > >
> > > ret = cdev_device_add(&uverbs_dev->cdev, &uverbs_dev->dev);
> > > if (ret)
> > > - goto err_cdev;
> > > + goto err_uapi;
> > >
> > > ib_set_client_data(device, &uverbs_client, uverbs_dev);
> > > return;
> > >
> > > -err_cdev:
> > > - cdev_del(&uverbs_dev->cdev);
> > > - put_device(&uverbs_dev->dev);
> > > err_uapi:
> > > clear_bit(devnum, dev_map);
> > > err:
> > > if (atomic_dec_and_test(&uverbs_dev->refcount))
> > > ib_uverbs_comp_dev(uverbs_dev);
> > > wait_for_completion(&uverbs_dev->comp);
> > > + put_device(&uverbs_dev->dev);
> > > return;
> > > }
> > >
> > > --
> > > 2.19.0
> >
> > Reviewed-by: parav@xxxxxxxxxxxx
> >
>
> Parav,
>
> You need to write full tag, so it will be recorded in patchworks.
>
Sorry.
Here it is.
Reviewed-by: Parav Pandit <parav@xxxxxxxxxxxx>
> Thanks,
