[PATCH] libxc: fix memory leak in migration v2

[PATCH] libxc: fix memory leak in migration v2

[Wei Liu]

Originally there was only one counter to keep track of pages. It was
used erroneously to keep track of how many pages were mapped and how
many pages needed to be send. In the end munmap(2) always has 0 as the
length argument, which resulted in leaking the mapping.

This problem is discovered on 32bit toolstack because 32bit application
has notably smaller address space. In fact this bug affects 64bit
toolstack too.

Use a separate counter to keep track of how many pages to send to solve
this issue.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxc/xc_sr_save.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index d63b783..bbcfeb6 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -84,7 +84,7 @@ static int write_batch(struct xc_sr_context *ctx)
     void **guest_data = NULL;
     void **local_pages = NULL;
     int *errors = NULL, rc = -1;
-    unsigned i, p, nr_pages = 0;
+    unsigned i, p, nr_pages = 0, nr_pages_to_send;
     unsigned nr_pfns = ctx->save.nr_batch_pfns;
     void *page, *orig_page;
     uint64_t *rec_pfns = NULL;
@@ -151,10 +151,12 @@ static int write_batch(struct xc_sr_context *ctx)
         mfns[nr_pages++] = mfns[i];
     }
 
-    if ( nr_pages > 0 )
+    nr_pages_to_send = nr_pages;
+
+    if ( nr_pages_to_send > 0 )
     {
         guest_mapping = xc_map_foreign_bulk(
-            xch, ctx->domid, PROT_READ, mfns, errors, nr_pages);
+            xch, ctx->domid, PROT_READ, mfns, errors, nr_pages_to_send);
         if ( !guest_mapping )
         {
             PERROR("Failed to map guest pages");
@@ -191,7 +193,7 @@ static int write_batch(struct xc_sr_context *ctx)
                     set_bit(ctx->save.batch_pfns[i], ctx->save.deferred_pages);
                     ++ctx->save.nr_deferred_pages;
                     types[i] = XEN_DOMCTL_PFINFO_XTAB;
-                    --nr_pages;
+                    --nr_pages_to_send;
                 }
                 else
                     goto err;
@@ -216,7 +218,7 @@ static int write_batch(struct xc_sr_context *ctx)
 
     rec.length = sizeof(hdr);
     rec.length += nr_pfns * sizeof(*rec_pfns);
-    rec.length += nr_pages * PAGE_SIZE;
+    rec.length += nr_pages_to_send * PAGE_SIZE;
 
     for ( i = 0; i < nr_pfns; ++i )
         rec_pfns[i] = ((uint64_t)(types[i]) << 32) | ctx->save.batch_pfns[i];
@@ -235,7 +237,7 @@ static int write_batch(struct xc_sr_context *ctx)
 
     iovcnt = 4;
 
-    if ( nr_pages )
+    if ( nr_pages_to_send )
     {
         for ( i = 0; i < nr_pfns; ++i )
         {
@@ -244,7 +246,7 @@ static int write_batch(struct xc_sr_context *ctx)
                 iov[iovcnt].iov_base = guest_data[i];
                 iov[iovcnt].iov_len = PAGE_SIZE;
                 iovcnt++;
-                --nr_pages;
+                --nr_pages_to_send;
             }
         }
     }
@@ -256,7 +258,7 @@ static int write_batch(struct xc_sr_context *ctx)
     }
 
     /* Sanity check we have sent all the pages we expected to. */
-    assert(nr_pages == 0);
+    assert(nr_pages_to_send == 0);
     rc = ctx->save.nr_batch_pfns = 0;
 
  err:
-- 
2.1.4

Re: [PATCH] libxc: fix memory leak in migration v2

[Konrad Rzeszutek Wilk]

On July 24, 2015 6:40:27 PM EDT, Wei Liu <wei.liu2@citrix.com> wrote:
>Originally there was only one counter to keep track of pages. It was
>used erroneously to keep track of how many pages were mapped and how
>many pages needed to be send. In the


sent

> end munmap(2) always has 0 as the
>length argument, which resulted in leaking the mapping.
>
>This problem is discovered on 32bit toolstack because 32bit application

/is/was/

>has notably smaller address space. In fact this bug affects 64bit
>toolstack too.

s/has/have/

