lkml.org 
[lkml]   [2011]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] SCSI, target: Avoid mem leak and needless work in transport_generic_get_mem().
    From
    Date
    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



    \
     
     \ /
      Last update: 2011-01-30 00:51    [W:0.057 / U:0.428 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site