WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-c

To: Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-changelog] [xen-unstable] acm: Further fixes after grant-table changes.)
From: Stefan Berger <stefanb@xxxxxxxxxx>
Date: Tue, 20 Feb 2007 14:00:00 -0500
Cc: Isaku Yamahata <yamahata@xxxxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxx, Keir Fraser <keir@xxxxxxxxxxxxx>
Delivery-date: Tue, 20 Feb 2007 10:59:24 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <C2005DFC.243B%Keir.Fraser@xxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx

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