Fix correctness race in xc_mem_paging_prep

Andres Lagar-Cavilla writes ("[PATCH 1 of 2] After preparing a page for page-in, allow immediate fill-in of the page contents"):

xen/include/public/domctl.h struct xen_domctl_mem_event_op { · line 750

Quoted 9 lines

(quoted diff) +0 -2

-    /* OP_ENABLE */
-    uint64_aligned_t shared_addr;  /* IN:  Virtual address of shared

page */

xen/include/public/domctl.h +6 -0

+    union {
+        /* OP_ENABLE IN:  Virtual address of shared page */
+        uint64_aligned_t shared_addr;
+        /* PAGING_PREP IN: buffer to immediately fill page in */
+        uint64_aligned_t buffer;
+    } u;

Do we care that this interface is very binary-incompatible ? Is there a flag or version somewhere where we can at least arrange for this to be detected ? Perhaps we should allocate a new domctl number for this version, so old code gets "no idea what you're talking about" rather than wrong behaviour ?

Ian.

I turned the field into a union of the same size, so it should be binary compatible. Should...

diff -r 4ee6d40edc2c -r 0ce71e5bfaac xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +22 -3

@@ -974,14 +974,43 @@ void p2m_mem_paging_populate(struct doma
  * mfn if populate was called for  gfn which was nominated but not evicted. In
  * this case only the p2mt needs to be forwarded.
  */
-int p2m_mem_paging_prep(struct domain *d, unsigned long gfn)
+int p2m_mem_paging_prep(struct domain *d, unsigned long gfn, uint64_t buffer)
 {
     struct page_info *page;
     p2m_type_t p2mt;
     p2m_access_t a;
-    mfn_t mfn;
+    mfn_t mfn, buf_mfn = _mfn(INVALID_MFN);
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
-    int ret;
+    int ret, page_extant = 1;
+    void *buf_map = NULL;
+
+    /* Map buffer page, if any, and get a reference */
+    if ( buffer )
+    {
+        l1_pgentry_t l1e;
+        unsigned long buf_gfn;
+        p2m_type_t buf_p2mt;
+
+        if ( (buffer & (PAGE_SIZE - 1)) || 
+             (!access_ok(buffer, PAGE_SIZE)) )
+            return -EINVAL;
+
+        guest_get_eff_l1e(current, buffer, &l1e);
+        buf_gfn = l1e_get_pfn(l1e);
+        buf_mfn = get_gfn(current->domain, buf_gfn, 
+                            &buf_p2mt);
+
+        if ( likely( mfn_valid(buf_mfn) &&

Hi,

This looks good to me. I think it needs two more things to make it correct (as well as the tools patch 2/2):
- an update to the xenpaging tool to use the new interface; and

- a possible update to the paging state machine --- after all, if the prep call allocates the pageand fills its contents, do we need any more stages on the page-in path?

One more comment below:

Quoted 39 lines · Andres Lagar-Cavilla

diff -r 4ee6d40edc2c -r 0ce71e5bfaac xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +0 -0

@@ -974,14 +974,43 @@ void p2m_mem_paging_populate(struct doma
  * mfn if populate was called for  gfn which was nominated but not

evicted. In
* this case only the p2mt needs to be forwarded. */

xen/arch/x86/mm/p2m.c +1 -1

-int p2m_mem_paging_prep(struct domain *d, unsigned long gfn)
+int p2m_mem_paging_prep(struct domain *d, unsigned long gfn, uint64_t

buffer)

 {
     struct page_info *page;
     p2m_type_t p2mt;
     p2m_access_t a;

xen/arch/x86/mm/p2m.c +21 -2

-    mfn_t mfn;
+    mfn_t mfn, buf_mfn = _mfn(INVALID_MFN);
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
-    int ret;
+    int ret, page_extant = 1;
+    void *buf_map = NULL;
+
+    /* Map buffer page, if any, and get a reference */
+    if ( buffer )
+    {
+        l1_pgentry_t l1e;
+        unsigned long buf_gfn;
+        p2m_type_t buf_p2mt;
+
+        if ( (buffer & (PAGE_SIZE - 1)) ||
+             (!access_ok(buffer, PAGE_SIZE)) )
+            return -EINVAL;
+
+        guest_get_eff_l1e(current, buffer, &l1e);
+        buf_gfn = l1e_get_pfn(l1e);
+        buf_mfn = get_gfn(current->domain, buf_gfn,
+                            &buf_p2mt);
+
+        if ( likely( mfn_valid(buf_mfn) &&

xen/arch/x86/mm/p2m.c void p2m_mem_paging_populate(struct doma · line 1013

Quoted 10 lines

xen/arch/x86/mm/p2m.c +10 -0

+                     p2m_is_ram(buf_p2mt) ) )
+        {
+            get_page(mfn_to_page(buf_mfn), current->domain);
+            buf_map = map_domain_page(mfn_x(buf_mfn));
+            put_gfn(current->domain, buf_gfn);
+        } else {
+            put_gfn(current->domain, buf_gfn);
+            return -EINVAL;
+        }
+    }

We could maybe avoid all this mechanism by doing a copy_from_user() of the buffer contents directly into the new page, instead of an explicit map-and-memcpy().

Cheers,

Tim.

P2m_mem_paging_prep ensures that an mfn is backing the paged-out gfn, and transitions to the next state in the paging state machine for this page. Foreign mappings of the gfn will now succeed. This is the key idea, as it allows the pager to now map the gfn and fill in its contents.

Unfortunately, it also allows any other foreign mapper to map the gfn and read its contents. This is particularly dangerous when the populate is launched by a foreign mapper in the first place, which will be actively retrying the map operation and might race with the pager. Qemu-dm being a prime example.

Quoted 5 lines · Andres Lagar-Cavilla

P2m_mem_paging_prep ensures that an mfn is backing the paged-out gfn, and transitions to the next state in the paging state machine for this page. Foreign mappings of the gfn will now succeed. This is the key idea, as it allows the pager to now map the gfn and fill in its contents.

Quoted 4 lines

Unfortunately, it also allows any other foreign mapper to map the gfn and read its contents. This is particularly dangerous when the populate is launched by a foreign mapper in the first place, which will be actively retrying the map operation and might race with the pager. Qemu-dm being a prime example.

Yes, I think thats a real issue. The concept looks ok to me.