xen/arm: p2m: Prevent deadlock when using memaccess

Commit 7d623b358a4 "arm/mem_access: Add long-descriptor based gpt"
assumed the read-write lock can be taken recursively. However, this
assumption is wrong and will lead to deadlock when the lock is
contended.

The read lock is taken recursively in the following case:
    1) get_page_from_gva
        => Take the read lock (first read lock)
        => Call p2m_mem_access_check_and_get_page on failure when
        memaccess is enabled
    2) p2m_mem_access_check_and_get_page
        => If hardware translation failed fallback to software lookup
        => Call guest_walk_tables
    3) guest_walk_tables
        => Will use access_guest_memory_by_ipa to access stage-1 page-table
    4) access_guest_memory_by_ipa
        => Because Arm does not have hardware instruction to only do
        stage-2 page-table, this is done in software.
        => Take the read lock (second read lock)

To avoid the nested lock, rework the locking in get_page_from_gva and
p2m_mem_access_check_and_get_page. The latter will now be called without
the p2m lock. The new locking in p2m_mem_accces_check_and_get_page will
not cover the translation of the VA to an IPA.

This is fine because we can't promise that the stage-1 page-table have
changed behind our back (they are under guest control). Modification in
the stage-2 page-table can now happen, but I can't issue any potential
issue here except with the break-before-make sequence used when updating
page-table. gva_to_ipa may fail if the sequence is executed at the same
on another CPU. In that case we would fallback in the software lookup
path.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Sergej Proskurin <proskurin@sec.in.tum.de>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
index 0f2cbb81d3ff2212ca8a2b1401fc3179615f7f24..11c2b03b7b8b04de344de0556a0f4865cbabec3c 100644 (file)
--- a/xen/arch/arm/mem_access.c
+++ b/xen/arch/arm/mem_access.c
@@ -126,7 +126,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
          * is not mapped.
          */
         if ( guest_walk_tables(v, gva, &ipa, &perms) < 0 )
-            goto err;
+            return NULL;
 
         /*
          * Check permissions that are assumed by the caller. For instance in
@@ -139,11 +139,13 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
          * test for execute permissions this check can be left out.
          */
         if ( (flag & GV2M_WRITE) && !(perms & GV2M_WRITE) )
-            goto err;
+            return NULL;
     }
 
     gfn = gaddr_to_gfn(ipa);
 
+    p2m_read_lock(p2m);
+
     /*
      * We do this first as this is faster in the default case when no
      * permission is set on the page.
@@ -216,6 +218,8 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
         page = NULL;
 
 err:
+    p2m_read_unlock(p2m);
+
     return page;
 }
 

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 65e8b9c6ea2721ae35b2a131b563bf82d5d8f251..5de82aafe1b31e962b17a3fffa40bc06500b1d86 100644 (file)
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1449,11 +1449,11 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
     }
 
 err:
+    p2m_read_unlock(p2m);
+
     if ( !page && p2m->mem_access_enabled )
         page = p2m_mem_access_check_and_get_page(va, flags, v);
 
-    p2m_read_unlock(p2m);
-
     return page;
 }
 

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index a0abc84ed82a0a271819edb35bdc9cb7d005c471..45ef2cd58b9b5a4d845a25a967bf269be3c0018b 100644 (file)
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -23,10 +23,6 @@ extern void memory_type_changed(struct domain *);
 struct p2m_domain {
     /*
      * Lock that protects updates to the p2m.
-     *
-     * Please note that we use this lock in a nested way by calling
-     * access_guest_memory_by_ipa in guest_walk_(sd|ld). This must be
-     * considered in the future implementation.
      */
     rwlock_t lock;