xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels
@ 2021-04-02 15:21 Julien Grall
  2021-04-02 15:21 ` [PATCH 1/2] xen/arm: kernel: Propagate the error if we fail to decompress the kernel Julien Grall
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Julien Grall @ 2021-04-02 15:21 UTC (permalink / raw)
  To: xen-devel
  Cc: bertrand.marquis, Julien Grall, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu

From: Julien Grall <jgrall@amazon.com>

Hi all,

The main goal of this series is to address the bug report [1]. It is not
possible

While testing the series, I also noticed that it is not possible to
re-use the same compressed kernel for multiple domains as the module
will be overwritten during the first decompression.

I am still a bit undecided how to fix it. We can either create a new
module for the uncompressed kernel and link with the compressed kernel.
Or we could decompress everytime.

One inconvenience to kernel to only decompress once is we have to keep
it until all the domains have booted. This may means a lot of memory to be
wasted just for keeping the uncompressed kernel (one my setup, it takes
~3 times more space).

Any opinions on how to approach it?

Cheers,

[1] https://lists.xenproject.org/archives/html/xen-users/2021-02/msg00027.html

Julien Grall (2):
  xen/arm: kernel: Propagate the error if we fail to decompress the
    kernel
  xen/gunzip: Allow perform_gunzip() to be called multiple times

 xen/arch/arm/kernel.c | 8 +++++++-
 xen/common/gunzip.c   | 5 +++++
 xen/common/inflate.c  | 6 ++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.17.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] xen/arm: kernel: Propagate the error if we fail to decompress the kernel
  2021-04-02 15:21 [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels Julien Grall
@ 2021-04-02 15:21 ` Julien Grall
  2021-04-06 19:15   ` Julien Grall
  2021-04-02 15:21 ` [PATCH 2/2] xen/gunzip: Allow perform_gunzip() to be called multiple times Julien Grall
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Julien Grall @ 2021-04-02 15:21 UTC (permalink / raw)
  To: xen-devel
  Cc: bertrand.marquis, Julien Grall, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk

From: Julien Grall <jgrall@amazon.com>

Currently, we are ignoring any error from perform_gunzip() and replacing
the compressed kernel with the "uncompressed" kernel.

If there is a gzip failure, then it means that the output buffer may
contain garbagge. So it can result to various sort of behavior that may
be difficult to root cause.

In case of failure, free the output buffer and propagate the error.
We also need to adjust the return check for kernel_compress() as
perform_gunzip() may return a positive value.

Take the opportunity to adjust the code style for the check.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/kernel.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index ab78689ed2a6..f6b60ab77355 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -292,6 +292,12 @@ static __init int kernel_decompress(struct bootmodule *mod)
     iounmap(input);
     vunmap(output);
 
+    if ( rc )
+    {
+        free_domheap_pages(pages);
+        return rc;
+    }
+
     mod->start = page_to_maddr(pages);
     mod->size = output_size;
 
