Please see some of the patches for tmem that Coverity has flagged.
And also the xenstat which I had fixed in the past but did not fix
it completely.
tools/libxc/xc_domain_save.c | 6 ++--
tools/libxc/xc_tmem.c | 51 ++++++++++++++++++++--------------
tools/libxc/xenctrl.h | 2 +-
tools/xenstat/libxenstat/src/xenstat.c | 2 +-
xen/common/tmem.c | 23 ++++++++++-----
5 files changed, 52 insertions(+), 32 deletions(-)
Konrad Rzeszutek Wilk (5):
xc/tmem: Free temporary buffer used during migration.
xc/tmem: Unchecked return value.
tmem: Check copy_to_user_* return value.
tmem: Pointless check of NULL against an array
xenstat: Fix buffer over-run with new_domains being negative (again).
Konrad Rzeszutek Wilk
2013-Nov-25 17:00 UTC
[PATCH 1/5] xc/tmem: Free temporary buffer used during migration.
CID 1090388.
Within the loop reading the pool_id we set the buf:
if ( (buf = realloc(buf,bufsize)) == NULL )
and then continue on using it without freeing. Worst yet
there are multiple ''if (..) return -1'' which do not free
the buffer.
As such insert a ''fail'' goto label to free the buffer
and also add on the OK path a mechanism to free the buffer.
Replace all of the ''return -1'' with a jump to the failed
label.
CC: Bob Liu <bob.liu@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
tools/libxc/xc_tmem.c | 47 ++++++++++++++++++++++++++++-------------------
1 file changed, 28 insertions(+), 19 deletions(-)
diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
index 61e1549..0918615 100644
--- a/tools/libxc/xc_tmem.c
+++ b/tools/libxc/xc_tmem.c
@@ -225,6 +225,7 @@ int xc_tmem_save(xc_interface *xch,
uint32_t pool_id;
uint32_t minusone = -1;
struct tmem_handle *h;
+ char *buf = NULL;
if ( xc_tmem_control(xch,0,TMEMC_SAVE_BEGIN,dom,live,0,0,NULL) <= 0 )
return 0;
@@ -259,7 +260,6 @@ int xc_tmem_save(xc_interface *xch,
uint64_t uuid[2];
uint32_t n_pages;
uint32_t pagesize;
- char *buf = NULL;
int bufsize = 0;
int checksum = 0;
@@ -273,13 +273,13 @@ int xc_tmem_save(xc_interface *xch,
n_pages = 0;
(void)xc_tmem_control(xch,i,TMEMC_SAVE_GET_POOL_UUID,dom,sizeof(uuid),0,0,&uuid);
if ( write_exact(io_fd, &pool_id, sizeof(pool_id)) )
- return -1;
+ goto failed;
if ( write_exact(io_fd, &flags, sizeof(flags)) )
- return -1;
+ goto failed;
if ( write_exact(io_fd, &n_pages, sizeof(n_pages)) )
- return -1;
+ goto failed;
if ( write_exact(io_fd, &uuid, sizeof(uuid)) )
- return -1;
+ goto failed;
if ( n_pages == 0 )
continue;
@@ -289,7 +289,7 @@ int xc_tmem_save(xc_interface *xch,
{
bufsize = pagesize + sizeof(struct tmem_handle);
if ( (buf = realloc(buf,bufsize)) == NULL )
- return -1;
+ goto failed;
}
for ( j = n_pages; j > 0; j-- )
{
@@ -300,21 +300,22 @@ int xc_tmem_save(xc_interface *xch,
{
h = (struct tmem_handle *)buf;
if ( write_exact(io_fd, &h->oid, sizeof(h->oid))
)
- return -1;
+ goto failed;
if ( write_exact(io_fd, &h->index,
sizeof(h->index)) )
- return -1;
+ goto failed;
h++;
checksum += *(char *)h;
if ( write_exact(io_fd, h, pagesize) )
- return -1;
+ goto failed;
} else if ( ret == 0 ) {
continue;
} else {
/* page list terminator */
h = (struct tmem_handle *)buf;
h->oid[0] = h->oid[1] = h->oid[2] = -1L;
- if ( write_exact(io_fd, &h->oid, sizeof(h->oid))
)
- return -1;
+ if ( write_exact(io_fd, &h->oid, sizeof(h->oid))
) {
+ goto failed;
+ }
break;
}
}
@@ -325,9 +326,13 @@ int xc_tmem_save(xc_interface *xch,
/* pool list terminator */
minusone = -1;
if ( write_exact(io_fd, &minusone, sizeof(minusone)) )
- return -1;
+ goto failed;
+ free(buf);
return 1;
+failed:
+ free(buf);
+ return -1;
}
/* only called for live migration */
@@ -396,6 +401,7 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
uint32_t minusone;
uint32_t weight, cap, flags;
int checksum = 0;
+ char *buf = NULL;
save_version =
xc_tmem_control(xch,0,TMEMC_SAVE_GET_VERSION,dom,0,0,0,NULL);
if ( save_version == -1 )
@@ -433,7 +439,6 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
{
uint64_t uuid[2];
uint32_t n_pages;
- char *buf = NULL;
int bufsize = 0, pagesize;
int j;
@@ -455,7 +460,7 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
{
bufsize = pagesize;
if ( (buf = realloc(buf,bufsize)) == NULL )
- return -1;
+ goto failed;
}
for ( j = n_pages; j > 0; j-- )
{
@@ -463,20 +468,20 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
uint32_t index;
int rc;
if ( read_exact(io_fd, &oid, sizeof(oid)) )
- return -1;
+ goto failed;
if ( oid.oid[0] == -1L && oid.oid[1] == -1L &&
oid.oid[2] == -1L )
break;
if ( read_exact(io_fd, &index, sizeof(index)) )
- return -1;
+ goto failed;
if ( read_exact(io_fd, buf, pagesize) )
- return -1;
+ goto failed;
checksum += *buf;
if ( (rc = xc_tmem_control_oid(xch, pool_id,
TMEMC_RESTORE_PUT_PAGE, dom,
bufsize, index, oid, buf)) <= 0 )
{
DPRINTF("xc_tmem_restore: putting page failed,
rc=%d\n",rc);
- return -1;
+ goto failed;
}
}
if ( n_pages )
@@ -484,9 +489,13 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
n_pages-j,dom,pool_id,checksum);
}
if ( pool_id != -1 )
- return -1;
+ goto failed;
+ free(buf);
return 0;
+failed:
+ free(buf);
+ return -1;
}
/* only called for live migration, must be called after suspend */
--
1.8.3.1
CID 1055045 complains that the return value needs checking.
The tmem op is a flush call and there is no expectation
right now of any return besides zero. But just in case
this changes lets blow up.
CC: Bob Liu <bob.liu@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
tools/libxc/xc_domain_save.c | 6 ++++--
tools/libxc/xc_tmem.c | 4 ++--
tools/libxc/xenctrl.h | 2 +-
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index 42c4752..c3f0f19 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -2095,8 +2095,10 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t
dom, uint32_t max_iter
goto copypages;
}
- if ( tmem_saved != 0 && live )
- xc_tmem_save_done(xch, dom);
+ if ( tmem_saved != 0 && live ) {
+ if ( xc_tmem_save_done(xch, dom) )
+ DPRINTF("Warning - failed to flush tmem on originating
server.");
+ }
if ( live )
{
diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
index 0918615..df50ef5 100644
--- a/tools/libxc/xc_tmem.c
+++ b/tools/libxc/xc_tmem.c
@@ -366,9 +366,9 @@ int xc_tmem_save_extra(xc_interface *xch, int dom, int
io_fd, int field_marker)
}
/* only called for live migration */
-void xc_tmem_save_done(xc_interface *xch, int dom)
+int xc_tmem_save_done(xc_interface *xch, int dom)
{
- xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
+ return xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
}
/* restore routines */
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 4ac6b8a..18d4f78 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1996,7 +1996,7 @@ int xc_tmem_control(xc_interface *xch,
int xc_tmem_auth(xc_interface *xch, int cli_id, char *uuid_str, int arg1);
int xc_tmem_save(xc_interface *xch, int dom, int live, int fd, int
field_marker);
int xc_tmem_save_extra(xc_interface *xch, int dom, int fd, int field_marker);
-void xc_tmem_save_done(xc_interface *xch, int dom);
+int xc_tmem_save_done(xc_interface *xch, int dom);
int xc_tmem_restore(xc_interface *xch, int dom, int fd);
int xc_tmem_restore_extra(xc_interface *xch, int dom, int fd);
--
1.8.3.1
Konrad Rzeszutek Wilk
2013-Nov-25 17:00 UTC
[PATCH 3/5] tmem: Check copy_to_user_* return value.
We weren''t checking whether that operation fails and
return the proper error.
This fixes CID 1055125, 105512, 1055127, 1055128, 1055129,
1055130.
CC: Bob Liu <bob.liu@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
xen/common/tmem.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index 081772e..3bc35fd 100644
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -2146,8 +2146,12 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t
buf, uint32_t len,
if ( cli_id == TMEM_CLI_ID_NULL ) {
off = tmemc_list_global(buf,0,len,use_long);
off += tmemc_list_shared(buf,off,len-off,use_long);
- list_for_each_entry(client,&global_client_list,client_list)
- off += tmemc_list_client(client, buf, off, len-off, use_long);
+ list_for_each_entry(client,&global_client_list,client_list) {
+ int ret = tmemc_list_client(client, buf, off, len-off, use_long);
+ if ( ret < 0 )
+ return ret;
+ off += ret;
+ }
off += tmemc_list_global_perf(buf,off,len-off,use_long);
}
else if ( (client = tmem_client_from_cli_id(cli_id)) == NULL)
@@ -2155,6 +2159,8 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t
buf, uint32_t len,
else
off = tmemc_list_client(client, buf, 0, len, use_long);
+ if ( off < 0 )
+ return off;
return 0;
}
@@ -2319,8 +2325,9 @@ static int tmemc_save_subop(int cli_id, uint32_t pool_id,
case TMEMC_SAVE_GET_POOL_UUID:
if ( pool == NULL )
break;
- tmem_copy_to_client_buf(buf, pool->uuid, 2);
rc = 0;
+ if ( tmem_copy_to_client_buf(buf, pool->uuid, 2) )
+ rc = -EFAULT;
break;
case TMEMC_SAVE_END:
if ( client == NULL )
@@ -2383,7 +2390,10 @@ static int tmemc_save_get_next_page(int cli_id, uint32_t
pool_id,
BUILD_BUG_ON(sizeof(h.oid) != sizeof(oid));
memcpy(h.oid, oid.oid, sizeof(h.oid));
h.index = pgp->index;
- tmem_copy_to_client_buf(buf, &h, 1);
+ if ( tmem_copy_to_client_buf(buf, &h, 1) ) {
+ ret = -EFAULT;
+ goto out;
+ }
tmem_client_buf_add(buf, sizeof(h));
ret = do_tmem_get(pool, &oid, pgp->index, 0, 0, 0, pagesize, buf);
@@ -2427,8 +2437,9 @@ static int tmemc_save_get_next_inv(int cli_id,
tmem_cli_va_param_t buf,
BUILD_BUG_ON(sizeof(h.oid) != sizeof(pgp->inv_oid));
memcpy(h.oid, pgp->inv_oid.oid, sizeof(h.oid));
h.index = pgp->index;
- tmem_copy_to_client_buf(buf, &h, 1);
ret = 1;
+ if ( tmem_copy_to_client_buf(buf, &h, 1) )
+ ret = -EFAULT;
out:
tmem_spin_unlock(&pers_lists_spinlock);
return ret;
--
1.8.3.1
Konrad Rzeszutek Wilk
2013-Nov-25 17:00 UTC
[PATCH 4/5] tmem: Pointless check of NULL against an array
CID 1055632 tells us that we are comparing an array to a NULL
which is indeed silly. Lets not do it.
CC: Bob Liu <bob.liu@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
xen/common/tmem.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index 3bc35fd..1ebed8a 100644
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -1798,8 +1798,6 @@ static int do_tmem_destroy_pool(uint32_t pool_id)
struct client *client = tmem_client_from_current();
struct tmem_pool *pool;
- if ( client->pools == NULL )
- return 0;
if ( pool_id >= MAX_POOLS_PER_DOMAIN )
return 0;
if ( (pool = client->pools[pool_id]) == NULL )
--
1.8.3.1
Konrad Rzeszutek Wilk
2013-Nov-25 17:00 UTC
[PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again).
The git commit 1438d36f96e90d1116bebc6b3013634ca21c49c8 "xenstat: Fix buffer over-run with new_domains being negative." fixed part of the problem, but it failed to do one more check. That is the if we don''t exit out of the loop if the xc_domain_getinfolist returns -1. This makes the check by casting the unsigned int value to int as otherwise the if (new_domains < 0) would never get executed (as unsigned int won''t be negative). Fixes CID 1055740. CC: andrew.cooper3@citrix.com Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- tools/xenstat/libxenstat/src/xenstat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c index e5facb8..27d8e22 100644 --- a/tools/xenstat/libxenstat/src/xenstat.c +++ b/tools/xenstat/libxenstat/src/xenstat.c @@ -208,7 +208,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags) node->num_domains, DOMAIN_CHUNK_SIZE, domaininfo); - if (new_domains < 0) + if ((int)new_domains < 0) goto err; tmp = realloc(node->domains, -- 1.8.3.1
Ian Campbell
2013-Nov-25 17:06 UTC
Re: [PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again).
On Mon, 2013-11-25 at 12:00 -0500, Konrad Rzeszutek Wilk wrote:> The git commit 1438d36f96e90d1116bebc6b3013634ca21c49c8 > "xenstat: Fix buffer over-run with new_domains being negative." > fixed part of the problem, but it failed to do one more check. > > That is the if we don''t exit out of the loop if the > xc_domain_getinfolist returns -1. This makes the check > by casting the unsigned int value to int as otherwise > the > if (new_domains < 0) > > would never get executed (as unsigned int won''t be negative). > > Fixes CID 1055740. > > CC: andrew.cooper3@citrix.com > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > tools/xenstat/libxenstat/src/xenstat.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c > index e5facb8..27d8e22 100644 > --- a/tools/xenstat/libxenstat/src/xenstat.c > +++ b/tools/xenstat/libxenstat/src/xenstat.c > @@ -208,7 +208,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags) > node->num_domains, > DOMAIN_CHUNK_SIZE, > domaininfo); > - if (new_domains < 0) > + if ((int)new_domains < 0)I expect that the right fix is to make new_domains the correct (signed) type, no? Ian.
Andrew Cooper
2013-Nov-25 17:09 UTC
Re: [PATCH 1/5] xc/tmem: Free temporary buffer used during migration.
On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote:> CID 1090388. > > Within the loop reading the pool_id we set the buf: > > if ( (buf = realloc(buf,bufsize)) == NULL ) > > and then continue on using it without freeing. Worst yet > there are multiple ''if (..) return -1'' which do not free > the buffer. > > As such insert a ''fail'' goto label to free the buffer > and also add on the OK path a mechanism to free the buffer. > > Replace all of the ''return -1'' with a jump to the failed > label. > > CC: Bob Liu <bob.liu@oracle.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>> --- > tools/libxc/xc_tmem.c | 47 ++++++++++++++++++++++++++++------------------- > 1 file changed, 28 insertions(+), 19 deletions(-) > > diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c > index 61e1549..0918615 100644 > --- a/tools/libxc/xc_tmem.c > +++ b/tools/libxc/xc_tmem.c > @@ -225,6 +225,7 @@ int xc_tmem_save(xc_interface *xch, > uint32_t pool_id; > uint32_t minusone = -1; > struct tmem_handle *h; > + char *buf = NULL; > > if ( xc_tmem_control(xch,0,TMEMC_SAVE_BEGIN,dom,live,0,0,NULL) <= 0 ) > return 0; > @@ -259,7 +260,6 @@ int xc_tmem_save(xc_interface *xch, > uint64_t uuid[2]; > uint32_t n_pages; > uint32_t pagesize; > - char *buf = NULL; > int bufsize = 0; > int checksum = 0; > > @@ -273,13 +273,13 @@ int xc_tmem_save(xc_interface *xch, > n_pages = 0; > (void)xc_tmem_control(xch,i,TMEMC_SAVE_GET_POOL_UUID,dom,sizeof(uuid),0,0,&uuid); > if ( write_exact(io_fd, &pool_id, sizeof(pool_id)) ) > - return -1; > + goto failed; > if ( write_exact(io_fd, &flags, sizeof(flags)) ) > - return -1; > + goto failed; > if ( write_exact(io_fd, &n_pages, sizeof(n_pages)) ) > - return -1; > + goto failed; > if ( write_exact(io_fd, &uuid, sizeof(uuid)) ) > - return -1; > + goto failed; > if ( n_pages == 0 ) > continue; > > @@ -289,7 +289,7 @@ int xc_tmem_save(xc_interface *xch, > { > bufsize = pagesize + sizeof(struct tmem_handle); > if ( (buf = realloc(buf,bufsize)) == NULL ) > - return -1; > + goto failed; > } > for ( j = n_pages; j > 0; j-- ) > { > @@ -300,21 +300,22 @@ int xc_tmem_save(xc_interface *xch, > { > h = (struct tmem_handle *)buf; > if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) ) > - return -1; > + goto failed; > if ( write_exact(io_fd, &h->index, sizeof(h->index)) ) > - return -1; > + goto failed; > h++; > checksum += *(char *)h; > if ( write_exact(io_fd, h, pagesize) ) > - return -1; > + goto failed; > } else if ( ret == 0 ) { > continue; > } else { > /* page list terminator */ > h = (struct tmem_handle *)buf; > h->oid[0] = h->oid[1] = h->oid[2] = -1L; > - if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) ) > - return -1; > + if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) ) { > + goto failed; > + } > break; > } > } > @@ -325,9 +326,13 @@ int xc_tmem_save(xc_interface *xch, > /* pool list terminator */ > minusone = -1; > if ( write_exact(io_fd, &minusone, sizeof(minusone)) ) > - return -1; > + goto failed; > > + free(buf); > return 1; > +failed: > + free(buf); > + return -1; > } > > /* only called for live migration */ > @@ -396,6 +401,7 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd) > uint32_t minusone; > uint32_t weight, cap, flags; > int checksum = 0; > + char *buf = NULL; > > save_version = xc_tmem_control(xch,0,TMEMC_SAVE_GET_VERSION,dom,0,0,0,NULL); > if ( save_version == -1 ) > @@ -433,7 +439,6 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd) > { > uint64_t uuid[2]; > uint32_t n_pages; > - char *buf = NULL; > int bufsize = 0, pagesize; > int j; > > @@ -455,7 +460,7 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd) > { > bufsize = pagesize; > if ( (buf = realloc(buf,bufsize)) == NULL ) > - return -1; > + goto failed; > } > for ( j = n_pages; j > 0; j-- ) > { > @@ -463,20 +468,20 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd) > uint32_t index; > int rc; > if ( read_exact(io_fd, &oid, sizeof(oid)) ) > - return -1; > + goto failed; > if ( oid.oid[0] == -1L && oid.oid[1] == -1L && oid.oid[2] == -1L ) > break; > if ( read_exact(io_fd, &index, sizeof(index)) ) > - return -1; > + goto failed; > if ( read_exact(io_fd, buf, pagesize) ) > - return -1; > + goto failed; > checksum += *buf; > if ( (rc = xc_tmem_control_oid(xch, pool_id, > TMEMC_RESTORE_PUT_PAGE, dom, > bufsize, index, oid, buf)) <= 0 ) > { > DPRINTF("xc_tmem_restore: putting page failed, rc=%d\n",rc); > - return -1; > + goto failed; > } > } > if ( n_pages ) > @@ -484,9 +489,13 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd) > n_pages-j,dom,pool_id,checksum); > } > if ( pool_id != -1 ) > - return -1; > + goto failed; > > + free(buf); > return 0; > +failed: > + free(buf); > + return -1; > } > > /* only called for live migration, must be called after suspend */
On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote:> CID 1055045 complains that the return value needs checking. > > The tmem op is a flush call and there is no expectation > right now of any return besides zero. But just in case > this changes lets blow up. > > CC: Bob Liu <bob.liu@oracle.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > tools/libxc/xc_domain_save.c | 6 ++++-- > tools/libxc/xc_tmem.c | 4 ++-- > tools/libxc/xenctrl.h | 2 +- > 3 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c > index 42c4752..c3f0f19 100644 > --- a/tools/libxc/xc_domain_save.c > +++ b/tools/libxc/xc_domain_save.c > @@ -2095,8 +2095,10 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter > goto copypages; > } > > - if ( tmem_saved != 0 && live ) > - xc_tmem_save_done(xch, dom); > + if ( tmem_saved != 0 && live ) { > + if ( xc_tmem_save_done(xch, dom) )These could be combined into a single if statement.> + DPRINTF("Warning - failed to flush tmem on originating server.");Does this need to bail at this point then?> + } > > if ( live ) > { > diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c > index 0918615..df50ef5 100644 > --- a/tools/libxc/xc_tmem.c > +++ b/tools/libxc/xc_tmem.c > @@ -366,9 +366,9 @@ int xc_tmem_save_extra(xc_interface *xch, int dom, int io_fd, int field_marker) > } > > /* only called for live migration */ > -void xc_tmem_save_done(xc_interface *xch, int dom) > +int xc_tmem_save_done(xc_interface *xch, int dom) > { > - xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL); > + return xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);As you are tweaking this, could you apply some coding style with spaces following commas? ~Andrew> } > > /* restore routines */ > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h > index 4ac6b8a..18d4f78 100644 > --- a/tools/libxc/xenctrl.h > +++ b/tools/libxc/xenctrl.h > @@ -1996,7 +1996,7 @@ int xc_tmem_control(xc_interface *xch, > int xc_tmem_auth(xc_interface *xch, int cli_id, char *uuid_str, int arg1); > int xc_tmem_save(xc_interface *xch, int dom, int live, int fd, int field_marker); > int xc_tmem_save_extra(xc_interface *xch, int dom, int fd, int field_marker); > -void xc_tmem_save_done(xc_interface *xch, int dom); > +int xc_tmem_save_done(xc_interface *xch, int dom); > int xc_tmem_restore(xc_interface *xch, int dom, int fd); > int xc_tmem_restore_extra(xc_interface *xch, int dom, int fd); >
Andrew Cooper
2013-Nov-25 17:16 UTC
Re: [PATCH 3/5] tmem: Check copy_to_user_* return value.
On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote:> We weren''t checking whether that operation fails and > return the proper error. > > This fixes CID 1055125, 105512, 1055127, 1055128, 1055129, > 1055130. > > CC: Bob Liu <bob.liu@oracle.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > xen/common/tmem.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/xen/common/tmem.c b/xen/common/tmem.c > index 081772e..3bc35fd 100644 > --- a/xen/common/tmem.c > +++ b/xen/common/tmem.c > @@ -2146,8 +2146,12 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len, > if ( cli_id == TMEM_CLI_ID_NULL ) { > off = tmemc_list_global(buf,0,len,use_long); > off += tmemc_list_shared(buf,off,len-off,use_long); > - list_for_each_entry(client,&global_client_list,client_list) > - off += tmemc_list_client(client, buf, off, len-off, use_long); > + list_for_each_entry(client,&global_client_list,client_list) {Spaces and commas.> + int ret = tmemc_list_client(client, buf, off, len-off, use_long); > + if ( ret < 0 ) > + return ret; > + off += ret; > + } > off += tmemc_list_global_perf(buf,off,len-off,use_long); > } > else if ( (client = tmem_client_from_cli_id(cli_id)) == NULL) > @@ -2155,6 +2159,8 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len, > else > off = tmemc_list_client(client, buf, 0, len, use_long); > > + if ( off < 0 ) > + return off;This looks to check for an overflow of ''off'', but it is too late. Overflow needs to be checked each time you possibly add more to it. ~Andrew> return 0; > } > > @@ -2319,8 +2325,9 @@ static int tmemc_save_subop(int cli_id, uint32_t pool_id, > case TMEMC_SAVE_GET_POOL_UUID: > if ( pool == NULL ) > break; > - tmem_copy_to_client_buf(buf, pool->uuid, 2); > rc = 0; > + if ( tmem_copy_to_client_buf(buf, pool->uuid, 2) ) > + rc = -EFAULT; > break; > case TMEMC_SAVE_END: > if ( client == NULL ) > @@ -2383,7 +2390,10 @@ static int tmemc_save_get_next_page(int cli_id, uint32_t pool_id, > BUILD_BUG_ON(sizeof(h.oid) != sizeof(oid)); > memcpy(h.oid, oid.oid, sizeof(h.oid)); > h.index = pgp->index; > - tmem_copy_to_client_buf(buf, &h, 1); > + if ( tmem_copy_to_client_buf(buf, &h, 1) ) { > + ret = -EFAULT; > + goto out; > + } > tmem_client_buf_add(buf, sizeof(h)); > ret = do_tmem_get(pool, &oid, pgp->index, 0, 0, 0, pagesize, buf); > > @@ -2427,8 +2437,9 @@ static int tmemc_save_get_next_inv(int cli_id, tmem_cli_va_param_t buf, > BUILD_BUG_ON(sizeof(h.oid) != sizeof(pgp->inv_oid)); > memcpy(h.oid, pgp->inv_oid.oid, sizeof(h.oid)); > h.index = pgp->index; > - tmem_copy_to_client_buf(buf, &h, 1); > ret = 1; > + if ( tmem_copy_to_client_buf(buf, &h, 1) ) > + ret = -EFAULT; > out: > tmem_spin_unlock(&pers_lists_spinlock); > return ret;
Andrew Cooper
2013-Nov-25 17:19 UTC
Re: [PATCH 4/5] tmem: Pointless check of NULL against an array
On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote:> CID 1055632 tells us that we are comparing an array to a NULL > which is indeed silly. Lets not do it. > > CC: Bob Liu <bob.liu@oracle.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > xen/common/tmem.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/xen/common/tmem.c b/xen/common/tmem.c > index 3bc35fd..1ebed8a 100644 > --- a/xen/common/tmem.c > +++ b/xen/common/tmem.c > @@ -1798,8 +1798,6 @@ static int do_tmem_destroy_pool(uint32_t pool_id) > struct client *client = tmem_client_from_current(); > struct tmem_pool *pool; > > - if ( client->pools == NULL ) > - return 0;client->pools certainly can''t be null, but client certainly can. This should be "if ( !client )" ~Andrew> if ( pool_id >= MAX_POOLS_PER_DOMAIN ) > return 0; > if ( (pool = client->pools[pool_id]) == NULL )
Andrew Cooper
2013-Nov-25 17:36 UTC
Re: [PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again).
On 25/11/13 17:06, Ian Campbell wrote:> On Mon, 2013-11-25 at 12:00 -0500, Konrad Rzeszutek Wilk wrote: >> The git commit 1438d36f96e90d1116bebc6b3013634ca21c49c8 >> "xenstat: Fix buffer over-run with new_domains being negative." >> fixed part of the problem, but it failed to do one more check. >> >> That is the if we don''t exit out of the loop if the >> xc_domain_getinfolist returns -1. This makes the check >> by casting the unsigned int value to int as otherwise >> the >> if (new_domains < 0) >> >> would never get executed (as unsigned int won''t be negative). >> >> Fixes CID 1055740. >> >> CC: andrew.cooper3@citrix.com >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> --- >> tools/xenstat/libxenstat/src/xenstat.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c >> index e5facb8..27d8e22 100644 >> --- a/tools/xenstat/libxenstat/src/xenstat.c >> +++ b/tools/xenstat/libxenstat/src/xenstat.c >> @@ -208,7 +208,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags) >> node->num_domains, >> DOMAIN_CHUNK_SIZE, >> domaininfo); >> - if (new_domains < 0) >> + if ((int)new_domains < 0) > I expect that the right fix is to make new_domains the correct (signed) > type, no? > > Ian. >xc_domain_getinfolist() is just as crazy as its partner in crime, xc_domain_getinfo(), and takes an unsigned int "max_doms" and returns a signed number of domains gotten, or -1 for certain failed hypercalls. I might be inclined to introduce a signed rc to hold the return value xc_domain_getinfolist(), check for a negative return before assigning to new_domains and also check for an overflow of node->num_domains + new_domains for the realloc. ~Andrew
Konrad Rzeszutek Wilk
2013-Nov-25 19:51 UTC
Re: [PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again).
On Mon, Nov 25, 2013 at 05:36:29PM +0000, Andrew Cooper wrote:> On 25/11/13 17:06, Ian Campbell wrote: > > On Mon, 2013-11-25 at 12:00 -0500, Konrad Rzeszutek Wilk wrote: > >> The git commit 1438d36f96e90d1116bebc6b3013634ca21c49c8 > >> "xenstat: Fix buffer over-run with new_domains being negative." > >> fixed part of the problem, but it failed to do one more check. > >> > >> That is the if we don''t exit out of the loop if the > >> xc_domain_getinfolist returns -1. This makes the check > >> by casting the unsigned int value to int as otherwise > >> the > >> if (new_domains < 0) > >> > >> would never get executed (as unsigned int won''t be negative). > >> > >> Fixes CID 1055740. > >> > >> CC: andrew.cooper3@citrix.com > >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > >> --- > >> tools/xenstat/libxenstat/src/xenstat.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c > >> index e5facb8..27d8e22 100644 > >> --- a/tools/xenstat/libxenstat/src/xenstat.c > >> +++ b/tools/xenstat/libxenstat/src/xenstat.c > >> @@ -208,7 +208,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags) > >> node->num_domains, > >> DOMAIN_CHUNK_SIZE, > >> domaininfo); > >> - if (new_domains < 0) > >> + if ((int)new_domains < 0) > > I expect that the right fix is to make new_domains the correct (signed) > > type, no?That can be done too.> > > > Ian. > > > > xc_domain_getinfolist() is just as crazy as its partner in crime, > xc_domain_getinfo(), and takes an unsigned int "max_doms" and returns a > signed number of domains gotten, or -1 for certain failed hypercalls. > > I might be inclined to introduce a signed rc to hold the return value > xc_domain_getinfolist(), check for a negative return before assigning to > new_domains and also check for an overflow of node->num_domains + > new_domains for the realloc.Oh, hadn''t thought about the overflow.
Konrad Rzeszutek Wilk
2013-Nov-25 19:53 UTC
Re: [PATCH 2/5] xc/tmem: Unchecked return value.
On Mon, Nov 25, 2013 at 05:11:31PM +0000, Andrew Cooper wrote:> On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote: > > CID 1055045 complains that the return value needs checking. > > > > The tmem op is a flush call and there is no expectation > > right now of any return besides zero. But just in case > > this changes lets blow up. > > > > CC: Bob Liu <bob.liu@oracle.com> > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > tools/libxc/xc_domain_save.c | 6 ++++-- > > tools/libxc/xc_tmem.c | 4 ++-- > > tools/libxc/xenctrl.h | 2 +- > > 3 files changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c > > index 42c4752..c3f0f19 100644 > > --- a/tools/libxc/xc_domain_save.c > > +++ b/tools/libxc/xc_domain_save.c > > @@ -2095,8 +2095,10 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter > > goto copypages; > > } > > > > - if ( tmem_saved != 0 && live ) > > - xc_tmem_save_done(xch, dom); > > + if ( tmem_saved != 0 && live ) { > > + if ( xc_tmem_save_done(xch, dom) ) > > These could be combined into a single if statement. > > > + DPRINTF("Warning - failed to flush tmem on originating server."); > > Does this need to bail at this point then?No. This failing means that the host hypervisor has some pages it couldn''t free. It should never happen (ha!) - but in case it does it is OK to forget about them and just leave the ''zombie'' guest behind.> > > + } > > > > if ( live ) > > { > > diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c > > index 0918615..df50ef5 100644 > > --- a/tools/libxc/xc_tmem.c > > +++ b/tools/libxc/xc_tmem.c > > @@ -366,9 +366,9 @@ int xc_tmem_save_extra(xc_interface *xch, int dom, int io_fd, int field_marker) > > } > > > > /* only called for live migration */ > > -void xc_tmem_save_done(xc_interface *xch, int dom) > > +int xc_tmem_save_done(xc_interface *xch, int dom) > > { > > - xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL); > > + return xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL); > > As you are tweaking this, could you apply some coding style with spaces > following commas?Of course. I thought it was frowend to mix coding style changes with bug-fixes? Or was it more of an follow-up patch that would clean this mess up?> > ~Andrew > > > } > > > > /* restore routines */ > > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h > > index 4ac6b8a..18d4f78 100644 > > --- a/tools/libxc/xenctrl.h > > +++ b/tools/libxc/xenctrl.h > > @@ -1996,7 +1996,7 @@ int xc_tmem_control(xc_interface *xch, > > int xc_tmem_auth(xc_interface *xch, int cli_id, char *uuid_str, int arg1); > > int xc_tmem_save(xc_interface *xch, int dom, int live, int fd, int field_marker); > > int xc_tmem_save_extra(xc_interface *xch, int dom, int fd, int field_marker); > > -void xc_tmem_save_done(xc_interface *xch, int dom); > > +int xc_tmem_save_done(xc_interface *xch, int dom); > > int xc_tmem_restore(xc_interface *xch, int dom, int fd); > > int xc_tmem_restore_extra(xc_interface *xch, int dom, int fd); > > >
Konrad Rzeszutek Wilk
2013-Nov-25 19:54 UTC
Re: [PATCH 4/5] tmem: Pointless check of NULL against an array
On Mon, Nov 25, 2013 at 05:19:32PM +0000, Andrew Cooper wrote:> On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote: > > CID 1055632 tells us that we are comparing an array to a NULL > > which is indeed silly. Lets not do it. > > > > CC: Bob Liu <bob.liu@oracle.com> > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > xen/common/tmem.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/xen/common/tmem.c b/xen/common/tmem.c > > index 3bc35fd..1ebed8a 100644 > > --- a/xen/common/tmem.c > > +++ b/xen/common/tmem.c > > @@ -1798,8 +1798,6 @@ static int do_tmem_destroy_pool(uint32_t pool_id) > > struct client *client = tmem_client_from_current(); > > struct tmem_pool *pool; > > > > - if ( client->pools == NULL ) > > - return 0; > > client->pools certainly can''t be null, but client certainly can. This > should be "if ( !client )"No need. The caller of this function does that already and if there does not exist one - it will create one. I can add an ASSERT here?> > ~Andrew > > > if ( pool_id >= MAX_POOLS_PER_DOMAIN ) > > return 0; > > if ( (pool = client->pools[pool_id]) == NULL ) >
Jan Beulich
2013-Nov-26 08:21 UTC
Re: [PATCH 3/5] tmem: Check copy_to_user_* return value.
>>> On 25.11.13 at 18:00, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:First of all, the title is wrong: You really mean copy_to_guest*().> We weren''t checking whether that operation fails and > return the proper error. > > This fixes CID 1055125, 105512, 1055127, 1055128, 1055129, > 1055130.But if you''re doing something like this, you should fix all instances, not just some. Which would e.g. require tmem_copy_to_client_buf_offset() to propagate the return value of copy_to_guest_offset() (it''s odd anyway that this one is an inline function, while tmem_copy_to_client_buf() is a macro). But then again I''m wondering what baseline your patch uses:> --- a/xen/common/tmem.c > +++ b/xen/common/tmem.c > @@ -2146,8 +2146,12 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len, > if ( cli_id == TMEM_CLI_ID_NULL ) { > off = tmemc_list_global(buf,0,len,use_long); > off += tmemc_list_shared(buf,off,len-off,use_long);This isn''t on line 2146 of today''s staging tree, but on line 2239. Hence looking at the individual changes may not make much sense, as it''s not clear whether there are other dependencies on earlier changes here. Jan
Jan Beulich
2013-Nov-26 08:27 UTC
Re: [PATCH 4/5] tmem: Pointless check of NULL against an array
>>> On 25.11.13 at 20:54, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Mon, Nov 25, 2013 at 05:19:32PM +0000, Andrew Cooper wrote: >> On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote: >> > CID 1055632 tells us that we are comparing an array to a NULL >> > which is indeed silly. Lets not do it. >> > >> > CC: Bob Liu <bob.liu@oracle.com> >> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> > --- >> > xen/common/tmem.c | 2 -- >> > 1 file changed, 2 deletions(-) >> > >> > diff --git a/xen/common/tmem.c b/xen/common/tmem.c >> > index 3bc35fd..1ebed8a 100644 >> > --- a/xen/common/tmem.c >> > +++ b/xen/common/tmem.c >> > @@ -1798,8 +1798,6 @@ static int do_tmem_destroy_pool(uint32_t pool_id) >> > struct client *client = tmem_client_from_current(); >> > struct tmem_pool *pool; >> > >> > - if ( client->pools == NULL ) >> > - return 0; >> >> client->pools certainly can''t be null, but client certainly can. This >> should be "if ( !client )" > > No need. The caller of this function does that already and if there > does not exist one - it will create one. > > I can add an ASSERT here?Yes. Or just pass the already validated "client" into the function. Jan
Ian Campbell
2013-Nov-26 08:40 UTC
Re: [PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again).
On Mon, 2013-11-25 at 17:36 +0000, Andrew Cooper wrote:> xc_domain_getinfolist() is just as crazy as its partner in crime, > xc_domain_getinfo(), and takes an unsigned int "max_doms" and returns a > signed number of domains gotten, or -1 for certain failed hypercalls.Perhaps we should be changing those two to take a signed int max? The domid_t typedef might be a suitable type. For evtchns we have evtchn_port_or_error_t to cover the -1..2^16 space, we could do something similar here for the return type, or just leave it as is.
Konrad Rzeszutek Wilk
2013-Nov-26 18:16 UTC
Re: [PATCH 3/5] tmem: Check copy_to_user_* return value.
On Tue, Nov 26, 2013 at 08:21:09AM +0000, Jan Beulich wrote:> >>> On 25.11.13 at 18:00, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > First of all, the title is wrong: You really mean copy_to_guest*(). > > > We weren''t checking whether that operation fails and > > return the proper error. > > > > This fixes CID 1055125, 105512, 1055127, 1055128, 1055129, > > 1055130. > > But if you''re doing something like this, you should fix all instances, > not just some. Which would e.g. require > tmem_copy_to_client_buf_offset() to propagate the return value > of copy_to_guest_offset() (it''s odd anyway that this one is an > inline function, while tmem_copy_to_client_buf() is a macro).No need. One of the Bobs patches fixes that. (the ones he posted a couple of days ago that - not the ones you pulled).> > But then again I''m wondering what baseline your patch uses:It was on top of the earlier tmem patches. Which is staging, so this should apply nicely on top of that. But I also realize that it has the first of his patches that he had posted this week. That is the tmem: cleanup: drop some debug code patch. Sorry about that - I should have mentioned that in the cover letter and I completely forgot.> > > --- a/xen/common/tmem.c > > +++ b/xen/common/tmem.c > > @@ -2146,8 +2146,12 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len, > > if ( cli_id == TMEM_CLI_ID_NULL ) { > > off = tmemc_list_global(buf,0,len,use_long); > > off += tmemc_list_shared(buf,off,len-off,use_long); > > This isn''t on line 2146 of today''s staging tree, but on line 2239. > > Hence looking at the individual changes may not make much sense, > as it''s not clear whether there are other dependencies on earlier > changes here.<nods>> > Jan >