[Altp2m,cleanup,2/3,v12,2/3] Altp2m cleanup: cleaning up partial memory allocations in hap_enable().

Commit Message

Paul Lai Nov. 10, 2016, 11:45 p.m. UTC

In hap_enable(), clean up memory leaks upon failure of altp2m_domain_init().
Comment the memory error handling to help match allocs with cleanups.
Consolidate the memory error handing into single code path.
---
 xen/arch/x86/mm/hap/hap.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

Comments

Jan Beulich Nov. 15, 2016, 2:28 p.m. UTC | #1

>>> On 11.11.16 at 00:45, <[e-mail]> wrote:
> @@ -488,43 +489,44 @@ int hap_enable(struct domain *d, u32 mode)
>      /* allocate P2m table */
>      if ( mode & PG_translate )
>      {
> +        /* p2m_alloc_table() #1 */
>          rv = p2m_alloc_table(p2m_get_hostp2m(d));
>          if ( rv != 0 )
>              goto out;
>      }
>  
>      for (i = 0; i < MAX_NESTEDP2M; i++) {
> +        /* nested p2m_alloc_table() #2 */

I don't consider these comments particularly useful.

>          rv = p2m_alloc_table(d->arch.nested_p2m[i]);
>          if ( rv != 0 )
>             goto out;
>      }
>  
> -    if ( hvm_altp2m_supported() )
> -    {
> -        /* Init alternate p2m data */
> -        if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> -        {
> -            rv = -ENOMEM;
> -            goto out;
> -        }
> +    if ( (rv = altp2m_domain_init(d)) < 0 )
> +        goto out;

Hmm, so you at once switch to the new function here. I think I've
been quite clear on previous rounds that the bug fix should be
first in the series, to aid potential backporting. That said, the
description still does not make clear that there is any issue here
in the first place. I can only re-iterate: Whether there's a leak to
be fixed here depends on how this function and any error
cleanup up the call stack leading here interact. And in order to
determine whether the patch here is a bug fix or mere cleanup
(and maybe unnecessary, due to only making the code bigger
and hence [slightly] harder to read), this is imperative to know.
I think you understand that the decision whether to backport
such a change also depends on knowing whether it fixes an
actual bug.

> -        for ( i = 0; i < MAX_EPTP; i++ )
> -            d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> +    /* Now let other users see the new mode */
> +    d->arch.paging.mode = mode | PG_HAP_enable;
>  
> -        for ( i = 0; i < MAX_ALTP2M; i++ )
> -        {
> -            rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> -            if ( rv != 0 )
> -               goto out;
> -        }
> +    domain_unpause(d);
> +    return rv;
>  
> -        d->arch.altp2m_active = 0;
> + out:
> +    /* Unravel the partial nested p2m_alloc_table() #2 above */
> +    for ( i = 0; i < MAX_NESTEDP2M; i++ )
> +    {
> +        p2m_teardown(d->arch.nested_p2m[i]);
>      }

Unnecessary braces.

Jan

Patch

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 7fe6f83..e63ffb0 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -37,6 +37,7 @@ 
 #include <asm/hap.h>
 #include <asm/paging.h>
 #include <asm/p2m.h>
+#include <asm/altp2m.h>
 #include <asm/domain.h>
 #include <xen/numa.h>
 #include <asm/hvm/nestedhvm.h>
@@ -488,43 +489,44 @@  int hap_enable(struct domain *d, u32 mode)
     /* allocate P2m table */
     if ( mode & PG_translate )
     {
+        /* p2m_alloc_table() #1 */
         rv = p2m_alloc_table(p2m_get_hostp2m(d));
         if ( rv != 0 )
             goto out;
     }
 
     for (i = 0; i < MAX_NESTEDP2M; i++) {
+        /* nested p2m_alloc_table() #2 */
         rv = p2m_alloc_table(d->arch.nested_p2m[i]);
         if ( rv != 0 )
            goto out;
     }
 
-    if ( hvm_altp2m_supported() )
-    {
-        /* Init alternate p2m data */
-        if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
-        {
-            rv = -ENOMEM;
-            goto out;
-        }
+    if ( (rv = altp2m_domain_init(d)) < 0 )
+        goto out;
 
-        for ( i = 0; i < MAX_EPTP; i++ )
-            d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
+    /* Now let other users see the new mode */
+    d->arch.paging.mode = mode | PG_HAP_enable;
 
-        for ( i = 0; i < MAX_ALTP2M; i++ )
-        {
-            rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
-            if ( rv != 0 )
-               goto out;
-        }
+    domain_unpause(d);
+    return rv;
 
-        d->arch.altp2m_active = 0;
+ out:
+    /* Unravel the partial nested p2m_alloc_table() #2 above */
+    for ( i = 0; i < MAX_NESTEDP2M; i++ )
+    {
+        p2m_teardown(d->arch.nested_p2m[i]);
     }
 
-    /* Now let other users see the new mode */
-    d->arch.paging.mode = mode | PG_HAP_enable;
+    /* Unravel p2m_alloc_table() #1 above */
+    p2m_teardown(p2m_get_hostp2m(d));
+
+    /* Unravel the hap_set_allocation(d, 256, NULL) above */
+    paging_lock(d);
+    hap_set_allocation(d, 0, NULL);
+    ASSERT(d->arch.paging.hap.p2m_pages == 0);
+    paging_unlock(d);
 
- out:
     domain_unpause(d);
     return rv;
 }