linux-kernel - Re: [PATCH] SCSI, target: Avoid mem leak and needless work in transport_generic_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LNX.2.00.1101300025360.19392@swampdragon.chaosbits.net>
Date:	Sun, 30 Jan 2011 00:32:11 +0100 (CET)
From:	Jesper Juhl <jj@...osbits.net>
To:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>
cc:	linux-kernel@...r.kernel.org,
	James Bottomley <James.Bottomley@...e.de>
Subject: Re: [PATCH] SCSI, target: Avoid mem leak and needless work in
 transport_generic_get_mem().

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@...osbits.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?

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.

-- 
Jesper Juhl <jj@...osbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/