@@ -503,7 +509,7 @@ int __init kernel_probe(struct kernel_info *info,
 
     /* if it is a gzip'ed image, 32bit or 64bit, uncompress it */
     rc = kernel_decompress(mod);
-    if (rc < 0 && rc != -EINVAL)
+    if ( rc && rc != -EINVAL )
         return rc;
 
 #ifdef CONFIG_ARM_64
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] xen/gunzip: Allow perform_gunzip() to be called multiple times
  2021-04-02 15:21 [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels Julien Grall
  2021-04-02 15:21 ` [PATCH 1/2] xen/arm: kernel: Propagate the error if we fail to decompress the kernel Julien Grall
@ 2021-04-02 15:21 ` Julien Grall
  2021-04-06  7:40   ` Jan Beulich
  2021-04-07 10:39   ` Jan Beulich
  2021-04-06  7:45 ` [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels Jan Beulich
  2021-04-06 18:31 ` Julien Grall
  3 siblings, 2 replies; 10+ messages in thread
From: Julien Grall @ 2021-04-02 15:21 UTC (permalink / raw)
  To: xen-devel
  Cc: bertrand.marquis, Julien Grall, Andrew Cooper, George Dunlap,
	Ian Jackson, Jan Beulich, Julien Grall, Stefano Stabellini,
	Wei Liu

From: Julien Grall <jgrall@amazon.com>

Currently perform_gunzip() can only be called once because the
the internal state (e.g allocate) 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 bytes_out, malloc_ptr,
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>
---
 xen/common/gunzip.c  | 5 +++++
 xen/common/inflate.c | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/xen/common/gunzip.c b/xen/common/gunzip.c
index db4efcd34b77..425d64e904d8 100644
--- a/xen/common/gunzip.c
+++ b/xen/common/gunzip.c
@@ -114,11 +114,16 @@ __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;
     inptr = 0;
+    bytes_out = 0;
 
     makecrc();
 
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;
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] xen/gunzip: Allow perform_gunzip() to be called multiple times
  2021-04-02 15:21 ` [PATCH 2/2] xen/gunzip: Allow perform_gunzip() to be called multiple times Julien Grall
@ 2021-04-06  7:40   ` Jan Beulich
  2021-04-07 10:39   ` Jan Beulich
  1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2021-04-06  7:40 UTC (permalink / raw)
  To: Julien Grall
  Cc: bertrand.marquis, Julien Grall, Andrew Cooper, George Dunlap,
	Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel

On 02.04.2021 17:21, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Currently perform_gunzip() can only be called once because the
> the internal state (e.g allocate) 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 bytes_out, malloc_ptr,
> 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>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels
  2021-04-02 15:21 [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels Julien Grall
  2021-04-02 15:21 ` [PATCH 1/2] xen/arm: kernel: Propagate the error if we fail to decompress the kernel Julien Grall
  2021-04-02 15:21 ` [PATCH 2/2] xen/gunzip: Allow perform_gunzip() to be called multiple times Julien Grall
@ 2021-04-06  7:45 ` Jan Beulich
  2021-04-06 14:13   ` Julien Grall
  2021-04-06 18:31 ` Julien Grall
  3 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2021-04-06  7:45 UTC (permalink / raw)
  To: Julien Grall
  Cc: bertrand.marquis, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Wei Liu, xen-devel

On 02.04.2021 17:21, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Hi all,
> 
> The main goal of this series is to address the bug report [1]. It is not
> possible

...?

> While testing the series, I also noticed that it is not possible to
> re-use the same compressed kernel for multiple domains as the module
> will be overwritten during the first decompression.
> 
> I am still a bit undecided how to fix it. We can either create a new
> module for the uncompressed kernel and link with the compressed kernel.
> Or we could decompress everytime.
> 
> One inconvenience to kernel to only decompress once is we have to keep
> it until all the domains have booted. This may means a lot of memory to be
> wasted just for keeping the uncompressed kernel (one my setup, it takes
> ~3 times more space).
> 
> Any opinions on how to approach it?

Well, it's not "until all the domains have booted", but until all the
domains have had their kernel image placed in the designated piece of
memory. So while for the time being multiple decompression may indeed
be a reasonable approach, longer term one could populate all the
domains' memory in two steps - first just the kernel space for all of
them, then load the kernel(s), then populate the rest of the memory.

Jan


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels
  2021-04-06  7:45 ` [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels Jan Beulich
@ 2021-04-06 14:13   ` Julien Grall
  0 siblings, 0 replies; 10+ messages in thread
From: Julien Grall @ 2021-04-06 14:13 UTC (permalink / raw)
  To: Jan Beulich
  Cc: bertrand.marquis, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Wei Liu, xen-devel

Hi Jan,

On 06/04/2021 08:45, Jan Beulich wrote:
> On 02.04.2021 17:21, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Hi all,
>>
>> The main goal of this series is to address the bug report [1]. It is not
>> possible
> 
> ...?

