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 Cc: Dan Magenheimer --- 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)