[PATCH] iscsi-target: Fix memory leak if iscsit_alloc_buffs() fails

[PATCH] iscsi-target: Fix memory leak if iscsit_alloc_buffs() fails

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

The function kzalloc()s sgl, but if page allocation fails, it never
frees sgl on the page_alloc_failed path.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/target/iscsi/iscsi_target.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 9cd2837..529bf3b 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -830,6 +830,7 @@ page_alloc_failed:
 		__free_page(sg_page(&sgl[i]));
 		i--;
 	}
+	kfree(sgl);
 	kfree(cmd->t_mem_sg);
 	cmd->t_mem_sg = NULL;
 	return -ENOMEM;
-- 
1.7.9

Re: [PATCH] iscsi-target: Fix memory leak if iscsit_alloc_buffs() fails

[Nicholas A. Bellinger]

On Thu, 2012-02-23 at 17:28 -0800, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> The function kzalloc()s sgl, but if page allocation fails, it never
> frees sgl on the page_alloc_failed path.
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/target/iscsi/iscsi_target.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 9cd2837..529bf3b 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -830,6 +830,7 @@ page_alloc_failed:
>  		__free_page(sg_page(&sgl[i]));
>  		i--;
>  	}
> +	kfree(sgl);
>  	kfree(cmd->t_mem_sg);
>  	cmd->t_mem_sg = NULL;
>  	return -ENOMEM;

This looks like a double free here on the second failure of
iscsit_allocate_iovecs().  Fixing this up now to drop the extra bogus
kfree(cmd->t_mem_sg) here..

--nab

Re: [PATCH] iscsi-target: Fix memory leak if iscsit_alloc_buffs() fails

[Roland Dreier]

On Fri, Feb 24, 2012 at 5:17 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> This looks like a double free here on the second failure of
> iscsit_allocate_iovecs().  Fixing this up now to drop the extra bogus
> kfree(cmd->t_mem_sg) here..

I don't see how the patch you committed:

--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -830,7 +830,7 @@ page_alloc_failed:
                __free_page(sg_page(&sgl[i]));
                i--;
        }
-       kfree(cmd->t_mem_sg);
+       kfree(sgl);
        cmd->t_mem_sg = NULL;
        return -ENOMEM;
 }

could possibly be right... you drop the kfree() of cmd->t_mem_sg but leave
in the "cmd->t_mem_sg = NULL".  Isn't that a guaranteed leak of cmd->t_mem_sg?

I think the old code was probably OK (although I get lost a bit in the
twisty maze of what exactly happens to the cmd on this failure path), and
maybe the new code would be OK if you got rid of the "cmd->t_mem_sg = NULL".

 - R.

Re: [PATCH] iscsi-target: Fix memory leak if iscsit_alloc_buffs() fails

[Nicholas A. Bellinger]

On Fri, 2012-02-24 at 21:19 -0800, Roland Dreier wrote:
> On Fri, Feb 24, 2012 at 5:17 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> > This looks like a double free here on the second failure of
> > iscsit_allocate_iovecs().  Fixing this up now to drop the extra bogus
> > kfree(cmd->t_mem_sg) here..
> 
> I don't see how the patch you committed:
> 
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -830,7 +830,7 @@ page_alloc_failed:
>                 __free_page(sg_page(&sgl[i]));
>                 i--;
>         }
> -       kfree(cmd->t_mem_sg);
> +       kfree(sgl);
>         cmd->t_mem_sg = NULL;
>         return -ENOMEM;
>  }
> 
> could possibly be right... you drop the kfree() of cmd->t_mem_sg but leave
> in the "cmd->t_mem_sg = NULL".  Isn't that a guaranteed leak of cmd->t_mem_sg?
> 

It's the same memory that's being allocated in iscsit_alloc_buffs(), but
you're right that this is bogus for the iscsit_allocate_iovecs() failure
case.

> I think the old code was probably OK (although I get lost a bit in the
> twisty maze of what exactly happens to the cmd on this failure path), and
> maybe the new code would be OK if you got rid of the "cmd->t_mem_sg = NULL".
> 

After testing with simulated alloc_page() + iscsit_allocate_iovecs()
failures in iscsit_alloc_buffs(), the old code was in fact doing the
wrong thing.

Here is the incremental patch that I'm pushing for lio-core, and will
get this fixed up in for-next shortly.

Thanks,

--nab

commit 742c0d2e4436c7bc3c62c07dd838c7f52b3ccdcf
Author: Nicholas Bellinger <nab@linux-iscsi.org>
Date:   Sat Feb 25 12:18:48 2012 -0800

    iscsi-target: Fix iscsit_alloc_buffs() failure cases
    
    Make iscsit_alloc_buffs() failure case for page_alloc_failed use correct
    __free_page() SGL pointer, and return -ENOMEM for iscsit_allocate_iovecs
    failure to push se_cmd->t_mem_sg release into iscsit_release_cmd()
    callback during iscsit_add_reject_from_cmd() connection reset.
    
    Also drop cmd->t_mem_sg = NULL assignment from page_alloc_failed
    failure case.
    
    Cc: Roland Dreier <roland@purestorage.com>
    Cc: Andy Grover <agrover@redhat.com>
    Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 3e18efd..eb25737 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -780,7 +780,7 @@ static int iscsit_alloc_buffs(struct iscsi_cmd *cmd)
        struct scatterlist *sgl;
        u32 length = cmd->se_cmd.data_length;
        int nents = DIV_ROUND_UP(length, PAGE_SIZE);
-       int i = 0, ret;
+       int i = 0, j = 0, ret;
        /*
         * If no SCSI payload is present, allocate the default iovecs used for
         * iSCSI PDU Header
@@ -821,17 +821,15 @@ static int iscsit_alloc_buffs(struct iscsi_cmd *cmd)
         */
         ret = iscsit_allocate_iovecs(cmd);
         if (ret < 0)
-               goto page_alloc_failed;
+               return -ENOMEM;
 
        return 0;
 
 page_alloc_failed:
-       while (i >= 0) {
-               __free_page(sg_page(&sgl[i]));
-               i--;
-       }
+       while (j < i)
+               __free_page(sg_page(&sgl[j++]));
+
        kfree(sgl);
-       cmd->t_mem_sg = NULL;
        return -ENOMEM;
 }

Re: [PATCH] iscsi-target: Fix memory leak if iscsit_alloc_buffs() fails

[Christoph Hellwig]

On Thu, Feb 23, 2012 at 05:28:43PM -0800, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> The function kzalloc()s sgl, but if page allocation fails, it never
> frees sgl on the page_alloc_failed path.

Can we fix this properly by killing the code?  After the last round
of cleanups there is no reason why iscsi can't use the generic page
allocation code in transport_generic_get_mem().

Re: [PATCH] iscsi-target: Fix memory leak if iscsit_alloc_buffs() fails

[Christoph Hellwig]

On Sat, Feb 25, 2012 at 04:24:55AM -0500, Christoph Hellwig wrote:
> On Thu, Feb 23, 2012 at 05:28:43PM -0800, Roland Dreier wrote:
> > From: Roland Dreier <roland@purestorage.com>
> > 
> > The function kzalloc()s sgl, but if page allocation fails, it never
> > frees sgl on the page_alloc_failed path.
> 
> Can we fix this properly by killing the code?  After the last round
> of cleanups there is no reason why iscsi can't use the generic page
> allocation code in transport_generic_get_mem().

Nic?  Looks like the original version got in instead.