Re: [PATCH] SCSI, target: Avoid mem leak and needless work in transport_generic_get_mem().

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

On Sat, 2011-01-29 at 23:21 +0100, Jesper Juhl wrote:
> In drivers/target/target_core_transport.c::transport_generic_get_mem() 
> there are a few potential memory leaks in the error paths. This patch 
> makes sure that we free previously allocated memory when other allocations 
> fail. It also moves some work (INIT_LIST_HEAD() and assignment to 
> se_mem->se_len) below all the allocations so that if something fails we 
> don't do the work at all.
> 

Hi Jesper,

> Please review and consider for inclusion.
> I don't have any hardware to actually test this so it is compile tested 
> only.
> 

Btw, you don't need any special hardware to test this.  Just a
virtual NIC and a couple of VMs.  ;)

> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
>  target_core_transport.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 28b6292..4776293 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -4334,11 +4334,9 @@ transport_generic_get_mem(struct se_cmd *cmd, u32 length, u32 dma_size)
>  			printk(KERN_ERR "Unable to allocate struct se_mem\n");
>  			goto out;
>  		}
> -		INIT_LIST_HEAD(&se_mem->se_list);
> -		se_mem->se_len = (length > dma_size) ? dma_size : length;
>  
>  /* #warning FIXME Allocate contigous pages for struct se_mem elements */
> -		se_mem->se_page = (struct page *) alloc_pages(GFP_KERNEL, 0);
> +		se_mem->se_page = alloc_pages(GFP_KERNEL, 0);
>  		if (!(se_mem->se_page)) {
>  			printk(KERN_ERR "alloc_pages() failed\n");
>  			goto out;
> @@ -4349,6 +4347,8 @@ transport_generic_get_mem(struct se_cmd *cmd, u32 length, u32 dma_size)
>  			printk(KERN_ERR "kmap_atomic() failed\n");
>  			goto out;
>  		}
> +		INIT_LIST_HEAD(&se_mem->se_list);
> +		se_mem->se_len = (length > dma_size) ? dma_size : length;
>  		memset(buf, 0, se_mem->se_len);
>  		kunmap_atomic(buf, KM_IRQ0);
>  
> @@ -4367,6 +4367,9 @@ transport_generic_get_mem(struct se_cmd *cmd, u32 length, u32 dma_size)
>  
>  	return 0;
>  out:
> +	if (se_mem)
> +		__free_pages(se_mem->se_page, 0);
> +	kmem_cache_free(se_mem_cache, se_mem);
>  	return -1;
>  }
>  
> 

There is actually not a memory leak here.

The T_TASK(cmd)->t_mem_list (and associated struct se_pages) are
released during a transport_generic_get_mem() allocation failure
directly from the 'normal' struct se_cmd descriptor release path called
by all target fabric modules in transport_generic_remove() ->
transport_free_pages().

So I think the allocation failure case in trasnport_generic_new_cmd() ->
transport_allocate_resources() -> transport_generic_get_mem()
is better served by some additional code comments perhaps..?

Thanks!

--nab