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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Mar 17, 2014 at 02:53:23PM +0000, David Vrabel wrote:
> 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.
> 

Do you want me to send V4 with updated commit message?

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

Sure.

Wei.

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