infiniband: ulp/iser, fix error retval in iser_create_ib_conn_res

PATCH In review Jiri Slaby 4 participants 8 messages Mar 16, 2010 – May 6, 2010

Subsystems (2)

InfiniBand iSCSI extensions for RDMA (iSER) initiator

lore

Share & tools

lore

Jiri Slaby

author

2010-03-16 14:49 UTC #0 lore raw

PATCH 1/1 infiniband: ulp/iser, fix error retval in iser_create_ib_conn_res

Stanse foudn unreachable code in iser_create_ib_conn_res. Namely there is switched goto and ret assignment. Switch them to make sense and return ENOMEM in case of error.

Signed-off-by Jiri Slaby <[email protected]>

Cc Or Gerlitz <[email protected]>

Cc Roland Dreier <[email protected]>

---
 drivers/infiniband/ulp/iser/iser_verbs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/infiniband/ulp/iser/iser_verbs.c +1 -1

index 308d17b..6d4ba35 100644
@@ -149,8 +149,8 @@ static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 
 	ib_conn->login_buf = kmalloc(ISER_RX_LOGIN_SIZE, GFP_KERNEL);
 	if (!ib_conn->login_buf) {
-		goto alloc_err;
 		ret = -ENOMEM;
+		goto alloc_err;
 	}
 
 	ib_conn->login_dma = ib_dma_map_single(ib_conn->device->ib_device,

Signature · 1 line

1.7.0.1

Roland Dreier

2010-03-16 23:52 UTC #1 lore raw

drivers/infiniband/ulp/iser/iser_verbs.c static int iser_create_ib_conn_res(struct iser_conn *ib_conn) · line 154

Quoted 6 lines

drivers/infiniband/ulp/iser/iser_verbs.c +1 -1

 	ib_conn->login_buf = kmalloc(ISER_RX_LOGIN_SIZE, GFP_KERNEL);
 	if (!ib_conn->login_buf) {
-		goto alloc_err;
 		ret = -ENOMEM;
+		goto alloc_err;
 	}

This looks like a valid fix but it's the same part of the code that Dan found more extensive bugs in. Or, can I assume you'll sort this out and make sure a final patch is sorted out?

Thanks,
Roland

Signature · 3 lines

Roland Dreier  <[email protected]>
For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Dan Carpenter

2010-03-17 15:11 UTC #2 lore raw

Quoted 11 lines · Roland Dreier

drivers/infiniband/ulp/iser/iser_verbs.c static int iser_create_ib_conn_res(struct iser_conn *ib_conn) · line 154
Quoted 6 lines
drivers/infiniband/ulp/iser/iser_verbs.c +1 -1
 	ib_conn->login_buf = kmalloc(ISER_RX_LOGIN_SIZE, GFP_KERNEL);
 	if (!ib_conn->login_buf) {
-		goto alloc_err;
 		ret = -ENOMEM;
+		goto alloc_err;
 	}
This looks like a valid fix but it's the same part of the code that Dan found more extensive bugs in. Or, can I assume you'll sort this out and make sure a final patch is sorted out?

My patch is the one to take. It addresses this problem here and fixes some double frees.

http://marc.info/?l=linux-rdma&m=126872745710910&w=2

Sorry about that Jiri. Btw. Julia Lawall, Daren Jenkins and I normally CC kernel-janitors for static checker patches.

regards,
dan carpenter

Quoted 6 lines

Thanks,
Roland

Signature · 3 lines

Roland Dreier  <[email protected]>
For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Roland Dreier

2010-03-31 21:06 UTC #3 lore raw

So looking at merging this finally, I think I see one problem with the proposed patch. We have:

Review static int iser_create_ib_conn_res(struct iser_conn *ib_conn) · line 184

Quoted 7 lines

(quoted diff) +1 -1

@@ -183,7 +180,7 @@  static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 	ib_conn->fmr_pool = ib_create_fmr_pool(device->pd, &params);
 	if (IS_ERR(ib_conn->fmr_pool)) {
 		ret = PTR_ERR(ib_conn->fmr_pool);
-		goto fmr_pool_err;
+		goto out_err;
 	}

and

Quoted 4 lines

(quoted diff) +0 -0

@@ -209,12 +206,7 @@  static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 		 ib_conn->fmr_pool, ib_conn->cma_id->qp);
 	return ret;

Review static int iser_create_ib_conn_res(struct iser_conn *ib_conn) · line 212

Quoted 10 lines

(quoted diff) +1 -6

-qp_err:
-	(void)ib_destroy_fmr_pool(ib_conn->fmr_pool);
-fmr_pool_err:
-	kfree(ib_conn->page_vec);
-	kfree(ib_conn->login_buf);
-alloc_err:
+out_err:
 	iser_err("unable to alloc mem or create resource, err %d\n", ret);
 	return ret;
 }

so if ib_create_fmr_pool() fails, we're left with ib_conn->fmr_pool holding an error pointer, right? But we're relying on iser_free_ib_conn_res() to clean up after us, and that has:

	if (ib_conn->fmr_pool != NULL)
		ib_destroy_fmr_pool(ib_conn->fmr_pool);

