Re: dom0less boot two compressed kernel images out-of-memory work-around
(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 Date: Wed Mar 3 19:27:56 2021 + 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 Signed-off-by: Julien Grall --- 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
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 > Date: Wed Mar 3 19:27:56 2021 + > > 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 > Signed-off-by: Julien Grall > > --- > > 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
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 Date: Wed Mar 3 19:27:56 2021 + 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 Signed-off-by: Julien Grall --- 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