All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Jan Beulich" <JBeulich@suse.com>
To: xen-devel <xen-devel@lists.xenproject.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	Tamas K Lengyel <tamas@tklengyel.com>
Subject: [PATCH 2/4] x86: correct instances of PGC_allocated clearing
Date: Tue, 20 Nov 2018 09:18:47 -0700	[thread overview]
Message-ID: <5BF433E702000078001FE270@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <5BF4328902000078001FE240@prv1-mh.provo.novell.com>

For domain heap pages assigned to a domain dropping the page reference
tied to PGC_allocated may not drop the last reference, as otherwise the
test_and_clear_bit() might already act on an unowned page.

Work around this where possible, but the need to acquire extra page
references is a fair hint that references should have been acquired in
other places instead.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Compile tested only, as I have neither a mem-sharing nor a mem-paging
environment set up ready to be used for such testing.

--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -964,6 +964,15 @@ static int share_pages(struct domain *sd
         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)
@@ -993,6 +1002,7 @@ static int share_pages(struct domain *sd
     /* Free the client page */
     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
         put_page(cpage);
+    put_page(cpage);
 
     /* We managed to free a domain page. */
     atomic_dec(&nr_shared_mfns);
@@ -1066,9 +1076,16 @@ int mem_sharing_add_to_physmap(struct do
             if ( mfn_valid(cmfn) )
             {
                 struct page_info *cpage = mfn_to_page(cmfn);
-                ASSERT(cpage != NULL);
+
+                if ( !get_page(cpage, cd) )
+                {
+                    domain_crash(cd);
+                    ret = -EOVERFLOW;
+                    goto err_unlock;
+                }
                 if ( test_and_clear_bit(_PGC_allocated, &cpage->count_info) )
                     put_page(cpage);
+                put_page(cpage);
             }
         }
     }
@@ -1153,9 +1170,18 @@ int __mem_sharing_unshare_page(struct do
             mem_sharing_gfn_destroy(page, d, gfn_info);
         put_page_and_type(page);
         mem_sharing_page_unlock(page);
-        if ( last_gfn && 
-            test_and_clear_bit(_PGC_allocated, &page->count_info) ) 
+        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);
             put_page(page);
+        }
         put_gfn(d, gfn);
 
         return 0;
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -313,20 +313,36 @@ int guest_remove_page(struct domain *d,
 
     if ( unlikely(p2m_is_paging(p2mt)) )
     {
+        /*
+         * If the page hasn't yet been paged out, there is an
+         * actual page that needs to be released.
+         */
+        if ( p2mt == p2m_ram_paging_out )
+        {
+            ASSERT(mfn_valid(mfn));
+            page = mfn_to_page(mfn);
+            rc = -ENXIO;
+            if ( !get_page(page, d) )
+                goto out_put_gfn;
+        }
+        else
+            page = NULL;
+
         rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
         if ( rc )
+        {
+            if ( page )
+                put_page(page);
             goto out_put_gfn;
+        }
 
         put_gfn(d, gmfn);
 
-        /* If the page hasn't yet been paged out, there is an
-         * actual page that needs to be released. */
-        if ( p2mt == p2m_ram_paging_out )
+        if ( page )
         {
-            ASSERT(mfn_valid(mfn));
-            page = mfn_to_page(mfn);
             if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
                 put_page(page);
+            put_page(page);
         }
         p2m_mem_paging_drop_page(d, gmfn, p2mt);
 





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

  parent reply	other threads:[~2018-11-20 16:18 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-20 16:12 [PATCH 0/4] XSA-276 follow-up Jan Beulich
2018-11-20 16:17 ` [PATCH 1/4] mm: disallow MEMF_no_refcount to be passed for domain-owned allocations Jan Beulich
2018-11-20 16:35   ` Andrew Cooper
2018-11-23  9:45   ` [PATCH v2] " Jan Beulich
2018-11-23 10:28     ` Andrew Cooper
2018-11-23 10:49       ` Jan Beulich
2018-11-23 10:50         ` Andrew Cooper
2018-11-20 16:18 ` Jan Beulich [this message]
2018-11-20 16:59   ` [PATCH 2/4] x86: correct instances of PGC_allocated clearing Andrew Cooper
2018-11-20 17:12     ` Jan Beulich
2018-11-21  2:47       ` Tamas K Lengyel
2018-11-20 16:19 ` [PATCH 3/4] x86: reduce code duplication in guest_remove_page() Jan Beulich
2018-11-20 17:06   ` Andrew Cooper
2018-11-21  9:28     ` Jan Beulich
2018-11-20 16:19 ` [PATCH 4/4] make domain_adjust_tot_pages() __must_check Jan Beulich
2018-12-05 16:14 ` [PATCH v2 0/2] remaining XSA-276 follow-up Jan Beulich
2018-12-05 16:17   ` [PATCH v2 1/2] x86: reduce code duplication in guest_remove_page() Jan Beulich
2018-12-05 20:03     ` Andrew Cooper
2018-12-05 16:17   ` [PATCH v2 2/2] make domain_adjust_tot_pages() __must_check Jan Beulich
2018-12-05 20:08     ` Andrew Cooper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5BF433E702000078001FE270@prv1-mh.provo.novell.com \
    --to=jbeulich@suse.com \
    --cc=George.Dunlap@eu.citrix.com \
    --cc=Ian.Jackson@eu.citrix.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=julien.grall@arm.com \
    --cc=konrad.wilk@oracle.com \
    --cc=sstabellini@kernel.org \
    --cc=tamas@tklengyel.com \
    --cc=tim@xen.org \
    --cc=wei.liu2@citrix.com \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.