Re: [PATCH] x86/p2m-pt: delay freeing of intermediate page tables

[George Dunlap <george.dunlap@citrix.com>]

On 29/09/15 11:51, Jan Beulich wrote:
> Old intermediate page tables must be freed only after IOMMU side
> updates/flushes have got carried out.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

> ---
> This was split out from a previously submitted patch.
> 
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -487,8 +487,9 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>      /* XXX -- this might be able to be faster iff current->domain == d */
>      void *table;
>      unsigned long i, gfn_remainder = gfn;
> -    l1_pgentry_t *p2m_entry;
> -    l1_pgentry_t entry_content;
> +    l1_pgentry_t *p2m_entry, entry_content;
> +    /* Intermediate table to free if we're replacing it with a superpage. */
> +    l1_pgentry_t intermediate_entry = l1e_empty();
>      l2_pgentry_t l2e_content;
>      l3_pgentry_t l3e_content;
>      int rc;
> @@ -538,7 +539,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>       */
>      if ( page_order == PAGE_ORDER_1G )
>      {
> -        l1_pgentry_t old_entry = l1e_empty();
>          p2m_entry = p2m_find_entry(table, &gfn_remainder, gfn,
>                                     L3_PAGETABLE_SHIFT - PAGE_SHIFT,
>                                     L3_PAGETABLE_ENTRIES);
> @@ -548,7 +548,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>          {
>              /* We're replacing a non-SP page with a superpage.  Make sure to
>               * handle freeing the table properly. */
> -            old_entry = *p2m_entry;
> +            intermediate_entry = *p2m_entry;
>          }
>  
>          ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
> @@ -566,10 +566,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>  
>          p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 3);
>          /* NB: paging_write_p2m_entry() handles tlb flushes properly */
> -
> -        /* Free old intermediate tables if necessary */
> -        if ( l1e_get_flags(old_entry) & _PAGE_PRESENT )
> -            p2m_free_entry(p2m, &old_entry, page_order);
>      }
>      else 
>      {
> @@ -610,7 +606,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>      }
>      else if ( page_order == PAGE_ORDER_2M )
>      {
> -        l1_pgentry_t old_entry = l1e_empty();
>          p2m_entry = p2m_find_entry(table, &gfn_remainder, gfn,
>                                     L2_PAGETABLE_SHIFT - PAGE_SHIFT,
>                                     L2_PAGETABLE_ENTRIES);
> @@ -622,7 +617,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>          {
>              /* We're replacing a non-SP page with a superpage.  Make sure to
>               * handle freeing the table properly. */
> -            old_entry = *p2m_entry;
> +            intermediate_entry = *p2m_entry;
>          }
>          
>          ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
> @@ -643,10 +638,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>  
>          p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 2);
>          /* NB: paging_write_p2m_entry() handles tlb flushes properly */
> -
> -        /* Free old intermediate tables if necessary */
> -        if ( l1e_get_flags(old_entry) & _PAGE_PRESENT )
> -            p2m_free_entry(p2m, &old_entry, page_order);
>      }
>  
>      /* Track the highest gfn for which we have ever had a valid mapping */
> @@ -674,6 +665,14 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>          }
>      }
>  
> +    /*
> +     * Free old intermediate tables if necessary.  This has to be the
> +     * last thing we do, after removal from the IOMMU tables, so as to
> +     * avoid a potential use-after-free.
> +     */
> +    if ( l1e_get_flags(intermediate_entry) & _PAGE_PRESENT )
> +        p2m_free_entry(p2m, &intermediate_entry, page_order);
> +
>   out:
>      unmap_domain_page(table);
>      return rc;
> 
> 
>