[v4,6/8] x86: bump ZO_z_extra_bytes margin for zstd
diff mbox series

Message ID 20200401053913.216783-7-nickrterrell@gmail.com
State New
Headers show
Series
  • Add support for ZSTD-compressed kernel and initramfs
Related show

Commit Message

Nick Terrell April 1, 2020, 5:39 a.m. UTC
From: Nick Terrell <terrelln@fb.com>

Bump the ZO_z_extra_bytes margin for zstd.

Zstd needs 3 bytes per 128 KB, and has a 22 byte fixed overhead.
Zstd needs to maintain 128 KB of space at all times, since that is
the maximum block size. See the comments regarding in-place
decompression added in lib/decompress_unzstd.c for details.

Reviewed-by: Kees Cook <keescook@chromium.org>
Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
Signed-off-by: Nick Terrell <terrelln@fb.com>
---
 arch/x86/boot/header.S | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Borislav Petkov April 1, 2020, 9:33 a.m. UTC | #1
On Tue, Mar 31, 2020 at 10:39:11PM -0700, Nick Terrell wrote:
> From: Nick Terrell <terrelln@fb.com>
> 
> Bump the ZO_z_extra_bytes margin for zstd.
> 
> Zstd needs 3 bytes per 128 KB, and has a 22 byte fixed overhead.
> Zstd needs to maintain 128 KB of space at all times, since that is
> the maximum block size. See the comments regarding in-place
> decompression added in lib/decompress_unzstd.c for details.
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
> Signed-off-by: Nick Terrell <terrelln@fb.com>
> ---
>  arch/x86/boot/header.S | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> index 97d9b6d6c1af..b820875c5c95 100644
> --- a/arch/x86/boot/header.S
> +++ b/arch/x86/boot/header.S
> @@ -536,8 +536,14 @@ pref_address:		.quad LOAD_PHYSICAL_ADDR	# preferred load addr
>  # the size-dependent part now grows so fast.
>  #
>  # extra_bytes = (uncompressed_size >> 8) + 65536
> +#
> +# ZSTD compressed data grows by at most 3 bytes per 128K, and only has a 22
> +# byte fixed overhead but has a maximum block size of 128K, so it needs a
> +# larger margin.
> +#
> +# extra_bytes = (uncompressed_size >> 8) + 131072
>  
> -#define ZO_z_extra_bytes	((ZO_z_output_len >> 8) + 65536)
> +#define ZO_z_extra_bytes	((ZO_z_output_len >> 8) + 131072)
>  #if ZO_z_output_len > ZO_z_input_len
>  # define ZO_z_extract_offset	(ZO_z_output_len + ZO_z_extra_bytes - \
>  				 ZO_z_input_len)
> -- 

So why is this change unconditional if only this compression alg. needs
it?
Nick Terrell April 1, 2020, 5:33 p.m. UTC | #2
> On Apr 1, 2020, at 2:33 AM, Borislav Petkov <bp@alien8.de> wrote:
> 
> On Tue, Mar 31, 2020 at 10:39:11PM -0700, Nick Terrell wrote:
>> From: Nick Terrell <terrelln@fb.com>
>> 
>> Bump the ZO_z_extra_bytes margin for zstd.
>> 
>> Zstd needs 3 bytes per 128 KB, and has a 22 byte fixed overhead.
>> Zstd needs to maintain 128 KB of space at all times, since that is
>> the maximum block size. See the comments regarding in-place
>> decompression added in lib/decompress_unzstd.c for details.
>> 
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
>> Signed-off-by: Nick Terrell <terrelln@fb.com>
>> ---
>> arch/x86/boot/header.S | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
>> index 97d9b6d6c1af..b820875c5c95 100644
>> --- a/arch/x86/boot/header.S
>> +++ b/arch/x86/boot/header.S
>> @@ -536,8 +536,14 @@ pref_address:		.quad LOAD_PHYSICAL_ADDR	# preferred load addr
>> # the size-dependent part now grows so fast.
>> #
>> # extra_bytes = (uncompressed_size >> 8) + 65536
>> +#
>> +# ZSTD compressed data grows by at most 3 bytes per 128K, and only has a 22
>> +# byte fixed overhead but has a maximum block size of 128K, so it needs a
>> +# larger margin.
>> +#
>> +# extra_bytes = (uncompressed_size >> 8) + 131072
>> 
>> -#define ZO_z_extra_bytes	((ZO_z_output_len >> 8) + 65536)
>> +#define ZO_z_extra_bytes	((ZO_z_output_len >> 8) + 131072)
>> #if ZO_z_output_len > ZO_z_input_len
>> # define ZO_z_extract_offset	(ZO_z_output_len + ZO_z_extra_bytes - \
>> 				 ZO_z_input_len)
>> -- 
> 
> So why is this change unconditional if only this compression alg. needs
> it?

The code is currently written so that all the compression algorithms use the
same ZO_z_extra_bytes. It is taken to be the maximum of the growth rate
plus the maximum fixed overhead. Just a few lines above is the comment:

# … Hence safety
# margin should be updated to cover all decompressors so that we don't
# need to deal with each of them separately. Please check
# the description in lib/decompressor_xxx.c for specific information.

So I was been following the guidance in the comments. If we want to refactor
it to handle each compressor individually we could make ZO_z_extra_bytes
smaller for most algorithms.

Does it matter? I’m not an expert here, but it seems to me that requiring an
extra 64 KB of RAM for kernel decompression isn’t such an onerous addition.

