On Wed, Feb 13, 2019 at 10:50:41PM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 13, 2019 at 07:07:05PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >
> > There is no need to call kfree(pd) because ib_dealloc_pd()
> > internally frees PD.
> >
> > Fixes: 21a428a019c9 ("RDMA: Handle PD allocations by IB/core")
> > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > ---
> > drivers/infiniband/core/uverbs_cmd.c | 1 +
> > 1 file changed, 1 insertion(+)
>
> > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> > index 4f70b34dc3fa..5425e0e534e2 100644
> > --- a/drivers/infiniband/core/uverbs_cmd.c
> > +++ b/drivers/infiniband/core/uverbs_cmd.c
> > @@ -440,6 +440,7 @@ static int ib_uverbs_alloc_pd(struct uverbs_attr_bundle *attrs)
> >
> > err_copy:
> > ib_dealloc_pd(pd);
> > + pd = NULL;
> > err_alloc:
> > kfree(pd);
> > err:
> > uobj_alloc_abort(uobj);
>
> Hum. This becomes quite ugly.. I had left it in this weird state
> because it was too big to fix at the time, but maybe we should have a
> go at it now...
>
> After the HW object is created it should immediately set the
> uobj->object and from then on the uobj system owns it and will be
> responsible to destroy it. ie rdma_alloc_abort() will fully destroy
> the thing.
>
> This makes all the old style flows pretty clean, but trades a sketchy
> pd = NULL for a sketchy out of order goto for the new flows, though I
> think we could tidy that further and reduce more repetative code with
> some small effort..
>
> Something like this as a first pass:
>
> drivers/infiniband/core/rdma_core.c | 1 -
> drivers/infiniband/core/uverbs_cmd.c | 85 ++++++-------------
> .../core/uverbs_std_types_counters.c | 10 +--
> drivers/infiniband/core/uverbs_std_types_cq.c | 10 +--
> 4 files changed, 28 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
> index a260d2f8e0b771..3599b97b00b59b 100644
> --- a/drivers/infiniband/core/rdma_core.c
> +++ b/drivers/infiniband/core/rdma_core.c
> @@ -679,7 +679,6 @@ void rdma_alloc_abort_uobject(struct ib_uobject *uobj)
> {
> struct ib_uverbs_file *ufile = uobj->ufile;
>
> - uobj->object = NULL;
> uverbs_destroy_uobject(uobj, RDMA_REMOVE_ABORT);
>
> /* Matches the down_read in rdma_alloc_begin_uobject */
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 5ac143f22df009..15f31c915cd1fd 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ 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".
> err:
> @@ -609,20 +607,20 @@ static int ib_uverbs_open_xrcd(struct uverbs_attr_bundle *attrs)
> memset(&resp, 0, sizeof resp);
> resp.xrcd_handle = obj->uobject.id;
>
> + ret = uverbs_response(attrs, &resp, sizeof(resp));
> + if (ret)
> + goto err;
> +
> if (inode) {
> if (new_xrcd) {
> /* create new inode/xrcd table entry */
> ret = xrcd_table_insert(ibudev, inode, xrcd);
> if (ret)
> - goto err_dealloc_xrcd;
> + goto err;
> }
> atomic_inc(&xrcd->usecnt);
> }
>
> - ret = uverbs_response(attrs, &resp, sizeof(resp));
> - if (ret)
> - goto err_copy;
> -
> if (f.file)
> fdput(f);
>
> @@ -630,16 +628,6 @@ static int ib_uverbs_open_xrcd(struct uverbs_attr_bundle *attrs)
>
> return uobj_alloc_commit(&obj->uobject);
>
> -err_copy:
> - if (inode) {
> - if (new_xrcd)
> - xrcd_table_delete(ibudev, inode);
> - atomic_dec(&xrcd->usecnt);
> - }
> -
> -err_dealloc_xrcd:
> - ib_dealloc_xrcd(xrcd);
> -
> err:
> uobj_alloc_abort(&obj->uobject);
>
> @@ -754,15 +742,12 @@ static int ib_uverbs_reg_mr(struct uverbs_attr_bundle *attrs)
>
> ret = uverbs_response(attrs, &resp, sizeof(resp));
> if (ret)
> - goto err_copy;
> + goto err_put;
>
> uobj_put_obj_read(pd);
>
> return uobj_alloc_commit(uobj);
>
> -err_copy:
> - ib_dereg_mr(mr);
> -
> err_put:
> uobj_put_obj_read(pd);
>
> @@ -905,13 +890,11 @@ static int ib_uverbs_alloc_mw(struct uverbs_attr_bundle *attrs)
>
> ret = uverbs_response(attrs, &resp, sizeof(resp));
> if (ret)
> - goto err_copy;
> + goto err_put;
>
> uobj_put_obj_read(pd);
> return uobj_alloc_commit(uobj);
>
> -err_copy:
> - uverbs_dealloc_mw(mw);
> err_put:
> uobj_put_obj_read(pd);
> err_free:
> @@ -1025,16 +1008,13 @@ static struct ib_ucq_object *create_cq(struct uverbs_attr_bundle *attrs,
>
> ret = uverbs_response(attrs, &resp, sizeof(resp));
> if (ret)
> - goto err_cb;
> + goto err;
>
> ret = uobj_alloc_commit(&obj->uobject);
> if (ret)
> return ERR_PTR(ret);
> return obj;
>
> -err_cb:
> - ib_destroy_cq(cq);
> -
> err_file:
> if (ev_file)
> ib_uverbs_release_ucq(attrs->ufile, ev_file, obj);
> @@ -1404,12 +1384,9 @@ static int create_qp(struct uverbs_attr_bundle *attrs,
> ret = PTR_ERR(qp);
> goto err_put;
> }
> + obj->uevent.uobject.object = qp;
>
> if (cmd->qp_type != IB_QPT_XRC_TGT) {
> - ret = ib_create_qp_security(qp, device);
> - if (ret)
> - goto err_cb;
> -
> qp->real_qp = qp;
> qp->pd = pd;
> qp->send_cq = attr.send_cq;
> @@ -1430,13 +1407,15 @@ static int create_qp(struct uverbs_attr_bundle *attrs,
> atomic_inc(&attr.srq->usecnt);
> if (ind_tbl)
> atomic_inc(&ind_tbl->usecnt);
> +
> + ret = ib_create_qp_security(qp, device);
> + if (ret)
> + goto err_put;
> } else {
> /* It is done in _ib_create_qp for other QP types */
> qp->uobject = &obj->uevent.uobject;
> }
>
> - obj->uevent.uobject.object = qp;
> -
> memset(&resp, 0, sizeof resp);
> resp.base.qpn = qp->qp_num;
> resp.base.qp_handle = obj->uevent.uobject.id;
> @@ -1449,7 +1428,7 @@ static int create_qp(struct uverbs_attr_bundle *attrs,
>
> ret = uverbs_response(attrs, &resp, sizeof(resp));
> if (ret)
> - goto err_cb;
> + goto err_put;
>
> if (xrcd) {
> obj->uxrcd = container_of(xrcd_uobj, struct ib_uxrcd_object,
> @@ -1470,8 +1449,6 @@ static int create_qp(struct uverbs_attr_bundle *attrs,
> uobj_put_obj_read(ind_tbl);
>
> return uobj_alloc_commit(&obj->uevent.uobject);
> -err_cb:
> - ib_destroy_qp(qp);
>
> err_put:
> if (!IS_ERR(xrcd_uobj))
> @@ -1594,7 +1571,7 @@ static int ib_uverbs_open_qp(struct uverbs_attr_bundle *attrs)
>
> ret = uverbs_response(attrs, &resp, sizeof(resp));
> if (ret)
> - goto err_destroy;
> + goto err_xrcd;
>
> obj->uxrcd = container_of(xrcd_uobj, struct ib_uxrcd_object, uobject);
> atomic_inc(&obj->uxrcd->refcnt);
> @@ -1603,8 +1580,6 @@ static int ib_uverbs_open_qp(struct uverbs_attr_bundle *attrs)
>
> return uobj_alloc_commit(&obj->uevent.uobject);
>
> -err_destroy:
> - ib_destroy_qp(qp);
> err_xrcd:
> uobj_put_read(xrcd_uobj);
> err_put:
> @@ -2440,14 +2415,11 @@ static int ib_uverbs_create_ah(struct uverbs_attr_bundle *attrs)
>
> ret = uverbs_response(attrs, &resp, sizeof(resp));
> if (ret)
> - goto err_copy;
> + goto err_put;
>
> uobj_put_obj_read(pd);
> return uobj_alloc_commit(uobj);
>
> -err_copy:
> - rdma_destroy_ah(ah, RDMA_DESTROY_AH_SLEEPABLE);
> -
> err_put:
> uobj_put_obj_read(pd);
>
> @@ -2950,14 +2922,12 @@ static int ib_uverbs_ex_create_wq(struct uverbs_attr_bundle *attrs)
> resp.response_length = uverbs_response_length(attrs, sizeof(resp));
> err = uverbs_response(attrs, &resp, sizeof(resp));
> if (err)
> - goto err_copy;
> + goto err_put_cq;
>
> uobj_put_obj_read(pd);
> uobj_put_obj_read(cq);
> return uobj_alloc_commit(&obj->uevent.uobject);
>
> -err_copy:
> - ib_destroy_wq(wq);
> err_put_cq:
> uobj_put_obj_read(cq);
> err_put_pd:
> @@ -3105,6 +3075,7 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
> goto err_uobj;
> }
>
> + wqs = NULL;
> rwq_ind_tbl->ind_tbl = wqs;
> rwq_ind_tbl->log_ind_tbl_size = init_attr.log_ind_tbl_size;
> rwq_ind_tbl->uobject = uobj;
> @@ -3121,7 +3092,7 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
>
> err = uverbs_response(attrs, &resp, sizeof(resp));
> if (err)
> - goto err_copy;
> + goto err_uobj;
>
> kfree(wqs_handles);
>
> @@ -3130,8 +3101,6 @@ static int ib_uverbs_ex_create_rwq_ind_table(struct uverbs_attr_bundle *attrs)
>
> return uobj_alloc_commit(uobj);
>
> -err_copy:
> - ib_destroy_rwq_ind_table(rwq_ind_tbl);
> err_uobj:
> uobj_alloc_abort(uobj);
> put_wqs:
> @@ -3293,23 +3262,20 @@ static int ib_uverbs_ex_create_flow(struct uverbs_attr_bundle *attrs)
> goto err_free;
> }
>
> - ib_set_flow(uobj, flow_id, qp, qp->device, uflow_res);
> -
> memset(&resp, 0, sizeof(resp));
> resp.flow_handle = uobj->id;
>
> err = uverbs_response(attrs, &resp, sizeof(resp));
> if (err)
> - goto err_copy;
> + goto err_free;
> +
> + ib_set_flow(uobj, flow_id, qp, qp->device, uflow_res);
>
> uobj_put_obj_read(qp);
> kfree(flow_attr);
> if (cmd.flow_attr.num_of_specs)
> kfree(kern_flow_attr);
> return uobj_alloc_commit(uobj);
> -err_copy:
> - if (!qp->device->ops.destroy_flow(flow_id))
> - atomic_dec(&qp->usecnt);
> err_free:
> ib_uverbs_flow_resources_free(uflow_res);
> err_free_flow_attr:
> @@ -3441,7 +3407,7 @@ static int __uverbs_create_xsrq(struct uverbs_attr_bundle *attrs,
>
> ret = uverbs_response(attrs, &resp, sizeof(resp));
> if (ret)
> - goto err_copy;
> + goto err;
>
> if (cmd->srq_type == IB_SRQT_XRC)
> uobj_put_read(xrcd_uobj);
> @@ -3452,9 +3418,6 @@ static int __uverbs_create_xsrq(struct uverbs_attr_bundle *attrs,
> uobj_put_obj_read(pd);
> return uobj_alloc_commit(&obj->uevent.uobject);
>
> -err_copy:
> - ib_destroy_srq(srq);
> -
> err_put:
> uobj_put_obj_read(pd);
>
> diff --git a/drivers/infiniband/core/uverbs_std_types_counters.c b/drivers/infiniband/core/uverbs_std_types_counters.c
> index 309c5e80988d2e..9b096d7e36cf59 100644
> --- a/drivers/infiniband/core/uverbs_std_types_counters.c
> +++ b/drivers/infiniband/core/uverbs_std_types_counters.c
> @@ -54,7 +54,6 @@ static int UVERBS_HANDLER(UVERBS_METHOD_COUNTERS_CREATE)(
> attrs, UVERBS_ATTR_CREATE_COUNTERS_HANDLE);
> struct ib_device *ib_dev = uobj->context->device;
> struct ib_counters *counters;
> - int ret;
>
> /*
> * This check should be removed once the infrastructure
> @@ -65,10 +64,8 @@ static int UVERBS_HANDLER(UVERBS_METHOD_COUNTERS_CREATE)(
> return -EOPNOTSUPP;
>
> counters = ib_dev->ops.create_counters(ib_dev, attrs);
> - if (IS_ERR(counters)) {
> - ret = PTR_ERR(counters);
> - goto err_create_counters;
> - }
> + if (IS_ERR(counters))
> + return PTR_ERR(counters);
>
> counters->device = ib_dev;
> counters->uobject = uobj;
> @@ -76,9 +73,6 @@ static int UVERBS_HANDLER(UVERBS_METHOD_COUNTERS_CREATE)(
> atomic_set(&counters->usecnt, 0);
>
> return 0;
> -
> -err_create_counters:
> - return ret;
> }
>
> static int UVERBS_HANDLER(UVERBS_METHOD_COUNTERS_READ)(
> diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c b/drivers/infiniband/core/uverbs_std_types_cq.c
> index a59ea89e3f2b06..f9f4c1460c7a58 100644
> --- a/drivers/infiniband/core/uverbs_std_types_cq.c
> +++ b/drivers/infiniband/core/uverbs_std_types_cq.c
> @@ -128,14 +128,8 @@ static int UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(
> cq->res.type = RDMA_RESTRACK_CQ;
> rdma_restrack_uadd(&cq->res);
>
> - ret = uverbs_copy_to(attrs, UVERBS_ATTR_CREATE_CQ_RESP_CQE, &cq->cqe,
> - sizeof(cq->cqe));
> - if (ret)
> - goto err_cq;
> -
> - return 0;
> -err_cq:
> - ib_destroy_cq(cq);
> + return uverbs_copy_to(attrs, UVERBS_ATTR_CREATE_CQ_RESP_CQE, &cq->cqe,
> + sizeof(cq->cqe));
>
> err_event_file:
> if (ev_file)
> --
> 2.20.1
>
