[Xen-devel] [PATCH] gnttab: fix and clean up gnttab_set_version()

["Jan Beulich" <JBeulich@xxxxxxxx>]

- don't silently truncate frame numbers
- drop pointless nr_grant_entries() check from loop over reserved
  entries (adding suitable BUILD_BUG_ON()s to validate that)
- adjust types
- rename d to currd
- formatting

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -356,10 +356,16 @@ static unsigned int nr_grant_entries(str
 {
     switch ( gt->gt_version )
     {
+#define f2e(nr, ver) (((nr) << PAGE_SHIFT) / sizeof(grant_entry_v##ver##_t))
     case 1:
-        return (nr_grant_frames(gt) << PAGE_SHIFT) / sizeof(grant_entry_v1_t);
+        BUILD_BUG_ON(f2e(INITIAL_NR_GRANT_FRAMES, 1) <
+                     GNTTAB_NR_RESERVED_ENTRIES);
+        return f2e(nr_grant_frames(gt), 1);
     case 2:
-        return (nr_grant_frames(gt) << PAGE_SHIFT) / sizeof(grant_entry_v2_t);
+        BUILD_BUG_ON(f2e(INITIAL_NR_GRANT_FRAMES, 2) <
+                     GNTTAB_NR_RESERVED_ENTRIES);
+        return f2e(nr_grant_frames(gt), 2);
+#undef f2e
     }
 
     return 0;
@@ -2453,17 +2459,17 @@ static long
 gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
 {
     gnttab_set_version_t op;
-    struct domain *d = current->domain;
-    struct grant_table *gt = d->grant_table;
+    struct domain *currd = current->domain;
+    struct grant_table *gt = currd->grant_table;
     grant_entry_v1_t reserved_entries[GNTTAB_NR_RESERVED_ENTRIES];
-    long res;
-    int i;
+    int res;
+    unsigned int i;
 
-    if (copy_from_guest(&op, uop, 1))
+    if ( copy_from_guest(&op, uop, 1) )
         return -EFAULT;
 
     res = -EINVAL;
-    if (op.version != 1 && op.version != 2)
+    if ( op.version != 1 && op.version != 2 )
         goto out;
 
     res = 0;
@@ -2471,10 +2477,12 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARA
         goto out;
 
     write_lock(&gt->lock);
-    /* Make sure that the grant table isn't currently in use when we
-       change the version number, except for the first 8 entries which
-       are allowed to be in use (xenstore/xenconsole keeps them mapped).
-       (You need to change the version number for e.g. kexec.) */
+    /*
+     * Make sure that the grant table isn't currently in use when we
+     * change the version number, except for the first 8 entries which
+     * are allowed to be in use (xenstore/xenconsole keeps them mapped).
+     * (You need to change the version number for e.g. kexec.)
+     */
     for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
     {
         if ( read_atomic(&_active_entry(gt, i).pin) != 0 )
@@ -2487,76 +2495,106 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARA
         }
     }
 
-    /* XXX: If we're going to version 2, we could maybe shrink the
-       active grant table here. */
-
-    if ( op.version == 2 && gt->gt_version < 2 )
+    switch ( gt->gt_version )
     {
-        res = gnttab_populate_status_frames(d, gt, nr_grant_frames(gt));
-        if ( res < 0)
-            goto out_unlock;
+    case 0:
+        if ( op.version == 2 )
+        {
+    case 1:
+            /* XXX: We could maybe shrink the active grant table here. */
+            res = gnttab_populate_status_frames(currd, gt, 
nr_grant_frames(gt));
+            if ( res < 0)
+                goto out_unlock;
+        }
+        break;
+    case 2:
+        for ( i = 0; i < GNTTAB_NR_RESERVED_ENTRIES; i++ )
+        {
+            if ( ((shared_entry_v2(gt, i).hdr.flags & GTF_type_mask) ==
+                  GTF_permit_access) &&
+                 (shared_entry_v2(gt, i).full_page.frame >> 32) )
+            {
+                gdprintk(XENLOG_WARNING,
+                         "tried to change grant table version to 1 with 
non-representable entries\n");
+                res = -ERANGE;
+                goto out_unlock;
+            }
+        }
+        break;
     }
 
-    /* Preserve the first 8 entries (toolstack reserved grants) */
-    if ( gt->gt_version == 1 )
-    {
-        memcpy(reserved_entries, &shared_entry_v1(gt, 0), 
sizeof(reserved_entries));
-    }
-    else if ( gt->gt_version == 2 )
+    /* Preserve the first 8 entries (toolstack reserved grants). */
+    switch ( gt->gt_version )
     {
-        for ( i = 0; i < GNTTAB_NR_RESERVED_ENTRIES && i < 
nr_grant_entries(gt); i++ )
+    case 1:
+        memcpy(reserved_entries, &shared_entry_v1(gt, 0),
+               sizeof(reserved_entries));
+        break;
+    case 2:
+        for ( i = 0; i < GNTTAB_NR_RESERVED_ENTRIES; i++ )
         {
-            int flags = status_entry(gt, i);
-            flags |= shared_entry_v2(gt, i).hdr.flags;
-            if ((flags & GTF_type_mask) == GTF_permit_access)
+            unsigned int flags = shared_entry_v2(gt, i).hdr.flags;
+
+            switch ( flags & GTF_type_mask )
             {
-                reserved_entries[i].flags = flags;
+            case GTF_permit_access:
+                reserved_entries[i].flags = flags | status_entry(gt, i);
                 reserved_entries[i].domid = shared_entry_v2(gt, i).hdr.domid;
                 reserved_entries[i].frame = shared_entry_v2(gt, 
i).full_page.frame;
-            }
-            else
-            {
-                if ((flags & GTF_type_mask) != GTF_invalid)
-                    gdprintk(XENLOG_INFO, "d%d: bad flags %x in grant %d when 
switching grant version\n",
-                           d->domain_id, flags, i);
+                break;
+            default:
+                gdprintk(XENLOG_INFO,
+                         "bad flags %x in grant %u when switching version\n",
+                         flags, i);
+                /* fall through */
+            case GTF_invalid:
                 memset(&reserved_entries[i], 0, sizeof(reserved_entries[i]));
+                break;
             }
         }
+        break;
     }
 
     if ( op.version < 2 && gt->gt_version == 2 )
-        gnttab_unpopulate_status_frames(d, gt);
+        gnttab_unpopulate_status_frames(currd, gt);
 
-    /* Make sure there's no crud left over in the table from the
-       old version. */
+    /* Make sure there's no crud left over from the old version. */
     for ( i = 0; i < nr_grant_frames(gt); i++ )
         clear_page(gt->shared_raw[i]);
 
-    /* Restore the first 8 entries (toolstack reserved grants) */
-    if ( gt->gt_version != 0 && op.version == 1 )
-    {
-        memcpy(&shared_entry_v1(gt, 0), reserved_entries, 
sizeof(reserved_entries));
-    }
-    else if ( gt->gt_version != 0 && op.version == 2 )
+    /* Restore the first 8 entries (toolstack reserved grants). */
+    if ( gt->gt_version )
     {
-        for ( i = 0; i < GNTTAB_NR_RESERVED_ENTRIES; i++ )
+        switch ( op.version )
         {
-            status_entry(gt, i) = reserved_entries[i].flags & 
(GTF_reading|GTF_writing);
-            shared_entry_v2(gt, i).hdr.flags = reserved_entries[i].flags & 
~(GTF_reading|GTF_writing);
-            shared_entry_v2(gt, i).hdr.domid = reserved_entries[i].domid;
-            shared_entry_v2(gt, i).full_page.frame = reserved_entries[i].frame;
+        case 1:
+            memcpy(&shared_entry_v1(gt, 0), reserved_entries, 
sizeof(reserved_entries));
+            break;
+        case 2:
+            for ( i = 0; i < GNTTAB_NR_RESERVED_ENTRIES; i++ )
+            {
+                status_entry(gt, i) =
+                    reserved_entries[i].flags & (GTF_reading | GTF_writing);
+                shared_entry_v2(gt, i).hdr.flags =
+                    reserved_entries[i].flags & ~(GTF_reading | GTF_writing);
+                shared_entry_v2(gt, i).hdr.domid =
+                    reserved_entries[i].domid;
+                shared_entry_v2(gt, i).full_page.frame =
+                    reserved_entries[i].frame;
+            }
+            break;
         }
     }
 
     gt->gt_version = op.version;
 
-out_unlock:
+ out_unlock:
     write_unlock(&gt->lock);
 
-out:
+ out:
     op.version = gt->gt_version;
 
-    if (__copy_to_guest(uop, &op, 1))
+    if ( __copy_to_guest(uop, &op, 1) )
         res = -EFAULT;
 
     return res;

Attachment: gnttab-set-version.patch
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel