Re: [PATCH v4 6/8] x86: bump ZO_z_extra_bytes margin for zstd

[Nick Terrell <terrelln@fb.com>]

> 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