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

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

[Julien Grall]

(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@amazon.com>
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@ambarella.com>
     Signed-off-by: Julien Grall <jgrall@amazon.com>

     ---

     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

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

[Jan Beulich]

On 03.03.2021 20:36, Julien Grall wrote:
> (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.

I agree.

> How about the following (untested):
> 
> commit e1cd2d85234c8d0aa62ad32c824a5568a57be930 (HEAD -> dev)
> Author: Julien Grall <jgrall@amazon.com>
> 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@ambarella.com>
>      Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
>      ---
> 
>      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.

Other decompression methods are unaffected?

> --- 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)

Please add INIT here. (I wouldn't mind if you used __init instead,
as there's going to be file-wide replacement after 4.15 anyway,
but of course this would render things inconsistent until then.)

Jan

> +{
> +    malloc_ptr = free_mem_ptr;
> +    malloc_count = 0;
> +}
> +
>   static void *INIT malloc(int size)
>   {
>       void *p;
> 
> Best regards,
> 

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

[Julien Grall]

Hi Jan,

On 04/03/2021 08:34, Jan Beulich wrote:
> On 03.03.2021 20:36, Julien Grall wrote:
>> (BCCing xen-users, CCing xen-devel + a few folks)
>> How about the following (untested):
>>
>> commit e1cd2d85234c8d0aa62ad32c824a5568a57be930 (HEAD -> dev)
>> Author: Julien Grall <jgrall@amazon.com>
>> 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@ambarella.com>
>>       Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>>       ---
>>
>>       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.
> 
> Other decompression methods are unaffected?

Arm is only using gzip so far. I quickly looked through bunzip2 and 
unlz4 (I know there are others), they look fine because they don't 
allocate a large global pool.

We probably want to check the others.

> 
>> --- 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)
> 
> Please add INIT here. (I wouldn't mind if you used __init instead,
> as there's going to be file-wide replacement after 4.15 anyway,
> but of course this would render things inconsistent until then.)

I will use INIT. I will wait a bit before sending the patch formally 
(I'd like a confirmation that it solves the problem reported).

Cheers,

-- 
Julien Grall