[PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Ian Campbell]

The libxc xc_dom_* infrastructure uses a very simple malloc memory pool which
is freed by xc_dom_release. However the various xc_try_*_decode routines (other
than the gzip one) just use plain malloc/realloc and therefore the buffer ends
up leaked.

The memory pool currently supports mmap'd buffers as well as a directly
allocated buffers, however the try decode routines make use of realloc and do
not fit well into this model. Introduce a concept of an external memory block
to the memory pool and provide an interface to register such memory.

The mmap_ptr and mmap_len fields of the memblock tracking struct lose their
mmap_ prefix since they are now also used for external memory blocks.

We are only seeing this now because the gzip decoder doesn't leak and it's only
relatively recently that kernels in the wild have switched to better
compression.

This is https://bugs.debian.org/767295

Reported by: Gedalya <gedalya@gedalya.net>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Correct handling in xc_try_lzo1x_decode.

This is a bug fix and should go into 4.5.

I have a sneaking suspicion this is going to need to backport a very long way...
---
 tools/libxc/include/xc_dom.h        |   10 ++++--
 tools/libxc/xc_dom_bzimageloader.c  |   20 ++++++++++++
 tools/libxc/xc_dom_core.c           |   61 +++++++++++++++++++++++++++--------
 tools/libxc/xc_dom_decompress_lz4.c |    5 +++
 4 files changed, 80 insertions(+), 16 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 6ae6a9f..07d7224 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -33,8 +33,13 @@ struct xc_dom_seg {
 
 struct xc_dom_mem {
     struct xc_dom_mem *next;
-    void *mmap_ptr;
-    size_t mmap_len;
+    void *ptr;
+    enum {
+        XC_DOM_MEM_TYPE_MALLOC_INTERNAL,
+        XC_DOM_MEM_TYPE_MALLOC_EXTERNAL,
+        XC_DOM_MEM_TYPE_MMAP,
+    } type;
+    size_t len;
     unsigned char memory[0];
 };
 
@@ -298,6 +303,7 @@ void xc_dom_log_memory_footprint(struct xc_dom_image *dom);
 /* --- simple memory pool ------------------------------------------ */
 
 void *xc_dom_malloc(struct xc_dom_image *dom, size_t size);
+int xc_dom_register_external(struct xc_dom_image *dom, void *ptr, size_t size);
 void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size);
 void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
                             const char *filename, size_t * size,
diff --git a/tools/libxc/xc_dom_bzimageloader.c b/tools/libxc/xc_dom_bzimageloader.c
index 2225699..964ebdc 100644
--- a/tools/libxc/xc_dom_bzimageloader.c
+++ b/tools/libxc/xc_dom_bzimageloader.c
@@ -161,6 +161,13 @@ static int xc_try_bzip2_decode(
 
     total = (((uint64_t)stream.total_out_hi32) << 32) | stream.total_out_lo32;
 
+    if ( xc_dom_register_external(dom, out_buf, total) )
+    {
+        DOMPRINTF("BZIP2: Error registering stream output");
+        free(out_buf);
+        goto bzip2_cleanup;
+    }
+
     DOMPRINTF("%s: BZIP2 decompress OK, 0x%zx -> 0x%lx",
               __FUNCTION__, *size, (long unsigned int) total);
 
@@ -305,6 +312,13 @@ static int _xc_try_lzma_decode(
         }
     }
 
+    if ( xc_dom_register_external(dom, out_buf, stream->total_out) )
+    {
+        DOMPRINTF("%s: Error registering stream output", what);
+        free(out_buf);
+        goto lzma_cleanup;
+    }
+
     DOMPRINTF("%s: %s decompress OK, 0x%zx -> 0x%zx",
               __FUNCTION__, what, *size, (size_t)stream->total_out);
 
@@ -464,7 +478,13 @@ static int xc_try_lzo1x_decode(
 
         dst_len = lzo_read_32(cur);
         if ( !dst_len )
+        {
+            msg = "Error registering stream output";
+            if ( xc_dom_register_external(dom, out_buf, out_len) )
+                break;
+
             return 0;
+        }
 
         if ( dst_len > LZOP_MAX_BLOCK_SIZE )
         {
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index baa62a1..ecbf981 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -132,6 +132,7 @@ void *xc_dom_malloc(struct xc_dom_image *dom, size_t size)
         return NULL;
     }
     memset(block, 0, sizeof(*block) + size);
+    block->type = XC_DOM_MEM_TYPE_MALLOC_INTERNAL;
     block->next = dom->memblocks;
     dom->memblocks = block;
     dom->alloc_malloc += sizeof(*block) + size;
@@ -151,23 +152,45 @@ void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size)
         return NULL;
     }
     memset(block, 0, sizeof(*block));
-    block->mmap_len = size;
-    block->mmap_ptr = mmap(NULL, block->mmap_len,
-                           PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON,
-                           -1, 0);
-    if ( block->mmap_ptr == MAP_FAILED )
+    block->len = size;
+    block->ptr = mmap(NULL, block->len,
+                      PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON,
+                      -1, 0);
+    if ( block->ptr == MAP_FAILED )
     {
         DOMPRINTF("%s: mmap failed", __FUNCTION__);
         free(block);
         return NULL;
     }
+    block->type = XC_DOM_MEM_TYPE_MMAP;
     block->next = dom->memblocks;
     dom->memblocks = block;
     dom->alloc_malloc += sizeof(*block);
-    dom->alloc_mem_map += block->mmap_len;
+    dom->alloc_mem_map += block->len;
     if ( size > (100 * 1024) )
         print_mem(dom, __FUNCTION__, size);
-    return block->mmap_ptr;
+    return block->ptr;
+}
+
+int xc_dom_register_external(struct xc_dom_image *dom, void *ptr, size_t size)
+{
+    struct xc_dom_mem *block;
+
+    block = malloc(sizeof(*block));
+    if ( block == NULL )
+    {
+        DOMPRINTF("%s: allocation failed", __FUNCTION__);
+        return -1;
+    }
+    memset(block, 0, sizeof(*block));
+    block->ptr = ptr;
+    block->len = size;
+    block->type = XC_DOM_MEM_TYPE_MALLOC_EXTERNAL;
+    block->next = dom->memblocks;
+    dom->memblocks = block;
+    dom->alloc_malloc += sizeof(*block);
+    dom->alloc_mem_map += block->len;
+    return 0;
 }
 
 void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
@@ -212,24 +235,25 @@ void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
     }
 
     memset(block, 0, sizeof(*block));
-    block->mmap_len = *size;
-    block->mmap_ptr = mmap(NULL, block->mmap_len, PROT_READ,
+    block->len = *size;
+    block->ptr = mmap(NULL, block->len, PROT_READ,
                            MAP_SHARED, fd, 0);
-    if ( block->mmap_ptr == MAP_FAILED ) {
+    if ( block->ptr == MAP_FAILED ) {
         xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
                      "failed to mmap file: %s",
                      strerror(errno));
         goto err;
     }
 
+    block->type = XC_DOM_MEM_TYPE_MMAP;
     block->next = dom->memblocks;
     dom->memblocks = block;
     dom->alloc_malloc += sizeof(*block);
-    dom->alloc_file_map += block->mmap_len;
+    dom->alloc_file_map += block->len;
     close(fd);
     if ( *size > (100 * 1024) )
         print_mem(dom, __FUNCTION__, *size);
-    return block->mmap_ptr;
+    return block->ptr;
 
  err:
     if ( fd != -1 )
@@ -246,8 +270,17 @@ static void xc_dom_free_all(struct xc_dom_image *dom)
     while ( (block = dom->memblocks) != NULL )
     {
         dom->memblocks = block->next;
-        if ( block->mmap_ptr )
-            munmap(block->mmap_ptr, block->mmap_len);
+        switch ( block->type )
+        {
+        case XC_DOM_MEM_TYPE_MALLOC_INTERNAL:
+            break;
+        case XC_DOM_MEM_TYPE_MALLOC_EXTERNAL:
+            free(block->ptr);
+            break;
+        case XC_DOM_MEM_TYPE_MMAP:
+            munmap(block->ptr, block->len);
+            break;
+        }
         free(block);
     }
 }
diff --git a/tools/libxc/xc_dom_decompress_lz4.c b/tools/libxc/xc_dom_decompress_lz4.c
index 490ec56..b6a33f2 100644
--- a/tools/libxc/xc_dom_decompress_lz4.c
+++ b/tools/libxc/xc_dom_decompress_lz4.c
@@ -103,6 +103,11 @@ int xc_try_lz4_decode(
 
 		if (size == 0)
 		{
+			if ( xc_dom_register_external(dom, output, out_len) )
+			{
+				msg = "Error registering stream output";
+				goto exit_2;
+			}
 			*blob = output;
 			*psize = out_len;
 			return 0;
-- 
1.7.10.4

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Wei Liu]

On Thu, Nov 20, 2014 at 03:48:47PM +0000, Ian Campbell wrote:
> The libxc xc_dom_* infrastructure uses a very simple malloc memory pool which
> is freed by xc_dom_release. However the various xc_try_*_decode routines (other
> than the gzip one) just use plain malloc/realloc and therefore the buffer ends
> up leaked.
> 
> The memory pool currently supports mmap'd buffers as well as a directly
> allocated buffers, however the try decode routines make use of realloc and do
> not fit well into this model. Introduce a concept of an external memory block
> to the memory pool and provide an interface to register such memory.
> 
> The mmap_ptr and mmap_len fields of the memblock tracking struct lose their
> mmap_ prefix since they are now also used for external memory blocks.
> 
> We are only seeing this now because the gzip decoder doesn't leak and it's only
> relatively recently that kernels in the wild have switched to better
> compression.
> 
> This is https://bugs.debian.org/767295
> 
> Reported by: Gedalya <gedalya@gedalya.net>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Ian Campbell]

