Re: [Xen-devel] [RFC PATCH 58/84] x86/mm: fix leaks in map_xen_pagetable.

[Julien Grall]

Hi,

On 9/26/19 11:45 AM, hong...@amazon.com wrote:
On 26/09/2019 11:23, Julien Grall wrote:
Hi,
NIT: we don't usually add full stop at the end of the title. This 
applies for the rest of the series.

Thanks.


On 9/26/19 10:46 AM, hong...@amazon.com wrote:
From: Hongyan Xia <hong...@amazon.com>

Not unmapping pages after map_xen_pagetable can leak the virtual address
space over time. Also this fix makes vmap_to_mfn non-trivial to be a
macro. There might be better options but move it into vmap.c for now.

Signed-off-by: Hongyan Xia <hong...@amazon.com>
---
  xen/arch/x86/mm.c          |  5 +----
  xen/common/vmap.c          | 13 +++++++++++++
  xen/include/asm-arm/mm.h   |  2 --
  xen/include/asm-x86/page.h |  2 --
  xen/include/xen/vmap.h     |  3 +++
  5 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index b2b2edbed1..145c5ab47c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5160,6 +5160,7 @@ int map_pages_to_xen(
                               !(l2e_get_flags(ol2e) & _PAGE_PSE) )
                              free_xen_pagetable(l2e_get_mfn(ol2e));
                      }
+                    UNMAP_XEN_PAGETABLE(l2t);
                      free_xen_pagetable(l2t_mfn);
                  }
              }
@@ -5225,7 +5226,6 @@ int map_pages_to_xen(
                  l3e_write_atomic(pl3e,
                                   l3e_from_mfn(l2t_mfn, 
__PAGE_HYPERVISOR));
                  UNMAP_XEN_PAGETABLE(l2t);
-                l2t = NULL;
              }
              if ( locking )
                  spin_unlock(&map_pgdir_lock);
@@ -5346,7 +5346,6 @@ int map_pages_to_xen(
                      l2e_write_atomic(pl2e, l2e_from_mfn(l1t_mfn,
                                                          
__PAGE_HYPERVISOR));
                      UNMAP_XEN_PAGETABLE(l1t);
-                    l1t = NULL;
                  }
                  if ( locking )
                      spin_unlock(&map_pgdir_lock);
@@ -5589,7 +5588,6 @@ int modify_xen_mappings(unsigned long s, 
unsigned long e, unsigned int nf)
              {
                  l3e_write_atomic(pl3e, l3e_from_mfn(mfn, 
__PAGE_HYPERVISOR));
                  UNMAP_XEN_PAGETABLE(l2t);
-                l2t = NULL;
              }
              if ( locking )
                  spin_unlock(&map_pgdir_lock);
@@ -5657,7 +5655,6 @@ int modify_xen_mappings(unsigned long s, 
unsigned long e, unsigned int nf)
                      l2e_write_atomic(pl2e, l2e_from_mfn(mfn,
                                                          
__PAGE_HYPERVISOR));
                      UNMAP_XEN_PAGETABLE(l1t);
-                    l1t = NULL;
                  }
                  if ( locking )
                      spin_unlock(&map_pgdir_lock);
diff --git a/xen/common/vmap.c b/xen/common/vmap.c
index faebc1ddf1..fcdb8495c8 100644
--- a/xen/common/vmap.c
+++ b/xen/common/vmap.c
@@ -19,6 +19,19 @@ static unsigned int __read_mostly 
vm_end[VMAP_REGION_NR];
  /* lowest known clear bit in the bitmap */
  static unsigned int vm_low[VMAP_REGION_NR];
+mfn_t vmap_to_mfn(void *va)
+{
+    l1_pgentry_t *pl1e = virt_to_xen_l1e((unsigned long)(va));
+    mfn_t ret = _mfn(l1e_get_pfn(*pl1e));

We already

+    unmap_xen_pagetable(pl1e);

My knowledge of the x86 port is quite limited, but this feels 
suspicious to see an unmap in a function call vmap_to_mfn(). Where 
does is the map done?

Furthermore, this is not going to compile on Arm. You probably want to 
implement this function in x86 code and keep the current 
implementation for Arm.


Without the direct map, Xen page tables are accessed after mapping them 
into the address space with map_domain_page(), and unmapped when done. 
To read the l1e for the vmap, the page the l1e is in is first mapped, 
then the mfn is read, then the page is unmapped.

I am afraid I still don't understand. Maybe it will become clearer once 
the branch is provided.

Can you provide the exact call path where the corresponding 
map_domain_page will be done. Is is done by virt_to_xen_l1e()?


This series is based on 60 patches from Wei, so he might also be able to 
comment more on the details.

So is this a leak introduced by Wei's series?

Cheers,

--
Julien Grall

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