[PATCH V3] xen/balloon: flush persistent kmaps in correct position

[PATCH V3] xen/balloon: flush persistent kmaps in correct position

[Wei Liu]

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.

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

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Konrad Wilk <konrad.wilk@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 drivers/xen/balloon.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 37d06ea..6e56174 100644
--- 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();
+
+	/* No more mappings: invalidate P2M and add to balloon. */
+	for (i = 0; i < nr_pages; i++) {
+		pfn = mfn_to_pfn(frame_list[i]);
 
 #ifdef CONFIG_XEN_HAVE_PVMMU
 		/*
@@ -432,10 +441,6 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
 		balloon_append(pfn_to_page(pfn));
 	}
 
-	/* Ensure that ballooned highmem pages don't have kmaps. */
-	kmap_flush_unused();
-	flush_tlb_all();
-
 	set_xen_guest_handle(reservation.extent_start, frame_list);
 	reservation.nr_extents   = nr_pages;
 	ret = HYPERVISOR_memory_op(XENMEM_decrease_reservation, &reservation);
-- 
1.7.10.4

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

[David Vrabel]

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@citrix.com>
Cc: stable@vger.kernel.org # 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

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

[Wei Liu]

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@citrix.com>
> Cc: stable@vger.kernel.org # 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.

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

[David Vrabel]

On 17/03/14 15:04, Wei Liu wrote:
> 
> Do you want me to send V4 with updated commit message?

No need.

David

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

[Wei Liu]

On Sat, Mar 15, 2014 at 04:11:47PM +0000, 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.
> 
> Move the flush back between get_page and __set_phys_to_machine to fix
> this.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: Konrad Wilk <konrad.wilk@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  drivers/xen/balloon.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 37d06ea..6e56174 100644
> --- 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();
> +
> +	/* No more mappings: invalidate P2M and add to balloon. */
> +	for (i = 0; i < nr_pages; i++) {
> +		pfn = mfn_to_pfn(frame_list[i]);
>  

  +		page = pfn_to_page(pfn); // missing in this patch

This missing line causes PageHighMem to test on the wrong page.

David, you can either take V3 and add this line, or take V4.

Sorry for this mess.

Wei.

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

[David Vrabel]

On 18/03/14 13:47, Wei Liu wrote:
> On Sat, Mar 15, 2014 at 04:11:47PM +0000, Wei Liu wrote:
>> 
>> --- 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();
>> +
>> +	/* No more mappings: invalidate P2M and add to balloon. */
>> +	for (i = 0; i < nr_pages; i++) {
>> +		pfn = mfn_to_pfn(frame_list[i]);
>>  
> 
>   +		page = pfn_to_page(pfn); // missing in this patch
> 
> This missing line causes PageHighMem to test on the wrong page.
> 
> David, you can either take V3 and add this line, or take V4.

Wei, how about this?  I expanded the comment, left the tlb flush after
changing the mappings, and shuffled a few bits around to avoid a few
pfn/mfn/page conversions.

Can you test it, please?

Thanks.

8<---------------------------------
>From e0c38006cfd0395d500a66d2ab07dfd1327d140d Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Sat, 15 Mar 2014 16:11:47 +0000
Subject: [PATCH] xen/balloon: flush persistent kmaps in correct position

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)

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.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Cc: stable@vger.kernel.org # 3.12+
---
 drivers/xen/balloon.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 37d06ea..3e27d11 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -399,12 +399,26 @@ static enum bp_state decrease_reservation(unsigned
long nr_pages, gfp_t gfp)
 			state = BP_EAGAIN;
 			break;
 		}
+		scrub_page(page);

-		pfn = page_to_pfn(page);
-		frame_list[i] = pfn_to_mfn(pfn);
+		frame_list[i] = page_to_pfn(page);
+	}

-		scrub_page(page);
+	/*
+	 * Ensure that ballooned highmem pages don't have kmaps.
+	 *
+	 * Do this before changing the p2m as kmap_flush_unused()
+	 * reads PTEs to obtain pages (and hence needs the original
+	 * p2m entry).
+	 */
+	kmap_flush_unused();

+	/* Update direct mapping, invalidate P2M, and add to balloon. */
+	for (i = 0; i < nr_pages; i++) {
+		pfn = frame_list[i];
+		frame_list[i] = pfn_to_mfn(pfn);
+		page = pfn_to_page(pfn);
+		
 #ifdef CONFIG_XEN_HAVE_PVMMU
 		/*
 		 * Ballooned out frames are effectively replaced with
@@ -429,11 +443,9 @@ static enum bp_state decrease_reservation(unsigned
long nr_pages, gfp_t gfp)
 		}
 #endif

-		balloon_append(pfn_to_page(pfn));
+		balloon_append(page);
 	}

-	/* Ensure that ballooned highmem pages don't have kmaps. */
-	kmap_flush_unused();
 	flush_tlb_all();

 	set_xen_guest_handle(reservation.extent_start, frame_list);
-- 
1.7.2.5

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

[Wei Liu]

On Tue, Mar 18, 2014 at 05:40:55PM +0000, David Vrabel wrote:
[...]
> Wei, how about this?  I expanded the comment, left the tlb flush after
> changing the mappings, and shuffled a few bits around to avoid a few
> pfn/mfn/page conversions.
> 
> Can you test it, please?
> 

Yes, I tested it on 32-bit PV and it worked. I manually copied and
pasted the changes.

> Thanks.
> 
> 8<---------------------------------
> >From e0c38006cfd0395d500a66d2ab07dfd1327d140d Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Sat, 15 Mar 2014 16:11:47 +0000
> Subject: [PATCH] xen/balloon: flush persistent kmaps in correct position
> 
> 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)
> 
> 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.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> Cc: stable@vger.kernel.org # 3.12+
> ---
>  drivers/xen/balloon.c |   24 ++++++++++++++++++------
>  1 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 37d06ea..3e27d11 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -399,12 +399,26 @@ static enum bp_state decrease_reservation(unsigned
> long nr_pages, gfp_t gfp)

FYI, This line was wrapped by your email client.

Wei

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

[David Vrabel]

On 19/03/14 11:19, Wei Liu wrote:
> On Tue, Mar 18, 2014 at 05:40:55PM +0000, David Vrabel wrote:
> [...]
>> Wei, how about this?  I expanded the comment, left the tlb flush after
>> changing the mappings, and shuffled a few bits around to avoid a few
>> pfn/mfn/page conversions.
>>
>> Can you test it, please?
>>
> 
> Yes, I tested it on 32-bit PV and it worked. I manually copied and
> pasted the changes.

Applied to stable/for-linus-3.14.

Thanks.

>> @@ -399,12 +399,26 @@ static enum bp_state decrease_reservation(unsigned
>> long nr_pages, gfp_t gfp)
> 
> FYI, This line was wrapped by your email client.

Sorry, forgot to disable word wrapping.

David