* [PATCH] libxc: fix memory leak in migration v2
@ 2015-07-24 22:40 Wei Liu
2015-07-24 23:00 ` Konrad Rzeszutek Wilk
2015-07-26 16:47 ` Andrew Cooper
0 siblings, 2 replies; 4+ messages in thread
From: Wei Liu @ 2015-07-24 22:40 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Andrew Cooper, Wei Liu, Ian Campbell
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] libxc: fix memory leak in migration v2
2015-07-24 22:40 [PATCH] libxc: fix memory leak in migration v2 Wei Liu
@ 2015-07-24 23:00 ` Konrad Rzeszutek Wilk
2015-07-26 16:47 ` Andrew Cooper
1 sibling, 0 replies; 4+ messages in thread
From: Konrad Rzeszutek Wilk @ 2015-07-24 23:00 UTC (permalink / raw)
To: Wei Liu, xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell
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:
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] libxc: fix memory leak in migration v2
2015-07-24 22:40 [PATCH] libxc: fix memory leak in migration v2 Wei Liu
2015-07-24 23:00 ` Konrad Rzeszutek Wilk
@ 2015-07-26 16:47 ` Andrew Cooper
2015-07-27 8:04 ` Ian Campbell
1 sibling, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2015-07-26 16:47 UTC (permalink / raw)
To: Wei Liu, xen-devel; +Cc: Ian Jackson, Ian Campbell
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] libxc: fix memory leak in migration v2
2015-07-26 16:47 ` Andrew Cooper
@ 2015-07-27 8:04 ` Ian Campbell
0 siblings, 0 replies; 4+ messages in thread
From: Ian Campbell @ 2015-07-27 8:04 UTC (permalink / raw)
To: Andrew Cooper, Wei Liu, xen-devel; +Cc: Ian Jackson
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-07-27 8:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-24 22:40 [PATCH] libxc: fix memory leak in migration v2 Wei Liu
2015-07-24 23:00 ` Konrad Rzeszutek Wilk
2015-07-26 16:47 ` Andrew Cooper
2015-07-27 8:04 ` Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).