Re: [Xen-devel] [PATCH] acm: fix deadlock and bogus loop (Was Re: [Xen-changelog] [xen-unstable] acm: Further fixes after grant-table changes.)

[Stefan Berger <stefanb@xxxxxxxxxx>]

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