so we're going to end up trying to free an error pointer, which will probably crash.

I think.

Dan or Or, am I wrong here or do we need another iteration of this patch? (the login_buf and page_vec changes do look correct to me, since a failed kmalloc() will leave us with a NULL pointer that it is safe to kfree() later)

- R.

Signature · 2 lines

Roland Dreier <[email protected]> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Dan Carpenter

2010-04-01 15:39 UTC #4 lore raw

Quoted 38 lines · Roland Dreier

So looking at merging this finally, I think I see one problem with the proposed patch. We have:
Review static int iser_create_ib_conn_res(struct iser_conn *ib_conn) · line 184
Quoted 7 lines
(quoted diff) +1 -1
@@ -183,7 +180,7 @@  static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 	ib_conn->fmr_pool = ib_create_fmr_pool(device->pd, &params);
 	if (IS_ERR(ib_conn->fmr_pool)) {
 		ret = PTR_ERR(ib_conn->fmr_pool);
-		goto fmr_pool_err;
+		goto out_err;
 	}
and
Quoted 4 lines
(quoted diff) +0 -0
@@ -209,12 +206,7 @@  static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 		 ib_conn->fmr_pool, ib_conn->cma_id->qp);
 	return ret;
 
Review static int iser_create_ib_conn_res(struct iser_conn *ib_conn) · line 212
Quoted 10 lines
(quoted diff) +1 -6
-qp_err:
-	(void)ib_destroy_fmr_pool(ib_conn->fmr_pool);
-fmr_pool_err:
-	kfree(ib_conn->page_vec);
-	kfree(ib_conn->login_buf);
-alloc_err:
+out_err:
 	iser_err("unable to alloc mem or create resource, err %d\n", ret);
 	return ret;
 }
so if ib_create_fmr_pool() fails, we're left with ib_conn->fmr_pool holding an error pointer, right? But we're relying on iser_free_ib_conn_res() to clean up after us, and that has:
	if (ib_conn->fmr_pool != NULL)
		ib_destroy_fmr_pool(ib_conn->fmr_pool);
so we're going to end up trying to free an error pointer, which will probably crash.

Quoted 6 lines

I think.
Dan or Or, am I wrong here or do we need another iteration of this patch? (the login_buf and page_vec changes do look correct to me, since a failed kmalloc() will leave us with a NULL pointer that it is safe to kfree() later)

You're right. I missed that.

I will send an updated patch tomorrow.

regards,
dan carpenter

Quoted 5 lines

- R.

Signature · 2 lines

Roland Dreier <[email protected]> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Dan Carpenter

2010-04-02 11:34 UTC #5 lore raw

PATCH v3 1/1 infiniband: ulp/iser, fix error retval in iser_create_ib_conn_res

In reply to Roland Dreier

We shouldn't free things here because we free them later. The call tree looks like this:
iser_connect() => iser_cma_handler() => iser_route_handler()
=> iser_create_ib_conn_res()

We fail here and return failures back down the tree.

iser_connect() => iser_conn_release()

Double free.

Signed-off-by Dan Carpenter <[email protected]>

---
V1 fixed unreachable code
V2 noticed that the original code had a double free V3 Roland Dreier points out that I left a dangling ERR_PTR() in
"ib_conn->fmr_pool" which would be freed later on.

drivers/infiniband/ulp/iser/iser_verbs.c +9 ~~-16~~

index 308d17b..ccab795 100644
@@ -148,10 +148,8 @@ static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 	device = ib_conn->device;
 
 	ib_conn->login_buf = kmalloc(ISER_RX_LOGIN_SIZE, GFP_KERNEL);
-	if (!ib_conn->login_buf) {
-		goto alloc_err;
-		ret = -ENOMEM;
-	}
+	if (!ib_conn->login_buf)
+		goto out_err;
 
 	ib_conn->login_dma = ib_dma_map_single(ib_conn->device->ib_device,
 				(void *)ib_conn->login_buf, ISER_RX_LOGIN_SIZE,

@@ -160,10 +158,9 @@ static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 	ib_conn->page_vec = kmalloc(sizeof(struct iser_page_vec) +
 				    (sizeof(u64) * (ISCSI_ISER_SG_TABLESIZE +1)),
 				    GFP_KERNEL);
-	if (!ib_conn->page_vec) {
-		ret = -ENOMEM;
-		goto alloc_err;
-	}
+	if (!ib_conn->page_vec)
+		goto out_err;
+
 	ib_conn->page_vec->pages = (u64 *) (ib_conn->page_vec + 1);
 
 	params.page_shift        = SHIFT_4K;

@@ -183,7 +180,8 @@ static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 	ib_conn->fmr_pool = ib_create_fmr_pool(device->pd, &params);
 	if (IS_ERR(ib_conn->fmr_pool)) {
 		ret = PTR_ERR(ib_conn->fmr_pool);
-		goto fmr_pool_err;
+		ib_conn->fmr_pool = NULL;
+		goto out_err;
 	}
 
 	memset(&init_attr, 0, sizeof init_attr);

@@ -201,7 +199,7 @@ static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 
 	ret = rdma_create_qp(ib_conn->cma_id, device->pd, &init_attr);
 	if (ret)
-		goto qp_err;
+		goto out_err;
 
 	ib_conn->qp = ib_conn->cma_id->qp;
 	iser_err("setting conn %p cma_id %p: fmr_pool %p qp %p\n",

@@ -209,12 +207,7 @@ static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 		 ib_conn->fmr_pool, ib_conn->cma_id->qp);
 	return ret;
 
-qp_err:
-	(void)ib_destroy_fmr_pool(ib_conn->fmr_pool);
-fmr_pool_err:
-	kfree(ib_conn->page_vec);
-	kfree(ib_conn->login_buf);
-alloc_err:
+out_err:
 	iser_err("unable to alloc mem or create resource, err %d\n", ret);
 	return ret;
 }

