diff mbox series

[2/5] x86/p2m: don't assert that the passed in MFN matches for a remove

Message ID da2e71b2-085b-390d-69ba-9edcbbf4fcd2@suse.com (mailing list archive)
State New, archived
Headers show
Series x86/p2m: misc page add/remove adjustments | expand

Commit Message

Jan Beulich April 1, 2020, 11:39 a.m. UTC
guest_physmap_remove_page() gets handed an MFN from the outside, yet
takes the necessary lock to prevent further changes to the GFN <-> MFN
mapping itself. While some callers, in particular guest_remove_page()
(by way of having called get_gfn_query()), hold the GFN lock already,
various others (most notably perhaps the 2nd instance in
xenmem_add_to_physmap_one()) don't. While it also is an option to fix
all the callers, deal with the issue in p2m_remove_page() instead:
Replace the ASSERT() by a conditional and split the loop into two, such
that all checking gets done before any modification would occur.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

Comments

Andrew Cooper April 2, 2020, 10:39 p.m. UTC | #1
On 01/04/2020 12:39, Jan Beulich wrote:
> guest_physmap_remove_page() gets handed an MFN from the outside, yet
> takes the necessary lock to prevent further changes to the GFN <-> MFN
> mapping itself. While some callers, in particular guest_remove_page()
> (by way of having called get_gfn_query()), hold the GFN lock already,
> various others (most notably perhaps the 2nd instance in
> xenmem_add_to_physmap_one()) don't. While it also is an option to fix
> all the callers, deal with the issue in p2m_remove_page() instead:
> Replace the ASSERT() by a conditional and split the loop into two, such
> that all checking gets done before any modification would occur.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
diff mbox series

Patch

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -773,7 +773,6 @@  p2m_remove_page(struct p2m_domain *p2m,
 {
     unsigned long i;
     gfn_t gfn = _gfn(gfn_l);
-    mfn_t mfn_return;
     p2m_type_t t;
     p2m_access_t a;
 
@@ -784,15 +783,26 @@  p2m_remove_page(struct p2m_domain *p2m,
     ASSERT(gfn_locked_by_me(p2m, gfn));
     P2M_DEBUG("removing gfn=%#lx mfn=%#lx\n", gfn_l, mfn);
 
+    for ( i = 0; i < (1UL << page_order); )
+    {
+        unsigned int cur_order;
+        mfn_t mfn_return = p2m->get_entry(p2m, gfn_add(gfn, i), &t, &a, 0,
+                                          &cur_order, NULL);
+
+        if ( p2m_is_valid(t) &&
+             (!mfn_valid(_mfn(mfn)) || mfn + i != mfn_x(mfn_return)) )
+            return -EILSEQ;
+
+        i += (1UL << cur_order) - ((gfn_l + i) & ((1UL << cur_order) - 1));
+    }
+
     if ( mfn_valid(_mfn(mfn)) )
     {
         for ( i = 0; i < (1UL << page_order); i++ )
         {
-            mfn_return = p2m->get_entry(p2m, gfn_add(gfn, i), &t, &a, 0,
-                                        NULL, NULL);
+            p2m->get_entry(p2m, gfn_add(gfn, i), &t, &a, 0, NULL, NULL);
             if ( !p2m_is_grant(t) && !p2m_is_shared(t) && !p2m_is_foreign(t) )
                 set_gpfn_from_mfn(mfn+i, INVALID_M2P_ENTRY);
-            ASSERT( !p2m_is_valid(t) || mfn + i == mfn_x(mfn_return) );
         }
     }
     return p2m_set_entry(p2m, gfn, INVALID_MFN, page_order, p2m_invalid,