On Thu, 2014-11-20 at 16:03 +0000, Wei Liu wrote:
> On Thu, Nov 20, 2014 at 03:48:47PM +0000, Ian Campbell wrote:
> > The libxc xc_dom_* infrastructure uses a very simple malloc memory pool which
> > is freed by xc_dom_release. However the various xc_try_*_decode routines (other
> > than the gzip one) just use plain malloc/realloc and therefore the buffer ends
> > up leaked.
> > 
> > The memory pool currently supports mmap'd buffers as well as a directly
> > allocated buffers, however the try decode routines make use of realloc and do
> > not fit well into this model. Introduce a concept of an external memory block
> > to the memory pool and provide an interface to register such memory.
> > 
> > The mmap_ptr and mmap_len fields of the memblock tracking struct lose their
> > mmap_ prefix since they are now also used for external memory blocks.
> > 
> > We are only seeing this now because the gzip decoder doesn't leak and it's only
> > relatively recently that kernels in the wild have switched to better
> > compression.
> > 
> > This is https://bugs.debian.org/767295
> > 
> > Reported by: Gedalya <gedalya@gedalya.net>
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Thanks. Konrad release-acked on IRC so I've applied.

Ian.

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Ian Campbell]

On Tue, 2014-11-25 at 16:24 +0000, Ian Campbell wrote:
> On Thu, 2014-11-20 at 16:03 +0000, Wei Liu wrote:
> > On Thu, Nov 20, 2014 at 03:48:47PM +0000, Ian Campbell wrote:
> > > The libxc xc_dom_* infrastructure uses a very simple malloc memory pool which
> > > is freed by xc_dom_release. However the various xc_try_*_decode routines (other
> > > than the gzip one) just use plain malloc/realloc and therefore the buffer ends
> > > up leaked.
> > > 
> > > The memory pool currently supports mmap'd buffers as well as a directly
> > > allocated buffers, however the try decode routines make use of realloc and do
> > > not fit well into this model. Introduce a concept of an external memory block
> > > to the memory pool and provide an interface to register such memory.
> > > 
> > > The mmap_ptr and mmap_len fields of the memblock tracking struct lose their
> > > mmap_ prefix since they are now also used for external memory blocks.
> > > 
> > > We are only seeing this now because the gzip decoder doesn't leak and it's only
> > > relatively recently that kernels in the wild have switched to better
> > > compression.
> > > 
> > > This is https://bugs.debian.org/767295
> > > 
> > > Reported by: Gedalya <gedalya@gedalya.net>
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> 
> Thanks. Konrad release-acked on IRC so I've applied.

Ian: THis one should be backported I think.

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Frediano Ziglio]

