Re: dom0less boot two compressed kernel images out-of-memory work-around

[Julien Grall <julien@xxxxxxx>]

(BCCing xen-users, CCing xen-devel + a few folks)

Hi,

Moving the discussion to xen-devel.

On 22/02/2021 05:02, Charles Chiou wrote:
> When trying to boot two zImage using dom0less boot on ARM, we encountered this 
> problem when xen runs gunzip on second guest:
>
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Out of memory
> (XEN) ****************************************
>
> And worked around it with the following patch. We'd like to check to see if 
> this is a known issue and if the work-around looks reasonable. Thank you
I haven't seen any similar report in the past.

> diff --git a/xen/common/gunzip.c b/xen/common/gunzip.c
> index db4efcd34b..e5bd19ba7f 100644
> --- a/xen/common/gunzip.c
> +++ b/xen/common/gunzip.c
> @@ -113,8 +113,10 @@ __init int perform_gunzip(char *output, char *image, 
> unsigned long image_len)
>
>       window = (unsigned char *)output;
>
> +    if (!free_mem_ptr) {
>           free_mem_ptr = (unsigned long)alloc_xenheap_pages(HEAPORDER, 0);
>           free_mem_end_ptr = free_mem_ptr + (PAGE_SIZE << HEAPORDER);
> +    }
>
>       inbuf = (unsigned char *)image;
>       insize = image_len;
> @@ -131,7 +133,12 @@ __init int perform_gunzip(char *output, char *image, 
> unsigned long image_len)
>           rc = 0;
>       }
>
> +    if (free_mem_ptr) {
>           free_xenheap_pages((void *)free_mem_ptr, HEAPORDER);
> +        free_mem_ptr = 0;
> +    }
> +
> +    bytes_out = 0;
>
>       return rc;
> }
> diff --git a/xen/common/inflate.c b/xen/common/inflate.c
> index f99c985d61..de96002188 100644
> --- a/xen/common/inflate.c
> +++ b/xen/common/inflate.c
> @@ -244,7 +244,7 @@ static void *INIT malloc(int size)
>
>       if (size < 0)
>           error("Malloc error");
> -    if (!malloc_ptr)
> +    if ((!malloc_ptr) || (!malloc_count))
>           malloc_ptr = free_mem_ptr;
IMHO, this is a bit risky to assume that malloc_count will always be 0 
after each gunzip.
Instead I think, it would be better if we re-initialize the allocator 
every time. How about the following (untested):
commit e1cd2d85234c8d0aa62ad32c824a5568a57be930 (HEAD -> dev)
Author: Julien Grall <jgrall@xxxxxxxxxx>
Date:   Wed Mar 3 19:27:56 2021 +0000

    xen/gunzip: Allow perform_gunzip() to be called multiple times

    Currently perform_gunzip() can only be called once because the the
    internal allocator is not fully re-initialized.

    This works fine if you are only booting dom0. But this will break when
    booting multiple using the dom0less that uses compressed kernel images.

    This can be resolved by re-initializing malloc_ptr and malloc_count
    every time perform_gunzip() is called.

    Note the latter is only re-initialized for hardening purpose as 
there is
    no guarantee that every malloc() are followed by free() (It should in
    theory!).

    Take the opportunity to check the return of alloc_heap_pages() to 
return
    an error rather than dereferencing a NULL pointer later on failure.

    Reported-by: Charles Chiou <cchiou@xxxxxxxxxxxxx>
    Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

    ---

    This patch is candidate for Xen 4.15. Without this patch, it will 
not be
    possible to boot multiple domain using dom0less when they are using
    compressed kernel images.

diff --git a/xen/common/gunzip.c b/xen/common/gunzip.c
index db4efcd34b77..a5c2e25efc0f 100644
--- a/xen/common/gunzip.c
+++ b/xen/common/gunzip.c
@@ -114,7 +114,11 @@ __init int perform_gunzip(char *output, char 
*image, unsigned long image_len)
     window = (unsigned char *)output;

     free_mem_ptr = (unsigned long)alloc_xenheap_pages(HEAPORDER, 0);
+    if ( !free_mem_ptr )
+        return -ENOMEM;
+
     free_mem_end_ptr = free_mem_ptr + (PAGE_SIZE << HEAPORDER);
+    init_allocator();

     inbuf = (unsigned char *)image;
     insize = image_len;
diff --git a/xen/common/inflate.c b/xen/common/inflate.c
index f99c985d6135..d8c28a3e9593 100644
--- a/xen/common/inflate.c
+++ b/xen/common/inflate.c
@@ -238,6 +238,12 @@ STATIC const ush mask_bits[] = {
 static unsigned long INITDATA malloc_ptr;
 static int INITDATA malloc_count;

+static void init_allocator(void)
+{
+    malloc_ptr = free_mem_ptr;
+    malloc_count = 0;
+}
+
 static void *INIT malloc(int size)
 {
     void *p;

Best regards,

--
Julien Grall