[PATCH RFC] xen: prevent PVH Dom0 from having pages with more than one ref

[PATCH RFC] xen: prevent PVH Dom0 from having pages with more than one ref

[Roger Pau Monne]

With the current implementation of the Dom0 domain builder a PVH Dom0
ends up with pages that have 2 references, which means they are not
freed when Dom0 balloons memory down. This leads to those pages
not being freed in guest_remove_page, so they are leaked and the
memory accounting between Xen and Dom0 differs, because Dom0 OS
actually thinks they are freed, but d->tot_pages is not decreased.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/domain_build.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index ba42fc9..7073dac 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -1137,10 +1137,12 @@ int __init construct_dom0(
                                     L1_PROT : COMPAT_L1_PROT));
         l1tab++;
 
-        page = mfn_to_page(mfn);
-        if ( (page->u.inuse.type_info == 0) &&
-             !get_page_and_type(page, d, PGT_writable_page) )
-            BUG();
+        if ( !is_pvh_domain(d) ) {
+            page = mfn_to_page(mfn);
+            if ( (page->u.inuse.type_info == 0) &&
+                 !get_page_and_type(page, d, PGT_writable_page) )
+                BUG();
+        }
     }
 
     if ( is_pv_32on64_domain(d) )
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH RFC] xen: prevent PVH Dom0 from having pages with more than one ref

[Andrew Cooper]

On 13/06/14 10:12, Roger Pau Monne wrote:
> With the current implementation of the Dom0 domain builder a PVH Dom0
> ends up with pages that have 2 references, which means they are not
> freed when Dom0 balloons memory down. This leads to those pages
> not being freed in guest_remove_page, so they are leaked and the
> memory accounting between Xen and Dom0 differs, because Dom0 OS
> actually thinks they are freed, but d->tot_pages is not decreased.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---

In which scenarios do pages get two refs, and why do you consider this
to be valid?

(Or in other words, my gut feel is that this patch is wrong, but there
is insufficient information to decide one way or another.)

~Andrew

>  xen/arch/x86/domain_build.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index ba42fc9..7073dac 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -1137,10 +1137,12 @@ int __init construct_dom0(
>                                      L1_PROT : COMPAT_L1_PROT));
>          l1tab++;
>  
> -        page = mfn_to_page(mfn);
> -        if ( (page->u.inuse.type_info == 0) &&
> -             !get_page_and_type(page, d, PGT_writable_page) )
> -            BUG();
> +        if ( !is_pvh_domain(d) ) {
> +            page = mfn_to_page(mfn);
> +            if ( (page->u.inuse.type_info == 0) &&
> +                 !get_page_and_type(page, d, PGT_writable_page) )
> +                BUG();
> +        }
>      }
>  
>      if ( is_pv_32on64_domain(d) )


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

Re: [PATCH RFC] xen: prevent PVH Dom0 from having pages with more than one ref

[Roger Pau Monné]

On 13/06/14 11:23, Andrew Cooper wrote:
> On 13/06/14 10:12, Roger Pau Monne wrote:
>> With the current implementation of the Dom0 domain builder a PVH Dom0
>> ends up with pages that have 2 references, which means they are not
>> freed when Dom0 balloons memory down. This leads to those pages
>> not being freed in guest_remove_page, so they are leaked and the
>> memory accounting between Xen and Dom0 differs, because Dom0 OS
>> actually thinks they are freed, but d->tot_pages is not decreased.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
>> ---
> 
> In which scenarios do pages get two refs, and why do you consider this
> to be valid?

AFAICT (given my little knowledge about the domain builder), those pages
have been allocated by alloc_domheap_pages earlier in the code, which
makes them already have 1 ref, and then this loop adds another ref to
them, which makes no sense to me.

The same happens for PV guests, but it doesn't seem to be a problem for
pure PV guests, and in fact removing this code for PV guests makes Xen
crash when the PV Dom0 boots. I would really appreciate if someone with
more knowledge on the domain builder code could comment on why this is
needed.

Roger.

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

Re: [PATCH RFC] xen: prevent PVH Dom0 from having pages with more than one ref

[Jan Beulich]

>>> Roger Pau Monné<roger.pau@citrix.com> 06/13/14 12:17 PM >>>
>AFAICT (given my little knowledge about the domain builder), those pages
>have been allocated by alloc_domheap_pages earlier in the code, which
>makes them already have 1 ref, and then this loop adds another ref to
>them, which makes no sense to me.

alloc_domheap_pages() (or really assign_pages()) takes one reference for
setting PGC_allocated. This is the reference that gets dropped when freeing
the page. The code you modify takes a second reference for the usage type
of the page becoming writable, which is a result of it getting a writable page
table entry made for it. That reference gets dropped when the respective
page table entry gets cleared.

>The same happens for PV guests, but it doesn't seem to be a problem for
>pure PV guests, and in fact removing this code for PV guests makes Xen
>crash when the PV Dom0 boots. I would really appreciate if someone with
>more knowledge on the domain builder code could comment on why this is
>needed.

So other than Andrew I think the change is correct except for being only a
partial fix - there's a second place further down where a similar piece of code
exists that would need fixing (conditionalizing) the same way.

Jan



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

Re: [PATCH RFC] xen: prevent PVH Dom0 from having pages with more than one ref

[Andrew Cooper]

On 13/06/14 12:05, Jan Beulich wrote:
>>>> Roger Pau Monné<roger.pau@citrix.com> 06/13/14 12:17 PM >>>
>> AFAICT (given my little knowledge about the domain builder), those pages
>> have been allocated by alloc_domheap_pages earlier in the code, which
>> makes them already have 1 ref, and then this loop adds another ref to
>> them, which makes no sense to me.
> alloc_domheap_pages() (or really assign_pages()) takes one reference for
> setting PGC_allocated. This is the reference that gets dropped when freeing
> the page. The code you modify takes a second reference for the usage type
> of the page becoming writable, which is a result of it getting a writable page
> table entry made for it. That reference gets dropped when the respective
> page table entry gets cleared.

Ah, so it is a genuine difference between PV and HVM memory management.

Perhaps the conditional should be based on paging_mode_translate() ?

~Andrew

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

Re: [PATCH RFC] xen: prevent PVH Dom0 from having pages with more than one ref

[Jan Beulich]

>>> Andrew Cooper <andrew.cooper3@citrix.com> 06/13/14 1:17 PM >>>
>On 13/06/14 12:05, Jan Beulich wrote:
>>>>> Roger Pau Monné<roger.pau@citrix.com> 06/13/14 12:17 PM >>>
>>> AFAICT (given my little knowledge about the domain builder), those pages
>>> have been allocated by alloc_domheap_pages earlier in the code, which
>>> makes them already have 1 ref, and then this loop adds another ref to
>>> them, which makes no sense to me.
>> alloc_domheap_pages() (or really assign_pages()) takes one reference for
>> setting PGC_allocated. This is the reference that gets dropped when freeing
>> the page. The code you modify takes a second reference for the usage type
>> of the page becoming writable, which is a result of it getting a writable page
>> table entry made for it. That reference gets dropped when the respective
>> page table entry gets cleared.
>
>Ah, so it is a genuine difference between PV and HVM memory management.
>
>Perhaps the conditional should be based on paging_mode_translate() ?

That would make sense, yes.

Jan


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

Re: [PATCH RFC] xen: prevent PVH Dom0 from having pages with more than one ref

[Roger Pau Monné]

On 13/06/14 13:05, Jan Beulich wrote:
>>>> Roger Pau Monné<roger.pau@citrix.com> 06/13/14 12:17 PM >>>
>> AFAICT (given my little knowledge about the domain builder), those pages
>> have been allocated by alloc_domheap_pages earlier in the code, which
>> makes them already have 1 ref, and then this loop adds another ref to
>> them, which makes no sense to me.
> 
> alloc_domheap_pages() (or really assign_pages()) takes one reference for
> setting PGC_allocated. This is the reference that gets dropped when freeing
> the page. The code you modify takes a second reference for the usage type
> of the page becoming writable, which is a result of it getting a writable page
> table entry made for it. That reference gets dropped when the respective
> page table entry gets cleared.
> 
>> The same happens for PV guests, but it doesn't seem to be a problem for
>> pure PV guests, and in fact removing this code for PV guests makes Xen
>> crash when the PV Dom0 boots. I would really appreciate if someone with
>> more knowledge on the domain builder code could comment on why this is
>> needed.
> 
> So other than Andrew I think the change is correct except for being only a
> partial fix - there's a second place further down where a similar piece of code
> exists that would need fixing (conditionalizing) the same way.

Thanks, that makes sense, v2 on the way.

Roger.


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