Re: [Xen-devel] [PATCH] Allow programatic iomem permissions

[Kieran Mansley <kmansley@xxxxxxxxxxxxxx>]

On Fri, 2007-07-13 at 15:12 +0100, Keir Fraser wrote:
> On 13/7/07 14:03, "Kieran Mansley" <kmansley@xxxxxxxxxxxxxx> wrote:
> 
> >> The issue is that the granter is informed that the grant is released before
> >> stale grantee TLB entries are flushed. If the grantee is multi-vcpu then he
> >> could theoretically still access a granted page via a stale TLB entry after
> >> the granter has recycled the page. The window is extremely tiny though! The
> >> correct fix is to reorder the unmap operation to be unmap-list-of-grants
> >> then TLB-flush then update-grant-entries-to-indicate-release. Then the 
> >> whole
> >> problem disappears.
> > 
> > OK, that makes sense, and doesn't at first impression look too hard to
> > rectify.
> > 
> > Am I right in thinking that it's the shared grant table entry that is
> > the critical one in this sense (as opposed to the "active" entry).
> 
> That's correct.

OK, patch attached.  It compiles and seems to work, but hasn't been
heavily tested yet.  I'm sure it could be more efficient, but wanted to
check I was heading in the right direction.

Kieran

Fix TLB flush on grant unmap

diff -r 87cc3035108f xen/common/grant_table.c
--- a/xen/common/grant_table.c  Fri Jul 13 14:32:11 2007 +0100
+++ b/xen/common/grant_table.c  Fri Jul 13 16:35:27 2007 +0100
@@ -505,16 +505,49 @@ __gnttab_unmap_common(
         }
     }
 
-    if ( (map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 )
-    {
-        map->flags = 0;
-        put_maptrack_handle(ld->grant_table, op->handle);
-    }
-
     /* If just unmapped a writable mapping, mark as dirtied */
     if ( !(flags & GNTMAP_readonly) )
          gnttab_mark_dirty(rd, frame);
 
+ unmap_out:
+    op->status = rc;
+    spin_unlock(&rd->grant_table->lock);
+    rcu_unlock_domain(rd);
+}
+
+static void
+__gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
+{
+    struct domain   *ld, *rd;
+    domid_t          dom;
+    struct active_grant_entry *act;
+    grant_entry_t   *sha;
+    struct grant_mapping *map;
+    u16 flags;
+
+    ld = current->domain;
+    map = &maptrack_entry(ld->grant_table, op->handle);
+    dom = map->domid;
+    flags = map->flags;
+
+    if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) ) {
+        /* This shouldn't happen - __gnttab_unmap_common should have
+           already crashed the domain */
+        gdprintk(XENLOG_INFO, "Could not find domain %d\n", dom);
+        return;
+    }
+       
+    spin_lock(&rd->grant_table->lock);
+
+    act = &active_entry(rd->grant_table, map->ref);
+    sha = &shared_entry(rd->grant_table, map->ref);
+
+    if ( (map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 )
+    {
+        map->flags = 0;
+        put_maptrack_handle(ld->grant_table, op->handle);
+    }
+
     if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) &&
          !(flags & GNTMAP_readonly) )
         gnttab_clear_flag(_GTF_writing, &sha->flags);
@@ -522,8 +555,6 @@ __gnttab_unmap_common(
     if ( act->pin == 0 )
         gnttab_clear_flag(_GTF_reading, &sha->flags);
 
- unmap_out:
-    op->status = rc;
     spin_unlock(&rd->grant_table->lock);
     rcu_unlock_domain(rd);
 }
@@ -542,28 +573,51 @@ __gnttab_unmap_grant_ref(
     op->status = common.status;
 }
 
+static void
+__gnttab_unmap_grant_ref_complete(struct gnttab_unmap_grant_ref *op)
+{
+    struct gnttab_unmap_common common = {
+       .host_addr = op->host_addr,
+       .dev_bus_addr = op->dev_bus_addr,
+       .handle = op->handle,
+    };
+
+    __gnttab_unmap_common_complete(&common);
+}
+
 static long
 gnttab_unmap_grant_ref(
     XEN_GUEST_HANDLE(gnttab_unmap_grant_ref_t) uop, unsigned int count)
 {
-    int i;
+    int i, done = 0;
+    long rc = -EFAULT;
     struct gnttab_unmap_grant_ref op;
 
     for ( i = 0; i < count; i++ )
     {
         if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) )
-            goto fault;
+            goto out;
         __gnttab_unmap_grant_ref(&op);
+        ++done;
         if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) )
-            goto fault;
-    }
-
+            goto out;
+    }
+
+    rc = 0;
+
+out:
     flush_tlb_mask(current->domain->domain_dirty_cpumask);
-    return 0;
-
-fault:
-    flush_tlb_mask(current->domain->domain_dirty_cpumask);
-    return -EFAULT;    
+
+    for ( i = 0; i < done; i++ )
+    {
+        if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) )
+            /* This really shouldn't happen as it worked earlier in
+               the function */
+            continue;
+        __gnttab_unmap_grant_ref_complete(&op);
+
+    }
+    return rc;
 }
 
 static void
@@ -580,28 +634,51 @@ __gnttab_unmap_and_replace(
     op->status = common.status;
 }
 
+static void
+__gnttab_unmap_and_replace(
+    struct gnttab_unmap_and_replace *op)
+{
+    struct gnttab_unmap_common common = {
+       .host_addr = op->host_addr,
+       .new_addr = op->new_addr,
+       .handle = op->handle,
+    };
+
+    __gnttab_unmap_common_complete(&common);
+}
+
 static long
 gnttab_unmap_and_replace(
     XEN_GUEST_HANDLE(gnttab_unmap_and_replace_t) uop, unsigned int
count)
 {
-    int i;
+    int i, done = 0;
+    long rc = -EFAULT;
     struct gnttab_unmap_and_replace op;
 
     for ( i = 0; i < count; i++ )
     {
         if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) )
-            goto fault;
+            goto out;
         __gnttab_unmap_and_replace(&op);
+        ++done;
         if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) )
-            goto fault;
-    }
-
+            goto out;
+    }
+
+    rc = 0;
+
+out:
     flush_tlb_mask(current->domain->domain_dirty_cpumask);
-    return 0;
-
-fault:
-    flush_tlb_mask(current->domain->domain_dirty_cpumask);
-    return -EFAULT;    
+
+    for ( i = 0; i < done; i++ )
+    {
+        if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) )
+            /* This really shouldn't happen as it worked earlier in
+               the function */
+            continue;
+        __gnttab_unmap_and_replace_complete(&op);
+    }
+    return rc;    
 }
 
 int

Attachment: unmap_tlb_fix
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel