On Thu, Feb 14, 2019 at 08:27:56AM +0200, Leon Romanovsky wrote:
> > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> > index 5ac143f22df009..15f31c915cd1fd 100644
> > +++ b/drivers/infiniband/core/uverbs_cmd.c
> > @@ -430,12 +430,10 @@ static int ib_uverbs_alloc_pd(struct uverbs_attr_bundle *attrs)
> >
> > ret = uverbs_response(attrs, &resp, sizeof(resp));
> > if (ret)
> > - goto err_copy;
> > + goto err;
> >
> > return uobj_alloc_commit(uobj);
> >
> > -err_copy:
> > - ib_dealloc_pd(pd);
> > err_alloc:
> > kfree(pd);
>
>
> It looks even stranger, you are doing out of order cleanup. IMHO, it is
> worse than "pd = NULL".
Like I said, we can at least fix this with some more work..
The sensible way to handle this is to make the uobject allocation
always come along with the necessary driver memory:
pd = uobj_alloc_drv(&uobj, UVERBS_OBJECT_PD, attrs, &ib_dev);
if (IS_ERR(pd))
return PTR_ERR(uobj);
// ie pd == uobj->object == rdma_zalloc_drv_obj()
ret = ib_dev->ops.alloc_pd(pd, ..);
if (ret)
uobj_alloc_abort(uobj); // does kfree
uobj_completed_hw_setup(uobj);
...
if (ret)
uobj_alloc_abort(uobj); // does hw_destroy
This is now really quite a clean pattern - the uobject always owns the
driver HW object and always frees it as part of uobj cleanup.
Jason