Best,
Nick
Borislav Petkov April 2, 2020, 3:58 p.m. UTC | #3
On Wed, Apr 01, 2020 at 05:33:03PM +0000, Nick Terrell wrote:
> The code is currently written so that all the compression algorithms use the
> same ZO_z_extra_bytes. It is taken to be the maximum of the growth rate
> plus the maximum fixed overhead. Just a few lines above is the comment:
> 
> # … Hence safety
> # margin should be updated to cover all decompressors so that we don't
> # need to deal with each of them separately. Please check
> # the description in lib/decompressor_xxx.c for specific information.
> 
> So I was been following the guidance in the comments.

Please state that in the commit message when you send your next
revision.

> Does it matter? I’m not an expert here,

Huh, you're only sending the code then? Or what do you mean with not
being an expert?

Thx.
Nick Terrell April 2, 2020, 8:25 p.m. UTC | #4
> On Apr 2, 2020, at 8:58 AM, Borislav Petkov <bp@alien8.de> wrote:
> 
> On Wed, Apr 01, 2020 at 05:33:03PM +0000, Nick Terrell wrote:
>> The code is currently written so that all the compression algorithms use the
>> same ZO_z_extra_bytes. It is taken to be the maximum of the growth rate
>> plus the maximum fixed overhead. Just a few lines above is the comment:
>> 
>> # … Hence safety
>> # margin should be updated to cover all decompressors so that we don't
>> # need to deal with each of them separately. Please check
>> # the description in lib/decompressor_xxx.c for specific information.
>> 
>> So I was been following the guidance in the comments.
> 
> Please state that in the commit message when you send your next
> revision.

Will do.

>> Does it matter? I’m not an expert here,
> 
> Huh, you're only sending the code then? Or what do you mean with not
> being an expert?

I mean that while I’ve read and understood this piece of the code, have tested
the patches, have followed the template of other compression methods
added, and am confident in the correctness of the code, I’m not a regular
contributor to the pre-boot x86 kernel code. So it is possible that there is a
use case for kernel compression that I’m not aware of where RAM is extremely
tight and within 64 KB of the current limits.

It seems to me that adding 64KB to the memory requirement for kernel
decompression is not going to break anyone. If it did the kernel image is taking
up nearly all available RAM, which doesn’t seem likely. But, I don’t know all
use cases. If it does break someone, we can put up a separate patch that
switches all the compression methods over a per-method ZO_z_extra_bytes.

Best,
Nick
Borislav Petkov April 3, 2020, 10:19 a.m. UTC | #5
On Thu, Apr 02, 2020 at 08:25:49PM +0000, Nick Terrell wrote:
> So it is possible that there is a use case for kernel compression that
> I’m not aware of where RAM is extremely tight and within 64 KB of
> the current limits.

That's exactly my concern, albeit a very minor one.

> It seems to me that adding 64KB to the memory requirement for kernel
> decompression is not going to break anyone. If it did the kernel image
> is taking up nearly all available RAM, which doesn’t seem likely.
> But, I don’t know all use cases. If it does break someone, we can
> put up a separate patch that switches all the compression methods over
> a per-method ZO_z_extra_bytes.

Ok.

Thx.
Sedat Dilek April 6, 2020, 10:47 a.m. UTC | #6
On Thu, Apr 2, 2020 at 10:26 PM Nick Terrell <terrelln@fb.com> wrote:
>
>
>
> > On Apr 2, 2020, at 8:58 AM, Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Wed, Apr 01, 2020 at 05:33:03PM +0000, Nick Terrell wrote:
> >> The code is currently written so that all the compression algorithms use the
> >> same ZO_z_extra_bytes. It is taken to be the maximum of the growth rate
> >> plus the maximum fixed overhead. Just a few lines above is the comment:
> >>
> >> # … Hence safety
> >> # margin should be updated to cover all decompressors so that we don't
> >> # need to deal with each of them separately. Please check
> >> # the description in lib/decompressor_xxx.c for specific information.
> >>
> >> So I was been following the guidance in the comments.
> >
> > Please state that in the commit message when you send your next
> > revision.
>
> Will do.
>
> >> Does it matter? I’m not an expert here,
> >
> > Huh, you're only sending the code then? Or what do you mean with not
> > being an expert?
>
> I mean that while I’ve read and understood this piece of the code, have tested
> the patches, have followed the template of other compression methods
> added, and am confident in the correctness of the code, I’m not a regular
> contributor to the pre-boot x86 kernel code. So it is possible that there is a
> use case for kernel compression that I’m not aware of where RAM is extremely
> tight and within 64 KB of the current limits.
>
> It seems to me that adding 64KB to the memory requirement for kernel
> decompression is not going to break anyone. If it did the kernel image is taking
> up nearly all available RAM, which doesn’t seem likely. But, I don’t know all
> use cases. If it does break someone, we can put up a separate patch that
> switches all the compression methods over a per-method ZO_z_extra_bytes.
>

Hi Nick,

are you planning a zstd-v5?
If yes, please CC me, thanks.

Regards,
- Sedat -

Patch
diff mbox series

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 97d9b6d6c1af..b820875c5c95 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -536,8 +536,14 @@  pref_address:		.quad LOAD_PHYSICAL_ADDR	# preferred load addr
 # the size-dependent part now grows so fast.
 #
 # extra_bytes = (uncompressed_size >> 8) + 65536
+#
+# ZSTD compressed data grows by at most 3 bytes per 128K, and only has a 22
+# byte fixed overhead but has a maximum block size of 128K, so it needs a
+# larger margin.
+#
+# extra_bytes = (uncompressed_size >> 8) + 131072
 
-#define ZO_z_extra_bytes	((ZO_z_output_len >> 8) + 65536)
+#define ZO_z_extra_bytes	((ZO_z_output_len >> 8) + 131072)
 #if ZO_z_output_len > ZO_z_input_len
 # define ZO_z_extract_offset	(ZO_z_output_len + ZO_z_extra_bytes - \
 				 ZO_z_input_len)