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

[Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>]

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