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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 9/26/19 11:45 AM, hongyax@xxxxxxxxxx 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, hongyax@xxxxxxxxxx wrote:
> > > From: Hongyan Xia <hongyax@xxxxxxxxxx>
> > >
> > > 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 <hongyax@xxxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel