Re: [PATCH rdma-next] RDMA/uverbs: Don't do double free of allocated PD

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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);
 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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux