| Message ID | 20200401053913.216783-7-nickrterrell@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for ZSTD-compressed kernel and initramfs | expand |
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?
> 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
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.
> 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
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.
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 -
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)