[PATCH] xen: fix __set_phys_to_machine

[PATCH] xen: fix __set_phys_to_machine

[Wei Liu]

In commit cd9151e2: xen/balloon: set a mapping for ballooned out pages
we have the ballooned out page's mapping set to a scratch page. When the
page is ballooned in again its P2M entry can be the MFN of the scratch
page, hitting the BUG_ONs in __set_phys_to_machine.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/x86/xen/p2m.c    |   14 ++++++++++++--
 drivers/xen/balloon.c |    2 +-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 74672ee..4c11f6a 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -793,17 +793,27 @@ unsigned long __init set_phys_range_identity(unsigned long pfn_s,
 	return pfn - pfn_s;
 }
 
+DECLARE_PER_CPU(struct page *, balloon_scratch_page);
 /* Try to install p2m mapping; fail if intermediate bits missing */
 bool __set_phys_to_machine(unsigned long pfn, unsigned long mfn)
 {
 	unsigned topidx, mididx, idx;
+	unsigned long balloon_scratch_pfn;
+	unsigned long balloon_scratch_mfn;
+
+	balloon_scratch_pfn = page_to_pfn(__get_cpu_var(balloon_scratch_page));
+	balloon_scratch_mfn = pfn_to_mfn(balloon_scratch_pfn);
 
 	if (unlikely(xen_feature(XENFEAT_auto_translated_physmap))) {
-		BUG_ON(pfn != mfn && mfn != INVALID_P2M_ENTRY);
+		BUG_ON(pfn != mfn &&
+		       pfn != balloon_scratch_mfn &&
+		       mfn != INVALID_P2M_ENTRY &&
+		       mfn != balloon_scratch_mfn);
 		return true;
 	}
 	if (unlikely(pfn >= MAX_P2M_PFN)) {
-		BUG_ON(mfn != INVALID_P2M_ENTRY);
+		BUG_ON(mfn != INVALID_P2M_ENTRY &&
+		       mfn != balloon_scratch_mfn);
 		return true;
 	}
 
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index a3dc75d..89e2f3b 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -92,7 +92,7 @@ EXPORT_SYMBOL_GPL(balloon_stats);
 
 /* We increase/decrease in batches which fit in a page */
 static xen_pfn_t frame_list[PAGE_SIZE / sizeof(unsigned long)];
-static DEFINE_PER_CPU(struct page *, balloon_scratch_page);
+DEFINE_PER_CPU(struct page *, balloon_scratch_page);
 
 
 /* List of ballooned pages, threaded through the mem_map array. */
-- 
1.7.10.4

Re: [PATCH] xen: fix __set_phys_to_machine

[Ian Campbell]

On Thu, 2013-08-22 at 11:10 +0100, Wei Liu wrote:
> In commit cd9151e2: xen/balloon: set a mapping for ballooned out pages
> we have the ballooned out page's mapping set to a scratch page. When the
> page is ballooned in again its P2M entry can be the MFN of the scratch
> page, hitting the BUG_ONs in __set_phys_to_machine.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  arch/x86/xen/p2m.c    |   14 ++++++++++++--
>  drivers/xen/balloon.c |    2 +-
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 74672ee..4c11f6a 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -793,17 +793,27 @@ unsigned long __init set_phys_range_identity(unsigned long pfn_s,
>  	return pfn - pfn_s;
>  }
>  
> +DECLARE_PER_CPU(struct page *, balloon_scratch_page);

Should b in a header I think?

>  /* Try to install p2m mapping; fail if intermediate bits missing */
>  bool __set_phys_to_machine(unsigned long pfn, unsigned long mfn)
>  {
>  	unsigned topidx, mididx, idx;
> +	unsigned long balloon_scratch_pfn;
> +	unsigned long balloon_scratch_mfn;
> +
> +	balloon_scratch_pfn = page_to_pfn(__get_cpu_var(balloon_scratch_page));
> +	balloon_scratch_mfn = pfn_to_mfn(balloon_scratch_pfn);
>  
>  	if (unlikely(xen_feature(XENFEAT_auto_translated_physmap))) {
> -		BUG_ON(pfn != mfn && mfn != INVALID_P2M_ENTRY);
> +		BUG_ON(pfn != mfn &&
> +		       pfn != balloon_scratch_mfn &&
> +		       mfn != INVALID_P2M_ENTRY &&
> +		       mfn != balloon_scratch_mfn);

Is there any guarantee that __set_phys_to_machine will always operate on
a page which was ballooned out on this particular CPU?

>  		return true;
>  	}
>  	if (unlikely(pfn >= MAX_P2M_PFN)) {
> -		BUG_ON(mfn != INVALID_P2M_ENTRY);
> +		BUG_ON(mfn != INVALID_P2M_ENTRY &&
> +		       mfn != balloon_scratch_mfn);
>  		return true;
>  	}
>  
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index a3dc75d..89e2f3b 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -92,7 +92,7 @@ EXPORT_SYMBOL_GPL(balloon_stats);
>  
>  /* We increase/decrease in batches which fit in a page */
>  static xen_pfn_t frame_list[PAGE_SIZE / sizeof(unsigned long)];
> -static DEFINE_PER_CPU(struct page *, balloon_scratch_page);
> +DEFINE_PER_CPU(struct page *, balloon_scratch_page);
>  
> 
>  /* List of ballooned pages, threaded through the mem_map array. */

Re: [PATCH] xen: fix __set_phys_to_machine

[David Vrabel]

On 22/08/13 11:10, Wei Liu wrote:
> In commit cd9151e2: xen/balloon: set a mapping for ballooned out pages
> we have the ballooned out page's mapping set to a scratch page. When the
> page is ballooned in again its P2M entry can be the MFN of the scratch
> page, hitting the BUG_ONs in __set_phys_to_machine.

Looking at the commit that introduced this bug I wonder if the the
correct fix is to restore the original call of
__set_phys_to_machine(pfn, INVALID_P2M_ENTRY) in decrease_reservation().

We only need a valid kernel mapping for the ballooned out page, the p2m
should still be invalid for the ballooned out page, right?

> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -793,17 +793,27 @@ unsigned long __init set_phys_range_identity(unsigned long pfn_s,
>  	return pfn - pfn_s;
>  }
>  
> +DECLARE_PER_CPU(struct page *, balloon_scratch_page);
>  /* Try to install p2m mapping; fail if intermediate bits missing */
>  bool __set_phys_to_machine(unsigned long pfn, unsigned long mfn)
>  {
>  	unsigned topidx, mididx, idx;
> +	unsigned long balloon_scratch_pfn;
> +	unsigned long balloon_scratch_mfn;
> +
> +	balloon_scratch_pfn = page_to_pfn(__get_cpu_var(balloon_scratch_page));
> +	balloon_scratch_mfn = pfn_to_mfn(balloon_scratch_pfn);
>  
>  	if (unlikely(xen_feature(XENFEAT_auto_translated_physmap))) {
> -		BUG_ON(pfn != mfn && mfn != INVALID_P2M_ENTRY);
> +		BUG_ON(pfn != mfn &&
> +		       pfn != balloon_scratch_mfn &&
> +		       mfn != INVALID_P2M_ENTRY &&
> +		       mfn != balloon_scratch_mfn);
>  		return true;
>  	}
>  	if (unlikely(pfn >= MAX_P2M_PFN)) {
> -		BUG_ON(mfn != INVALID_P2M_ENTRY);
> +		BUG_ON(mfn != INVALID_P2M_ENTRY &&
> +		       mfn != balloon_scratch_mfn);

This bit looks wrong/unnecessary.

David

Re: [PATCH] xen: fix __set_phys_to_machine

[Wei Liu]

On Thu, Aug 22, 2013 at 01:08:29PM +0100, David Vrabel wrote:
> On 22/08/13 11:10, Wei Liu wrote:
> > In commit cd9151e2: xen/balloon: set a mapping for ballooned out pages
> > we have the ballooned out page's mapping set to a scratch page. When the
> > page is ballooned in again its P2M entry can be the MFN of the scratch
> > page, hitting the BUG_ONs in __set_phys_to_machine.
> 
> Looking at the commit that introduced this bug I wonder if the the
> correct fix is to restore the original call of
> __set_phys_to_machine(pfn, INVALID_P2M_ENTRY) in decrease_reservation().
> 
> We only need a valid kernel mapping for the ballooned out page, the p2m
> should still be invalid for the ballooned out page, right?
> 

Indeed, this is a simple and straightforward fix. Patch on the way.

Wei.

> > --- a/arch/x86/xen/p2m.c
> > +++ b/arch/x86/xen/p2m.c
> > @@ -793,17 +793,27 @@ unsigned long __init set_phys_range_identity(unsigned long pfn_s,
> >  	return pfn - pfn_s;
> >  }
> >  
> > +DECLARE_PER_CPU(struct page *, balloon_scratch_page);
> >  /* Try to install p2m mapping; fail if intermediate bits missing */
> >  bool __set_phys_to_machine(unsigned long pfn, unsigned long mfn)
> >  {
> >  	unsigned topidx, mididx, idx;
> > +	unsigned long balloon_scratch_pfn;
> > +	unsigned long balloon_scratch_mfn;
> > +
> > +	balloon_scratch_pfn = page_to_pfn(__get_cpu_var(balloon_scratch_page));
> > +	balloon_scratch_mfn = pfn_to_mfn(balloon_scratch_pfn);
> >  
> >  	if (unlikely(xen_feature(XENFEAT_auto_translated_physmap))) {
> > -		BUG_ON(pfn != mfn && mfn != INVALID_P2M_ENTRY);
> > +		BUG_ON(pfn != mfn &&
> > +		       pfn != balloon_scratch_mfn &&
> > +		       mfn != INVALID_P2M_ENTRY &&
> > +		       mfn != balloon_scratch_mfn);
> >  		return true;
> >  	}
> >  	if (unlikely(pfn >= MAX_P2M_PFN)) {
> > -		BUG_ON(mfn != INVALID_P2M_ENTRY);
> > +		BUG_ON(mfn != INVALID_P2M_ENTRY &&
> > +		       mfn != balloon_scratch_mfn);
> 
> This bit looks wrong/unnecessary.
> 
> David