Messages in this thread | | | Subject | Re: [PATCH] SCSI, target: Avoid mem leak and needless work in transport_generic_get_mem(). | From | "Nicholas A. Bellinger" <> | Date | Sat, 29 Jan 2011 15:49:32 -0800 |
| |
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
| |