|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-changelog] [xen-unstable] acm: Further fixes after grant-table changes.)
The patch below should solve the unlocking issue. I saw that sha_copy is used and a copy is being made of the shared entry. Is the copy necessary considering that there's a spinlock or could we not rather use a pointer? Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxx> diff -r 89ca591a2c21 xen/acm/acm_simple_type_enforcement_hooks.c --- a/xen/acm/acm_simple_type_enforcement_hooks.c Mon Feb 19 20:44:42 2007 -0800 +++ b/xen/acm/acm_simple_type_enforcement_hooks.c Tue Feb 20 13:47:53 2007 -0500 @@ -207,6 +208,7 @@ ste_init_state(struct acm_ste_policy_buf rdomid = (*pd)->evtchn[port]->u.unbound.remote_domid; if ((rdom = get_domain_by_id(rdomid)) == NULL) { printk("%s: Error finding domain to id %x!\n", __func__, rdomid); + spin_unlock(&(*pd)->evtchn_lock); goto out; } /* rdom now has remote domain */ @@ -245,7 +247,8 @@ ste_init_state(struct acm_ste_policy_buf rdomid = sha_copy.domid; if ((rdom = get_domain_by_id(rdomid)) == NULL) { printkd("%s: domain not found ERROR!\n", __func__); - goto out_gnttab; + spin_unlock(&(*pd)->grant_table->lock); + goto out; }; /* rdom now has remote domain */ ste_rssid = GET_SSIDP(ACM_SIMPLE_TYPE_ENFORCEMENT_POLICY, @@ -255,14 +258,14 @@ ste_init_state(struct acm_ste_policy_buf if (!have_common_type(ste_ssidref, ste_rssidref)) { printkd("%s: Policy violation in grant table sharing domain %x -> domain %x.\n", __func__, (*pd)->domain_id, rdomid); - goto out_gnttab; + spin_unlock(&(*pd)->grant_table->lock); + goto out; } } } + spin_unlock(&(*pd)->grant_table->lock); } violation = 0; - out_gnttab: - spin_unlock(&(*pd)->grant_table->lock); out: read_unlock(&domlist_lock); return violation; xen-devel-bounces@xxxxxxxxxxxxxxxxxxx wrote on 02/20/2007 03:12:44 AM: > Okay, but this ACM loop is highly suspect anyway. I have no idea why it > checks the shared table rather than the active table. > > -- Keir > > On 20/2/07 03:10, "Isaku Yamahata" <yamahata@xxxxxxxxxxxxx> wrote: > > > acm: fix deadlock and bogus loop. > > > > It is very likly to overlook the outside loop. > > for ( pd = &domain_list; *pd != NULL; pd = &(*pd)->next_in_list ) { > > ... > > spin_lock(&(*pd)->grant_table->lock); > > for ( i = 0; i < nr_grant_entries((*pd)->grant_table); i++ ) { > > ... > > } > > spin_unlock(&(*pd)->grant_table->lock); <<< necessary > > } > > out_gnttab: > > spin_unlock(&(*pd)->grant_table->lock); <<< bogus > > > > > > On Mon, Feb 19, 2007 at 05:20:09PM -0800, Xen patchbot-unstable wrote: > >> # HG changeset patch > >> # User kfraser@xxxxxxxxxxxxxxxxxxxxx > >> # Date 1171901134 0 > >> # Node ID 184db7a674d93d92d0d963a7b3c80f1889983a9e > >> # Parent 3b7bdb7bd1303eff6db3e223fc5bb8d06c86c570 > >> acm: Further fixes after grant-table changes. > >> Based on a patch from Isaku Yamahata <yamahata@xxxxxxxxxxxxx> > >> Signed-off-by: Keir Fraser <keir@xxxxxxxxxxxxx> > >> --- > >> xen/acm/acm_simple_type_enforcement_hooks.c | 20 ++++++++++---------- > >> 1 files changed, 10 insertions(+), 10 deletions(-) > >> > >> diff -r 3b7bdb7bd130 -r 184db7a674d9 > >> xen/acm/acm_simple_type_enforcement_hooks.c > >> --- a/xen/acm/acm_simple_type_enforcement_hooks.c Mon Feb 19 15:52:51 2007 > >> +0000 > >> +++ b/xen/acm/acm_simple_type_enforcement_hooks.c Mon Feb 19 16:05:34 2007 > >> +0000 > >> @@ -177,7 +177,7 @@ ste_init_state(struct acm_ste_policy_buf > >> ssidref_t ste_ssidref, ste_rssidref; > >> struct domain **pd, *rdom; > >> domid_t rdomid; > >> - struct grant_entry *sha_copy; > >> + struct grant_entry sha_copy; > >> int port, i; > >> > >> read_lock(&domlist_lock); /* go by domain? or directly by global? > >> event/grant list */ > >> @@ -234,20 +234,18 @@ ste_init_state(struct acm_ste_policy_buf > >> } > >> } > >> /* b) check for grant table conflicts on shared pages */ > >> - if ((*pd)->grant_table->shared == NULL) { > >> - printkd("%s: Grant ... sharing for domain %x not setup!\n", > >> __func__, (*pd)->domain_id); > >> - continue; > >> - } > >> + spin_lock(&(*pd)->grant_table->lock); > >> for ( i = 0; i < nr_grant_frames((*pd)->grant_table); i++ ) { > >> - sha_copy = (*pd)->grant_table->shared[i]; > >> - if ( sha_copy->flags ) { > >> +#define SPP (PAGE_SIZE / sizeof(struct grant_entry)) > >> + sha_copy = (*pd)->grant_table->shared[i/SPP][i%SPP]; > >> + if ( sha_copy.flags ) { > >> printkd("%s: grant dom (%hu) SHARED (%d) flags:(%hx) > >> dom:(%hu) frame:(%lx)\n", > >> __func__, (*pd)->domain_id, i, sha_copy.flags, > >> sha_copy.domid, > >> (unsigned long)sha_copy.frame); > >> - rdomid = sha_copy->domid; > >> + rdomid = sha_copy.domid; > >> if ((rdom = get_domain_by_id(rdomid)) == NULL) { > >> printkd("%s: domain not found ERROR!\n", __func__); > >> - goto out; > >> + goto out_gnttab; > >> }; > >> /* rdom now has remote domain */ > >> ste_rssid = GET_SSIDP(ACM_SIMPLE_TYPE_ENFORCEMENT_POLICY, > >> @@ -257,12 +255,14 @@ ste_init_state(struct acm_ste_policy_buf > >> if (!have_common_type(ste_ssidref, ste_rssidref)) { > >> printkd("%s: Policy violation in grant table sharing > >> domain %x -> domain %x.\n", > >> __func__, (*pd)->domain_id, rdomid); > >> - goto out; > >> + goto out_gnttab; > >> } > >> } > >> } > >> } > >> violation = 0; > >> + out_gnttab: > >> + spin_unlock(&(*pd)->grant_table->lock); > >> out: > >> read_unlock(&domlist_lock); > >> return violation; > >> > >> _______________________________________________ > >> Xen-changelog mailing list > >> Xen-changelog@xxxxxxxxxxxxxxxxxxx > >> http://lists.xensource.com/xen-changelog > >> > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@xxxxxxxxxxxxxxxxxxx > http://lists.xensource.com/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |