[PATCH] xen/balloon: set ballooned out pages as invalid in p2m

[PATCH] xen/balloon: set ballooned out pages as invalid in p2m

[David Vrabel]

Since cd9151e26d31048b2b5e00fd02e110e07d2200c9 (xen/balloon: set a
mapping for ballooned out pages), a ballooned out page had its entry
in the p2m set to the MFN of one of the scratch pages.  This means
that the p2m will contain many entries pointing to the same MFN.

During a domain save, these many-to-one entries are not identified as
such and the scratch page is saved multiple times. On restore the
ballooned pages are populated with new frames and the domain may use
up its allocation before all pages can be restored.

Set ballooned out pages as INVALID_P2M_ENTRY in the p2m (as they were
before), preventing them from being saved and re-populated on restore.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Reported-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
Tested-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
---
 drivers/xen/balloon.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index b7a506f..5c660c7 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -426,20 +426,18 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
 		 * p2m are consistent.
 		 */
 		if (!xen_feature(XENFEAT_auto_translated_physmap)) {
-			unsigned long p;
-			struct page   *scratch_page = get_balloon_scratch_page();
-
 			if (!PageHighMem(page)) {
+				struct page *scratch_page = get_balloon_scratch_page();
+
 				ret = HYPERVISOR_update_va_mapping(
 						(unsigned long)__va(pfn << PAGE_SHIFT),
 						pfn_pte(page_to_pfn(scratch_page),
 							PAGE_KERNEL_RO), 0);
 				BUG_ON(ret);
-			}
-			p = page_to_pfn(scratch_page);
-			__set_phys_to_machine(pfn, pfn_to_mfn(p));
 
-			put_balloon_scratch_page();
+				put_balloon_scratch_page();
+			}
+			__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
 		}
 #endif
 
-- 
1.7.10.4

Re: [PATCH] xen/balloon: set ballooned out pages as invalid in p2m

[Konrad Rzeszutek Wilk]

On Tue, Jul 01, 2014 at 02:37:48PM +0100, David Vrabel wrote:
> Since cd9151e26d31048b2b5e00fd02e110e07d2200c9 (xen/balloon: set a
> mapping for ballooned out pages), a ballooned out page had its entry
> in the p2m set to the MFN of one of the scratch pages.  This means
> that the p2m will contain many entries pointing to the same MFN.
> 
> During a domain save, these many-to-one entries are not identified as
> such and the scratch page is saved multiple times. On restore the
> ballooned pages are populated with new frames and the domain may use
> up its allocation before all pages can be restored.
> 
> Set ballooned out pages as INVALID_P2M_ENTRY in the p2m (as they were
> before), preventing them from being saved and re-populated on restore.
> 

Won't that invalide the primal purpose of the scratch page code?

That is  cd9151e26d31048b2b5e00fd02e110e07d2200c9

    xen/balloon: set a mapping for ballooned out pages
" Allocate a page per cpu and map all the ballooned out pages to the
    corresponding mfn. Set the p2m accordingly. This way reading from a
    ballooned out page won't cause a kernel crash (see
    http://lists.xen.org/archives/html/xen-devel/2012-12/msg01154.html).
"
?

> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> Reported-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
> Tested-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
> ---
>  drivers/xen/balloon.c |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index b7a506f..5c660c7 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -426,20 +426,18 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>  		 * p2m are consistent.
>  		 */
>  		if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> -			unsigned long p;
> -			struct page   *scratch_page = get_balloon_scratch_page();
> -
>  			if (!PageHighMem(page)) {
> +				struct page *scratch_page = get_balloon_scratch_page();
> +
>  				ret = HYPERVISOR_update_va_mapping(
>  						(unsigned long)__va(pfn << PAGE_SHIFT),
>  						pfn_pte(page_to_pfn(scratch_page),
>  							PAGE_KERNEL_RO), 0);
>  				BUG_ON(ret);
> -			}
> -			p = page_to_pfn(scratch_page);
> -			__set_phys_to_machine(pfn, pfn_to_mfn(p));
>  
> -			put_balloon_scratch_page();
> +				put_balloon_scratch_page();
> +			}
> +			__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
>  		}
>  #endif
>  
> -- 
> 1.7.10.4
> 

Re: [PATCH] xen/balloon: set ballooned out pages as invalid in p2m

[David Vrabel]

On 01/07/14 14:58, Konrad Rzeszutek Wilk wrote:
> On Tue, Jul 01, 2014 at 02:37:48PM +0100, David Vrabel wrote:
>> Since cd9151e26d31048b2b5e00fd02e110e07d2200c9 (xen/balloon: set a
>> mapping for ballooned out pages), a ballooned out page had its entry
>> in the p2m set to the MFN of one of the scratch pages.  This means
>> that the p2m will contain many entries pointing to the same MFN.
>>
>> During a domain save, these many-to-one entries are not identified as
>> such and the scratch page is saved multiple times. On restore the
>> ballooned pages are populated with new frames and the domain may use
>> up its allocation before all pages can be restored.
>>
>> Set ballooned out pages as INVALID_P2M_ENTRY in the p2m (as they were
>> before), preventing them from being saved and re-populated on restore.
>>
> 
> Won't that invalide the primal purpose of the scratch page code?
> 
> That is  cd9151e26d31048b2b5e00fd02e110e07d2200c9
> 
>     xen/balloon: set a mapping for ballooned out pages
> " Allocate a page per cpu and map all the ballooned out pages to the
>     corresponding mfn. Set the p2m accordingly. This way reading from a
>     ballooned out page won't cause a kernel crash (see
>     http://lists.xen.org/archives/html/xen-devel/2012-12/msg01154.html).
> "
> ?

No, because we still have a virtual mapping for the ballooned out page.

David

Re: [PATCH] xen/balloon: set ballooned out pages as invalid in p2m

[Konrad Rzeszutek Wilk]

On Tue, Jul 01, 2014 at 03:00:16PM +0100, David Vrabel wrote:
> On 01/07/14 14:58, Konrad Rzeszutek Wilk wrote:
> > On Tue, Jul 01, 2014 at 02:37:48PM +0100, David Vrabel wrote:
> >> Since cd9151e26d31048b2b5e00fd02e110e07d2200c9 (xen/balloon: set a
> >> mapping for ballooned out pages), a ballooned out page had its entry
> >> in the p2m set to the MFN of one of the scratch pages.  This means
> >> that the p2m will contain many entries pointing to the same MFN.
> >>
> >> During a domain save, these many-to-one entries are not identified as
> >> such and the scratch page is saved multiple times. On restore the
> >> ballooned pages are populated with new frames and the domain may use
> >> up its allocation before all pages can be restored.
> >>
> >> Set ballooned out pages as INVALID_P2M_ENTRY in the p2m (as they were
> >> before), preventing them from being saved and re-populated on restore.
> >>
> > 
> > Won't that invalide the primal purpose of the scratch page code?
> > 
> > That is  cd9151e26d31048b2b5e00fd02e110e07d2200c9
> > 
> >     xen/balloon: set a mapping for ballooned out pages
> > " Allocate a page per cpu and map all the ballooned out pages to the
> >     corresponding mfn. Set the p2m accordingly. This way reading from a
> >     ballooned out page won't cause a kernel crash (see
> >     http://lists.xen.org/archives/html/xen-devel/2012-12/msg01154.html).
> > "
> > ?
> 
> No, because we still have a virtual mapping for the ballooned out page.

If you could add that comment in that would reflect that - IMHO
it would be good.

> 
> David

Re: [PATCH] xen/balloon: set ballooned out pages as invalid in p2m

[Stefano Stabellini]