Urgh, I forgot to add the rest of the sentence :/. I was meant to say 
that it is not possible to decompress multiple kernels (doesn't need to 
be the same) today.

> 
>> While testing the series, I also noticed that it is not possible to
>> re-use the same compressed kernel for multiple domains as the module
>> will be overwritten during the first decompression.
>>
>> I am still a bit undecided how to fix it. We can either create a new
>> module for the uncompressed kernel and link with the compressed kernel.
>> Or we could decompress everytime.
>>
>> One inconvenience to kernel to only decompress once is we have to keep
>> it until all the domains have booted. This may means a lot of memory to be
>> wasted just for keeping the uncompressed kernel (one my setup, it takes
>> ~3 times more space).
>>
>> Any opinions on how to approach it?
> 
> Well, it's not "until all the domains have booted", but until all the
> domains have had their kernel image placed in the designated piece of
> memory. So while for the time being multiple decompression may indeed
> be a reasonable approach, longer term one could populate all the
> domains' memory in two steps - first just the kernel space for all of
> them, then load the kernel(s), then populate the rest of the memory.

Right, I am not sure this brings us to a better position. We would want 
to use superpage mapping for the guest memory as much as possible for 
performance reason.

So we may end up to populate the full memory of each guests. Therefore, 
I am not sure the split is that worth it.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels
  2021-04-02 15:21 [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels Julien Grall
                   ` (2 preceding siblings ...)
  2021-04-06  7:45 ` [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels Jan Beulich
@ 2021-04-06 18:31 ` Julien Grall
  3 siblings, 0 replies; 10+ messages in thread
From: Julien Grall @ 2021-04-06 18:31 UTC (permalink / raw)
  To: xen-devel
  Cc: bertrand.marquis, Julien Grall, Stefano Stabellini,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Wei Liu

Hi,

On 02/04/2021 16:21, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Hi all,
> 
> The main goal of this series is to address the bug report [1]. It is not
> possible
> 
> While testing the series, I also noticed that it is not possible to
> re-use the same compressed kernel for multiple domains as the module
> will be overwritten during the first decompression.
> 
> I am still a bit undecided how to fix it. We can either create a new
> module for the uncompressed kernel and link with the compressed kernel.
> Or we could decompress everytime.
> 
> One inconvenience to kernel to only decompress once is we have to keep
> it until all the domains have booted. This may means a lot of memory to be
> wasted just for keeping the uncompressed kernel (one my setup, it takes
> ~3 times more space).
> 
> Any opinions on how to approach it?
> 
> Cheers,
> 
> [1] https://lists.xenproject.org/archives/html/xen-users/2021-02/msg00027.html
> 
> Julien Grall (2):
>    xen/arm: kernel: Propagate the error if we fail to decompress the
>      kernel
>    xen/gunzip: Allow perform_gunzip() to be called multiple times

I have committed the second patch as it is independent. The first patch 
still need some reviews.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] xen/arm: kernel: Propagate the error if we fail to decompress the kernel
  2021-04-02 15:21 ` [PATCH 1/2] xen/arm: kernel: Propagate the error if we fail to decompress the kernel Julien Grall
@ 2021-04-06 19:15   ` Julien Grall
  0 siblings, 0 replies; 10+ messages in thread
From: Julien Grall @ 2021-04-06 19:15 UTC (permalink / raw)
  To: xen-devel
  Cc: bertrand.marquis, Julien Grall, Stefano Stabellini, Volodymyr Babchuk



On 02/04/2021 16:21, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Currently, we are ignoring any error from perform_gunzip() and replacing
> the compressed kernel with the "uncompressed" kernel.
> 
> If there is a gzip failure, then it means that the output buffer may
> contain garbagge. So it can result to various sort of behavior that may
> be difficult to root cause.
> 
> In case of failure, free the output buffer and propagate the error.
> We also need to adjust the return check for kernel_compress() as
> perform_gunzip() may return a positive value.
> 
> Take the opportunity to adjust the code style for the check.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>   xen/arch/arm/kernel.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index ab78689ed2a6..f6b60ab77355 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -292,6 +292,12 @@ static __init int kernel_decompress(struct bootmodule *mod)
>       iounmap(input);
>       vunmap(output);
>   
> +    if ( rc )
> +    {
> +        free_domheap_pages(pages);

This breaks the build. I am not sure how I managed to test it before... 
I will send a v2.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] xen/gunzip: Allow perform_gunzip() to be called multiple times
  2021-04-02 15:21 ` [PATCH 2/2] xen/gunzip: Allow perform_gunzip() to be called multiple times Julien Grall
  2021-04-06  7:40   ` Jan Beulich
@ 2021-04-07 10:39   ` Jan Beulich
  2021-04-07 18:18     ` Julien Grall
  1 sibling, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2021-04-07 10:39 UTC (permalink / raw)
  To: Julien Grall
  Cc: bertrand.marquis, Julien Grall, Andrew Cooper, George Dunlap,
	Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel

On 02.04.2021 17:21, Julien Grall wrote:
> --- 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;

I'm sorry for noticing this only now, but I'm afraid this may break
the build in certain environments / configurations. Iirc clang is
relatively likely to not inline functions in debug builds even when
they're used just once. Yet when the new function doesn't end up
getting inlined, it needs INIT added or else the cmd_obj_init_o
checking would find a non-empty .text section. (If there's no actual
build breakage anywhere, I can also address this in my to-be-re-based
"gunzip: drop INIT{,DATA} and STATIC", which is intended to go in as
soon as the tree is fully open again.)

Jan


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] xen/gunzip: Allow perform_gunzip() to be called multiple times
  2021-04-07 10:39   ` Jan Beulich
@ 2021-04-07 18:18     ` Julien Grall
  0 siblings, 0 replies; 10+ messages in thread
From: Julien Grall @ 2021-04-07 18:18 UTC (permalink / raw)
  To: Jan Beulich
  Cc: bertrand.marquis, Julien Grall, Andrew Cooper, George Dunlap,
	Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel

Hi Jan,

On 07/04/2021 11:39, Jan Beulich wrote:
> On 02.04.2021 17:21, Julien Grall wrote:
>> --- 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;
> 
> I'm sorry for noticing this only now, but I'm afraid this may break
> the build in certain environments / configurations.

You actually mentioned it on the original thread that reported the bug. 
But I forgot to add INIT. Sorry for that :(.

> Iirc clang is
> relatively likely to not inline functions in debug builds even when
> they're used just once. Yet when the new function doesn't end up
> getting inlined, it needs INIT added or else the cmd_obj_init_o
> checking would find a non-empty .text section. (If there's no actual
> build breakage anywhere, I can also address this in my to-be-re-based
> "gunzip: drop INIT{,DATA} and STATIC", which is intended to go in as
> soon as the tree is fully open again.)

The pipeline actually reported some failure. But I initially didn't 
notice them. I think we want to fix it because your rework, so I will 
send a patch.

Sorry for the breakage.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-04-07 18:18 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-02 15:21 [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels Julien Grall
2021-04-02 15:21 ` [PATCH 1/2] xen/arm: kernel: Propagate the error if we fail to decompress the kernel Julien Grall
2021-04-06 19:15   ` Julien Grall
2021-04-02 15:21 ` [PATCH 2/2] xen/gunzip: Allow perform_gunzip() to be called multiple times Julien Grall
2021-04-06  7:40   ` Jan Beulich
2021-04-07 10:39   ` Jan Beulich
2021-04-07 18:18     ` Julien Grall
2021-04-06  7:45 ` [PATCH 0/2] xen/arm: Couple of bug fixes when decompressing kernels Jan Beulich
2021-04-06 14:13   ` Julien Grall
2021-04-06 18:31 ` Julien Grall

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).