Re: [Xen-devel] [PATCH V3] xen/balloon: flush persistent kmaps in correct position

[David Vrabel <david.vrabel@xxxxxxxxxx>]

On 15/03/14 16:11, Wei Liu wrote:
> Xen balloon driver will update ballooned out pages' P2M entries to point
> to scratch page for PV guests. In 24f69373e2 ("xen/balloon: don't alloc
> page while non-preemptible", kmap_flush_unused was moved after updating
> P2M table. In that case for 32 bit PV guest we might end up with
> 
>   P2M    X -----> S  (S is mfn of balloon scratch page)
>   M2P    Y -----> X  (Y is mfn in persistent kmap entry)
> 
> When kmap_flush_unused is called, it will call into
> flush_all_zero_pkmaps, which calls pte_page. Pte_page will call into
> PVMMU, which relies on P2M and M2P tables to do the correct translation.
> When PVMMU sees X -> S and Y -> X, it gets confused and returns a wrong
> value, which causes the guest to crash high up the call chain.

I don't think using PVMMU as a thing in this paragraph really helps.

   kmap_flush_unused() iterates through all the PTEs in the kmap
   address space, using pte_to_page() to obtain the page. If the p2m
   and the m2p are inconsistent the incorrect page is returned.
   This will clear page->address on the wrong page which may cause
   subsequent oopses if that page is currently kmap'ed.

> Move the flush back between get_page and __set_phys_to_machine to fix
> this.

Reviewed-by: David Vrabel <david.vrabel@xxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx # 3.12+

> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -404,6 +404,15 @@ static enum bp_state decrease_reservation(unsigned long 
> nr_pages, gfp_t gfp)
>               frame_list[i] = pfn_to_mfn(pfn);
>  
>               scrub_page(page);
> +     }
> +
> +     /* Ensure that ballooned highmem pages don't have kmaps. */
> +     kmap_flush_unused();
> +     flush_tlb_all();

Can you check if this flush_tlb_all() is required and post a follow-up
if it isn't.  I suggest looking at the update_va_mapping hypercall -- I
took a quick look and it looks like it flushes the TLB already.

> +
> +     /* No more mappings: invalidate P2M and add to balloon. */
> +     for (i = 0; i < nr_pages; i++) {
> +             pfn = mfn_to_pfn(frame_list[i]);

With a bit of refactoring you could avoid some of the mfn_to_pfn() etc.
translations.  But a v3 isn't required for this.

David

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