* [PATCH] fix domain reference leaks
@ 2010-02-09 13:32 Jan Beulich
2010-02-11 21:41 ` Dan Magenheimer
0 siblings, 1 reply; 2+ messages in thread
From: Jan Beulich @ 2010-02-09 13:32 UTC (permalink / raw)
To: xen-devel; +Cc: dan.magenheimer
[-- Attachment #1: Type: text/plain, Size: 18177 bytes --]
Besides two unlikely/rarely hit ones in x86 code, the main offender
was tmh_client_from_cli_id(), which didn't even have a counterpart
(albeit it had a comment correctly saying that it causes d->refcnt to
get incremented). Unfortunately(?) this required a bit of code
restructuring (as I needed to change the code anyway, I also fixed
a couple os missing bounds checks which would sooner or later be
reported as security vulnerabilities), so I would hope Dan could give
it his blessing before it gets applied.
Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
--- 2010-02-09.orig/xen/arch/x86/debug.c 2010-02-09 10:50:02.000000000 +0100
+++ 2010-02-09/xen/arch/x86/debug.c 2010-02-09 09:24:58.000000000 +0100
@@ -252,10 +252,11 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf,
else
len = __copy_from_user(buf, (void *)addr, len);
}
- else
+ else if ( dp )
{
- if ( dp && !dp->is_dying ) /* make sure guest is still there */
+ if ( !dp->is_dying ) /* make sure guest is still there */
len= dbg_rw_guest_mem(addr, buf, len, dp, toaddr, pgd3);
+ put_domain(dp);
}
DBGP2("gmem:exit:len:$%d\n", len);
--- 2010-02-09.orig/xen/arch/x86/mm.c 2010-01-06 11:22:26.000000000 +0100
+++ 2010-02-09/xen/arch/x86/mm.c 2010-02-09 09:39:36.000000000 +0100
@@ -3805,6 +3805,7 @@ int steal_page(
struct domain *d, struct page_info *page, unsigned int memflags)
{
unsigned long x, y;
+ bool_t drop_dom_ref = 0;
spin_lock(&d->page_alloc_lock);
@@ -3832,11 +3833,13 @@ int steal_page(
} while ( (y = cmpxchg(&page->count_info, x, x | 1)) != x );
/* Unlink from original owner. */
- if ( !(memflags & MEMF_no_refcount) )
- d->tot_pages--;
+ if ( !(memflags & MEMF_no_refcount) && !--d->tot_pages )
+ drop_dom_ref = 1;
page_list_del(page, &d->page_list);
spin_unlock(&d->page_alloc_lock);
+ if ( unlikely(drop_dom_ref) )
+ put_domain(d);
return 0;
fail:
--- 2010-02-09.orig/xen/common/tmem.c 2010-02-09 10:50:02.000000000 +0100
+++ 2010-02-09/xen/common/tmem.c 2010-02-09 11:40:21.000000000 +0100
@@ -912,14 +912,14 @@ static client_t *client_create(cli_id_t
return NULL;
}
memset(client,0,sizeof(client_t));
- if ( (client->tmh = tmh_client_init()) == NULL )
+ if ( (client->tmh = tmh_client_init(cli_id)) == NULL )
{
printk("failed... can't allocate host-dependent part of client\n");
if ( client )
tmh_free_infra(client);
return NULL;
}
- tmh_set_client_from_id(client,cli_id);
+ tmh_set_client_from_id(client, client->tmh, cli_id);
client->cli_id = cli_id;
#ifdef __i386__
client->compress = 0;
@@ -1528,7 +1528,7 @@ static NOINLINE int do_tmem_destroy_pool
}
static NOINLINE int do_tmem_new_pool(cli_id_t this_cli_id,
- uint32_t this_pool_id, uint32_t flags,
+ uint32_t d_poolid, uint32_t flags,
uint64_t uuid_lo, uint64_t uuid_hi)
{
client_t *client;
@@ -1540,19 +1540,13 @@ static NOINLINE int do_tmem_new_pool(cli
int specversion = (flags >> TMEM_POOL_VERSION_SHIFT)
& TMEM_POOL_VERSION_MASK;
pool_t *pool, *shpool;
- int s_poolid, d_poolid, first_unused_s_poolid;
+ int s_poolid, first_unused_s_poolid;
int i;
if ( this_cli_id == CLI_ID_NULL )
- {
- client = tmh_client_from_current();
cli_id = tmh_get_cli_id_from_current();
- } else {
- if ( (client = tmh_client_from_cli_id(this_cli_id)) == NULL)
- return -EPERM;
+ else
cli_id = this_cli_id;
- }
- ASSERT(client != NULL);
printk("tmem: allocating %s-%s tmem pool for %s=%d...",
persistent ? "persistent" : "ephemeral" ,
shared ? "shared" : "private", cli_id_str, cli_id);
@@ -1573,19 +1567,24 @@ static NOINLINE int do_tmem_new_pool(cli
}
if ( this_cli_id != CLI_ID_NULL )
{
- d_poolid = this_pool_id;
- if ( client->pools[d_poolid] != NULL )
- return -EPERM;
- d_poolid = this_pool_id;
+ if ( (client = tmh_client_from_cli_id(this_cli_id)) == NULL
+ || d_poolid >= MAX_POOLS_PER_DOMAIN
+ || client->pools[d_poolid] != NULL )
+ goto fail;
}
- else for ( d_poolid = 0; d_poolid < MAX_POOLS_PER_DOMAIN; d_poolid++ )
- if ( client->pools[d_poolid] == NULL )
- break;
- if ( d_poolid >= MAX_POOLS_PER_DOMAIN )
+ else
{
- printk("failed... no more pool slots available for this %s\n",
- client_str);
- goto fail;
+ client = tmh_client_from_current();
+ ASSERT(client != NULL);
+ for ( d_poolid = 0; d_poolid < MAX_POOLS_PER_DOMAIN; d_poolid++ )
+ if ( client->pools[d_poolid] == NULL )
+ break;
+ if ( d_poolid >= MAX_POOLS_PER_DOMAIN )
+ {
+ printk("failed... no more pool slots available for this %s\n",
+ client_str);
+ goto fail;
+ }
}
if ( shared )
{
@@ -1618,6 +1617,8 @@ static NOINLINE int do_tmem_new_pool(cli
client->pools[d_poolid] = global_shared_pools[s_poolid];
shared_pool_join(global_shared_pools[s_poolid], client);
pool_free(pool);
+ if ( this_cli_id != CLI_ID_NULL )
+ tmh_client_put(client->tmh);
return d_poolid;
}
}
@@ -1638,6 +1639,8 @@ static NOINLINE int do_tmem_new_pool(cli
}
}
client->pools[d_poolid] = pool;
+ if ( this_cli_id != CLI_ID_NULL )
+ tmh_client_put(client->tmh);
list_add_tail(&pool->pool_list, &global_pool_list);
pool->pool_id = d_poolid;
pool->persistent = persistent;
@@ -1647,6 +1650,8 @@ static NOINLINE int do_tmem_new_pool(cli
fail:
pool_free(pool);
+ if ( this_cli_id != CLI_ID_NULL )
+ tmh_client_put(client->tmh);
return -EPERM;
}
@@ -1672,6 +1677,7 @@ static int tmemc_freeze_pools(cli_id_t c
if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
return -1;
client_freeze(client,freeze);
+ tmh_client_put(client->tmh);
printk("tmem: all pools %s for %s=%d\n",s,cli_id_str,cli_id);
}
return 0;
@@ -1876,8 +1882,10 @@ static int tmemc_list(cli_id_t cli_id, t
}
else if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
return -1;
- else
+ else {
off = tmemc_list_client(client, buf, 0, len, use_long);
+ tmh_client_put(client->tmh);
+ }
return 0;
}
@@ -1925,7 +1933,10 @@ static int tmemc_set_var(cli_id_t cli_id
else if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
return -1;
else
- tmemc_set_var_one(client, subop, arg1);
+ {
+ tmemc_set_var_one(client, subop, arg1);
+ tmh_client_put(client->tmh);
+ }
return 0;
}
@@ -1941,6 +1952,8 @@ static NOINLINE int tmemc_shared_pool_au
return 1;
}
client = tmh_client_from_cli_id(cli_id);
+ if ( client == NULL )
+ return -EINVAL;
for ( i = 0; i < MAX_GLOBAL_SHARED_POOLS; i++)
{
if ( (client->shared_auth_uuid[i][0] == uuid_lo) &&
@@ -1949,6 +1962,7 @@ static NOINLINE int tmemc_shared_pool_au
if ( auth == 0 )
client->shared_auth_uuid[i][0] =
client->shared_auth_uuid[i][1] = -1L;
+ tmh_client_put(client->tmh);
return 1;
}
if ( (auth == 1) && (client->shared_auth_uuid[i][0] == -1L) &&
@@ -1956,11 +1970,15 @@ static NOINLINE int tmemc_shared_pool_au
free = i;
}
if ( auth == 0 )
+ {
+ tmh_client_put(client->tmh);
return 0;
+ }
if ( auth == 1 && free == -1 )
return -ENOMEM;
client->shared_auth_uuid[free][0] = uuid_lo;
client->shared_auth_uuid[free][1] = uuid_hi;
+ tmh_client_put(client->tmh);
return 1;
}
@@ -1968,10 +1986,12 @@ static NOINLINE int tmemc_save_subop(int
uint32_t subop, tmem_cli_va_t buf, uint32_t arg1)
{
client_t *client = tmh_client_from_cli_id(cli_id);
- pool_t *pool = (client == NULL) ? NULL : client->pools[pool_id];
+ pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
+ ? NULL : client->pools[pool_id];
uint32_t p;
uint64_t *uuid;
pgp_t *pgp, *pgp2;
+ int rc = -1;
switch(subop)
{
@@ -1982,45 +2002,55 @@ static NOINLINE int tmemc_save_subop(int
if ( client->pools[p] != NULL )
break;
if ( p == MAX_POOLS_PER_DOMAIN )
- return 0;
+ {
+ rc = 0;
+ break;
+ }
client->was_frozen = client->frozen;
client->frozen = 1;
if ( arg1 != 0 )
client->live_migrating = 1;
- return 1;
+ rc = 1;
+ break;
case TMEMC_RESTORE_BEGIN:
- ASSERT(client == NULL);
- if ( (client = client_create(cli_id)) == NULL )
- return -1;
- return 1;
+ if ( client == NULL && (client = client_create(cli_id)) != NULL )
+ return 1;
+ break;
case TMEMC_SAVE_GET_VERSION:
- return TMEM_SPEC_VERSION;
+ rc = TMEM_SPEC_VERSION;
+ break;
case TMEMC_SAVE_GET_MAXPOOLS:
- return MAX_POOLS_PER_DOMAIN;
+ rc = MAX_POOLS_PER_DOMAIN;
+ break;
case TMEMC_SAVE_GET_CLIENT_WEIGHT:
- return client->weight == -1 ? -2 : client->weight;
+ rc = client->weight == -1 ? -2 : client->weight;
+ break;
case TMEMC_SAVE_GET_CLIENT_CAP:
- return client->cap == -1 ? -2 : client->cap;
+ rc = client->cap == -1 ? -2 : client->cap;
+ break;
case TMEMC_SAVE_GET_CLIENT_FLAGS:
- return (client->compress ? TMEM_CLIENT_COMPRESS : 0 ) |
- (client->was_frozen ? TMEM_CLIENT_FROZEN : 0 );
+ rc = (client->compress ? TMEM_CLIENT_COMPRESS : 0 ) |
+ (client->was_frozen ? TMEM_CLIENT_FROZEN : 0 );
+ break;
case TMEMC_SAVE_GET_POOL_FLAGS:
if ( pool == NULL )
- return -1;
- return (pool->persistent ? TMEM_POOL_PERSIST : 0) |
- (pool->shared ? TMEM_POOL_SHARED : 0) |
- (pool->pageshift << TMEM_POOL_PAGESIZE_SHIFT);
+ break;
+ rc = (pool->persistent ? TMEM_POOL_PERSIST : 0) |
+ (pool->shared ? TMEM_POOL_SHARED : 0) |
+ (pool->pageshift << TMEM_POOL_PAGESIZE_SHIFT);
+ break;
case TMEMC_SAVE_GET_POOL_NPAGES:
if ( pool == NULL )
- return -1;
- return _atomic_read(pool->pgp_count);
+ break;
+ rc = _atomic_read(pool->pgp_count);
+ break;
case TMEMC_SAVE_GET_POOL_UUID:
if ( pool == NULL )
- return -1;
+ break;
uuid = (uint64_t *)buf.p;
*uuid++ = pool->uuid[0];
*uuid = pool->uuid[1];
- return 0;
+ rc = 0;
case TMEMC_SAVE_END:
client->live_migrating = 0;
if ( !list_empty(&client->persistent_invalidated_list) )
@@ -2028,27 +2058,34 @@ static NOINLINE int tmemc_save_subop(int
&client->persistent_invalidated_list, client_inv_pages)
pgp_free_from_inv_list(client,pgp);
client->frozen = client->was_frozen;
- return 0;
+ rc = 0;
}
- return -1;
+ if ( client )
+ tmh_client_put(client->tmh);
+ return rc;
}
static NOINLINE int tmemc_save_get_next_page(int cli_id, int pool_id,
tmem_cli_va_t buf, uint32_t bufsize)
{
client_t *client = tmh_client_from_cli_id(cli_id);
- pool_t *pool = (client == NULL) ? NULL : client->pools[pool_id];
+ pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
+ ? NULL : client->pools[pool_id];
pgp_t *pgp;
int ret = 0;
struct tmem_handle *h;
unsigned int pagesize = 1 << (pool->pageshift+12);
- if ( pool == NULL )
- return -1;
- if ( is_ephemeral(pool) )
+ if ( pool == NULL || is_ephemeral(pool) )
+ {
+ tmh_client_put(client->tmh);
return -1;
+ }
if ( bufsize < pagesize + sizeof(struct tmem_handle) )
+ {
+ tmh_client_put(client->tmh);
return -ENOMEM;
+ }
tmem_spin_lock(&pers_lists_spinlock);
if ( list_empty(&pool->persistent_page_list) )
@@ -2080,6 +2117,7 @@ static NOINLINE int tmemc_save_get_next_
out:
tmem_spin_unlock(&pers_lists_spinlock);
+ tmh_client_put(client->tmh);
return ret;
}
@@ -2094,7 +2132,10 @@ static NOINLINE int tmemc_save_get_next_
if ( client == NULL )
return 0;
if ( bufsize < sizeof(struct tmem_handle) )
+ {
+ tmh_client_put(client->tmh);
return 0;
+ }
tmem_spin_lock(&pers_lists_spinlock);
if ( list_empty(&client->persistent_invalidated_list) )
goto out;
@@ -2121,6 +2162,7 @@ static NOINLINE int tmemc_save_get_next_
ret = 1;
out:
tmem_spin_unlock(&pers_lists_spinlock);
+ tmh_client_put(client->tmh);
return ret;
}
@@ -2128,22 +2170,26 @@ static int tmemc_restore_put_page(int cl
uint32_t index, tmem_cli_va_t buf, uint32_t bufsize)
{
client_t *client = tmh_client_from_cli_id(cli_id);
- pool_t *pool = (client == NULL) ? NULL : client->pools[pool_id];
+ pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
+ ? NULL : client->pools[pool_id];
+ int rc = pool ? do_tmem_put(pool,oid,index,0,0,0,bufsize,buf.p) : -1;
- if ( pool == NULL )
- return -1;
- return do_tmem_put(pool,oid,index,0,0,0,bufsize,buf.p);
+ if ( client )
+ tmh_client_put(client->tmh);
+ return rc;
}
static int tmemc_restore_flush_page(int cli_id, int pool_id, uint64_t oid,
uint32_t index)
{
client_t *client = tmh_client_from_cli_id(cli_id);
- pool_t *pool = (client == NULL) ? NULL : client->pools[pool_id];
+ pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
+ ? NULL : client->pools[pool_id];
+ int rc = pool ? do_tmem_flush_page(pool, oid, index) : -1;
- if ( pool == NULL )
- return -1;
- return do_tmem_flush_page(pool, oid, index);
+ if ( client )
+ tmh_client_put(client->tmh);
+ return rc;
}
static NOINLINE int do_tmem_control(struct tmem_op *op)
--- 2010-02-09.orig/xen/common/tmem_xen.c 2010-02-09 10:50:02.000000000 +0100
+++ 2010-02-09/xen/common/tmem_xen.c 2010-02-09 10:03:16.000000000 +0100
@@ -290,17 +290,16 @@ static void tmh_persistent_pool_page_put
/****************** XEN-SPECIFIC CLIENT HANDLING ********************/
-EXPORT tmh_client_t *tmh_client_init(void)
+EXPORT tmh_client_t *tmh_client_init(cli_id_t cli_id)
{
tmh_client_t *tmh;
char name[5];
- domid_t domid = current->domain->domain_id;
int i, shift;
if ( (tmh = xmalloc(tmh_client_t)) == NULL )
return NULL;
for (i = 0, shift = 12; i < 4; shift -=4, i++)
- name[i] = (((unsigned short)domid >> shift) & 0xf) + '0';
+ name[i] = (((unsigned short)cli_id >> shift) & 0xf) + '0';
name[4] = '\0';
#ifndef __i386__
tmh->persistent_pool = xmem_pool_create(name, tmh_persistent_pool_page_get,
@@ -311,7 +310,6 @@ EXPORT tmh_client_t *tmh_client_init(voi
return NULL;
}
#endif
- tmh->domain = current->domain;
return tmh;
}
@@ -321,6 +319,7 @@ EXPORT void tmh_client_destroy(tmh_clien
xmem_pool_destroy(tmh->persistent_pool);
#endif
put_domain(tmh->domain);
+ tmh->domain = NULL;
}
/****************** XEN-SPECIFIC HOST INITIALIZATION ********************/
--- 2010-02-09.orig/xen/include/xen/tmem_xen.h 2010-02-09 10:50:02.000000000 +0100
+++ 2010-02-09/xen/include/xen/tmem_xen.h 2010-02-09 11:39:13.000000000 +0100
@@ -43,8 +43,6 @@ extern rwlock_t tmem_rwlock;
extern void tmh_copy_page(char *to, char*from);
extern int tmh_init(void);
-extern tmh_client_t *tmh_client_init(void);
-extern void tmh_client_destroy(tmh_client_t *);
#define tmh_hash hash_long
extern void tmh_release_avail_pages_to_host(void);
@@ -281,6 +279,9 @@ typedef domid_t cli_id_t;
typedef struct domain tmh_cli_ptr_t;
typedef struct page_info pfp_t;
+extern tmh_client_t *tmh_client_init(cli_id_t);
+extern void tmh_client_destroy(tmh_client_t *);
+
/* this appears to be unreliable when a domain is being shut down */
static inline struct client *tmh_client_from_cli_id(cli_id_t cli_id)
{
@@ -290,6 +291,11 @@ static inline struct client *tmh_client_
return (struct client *)(d->tmem);
}
+static inline void tmh_client_put(tmh_client_t *tmh)
+{
+ put_domain(tmh->domain);
+}
+
static inline struct client *tmh_client_from_current(void)
{
return (struct client *)(current->domain->tmem);
@@ -307,10 +313,12 @@ static inline tmh_cli_ptr_t *tmh_get_cli
return current->domain;
}
-static inline void tmh_set_client_from_id(struct client *client,cli_id_t cli_id)
+static inline void tmh_set_client_from_id(struct client *client,
+ tmh_client_t *tmh, cli_id_t cli_id)
{
struct domain *d = get_domain_by_id(cli_id);
d->tmem = client;
+ tmh->domain = d;
}
static inline bool_t tmh_current_is_privileged(void)
[-- Attachment #2: put-domain.patch --]
[-- Type: text/plain, Size: 18173 bytes --]
Besides two unlikely/rarely hit ones in x86 code, the main offender
was tmh_client_from_cli_id(), which didn't even have a counterpart
(albeit it had a comment correctly saying that it causes d->refcnt to
get incremented). Unfortunately(?) this required a bit of code
restructuring (as I needed to change the code anyway, I also fixed
a couple os missing bounds checks which would sooner or later be
reported as security vulnerabilities), so I would hope Dan could give
it his blessing before it gets applied.
Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
--- 2010-02-09.orig/xen/arch/x86/debug.c 2010-02-09 10:50:02.000000000 +0100
+++ 2010-02-09/xen/arch/x86/debug.c 2010-02-09 09:24:58.000000000 +0100
@@ -252,10 +252,11 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf,
else
len = __copy_from_user(buf, (void *)addr, len);
}
- else
+ else if ( dp )
{
- if ( dp && !dp->is_dying ) /* make sure guest is still there */
+ if ( !dp->is_dying ) /* make sure guest is still there */
len= dbg_rw_guest_mem(addr, buf, len, dp, toaddr, pgd3);
+ put_domain(dp);
}
DBGP2("gmem:exit:len:$%d\n", len);
--- 2010-02-09.orig/xen/arch/x86/mm.c 2010-01-06 11:22:26.000000000 +0100
+++ 2010-02-09/xen/arch/x86/mm.c 2010-02-09 09:39:36.000000000 +0100
@@ -3805,6 +3805,7 @@ int steal_page(
struct domain *d, struct page_info *page, unsigned int memflags)
{
unsigned long x, y;
+ bool_t drop_dom_ref = 0;
spin_lock(&d->page_alloc_lock);
@@ -3832,11 +3833,13 @@ int steal_page(
} while ( (y = cmpxchg(&page->count_info, x, x | 1)) != x );
/* Unlink from original owner. */
- if ( !(memflags & MEMF_no_refcount) )
- d->tot_pages--;
+ if ( !(memflags & MEMF_no_refcount) && !--d->tot_pages )
+ drop_dom_ref = 1;
page_list_del(page, &d->page_list);
spin_unlock(&d->page_alloc_lock);
+ if ( unlikely(drop_dom_ref) )
+ put_domain(d);
return 0;
fail:
--- 2010-02-09.orig/xen/common/tmem.c 2010-02-09 10:50:02.000000000 +0100
+++ 2010-02-09/xen/common/tmem.c 2010-02-09 11:40:21.000000000 +0100
@@ -912,14 +912,14 @@ static client_t *client_create(cli_id_t
return NULL;
}
memset(client,0,sizeof(client_t));
- if ( (client->tmh = tmh_client_init()) == NULL )
+ if ( (client->tmh = tmh_client_init(cli_id)) == NULL )
{
printk("failed... can't allocate host-dependent part of client\n");
if ( client )
tmh_free_infra(client);
return NULL;
}
- tmh_set_client_from_id(client,cli_id);
+ tmh_set_client_from_id(client, client->tmh, cli_id);
client->cli_id = cli_id;
#ifdef __i386__
client->compress = 0;
@@ -1528,7 +1528,7 @@ static NOINLINE int do_tmem_destroy_pool
}
static NOINLINE int do_tmem_new_pool(cli_id_t this_cli_id,
- uint32_t this_pool_id, uint32_t flags,
+ uint32_t d_poolid, uint32_t flags,
uint64_t uuid_lo, uint64_t uuid_hi)
{
client_t *client;
@@ -1540,19 +1540,13 @@ static NOINLINE int do_tmem_new_pool(cli
int specversion = (flags >> TMEM_POOL_VERSION_SHIFT)
& TMEM_POOL_VERSION_MASK;
pool_t *pool, *shpool;
- int s_poolid, d_poolid, first_unused_s_poolid;
+ int s_poolid, first_unused_s_poolid;
int i;
if ( this_cli_id == CLI_ID_NULL )
- {
- client = tmh_client_from_current();
cli_id = tmh_get_cli_id_from_current();
- } else {
- if ( (client = tmh_client_from_cli_id(this_cli_id)) == NULL)
- return -EPERM;
+ else
cli_id = this_cli_id;
- }
- ASSERT(client != NULL);
printk("tmem: allocating %s-%s tmem pool for %s=%d...",
persistent ? "persistent" : "ephemeral" ,
shared ? "shared" : "private", cli_id_str, cli_id);
@@ -1573,19 +1567,24 @@ static NOINLINE int do_tmem_new_pool(cli
}
if ( this_cli_id != CLI_ID_NULL )
{
- d_poolid = this_pool_id;
- if ( client->pools[d_poolid] != NULL )
- return -EPERM;
- d_poolid = this_pool_id;
+ if ( (client = tmh_client_from_cli_id(this_cli_id)) == NULL
+ || d_poolid >= MAX_POOLS_PER_DOMAIN
+ || client->pools[d_poolid] != NULL )
+ goto fail;
}
- else for ( d_poolid = 0; d_poolid < MAX_POOLS_PER_DOMAIN; d_poolid++ )
- if ( client->pools[d_poolid] == NULL )
- break;
- if ( d_poolid >= MAX_POOLS_PER_DOMAIN )
+ else
{
- printk("failed... no more pool slots available for this %s\n",
- client_str);
- goto fail;
+ client = tmh_client_from_current();
+ ASSERT(client != NULL);
+ for ( d_poolid = 0; d_poolid < MAX_POOLS_PER_DOMAIN; d_poolid++ )
+ if ( client->pools[d_poolid] == NULL )
+ break;
+ if ( d_poolid >= MAX_POOLS_PER_DOMAIN )
+ {
+ printk("failed... no more pool slots available for this %s\n",
+ client_str);
+ goto fail;
+ }
}
if ( shared )
{
@@ -1618,6 +1617,8 @@ static NOINLINE int do_tmem_new_pool(cli
client->pools[d_poolid] = global_shared_pools[s_poolid];
shared_pool_join(global_shared_pools[s_poolid], client);
pool_free(pool);
+ if ( this_cli_id != CLI_ID_NULL )
+ tmh_client_put(client->tmh);
return d_poolid;
}
}
@@ -1638,6 +1639,8 @@ static NOINLINE int do_tmem_new_pool(cli
}
}
client->pools[d_poolid] = pool;
+ if ( this_cli_id != CLI_ID_NULL )
+ tmh_client_put(client->tmh);
list_add_tail(&pool->pool_list, &global_pool_list);
pool->pool_id = d_poolid;
pool->persistent = persistent;
@@ -1647,6 +1650,8 @@ static NOINLINE int do_tmem_new_pool(cli
fail:
pool_free(pool);
+ if ( this_cli_id != CLI_ID_NULL )
+ tmh_client_put(client->tmh);
return -EPERM;
}
@@ -1672,6 +1677,7 @@ static int tmemc_freeze_pools(cli_id_t c
if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
return -1;
client_freeze(client,freeze);
+ tmh_client_put(client->tmh);
printk("tmem: all pools %s for %s=%d\n",s,cli_id_str,cli_id);
}
return 0;
@@ -1876,8 +1882,10 @@ static int tmemc_list(cli_id_t cli_id, t
}
else if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
return -1;
- else
+ else {
off = tmemc_list_client(client, buf, 0, len, use_long);
+ tmh_client_put(client->tmh);
+ }
return 0;
}
@@ -1925,7 +1933,10 @@ static int tmemc_set_var(cli_id_t cli_id
else if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
return -1;
else
- tmemc_set_var_one(client, subop, arg1);
+ {
+ tmemc_set_var_one(client, subop, arg1);
+ tmh_client_put(client->tmh);
+ }
return 0;
}
@@ -1941,6 +1952,8 @@ static NOINLINE int tmemc_shared_pool_au
return 1;
}
client = tmh_client_from_cli_id(cli_id);
+ if ( client == NULL )
+ return -EINVAL;
for ( i = 0; i < MAX_GLOBAL_SHARED_POOLS; i++)
{
if ( (client->shared_auth_uuid[i][0] == uuid_lo) &&
@@ -1949,6 +1962,7 @@ static NOINLINE int tmemc_shared_pool_au
if ( auth == 0 )
client->shared_auth_uuid[i][0] =
client->shared_auth_uuid[i][1] = -1L;
+ tmh_client_put(client->tmh);
return 1;
}
if ( (auth == 1) && (client->shared_auth_uuid[i][0] == -1L) &&
@@ -1956,11 +1970,15 @@ static NOINLINE int tmemc_shared_pool_au
free = i;
}
if ( auth == 0 )
+ {
+ tmh_client_put(client->tmh);
return 0;
+ }
if ( auth == 1 && free == -1 )
return -ENOMEM;
client->shared_auth_uuid[free][0] = uuid_lo;
client->shared_auth_uuid[free][1] = uuid_hi;
+ tmh_client_put(client->tmh);
return 1;
}
@@ -1968,10 +1986,12 @@ static NOINLINE int tmemc_save_subop(int
uint32_t subop, tmem_cli_va_t buf, uint32_t arg1)
{
client_t *client = tmh_client_from_cli_id(cli_id);
- pool_t *pool = (client == NULL) ? NULL : client->pools[pool_id];
+ pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
+ ? NULL : client->pools[pool_id];
uint32_t p;
uint64_t *uuid;
pgp_t *pgp, *pgp2;
+ int rc = -1;
switch(subop)
{
@@ -1982,45 +2002,55 @@ static NOINLINE int tmemc_save_subop(int
if ( client->pools[p] != NULL )
break;
if ( p == MAX_POOLS_PER_DOMAIN )
- return 0;
+ {
+ rc = 0;
+ break;
+ }
client->was_frozen = client->frozen;
client->frozen = 1;
if ( arg1 != 0 )
client->live_migrating = 1;
- return 1;
+ rc = 1;
+ break;
case TMEMC_RESTORE_BEGIN:
- ASSERT(client == NULL);
- if ( (client = client_create(cli_id)) == NULL )
- return -1;
- return 1;
+ if ( client == NULL && (client = client_create(cli_id)) != NULL )
+ return 1;
+ break;
case TMEMC_SAVE_GET_VERSION:
- return TMEM_SPEC_VERSION;
+ rc = TMEM_SPEC_VERSION;
+ break;
case TMEMC_SAVE_GET_MAXPOOLS:
- return MAX_POOLS_PER_DOMAIN;
+ rc = MAX_POOLS_PER_DOMAIN;
+ break;
case TMEMC_SAVE_GET_CLIENT_WEIGHT:
- return client->weight == -1 ? -2 : client->weight;
+ rc = client->weight == -1 ? -2 : client->weight;
+ break;
case TMEMC_SAVE_GET_CLIENT_CAP:
- return client->cap == -1 ? -2 : client->cap;
+ rc = client->cap == -1 ? -2 : client->cap;
+ break;
case TMEMC_SAVE_GET_CLIENT_FLAGS:
- return (client->compress ? TMEM_CLIENT_COMPRESS : 0 ) |
- (client->was_frozen ? TMEM_CLIENT_FROZEN : 0 );
+ rc = (client->compress ? TMEM_CLIENT_COMPRESS : 0 ) |
+ (client->was_frozen ? TMEM_CLIENT_FROZEN : 0 );
+ break;
case TMEMC_SAVE_GET_POOL_FLAGS:
if ( pool == NULL )
- return -1;
- return (pool->persistent ? TMEM_POOL_PERSIST : 0) |
- (pool->shared ? TMEM_POOL_SHARED : 0) |
- (pool->pageshift << TMEM_POOL_PAGESIZE_SHIFT);
+ break;
+ rc = (pool->persistent ? TMEM_POOL_PERSIST : 0) |
+ (pool->shared ? TMEM_POOL_SHARED : 0) |
+ (pool->pageshift << TMEM_POOL_PAGESIZE_SHIFT);
+ break;
case TMEMC_SAVE_GET_POOL_NPAGES:
if ( pool == NULL )
- return -1;
- return _atomic_read(pool->pgp_count);
+ break;
+ rc = _atomic_read(pool->pgp_count);
+ break;
case TMEMC_SAVE_GET_POOL_UUID:
if ( pool == NULL )
- return -1;
+ break;
uuid = (uint64_t *)buf.p;
*uuid++ = pool->uuid[0];
*uuid = pool->uuid[1];
- return 0;
+ rc = 0;
case TMEMC_SAVE_END:
client->live_migrating = 0;
if ( !list_empty(&client->persistent_invalidated_list) )
@@ -2028,27 +2058,34 @@ static NOINLINE int tmemc_save_subop(int
&client->persistent_invalidated_list, client_inv_pages)
pgp_free_from_inv_list(client,pgp);
client->frozen = client->was_frozen;
- return 0;
+ rc = 0;
}
- return -1;
+ if ( client )
+ tmh_client_put(client->tmh);
+ return rc;
}
static NOINLINE int tmemc_save_get_next_page(int cli_id, int pool_id,
tmem_cli_va_t buf, uint32_t bufsize)
{
client_t *client = tmh_client_from_cli_id(cli_id);
- pool_t *pool = (client == NULL) ? NULL : client->pools[pool_id];
+ pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
+ ? NULL : client->pools[pool_id];
pgp_t *pgp;
int ret = 0;
struct tmem_handle *h;
unsigned int pagesize = 1 << (pool->pageshift+12);
- if ( pool == NULL )
- return -1;
- if ( is_ephemeral(pool) )
+ if ( pool == NULL || is_ephemeral(pool) )
+ {
+ tmh_client_put(client->tmh);
return -1;
+ }
if ( bufsize < pagesize + sizeof(struct tmem_handle) )
+ {
+ tmh_client_put(client->tmh);
return -ENOMEM;
+ }
tmem_spin_lock(&pers_lists_spinlock);
if ( list_empty(&pool->persistent_page_list) )
@@ -2080,6 +2117,7 @@ static NOINLINE int tmemc_save_get_next_
out:
tmem_spin_unlock(&pers_lists_spinlock);
+ tmh_client_put(client->tmh);
return ret;
}
@@ -2094,7 +2132,10 @@ static NOINLINE int tmemc_save_get_next_
if ( client == NULL )
return 0;
if ( bufsize < sizeof(struct tmem_handle) )
+ {
+ tmh_client_put(client->tmh);
return 0;
+ }
tmem_spin_lock(&pers_lists_spinlock);
if ( list_empty(&client->persistent_invalidated_list) )
goto out;
@@ -2121,6 +2162,7 @@ static NOINLINE int tmemc_save_get_next_
ret = 1;
out:
tmem_spin_unlock(&pers_lists_spinlock);
+ tmh_client_put(client->tmh);
return ret;
}
@@ -2128,22 +2170,26 @@ static int tmemc_restore_put_page(int cl
uint32_t index, tmem_cli_va_t buf, uint32_t bufsize)
{
client_t *client = tmh_client_from_cli_id(cli_id);
- pool_t *pool = (client == NULL) ? NULL : client->pools[pool_id];
+ pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
+ ? NULL : client->pools[pool_id];
+ int rc = pool ? do_tmem_put(pool,oid,index,0,0,0,bufsize,buf.p) : -1;
- if ( pool == NULL )
- return -1;
- return do_tmem_put(pool,oid,index,0,0,0,bufsize,buf.p);
+ if ( client )
+ tmh_client_put(client->tmh);
+ return rc;
}
static int tmemc_restore_flush_page(int cli_id, int pool_id, uint64_t oid,
uint32_t index)
{
client_t *client = tmh_client_from_cli_id(cli_id);
- pool_t *pool = (client == NULL) ? NULL : client->pools[pool_id];
+ pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
+ ? NULL : client->pools[pool_id];
+ int rc = pool ? do_tmem_flush_page(pool, oid, index) : -1;
- if ( pool == NULL )
- return -1;
- return do_tmem_flush_page(pool, oid, index);
+ if ( client )
+ tmh_client_put(client->tmh);
+ return rc;
}
static NOINLINE int do_tmem_control(struct tmem_op *op)
--- 2010-02-09.orig/xen/common/tmem_xen.c 2010-02-09 10:50:02.000000000 +0100
+++ 2010-02-09/xen/common/tmem_xen.c 2010-02-09 10:03:16.000000000 +0100
@@ -290,17 +290,16 @@ static void tmh_persistent_pool_page_put
/****************** XEN-SPECIFIC CLIENT HANDLING ********************/
-EXPORT tmh_client_t *tmh_client_init(void)
+EXPORT tmh_client_t *tmh_client_init(cli_id_t cli_id)
{
tmh_client_t *tmh;
char name[5];
- domid_t domid = current->domain->domain_id;
int i, shift;
if ( (tmh = xmalloc(tmh_client_t)) == NULL )
return NULL;
for (i = 0, shift = 12; i < 4; shift -=4, i++)
- name[i] = (((unsigned short)domid >> shift) & 0xf) + '0';
+ name[i] = (((unsigned short)cli_id >> shift) & 0xf) + '0';
name[4] = '\0';
#ifndef __i386__
tmh->persistent_pool = xmem_pool_create(name, tmh_persistent_pool_page_get,
@@ -311,7 +310,6 @@ EXPORT tmh_client_t *tmh_client_init(voi
return NULL;
}
#endif
- tmh->domain = current->domain;
return tmh;
}
@@ -321,6 +319,7 @@ EXPORT void tmh_client_destroy(tmh_clien
xmem_pool_destroy(tmh->persistent_pool);
#endif
put_domain(tmh->domain);
+ tmh->domain = NULL;
}
/****************** XEN-SPECIFIC HOST INITIALIZATION ********************/
--- 2010-02-09.orig/xen/include/xen/tmem_xen.h 2010-02-09 10:50:02.000000000 +0100
+++ 2010-02-09/xen/include/xen/tmem_xen.h 2010-02-09 11:39:13.000000000 +0100
@@ -43,8 +43,6 @@ extern rwlock_t tmem_rwlock;
extern void tmh_copy_page(char *to, char*from);
extern int tmh_init(void);
-extern tmh_client_t *tmh_client_init(void);
-extern void tmh_client_destroy(tmh_client_t *);
#define tmh_hash hash_long
extern void tmh_release_avail_pages_to_host(void);
@@ -281,6 +279,9 @@ typedef domid_t cli_id_t;
typedef struct domain tmh_cli_ptr_t;
typedef struct page_info pfp_t;
+extern tmh_client_t *tmh_client_init(cli_id_t);
+extern void tmh_client_destroy(tmh_client_t *);
+
/* this appears to be unreliable when a domain is being shut down */
static inline struct client *tmh_client_from_cli_id(cli_id_t cli_id)
{
@@ -290,6 +291,11 @@ static inline struct client *tmh_client_
return (struct client *)(d->tmem);
}
+static inline void tmh_client_put(tmh_client_t *tmh)
+{
+ put_domain(tmh->domain);
+}
+
static inline struct client *tmh_client_from_current(void)
{
return (struct client *)(current->domain->tmem);
@@ -307,10 +313,12 @@ static inline tmh_cli_ptr_t *tmh_get_cli
return current->domain;
}
-static inline void tmh_set_client_from_id(struct client *client,cli_id_t cli_id)
+static inline void tmh_set_client_from_id(struct client *client,
+ tmh_client_t *tmh, cli_id_t cli_id)
{
struct domain *d = get_domain_by_id(cli_id);
d->tmem = client;
+ tmh->domain = d;
}
static inline bool_t tmh_current_is_privileged(void)
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
* RE: [PATCH] fix domain reference leaks
2010-02-09 13:32 [PATCH] fix domain reference leaks Jan Beulich
@ 2010-02-11 21:41 ` Dan Magenheimer
0 siblings, 0 replies; 2+ messages in thread
From: Dan Magenheimer @ 2010-02-11 21:41 UTC (permalink / raw)
To: Jan Beulich, xen-devel
It has my blessing (though I see Keir already applied it :-)
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@novell.com]
> Sent: Tuesday, February 09, 2010 6:33 AM
> To: xen-devel@lists.xensource.com
> Cc: Dan Magenheimer
> Subject: [PATCH] fix domain reference leaks
>
> Besides two unlikely/rarely hit ones in x86 code, the main offender
> was tmh_client_from_cli_id(), which didn't even have a counterpart
> (albeit it had a comment correctly saying that it causes d->refcnt to
> get incremented). Unfortunately(?) this required a bit of code
> restructuring (as I needed to change the code anyway, I also fixed
> a couple os missing bounds checks which would sooner or later be
> reported as security vulnerabilities), so I would hope Dan could give
> it his blessing before it gets applied.
>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
>
> --- 2010-02-09.orig/xen/arch/x86/debug.c 2010-02-09 10:50:02.000000000
> +0100
> +++ 2010-02-09/xen/arch/x86/debug.c 2010-02-09 09:24:58.000000000 +0100
> @@ -252,10 +252,11 @@ dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf,
> else
> len = __copy_from_user(buf, (void *)addr, len);
> }
> - else
> + else if ( dp )
> {
> - if ( dp && !dp->is_dying ) /* make sure guest is still there
> */
> + if ( !dp->is_dying ) /* make sure guest is still there */
> len= dbg_rw_guest_mem(addr, buf, len, dp, toaddr, pgd3);
> + put_domain(dp);
> }
>
> DBGP2("gmem:exit:len:$%d\n", len);
> --- 2010-02-09.orig/xen/arch/x86/mm.c 2010-01-06 11:22:26.000000000
> +0100
> +++ 2010-02-09/xen/arch/x86/mm.c 2010-02-09 09:39:36.000000000 +0100
> @@ -3805,6 +3805,7 @@ int steal_page(
> struct domain *d, struct page_info *page, unsigned int memflags)
> {
> unsigned long x, y;
> + bool_t drop_dom_ref = 0;
>
> spin_lock(&d->page_alloc_lock);
>
> @@ -3832,11 +3833,13 @@ int steal_page(
> } while ( (y = cmpxchg(&page->count_info, x, x | 1)) != x );
>
> /* Unlink from original owner. */
> - if ( !(memflags & MEMF_no_refcount) )
> - d->tot_pages--;
> + if ( !(memflags & MEMF_no_refcount) && !--d->tot_pages )
> + drop_dom_ref = 1;
> page_list_del(page, &d->page_list);
>
> spin_unlock(&d->page_alloc_lock);
> + if ( unlikely(drop_dom_ref) )
> + put_domain(d);
> return 0;
>
> fail:
> --- 2010-02-09.orig/xen/common/tmem.c 2010-02-09 10:50:02.000000000
> +0100
> +++ 2010-02-09/xen/common/tmem.c 2010-02-09 11:40:21.000000000 +0100
> @@ -912,14 +912,14 @@ static client_t *client_create(cli_id_t
> return NULL;
> }
> memset(client,0,sizeof(client_t));
> - if ( (client->tmh = tmh_client_init()) == NULL )
> + if ( (client->tmh = tmh_client_init(cli_id)) == NULL )
> {
> printk("failed... can't allocate host-dependent part of
> client\n");
> if ( client )
> tmh_free_infra(client);
> return NULL;
> }
> - tmh_set_client_from_id(client,cli_id);
> + tmh_set_client_from_id(client, client->tmh, cli_id);
> client->cli_id = cli_id;
> #ifdef __i386__
> client->compress = 0;
> @@ -1528,7 +1528,7 @@ static NOINLINE int do_tmem_destroy_pool
> }
>
> static NOINLINE int do_tmem_new_pool(cli_id_t this_cli_id,
> - uint32_t this_pool_id, uint32_t
> flags,
> + uint32_t d_poolid, uint32_t
> flags,
> uint64_t uuid_lo, uint64_t
> uuid_hi)
> {
> client_t *client;
> @@ -1540,19 +1540,13 @@ static NOINLINE int do_tmem_new_pool(cli
> int specversion = (flags >> TMEM_POOL_VERSION_SHIFT)
> & TMEM_POOL_VERSION_MASK;
> pool_t *pool, *shpool;
> - int s_poolid, d_poolid, first_unused_s_poolid;
> + int s_poolid, first_unused_s_poolid;
> int i;
>
> if ( this_cli_id == CLI_ID_NULL )
> - {
> - client = tmh_client_from_current();
> cli_id = tmh_get_cli_id_from_current();
> - } else {
> - if ( (client = tmh_client_from_cli_id(this_cli_id)) == NULL)
> - return -EPERM;
> + else
> cli_id = this_cli_id;
> - }
> - ASSERT(client != NULL);
> printk("tmem: allocating %s-%s tmem pool for %s=%d...",
> persistent ? "persistent" : "ephemeral" ,
> shared ? "shared" : "private", cli_id_str, cli_id);
> @@ -1573,19 +1567,24 @@ static NOINLINE int do_tmem_new_pool(cli
> }
> if ( this_cli_id != CLI_ID_NULL )
> {
> - d_poolid = this_pool_id;
> - if ( client->pools[d_poolid] != NULL )
> - return -EPERM;
> - d_poolid = this_pool_id;
> + if ( (client = tmh_client_from_cli_id(this_cli_id)) == NULL
> + || d_poolid >= MAX_POOLS_PER_DOMAIN
> + || client->pools[d_poolid] != NULL )
> + goto fail;
> }
> - else for ( d_poolid = 0; d_poolid < MAX_POOLS_PER_DOMAIN;
> d_poolid++ )
> - if ( client->pools[d_poolid] == NULL )
> - break;
> - if ( d_poolid >= MAX_POOLS_PER_DOMAIN )
> + else
> {
> - printk("failed... no more pool slots available for this %s\n",
> - client_str);
> - goto fail;
> + client = tmh_client_from_current();
> + ASSERT(client != NULL);
> + for ( d_poolid = 0; d_poolid < MAX_POOLS_PER_DOMAIN;
> d_poolid++ )
> + if ( client->pools[d_poolid] == NULL )
> + break;
> + if ( d_poolid >= MAX_POOLS_PER_DOMAIN )
> + {
> + printk("failed... no more pool slots available for this
> %s\n",
> + client_str);
> + goto fail;
> + }
> }
> if ( shared )
> {
> @@ -1618,6 +1617,8 @@ static NOINLINE int do_tmem_new_pool(cli
> client->pools[d_poolid] =
> global_shared_pools[s_poolid];
> shared_pool_join(global_shared_pools[s_poolid],
> client);
> pool_free(pool);
> + if ( this_cli_id != CLI_ID_NULL )
> + tmh_client_put(client->tmh);
> return d_poolid;
> }
> }
> @@ -1638,6 +1639,8 @@ static NOINLINE int do_tmem_new_pool(cli
> }
> }
> client->pools[d_poolid] = pool;
> + if ( this_cli_id != CLI_ID_NULL )
> + tmh_client_put(client->tmh);
> list_add_tail(&pool->pool_list, &global_pool_list);
> pool->pool_id = d_poolid;
> pool->persistent = persistent;
> @@ -1647,6 +1650,8 @@ static NOINLINE int do_tmem_new_pool(cli
>
> fail:
> pool_free(pool);
> + if ( this_cli_id != CLI_ID_NULL )
> + tmh_client_put(client->tmh);
> return -EPERM;
> }
>
> @@ -1672,6 +1677,7 @@ static int tmemc_freeze_pools(cli_id_t c
> if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
> return -1;
> client_freeze(client,freeze);
> + tmh_client_put(client->tmh);
> printk("tmem: all pools %s for %s=%d\n",s,cli_id_str,cli_id);
> }
> return 0;
> @@ -1876,8 +1882,10 @@ static int tmemc_list(cli_id_t cli_id, t
> }
> else if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
> return -1;
> - else
> + else {
> off = tmemc_list_client(client, buf, 0, len, use_long);
> + tmh_client_put(client->tmh);
> + }
>
> return 0;
> }
> @@ -1925,7 +1933,10 @@ static int tmemc_set_var(cli_id_t cli_id
> else if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
> return -1;
> else
> - tmemc_set_var_one(client, subop, arg1);
> + {
> + tmemc_set_var_one(client, subop, arg1);
> + tmh_client_put(client->tmh);
> + }
> return 0;
> }
>
> @@ -1941,6 +1952,8 @@ static NOINLINE int tmemc_shared_pool_au
> return 1;
> }
> client = tmh_client_from_cli_id(cli_id);
> + if ( client == NULL )
> + return -EINVAL;
> for ( i = 0; i < MAX_GLOBAL_SHARED_POOLS; i++)
> {
> if ( (client->shared_auth_uuid[i][0] == uuid_lo) &&
> @@ -1949,6 +1962,7 @@ static NOINLINE int tmemc_shared_pool_au
> if ( auth == 0 )
> client->shared_auth_uuid[i][0] =
> client->shared_auth_uuid[i][1] = -1L;
> + tmh_client_put(client->tmh);
> return 1;
> }
> if ( (auth == 1) && (client->shared_auth_uuid[i][0] == -1L) &&
> @@ -1956,11 +1970,15 @@ static NOINLINE int tmemc_shared_pool_au
> free = i;
> }
> if ( auth == 0 )
> + {
> + tmh_client_put(client->tmh);
> return 0;
> + }
> if ( auth == 1 && free == -1 )
> return -ENOMEM;
> client->shared_auth_uuid[free][0] = uuid_lo;
> client->shared_auth_uuid[free][1] = uuid_hi;
> + tmh_client_put(client->tmh);
> return 1;
> }
>
> @@ -1968,10 +1986,12 @@ static NOINLINE int tmemc_save_subop(int
> uint32_t subop, tmem_cli_va_t buf, uint32_t
> arg1)
> {
> client_t *client = tmh_client_from_cli_id(cli_id);
> - pool_t *pool = (client == NULL) ? NULL : client->pools[pool_id];
> + pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
> + ? NULL : client->pools[pool_id];
> uint32_t p;
> uint64_t *uuid;
> pgp_t *pgp, *pgp2;
> + int rc = -1;
>
> switch(subop)
> {
> @@ -1982,45 +2002,55 @@ static NOINLINE int tmemc_save_subop(int
> if ( client->pools[p] != NULL )
> break;
> if ( p == MAX_POOLS_PER_DOMAIN )
> - return 0;
> + {
> + rc = 0;
> + break;
> + }
> client->was_frozen = client->frozen;
> client->frozen = 1;
> if ( arg1 != 0 )
> client->live_migrating = 1;
> - return 1;
> + rc = 1;
> + break;
> case TMEMC_RESTORE_BEGIN:
> - ASSERT(client == NULL);
> - if ( (client = client_create(cli_id)) == NULL )
> - return -1;
> - return 1;
> + if ( client == NULL && (client = client_create(cli_id)) !=
> NULL )
> + return 1;
> + break;
> case TMEMC_SAVE_GET_VERSION:
> - return TMEM_SPEC_VERSION;
> + rc = TMEM_SPEC_VERSION;
> + break;
> case TMEMC_SAVE_GET_MAXPOOLS:
> - return MAX_POOLS_PER_DOMAIN;
> + rc = MAX_POOLS_PER_DOMAIN;
> + break;
> case TMEMC_SAVE_GET_CLIENT_WEIGHT:
> - return client->weight == -1 ? -2 : client->weight;
> + rc = client->weight == -1 ? -2 : client->weight;
> + break;
> case TMEMC_SAVE_GET_CLIENT_CAP:
> - return client->cap == -1 ? -2 : client->cap;
> + rc = client->cap == -1 ? -2 : client->cap;
> + break;
> case TMEMC_SAVE_GET_CLIENT_FLAGS:
> - return (client->compress ? TMEM_CLIENT_COMPRESS : 0 ) |
> - (client->was_frozen ? TMEM_CLIENT_FROZEN : 0 );
> + rc = (client->compress ? TMEM_CLIENT_COMPRESS : 0 ) |
> + (client->was_frozen ? TMEM_CLIENT_FROZEN : 0 );
> + break;
> case TMEMC_SAVE_GET_POOL_FLAGS:
> if ( pool == NULL )
> - return -1;
> - return (pool->persistent ? TMEM_POOL_PERSIST : 0) |
> - (pool->shared ? TMEM_POOL_SHARED : 0) |
> - (pool->pageshift << TMEM_POOL_PAGESIZE_SHIFT);
> + break;
> + rc = (pool->persistent ? TMEM_POOL_PERSIST : 0) |
> + (pool->shared ? TMEM_POOL_SHARED : 0) |
> + (pool->pageshift << TMEM_POOL_PAGESIZE_SHIFT);
> + break;
> case TMEMC_SAVE_GET_POOL_NPAGES:
> if ( pool == NULL )
> - return -1;
> - return _atomic_read(pool->pgp_count);
> + break;
> + rc = _atomic_read(pool->pgp_count);
> + break;
> case TMEMC_SAVE_GET_POOL_UUID:
> if ( pool == NULL )
> - return -1;
> + break;
> uuid = (uint64_t *)buf.p;
> *uuid++ = pool->uuid[0];
> *uuid = pool->uuid[1];
> - return 0;
> + rc = 0;
> case TMEMC_SAVE_END:
> client->live_migrating = 0;
> if ( !list_empty(&client->persistent_invalidated_list) )
> @@ -2028,27 +2058,34 @@ static NOINLINE int tmemc_save_subop(int
> &client->persistent_invalidated_list, client_inv_pages)
> pgp_free_from_inv_list(client,pgp);
> client->frozen = client->was_frozen;
> - return 0;
> + rc = 0;
> }
> - return -1;
> + if ( client )
> + tmh_client_put(client->tmh);
> + return rc;
> }
>
> static NOINLINE int tmemc_save_get_next_page(int cli_id, int pool_id,
> tmem_cli_va_t buf, uint32_t bufsize)
> {
> client_t *client = tmh_client_from_cli_id(cli_id);
> - pool_t *pool = (client == NULL) ? NULL : client->pools[pool_id];
> + pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
> + ? NULL : client->pools[pool_id];
> pgp_t *pgp;
> int ret = 0;
> struct tmem_handle *h;
> unsigned int pagesize = 1 << (pool->pageshift+12);
>
> - if ( pool == NULL )
> - return -1;
> - if ( is_ephemeral(pool) )
> + if ( pool == NULL || is_ephemeral(pool) )
> + {
> + tmh_client_put(client->tmh);
> return -1;
> + }
> if ( bufsize < pagesize + sizeof(struct tmem_handle) )
> + {
> + tmh_client_put(client->tmh);
> return -ENOMEM;
> + }
>
> tmem_spin_lock(&pers_lists_spinlock);
> if ( list_empty(&pool->persistent_page_list) )
> @@ -2080,6 +2117,7 @@ static NOINLINE int tmemc_save_get_next_
>
> out:
> tmem_spin_unlock(&pers_lists_spinlock);
> + tmh_client_put(client->tmh);
> return ret;
> }
>
> @@ -2094,7 +2132,10 @@ static NOINLINE int tmemc_save_get_next_
> if ( client == NULL )
> return 0;
> if ( bufsize < sizeof(struct tmem_handle) )
> + {
> + tmh_client_put(client->tmh);
> return 0;
> + }
> tmem_spin_lock(&pers_lists_spinlock);
> if ( list_empty(&client->persistent_invalidated_list) )
> goto out;
> @@ -2121,6 +2162,7 @@ static NOINLINE int tmemc_save_get_next_
> ret = 1;
> out:
> tmem_spin_unlock(&pers_lists_spinlock);
> + tmh_client_put(client->tmh);
> return ret;
> }
>
> @@ -2128,22 +2170,26 @@ static int tmemc_restore_put_page(int cl
> uint32_t index, tmem_cli_va_t buf, uint32_t
> bufsize)
> {
> client_t *client = tmh_client_from_cli_id(cli_id);
> - pool_t *pool = (client == NULL) ? NULL : client->pools[pool_id];
> + pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
> + ? NULL : client->pools[pool_id];
> + int rc = pool ? do_tmem_put(pool,oid,index,0,0,0,bufsize,buf.p) :
> -1;
>
> - if ( pool == NULL )
> - return -1;
> - return do_tmem_put(pool,oid,index,0,0,0,bufsize,buf.p);
> + if ( client )
> + tmh_client_put(client->tmh);
> + return rc;
> }
>
> static int tmemc_restore_flush_page(int cli_id, int pool_id, uint64_t
> oid,
> uint32_t index)
> {
> client_t *client = tmh_client_from_cli_id(cli_id);
> - pool_t *pool = (client == NULL) ? NULL : client->pools[pool_id];
> + pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
> + ? NULL : client->pools[pool_id];
> + int rc = pool ? do_tmem_flush_page(pool, oid, index) : -1;
>
> - if ( pool == NULL )
> - return -1;
> - return do_tmem_flush_page(pool, oid, index);
> + if ( client )
> + tmh_client_put(client->tmh);
> + return rc;
> }
>
> static NOINLINE int do_tmem_control(struct tmem_op *op)
> --- 2010-02-09.orig/xen/common/tmem_xen.c 2010-02-09 10:50:02.000000000
> +0100
> +++ 2010-02-09/xen/common/tmem_xen.c 2010-02-09 10:03:16.000000000
> +0100
> @@ -290,17 +290,16 @@ static void tmh_persistent_pool_page_put
>
> /****************** XEN-SPECIFIC CLIENT HANDLING
> ********************/
>
> -EXPORT tmh_client_t *tmh_client_init(void)
> +EXPORT tmh_client_t *tmh_client_init(cli_id_t cli_id)
> {
> tmh_client_t *tmh;
> char name[5];
> - domid_t domid = current->domain->domain_id;
> int i, shift;
>
> if ( (tmh = xmalloc(tmh_client_t)) == NULL )
> return NULL;
> for (i = 0, shift = 12; i < 4; shift -=4, i++)
> - name[i] = (((unsigned short)domid >> shift) & 0xf) + '0';
> + name[i] = (((unsigned short)cli_id >> shift) & 0xf) + '0';
> name[4] = '\0';
> #ifndef __i386__
> tmh->persistent_pool = xmem_pool_create(name,
> tmh_persistent_pool_page_get,
> @@ -311,7 +310,6 @@ EXPORT tmh_client_t *tmh_client_init(voi
> return NULL;
> }
> #endif
> - tmh->domain = current->domain;
> return tmh;
> }
>
> @@ -321,6 +319,7 @@ EXPORT void tmh_client_destroy(tmh_clien
> xmem_pool_destroy(tmh->persistent_pool);
> #endif
> put_domain(tmh->domain);
> + tmh->domain = NULL;
> }
>
> /****************** XEN-SPECIFIC HOST INITIALIZATION
> ********************/
> --- 2010-02-09.orig/xen/include/xen/tmem_xen.h 2010-02-09
> 10:50:02.000000000 +0100
> +++ 2010-02-09/xen/include/xen/tmem_xen.h 2010-02-09 11:39:13.000000000
> +0100
> @@ -43,8 +43,6 @@ extern rwlock_t tmem_rwlock;
>
> extern void tmh_copy_page(char *to, char*from);
> extern int tmh_init(void);
> -extern tmh_client_t *tmh_client_init(void);
> -extern void tmh_client_destroy(tmh_client_t *);
> #define tmh_hash hash_long
>
> extern void tmh_release_avail_pages_to_host(void);
> @@ -281,6 +279,9 @@ typedef domid_t cli_id_t;
> typedef struct domain tmh_cli_ptr_t;
> typedef struct page_info pfp_t;
>
> +extern tmh_client_t *tmh_client_init(cli_id_t);
> +extern void tmh_client_destroy(tmh_client_t *);
> +
> /* this appears to be unreliable when a domain is being shut down */
> static inline struct client *tmh_client_from_cli_id(cli_id_t cli_id)
> {
> @@ -290,6 +291,11 @@ static inline struct client *tmh_client_
> return (struct client *)(d->tmem);
> }
>
> +static inline void tmh_client_put(tmh_client_t *tmh)
> +{
> + put_domain(tmh->domain);
> +}
> +
> static inline struct client *tmh_client_from_current(void)
> {
> return (struct client *)(current->domain->tmem);
> @@ -307,10 +313,12 @@ static inline tmh_cli_ptr_t *tmh_get_cli
> return current->domain;
> }
>
> -static inline void tmh_set_client_from_id(struct client
> *client,cli_id_t cli_id)
> +static inline void tmh_set_client_from_id(struct client *client,
> + tmh_client_t *tmh, cli_id_t
> cli_id)
> {
> struct domain *d = get_domain_by_id(cli_id);
> d->tmem = client;
> + tmh->domain = d;
> }
>
> static inline bool_t tmh_current_is_privileged(void)
>
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2010-02-11 21:41 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-09 13:32 [PATCH] fix domain reference leaks Jan Beulich
2010-02-11 21:41 ` Dan Magenheimer
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).