2014-11-20 15:48 GMT+00:00 Ian Campbell <ian.campbell@citrix.com>:
> The libxc xc_dom_* infrastructure uses a very simple malloc memory pool which
> is freed by xc_dom_release. However the various xc_try_*_decode routines (other
> than the gzip one) just use plain malloc/realloc and therefore the buffer ends
> up leaked.
>
> The memory pool currently supports mmap'd buffers as well as a directly
> allocated buffers, however the try decode routines make use of realloc and do
> not fit well into this model. Introduce a concept of an external memory block
> to the memory pool and provide an interface to register such memory.
>
> The mmap_ptr and mmap_len fields of the memblock tracking struct lose their
> mmap_ prefix since they are now also used for external memory blocks.
>
> We are only seeing this now because the gzip decoder doesn't leak and it's only
> relatively recently that kernels in the wild have switched to better
> compression.
>
> This is https://bugs.debian.org/767295
>
> Reported by: Gedalya <gedalya@gedalya.net>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: Correct handling in xc_try_lzo1x_decode.
>
> This is a bug fix and should go into 4.5.
>
> I have a sneaking suspicion this is going to need to backport a very long way...
> ---
>  tools/libxc/include/xc_dom.h        |   10 ++++--
>  tools/libxc/xc_dom_bzimageloader.c  |   20 ++++++++++++
>  tools/libxc/xc_dom_core.c           |   61 +++++++++++++++++++++++++++--------
>  tools/libxc/xc_dom_decompress_lz4.c |    5 +++
>  4 files changed, 80 insertions(+), 16 deletions(-)
>
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 6ae6a9f..07d7224 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -33,8 +33,13 @@ struct xc_dom_seg {
>
>  struct xc_dom_mem {
>      struct xc_dom_mem *next;
> -    void *mmap_ptr;
> -    size_t mmap_len;
> +    void *ptr;
> +    enum {
> +        XC_DOM_MEM_TYPE_MALLOC_INTERNAL,
> +        XC_DOM_MEM_TYPE_MALLOC_EXTERNAL,
> +        XC_DOM_MEM_TYPE_MMAP,
> +    } type;
> +    size_t len;
>      unsigned char memory[0];
>  };
>

Just a small optimization, if you move type after len you can reduce
structure size.
Unless you want memory field aligned to 8 bytes on 64 bit but in this
case I would force member alignment.

Frediano

> @@ -298,6 +303,7 @@ void xc_dom_log_memory_footprint(struct xc_dom_image *dom);
>  /* --- simple memory pool ------------------------------------------ */
>
>  void *xc_dom_malloc(struct xc_dom_image *dom, size_t size);
> +int xc_dom_register_external(struct xc_dom_image *dom, void *ptr, size_t size);
>  void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size);
>  void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
>                              const char *filename, size_t * size,
> diff --git a/tools/libxc/xc_dom_bzimageloader.c b/tools/libxc/xc_dom_bzimageloader.c
> index 2225699..964ebdc 100644
> --- a/tools/libxc/xc_dom_bzimageloader.c
> +++ b/tools/libxc/xc_dom_bzimageloader.c
> @@ -161,6 +161,13 @@ static int xc_try_bzip2_decode(
>
>      total = (((uint64_t)stream.total_out_hi32) << 32) | stream.total_out_lo32;
>
> +    if ( xc_dom_register_external(dom, out_buf, total) )
> +    {
> +        DOMPRINTF("BZIP2: Error registering stream output");
> +        free(out_buf);
> +        goto bzip2_cleanup;
> +    }
> +
>      DOMPRINTF("%s: BZIP2 decompress OK, 0x%zx -> 0x%lx",
>                __FUNCTION__, *size, (long unsigned int) total);
>
> @@ -305,6 +312,13 @@ static int _xc_try_lzma_decode(
>          }
>      }
>
> +    if ( xc_dom_register_external(dom, out_buf, stream->total_out) )
> +    {
> +        DOMPRINTF("%s: Error registering stream output", what);
> +        free(out_buf);
> +        goto lzma_cleanup;
> +    }
> +
>      DOMPRINTF("%s: %s decompress OK, 0x%zx -> 0x%zx",
>                __FUNCTION__, what, *size, (size_t)stream->total_out);
>
> @@ -464,7 +478,13 @@ static int xc_try_lzo1x_decode(
>
>          dst_len = lzo_read_32(cur);
>          if ( !dst_len )
> +        {
> +            msg = "Error registering stream output";
> +            if ( xc_dom_register_external(dom, out_buf, out_len) )
> +                break;
> +
>              return 0;
> +        }
>
>          if ( dst_len > LZOP_MAX_BLOCK_SIZE )
>          {
> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> index baa62a1..ecbf981 100644
> --- a/tools/libxc/xc_dom_core.c
> +++ b/tools/libxc/xc_dom_core.c
> @@ -132,6 +132,7 @@ void *xc_dom_malloc(struct xc_dom_image *dom, size_t size)
>          return NULL;
>      }
>      memset(block, 0, sizeof(*block) + size);
> +    block->type = XC_DOM_MEM_TYPE_MALLOC_INTERNAL;
>      block->next = dom->memblocks;
>      dom->memblocks = block;
>      dom->alloc_malloc += sizeof(*block) + size;
> @@ -151,23 +152,45 @@ void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size)
>          return NULL;
>      }
>      memset(block, 0, sizeof(*block));
> -    block->mmap_len = size;
> -    block->mmap_ptr = mmap(NULL, block->mmap_len,
> -                           PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON,
> -                           -1, 0);
> -    if ( block->mmap_ptr == MAP_FAILED )
> +    block->len = size;
> +    block->ptr = mmap(NULL, block->len,
> +                      PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON,
> +                      -1, 0);
> +    if ( block->ptr == MAP_FAILED )
>      {
>          DOMPRINTF("%s: mmap failed", __FUNCTION__);
>          free(block);
>          return NULL;
>      }
> +    block->type = XC_DOM_MEM_TYPE_MMAP;
>      block->next = dom->memblocks;
>      dom->memblocks = block;
>      dom->alloc_malloc += sizeof(*block);
> -    dom->alloc_mem_map += block->mmap_len;
> +    dom->alloc_mem_map += block->len;
>      if ( size > (100 * 1024) )
>          print_mem(dom, __FUNCTION__, size);
> -    return block->mmap_ptr;
> +    return block->ptr;
> +}
> +
> +int xc_dom_register_external(struct xc_dom_image *dom, void *ptr, size_t size)
> +{
> +    struct xc_dom_mem *block;
> +
> +    block = malloc(sizeof(*block));
> +    if ( block == NULL )
> +    {
> +        DOMPRINTF("%s: allocation failed", __FUNCTION__);
> +        return -1;
> +    }
> +    memset(block, 0, sizeof(*block));
> +    block->ptr = ptr;
> +    block->len = size;
> +    block->type = XC_DOM_MEM_TYPE_MALLOC_EXTERNAL;
> +    block->next = dom->memblocks;
> +    dom->memblocks = block;
> +    dom->alloc_malloc += sizeof(*block);
> +    dom->alloc_mem_map += block->len;
> +    return 0;
>  }
>
>  void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
> @@ -212,24 +235,25 @@ void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
>      }
>
>      memset(block, 0, sizeof(*block));
> -    block->mmap_len = *size;
> -    block->mmap_ptr = mmap(NULL, block->mmap_len, PROT_READ,
> +    block->len = *size;
> +    block->ptr = mmap(NULL, block->len, PROT_READ,
>                             MAP_SHARED, fd, 0);
> -    if ( block->mmap_ptr == MAP_FAILED ) {
> +    if ( block->ptr == MAP_FAILED ) {
>          xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
>                       "failed to mmap file: %s",
>                       strerror(errno));
>          goto err;
>      }
>
> +    block->type = XC_DOM_MEM_TYPE_MMAP;
>      block->next = dom->memblocks;
>      dom->memblocks = block;
>      dom->alloc_malloc += sizeof(*block);
> -    dom->alloc_file_map += block->mmap_len;
> +    dom->alloc_file_map += block->len;
>      close(fd);
>      if ( *size > (100 * 1024) )
>          print_mem(dom, __FUNCTION__, *size);
> -    return block->mmap_ptr;
> +    return block->ptr;
>
>   err:
>      if ( fd != -1 )
> @@ -246,8 +270,17 @@ static void xc_dom_free_all(struct xc_dom_image *dom)
>      while ( (block = dom->memblocks) != NULL )
>      {
>          dom->memblocks = block->next;
> -        if ( block->mmap_ptr )
> -            munmap(block->mmap_ptr, block->mmap_len);
> +        switch ( block->type )
> +        {
> +        case XC_DOM_MEM_TYPE_MALLOC_INTERNAL:
> +            break;
> +        case XC_DOM_MEM_TYPE_MALLOC_EXTERNAL:
> +            free(block->ptr);
> +            break;
> +        case XC_DOM_MEM_TYPE_MMAP:
> +            munmap(block->ptr, block->len);
> +            break;
> +        }
>          free(block);
>      }
>  }
> diff --git a/tools/libxc/xc_dom_decompress_lz4.c b/tools/libxc/xc_dom_decompress_lz4.c
> index 490ec56..b6a33f2 100644
> --- a/tools/libxc/xc_dom_decompress_lz4.c
> +++ b/tools/libxc/xc_dom_decompress_lz4.c
> @@ -103,6 +103,11 @@ int xc_try_lz4_decode(
>
>                 if (size == 0)
>                 {
> +                       if ( xc_dom_register_external(dom, output, out_len) )
> +                       {
> +                               msg = "Error registering stream output";
> +                               goto exit_2;
> +                       }
>                         *blob = output;
>                         *psize = out_len;
>                         return 0;
> --
> 1.7.10.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Konrad Rzeszutek Wilk]

On Thu, Nov 20, 2014 at 03:48:47PM +0000, Ian Campbell wrote:
> The libxc xc_dom_* infrastructure uses a very simple malloc memory pool which
> is freed by xc_dom_release. However the various xc_try_*_decode routines (other
> than the gzip one) just use plain malloc/realloc and therefore the buffer ends
> up leaked.
> 
> The memory pool currently supports mmap'd buffers as well as a directly
> allocated buffers, however the try decode routines make use of realloc and do
> not fit well into this model. Introduce a concept of an external memory block
> to the memory pool and provide an interface to register such memory.
> 
> The mmap_ptr and mmap_len fields of the memblock tracking struct lose their
> mmap_ prefix since they are now also used for external memory blocks.
> 
> We are only seeing this now because the gzip decoder doesn't leak and it's only
> relatively recently that kernels in the wild have switched to better
> compression.
> 
> This is https://bugs.debian.org/767295
> 
> Reported by: Gedalya <gedalya@gedalya.net>

Gedelya,

Could you also test this patch to make sure it does fix the
reported issue please?

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: Correct handling in xc_try_lzo1x_decode.
> 
> This is a bug fix and should go into 4.5.
> 
> I have a sneaking suspicion this is going to need to backport a very long way...
> ---
>  tools/libxc/include/xc_dom.h        |   10 ++++--
>  tools/libxc/xc_dom_bzimageloader.c  |   20 ++++++++++++
>  tools/libxc/xc_dom_core.c           |   61 +++++++++++++++++++++++++++--------
>  tools/libxc/xc_dom_decompress_lz4.c |    5 +++
>  4 files changed, 80 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 6ae6a9f..07d7224 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -33,8 +33,13 @@ struct xc_dom_seg {
>  
>  struct xc_dom_mem {
>      struct xc_dom_mem *next;
> -    void *mmap_ptr;
> -    size_t mmap_len;
> +    void *ptr;
> +    enum {
> +        XC_DOM_MEM_TYPE_MALLOC_INTERNAL,
> +        XC_DOM_MEM_TYPE_MALLOC_EXTERNAL,
> +        XC_DOM_MEM_TYPE_MMAP,
> +    } type;
> +    size_t len;
>      unsigned char memory[0];
>  };
>  
> @@ -298,6 +303,7 @@ void xc_dom_log_memory_footprint(struct xc_dom_image *dom);
>  /* --- simple memory pool ------------------------------------------ */
>  
>  void *xc_dom_malloc(struct xc_dom_image *dom, size_t size);
> +int xc_dom_register_external(struct xc_dom_image *dom, void *ptr, size_t size);
>  void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size);
>  void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
>                              const char *filename, size_t * size,
> diff --git a/tools/libxc/xc_dom_bzimageloader.c b/tools/libxc/xc_dom_bzimageloader.c
> index 2225699..964ebdc 100644
> --- a/tools/libxc/xc_dom_bzimageloader.c
> +++ b/tools/libxc/xc_dom_bzimageloader.c
> @@ -161,6 +161,13 @@ static int xc_try_bzip2_decode(
>  
>      total = (((uint64_t)stream.total_out_hi32) << 32) | stream.total_out_lo32;
>  
> +    if ( xc_dom_register_external(dom, out_buf, total) )
> +    {
> +        DOMPRINTF("BZIP2: Error registering stream output");
> +        free(out_buf);
> +        goto bzip2_cleanup;
> +    }
> +
>      DOMPRINTF("%s: BZIP2 decompress OK, 0x%zx -> 0x%lx",
>                __FUNCTION__, *size, (long unsigned int) total);
>  
> @@ -305,6 +312,13 @@ static int _xc_try_lzma_decode(
>          }
>      }
>  
> +    if ( xc_dom_register_external(dom, out_buf, stream->total_out) )
> +    {
> +        DOMPRINTF("%s: Error registering stream output", what);
> +        free(out_buf);
> +        goto lzma_cleanup;
> +    }
> +
>      DOMPRINTF("%s: %s decompress OK, 0x%zx -> 0x%zx",
>                __FUNCTION__, what, *size, (size_t)stream->total_out);
>  
> @@ -464,7 +478,13 @@ static int xc_try_lzo1x_decode(
>  
>          dst_len = lzo_read_32(cur);
>          if ( !dst_len )
> +        {
> +            msg = "Error registering stream output";
> +            if ( xc_dom_register_external(dom, out_buf, out_len) )
> +                break;
> +
>              return 0;
> +        }
>  
>          if ( dst_len > LZOP_MAX_BLOCK_SIZE )
>          {
> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> index baa62a1..ecbf981 100644
> --- a/tools/libxc/xc_dom_core.c
> +++ b/tools/libxc/xc_dom_core.c
> @@ -132,6 +132,7 @@ void *xc_dom_malloc(struct xc_dom_image *dom, size_t size)
>          return NULL;
>      }
>      memset(block, 0, sizeof(*block) + size);
> +    block->type = XC_DOM_MEM_TYPE_MALLOC_INTERNAL;
>      block->next = dom->memblocks;
>      dom->memblocks = block;
>      dom->alloc_malloc += sizeof(*block) + size;
> @@ -151,23 +152,45 @@ void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size)
>          return NULL;
>      }
>      memset(block, 0, sizeof(*block));
> -    block->mmap_len = size;
> -    block->mmap_ptr = mmap(NULL, block->mmap_len,
> -                           PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON,
> -                           -1, 0);
> -    if ( block->mmap_ptr == MAP_FAILED )
> +    block->len = size;
> +    block->ptr = mmap(NULL, block->len,
> +                      PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON,
> +                      -1, 0);
> +    if ( block->ptr == MAP_FAILED )
>      {
>          DOMPRINTF("%s: mmap failed", __FUNCTION__);
>          free(block);
>          return NULL;
>      }
> +    block->type = XC_DOM_MEM_TYPE_MMAP;
>      block->next = dom->memblocks;
>      dom->memblocks = block;
>      dom->alloc_malloc += sizeof(*block);
> -    dom->alloc_mem_map += block->mmap_len;
> +    dom->alloc_mem_map += block->len;
>      if ( size > (100 * 1024) )
>          print_mem(dom, __FUNCTION__, size);
> -    return block->mmap_ptr;
> +    return block->ptr;
> +}
> +
> +int xc_dom_register_external(struct xc_dom_image *dom, void *ptr, size_t size)
> +{
> +    struct xc_dom_mem *block;
> +
> +    block = malloc(sizeof(*block));
> +    if ( block == NULL )
> +    {
> +        DOMPRINTF("%s: allocation failed", __FUNCTION__);
> +        return -1;
> +    }
> +    memset(block, 0, sizeof(*block));
> +    block->ptr = ptr;
> +    block->len = size;
> +    block->type = XC_DOM_MEM_TYPE_MALLOC_EXTERNAL;
> +    block->next = dom->memblocks;
> +    dom->memblocks = block;
> +    dom->alloc_malloc += sizeof(*block);
> +    dom->alloc_mem_map += block->len;
> +    return 0;
>  }
>  
>  void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
> @@ -212,24 +235,25 @@ void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
>      }
>  
>      memset(block, 0, sizeof(*block));
> -    block->mmap_len = *size;
> -    block->mmap_ptr = mmap(NULL, block->mmap_len, PROT_READ,
> +    block->len = *size;
> +    block->ptr = mmap(NULL, block->len, PROT_READ,
>                             MAP_SHARED, fd, 0);
> -    if ( block->mmap_ptr == MAP_FAILED ) {
> +    if ( block->ptr == MAP_FAILED ) {
>          xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
>                       "failed to mmap file: %s",
>                       strerror(errno));
>          goto err;
>      }
>  
> +    block->type = XC_DOM_MEM_TYPE_MMAP;
>      block->next = dom->memblocks;
>      dom->memblocks = block;
>      dom->alloc_malloc += sizeof(*block);
> -    dom->alloc_file_map += block->mmap_len;
> +    dom->alloc_file_map += block->len;
>      close(fd);
>      if ( *size > (100 * 1024) )
>          print_mem(dom, __FUNCTION__, *size);
> -    return block->mmap_ptr;
> +    return block->ptr;
>  
>   err:
>      if ( fd != -1 )
> @@ -246,8 +270,17 @@ static void xc_dom_free_all(struct xc_dom_image *dom)
>      while ( (block = dom->memblocks) != NULL )
>      {
>          dom->memblocks = block->next;
> -        if ( block->mmap_ptr )
> -            munmap(block->mmap_ptr, block->mmap_len);
> +        switch ( block->type )
> +        {
> +        case XC_DOM_MEM_TYPE_MALLOC_INTERNAL:
> +            break;
> +        case XC_DOM_MEM_TYPE_MALLOC_EXTERNAL:
> +            free(block->ptr);
> +            break;
> +        case XC_DOM_MEM_TYPE_MMAP:
> +            munmap(block->ptr, block->len);
> +            break;
> +        }
>          free(block);
>      }
>  }
> diff --git a/tools/libxc/xc_dom_decompress_lz4.c b/tools/libxc/xc_dom_decompress_lz4.c
> index 490ec56..b6a33f2 100644
> --- a/tools/libxc/xc_dom_decompress_lz4.c
> +++ b/tools/libxc/xc_dom_decompress_lz4.c
> @@ -103,6 +103,11 @@ int xc_try_lz4_decode(
>  
>  		if (size == 0)
>  		{
> +			if ( xc_dom_register_external(dom, output, out_len) )
> +			{
> +				msg = "Error registering stream output";
> +				goto exit_2;
> +			}
>  			*blob = output;
>  			*psize = out_len;
>  			return 0;
> -- 
> 1.7.10.4
> 

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Gedalya]

On 11/20/2014 03:21 PM, Konrad Rzeszutek Wilk wrote:
> On Thu, Nov 20, 2014 at 03:48:47PM +0000, Ian Campbell wrote:
>> The libxc xc_dom_* infrastructure uses a very simple malloc memory pool which
>> is freed by xc_dom_release. However the various xc_try_*_decode routines (other
>> than the gzip one) just use plain malloc/realloc and therefore the buffer ends
>> up leaked.
>>
>> The memory pool currently supports mmap'd buffers as well as a directly
>> allocated buffers, however the try decode routines make use of realloc and do
>> not fit well into this model. Introduce a concept of an external memory block
>> to the memory pool and provide an interface to register such memory.
>>
>> The mmap_ptr and mmap_len fields of the memblock tracking struct lose their
>> mmap_ prefix since they are now also used for external memory blocks.
>>
>> We are only seeing this now because the gzip decoder doesn't leak and it's only
>> relatively recently that kernels in the wild have switched to better
>> compression.
>>
>> This is https://bugs.debian.org/767295
>>
>> Reported by: Gedalya <gedalya@gedalya.net>
> Gedelya,
>
> Could you also test this patch to make sure it does fix the
> reported issue please?

So here's what happens now.
1. Starts up tiny
2. reboot: leak
3. reboot: freed (process larger, but the delta is all/mostly shared pages)
4. reboot: leak
5. reboot: freed
etc..

root@xen:~/xen-pkgs# xl cr /etc/xen/auto/asterisk_deb80.cfg
Parsing config from /etc/xen/auto/asterisk_deb80.cfg
root@xen:~/xen-pkgs# ps aux | grep asterisk_deb80
root     22981  0.0  0.0  95968   588 ?        SLsl 21:55   0:00 
/usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
root@xen:~/xen-pkgs# pmap -x 22981
22981:   /usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
Address           Kbytes     RSS   Dirty Mode  Mapping
0000000000400000     144     128       0 r-x-- xl
0000000000623000       4       4       4 r---- xl
0000000000624000       8       8       8 rw--- xl
0000000000626000       4       4       4 rw---   [ anon ]
00000000009a6000     288     240     240 rw---   [ anon ]
00007f14d4000000     132       8       8 rw---   [ anon ]
00007f14d4021000   65404       0       0 -----   [ anon ]
<< snip >>
---------------- ------- ------- -------
total kB           95968    2728     596

--- reboot domu ---

root@xen:~/xen-pkgs# ps aux | grep asterisk_deb80
root     22981  0.6  3.3 131652 20008 ?        SLsl 21:55   0:00 
/usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
root@xen:~/xen-pkgs# pmap -x 22981
22981:   /usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
Address           Kbytes     RSS   Dirty Mode  Mapping
0000000000400000     144     144       0 r-x-- xl
0000000000623000       4       4       4 r---- xl
0000000000624000       8       8       8 rw--- xl
0000000000626000       4       4       4 rw---   [ anon ]
00000000009a6000     288     288     288 rw---   [ anon ]
00000000009ee000   35676   16772   16772 rw---   [ anon ]
00007f14d4000000     132       8       8 rw---   [ anon ]
00007f14d4021000   65404       0       0 -----   [ anon ]
00007f14d9ae0000     104     100       0 r-x-- libz.so.1.2.8
<< snip >>
---------------- ------- ------- -------
total kB          131652   20072   17424

--- reboot domu ---

root@xen:~/xen-pkgs# ps aux | grep asterisk_deb80
root     22981  0.5  0.5  95876  3136 ?        SLsl 21:55   0:01 
/usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
root@xen:~/xen-pkgs# pmap -x 22981
22981:   /usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
Address           Kbytes     RSS   Dirty Mode  Mapping
0000000000400000     144     144       0 r-x-- xl
0000000000623000       4       4       4 r---- xl
0000000000624000       8       8       8 rw--- xl
0000000000626000       4       4       4 rw---   [ anon ]
00000000009a6000     188     188     188 rw---   [ anon ]
00007f14d4000000     132       8       8 rw---   [ anon ]
00007f14d4021000   65404       0       0 -----   [ anon ]
00007f14d9ae0000     104     100       0 r-x-- libz.so.1.2.8
<< snip >>
---------------- ------- ------- -------
total kB           95876    3200     552

--- reboot domu ---

root@xen:~/xen-pkgs# ps aux | grep asterisk_deb80
root     22981  0.6  3.4 134660 20548 ?        SLsl 21:55   0:02 
/usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
root@xen:~/xen-pkgs# pmap -x 22981
22981:   /usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
Address           Kbytes     RSS   Dirty Mode  Mapping
0000000000400000     144     144       0 r-x-- xl
0000000000623000       4       4       4 r---- xl
0000000000624000       8       8       8 rw--- xl
0000000000626000       4       4       4 rw---   [ anon ]
00000000009a6000     188     188     188 rw---   [ anon ]
00000000009d5000   38784   17348   17348 rw---   [ anon ]
00007f14d4000000     132       8       8 rw---   [ anon ]
00007f14d4021000   65404       0       0 -----   [ anon ]
00007f14d9ae0000     104     100       0 r-x-- libz.so.1.2.8
<< snip >>
---------------- ------- ------- -------
total kB          134660   20548   17900

--- reboot domu ---

root@xen:~/xen-pkgs# ps aux | grep asterisk_deb80
root     22981  0.6  0.5  95876  3200 ?        SLsl 21:55   0:03 
/usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
root@xen:~/xen-pkgs# pmap -x 22981
22981:   /usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
Address           Kbytes     RSS   Dirty Mode  Mapping
0000000000400000     144     144       0 r-x-- xl
0000000000623000       4       4       4 r---- xl
0000000000624000       8       8       8 rw--- xl
0000000000626000       4       4       4 rw---   [ anon ]
00000000009a6000     188     188     188 rw---   [ anon ]
00007f14d4000000     132       8       8 rw---   [ anon ]
00007f14d4021000   65404       0       0 -----   [ anon ]
00007f14d9ae0000     104     100       0 r-x-- libz.so.1.2.8
<< snip >>
---------------- ------- ------- -------
total kB           95876    3200     552




Tried valgrind, it doesn't look like it was able to see what was going on

root@xen:~# valgrind --leak-check=full xl cr -F 
/etc/xen/auto/asterisk_deb80.cfg
==24967== Memcheck, a memory error detector
==24967== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==24967== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==24967== Command: /usr/sbin/xl cr -F /etc/xen/auto/asterisk_deb80.cfg
==24967==
==24971==
==24971== HEAP SUMMARY:
==24971==     in use at exit: 11,695 bytes in 41 blocks
==24971==   total heap usage: 75 allocs, 34 frees, 44,602 bytes allocated
==24971==
==24971== LEAK SUMMARY:
==24971==    definitely lost: 0 bytes in 0 blocks
==24971==    indirectly lost: 0 bytes in 0 blocks
==24971==      possibly lost: 0 bytes in 0 blocks
==24971==    still reachable: 11,695 bytes in 41 blocks
==24971==         suppressed: 0 bytes in 0 blocks
==24971== Reachable blocks (those to which a pointer was found) are not 
shown.
==24971== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==24971==
==24971== For counts of detected and suppressed errors, rerun with: -v
==24971== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
==24970==
==24970== HEAP SUMMARY:
==24970==     in use at exit: 10,743 bytes in 36 blocks
==24970==   total heap usage: 64 allocs, 28 frees, 35,289 bytes allocated
==24970==
==24970== LEAK SUMMARY:
==24970==    definitely lost: 0 bytes in 0 blocks
==24970==    indirectly lost: 0 bytes in 0 blocks
==24970==      possibly lost: 0 bytes in 0 blocks
==24970==    still reachable: 10,743 bytes in 36 blocks
==24970==         suppressed: 0 bytes in 0 blocks
==24970== Reachable blocks (those to which a pointer was found) are not 
shown.
==24970== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==24970==
==24970== For counts of detected and suppressed errors, rerun with: -v
==24970== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
==24975==
==24975== HEAP SUMMARY:
==24975==     in use at exit: 4,859 bytes in 43 blocks
==24975==   total heap usage: 92 allocs, 49 frees, 38,375 bytes allocated
==24975==
==24975== LEAK SUMMARY:
==24975==    definitely lost: 0 bytes in 0 blocks
==24975==    indirectly lost: 0 bytes in 0 blocks
==24975==      possibly lost: 0 bytes in 0 blocks
==24975==    still reachable: 4,859 bytes in 43 blocks
==24975==         suppressed: 0 bytes in 0 blocks
==24975== Reachable blocks (those to which a pointer was found) are not 
shown.
==24975== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==24975==
==24975== For counts of detected and suppressed errors, rerun with: -v
==24975== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
==24976==
==24976== HEAP SUMMARY:
==24976==     in use at exit: 13,066 bytes in 45 blocks
==24976==   total heap usage: 98 allocs, 53 frees, 48,704 bytes allocated
==24976==
==24976== LEAK SUMMARY:
==24976==    definitely lost: 0 bytes in 0 blocks
==24976==    indirectly lost: 0 bytes in 0 blocks
==24976==      possibly lost: 0 bytes in 0 blocks
==24976==    still reachable: 13,066 bytes in 45 blocks
==24976==         suppressed: 0 bytes in 0 blocks
==24976== Reachable blocks (those to which a pointer was found) are not 
shown.
==24976== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==24976==
==24976== For counts of detected and suppressed errors, rerun with: -v
==24976== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
==24969==
==24969== HEAP SUMMARY:
==24969==     in use at exit: 11,049 bytes in 42 blocks
==24969==   total heap usage: 92 allocs, 50 frees, 48,587 bytes allocated
==24969==
==24969== LEAK SUMMARY:
==24969==    definitely lost: 0 bytes in 0 blocks
==24969==    indirectly lost: 0 bytes in 0 blocks
==24969==      possibly lost: 0 bytes in 0 blocks
==24969==    still reachable: 11,049 bytes in 42 blocks
==24969==         suppressed: 0 bytes in 0 blocks
==24969== Reachable blocks (those to which a pointer was found) are not 
shown.
==24969== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==24969==
==24969== For counts of detected and suppressed errors, rerun with: -v
==24969== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Parsing config from /etc/xen/auto/asterisk_deb80.cfg
Waiting for domain asterisk_deb80 (domid 31) to die [pid 24967]
Domain 31 has shut down, reason code 1 0x1
Action for shutdown reason code 1 is restart
Domain 31 needs to be cleaned up: destroying the domain
Done. Rebooting now
Waiting for domain asterisk_deb80 (domid 32) to die [pid 24967]
Domain 32 has shut down, reason code 1 0x1
Action for shutdown reason code 1 is restart
Domain 32 needs to be cleaned up: destroying the domain
Done. Rebooting now
Waiting for domain asterisk_deb80 (domid 33) to die [pid 24967]
Domain 33 has shut down, reason code 1 0x1
Action for shutdown reason code 1 is restart
Domain 33 needs to be cleaned up: destroying the domain
Done. Rebooting now
Waiting for domain asterisk_deb80 (domid 34) to die [pid 24967]
Domain 34 has shut down, reason code 0 0x0
Action for shutdown reason code 0 is destroy
Domain 34 needs to be cleaned up: destroying the domain
Done. Exiting now


>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> ---
>> v2: Correct handling in xc_try_lzo1x_decode.
>>
>> This is a bug fix and should go into 4.5.
>>
>> I have a sneaking suspicion this is going to need to backport a very long way...
>> ---
>>   tools/libxc/include/xc_dom.h        |   10 ++++--
>>   tools/libxc/xc_dom_bzimageloader.c  |   20 ++++++++++++
>>   tools/libxc/xc_dom_core.c           |   61 +++++++++++++++++++++++++++--------
>>   tools/libxc/xc_dom_decompress_lz4.c |    5 +++
>>   4 files changed, 80 insertions(+), 16 deletions(-)
>>
>> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
>> index 6ae6a9f..07d7224 100644
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -33,8 +33,13 @@ struct xc_dom_seg {
>>   
>>   struct xc_dom_mem {
>>       struct xc_dom_mem *next;
>> -    void *mmap_ptr;
>> -    size_t mmap_len;
>> +    void *ptr;
>> +    enum {
>> +        XC_DOM_MEM_TYPE_MALLOC_INTERNAL,
>> +        XC_DOM_MEM_TYPE_MALLOC_EXTERNAL,
>> +        XC_DOM_MEM_TYPE_MMAP,
>> +    } type;
>> +    size_t len;
>>       unsigned char memory[0];
>>   };
>>   
>> @@ -298,6 +303,7 @@ void xc_dom_log_memory_footprint(struct xc_dom_image *dom);
>>   /* --- simple memory pool ------------------------------------------ */
>>   
>>   void *xc_dom_malloc(struct xc_dom_image *dom, size_t size);
>> +int xc_dom_register_external(struct xc_dom_image *dom, void *ptr, size_t size);
>>   void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size);
>>   void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
>>                               const char *filename, size_t * size,
>> diff --git a/tools/libxc/xc_dom_bzimageloader.c b/tools/libxc/xc_dom_bzimageloader.c
>> index 2225699..964ebdc 100644
>> --- a/tools/libxc/xc_dom_bzimageloader.c
>> +++ b/tools/libxc/xc_dom_bzimageloader.c
>> @@ -161,6 +161,13 @@ static int xc_try_bzip2_decode(
>>   
>>       total = (((uint64_t)stream.total_out_hi32) << 32) | stream.total_out_lo32;
>>   
>> +    if ( xc_dom_register_external(dom, out_buf, total) )
>> +    {
>> +        DOMPRINTF("BZIP2: Error registering stream output");
>> +        free(out_buf);
>> +        goto bzip2_cleanup;
>> +    }
>> +
>>       DOMPRINTF("%s: BZIP2 decompress OK, 0x%zx -> 0x%lx",
>>                 __FUNCTION__, *size, (long unsigned int) total);
>>   
>> @@ -305,6 +312,13 @@ static int _xc_try_lzma_decode(
>>           }
>>       }
>>   
>> +    if ( xc_dom_register_external(dom, out_buf, stream->total_out) )
>> +    {
>> +        DOMPRINTF("%s: Error registering stream output", what);
>> +        free(out_buf);
>> +        goto lzma_cleanup;
>> +    }
>> +
>>       DOMPRINTF("%s: %s decompress OK, 0x%zx -> 0x%zx",
>>                 __FUNCTION__, what, *size, (size_t)stream->total_out);
>>   
>> @@ -464,7 +478,13 @@ static int xc_try_lzo1x_decode(
>>   
>>           dst_len = lzo_read_32(cur);
>>           if ( !dst_len )
>> +        {
>> +            msg = "Error registering stream output";
>> +            if ( xc_dom_register_external(dom, out_buf, out_len) )
>> +                break;
>> +
>>               return 0;
>> +        }
>>   
>>           if ( dst_len > LZOP_MAX_BLOCK_SIZE )
>>           {
>> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
>> index baa62a1..ecbf981 100644
>> --- a/tools/libxc/xc_dom_core.c
>> +++ b/tools/libxc/xc_dom_core.c
>> @@ -132,6 +132,7 @@ void *xc_dom_malloc(struct xc_dom_image *dom, size_t size)
>>           return NULL;
>>       }
>>       memset(block, 0, sizeof(*block) + size);
>> +    block->type = XC_DOM_MEM_TYPE_MALLOC_INTERNAL;
>>       block->next = dom->memblocks;
>>       dom->memblocks = block;
>>       dom->alloc_malloc += sizeof(*block) + size;
>> @@ -151,23 +152,45 @@ void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size)
>>           return NULL;
>>       }
>>       memset(block, 0, sizeof(*block));
>> -    block->mmap_len = size;
>> -    block->mmap_ptr = mmap(NULL, block->mmap_len,
>> -                           PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON,
>> -                           -1, 0);
>> -    if ( block->mmap_ptr == MAP_FAILED )
>> +    block->len = size;
>> +    block->ptr = mmap(NULL, block->len,
>> +                      PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON,
>> +                      -1, 0);
>> +    if ( block->ptr == MAP_FAILED )
>>       {
>>           DOMPRINTF("%s: mmap failed", __FUNCTION__);
>>           free(block);
>>           return NULL;
>>       }
>> +    block->type = XC_DOM_MEM_TYPE_MMAP;
>>       block->next = dom->memblocks;
>>       dom->memblocks = block;
>>       dom->alloc_malloc += sizeof(*block);
>> -    dom->alloc_mem_map += block->mmap_len;
>> +    dom->alloc_mem_map += block->len;
>>       if ( size > (100 * 1024) )
>>           print_mem(dom, __FUNCTION__, size);
>> -    return block->mmap_ptr;
>> +    return block->ptr;
>> +}
>> +
>> +int xc_dom_register_external(struct xc_dom_image *dom, void *ptr, size_t size)
>> +{
>> +    struct xc_dom_mem *block;
>> +
>> +    block = malloc(sizeof(*block));
>> +    if ( block == NULL )
>> +    {
>> +        DOMPRINTF("%s: allocation failed", __FUNCTION__);
>> +        return -1;
>> +    }
>> +    memset(block, 0, sizeof(*block));
>> +    block->ptr = ptr;
>> +    block->len = size;
>> +    block->type = XC_DOM_MEM_TYPE_MALLOC_EXTERNAL;
>> +    block->next = dom->memblocks;
>> +    dom->memblocks = block;
>> +    dom->alloc_malloc += sizeof(*block);
>> +    dom->alloc_mem_map += block->len;
>> +    return 0;
>>   }
>>   
>>   void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
>> @@ -212,24 +235,25 @@ void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
>>       }
>>   
>>       memset(block, 0, sizeof(*block));
>> -    block->mmap_len = *size;
>> -    block->mmap_ptr = mmap(NULL, block->mmap_len, PROT_READ,
>> +    block->len = *size;
>> +    block->ptr = mmap(NULL, block->len, PROT_READ,
>>                              MAP_SHARED, fd, 0);
>> -    if ( block->mmap_ptr == MAP_FAILED ) {
>> +    if ( block->ptr == MAP_FAILED ) {
>>           xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
>>                        "failed to mmap file: %s",
>>                        strerror(errno));
>>           goto err;
>>       }
>>   
>> +    block->type = XC_DOM_MEM_TYPE_MMAP;
>>       block->next = dom->memblocks;
>>       dom->memblocks = block;
>>       dom->alloc_malloc += sizeof(*block);
>> -    dom->alloc_file_map += block->mmap_len;
>> +    dom->alloc_file_map += block->len;
>>       close(fd);
>>       if ( *size > (100 * 1024) )
>>           print_mem(dom, __FUNCTION__, *size);
>> -    return block->mmap_ptr;
>> +    return block->ptr;
>>   
>>    err:
>>       if ( fd != -1 )
>> @@ -246,8 +270,17 @@ static void xc_dom_free_all(struct xc_dom_image *dom)
>>       while ( (block = dom->memblocks) != NULL )
>>       {
>>           dom->memblocks = block->next;
>> -        if ( block->mmap_ptr )
>> -            munmap(block->mmap_ptr, block->mmap_len);
>> +        switch ( block->type )
>> +        {
>> +        case XC_DOM_MEM_TYPE_MALLOC_INTERNAL:
>> +            break;
>> +        case XC_DOM_MEM_TYPE_MALLOC_EXTERNAL:
>> +            free(block->ptr);
>> +            break;
>> +        case XC_DOM_MEM_TYPE_MMAP:
>> +            munmap(block->ptr, block->len);
>> +            break;
>> +        }
>>           free(block);
>>       }
>>   }
>> diff --git a/tools/libxc/xc_dom_decompress_lz4.c b/tools/libxc/xc_dom_decompress_lz4.c
>> index 490ec56..b6a33f2 100644
>> --- a/tools/libxc/xc_dom_decompress_lz4.c
>> +++ b/tools/libxc/xc_dom_decompress_lz4.c
>> @@ -103,6 +103,11 @@ int xc_try_lz4_decode(
>>   
>>   		if (size == 0)
>>   		{
>> +			if ( xc_dom_register_external(dom, output, out_len) )
>> +			{
>> +				msg = "Error registering stream output";
>> +				goto exit_2;
>> +			}
>>   			*blob = output;
>>   			*psize = out_len;
>>   			return 0;
>> -- 
>> 1.7.10.4
>>

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Gedalya]

[-- Attachment #1: Type: text/plain, Size: 1725 bytes --]

On 11/20/2014 10:13 PM, Gedalya wrote:
> On 11/20/2014 03:21 PM, Konrad Rzeszutek Wilk wrote:
>> On Thu, Nov 20, 2014 at 03:48:47PM +0000, Ian Campbell wrote:
>>> The libxc xc_dom_* infrastructure uses a very simple malloc memory 
>>> pool which
>>> is freed by xc_dom_release. However the various xc_try_*_decode 
>>> routines (other
>>> than the gzip one) just use plain malloc/realloc and therefore the 
>>> buffer ends
>>> up leaked.
>>>
>>> The memory pool currently supports mmap'd buffers as well as a directly
>>> allocated buffers, however the try decode routines make use of 
>>> realloc and do
>>> not fit well into this model. Introduce a concept of an external 
>>> memory block
>>> to the memory pool and provide an interface to register such memory.
>>>
>>> The mmap_ptr and mmap_len fields of the memblock tracking struct 
>>> lose their
>>> mmap_ prefix since they are now also used for external memory blocks.
>>>
>>> We are only seeing this now because the gzip decoder doesn't leak 
>>> and it's only
>>> relatively recently that kernels in the wild have switched to better
>>> compression.
>>>
>>> This is https://bugs.debian.org/767295
>>>
>>> Reported by: Gedalya <gedalya@gedalya.net>
>> Gedelya,
>>
>> Could you also test this patch to make sure it does fix the
>> reported issue please?
>
> So here's what happens now.
> 1. Starts up tiny
> 2. reboot: leak
> 3. reboot: freed (process larger, but the delta is all/mostly shared 
> pages)
> 4. reboot: leak
> 5. reboot: freed
> etc..
>
For the record, I applied it again the xen package currently in debian, 
4.4.1-3, see attached patch.
The only problem was that tools/libxc/include/xc_dom.h is in 
tools/libxc/xc_dom.h, otherwise it applied fine.


[-- Attachment #2: 0039-libxc-dont-leak-buffer-containing-the-uncompressed-pv-kernel --]
[-- Type: text/plain, Size: 6681 bytes --]

Index: xen-4.4.1/tools/libxc/xc_dom.h
===================================================================
--- xen-4.4.1.orig/tools/libxc/xc_dom.h
+++ xen-4.4.1/tools/libxc/xc_dom.h
@@ -33,8 +33,13 @@ struct xc_dom_seg {
 
 struct xc_dom_mem {
     struct xc_dom_mem *next;
-    void *mmap_ptr;
-    size_t mmap_len;
+    void *ptr;
+    enum {
+        XC_DOM_MEM_TYPE_MALLOC_INTERNAL,
+        XC_DOM_MEM_TYPE_MALLOC_EXTERNAL,
+        XC_DOM_MEM_TYPE_MMAP,
+    } type;
+    size_t len;
     unsigned char memory[0];
 };
 
@@ -290,6 +295,7 @@ void xc_dom_log_memory_footprint(struct
 /* --- simple memory pool ------------------------------------------ */
 
 void *xc_dom_malloc(struct xc_dom_image *dom, size_t size);
+int xc_dom_register_external(struct xc_dom_image *dom, void *ptr, size_t size);
 void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size);
 void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
                             const char *filename, size_t * size,
Index: xen-4.4.1/tools/libxc/xc_dom_bzimageloader.c
===================================================================
--- xen-4.4.1.orig/tools/libxc/xc_dom_bzimageloader.c
+++ xen-4.4.1/tools/libxc/xc_dom_bzimageloader.c
@@ -161,6 +161,13 @@ static int xc_try_bzip2_decode(
 
     total = (((uint64_t)stream.total_out_hi32) << 32) | stream.total_out_lo32;
 
+    if ( xc_dom_register_external(dom, out_buf, total) )
+    {
+        DOMPRINTF("BZIP2: Error registering stream output");
+        free(out_buf);
+        goto bzip2_cleanup;
+    }
+
     DOMPRINTF("%s: BZIP2 decompress OK, 0x%zx -> 0x%lx",
               __FUNCTION__, *size, (long unsigned int) total);
 
@@ -305,6 +312,13 @@ static int _xc_try_lzma_decode(
         }
     }
 
+    if ( xc_dom_register_external(dom, out_buf, stream->total_out) )
+    {
+        DOMPRINTF("%s: Error registering stream output", what);
+        free(out_buf);
+        goto lzma_cleanup;
+    }
+
     DOMPRINTF("%s: %s decompress OK, 0x%zx -> 0x%zx",
               __FUNCTION__, what, *size, (size_t)stream->total_out);
 
@@ -464,7 +478,13 @@ static int xc_try_lzo1x_decode(
 
         dst_len = lzo_read_32(cur);
         if ( !dst_len )
+        {
+            msg = "Error registering stream output";
+            if ( xc_dom_register_external(dom, out_buf, out_len) )
+                break;
+
             return 0;
+        }
 
         if ( dst_len > LZOP_MAX_BLOCK_SIZE )
         {
Index: xen-4.4.1/tools/libxc/xc_dom_core.c
===================================================================
--- xen-4.4.1.orig/tools/libxc/xc_dom_core.c
+++ xen-4.4.1/tools/libxc/xc_dom_core.c
@@ -132,6 +132,7 @@ void *xc_dom_malloc(struct xc_dom_image
         return NULL;
     }
     memset(block, 0, sizeof(*block) + size);
+    block->type = XC_DOM_MEM_TYPE_MALLOC_INTERNAL;
     block->next = dom->memblocks;
     dom->memblocks = block;
     dom->alloc_malloc += sizeof(*block) + size;
@@ -151,23 +152,45 @@ void *xc_dom_malloc_page_aligned(struct
         return NULL;
     }
     memset(block, 0, sizeof(*block));
-    block->mmap_len = size;
-    block->mmap_ptr = mmap(NULL, block->mmap_len,
-                           PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON,
-                           -1, 0);
-    if ( block->mmap_ptr == MAP_FAILED )
+    block->len = size;
+    block->ptr = mmap(NULL, block->len,
+                      PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON,
+                      -1, 0);
+    if ( block->ptr == MAP_FAILED )
     {
         DOMPRINTF("%s: mmap failed", __FUNCTION__);
         free(block);
         return NULL;
     }
+    block->type = XC_DOM_MEM_TYPE_MMAP;
     block->next = dom->memblocks;
     dom->memblocks = block;
     dom->alloc_malloc += sizeof(*block);
-    dom->alloc_mem_map += block->mmap_len;
+    dom->alloc_mem_map += block->len;
     if ( size > (100 * 1024) )
         print_mem(dom, __FUNCTION__, size);
-    return block->mmap_ptr;
+    return block->ptr;
+}
+
+int xc_dom_register_external(struct xc_dom_image *dom, void *ptr, size_t size)
+{
+    struct xc_dom_mem *block;
+
+    block = malloc(sizeof(*block));
+    if ( block == NULL )
+    {
+        DOMPRINTF("%s: allocation failed", __FUNCTION__);
+        return -1;
+    }
+    memset(block, 0, sizeof(*block));
+    block->ptr = ptr;
+    block->len = size;
+    block->type = XC_DOM_MEM_TYPE_MALLOC_EXTERNAL;
+    block->next = dom->memblocks;
+    dom->memblocks = block;
+    dom->alloc_malloc += sizeof(*block);
+    dom->alloc_mem_map += block->len;
+    return 0;
 }
 
 void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
@@ -212,24 +235,25 @@ void *xc_dom_malloc_filemap(struct xc_do
     }
 
     memset(block, 0, sizeof(*block));
-    block->mmap_len = *size;
-    block->mmap_ptr = mmap(NULL, block->mmap_len, PROT_READ,
+    block->len = *size;
+    block->ptr = mmap(NULL, block->len, PROT_READ,
                            MAP_SHARED, fd, 0);
-    if ( block->mmap_ptr == MAP_FAILED ) {
+    if ( block->ptr == MAP_FAILED ) {
         xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
                      "failed to mmap file: %s",
                      strerror(errno));
         goto err;
     }
 
+    block->type = XC_DOM_MEM_TYPE_MMAP;
     block->next = dom->memblocks;
     dom->memblocks = block;
     dom->alloc_malloc += sizeof(*block);
-    dom->alloc_file_map += block->mmap_len;
+    dom->alloc_file_map += block->len;
     close(fd);
     if ( *size > (100 * 1024) )
         print_mem(dom, __FUNCTION__, *size);
-    return block->mmap_ptr;
+    return block->ptr;
 
  err:
     if ( fd != -1 )
@@ -246,8 +270,17 @@ static void xc_dom_free_all(struct xc_do
     while ( (block = dom->memblocks) != NULL )
     {
         dom->memblocks = block->next;
-        if ( block->mmap_ptr )
-            munmap(block->mmap_ptr, block->mmap_len);
+        switch ( block->type )
+        {
+        case XC_DOM_MEM_TYPE_MALLOC_INTERNAL:
+            break;
+        case XC_DOM_MEM_TYPE_MALLOC_EXTERNAL:
+            free(block->ptr);
+            break;
+        case XC_DOM_MEM_TYPE_MMAP:
+            munmap(block->ptr, block->len);
+            break;
+        }
         free(block);
     }
 }
Index: xen-4.4.1/tools/libxc/xc_dom_decompress_lz4.c
===================================================================
--- xen-4.4.1.orig/tools/libxc/xc_dom_decompress_lz4.c
+++ xen-4.4.1/tools/libxc/xc_dom_decompress_lz4.c
@@ -104,6 +104,11 @@ int xc_try_lz4_decode(
 
 		if (size == 0)
 		{
+			if ( xc_dom_register_external(dom, output, out_len) )
+			{
+				msg = "Error registering stream output";
+				goto exit_2;
+			}
 			*blob = output;
 			*psize = out_len;
 			return 0;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Ian Campbell]

On Thu, 2014-11-20 at 22:13 -0500, Gedalya wrote:
> On 11/20/2014 03:21 PM, Konrad Rzeszutek Wilk wrote:
> > On Thu, Nov 20, 2014 at 03:48:47PM +0000, Ian Campbell wrote:
> >> The libxc xc_dom_* infrastructure uses a very simple malloc memory pool which
> >> is freed by xc_dom_release. However the various xc_try_*_decode routines (other
> >> than the gzip one) just use plain malloc/realloc and therefore the buffer ends
> >> up leaked.
> >>
> >> The memory pool currently supports mmap'd buffers as well as a directly
> >> allocated buffers, however the try decode routines make use of realloc and do
> >> not fit well into this model. Introduce a concept of an external memory block
> >> to the memory pool and provide an interface to register such memory.
> >>
> >> The mmap_ptr and mmap_len fields of the memblock tracking struct lose their
> >> mmap_ prefix since they are now also used for external memory blocks.
> >>
> >> We are only seeing this now because the gzip decoder doesn't leak and it's only
> >> relatively recently that kernels in the wild have switched to better
> >> compression.
> >>
> >> This is https://bugs.debian.org/767295
> >>
> >> Reported by: Gedalya <gedalya@gedalya.net>
> > Gedelya,
> >
> > Could you also test this patch to make sure it does fix the
> > reported issue please?
> 
> So here's what happens now.
> 1. Starts up tiny
> 2. reboot: leak
> 3. reboot: freed (process larger, but the delta is all/mostly shared pages)
> 4. reboot: leak
> 5. reboot: freed
> etc..

WTF, how very strange!

> root@xen:~/xen-pkgs# xl cr /etc/xen/auto/asterisk_deb80.cfg
> Parsing config from /etc/xen/auto/asterisk_deb80.cfg
> root@xen:~/xen-pkgs# ps aux | grep asterisk_deb80
> root     22981  0.0  0.0  95968   588 ?        SLsl 21:55   0:00 /usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
> root@xen:~/xen-pkgs# pmap -x 22981
> 22981:   /usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
> Address           Kbytes     RSS   Dirty Mode  Mapping
> 0000000000400000     144     128       0 r-x-- xl
> 0000000000623000       4       4       4 r---- xl
> 0000000000624000       8       8       8 rw--- xl
> 0000000000626000       4       4       4 rw---   [ anon ]
> 00000000009a6000     288     240     240 rw---   [ anon ]
> 00007f14d4000000     132       8       8 rw---   [ anon ]
> 00007f14d4021000   65404       0       0 -----   [ anon ]
> << snip >>
> ---------------- ------- ------- -------
> total kB           95968    2728     596
> 
> --- reboot domu ---
> 
> root@xen:~/xen-pkgs# ps aux | grep asterisk_deb80
> root     22981  0.6  3.3 131652 20008 ?        SLsl 21:55   0:00 /usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
> root@xen:~/xen-pkgs# pmap -x 22981
> 22981:   /usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
> Address           Kbytes     RSS   Dirty Mode  Mapping
> 0000000000400000     144     144       0 r-x-- xl
> 0000000000623000       4       4       4 r---- xl
> 0000000000624000       8       8       8 rw--- xl
> 0000000000626000       4       4       4 rw---   [ anon ]
> 00000000009a6000     288     288     288 rw---   [ anon ]
> 00000000009ee000   35676   16772   16772 rw---   [ anon ]

This is the (temporarily) leaked mapping, right?

> Tried valgrind, it doesn't look like it was able to see what was going on

Indeed. The values for total heap usage at exist and still reachable etc
also don't seem to account for the ~3M of mapping on each iteration.

I don't know how glibc's allocator works, but I suppose it isn't
impossible that it is retaining some mappings of free regions and
collecting them to free later somehow, which just happens to only
trigger every other reboot (e.g. perhaps it is based on some threshold
of free memory).

...investigates...

So, http://man7.org/linux/man-pages/man3/malloc.3.html talks about
special behaviour using mmap for allocations above MMAP_THRESHOLD (128K
by default), which we will be hitting here I think. That explains the
anon mapping.

http://man7.org/linux/man-pages/man3/mallopt.3.html also talks about
various dynamic thresholds for growing and shrinking the heap. My guess
is that we are bouncing up and down over some threshold with every other
reboot.

Ian.

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Ian Campbell]

On Fri, 2014-11-21 at 11:03 +0000, Ian Campbell wrote:
> http://man7.org/linux/man-pages/man3/mallopt.3.html also talks about
> various dynamic thresholds for growing and shrinking the heap. My guess
> is that we are bouncing up and down over some threshold with every other
> reboot.

IOW I'm not overly concerned with this apparent bi-modality, so long as
the amount isn't increasing in the long term...

I think the original patch should go in.

Ian.

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Gedalya]

On 11/21/2014 06:12 AM, Ian Campbell wrote:
> On Fri, 2014-11-21 at 11:03 +0000, Ian Campbell wrote:
>> http://man7.org/linux/man-pages/man3/mallopt.3.html also talks about
>> various dynamic thresholds for growing and shrinking the heap. My guess
>> is that we are bouncing up and down over some threshold with every other
>> reboot.
> IOW I'm not overly concerned with this apparent bi-modality, so long as
> the amount isn't increasing in the long term...
>
> I think the original patch should go in.
>
> Ian.
>
>
It's an improvement, but consider this:
Someone has a xen host running wheezy, 40 domu's, with 768MB for dom0, 
worked fine so far. Tries upgrading to jessie, and lo, each domu process 
takes up only 588 KB on dom0, great!
Then a new kernel package is released, all domu's get rebooted once. All 
host memory is now full. Dude might have had other plans for that 
memory... This is dead memory so I guess it can be swapped out, not 
easily a scenario where the server totally crashes, but it's a bit ugly, 
we're talking about memory usage leaping from 0.6 to 16 MB per domu.

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Ian Campbell]

On Fri, 2014-11-21 at 15:25 -0500, Gedalya wrote:
> On 11/21/2014 06:12 AM, Ian Campbell wrote:
> > On Fri, 2014-11-21 at 11:03 +0000, Ian Campbell wrote:
> >> http://man7.org/linux/man-pages/man3/mallopt.3.html also talks about
> >> various dynamic thresholds for growing and shrinking the heap. My guess
> >> is that we are bouncing up and down over some threshold with every other
> >> reboot.
> > IOW I'm not overly concerned with this apparent bi-modality, so long as
> > the amount isn't increasing in the long term...
> >
> > I think the original patch should go in.
> >
> > Ian.
> >
> >
> It's an improvement, but consider this:
> Someone has a xen host running wheezy, 40 domu's, with 768MB for dom0, 
> worked fine so far. Tries upgrading to jessie, and lo, each domu process 
> takes up only 588 KB on dom0, great!
> Then a new kernel package is released, all domu's get rebooted once. All 
> host memory is now full. Dude might have had other plans for that 
> memory... This is dead memory so I guess it can be swapped out, not 
> easily a scenario where the server totally crashes, but it's a bit ugly, 
> we're talking about memory usage leaping from 0.6 to 16 MB per domu.

Unfortunately this is down to the behaviour of the libc and not
something which appears to be under application control. 

The following program demonstrates the same behaviour and is certainly
not leaking anything. Notice that at "Freed block at XXXXX. Everything
is now freed, end of day" there is still an anon mapping of that
address. Notice also that the "in use" figures are zero.

If this concerns you then you should probably take a look at mallopt(3)
and/or be talking to the libc folks about it. It's not an xl issue
AFAICT.

Ian.

#include <sys/types.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

#include <malloc.h>

#define KB 196

int main(int argc, char **argv)
{
	void *p;
	char buf[1000];

	snprintf(buf, 1000, "pmap -x %d", getpid());

	printf("Start of day\n");
	system(buf);
	malloc_stats();

	printf("\n=========================\n\n");

	p = malloc(KB*0x1000);

	printf("allocated %dKB block at %p\n", KB, p);
	system(buf);
	malloc_stats();

	printf("\n=========================\n\n");

	free(p);

	printf("Freed block at %p\n", p);
	system(buf);
	malloc_stats();

	printf("\n=========================\n\n");

	p = malloc(KB*0x1000);

	printf("Allocated another %dKB block at %p\n", KB, p);
	system(buf);
	malloc_stats();

	printf("\n=========================\n\n");

	free(p);

	printf("Freed block at %p. Everything is now freed, end of day\n", p);
	system(buf);
	malloc_stats();

	printf("\n=========================\n\n");

	return 0;

}

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Gedalya]

On 11/24/2014 05:37 AM, Ian Campbell wrote:
> Unfortunately this is down to the behaviour of the libc and not
> something which appears to be under application control.
>
> The following program demonstrates the same behaviour and is certainly
> not leaking anything. Notice that at "Freed block at XXXXX. Everything
> is now freed, end of day" there is still an anon mapping of that
> address. Notice also that the "in use" figures are zero.
>
> If this concerns you then you should probably take a look at mallopt(3)
> and/or be talking to the libc folks about it. It's not an xl issue
> AFAICT.
Firstly, thank you very much for explaining this in such clear detail, 
above all this has been quite educational for me :-)
After reading the man page, it looks like glibc's behavior here is 
indeed by design. I'm unable to form, much less advocate an opinion 
about how libc should behave, any discussion about libc must be very broad.
Stepping away from the technical details, I still think that any future 
enhancement to make xl go out of its way to free this memory would 
definitely be nice. Right now we have memory that is allocated for a 
single, momentary use and it can stay allocated for the lifetime of a 
domu unnecessarily, taking the size of the xl process to another order 
of magnitude. Even if no longer technically a bug / memory leak, and the 
implied priority, this could still merit someone's attention.

Re: [PATCH for-4.5 v2] libxc: don't leak buffer containing the uncompressed PV kernel

[Gedalya]

On 11/21/2014 06:03 AM, Ian Campbell wrote:
>
>> So here's what happens now.
>> 1. Starts up tiny
>> 2. reboot: leak
>> 3. reboot: freed (process larger, but the delta is all/mostly shared pages)
>> 4. reboot: leak
>> 5. reboot: freed
>> etc..
> WTF, how very strange!
:-)

>
>> --- reboot domu ---
>>
>> root@xen:~/xen-pkgs# ps aux | grep asterisk_deb80
>> root     22981  0.6  3.3 131652 20008 ?        SLsl 21:55   0:00 /usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
>> root@xen:~/xen-pkgs# pmap -x 22981
>> 22981:   /usr/lib/xen-4.4/bin/xl cr /etc/xen/auto/asterisk_deb80.cfg
>> Address           Kbytes     RSS   Dirty Mode  Mapping
>> 0000000000400000     144     144       0 r-x-- xl
>> 0000000000623000       4       4       4 r---- xl
>> 0000000000624000       8       8       8 rw--- xl
>> 0000000000626000       4       4       4 rw---   [ anon ]
>> 00000000009a6000     288     288     288 rw---   [ anon ]
>> 00000000009ee000   35676   16772   16772 rw---   [ anon ]
> This is the (temporarily) leaked mapping, right?
Yea that's the one that popped in after the reboot..
About 16 MB.

>
>> Tried valgrind, it doesn't look like it was able to see what was going on
> Indeed. The values for total heap usage at exist and still reachable etc
> also don't seem to account for the ~3M of mapping on each iteration.
>
> I don't know how glibc's allocator works, but I suppose it isn't
> impossible that it is retaining some mappings of free regions and
> collecting them to free later somehow, which just happens to only
> trigger every other reboot (e.g. perhaps it is based on some threshold
> of free memory).
>
> ...investigates...
>
> So, http://man7.org/linux/man-pages/man3/malloc.3.html talks about
> special behaviour using mmap for allocations above MMAP_THRESHOLD (128K
> by default), which we will be hitting here I think. That explains the
> anon mapping.
>
> http://man7.org/linux/man-pages/man3/mallopt.3.html also talks about
> various dynamic thresholds for growing and shrinking the heap. My guess
> is that we are bouncing up and down over some threshold with every other
> reboot.
>
> Ian.
OK this is way over my head.. I don't have a full and precise 
understanding of all of the above, but let me try to comment nevertheless.
There are two issues here. The added mappings (as I understand, files, 
non-anon, shared) do happen only on reboot, but they're not a real 
memory leak issue because they are shared with other processes, so no 
matter ho many xl processes we have, it's only another 2.6 or so MB 
added to the total memory usage of the server, right?
On the other hand we have this anon area, 16 MB, that pops in on one 
reboot and gets freed on the next. That's the real issue, as I see it..!
And all that only starts happening after the last line of valgrind 
output. Valgrind only had output up to the first boot of the VM, none later.
For a fresh, non-rebooted domu, the xl process shows up as in top as 
~588 KB RES, 0 SHR, how can the latter be anyway?? I don't understand.