[PATCH] mem_sharing: fix race condition of nominate and unshare

* [PATCH] mem_sharing: fix race condition of nominate and unshare
@ 2011-01-06 16:11 Jui-Hao Chiang
  2011-01-06 16:54 ` Tim Deegan
  2011-01-07  3:14 ` tinnycloud
  0 siblings, 2 replies; 50+ messages in thread
From: Jui-Hao Chiang @ 2011-01-06 16:11 UTC (permalink / raw)
  To: Tim Deegan; +Cc: tinnycloud, xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 638 bytes --]

Hi, this patch does the following
(1) When updating/checking p2m type for mem_sharing, we must hold shr_lock
(2) For nominate operation, if the page is already nominated, return the
handle from page_info->shr_handle
(3) For unshare operation, it is possible that multiple users unshare a page
via hvm_hap_nested_page_fault() at the same time. If the page is already
un-shared by someone else, simply return success.

NOTE: we assume that nobody holds page_alloc_lock/p2m_lock before calling
nominate/share/unshare.

Signed-off-by: Jui-Hao Chiang <juihaochiang@gmail.com>
Signed-off-by: Han-Lin Li <Han-Lin.Li@itri.org.tw>

Bests,
Jui-Hao

[-- Attachment #1.2: Type: text/html, Size: 1575 bytes --]

[-- Attachment #2: mem_sharing_p2mt_race.patch --]
[-- Type: text/x-diff, Size: 2913 bytes --]

diff -r 7b4c82f07281 xen/arch/x86/mm/mem_sharing.c

--- a/xen/arch/x86/mm/mem_sharing.c	Wed Jan 05 23:54:15 2011 +0000
+++ b/xen/arch/x86/mm/mem_sharing.c	Thu Jan 06 23:46:28 2011 +0800
@@ -502,6 +502,7 @@ int mem_sharing_nominate_page(struct p2m
 
     *phandle = 0UL;
 
+    shr_lock(); 
     mfn = gfn_to_mfn(p2m, gfn, &p2mt);
 
     /* Check if mfn is valid */
@@ -509,29 +510,33 @@ int mem_sharing_nominate_page(struct p2m
     if (!mfn_valid(mfn))
         goto out;
 
+    /* Return the handle if the page is already shared */
+    page = mfn_to_page(mfn);
+    if (p2m_is_shared(p2mt)) {
+        *phandle = page->shr_handle;
+        ret = 0;
+        goto out;
+    }
+
     /* Check p2m type */
     if (!p2m_is_sharable(p2mt))
         goto out;
 
     /* Try to convert the mfn to the sharable type */
-    page = mfn_to_page(mfn);
     ret = page_make_sharable(d, page, expected_refcnt); 
     if(ret) 
         goto out;
 
     /* Create the handle */
     ret = -ENOMEM;
-    shr_lock(); 
     handle = next_handle++;  
     if((hash_entry = mem_sharing_hash_insert(handle, mfn)) == NULL)
     {
-        shr_unlock();
         goto out;
     }
     if((gfn_info = mem_sharing_gfn_alloc()) == NULL)
     {
         mem_sharing_hash_destroy(hash_entry);
-        shr_unlock();
         goto out;
     }
 
@@ -545,7 +550,6 @@ int mem_sharing_nominate_page(struct p2m
         BUG_ON(page_make_private(d, page) != 0);
         mem_sharing_hash_destroy(hash_entry);
         mem_sharing_gfn_destroy(gfn_info, 0);
-        shr_unlock();
         goto out;
     }
 
@@ -559,11 +563,11 @@ int mem_sharing_nominate_page(struct p2m
     gfn_info->domain = d->domain_id;
     page->shr_handle = handle;
     *phandle = handle;
-    shr_unlock();
 
     ret = 0;
 
 out:
+    shr_unlock();
     return ret;
 }
 
@@ -633,14 +637,21 @@ int mem_sharing_unshare_page(struct p2m_
     struct list_head *le;
     struct domain *d = p2m->domain;
 
+    mem_sharing_audit();
+    /* Remove the gfn_info from the list */
+    shr_lock();
+    
     mfn = gfn_to_mfn(p2m, gfn, &p2mt);
+    
+    /* Has someone already unshared it? */
+    if (!p2m_is_shared(p2mt)) {
+        shr_unlock();
+        return 0;
+    }
 
     page = mfn_to_page(mfn);
     handle = page->shr_handle;
  
-    mem_sharing_audit();
-    /* Remove the gfn_info from the list */
-    shr_lock();
     hash_entry = mem_sharing_hash_lookup(handle); 
     list_for_each(le, &hash_entry->gfns)
     {
@@ -707,7 +718,6 @@ private_page_found:
         mem_sharing_hash_delete(handle);
     else
         atomic_dec(&nr_saved_mfns);
-    shr_unlock();
 
     if(p2m_change_type(p2m, gfn, p2m_ram_shared, p2m_ram_rw) != 
                                                 p2m_ram_shared) 
@@ -718,6 +728,7 @@ private_page_found:
     /* Update m2p entry */
     set_gpfn_from_mfn(mfn_x(page_to_mfn(page)), gfn);
 
+    shr_unlock();
     return 0;
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

^ permalink raw reply	[flat|nested] 50+ messages in thread