Roland Dreier

2010-05-05 18:09 UTC #6 lore raw

Or, I don't think we ever fixed this. This patch looks correct to me, any problem with merging this for 2.6.35?

Signature · 2 lines

Roland Dreier <[email protected]> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Or Gerlitz

2010-05-06 13:22 UTC #7 lore raw

Quoted 2 lines · Roland Dreier

Or, I don't think we ever fixed this. This patch looks correct to me, any problem with merging this for 2.6.35?

Roland, please use V4 below, the patch is okay and would apply before and after applying the multipathing patches I sent yesterday (same goes for them).

[PATCH V4] ib/iser: fix error flow in iser_create_ib_conn_res

From: Dan Carpenter <[email protected]>

We shouldn't free things here because we free them later. The call tree looks like this:

	iser_connect() ==> initiating the connection establishment
and later 
	iser_cma_handler() => iser_route_handler() => iser_create_ib_conn_res()

if we fail here, eventually iser_conn_release() is called, resulted in double free.

Signed-off-by Dan Carpenter <[email protected]>

Signed-off-by Or Gerlitz <[email protected]>

---
 drivers/infiniband/ulp/iser/iser_verbs.c |   25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

drivers/infiniband/ulp/iser/iser_verbs.c +9 ~~-16~~

Index: linux-2.6.34-rc6/drivers/infiniband/ulp/iser/iser_verbs.c
===================================================================
@@ -163,10 +163,8 @@ static int iser_create_ib_conn_res(struc
 	device = ib_conn->device;
 
 	ib_conn->login_buf = kmalloc(ISER_RX_LOGIN_SIZE, GFP_KERNEL);
-	if (!ib_conn->login_buf) {
-		goto alloc_err;
-		ret = -ENOMEM;
-	}
+	if (!ib_conn->login_buf)
+		goto out_err;
 
 	ib_conn->login_dma = ib_dma_map_single(ib_conn->device->ib_device,
 				(void *)ib_conn->login_buf, ISER_RX_LOGIN_SIZE,

@@ -175,10 +173,9 @@ static int iser_create_ib_conn_res(struc
 	ib_conn->page_vec = kmalloc(sizeof(struct iser_page_vec) +
 				    (sizeof(u64) * (ISCSI_ISER_SG_TABLESIZE +1)),
 				    GFP_KERNEL);
-	if (!ib_conn->page_vec) {
-		ret = -ENOMEM;
-		goto alloc_err;
-	}
+	if (!ib_conn->page_vec)
+		goto out_err;
+
 	ib_conn->page_vec->pages = (u64 *) (ib_conn->page_vec + 1);
 
 	params.page_shift        = SHIFT_4K;

@@ -198,7 +195,8 @@ static int iser_create_ib_conn_res(struc
 	ib_conn->fmr_pool = ib_create_fmr_pool(device->pd, &params);
 	if (IS_ERR(ib_conn->fmr_pool)) {
 		ret = PTR_ERR(ib_conn->fmr_pool);
-		goto fmr_pool_err;
+		ib_conn->fmr_pool = NULL;
+		goto out_err;
 	}
 
 	memset(&init_attr, 0, sizeof init_attr);

@@ -216,7 +214,7 @@ static int iser_create_ib_conn_res(struc
 
 	ret = rdma_create_qp(ib_conn->cma_id, device->pd, &init_attr);
 	if (ret)
-		goto qp_err;
+		goto out_err;
 
 	ib_conn->qp = ib_conn->cma_id->qp;
 	iser_err("setting conn %p cma_id %p: fmr_pool %p qp %p\n",

@@ -224,12 +222,7 @@ static int iser_create_ib_conn_res(struc
 		 ib_conn->fmr_pool, ib_conn->cma_id->qp);
 	return ret;
 
-qp_err:
-	(void)ib_destroy_fmr_pool(ib_conn->fmr_pool);
-fmr_pool_err:
-	kfree(ib_conn->page_vec);
-	kfree(ib_conn->login_buf);
-alloc_err:
+out_err:
 	iser_err("unable to alloc mem or create resource, err %d\n", ret);
 	return ret;
 }