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

[Wei Liu <wl@xxxxxxx>]

On Thu, Sep 26, 2019 at 10:46:21AM +0100, hongyax@xxxxxxxxxx wrote:
> From: Hongyan Xia <hongyax@xxxxxxxxxx>
> 
> Not unmapping pages after map_xen_pagetable can leak the virtual address
> space over time.

I understand this part, but ...

> 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.
> 

... not this part.

> 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);

This is presumably the issue you try to fix.

>                      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;

This and similar changes below are irrelevant to the bug your try to
fix.

UNMAP_XEN_PAGETABLE already sets lXt to NULL. Deleting these two lines
are fine, but they should be folded into one of the previous patches
where UNMAP_XEN_PAGETABLE was used in this function.

>              }
>              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

I fail to see why you need to change vmap to fix a leak somewhere else.

I guess I will need to wait for your branch to have a closer look.

Wei.

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