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

[Julien Grall <julien@xxxxxxx>]

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@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.
> 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