* [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-changelog] [xen-unstable] acm: Further fixes after grant-table changes.)
[not found] <200702200120.l1K1K9WM016337@xenbits2.xensource.com>
@ 2007-02-20 3:10 ` Isaku Yamahata
2007-02-20 8:12 ` Keir Fraser
0 siblings, 1 reply; 4+ messages in thread
From: Isaku Yamahata @ 2007-02-20 3:10 UTC (permalink / raw)
To: Keir Fraser; +Cc: xen-devel
[-- Attachment #1: Type: text/plain, Size: 3837 bytes --]
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@localhost.localdomain
> # Date 1171901134 0
> # Node ID 184db7a674d93d92d0d963a7b3c80f1889983a9e
> # Parent 3b7bdb7bd1303eff6db3e223fc5bb8d06c86c570
> acm: Further fixes after grant-table changes.
> Based on a patch from Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Keir Fraser <keir@xensource.com>
> ---
> 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@lists.xensource.com
> http://lists.xensource.com/xen-changelog
>
--
yamahata
[-- Attachment #2: 13997_fe78383d8218_acm_fix_13991.patch --]
[-- Type: text/x-diff, Size: 2362 bytes --]
# HG changeset patch
# User yamahata@valinux.co.jp
# Date 1171940459 -32400
# Node ID fe78383d82188f72c34ef656bb6ee27334b4d500
# Parent b83cfb117bddc1f44be53bde49d86b5075bdaa2a
acm: fix deadlock and bogus loop.
PATCHNAME: acm_fix_13991
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
diff -r b83cfb117bdd -r fe78383d8218 xen/acm/acm_simple_type_enforcement_hooks.c
--- a/xen/acm/acm_simple_type_enforcement_hooks.c Mon Feb 19 22:50:07 2007 +0000
+++ b/xen/acm/acm_simple_type_enforcement_hooks.c Tue Feb 20 12:00:59 2007 +0900
@@ -235,7 +235,7 @@ ste_init_state(struct acm_ste_policy_buf
}
/* b) check for grant table conflicts on shared pages */
spin_lock(&(*pd)->grant_table->lock);
- for ( i = 0; i < nr_grant_frames((*pd)->grant_table); i++ ) {
+ for ( i = 0; i < nr_grant_entries((*pd)->grant_table); i++ ) {
#define SPP (PAGE_SIZE / sizeof(struct grant_entry))
sha_copy = (*pd)->grant_table->shared[i/SPP][i%SPP];
if ( sha_copy.flags ) {
@@ -244,8 +244,9 @@ ste_init_state(struct acm_ste_policy_buf
(unsigned long)sha_copy.frame);
rdomid = sha_copy.domid;
if ((rdom = get_domain_by_id(rdomid)) == NULL) {
+ spin_unlock(&(*pd)->grant_table->lock);
printkd("%s: domain not found ERROR!\n", __func__);
- goto out_gnttab;
+ goto out;
};
/* rdom now has remote domain */
ste_rssid = GET_SSIDP(ACM_SIMPLE_TYPE_ENFORCEMENT_POLICY,
@@ -253,16 +254,16 @@ ste_init_state(struct acm_ste_policy_buf
ste_rssidref = ste_rssid->ste_ssidref;
put_domain(rdom);
if (!have_common_type(ste_ssidref, ste_rssidref)) {
+ spin_unlock(&(*pd)->grant_table->lock);
printkd("%s: Policy violation in grant table sharing domain %x -> domain %x.\n",
__func__, (*pd)->domain_id, rdomid);
- goto out_gnttab;
+ 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;
[-- 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] 4+ messages in thread
* Re: [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-changelog] [xen-unstable] acm: Further fixes after grant-table changes.)
2007-02-20 3:10 ` [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-changelog] [xen-unstable] acm: Further fixes after grant-table changes.) Isaku Yamahata
@ 2007-02-20 8:12 ` Keir Fraser
2007-02-20 19:00 ` Stefan Berger
0 siblings, 1 reply; 4+ messages in thread
From: Keir Fraser @ 2007-02-20 8:12 UTC (permalink / raw)
To: Isaku Yamahata, Keir Fraser; +Cc: xen-devel
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@valinux.co.jp> 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@localhost.localdomain
>> # Date 1171901134 0
>> # Node ID 184db7a674d93d92d0d963a7b3c80f1889983a9e
>> # Parent 3b7bdb7bd1303eff6db3e223fc5bb8d06c86c570
>> acm: Further fixes after grant-table changes.
>> Based on a patch from Isaku Yamahata <yamahata@valinux.co.jp>
>> Signed-off-by: Keir Fraser <keir@xensource.com>
>> ---
>> 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@lists.xensource.com
>> http://lists.xensource.com/xen-changelog
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-changelog] [xen-unstable] acm: Further fixes after grant-table changes.)
2007-02-20 8:12 ` Keir Fraser
@ 2007-02-20 19:00 ` Stefan Berger
2007-02-20 22:53 ` Keir Fraser
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Berger @ 2007-02-20 19:00 UTC (permalink / raw)
To: Keir Fraser; +Cc: Isaku Yamahata, xen-devel, Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 6772 bytes --]
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@us.ibm.com>
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@lists.xensource.com 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@valinux.co.jp> 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@localhost.localdomain
> >> # Date 1171901134 0
> >> # Node ID 184db7a674d93d92d0d963a7b3c80f1889983a9e
> >> # Parent 3b7bdb7bd1303eff6db3e223fc5bb8d06c86c570
> >> acm: Further fixes after grant-table changes.
> >> Based on a patch from Isaku Yamahata <yamahata@valinux.co.jp>
> >> Signed-off-by: Keir Fraser <keir@xensource.com>
> >> ---
> >> 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@lists.xensource.com
> >> http://lists.xensource.com/xen-changelog
> >>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 11690 bytes --]
[-- Attachment #2: 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] 4+ messages in thread
* Re: [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-changelog] [xen-unstable] acm: Further fixes after grant-table changes.)
2007-02-20 19:00 ` Stefan Berger
@ 2007-02-20 22:53 ` Keir Fraser
0 siblings, 0 replies; 4+ messages in thread
From: Keir Fraser @ 2007-02-20 22:53 UTC (permalink / raw)
To: Stefan Berger; +Cc: Isaku Yamahata, xen-devel, Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 412 bytes --]
On 20/2/07 19:00, "Stefan Berger" <stefanb@us.ibm.com> wrote:
> 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?
Why are you looking at the shared grant table at all, when it¹s the active
table that tells you with certainty which grant entries are in use?
-- Keir
[-- Attachment #1.2: Type: text/html, Size: 1000 bytes --]
[-- Attachment #2: 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] 4+ messages in thread
end of thread, other threads:[~2007-02-20 22:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <200702200120.l1K1K9WM016337@xenbits2.xensource.com>
2007-02-20 3:10 ` [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-changelog] [xen-unstable] acm: Further fixes after grant-table changes.) Isaku Yamahata
2007-02-20 8:12 ` Keir Fraser
2007-02-20 19:00 ` Stefan Berger
2007-02-20 22:53 ` Keir Fraser
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.