>
>Use a separate counter to keep track of how many pages to send to solve
>this issue.
>
>Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>---
>Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>Cc: Ian Campbell <ian.campbell@citrix.com>
>Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>---
> tools/libxc/xc_sr_save.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
>diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
>index d63b783..bbcfeb6 100644
>--- a/tools/libxc/xc_sr_save.c
>+++ b/tools/libxc/xc_sr_save.c
>@@ -84,7 +84,7 @@ static int write_batch(struct xc_sr_context *ctx)
>     void **guest_data = NULL;
>     void **local_pages = NULL;
>     int *errors = NULL, rc = -1;
>-    unsigned i, p, nr_pages = 0;
>+    unsigned i, p, nr_pages = 0, nr_pages_to_send;
>     unsigned nr_pfns = ctx->save.nr_batch_pfns;
>     void *page, *orig_page;
>     uint64_t *rec_pfns = NULL;
>@@ -151,10 +151,12 @@ static int write_batch(struct xc_sr_context *ctx)
>         mfns[nr_pages++] = mfns[i];
>     }
> 
>-    if ( nr_pages > 0 )
>+    nr_pages_to_send = nr_pages;
>+
>+    if ( nr_pages_to_send > 0 )
>     {
>         guest_mapping = xc_map_foreign_bulk(
>-            xch, ctx->domid, PROT_READ, mfns, errors, nr_pages);
>+            xch, ctx->domid, PROT_READ, mfns, errors,
>nr_pages_to_send);
>         if ( !guest_mapping )
>         {
>             PERROR("Failed to map guest pages");
>@@ -191,7 +193,7 @@ static int write_batch(struct xc_sr_context *ctx)
>            set_bit(ctx->save.batch_pfns[i], ctx->save.deferred_pages);
>                     ++ctx->save.nr_deferred_pages;
>                     types[i] = XEN_DOMCTL_PFINFO_XTAB;
>-                    --nr_pages;
>+                    --nr_pages_to_send;
>                 }
>                 else
>                     goto err;
>@@ -216,7 +218,7 @@ static int write_batch(struct xc_sr_context *ctx)
> 
>     rec.length = sizeof(hdr);
>     rec.length += nr_pfns * sizeof(*rec_pfns);
>-    rec.length += nr_pages * PAGE_SIZE;
>+    rec.length += nr_pages_to_send * PAGE_SIZE;
> 
>     for ( i = 0; i < nr_pfns; ++i )
>  rec_pfns[i] = ((uint64_t)(types[i]) << 32) | ctx->save.batch_pfns[i];
>@@ -235,7 +237,7 @@ static int write_batch(struct xc_sr_context *ctx)
> 
>     iovcnt = 4;
> 
>-    if ( nr_pages )
>+    if ( nr_pages_to_send )
>     {
>         for ( i = 0; i < nr_pfns; ++i )
>         {
>@@ -244,7 +246,7 @@ static int write_batch(struct xc_sr_context *ctx)
>                 iov[iovcnt].iov_base = guest_data[i];
>                 iov[iovcnt].iov_len = PAGE_SIZE;
>                 iovcnt++;
>-                --nr_pages;
>+                --nr_pages_to_send;
>             }
>         }
>     }
>@@ -256,7 +258,7 @@ static int write_batch(struct xc_sr_context *ctx)
>     }
> 
>     /* Sanity check we have sent all the pages we expected to. */
>-    assert(nr_pages == 0);
>+    assert(nr_pages_to_send == 0);
>     rc = ctx->save.nr_batch_pfns = 0;
> 
>  err:

Re: [PATCH] libxc: fix memory leak in migration v2

[Andrew Cooper]

On 24/07/2015 23:40, Wei Liu wrote:
> Originally there was only one counter to keep track of pages. It was
> used erroneously to keep track of how many pages were mapped and how
> many pages needed to be send. In the end munmap(2) always has 0 as the
> length argument, which resulted in leaking the mapping.
>
> This problem is discovered on 32bit toolstack because 32bit application
> has notably smaller address space. In fact this bug affects 64bit
> toolstack too.
>
> Use a separate counter to keep track of how many pages to send to solve
> this issue.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Yikes - good catch.  (This logic definitely worked at first, because I
had debugging which confirmed as much.  It must have gotten broken in
the 18 months of minor tweaks since it was originally written).

I believe the patch is correct.  However, can I recommend instead having
"nr_pages_mapped" which is set after the call to xc_map_foreign_bulk(). 
This will make a shorter and more-obviously correct patch.

Valgrind doesn't track mmap()/munmap() calls because of their typical
use for shared libraries.  However, it turns out that the
VALGRIND_{MALLOC,FREE}LIKE_BLOCK client requests can be used to mark
arbitrary blocks of memory as tracked by the standard memcheck
infrastructure.

For 4.7 (which happens to coincide with the splitting up of libxc), I
recommend introducing xc_unmap() and requiring that all uses of
xc_map_$FOO() use it.  This way, the client requests can be present in
the library, and all mappings of foreign memory can be tracked by
valgrind in debug builds.

~Andrew

Re: [PATCH] libxc: fix memory leak in migration v2

[Ian Campbell]

On Sun, 2015-07-26 at 17:47 +0100, Andrew Cooper wrote:
> For 4.7 (which happens to coincide with the splitting up of libxc), I
> recommend introducing xc_unmap()

I thought I'd done that in my latest series, but looking for the
precise name now I see that there is no such function and
xenforeignmemory_map() still documents unmapping with munmap. I shall
make a note to introduce xenforeignmemory_unmap()!

Ian.