[PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" introduced
grabbing extra references for pages that drop references tied to PGC_allocated.
However, the way these extra references were grabbed were incorrect, resulting
in both share_pages and unshare_pages failing. There actually is no need to
grab extra references, only a reordering was needed for when the existing
references are being released. This is in accordance to the XSA-242
recommendation of not calling _put_page_type while also holding the page_lock
for that page.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
---
v2: Add assert that put_count is at least 1
---
 xen/arch/x86/mm/mem_sharing.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 5ac9d8f54c..708037d91e 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -900,6 +900,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
     p2m_type_t smfn_type, cmfn_type;
     struct two_gfns tg;
     struct rmap_iterator ri;
+    unsigned long put_count = 0;
 
     get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
                  cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
@@ -964,15 +965,6 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
         goto err_out;
     }
 
-    /* Acquire an extra reference, for the freeing below to be safe. */
-    if ( !get_page(cpage, cd) )
-    {
-        ret = -EOVERFLOW;
-        mem_sharing_page_unlock(secondpg);
-        mem_sharing_page_unlock(firstpg);
-        goto err_out;
-    }
-
     /* Merge the lists together */
     rmap_seed_iterator(cpage, &ri);
     while ( (gfn = rmap_iterate(cpage, &ri)) != NULL)
@@ -984,7 +976,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
          * Don't change the type of rmap for the client page. */
         rmap_del(gfn, cpage, 0);
         rmap_add(gfn, spage);
-        put_page_and_type(cpage);
+        put_count++;
         d = get_domain_by_id(gfn->domain);
         BUG_ON(!d);
         BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
@@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
     /* Free the client page */
     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
         put_page(cpage);
-    put_page(cpage);
+
+    ASSERT(put_count);
+    while ( put_count-- )
+        put_page_and_type(cpage);
 
     /* We managed to free a domain page. */
     atomic_dec(&nr_shared_mfns);
