WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-changelog

[Xen-changelog] [xen-unstable] Be careful with page_get_owner() now that

To: xen-changelog@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-changelog] [xen-unstable] Be careful with page_get_owner() now that owner field can be clobbered
From: Xen patchbot-unstable <patchbot-unstable@xxxxxxxxxxxxxxxxxxx>
Date: Sat, 07 Mar 2009 06:35:10 -0800
Delivery-date: Sat, 07 Mar 2009 06:34:57 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
List-help: <mailto:xen-changelog-request@lists.xensource.com?subject=help>
List-id: BK change log <xen-changelog.lists.xensource.com>
List-post: <mailto:xen-changelog@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-changelog>, <mailto:xen-changelog-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-changelog>, <mailto:xen-changelog-request@lists.xensource.com?subject=unsubscribe>
Reply-to: xen-devel@xxxxxxxxxxxxxxxxxxx
Sender: xen-changelog-bounces@xxxxxxxxxxxxxxxxxxx
# HG changeset patch
# User Keir Fraser <keir.fraser@xxxxxxxxxx>
# Date 1236176930 0
# Node ID 4b7d638a8b89b6d49094e77d6295c6d8ffafc192
# Parent  7f573cb76db41a9fc46052867b02e9e0c107aa86
Be careful with page_get_owner() now that owner field can be clobbered
by some users. Introduce get_page_owner_and_reference() where that can
be more useful.

Signed-off-by: Keir Fraser <keir.fraser@xxxxxxxxxx>
---
 xen/arch/x86/mm.c         |   92 +++++++++++++++++++++++++++++-----------------
 xen/common/grant_table.c  |   41 ++++++++++++--------
 xen/common/xencomm.c      |   28 +++++---------
 xen/include/asm-ia64/mm.h |   19 ++++++---
 xen/include/asm-x86/mm.h  |    1 
 5 files changed, 106 insertions(+), 75 deletions(-)

diff -r 7f573cb76db4 -r 4b7d638a8b89 xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c Tue Mar 03 13:22:28 2009 +0000
+++ b/xen/arch/x86/mm.c Wed Mar 04 14:28:50 2009 +0000
@@ -371,14 +371,14 @@ void share_xen_page_with_privileged_gues
 #else
 /*
  * In debug builds we shadow a selection of <4GB PDPTs to exercise code paths.
- * We cannot safely shadow the idle page table, nor shadow (v1) page tables
- * (detected by lack of an owning domain). As required for correctness, we
+ * We cannot safely shadow the idle page table, nor shadow page tables
+ * (detected by zero reference count). As required for correctness, we
  * always shadow PDPTs above 4GB.
  */
