Isaku Yamahata
2007-Feb-20 03:10 UTC
[Xen-devel] [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-changelog] [xen-unstable] acm: Further fixes after grant-table changes.)
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2007-Feb-20 08:12 UTC
Re: [Xen-devel] [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-changelog] [xen-unstable] acm: Further fixes after grant-table changes.)
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
Stefan Berger
2007-Feb-20 19:00 UTC
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@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2007-Feb-20 22:53 UTC
Re: [Xen-devel] [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-changelog] [xen-unstable] acm: Further fixes after grant-table changes.)
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel