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

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

[Jiri Slaby]

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 <jslaby@suse.cz>
Cc: Or Gerlitz <ogerlitz@voltaire.com>
Cc: Roland Dreier <rolandd@cisco.com>
---
 drivers/infiniband/ulp/iser/iser_verbs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 308d17b..6d4ba35 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -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,
-- 
1.7.0.1

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

[Roland Dreier]

 >  	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
-- 
Roland Dreier  <rolandd@cisco.com>
For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

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

[Dan Carpenter]

On Tue, Mar 16, 2010 at 04:52:43PM -0700, Roland Dreier wrote:
>  >  	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

> Thanks,
>   Roland
> -- 
> Roland Dreier  <rolandd@cisco.com>
> For corporate legal information go to:
> http://www.cisco.com/web/about/doing_business/legal/cri/index.html

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

[Roland Dreier]

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

 > @@ -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

 > @@ -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;
 >  
 > -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.
-- 
Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

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

[Dan Carpenter]

On Wed, Mar 31, 2010 at 02:06:21PM -0700, Roland Dreier wrote:
> So looking at merging this finally, I think I see one problem with the
> proposed patch.  We have:
> 
>  > @@ -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
> 
>  > @@ -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;
>  >  
>  > -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)

You're right.  I missed that.

I will send an updated patch tomorrow.

regards,
dan carpenter

> 
>  - R.
> -- 
> Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
> http://www.cisco.com/web/about/doing_business/legal/cri/index.html

[patch v3] infiniband: ulp/iser, fix error retval in iser_create_ib_conn_res

[Dan Carpenter]

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 <error27@gmail.com>
---
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.

diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 308d17b..ccab795 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -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;
 }

Re: [patch v3] infiniband: ulp/iser, fix error retval in iser_create_ib_conn_res

[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 Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Re: [patch v3] infiniband: ulp/iser, fix error retval in iser_create_ib_conn_res

[Or Gerlitz]

Roland Dreier wrote:
> 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 <error27@gmail.com>

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 <error27@gmail.com>
Signed-off-by: Or Gerlitz <ogerlitz@voltaire.com>
---
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.
V4 reviewed/enhanced the change-log
---
 drivers/infiniband/ulp/iser/iser_verbs.c |   25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

Index: linux-2.6.34-rc6/drivers/infiniband/ulp/iser/iser_verbs.c
===================================================================
--- linux-2.6.34-rc6.orig/drivers/infiniband/ulp/iser/iser_verbs.c
+++ 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;
 }