-#define l3tab_needs_shadow(mfn)                         \
-    (((((mfn) << PAGE_SHIFT) != __pa(idle_pg_table)) && \
-      (page_get_owner(mfn_to_page(mfn)) != NULL) &&     \
-      ((mfn) & 1)) || /* odd MFNs are shadowed */       \
+#define l3tab_needs_shadow(mfn)                          \
+    (((((mfn) << PAGE_SHIFT) != __pa(idle_pg_table)) &&  \
+      (mfn_to_page(mfn)->count_info & PGC_count_mask) && \
+      ((mfn) & 1)) || /* odd MFNs are shadowed */        \
      ((mfn) >= 0x100000))
 #endif
 
@@ -690,7 +690,16 @@ get_##level##_linear_pagetable(         
 
 int is_iomem_page(unsigned long mfn)
 {
-    return (!mfn_valid(mfn) || (page_get_owner(mfn_to_page(mfn)) == dom_io));
+    struct page_info *page;
+
+    if ( !mfn_valid(mfn) )
+        return 1;
+
+    /* Caller must know that it is an iomem page, or a reference is held. */
+    page = mfn_to_page(mfn);
+    ASSERT((page->count_info & PGC_count_mask) != 0);
+
+    return (page_get_owner(page) == dom_io);
 }
 
 
@@ -703,7 +712,6 @@ get_page_from_l1e(
     uint32_t l1f = l1e_get_flags(l1e);
     struct vcpu *curr = current;
     struct domain *owner;
-    int okay;
 
     if ( !(l1f & _PAGE_PRESENT) )
         return 1;
@@ -714,8 +722,13 @@ get_page_from_l1e(
         return 0;
     }
 
-    if ( is_iomem_page(mfn) )
-    {
+    if ( !mfn_valid(mfn) ||
+         (owner = page_get_owner_and_reference(page)) == dom_io )
+    {
+        /* Only needed the reference to confirm dom_io ownership. */
+        if ( mfn_valid(mfn) )
+            put_page(page);
+
         /* DOMID_IO reverts to caller for privilege checks. */
         if ( d == dom_io )
             d = curr->domain;
@@ -730,6 +743,9 @@ get_page_from_l1e(
 
         return 1;
     }
+
+    if ( owner == NULL )
+        goto could_not_pin;
 
     /*
      * Let privileged domains transfer the right to map their target
@@ -737,27 +753,20 @@ get_page_from_l1e(
      * until pvfb supports granted mappings. At that time this minor hack
      * can go away.
      */
-    owner = page_get_owner(page);
-    if ( unlikely(d != owner) && (owner != NULL) &&
-         (d != curr->domain) && IS_PRIV_FOR(d, owner) )
+    if ( unlikely(d != owner) && (d != curr->domain) && IS_PRIV_FOR(d, owner) )
         d = owner;
 
     /* Foreign mappings into guests in shadow external mode don't
      * contribute to writeable mapping refcounts.  (This allows the
      * qemu-dm helper process in dom0 to map the domain's memory without
      * messing up the count of "real" writable mappings.) */
-    okay = get_data_page(
-        page, d,
-        (l1f & _PAGE_RW) && !(paging_mode_external(d) && (d != curr->domain)));
-    if ( !okay )
-    {
-        MEM_LOG("Error getting mfn %lx (pfn %lx) from L1 entry %" PRIpte
-                " for dom%d",
-                mfn, get_gpfn_from_mfn(mfn),
-                l1e_get_intpte(l1e), d->domain_id);
-    }
-    else if ( pte_flags_to_cacheattr(l1f) !=
-              ((page->count_info >> PGC_cacheattr_base) & 7) )
+    if ( (l1f & _PAGE_RW) &&
+         !(paging_mode_external(d) && (d != curr->domain)) &&
+         !get_page_type(page, PGT_writable_page) )
+        goto could_not_pin;
+
+    if ( pte_flags_to_cacheattr(l1f) !=
+         ((page->count_info >> PGC_cacheattr_base) & 7) )
     {
         unsigned long x, nx, y = page->count_info;
         unsigned long cacheattr = pte_flags_to_cacheattr(l1f);
@@ -786,7 +795,16 @@ get_page_from_l1e(
 #endif
     }
 
-    return okay;
+    return 1;
+
+ could_not_pin:
+    MEM_LOG("Error getting mfn %lx (pfn %lx) from L1 entry %" PRIpte
+            " for dom%d",
+            mfn, get_gpfn_from_mfn(mfn),
+            l1e_get_intpte(l1e), d->domain_id);
+    if ( owner != NULL )
+        put_page(page);
+    return 0;
 }
 
 
@@ -1174,7 +1192,7 @@ static int create_pae_xen_mappings(struc
     for ( i = 0; i < PDPT_L2_ENTRIES; i++ )
     {
         l2e = l2e_from_page(
-            virt_to_page(page_get_owner(page)->arch.mm_perdomain_pt) + i,
+            virt_to_page(d->arch.mm_perdomain_pt) + i,
             __PAGE_HYPERVISOR);
         l2e_write(&pl2e[l2_table_offset(PERDOMAIN_VIRT_START) + i], l2e);
     }
@@ -1924,7 +1942,7 @@ void put_page(struct page_info *page)
 }
 
 
-int get_page(struct page_info *page, struct domain *domain)
+struct domain *page_get_owner_and_reference(struct page_info *page)
 {
     unsigned long x, y = page->count_info;
 
@@ -1933,22 +1951,29 @@ int get_page(struct page_info *page, str
         if ( unlikely((x & PGC_count_mask) == 0) ||  /* Not allocated? */
              /* Keep one spare reference to be acquired by get_page_light(). */
              unlikely(((x + 2) & PGC_count_mask) <= 1) ) /* Overflow? */
-            goto fail;
+            return NULL;
     }
     while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
 
-    if ( likely(page_get_owner(page) == domain) )
+    return page_get_owner(page);
+}
+
+
+int get_page(struct page_info *page, struct domain *domain)
+{
+    struct domain *owner = page_get_owner_and_reference(page);
+
+    if ( likely(owner == domain) )
         return 1;
 
     put_page(page);
 
- fail:
     if ( !_shadow_mode_refcounts(domain) && !domain->is_dying )
         gdprintk(XENLOG_INFO,
                  "Error pfn %lx: rd=%p, od=%p, caf=%08lx, taf=%"
                  PRtype_info "\n",
-                 page_to_mfn(page), domain, page_get_owner(page),
-                 y, page->u.inuse.type_info);
+                 page_to_mfn(page), domain, owner,
+                 page->count_info, page->u.inuse.type_info);
     return 0;
 }
 
@@ -1973,7 +1998,6 @@ static void get_page_light(struct page_i
     }
     while ( unlikely(y != x) );
 }
-
 
 static int alloc_page_type(struct page_info *page, unsigned long type,
                            int preemptible)
diff -r 7f573cb76db4 -r 4b7d638a8b89 xen/common/grant_table.c
--- a/xen/common/grant_table.c  Tue Mar 03 13:22:28 2009 +0000
+++ b/xen/common/grant_table.c  Wed Mar 04 14:28:50 2009 +0000
@@ -195,7 +195,7 @@ __gnttab_map_grant_ref(
 __gnttab_map_grant_ref(
     struct gnttab_map_grant_ref *op)
 {
-    struct domain *ld, *rd;
+    struct domain *ld, *rd, *owner;
     struct vcpu   *led;
     int            handle;
     unsigned long  frame = 0, nr_gets = 0;
@@ -336,8 +336,13 @@ __gnttab_map_grant_ref(
 
     spin_unlock(&rd->grant_table->lock);
 
-    if ( is_iomem_page(frame) )
-    {
+    if ( !mfn_valid(frame) ||
+         (owner = page_get_owner_and_reference(mfn_to_page(frame))) == dom_io )
+    {
+        /* Only needed the reference to confirm dom_io ownership. */
+        if ( mfn_valid(frame) )
+            put_page(mfn_to_page(frame));
+
         if ( !iomem_access_permitted(rd, frame, frame) )
         {
             gdprintk(XENLOG_WARNING,
@@ -352,20 +357,11 @@ __gnttab_map_grant_ref(
         if ( rc != GNTST_okay )
             goto undo_out;
     }
-    else
-    {
-        if ( unlikely(!mfn_valid(frame)) ||
-             unlikely(!(gnttab_host_mapping_get_page_type(op, ld, rd) ?
-                        get_page_and_type(mfn_to_page(frame), rd,
-                                          PGT_writable_page) :
-                        get_page(mfn_to_page(frame), rd))) )
-        {
-            if ( !rd->is_dying )
-                gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n",
-                         frame);
-            rc = GNTST_general_error;
-            goto undo_out;
-        }
+    else if ( owner == rd )
+    {
+        if ( gnttab_host_mapping_get_page_type(op, ld, rd) &&
+             !get_page_type(mfn_to_page(frame), PGT_writable_page) )
+            goto could_not_pin;
 
         nr_gets++;
         if ( op->flags & GNTMAP_host_map )
@@ -382,6 +378,17 @@ __gnttab_map_grant_ref(
                     get_page_type(mfn_to_page(frame), PGT_writable_page);
             }
         }
+    }
+    else
+    {
+    could_not_pin:
+        if ( !rd->is_dying )
+            gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n",
+                     frame);
+        if ( owner != NULL )
+            put_page(mfn_to_page(frame));
+        rc = GNTST_general_error;
+        goto undo_out;
     }
 
     if ( need_iommu(ld) &&
diff -r 7f573cb76db4 -r 4b7d638a8b89 xen/common/xencomm.c
--- a/xen/common/xencomm.c      Tue Mar 03 13:22:28 2009 +0000
+++ b/xen/common/xencomm.c      Wed Mar 04 14:28:50 2009 +0000
@@ -51,24 +51,16 @@ xencomm_get_page(unsigned long paddr, st
         return -EFAULT;
         
     *page = maddr_to_page(maddr);
-    if ( get_page(*page, current->domain) == 0 )
-    {
-        if ( page_get_owner(*page) != current->domain )
-        {
-            /*
-             * This page might be a page granted by another domain, or
-             * this page is freed with decrease reservation hypercall at
-             * the same time.
-             */
-            gdprintk(XENLOG_WARNING,
-                     "bad page is passed. paddr 0x%lx maddr 0x%lx\n",
-                     paddr, maddr);
-            return -EFAULT;
-        }
-
-        /* Try again. */
-        cpu_relax();
-        return -EAGAIN;
+    if ( !get_page(*page, current->domain) == 0 )
+    {
+        /*
+         * This page might be a page granted by another domain, or this page 
+         * is freed with decrease reservation hypercall at the same time.
+         */
+        gdprintk(XENLOG_WARNING,
+                 "bad page is passed. paddr 0x%lx maddr 0x%lx\n",
+                 paddr, maddr);
+        return -EFAULT;
     }
 
     return 0;
diff -r 7f573cb76db4 -r 4b7d638a8b89 xen/include/asm-ia64/mm.h
--- a/xen/include/asm-ia64/mm.h Tue Mar 03 13:22:28 2009 +0000
+++ b/xen/include/asm-ia64/mm.h Wed Mar 04 14:28:50 2009 +0000
@@ -200,9 +200,7 @@ static inline void put_page(struct page_
         free_domheap_page(page);
 }
 
-/* count_info and ownership are checked atomically. */
-static inline int get_page(struct page_info *page,
-                           struct domain *domain)
+static inline page_get_owner_and_reference(struct page_info *page)
 {
     unsigned long x, y = page->count_info;
 
@@ -210,12 +208,21 @@ static inline int get_page(struct page_i
         x = y;
         if (unlikely((x & PGC_count_mask) == 0) ||  /* Not allocated? */
             unlikely(((x + 1) & PGC_count_mask) == 0) ) {/* Count overflow? */
-            goto fail;
+            return NULL;
         }
         y = cmpxchg_acq(&page->count_info, x, x + 1);
     } while (unlikely(y != x));
 
-    if (likely(page_get_owner(page) == domain))
+    return page_get_owner(page);
+}
+
+/* count_info and ownership are checked atomically. */
+static inline int get_page(struct page_info *page,
+                           struct domain *domain)
+{
+    struct domain *owner = page_get_owner_and_reference(page);
+
+    if (likely(owner == domain))
         return 1;
 
     put_page(page);
@@ -224,7 +231,7 @@ fail:
     gdprintk(XENLOG_INFO,
              "Error pfn %lx: rd=%p, od=%p, caf=%016lx, taf=%" PRtype_info "\n",
              page_to_mfn(page), domain,
-             page_get_owner(page), y, page->u.inuse.type_info);
+             owner, page->count_info, page->u.inuse.type_info);
     return 0;
 }
 
diff -r 7f573cb76db4 -r 4b7d638a8b89 xen/include/asm-x86/mm.h
--- a/xen/include/asm-x86/mm.h  Tue Mar 03 13:22:28 2009 +0000
+++ b/xen/include/asm-x86/mm.h  Wed Mar 04 14:28:50 2009 +0000
@@ -258,6 +258,7 @@ void cleanup_page_cacheattr(struct page_
 
 int is_iomem_page(unsigned long mfn);
 
+struct domain *page_get_owner_and_reference(struct page_info *page);
 void put_page(struct page_info *page);
 int  get_page(struct page_info *page, struct domain *domain);
 void put_page_type(struct page_info *page);

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

<Prev in Thread] Current Thread [Next in Thread>
  • [Xen-changelog] [xen-unstable] Be careful with page_get_owner() now that owner field can be clobbered, Xen patchbot-unstable <=