Re: save/restore race

[John Levon <levon@movementarian.org>]

On Tue, Jan 23, 2007 at 11:38:29PM -0000, Ian Pratt wrote:

> We should make xc_linux_save cope more gracefully with
> arch.pfn_to_mfn_frame_list_list being NULL, have xc_linux_save zero the
> field in shared_info before writing it out (its an MFN and not valid
> anyhow), and then move the field's re-initialisation a little later in
> the post_suspend function to close the race.

I've done some testing with the below, and it seems to do the trick.

regards
john


# HG changeset patch
# User john.levon@sun.com
# Date 1169600022 28800
# Node ID 3121e0cb7be68fa46c59f61422f05adc03ec5a5e
# Parent  a75413c0072fa5b892bd8b6a05c1f1d3435bb093
Close save-after-restore race. Make xc_linux_save() wait for the
frame_list_list MFN to be updated by the domain before trying to use it.

Signed-off-by: John Levon <john.levon@sun.com>

diff --git a/tools/libxc/xc_linux_save.c b/tools/libxc/xc_linux_save.c
--- a/tools/libxc/xc_linux_save.c
+++ b/tools/libxc/xc_linux_save.c
@@ -403,6 +403,33 @@ static int suspend_and_state(int (*suspe
     return -1;
 }
 
+/*
+** Map the top-level page of MFNs from the guest. The guest might not have
+** finished resuming from a previous restore operation, so we wait a while for
+** it to update the MFN to a reasonable value.
+*/
+static void *map_frame_list_list(int xc_handle, uint32_t dom,
+                                 shared_info_t *shinfo)
+{
+    int count = 3000;
+    void *p;
+
+    while (count-- && shinfo->arch.pfn_to_mfn_frame_list_list == 0)
+        usleep(10000);
+
+    if (shinfo->arch.pfn_to_mfn_frame_list_list == 0) {
+        ERROR("Timed out waiting for frame list updated.");
+        return NULL;
+    }
+
+    p = xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ,
+                             shinfo->arch.pfn_to_mfn_frame_list_list);
+
+    if (p == NULL)
+        ERROR("Couldn't map p2m_frame_list_list (errno %d)", errno);
+
+    return p;
+}
 
 /*
 ** During transfer (or in the state file), all page-table pages must be
@@ -663,14 +690,11 @@ int xc_linux_save(int xc_handle, int io_
 
     max_pfn = live_shinfo->arch.max_pfn;
 
-    live_p2m_frame_list_list =
-        xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ,
-                             live_shinfo->arch.pfn_to_mfn_frame_list_list);
-
-    if (!live_p2m_frame_list_list) {
-        ERROR("Couldn't map p2m_frame_list_list (errno %d)", errno);
-        goto out;
-    }
+    live_p2m_frame_list_list = map_frame_list_list(xc_handle, dom,
+                                                   live_shinfo);
+
+    if (!live_p2m_frame_list_list)
+        goto out;
 
     live_p2m_frame_list =
         xc_map_foreign_batch(xc_handle, dom, PROT_READ,
@@ -1169,8 +1193,14 @@ int xc_linux_save(int xc_handle, int io_
     ctxt.ctrlreg[3] = 
         xen_pfn_to_cr3(mfn_to_pfn(xen_cr3_to_pfn(ctxt.ctrlreg[3])));
 
+    /*
+     * Reset the MFN to be a known-invalid value. See map_frame_list_list().
+     */
+    memcpy(page, live_shinfo, PAGE_SIZE);
+    ((shared_info_t *)page)->arch.pfn_to_mfn_frame_list_list = 0;
+
     if (!write_exact(io_fd, &ctxt, sizeof(ctxt)) ||
-        !write_exact(io_fd, live_shinfo, PAGE_SIZE)) {
+        !write_exact(io_fd, page, PAGE_SIZE)) {
         ERROR("Error when writing to state file (1) (errno %d)", errno);
         goto out;
     }