[for-4.7 v2 0/2] xen/arm: Bug fixes in the P2M code

[for-4.7 v2 0/2] xen/arm: Bug fixes in the P2M code

[Julien Grall]

Hello,

This small series fixes potential deadlock and the removal of unrelated
mapping when the P2M code fails to insert/allocate mappings.

This series contains bug fix for Xen 4.7 and candidate for backporting
up to Xen 4.5.

Yours sincerely,

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

Julien Grall (2):
  xen/arm: p2m: apply_p2m_changes: Do not undo more than necessary
  xen/arm: p2m: Release the p2m lock before undoing the mappings

 xen/arch/arm/p2m.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[for-4.7 v2 1/2] xen/arm: p2m: apply_p2m_changes: Do not undo more than necessary

[Julien Grall]

Since commit 4b25423a "arch/arm: unmap partially-mapped memory regions",
Xen has been undoing the P2M mappings when an error occurred during
insertion or memory allocation.

The function apply_p2m_changes can work with region not-aligned to a
block size (2MB, 1G) or page size (4K). The mapping will be done by
splitting the region in a set of regions aligned to the size supported
by the page table.

The mapping of a region could fail when it is not possible to allocate
memory for an intermediate table (i.e a new or when shattering a block).

When the mapping is undone, the end of the region is computed using the
base address of the current region and the size of the failing level.
However the failing level may not be the leaf one, therefore unrelated
entries will be removed.

Fix it by removing the mapping from the start address up to the last
region that has been successfully mapped.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    This patch is a bug fix for Xen 4.7 and candidate for backporting
    up to Xen 4.5. Without this patch, Xen may undo mapping which are
    not part of the region mapped when memory allocation has failed.

    Note that Xen 4.7 has code to remove empty translation table (see
    commit de5162b "xen/arm: p2m: Remove translation table when it's
    empty"), however with this patch those tables will not be removed
    in case of failure. This will be fixed after the release as
    the change will be too intrusive for Xen 4.7.

    Changes in v2:
        - Add Stefano's and Wei's reviewed-by
---
 xen/arch/arm/p2m.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index db21433..68c67b0 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1189,13 +1189,10 @@ out:
     {
         BUG_ON(addr == end_gpaddr);
         /*
-         * addr keeps the address of the last successfully-inserted mapping,
-         * while apply_p2m_changes() considers an address range which is
-         * exclusive of end_gpaddr: add level_size to addr to obtain the
-         * right end of the range
+         * addr keeps the address of the end of the last successfully-inserted
+         * mapping.
          */
-        apply_p2m_changes(d, REMOVE,
-                          start_gpaddr, addr + level_sizes[level], orig_maddr,
+        apply_p2m_changes(d, REMOVE, start_gpaddr, addr, orig_maddr,
                           mattr, 0, p2m_invalid, d->arch.p2m.default_access);
     }
 
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[for-4.7 v2 2/2] xen/arm: p2m: Release the p2m lock before undoing the mappings

[Julien Grall]

Since commit 4b25423a "arch/arm: unmap partially-mapped memory regions",
Xen has been undoing the P2M mappings when an error occurred during
insertion or memory allocation.

This is done by calling recursively apply_p2m_changes, however the
second call is done with the p2m lock taken which will result in a
deadlock for the current processor.

The p2m lock is here to protect 2 threads modifying concurrently the
page tables. However, it does not guarantee the ordering of the
changes. I.e if 2 threads request change on regions that overlaps,
then the result is undefined.

Therefore it is fine to move the recursive call to undo the changes
after the lock is released.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
Tested-by: Wei Chen <Wei.Chen@arm.com>

---
    I think we could unlock the p2m lock before freeing the temporary
    mapping. Although, I played safe as this is a bug fix for Xen 4.7
    and to be backported up to Xen 4.5.

    Changes in v2:
        - Update the commit message to explain why unlocking before
        the recursive call is fine.
        - Add Wei Chen's reviewed-by and tested-by
---
 xen/arch/arm/p2m.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 68c67b0..838d004 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1184,6 +1184,14 @@ out:
     while ( (pg = page_list_remove_head(&free_pages)) )
         free_domheap_page(pg);
 
+    for ( level = P2M_ROOT_LEVEL; level < 4; level ++ )
+    {
+        if ( mappings[level] )
+            unmap_domain_page(mappings[level]);
+    }
+
+    spin_unlock(&p2m->lock);
+
     if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) &&
          addr != start_gpaddr )
     {
@@ -1196,14 +1204,6 @@ out:
                           mattr, 0, p2m_invalid, d->arch.p2m.default_access);
     }
 
-    for ( level = P2M_ROOT_LEVEL; level < 4; level ++ )
-    {
-        if ( mappings[level] )
-            unmap_domain_page(mappings[level]);
-    }
-
-    spin_unlock(&p2m->lock);
-
     return rc;
 }
 
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel