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

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

On Sun, 2011-01-30 at 00:32 +0100, Jesper Juhl wrote:
> On Sat, 29 Jan 2011, Nicholas A. Bellinger wrote:
> 
> > 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..?
> > 
> 
> well,
> 
>   static int
>   transport_generic_get_mem(struct se_cmd *cmd, u32 length, u32 dma_size)
>   {
>   	unsigned char *buf;
>   	struct se_mem *se_mem;
> se_mem is a local variable --^
>   ...
>   	while (length) {
>   		se_mem = kmem_cache_zalloc(se_mem_cache, GFP_KERNEL);
> We allocate mem --^
>   ...
>   		se_mem->se_page = alloc_pages(GFP_KERNEL, 0);
>   		if (!(se_mem->se_page)) {
>   			printk(KERN_ERR "alloc_pages() failed\n");
>   			goto out;
> we've no assigned se_mem anywhere and now jump to 'out' --^
>   ...
>   out:
>   	return -1;
> 'se_mem' goes out of scope --^
> 
> how is that not a leak?
> what am I missing?
> 

Sorry, I did originally mis-read the intention of this patch.  

> I also think the moving of 'INIT_LIST_HEAD()' and assignment to 
> 'se_mem->se_len' to after we know all mem allocations are ok is still 
> worth doing.
> 

Fair enough.   I will commit your original patch as-is into
lio-core-2.6.git, and queue up for the next mainline series.

Thanks!

--nab