On Tue, 1 Jul 2014, Konrad Rzeszutek Wilk wrote:
> On Tue, Jul 01, 2014 at 03:00:16PM +0100, David Vrabel wrote:
> > On 01/07/14 14:58, Konrad Rzeszutek Wilk wrote:
> > > On Tue, Jul 01, 2014 at 02:37:48PM +0100, David Vrabel wrote:
> > >> Since cd9151e26d31048b2b5e00fd02e110e07d2200c9 (xen/balloon: set a
> > >> mapping for ballooned out pages), a ballooned out page had its entry
> > >> in the p2m set to the MFN of one of the scratch pages.  This means
> > >> that the p2m will contain many entries pointing to the same MFN.
> > >>
> > >> During a domain save, these many-to-one entries are not identified as
> > >> such and the scratch page is saved multiple times. On restore the
> > >> ballooned pages are populated with new frames and the domain may use
> > >> up its allocation before all pages can be restored.
> > >>
> > >> Set ballooned out pages as INVALID_P2M_ENTRY in the p2m (as they were
> > >> before), preventing them from being saved and re-populated on restore.
> > >>
> > > 
> > > Won't that invalide the primal purpose of the scratch page code?
> > > 
> > > That is  cd9151e26d31048b2b5e00fd02e110e07d2200c9
> > > 
> > >     xen/balloon: set a mapping for ballooned out pages
> > > " Allocate a page per cpu and map all the ballooned out pages to the
> > >     corresponding mfn. Set the p2m accordingly. This way reading from a
> > >     ballooned out page won't cause a kernel crash (see
> > >     http://lists.xen.org/archives/html/xen-devel/2012-12/msg01154.html).
> > > "
> > > ?
> > 
> > No, because we still have a virtual mapping for the ballooned out page.
> 
> If you could add that comment in that would reflect that - IMHO
> it would be good.

Agreed.
Aside from that:

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

[PATCH] xen/balloon: set ballooned out pages as invalid in p2m

[David Vrabel]

Since cd9151e26d31048b2b5e00fd02e110e07d2200c9 (xen/balloon: set a
mapping for ballooned out pages), a ballooned out page had its entry
in the p2m set to the MFN of one of the scratch pages.  This means
that the p2m will contain many entries pointing to the same MFN.

During a domain save, these many-to-one entries are not identified as
such and the scratch page is saved multiple times. On restore the
ballooned pages are populated with new frames and the domain may use
up its allocation before all pages can be restored.

Since the original fix only needed to keep a mapping for the ballooned
page it is safe to set ballooned out pages as INVALID_P2M_ENTRY in the
p2m (as they were before). Thus preventing them from being saved and
re-populated on restore.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Reported-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
Tested-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
---
Cc: Marek Marczykowski <marmarek@invisiblethingslab.com>

v2:
- improve commit message
---
 drivers/xen/balloon.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index b7a506f..5c660c7 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -426,20 +426,18 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
 		 * p2m are consistent.
 		 */
 		if (!xen_feature(XENFEAT_auto_translated_physmap)) {
-			unsigned long p;
-			struct page   *scratch_page = get_balloon_scratch_page();
-
 			if (!PageHighMem(page)) {
+				struct page *scratch_page = get_balloon_scratch_page();
+
 				ret = HYPERVISOR_update_va_mapping(
 						(unsigned long)__va(pfn << PAGE_SHIFT),
 						pfn_pte(page_to_pfn(scratch_page),
 							PAGE_KERNEL_RO), 0);
 				BUG_ON(ret);
-			}
-			p = page_to_pfn(scratch_page);
-			__set_phys_to_machine(pfn, pfn_to_mfn(p));
 
-			put_balloon_scratch_page();
+				put_balloon_scratch_page();
+			}
+			__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
 		}
 #endif
 
-- 
1.7.10.4

Re: [PATCH] xen/balloon: set ballooned out pages as invalid in p2m

[David Vrabel]

On 02/07/14 10:40, David Vrabel wrote:
> Since cd9151e26d31048b2b5e00fd02e110e07d2200c9 (xen/balloon: set a
> mapping for ballooned out pages), a ballooned out page had its entry
> in the p2m set to the MFN of one of the scratch pages.  This means
> that the p2m will contain many entries pointing to the same MFN.
> 
> During a domain save, these many-to-one entries are not identified as
> such and the scratch page is saved multiple times. On restore the
> ballooned pages are populated with new frames and the domain may use
> up its allocation before all pages can be restored.
> 
> Since the original fix only needed to keep a mapping for the ballooned
> page it is safe to set ballooned out pages as INVALID_P2M_ENTRY in the
> p2m (as they were before). Thus preventing them from being saved and
> re-populated on restore.

Stefano acked v1 so I've applied to to stable/for-linus-2.16 and tagged
for stable.

David