@@ -1167,20 +1162,11 @@ int __mem_sharing_unshare_page(struct domain *d,
     {
         if ( !last_gfn )
             mem_sharing_gfn_destroy(page, d, gfn_info);
-        put_page_and_type(page);
         mem_sharing_page_unlock(page);
-        if ( last_gfn )
-        {
-            if ( !get_page(page, d) )
-            {
-                put_gfn(d, gfn);
-                domain_crash(d);
-                return -EOVERFLOW;
-            }
-            if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
-                put_page(page);
+        if ( last_gfn &&
+            test_and_clear_bit(_PGC_allocated, &page->count_info) )
             put_page(page);
-        }
+        put_page_and_type(page);
         put_gfn(d, gfn);
 
         return 0;
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" introduced
grabbing extra references for pages that drop references tied to PGC_allocated.
However, the way these extra references were grabbed were incorrect, resulting
in both share_pages and unshare_pages failing. There actually is no need to
grab extra references, only a reordering was needed for when the existing
references are being released. This is in accordance to the XSA-242
recommendation of not calling _put_page_type while also holding the page_lock
for that page.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
---
v2: Add assert that put_count is at least 1
---
 xen/arch/x86/mm/mem_sharing.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 5ac9d8f54c..708037d91e 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -900,6 +900,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
     p2m_type_t smfn_type, cmfn_type;
     struct two_gfns tg;
     struct rmap_iterator ri;
+    unsigned long put_count = 0;
 
     get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
                  cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
@@ -964,15 +965,6 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
         goto err_out;
     }
 
-    /* Acquire an extra reference, for the freeing below to be safe. */
-    if ( !get_page(cpage, cd) )
-    {
-        ret = -EOVERFLOW;
-        mem_sharing_page_unlock(secondpg);
-        mem_sharing_page_unlock(firstpg);
-        goto err_out;
-    }
-
     /* Merge the lists together */
     rmap_seed_iterator(cpage, &ri);
     while ( (gfn = rmap_iterate(cpage, &ri)) != NULL)
@@ -984,7 +976,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
          * Don't change the type of rmap for the client page. */
         rmap_del(gfn, cpage, 0);
         rmap_add(gfn, spage);
-        put_page_and_type(cpage);
+        put_count++;
         d = get_domain_by_id(gfn->domain);
         BUG_ON(!d);
         BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
@@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
     /* Free the client page */
     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
         put_page(cpage);
-    put_page(cpage);
+
+    ASSERT(put_count);
+    while ( put_count-- )
+        put_page_and_type(cpage);
 
     /* We managed to free a domain page. */
     atomic_dec(&nr_shared_mfns);
@@ -1167,20 +1162,11 @@ int __mem_sharing_unshare_page(struct domain *d,
     {
         if ( !last_gfn )
             mem_sharing_gfn_destroy(page, d, gfn_info);
-        put_page_and_type(page);
         mem_sharing_page_unlock(page);
-        if ( last_gfn )
-        {
-            if ( !get_page(page, d) )
-            {
-                put_gfn(d, gfn);
-                domain_crash(d);
-                return -EOVERFLOW;
-            }
-            if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
-                put_page(page);
+        if ( last_gfn &&
+            test_and_clear_bit(_PGC_allocated, &page->count_info) )
             put_page(page);
-        }
+        put_page_and_type(page);
         put_gfn(d, gfn);
 
         return 0;
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/2] x86/mem_sharing: replace using page_lock with our own lock

[Tamas K Lengyel]

The page_lock system was not intended to be used outside the PV pagetable
code. Replace its use in mem_sharing with an internal rwlock.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
---
 xen/arch/x86/mm/mem_sharing.c     | 88 ++++++++++++-------------------
 xen/include/asm-x86/mem_sharing.h |  1 +
 2 files changed, 34 insertions(+), 55 deletions(-)

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 708037d91e..0afbff1d89 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -50,7 +50,7 @@ typedef struct pg_lock_data {
 static DEFINE_PER_CPU(pg_lock_data_t, __pld);
 
 #define MEM_SHARING_DEBUG(_f, _a...)                                  \
-    debugtrace_printk("mem_sharing_debug: %s(): " _f, __func__, ##_a)
+    gdprintk(XENLOG_INFO, _f, ##_a)
 
 /* Reverse map defines */
 #define RMAP_HASHTAB_ORDER  0
@@ -112,30 +112,21 @@ static inline void page_sharing_dispose(struct page_info *page)
 
 #endif /* MEM_SHARING_AUDIT */
 
-static inline int mem_sharing_page_lock(struct page_info *pg)
+static inline void mem_sharing_page_lock(struct page_info *pg)
 {
-    int rc;
     pg_lock_data_t *pld = &(this_cpu(__pld));
-
     page_sharing_mm_pre_lock();
-    rc = page_lock(pg);
-    if ( rc )
-    {
-        preempt_disable();
-        page_sharing_mm_post_lock(&pld->mm_unlock_level, 
-                                  &pld->recurse_count);
-    }
-    return rc;
+    write_lock(&pg->sharing->lock);
+    page_sharing_mm_post_lock(&pld->mm_unlock_level, &pld->recurse_count);
 }
 
 static inline void mem_sharing_page_unlock(struct page_info *pg)
 {
     pg_lock_data_t *pld = &(this_cpu(__pld));
+    page_sharing_mm_unlock(pld->mm_unlock_level, &pld->recurse_count);
 
-    page_sharing_mm_unlock(pld->mm_unlock_level, 
-                           &pld->recurse_count);
-    preempt_enable();
-    page_unlock(pg);
+    if ( pg->sharing )
+        write_unlock(&pg->sharing->lock);
 }
 
 static inline shr_handle_t get_next_handle(void)
@@ -398,12 +389,8 @@ static struct page_info* mem_sharing_lookup(unsigned long mfn)
         struct page_info* page = mfn_to_page(_mfn(mfn));
         if ( page_get_owner(page) == dom_cow )
         {
-            /* Count has to be at least two, because we're called
-             * with the mfn locked (1) and this is supposed to be 
-             * a shared page (1). */
             unsigned long t = read_atomic(&page->u.inuse.type_info);
             ASSERT((t & PGT_type_mask) == PGT_shared_page);
-            ASSERT((t & PGT_count_mask) >= 2);
             ASSERT(SHARED_M2P(get_gpfn_from_mfn(mfn)));
             return page;
         }
@@ -437,14 +424,7 @@ static int audit(void)
         pg = pg_shared_info->pg;
         mfn = page_to_mfn(pg);
 
-        /* If we can't lock it, it's definitely not a shared page */
-        if ( !mem_sharing_page_lock(pg) )
-        {
-           MEM_SHARING_DEBUG("mfn %lx in audit list, but cannot be locked (%lx)!\n",
-                              mfn_x(mfn), pg->u.inuse.type_info);
-           errors++;
-           continue;
-        }
+        mem_sharing_page_lock(pg);
 
         /* Check if the MFN has correct type, owner and handle. */ 
         if ( (pg->u.inuse.type_info & PGT_type_mask) != PGT_shared_page )
@@ -517,12 +497,12 @@ static int audit(void)
             put_domain(d);
             nr_gfns++;
         }
-        /* The type count has an extra ref because we have locked the page */
-        if ( (nr_gfns + 1) != (pg->u.inuse.type_info & PGT_count_mask) )
+
+        if ( nr_gfns != (pg->u.inuse.type_info & PGT_count_mask) )
         {
             MEM_SHARING_DEBUG("Mismatched counts for MFN=%lx."
                               "nr_gfns in list %lu, in type_info %lx\n",
-                              mfn_x(mfn), nr_gfns, 
+                              mfn_x(mfn), nr_gfns,
                               (pg->u.inuse.type_info & PGT_count_mask));
             errors++;
         }
@@ -626,7 +606,6 @@ static int page_make_sharable(struct domain *d,
     drop_dom_ref = !domain_adjust_tot_pages(d, -1);
     page_list_del(page, &d->page_list);
     spin_unlock(&d->page_alloc_lock);
-
     if ( drop_dom_ref )
         put_domain(d);
     return 0;
@@ -652,7 +631,7 @@ static int page_make_private(struct domain *d, struct page_info *page)
     /* Because we are locking pages individually, we need to drop
      * the lock here, while the page is typed. We cannot risk the 
      * race of page_unlock and then put_page_type. */
-    expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
+    expected_type = (PGT_shared_page | PGT_validated | 2);
     if ( page->u.inuse.type_info != expected_type )
     {
         spin_unlock(&d->page_alloc_lock);
@@ -688,10 +667,7 @@ static inline struct page_info *__grab_shared_page(mfn_t mfn)
         return NULL;
     pg = mfn_to_page(mfn);
 
-    /* If the page is not validated we can't lock it, and if it's  
-     * not validated it's obviously not shared. */
-    if ( !mem_sharing_page_lock(pg) )
-        return NULL;
+    mem_sharing_page_lock(pg);
 
     if ( mem_sharing_lookup(mfn_x(mfn)) == NULL )
     {
@@ -770,6 +746,7 @@ static int nominate_page(struct domain *d, gfn_t gfn,
     p2m_type_t p2mt;
     p2m_access_t p2ma;
     mfn_t mfn;
+    struct page_sharing_info *sharing = NULL;
     struct page_info *page = NULL; /* gcc... */
     int ret;
 
@@ -833,38 +810,39 @@ static int nominate_page(struct domain *d, gfn_t gfn,
     }
 #endif
 
-    /* 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;
+    /* We hold the gfn lock and this domain has the only reference to this page
+     * so we don't need any other locking, no other domain can use this page
+     * for sharing until we release the gfn lock */
 
-    /* Now that the page is validated, we can lock it. There is no 
-     * race because we're holding the p2m entry, so no one else 
-     * could be nominating this gfn */
-    ret = -ENOENT;
-    if ( !mem_sharing_page_lock(page) )
+    page = mfn_to_page(mfn);
+    /* Try to convert the mfn to the sharable type */
+    ret = page_make_sharable(d, page, expected_refcnt);
+    if ( ret )
         goto out;
 
     /* Initialize the shared state */
     ret = -ENOMEM;
-    if ( (page->sharing = 
-            xmalloc(struct page_sharing_info)) == NULL )
+    sharing = xmalloc(struct page_sharing_info);
+    if ( !sharing )
     {
-        /* Making a page private atomically unlocks it */
-        BUG_ON(page_make_private(d, page) != 0);
+        BUG_ON(!page_make_private(d, page));
         goto out;
     }
-    page->sharing->pg = page;
+
+    rwlock_init(&sharing->lock);
+
+    sharing->pg = page;
+    page->sharing = sharing;
+
     rmap_init(page);
 
     /* Create the handle */
-    page->sharing->handle = get_next_handle();  
+    page->sharing->handle = get_next_handle();
 
     /* Create the local gfn info */
     if ( mem_sharing_gfn_alloc(page, d, gfn_x(gfn)) == NULL )
     {
-        xfree(page->sharing);
+        xfree(sharing);
         page->sharing = NULL;
         BUG_ON(page_make_private(d, page) != 0);
         goto out;
@@ -881,7 +859,7 @@ static int nominate_page(struct domain *d, gfn_t gfn,
 
     *phandle = page->sharing->handle;
     audit_add_list(page);
-    mem_sharing_page_unlock(page);
+
     ret = 0;
 
 out:
diff --git a/xen/include/asm-x86/mem_sharing.h b/xen/include/asm-x86/mem_sharing.h
index 0e77b7d935..03cf651337 100644
--- a/xen/include/asm-x86/mem_sharing.h
+++ b/xen/include/asm-x86/mem_sharing.h
@@ -38,6 +38,7 @@ typedef struct rmap_hashtab {
 
 struct page_sharing_info
 {
+    rwlock_t lock;
     struct page_info *pg;   /* Back pointer to the page. */
     shr_handle_t handle;    /* Globally unique version / handle. */
 #if MEM_SHARING_AUDIT
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 2/2] x86/mem_sharing: replace using page_lock with our own lock

[Tamas K Lengyel]

The page_lock system was not intended to be used outside the PV pagetable
code. Replace its use in mem_sharing with an internal rwlock.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
---
 xen/arch/x86/mm/mem_sharing.c     | 88 ++++++++++++-------------------
 xen/include/asm-x86/mem_sharing.h |  1 +
 2 files changed, 34 insertions(+), 55 deletions(-)

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 708037d91e..0afbff1d89 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -50,7 +50,7 @@ typedef struct pg_lock_data {
 static DEFINE_PER_CPU(pg_lock_data_t, __pld);
 
 #define MEM_SHARING_DEBUG(_f, _a...)                                  \
-    debugtrace_printk("mem_sharing_debug: %s(): " _f, __func__, ##_a)
+    gdprintk(XENLOG_INFO, _f, ##_a)
 
 /* Reverse map defines */
 #define RMAP_HASHTAB_ORDER  0
@@ -112,30 +112,21 @@ static inline void page_sharing_dispose(struct page_info *page)
 
 #endif /* MEM_SHARING_AUDIT */
 
-static inline int mem_sharing_page_lock(struct page_info *pg)
+static inline void mem_sharing_page_lock(struct page_info *pg)
 {
-    int rc;
     pg_lock_data_t *pld = &(this_cpu(__pld));
-
     page_sharing_mm_pre_lock();
-    rc = page_lock(pg);
-    if ( rc )
-    {
-        preempt_disable();
-        page_sharing_mm_post_lock(&pld->mm_unlock_level, 
-                                  &pld->recurse_count);
-    }
-    return rc;
+    write_lock(&pg->sharing->lock);
+    page_sharing_mm_post_lock(&pld->mm_unlock_level, &pld->recurse_count);
 }
 
 static inline void mem_sharing_page_unlock(struct page_info *pg)
 {
     pg_lock_data_t *pld = &(this_cpu(__pld));
+    page_sharing_mm_unlock(pld->mm_unlock_level, &pld->recurse_count);
 
-    page_sharing_mm_unlock(pld->mm_unlock_level, 
-                           &pld->recurse_count);
-    preempt_enable();
-    page_unlock(pg);
+    if ( pg->sharing )
+        write_unlock(&pg->sharing->lock);
 }
 
 static inline shr_handle_t get_next_handle(void)
@@ -398,12 +389,8 @@ static struct page_info* mem_sharing_lookup(unsigned long mfn)
         struct page_info* page = mfn_to_page(_mfn(mfn));
         if ( page_get_owner(page) == dom_cow )
         {
-            /* Count has to be at least two, because we're called
-             * with the mfn locked (1) and this is supposed to be 
-             * a shared page (1). */
             unsigned long t = read_atomic(&page->u.inuse.type_info);
             ASSERT((t & PGT_type_mask) == PGT_shared_page);
-            ASSERT((t & PGT_count_mask) >= 2);
             ASSERT(SHARED_M2P(get_gpfn_from_mfn(mfn)));
             return page;
         }
@@ -437,14 +424,7 @@ static int audit(void)
         pg = pg_shared_info->pg;
         mfn = page_to_mfn(pg);
 
-        /* If we can't lock it, it's definitely not a shared page */
-        if ( !mem_sharing_page_lock(pg) )
-        {
-           MEM_SHARING_DEBUG("mfn %lx in audit list, but cannot be locked (%lx)!\n",
-                              mfn_x(mfn), pg->u.inuse.type_info);
-           errors++;
-           continue;
-        }
+        mem_sharing_page_lock(pg);
 
         /* Check if the MFN has correct type, owner and handle. */ 
         if ( (pg->u.inuse.type_info & PGT_type_mask) != PGT_shared_page )
@@ -517,12 +497,12 @@ static int audit(void)
             put_domain(d);
             nr_gfns++;
         }
-        /* The type count has an extra ref because we have locked the page */
-        if ( (nr_gfns + 1) != (pg->u.inuse.type_info & PGT_count_mask) )
+
+        if ( nr_gfns != (pg->u.inuse.type_info & PGT_count_mask) )
         {
             MEM_SHARING_DEBUG("Mismatched counts for MFN=%lx."
                               "nr_gfns in list %lu, in type_info %lx\n",
-                              mfn_x(mfn), nr_gfns, 
+                              mfn_x(mfn), nr_gfns,
                               (pg->u.inuse.type_info & PGT_count_mask));
             errors++;
         }
@@ -626,7 +606,6 @@ static int page_make_sharable(struct domain *d,
     drop_dom_ref = !domain_adjust_tot_pages(d, -1);
     page_list_del(page, &d->page_list);
     spin_unlock(&d->page_alloc_lock);
-
     if ( drop_dom_ref )
         put_domain(d);
     return 0;
@@ -652,7 +631,7 @@ static int page_make_private(struct domain *d, struct page_info *page)
     /* Because we are locking pages individually, we need to drop
      * the lock here, while the page is typed. We cannot risk the 
      * race of page_unlock and then put_page_type. */
-    expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
+    expected_type = (PGT_shared_page | PGT_validated | 2);
     if ( page->u.inuse.type_info != expected_type )
     {
         spin_unlock(&d->page_alloc_lock);
@@ -688,10 +667,7 @@ static inline struct page_info *__grab_shared_page(mfn_t mfn)
         return NULL;
     pg = mfn_to_page(mfn);
 
-    /* If the page is not validated we can't lock it, and if it's  
-     * not validated it's obviously not shared. */
-    if ( !mem_sharing_page_lock(pg) )
-        return NULL;
+    mem_sharing_page_lock(pg);
 
     if ( mem_sharing_lookup(mfn_x(mfn)) == NULL )
     {
@@ -770,6 +746,7 @@ static int nominate_page(struct domain *d, gfn_t gfn,
     p2m_type_t p2mt;
     p2m_access_t p2ma;
     mfn_t mfn;
+    struct page_sharing_info *sharing = NULL;
     struct page_info *page = NULL; /* gcc... */
     int ret;
 
@@ -833,38 +810,39 @@ static int nominate_page(struct domain *d, gfn_t gfn,
     }
 #endif
 
-    /* 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;
+    /* We hold the gfn lock and this domain has the only reference to this page
+     * so we don't need any other locking, no other domain can use this page
+     * for sharing until we release the gfn lock */
 
-    /* Now that the page is validated, we can lock it. There is no 
-     * race because we're holding the p2m entry, so no one else 
-     * could be nominating this gfn */
-    ret = -ENOENT;
-    if ( !mem_sharing_page_lock(page) )
+    page = mfn_to_page(mfn);
+    /* Try to convert the mfn to the sharable type */
+    ret = page_make_sharable(d, page, expected_refcnt);
+    if ( ret )
         goto out;
 
     /* Initialize the shared state */
     ret = -ENOMEM;
-    if ( (page->sharing = 
-            xmalloc(struct page_sharing_info)) == NULL )
+    sharing = xmalloc(struct page_sharing_info);
+    if ( !sharing )
     {
-        /* Making a page private atomically unlocks it */
-        BUG_ON(page_make_private(d, page) != 0);
+        BUG_ON(!page_make_private(d, page));
         goto out;
     }
-    page->sharing->pg = page;
+
+    rwlock_init(&sharing->lock);
+
+    sharing->pg = page;
+    page->sharing = sharing;
+
     rmap_init(page);
 
     /* Create the handle */
-    page->sharing->handle = get_next_handle();  
+    page->sharing->handle = get_next_handle();
 
     /* Create the local gfn info */
     if ( mem_sharing_gfn_alloc(page, d, gfn_x(gfn)) == NULL )
     {
-        xfree(page->sharing);
+        xfree(sharing);
         page->sharing = NULL;
         BUG_ON(page_make_private(d, page) != 0);
         goto out;
@@ -881,7 +859,7 @@ static int nominate_page(struct domain *d, gfn_t gfn,
 
     *phandle = page->sharing->handle;
     audit_add_list(page);
-    mem_sharing_page_unlock(page);
+
     ret = 0;
 
 out:
diff --git a/xen/include/asm-x86/mem_sharing.h b/xen/include/asm-x86/mem_sharing.h
index 0e77b7d935..03cf651337 100644
--- a/xen/include/asm-x86/mem_sharing.h
+++ b/xen/include/asm-x86/mem_sharing.h
@@ -38,6 +38,7 @@ typedef struct rmap_hashtab {
 
 struct page_sharing_info
 {
+    rwlock_t lock;
     struct page_info *pg;   /* Back pointer to the page. */
     shr_handle_t handle;    /* Globally unique version / handle. */
 #if MEM_SHARING_AUDIT
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/2] x86/mem_sharing: replace using page_lock with our own lock

[Tamas K Lengyel]

On Fri, Apr 12, 2019 at 1:50 PM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>
> The page_lock system was not intended to be used outside the PV pagetable
> code. Replace its use in mem_sharing with an internal rwlock.
>

After further testing I'm actually running into a lot of strange
problems with this when domains with shared memory get destroyed,
oddly enough after mem_sharing finishes cleaning up: (XEN)
[<ffff82d0802729b9>] domain.c#relinquish_memory+0x62/0x4f8. I tried
moving the lock into page_info instead (in a new struct with
sharing_info) but this approach seems to require a lot more effort and
a lot more thought put into it as page_lock seems to have some other
side-effect that makes this all work. Unfortunately at this time I
can't afford chasing this path further so I'll just go with the
band-aid solution of making page_lock/page_unlock mem_sharing
friendly.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/mem_sharing: replace using page_lock with our own lock

[Tamas K Lengyel]

On Fri, Apr 12, 2019 at 1:50 PM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>
> The page_lock system was not intended to be used outside the PV pagetable
> code. Replace its use in mem_sharing with an internal rwlock.
>

After further testing I'm actually running into a lot of strange
problems with this when domains with shared memory get destroyed,
oddly enough after mem_sharing finishes cleaning up: (XEN)
[<ffff82d0802729b9>] domain.c#relinquish_memory+0x62/0x4f8. I tried
moving the lock into page_info instead (in a new struct with
sharing_info) but this approach seems to require a lot more effort and
a lot more thought put into it as page_lock seems to have some other
side-effect that makes this all work. Unfortunately at this time I
can't afford chasing this path further so I'll just go with the
band-aid solution of making page_lock/page_unlock mem_sharing
friendly.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/12/19 8:47 PM, Tamas K Lengyel wrote:
> Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" introduced
> grabbing extra references for pages that drop references tied to PGC_allocated.
> However, the way these extra references were grabbed were incorrect, resulting
> in both share_pages and unshare_pages failing.

Why?  Is cd not the owner of the page?

> There actually is no need to
> grab extra references, only a reordering was needed for when the existing
> references are being released. This is in accordance to the XSA-242
> recommendation of not calling _put_page_type while also holding the page_lock
> for that page.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Roger Pau Monne <roger.pau@citrix.com>
> ---
> v2: Add assert that put_count is at least 1
> ---
>  xen/arch/x86/mm/mem_sharing.c | 32 +++++++++-----------------------
>  1 file changed, 9 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index 5ac9d8f54c..708037d91e 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -900,6 +900,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>      p2m_type_t smfn_type, cmfn_type;
>      struct two_gfns tg;
>      struct rmap_iterator ri;
> +    unsigned long put_count = 0;
>  
>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
>                   cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
> @@ -964,15 +965,6 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>          goto err_out;
>      }
>  
> -    /* Acquire an extra reference, for the freeing below to be safe. */
> -    if ( !get_page(cpage, cd) )
> -    {
> -        ret = -EOVERFLOW;
> -        mem_sharing_page_unlock(secondpg);
> -        mem_sharing_page_unlock(firstpg);
> -        goto err_out;
> -    }

Why does this fail?  Is cd not the owner of the page?

> -
>      /* Merge the lists together */
>      rmap_seed_iterator(cpage, &ri);
>      while ( (gfn = rmap_iterate(cpage, &ri)) != NULL)
> @@ -984,7 +976,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>           * Don't change the type of rmap for the client page. */
>          rmap_del(gfn, cpage, 0);
>          rmap_add(gfn, spage);
> -        put_page_and_type(cpage);
> +        put_count++;
>          d = get_domain_by_id(gfn->domain);
>          BUG_ON(!d);
>          BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
> @@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>      /* Free the client page */
>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>          put_page(cpage);
> -    put_page(cpage);
> +
> +    ASSERT(put_count);
> +    while ( put_count-- )
> +        put_page_and_type(cpage);

I don't think this is in the spirit of the fix in 0502e0adae2; the idea
there seems to have been to make sure that cpage belongs to the right
domain, and that the ownership doesn't change.  If such a race / mistake
were possible before that change, such a race / mistake would be
possible after this change, wouldn't it?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/12/19 8:47 PM, Tamas K Lengyel wrote:
> Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" introduced
> grabbing extra references for pages that drop references tied to PGC_allocated.
> However, the way these extra references were grabbed were incorrect, resulting
> in both share_pages and unshare_pages failing.

Why?  Is cd not the owner of the page?

> There actually is no need to
> grab extra references, only a reordering was needed for when the existing
> references are being released. This is in accordance to the XSA-242
> recommendation of not calling _put_page_type while also holding the page_lock
> for that page.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Roger Pau Monne <roger.pau@citrix.com>
> ---
> v2: Add assert that put_count is at least 1
> ---
>  xen/arch/x86/mm/mem_sharing.c | 32 +++++++++-----------------------
>  1 file changed, 9 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index 5ac9d8f54c..708037d91e 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -900,6 +900,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>      p2m_type_t smfn_type, cmfn_type;
>      struct two_gfns tg;
>      struct rmap_iterator ri;
> +    unsigned long put_count = 0;
>  
>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
>                   cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
> @@ -964,15 +965,6 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>          goto err_out;
>      }
>  
> -    /* Acquire an extra reference, for the freeing below to be safe. */
> -    if ( !get_page(cpage, cd) )
> -    {
> -        ret = -EOVERFLOW;
> -        mem_sharing_page_unlock(secondpg);
> -        mem_sharing_page_unlock(firstpg);
> -        goto err_out;
> -    }

Why does this fail?  Is cd not the owner of the page?

> -
>      /* Merge the lists together */
>      rmap_seed_iterator(cpage, &ri);
>      while ( (gfn = rmap_iterate(cpage, &ri)) != NULL)
> @@ -984,7 +976,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>           * Don't change the type of rmap for the client page. */
>          rmap_del(gfn, cpage, 0);
>          rmap_add(gfn, spage);
> -        put_page_and_type(cpage);
> +        put_count++;
>          d = get_domain_by_id(gfn->domain);
>          BUG_ON(!d);
>          BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
> @@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>      /* Free the client page */
>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>          put_page(cpage);
> -    put_page(cpage);
> +
> +    ASSERT(put_count);
> +    while ( put_count-- )
> +        put_page_and_type(cpage);

I don't think this is in the spirit of the fix in 0502e0adae2; the idea
there seems to have been to make sure that cpage belongs to the right
domain, and that the ownership doesn't change.  If such a race / mistake
were possible before that change, such a race / mistake would be
possible after this change, wouldn't it?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 15, 2019 at 9:21 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/12/19 8:47 PM, Tamas K Lengyel wrote:
> > Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" introduced
> > grabbing extra references for pages that drop references tied to PGC_allocated.
> > However, the way these extra references were grabbed were incorrect, resulting
> > in both share_pages and unshare_pages failing.
>
> Why?  Is cd not the owner of the page?

It's not, dom_cow is.

> > @@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >      /* Free the client page */
> >      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >          put_page(cpage);
> > -    put_page(cpage);
> > +
> > +    ASSERT(put_count);
> > +    while ( put_count-- )
> > +        put_page_and_type(cpage);
>
> I don't think this is in the spirit of the fix in 0502e0adae2; the idea
> there seems to have been to make sure that cpage belongs to the right
> domain, and that the ownership doesn't change.  If such a race / mistake
> were possible before that change, such a race / mistake would be
> possible after this change, wouldn't it?

I don't have an answer to your question. AFAIU this is just to ensure
that the page isn't dropped before test_and_clear_bit is used on the
page.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 15, 2019 at 9:21 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/12/19 8:47 PM, Tamas K Lengyel wrote:
> > Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" introduced
> > grabbing extra references for pages that drop references tied to PGC_allocated.
> > However, the way these extra references were grabbed were incorrect, resulting
> > in both share_pages and unshare_pages failing.
>
> Why?  Is cd not the owner of the page?

It's not, dom_cow is.

> > @@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >      /* Free the client page */
> >      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >          put_page(cpage);
> > -    put_page(cpage);
> > +
> > +    ASSERT(put_count);
> > +    while ( put_count-- )
> > +        put_page_and_type(cpage);
>
> I don't think this is in the spirit of the fix in 0502e0adae2; the idea
> there seems to have been to make sure that cpage belongs to the right
> domain, and that the ownership doesn't change.  If such a race / mistake
> were possible before that change, such a race / mistake would be
> possible after this change, wouldn't it?

I don't have an answer to your question. AFAIU this is just to ensure
that the page isn't dropped before test_and_clear_bit is used on the
page.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

> On Apr 15, 2019, at 4:31 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
> 
> On Mon, Apr 15, 2019 at 9:21 AM George Dunlap <george.dunlap@citrix.com> wrote:
>> 
>> On 4/12/19 8:47 PM, Tamas K Lengyel wrote:
>>> Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" introduced
>>> grabbing extra references for pages that drop references tied to PGC_allocated.
>>> However, the way these extra references were grabbed were incorrect, resulting
>>> in both share_pages and unshare_pages failing.
>> 
>> Why?  Is cd not the owner of the page?
> 
> It's not, dom_cow is.
> 
>>> @@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>     /* Free the client page */
>>>     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>>>         put_page(cpage);
>>> -    put_page(cpage);
>>> +
>>> +    ASSERT(put_count);
>>> +    while ( put_count-- )
>>> +        put_page_and_type(cpage);
>> 
>> I don't think this is in the spirit of the fix in 0502e0adae2; the idea
>> there seems to have been to make sure that cpage belongs to the right
>> domain, and that the ownership doesn't change.  If such a race / mistake
>> were possible before that change, such a race / mistake would be
>> possible after this change, wouldn't it?
> 
> I don't have an answer to your question. AFAIU this is just to ensure
> that the page isn't dropped before test_and_clear_bit is used on the
> page.

Right, but why would that be a bad thing?

I think it’s to avoid races like this:

P1: Gets pointer to page A, owned by domain M
P2: Gets pointer to page A, owned by domain M
P2: test_and_clear_bit(PGC_allocated) succeeds
P2: put_page()
  -> page freed
  -> Page re-allocated to domain N
P1: test_and_clear_bit(PGC_allocated) succeeds
P1: put_page() 
  -> page freed ##

Now P1 incorrectly “frees” a page owned by domain N.

If both P1 and P2 call get_page(A, M) first, and drop that reference afterwards, this race can’t happen.

Jan, you’re the author of that patch — is that not the case?

Could a similar race to the above occur in this code?  It looks like it could have before 0502e0adae2 for sure, since all references and locks were dropped.  It’s not clear to me whether mem_sharing_page_lock() will protect against ownership changes.

*If* that assertion you added is always true, that there is always at least one reference in the rmap that will be held; *and* those references are always “held” on behalf of dom_cow, then I think your version will probably be safe (as the reference in the rmap will prevent it from changing ownership until the final put_page_and_type()).  But if #2 ever false for any reason (even due to a bug in Xen), then this will open up a potential security issue (since ASSERT()s don’t trigger in production code).

A couple of options:
* Make it a BUG_ON(); that would change a potential privilege escalation into a DoS
* Only clear PGC_allocated if put_count > 0.  This changes a potential privilege escalation into a slow memory leak
* If put_count == 0, grab an extra reference and drop it afterwards (with an ASSERT_UNREACHABLE() to help catch errors early).  This avoids any XSA, but makes the code a bit uglier.
* Leave the code as it is; just change the get_page() to grabbing dom_cow rather than cd.  This avoids any XSA, but adds an extra get / put page which will be pure overhead in the common case.

Thoughts?

 -George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

> On Apr 15, 2019, at 4:31 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
> 
> On Mon, Apr 15, 2019 at 9:21 AM George Dunlap <george.dunlap@citrix.com> wrote:
>> 
>> On 4/12/19 8:47 PM, Tamas K Lengyel wrote:
>>> Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" introduced
>>> grabbing extra references for pages that drop references tied to PGC_allocated.
>>> However, the way these extra references were grabbed were incorrect, resulting
>>> in both share_pages and unshare_pages failing.
>> 
>> Why?  Is cd not the owner of the page?
> 
> It's not, dom_cow is.
> 
>>> @@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>     /* Free the client page */
>>>     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>>>         put_page(cpage);
>>> -    put_page(cpage);
>>> +
>>> +    ASSERT(put_count);
>>> +    while ( put_count-- )
>>> +        put_page_and_type(cpage);
>> 
>> I don't think this is in the spirit of the fix in 0502e0adae2; the idea
>> there seems to have been to make sure that cpage belongs to the right
>> domain, and that the ownership doesn't change.  If such a race / mistake
>> were possible before that change, such a race / mistake would be
>> possible after this change, wouldn't it?
> 
> I don't have an answer to your question. AFAIU this is just to ensure
> that the page isn't dropped before test_and_clear_bit is used on the
> page.

Right, but why would that be a bad thing?

I think it’s to avoid races like this:

P1: Gets pointer to page A, owned by domain M
P2: Gets pointer to page A, owned by domain M
P2: test_and_clear_bit(PGC_allocated) succeeds
P2: put_page()
  -> page freed
  -> Page re-allocated to domain N
P1: test_and_clear_bit(PGC_allocated) succeeds
P1: put_page() 
  -> page freed ##

Now P1 incorrectly “frees” a page owned by domain N.

If both P1 and P2 call get_page(A, M) first, and drop that reference afterwards, this race can’t happen.

Jan, you’re the author of that patch — is that not the case?

Could a similar race to the above occur in this code?  It looks like it could have before 0502e0adae2 for sure, since all references and locks were dropped.  It’s not clear to me whether mem_sharing_page_lock() will protect against ownership changes.

*If* that assertion you added is always true, that there is always at least one reference in the rmap that will be held; *and* those references are always “held” on behalf of dom_cow, then I think your version will probably be safe (as the reference in the rmap will prevent it from changing ownership until the final put_page_and_type()).  But if #2 ever false for any reason (even due to a bug in Xen), then this will open up a potential security issue (since ASSERT()s don’t trigger in production code).

A couple of options:
* Make it a BUG_ON(); that would change a potential privilege escalation into a DoS
* Only clear PGC_allocated if put_count > 0.  This changes a potential privilege escalation into a slow memory leak
* If put_count == 0, grab an extra reference and drop it afterwards (with an ASSERT_UNREACHABLE() to help catch errors early).  This avoids any XSA, but makes the code a bit uglier.
* Leave the code as it is; just change the get_page() to grabbing dom_cow rather than cd.  This avoids any XSA, but adds an extra get / put page which will be pure overhead in the common case.

Thoughts?

 -George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 15, 2019 at 11:28 AM George Dunlap <George.Dunlap@citrix.com> wrote:
>
>
>
> > On Apr 15, 2019, at 4:31 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
> >
> > On Mon, Apr 15, 2019 at 9:21 AM George Dunlap <george.dunlap@citrix.com> wrote:
> >>
> >> On 4/12/19 8:47 PM, Tamas K Lengyel wrote:
> >>> Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" introduced
> >>> grabbing extra references for pages that drop references tied to PGC_allocated.
> >>> However, the way these extra references were grabbed were incorrect, resulting
> >>> in both share_pages and unshare_pages failing.
> >>
> >> Why?  Is cd not the owner of the page?
> >
> > It's not, dom_cow is.
> >
> >>> @@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >>>     /* Free the client page */
> >>>     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >>>         put_page(cpage);
> >>> -    put_page(cpage);
> >>> +
> >>> +    ASSERT(put_count);
> >>> +    while ( put_count-- )
> >>> +        put_page_and_type(cpage);
> >>
> >> I don't think this is in the spirit of the fix in 0502e0adae2; the idea
> >> there seems to have been to make sure that cpage belongs to the right
> >> domain, and that the ownership doesn't change.  If such a race / mistake
> >> were possible before that change, such a race / mistake would be
> >> possible after this change, wouldn't it?
> >
> > I don't have an answer to your question. AFAIU this is just to ensure
> > that the page isn't dropped before test_and_clear_bit is used on the
> > page.
>
> Right, but why would that be a bad thing?
>
> I think it’s to avoid races like this:
>
> P1: Gets pointer to page A, owned by domain M
> P2: Gets pointer to page A, owned by domain M
> P2: test_and_clear_bit(PGC_allocated) succeeds
> P2: put_page()
>   -> page freed
>   -> Page re-allocated to domain N
> P1: test_and_clear_bit(PGC_allocated) succeeds
> P1: put_page()
>   -> page freed ##
>
> Now P1 incorrectly “frees” a page owned by domain N.
>
> If both P1 and P2 call get_page(A, M) first, and drop that reference afterwards, this race can’t happen.
>
> Jan, you’re the author of that patch — is that not the case?
>
> Could a similar race to the above occur in this code?  It looks like it could have before 0502e0adae2 for sure, since all references and locks were dropped.  It’s not clear to me whether mem_sharing_page_lock() will protect against ownership changes.
>
> *If* that assertion you added is always true, that there is always at least one reference in the rmap that will be held; *and* those references are always “held” on behalf of dom_cow, then I think your version will probably be safe (as the reference in the rmap will prevent it from changing ownership until the final put_page_and_type()).  But if #2 ever false for any reason (even due to a bug in Xen), then this will open up a potential security issue (since ASSERT()s don’t trigger in production code).
>
> A couple of options:
> * Make it a BUG_ON(); that would change a potential privilege escalation into a DoS

I'm fine with changing that ASSERT into a BUG_ON.

> * Only clear PGC_allocated if put_count > 0.  This changes a potential privilege escalation into a slow memory leak
> * If put_count == 0, grab an extra reference and drop it afterwards (with an ASSERT_UNREACHABLE() to help catch errors early).  This avoids any XSA, but makes the code a bit uglier.
> * Leave the code as it is; just change the get_page() to grabbing dom_cow rather than cd.  This avoids any XSA, but adds an extra get / put page which will be pure overhead in the common case.

Simply changing to get_page with dom_cow is not enough, I ran into
issues where the system still deadlocks due to the ordering of
put_page_and_type being called before mem_sharing_page_unlock. So the
ordering still has to be changed.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 15, 2019 at 11:28 AM George Dunlap <George.Dunlap@citrix.com> wrote:
>
>
>
> > On Apr 15, 2019, at 4:31 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
> >
> > On Mon, Apr 15, 2019 at 9:21 AM George Dunlap <george.dunlap@citrix.com> wrote:
> >>
> >> On 4/12/19 8:47 PM, Tamas K Lengyel wrote:
> >>> Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" introduced
> >>> grabbing extra references for pages that drop references tied to PGC_allocated.
> >>> However, the way these extra references were grabbed were incorrect, resulting
> >>> in both share_pages and unshare_pages failing.
> >>
> >> Why?  Is cd not the owner of the page?
> >
> > It's not, dom_cow is.
> >
> >>> @@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >>>     /* Free the client page */
> >>>     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >>>         put_page(cpage);
> >>> -    put_page(cpage);
> >>> +
> >>> +    ASSERT(put_count);
> >>> +    while ( put_count-- )
> >>> +        put_page_and_type(cpage);
> >>
> >> I don't think this is in the spirit of the fix in 0502e0adae2; the idea
> >> there seems to have been to make sure that cpage belongs to the right
> >> domain, and that the ownership doesn't change.  If such a race / mistake
> >> were possible before that change, such a race / mistake would be
> >> possible after this change, wouldn't it?
> >
> > I don't have an answer to your question. AFAIU this is just to ensure
> > that the page isn't dropped before test_and_clear_bit is used on the
> > page.
>
> Right, but why would that be a bad thing?
>
> I think it’s to avoid races like this:
>
> P1: Gets pointer to page A, owned by domain M
> P2: Gets pointer to page A, owned by domain M
> P2: test_and_clear_bit(PGC_allocated) succeeds
> P2: put_page()
>   -> page freed
>   -> Page re-allocated to domain N
> P1: test_and_clear_bit(PGC_allocated) succeeds
> P1: put_page()
>   -> page freed ##
>
> Now P1 incorrectly “frees” a page owned by domain N.
>
> If both P1 and P2 call get_page(A, M) first, and drop that reference afterwards, this race can’t happen.
>
> Jan, you’re the author of that patch — is that not the case?
>
> Could a similar race to the above occur in this code?  It looks like it could have before 0502e0adae2 for sure, since all references and locks were dropped.  It’s not clear to me whether mem_sharing_page_lock() will protect against ownership changes.
>
> *If* that assertion you added is always true, that there is always at least one reference in the rmap that will be held; *and* those references are always “held” on behalf of dom_cow, then I think your version will probably be safe (as the reference in the rmap will prevent it from changing ownership until the final put_page_and_type()).  But if #2 ever false for any reason (even due to a bug in Xen), then this will open up a potential security issue (since ASSERT()s don’t trigger in production code).
>
> A couple of options:
> * Make it a BUG_ON(); that would change a potential privilege escalation into a DoS

I'm fine with changing that ASSERT into a BUG_ON.

> * Only clear PGC_allocated if put_count > 0.  This changes a potential privilege escalation into a slow memory leak
> * If put_count == 0, grab an extra reference and drop it afterwards (with an ASSERT_UNREACHABLE() to help catch errors early).  This avoids any XSA, but makes the code a bit uglier.
> * Leave the code as it is; just change the get_page() to grabbing dom_cow rather than cd.  This avoids any XSA, but adds an extra get / put page which will be pure overhead in the common case.

Simply changing to get_page with dom_cow is not enough, I ran into
issues where the system still deadlocks due to the ordering of
put_page_and_type being called before mem_sharing_page_unlock. So the
ordering still has to be changed.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Jan Beulich]

>>> On 15.04.19 at 19:28, <George.Dunlap@citrix.com> wrote:
>> On Apr 15, 2019, at 4:31 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>> On Mon, Apr 15, 2019 at 9:21 AM George Dunlap <george.dunlap@citrix.com> wrote:
>>> On 4/12/19 8:47 PM, Tamas K Lengyel wrote:
>>>> Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" introduced
>>>> grabbing extra references for pages that drop references tied to PGC_allocated.
>>>> However, the way these extra references were grabbed were incorrect, resulting
>>>> in both share_pages and unshare_pages failing.
>>> 
>>> Why?  Is cd not the owner of the page?
>> 
>> It's not, dom_cow is.
>> 
>>>> @@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>>     /* Free the client page */
>>>>     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>>>>         put_page(cpage);
>>>> -    put_page(cpage);
>>>> +
>>>> +    ASSERT(put_count);
>>>> +    while ( put_count-- )
>>>> +        put_page_and_type(cpage);
>>> 
>>> I don't think this is in the spirit of the fix in 0502e0adae2; the idea
>>> there seems to have been to make sure that cpage belongs to the right
>>> domain, and that the ownership doesn't change.  If such a race / mistake
>>> were possible before that change, such a race / mistake would be
>>> possible after this change, wouldn't it?
>> 
>> I don't have an answer to your question. AFAIU this is just to ensure
>> that the page isn't dropped before test_and_clear_bit is used on the
>> page.
> 
> Right, but why would that be a bad thing?
> 
> I think it’s to avoid races like this:
> 
> P1: Gets pointer to page A, owned by domain M
> P2: Gets pointer to page A, owned by domain M
> P2: test_and_clear_bit(PGC_allocated) succeeds
> P2: put_page()
>   -> page freed
>   -> Page re-allocated to domain N
> P1: test_and_clear_bit(PGC_allocated) succeeds
> P1: put_page() 
>   -> page freed ##
> 
> Now P1 incorrectly “frees” a page owned by domain N.
> 
> If both P1 and P2 call get_page(A, M) first, and drop that reference 
> afterwards, this race can’t happen.
> 
> Jan, you’re the author of that patch — is that not the case?

Yes indeed, that's why I did add the extra get_page() (not
realizing the owning domain is the wrong one).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Jan Beulich]

>>> On 15.04.19 at 19:28, <George.Dunlap@citrix.com> wrote:
>> On Apr 15, 2019, at 4:31 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>> On Mon, Apr 15, 2019 at 9:21 AM George Dunlap <george.dunlap@citrix.com> wrote:
>>> On 4/12/19 8:47 PM, Tamas K Lengyel wrote:
>>>> Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" introduced
>>>> grabbing extra references for pages that drop references tied to PGC_allocated.
>>>> However, the way these extra references were grabbed were incorrect, resulting
>>>> in both share_pages and unshare_pages failing.
>>> 
>>> Why?  Is cd not the owner of the page?
>> 
>> It's not, dom_cow is.
>> 
>>>> @@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>>     /* Free the client page */
>>>>     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>>>>         put_page(cpage);
>>>> -    put_page(cpage);
>>>> +
>>>> +    ASSERT(put_count);
>>>> +    while ( put_count-- )
>>>> +        put_page_and_type(cpage);
>>> 
>>> I don't think this is in the spirit of the fix in 0502e0adae2; the idea
>>> there seems to have been to make sure that cpage belongs to the right
>>> domain, and that the ownership doesn't change.  If such a race / mistake
>>> were possible before that change, such a race / mistake would be
>>> possible after this change, wouldn't it?
>> 
>> I don't have an answer to your question. AFAIU this is just to ensure
>> that the page isn't dropped before test_and_clear_bit is used on the
>> page.
> 
> Right, but why would that be a bad thing?
> 
> I think it’s to avoid races like this:
> 
> P1: Gets pointer to page A, owned by domain M
> P2: Gets pointer to page A, owned by domain M
> P2: test_and_clear_bit(PGC_allocated) succeeds
> P2: put_page()
>   -> page freed
>   -> Page re-allocated to domain N
> P1: test_and_clear_bit(PGC_allocated) succeeds
> P1: put_page() 
>   -> page freed ##
> 
> Now P1 incorrectly “frees” a page owned by domain N.
> 
> If both P1 and P2 call get_page(A, M) first, and drop that reference 
> afterwards, this race can’t happen.
> 
> Jan, you’re the author of that patch — is that not the case?

Yes indeed, that's why I did add the extra get_page() (not
realizing the owning domain is the wrong one).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Jan Beulich]

>>> On 15.04.19 at 19:51, <tamas@tklengyel.com> wrote:
> Simply changing to get_page with dom_cow is not enough, I ran into
> issues where the system still deadlocks due to the ordering of
> put_page_and_type being called before mem_sharing_page_unlock. So the
> ordering still has to be changed.

Dead locks because of the ordering of releases (or locks or alike),
not acquires? That looks suspicious to me, i.e. suggests yet
another problem elsewhere.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Jan Beulich]

>>> On 15.04.19 at 19:51, <tamas@tklengyel.com> wrote:
> Simply changing to get_page with dom_cow is not enough, I ran into
> issues where the system still deadlocks due to the ordering of
> put_page_and_type being called before mem_sharing_page_unlock. So the
> ordering still has to be changed.

Dead locks because of the ordering of releases (or locks or alike),
not acquires? That looks suspicious to me, i.e. suggests yet
another problem elsewhere.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Thu, Apr 25, 2019 at 5:14 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 15.04.19 at 19:51, <tamas@tklengyel.com> wrote:
> > Simply changing to get_page with dom_cow is not enough, I ran into
> > issues where the system still deadlocks due to the ordering of
> > put_page_and_type being called before mem_sharing_page_unlock. So the
> > ordering still has to be changed.
>
> Dead locks because of the ordering of releases (or locks or alike),
> not acquires? That looks suspicious to me, i.e. suggests yet
> another problem elsewhere.

FWIW page_lock has this warning:

 * We must never call _put_page_type() while holding a page_lock() for
 * that page; doing so may cause a deadlock under the right
 * conditions.

That said, I'm working on getting rid of using page_lock altogether
because it just over-complicates things for no good reason. Should be
able to send the new patches today.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Thu, Apr 25, 2019 at 5:14 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 15.04.19 at 19:51, <tamas@tklengyel.com> wrote:
> > Simply changing to get_page with dom_cow is not enough, I ran into
> > issues where the system still deadlocks due to the ordering of
> > put_page_and_type being called before mem_sharing_page_unlock. So the
> > ordering still has to be changed.
>
> Dead locks because of the ordering of releases (or locks or alike),
> not acquires? That looks suspicious to me, i.e. suggests yet
> another problem elsewhere.

FWIW page_lock has this warning:

 * We must never call _put_page_type() while holding a page_lock() for
 * that page; doing so may cause a deadlock under the right
 * conditions.

That said, I'm working on getting rid of using page_lock altogether
because it just over-complicates things for no good reason. Should be
able to send the new patches today.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Jan Beulich]

>>> On 25.04.19 at 15:33, <tamas@tklengyel.com> wrote:
> On Thu, Apr 25, 2019 at 5:14 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> On 15.04.19 at 19:51, <tamas@tklengyel.com> wrote:
>> > Simply changing to get_page with dom_cow is not enough, I ran into
>> > issues where the system still deadlocks due to the ordering of
>> > put_page_and_type being called before mem_sharing_page_unlock. So the
>> > ordering still has to be changed.
>>
>> Dead locks because of the ordering of releases (or locks or alike),
>> not acquires? That looks suspicious to me, i.e. suggests yet
>> another problem elsewhere.
> 
> FWIW page_lock has this warning:
> 
>  * We must never call _put_page_type() while holding a page_lock() for
>  * that page; doing so may cause a deadlock under the right
>  * conditions.

Oh, of course. I once again forgot that
mem_sharing_page_unlock() calls page_unlock().

> That said, I'm working on getting rid of using page_lock altogether
> because it just over-complicates things for no good reason. Should be
> able to send the new patches today.

Oh, that's good news.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Jan Beulich]

>>> On 25.04.19 at 15:33, <tamas@tklengyel.com> wrote:
> On Thu, Apr 25, 2019 at 5:14 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> On 15.04.19 at 19:51, <tamas@tklengyel.com> wrote:
>> > Simply changing to get_page with dom_cow is not enough, I ran into
>> > issues where the system still deadlocks due to the ordering of
>> > put_page_and_type being called before mem_sharing_page_unlock. So the
>> > ordering still has to be changed.
>>
>> Dead locks because of the ordering of releases (or locks or alike),
>> not acquires? That looks suspicious to me, i.e. suggests yet
>> another problem elsewhere.
> 
> FWIW page_lock has this warning:
> 
>  * We must never call _put_page_type() while holding a page_lock() for
>  * that page; doing so may cause a deadlock under the right
>  * conditions.

Oh, of course. I once again forgot that
mem_sharing_page_unlock() calls page_unlock().

> That said, I'm working on getting rid of using page_lock altogether
> because it just over-complicates things for no good reason. Should be
> able to send the new patches today.

Oh, that's good news.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel