| Message ID | 20221006224212.569555-6-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some pstore improvements | expand |
On Thu, Oct 06, 2022 at 07:42:09PM -0300, Guilherme G. Piccoli wrote: > The pstore infrastructure is capable of compressing collected logs, > relying for that in many compression "libraries" present on kernel. > Happens that the (de)compression code in pstore performs many > implicit conversions from unsigned int/size_t to int, and vice-versa. > Specially in the compress buffer size calculation, we notice that > even the libs are not consistent, some of them return int, most of > them unsigned int and others rely on preprocessor calculation. > > Here is an attempt to make it consistent: since we're talking > about buffer sizes, let's go with unsigned types, since negative > sizes don't make sense. Thanks for this! I want to go through this more carefully, but I'm a fan of the clean-up. I'd also like to get Ard's compression refactor landed again, and then do this on top of it.
On Fri, 7 Oct 2022 at 01:36, Kees Cook <keescook@chromium.org> wrote: > > On Thu, Oct 06, 2022 at 07:42:09PM -0300, Guilherme G. Piccoli wrote: > > The pstore infrastructure is capable of compressing collected logs, > > relying for that in many compression "libraries" present on kernel. > > Happens that the (de)compression code in pstore performs many > > implicit conversions from unsigned int/size_t to int, and vice-versa. > > Specially in the compress buffer size calculation, we notice that > > even the libs are not consistent, some of them return int, most of > > them unsigned int and others rely on preprocessor calculation. > > > > Here is an attempt to make it consistent: since we're talking > > about buffer sizes, let's go with unsigned types, since negative > > sizes don't make sense. > > Thanks for this! I want to go through this more carefully, but I'm a fan > of the clean-up. I'd also like to get Ard's compression refactor landed > again, and then do this on top of it. > Isn't this the stuff we want to move into the crypto API?
On Fri, Oct 07, 2022 at 10:47:01AM +0200, Ard Biesheuvel wrote: > On Fri, 7 Oct 2022 at 01:36, Kees Cook <keescook@chromium.org> wrote: > > > > On Thu, Oct 06, 2022 at 07:42:09PM -0300, Guilherme G. Piccoli wrote: > > > The pstore infrastructure is capable of compressing collected logs, > > > relying for that in many compression "libraries" present on kernel. > > > Happens that the (de)compression code in pstore performs many > > > implicit conversions from unsigned int/size_t to int, and vice-versa. > > > Specially in the compress buffer size calculation, we notice that > > > even the libs are not consistent, some of them return int, most of > > > them unsigned int and others rely on preprocessor calculation. > > > > > > Here is an attempt to make it consistent: since we're talking > > > about buffer sizes, let's go with unsigned types, since negative > > > sizes don't make sense. > > > > Thanks for this! I want to go through this more carefully, but I'm a fan > > of the clean-up. I'd also like to get Ard's compression refactor landed > > again, and then do this on top of it. > > > > Isn't this the stuff we want to move into the crypto API? It is, yes. Guilherme, for background: https://lore.kernel.org/linux-crypto/20180802215118.17752-1-keescook@chromium.org/
On 07/10/2022 16:37, Kees Cook wrote: > On Fri, Oct 07, 2022 at 10:47:01AM +0200, Ard Biesheuvel wrote: >> [...] >> Isn't this the stuff we want to move into the crypto API? > > It is, yes. Guilherme, for background: > https://lore.kernel.org/linux-crypto/20180802215118.17752-1-keescook@chromium.org/ > Thanks a bunch Kees / Ard for pointing me that! But I'm confused with regards to the state of this conversion: the patches seem to be quite mature and work fine, but at the same time, they focus in what Herbert consider a deprecated/old API, so they were never merged right? The proposal from Ard (to move to crypto scomp/acomp) allow to rework the zbufsize worst case thing and wire it on such new API, correct? Do you intend to do that Kees? At the same time, I feel it is still valid to avoid these bunch of implicit conversions on pstore, as this patch proposes - what do you all think? I could rework this one on top of Ard's acomp migration while we don't have an official zbufsize API for on crypto scomp - and once we have, it'd be just a matter of removing the zbufsize functions of pstore and make use of the new API, which shouldn't be affected by this implicit conversion fix. Cheers, Guilherme
On Sat, 8 Oct 2022 at 16:14, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote: > > On 07/10/2022 16:37, Kees Cook wrote: > > On Fri, Oct 07, 2022 at 10:47:01AM +0200, Ard Biesheuvel wrote: > >> [...] > >> Isn't this the stuff we want to move into the crypto API? > > > > It is, yes. Guilherme, for background: > > https://lore.kernel.org/linux-crypto/20180802215118.17752-1-keescook@chromium.org/ > > > > Thanks a bunch Kees / Ard for pointing me that! > > But I'm confused with regards to the state of this conversion: the > patches seem to be quite mature and work fine, but at the same time, > they focus in what Herbert consider a deprecated/old API, so they were > never merged right? > > The proposal from Ard (to move to crypto scomp/acomp) allow to rework > the zbufsize worst case thing and wire it on such new API, correct? Do > you intend to do that Kees? > > At the same time, I feel it is still valid to avoid these bunch of > implicit conversions on pstore, as this patch proposes - what do you all > think? > > I could rework this one on top of Ard's acomp migration while we don't > have an official zbufsize API for on crypto scomp - and once we have, > it'd be just a matter of removing the zbufsize functions of pstore and > make use of the new API, which shouldn't be affected by this implicit > conversion fix. > So one thing I don't understand about these changes is why we need them in the first place. The zbufsize routines are all worst case routines, which means each one of those will return a value that exceeds the size parameter. We only use compression for dmesg, which compresses quite well. If it doesn't compress well, there is probably something wrong with the input data, so preserving it may not be as critical. And if compressing the data makes it bigger, can't we just omit the compression for that particular record? In summary, while adding zbufsize to the crypto API seems a reasonable thing to do, I don't see why we'd want to make use of it in pstore - can't we just use the decompressed size as the worst case compressed size for all algorithms, and skip the compression if it doesn't fit? Or am I missing something here?
On 08/10/2022 12:53, Ard Biesheuvel wrote: > [...] > So one thing I don't understand about these changes is why we need > them in the first place. > > The zbufsize routines are all worst case routines, which means each > one of those will return a value that exceeds the size parameter. > > We only use compression for dmesg, which compresses quite well. If it > doesn't compress well, there is probably something wrong with the > input data, so preserving it may not be as critical. And if > compressing the data makes it bigger, can't we just omit the > compression for that particular record? > > In summary, while adding zbufsize to the crypto API seems a reasonable > thing to do, I don't see why we'd want to make use of it in pstore - > can't we just use the decompressed size as the worst case compressed > size for all algorithms, and skip the compression if it doesn't fit? > > Or am I missing something here? In a way (and if I understand you correctly - please let me know if not) you are making lot of sense: why not just use the maximum size (which is the full decompressed size + header) as the worst case in pstore and skip these highly specialized routines that calculate the worst case for each algorithm, right? This is exactly what 842 (sw compress) is doing now. If that's interesting and Kees agrees, and if nobody else plans on doing that, I could work on it. Extra question (maybe silly on my side?): is it possible that _compressed_ data is bigger than the original one? Isn't there any "protection" on the compress APIs for that? In that case, it'd purely waste of time / CPU cycles heheh Cheers, Guilherme
On Sat, 8 Oct 2022 at 18:04, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote: > > On 08/10/2022 12:53, Ard Biesheuvel wrote: > > [...] > > So one thing I don't understand about these changes is why we need > > them in the first place. > > > > The zbufsize routines are all worst case routines, which means each > > one of those will return a value that exceeds the size parameter. > > > > We only use compression for dmesg, which compresses quite well. If it > > doesn't compress well, there is probably something wrong with the > > input data, so preserving it may not be as critical. And if > > compressing the data makes it bigger, can't we just omit the > > compression for that particular record? > > > > In summary, while adding zbufsize to the crypto API seems a reasonable > > thing to do, I don't see why we'd want to make use of it in pstore - > > can't we just use the decompressed size as the worst case compressed > > size for all algorithms, and skip the compression if it doesn't fit? > > > > Or am I missing something here? > > In a way (and if I understand you correctly - please let me know if not) > you are making lot of sense: why not just use the maximum size (which is > the full decompressed size + header) as the worst case in pstore and > skip these highly specialized routines that calculate the worst case for > each algorithm, right? > Yes > This is exactly what 842 (sw compress) is doing now. If that's > interesting and Kees agrees, and if nobody else plans on doing that, I > could work on it. > > Extra question (maybe silly on my side?): is it possible that > _compressed_ data is bigger than the original one? Isn't there any > "protection" on the compress APIs for that? In that case, it'd purely > waste of time / CPU cycles heheh > No, this is the whole point of those helper routines, as far as I can tell. Basically, if you put data that cannot be compressed losslessly (e.g., a H264 video) through a lossless compression routine, the resulting data will be bigger due to the overhead of the compression metadata. However, we are compressing ASCII text here, so using the uncompressed size as an upper bound for the compressed size is reasonable for any compression algorithm. And if dmesg output is not compressible, there must be something seriously wrong with it. So we could either just drop such input, or simply not bother compressing it if doing so would only take up more space. Given the low likelihood that we will ever hit this case, I'd say we just ignore those. Again, please correct me if I am missing something here (Kees?). Are there cases where we compress data that may be compressed already?
On 08/10/2022 14:52, Ard Biesheuvel wrote: > [...] >> This is exactly what 842 (sw compress) is doing now. If that's >> interesting and Kees agrees, and if nobody else plans on doing that, I >> could work on it. >> >> Extra question (maybe silly on my side?): is it possible that >> _compressed_ data is bigger than the original one? Isn't there any >> "protection" on the compress APIs for that? In that case, it'd purely >> waste of time / CPU cycles heheh >> > > No, this is the whole point of those helper routines, as far as I can > tell. Basically, if you put data that cannot be compressed losslessly > (e.g., a H264 video) through a lossless compression routine, the > resulting data will be bigger due to the overhead of the compression > metadata. > > However, we are compressing ASCII text here, so using the uncompressed > size as an upper bound for the compressed size is reasonable for any > compression algorithm. And if dmesg output is not compressible, there > must be something seriously wrong with it. > > So we could either just drop such input, or simply not bother > compressing it if doing so would only take up more space. Given the > low likelihood that we will ever hit this case, I'd say we just ignore > those. > > Again, please correct me if I am missing something here (Kees?). Are > there cases where we compress data that may be compressed already? This is an interesting point of view, thanks for sharing! And it's possible to kinda test it - I did in the past to test maximum size of ramoops buffers, but I didn't output the values to compare compressed vs. uncompressed size (given I didn't need the info at the time). The trick I used was: suppose I'm using lz4, I polluted dmesg with lz4 already compressed garbage, a huge amount of it, then provoked a crash. I'll try it again to grab the sizes heheh
On Sat, Oct 08, 2022 at 07:52:48PM +0200, Ard Biesheuvel wrote: > Again, please correct me if I am missing something here (Kees?). Are > there cases where we compress data that may be compressed already? Nope, for dmesg, it should all only be text. I'd agree -- the worst case seems a weird thing to need.
On Sat, 8 Oct 2022 at 21:44, Kees Cook <keescook@chromium.org> wrote: > > On Sat, Oct 08, 2022 at 07:52:48PM +0200, Ard Biesheuvel wrote: > > Again, please correct me if I am missing something here (Kees?). Are > > there cases where we compress data that may be compressed already? > > Nope, for dmesg, it should all only be text. I'd agree -- the worst case > seems a weird thing to need. > I've spent a bit more time looking into this, and it seems the pstore code already deals with the compression failure gracefully, so we should be able to just use the uncompressed size as an upper bound for the compressed size without the need for elaborate support in the crypto API. Then I wondered if we still need the big_oops_buf, or whether we could just compress in place. So after a bit more digging, I found out that we can, but only because the scompress backend of the acompress API we intend to use allocates 256 KiB of scratch buffers *per CPU* (which amounts to 32 MB of DRAM permanently tied up in the kernel on my 32x4 ThunderX2 box). So then I wondered why we are using this terrible API in the first place, and what the added value is of having 6 different algorithms available to compress a small amount of ASCII text, which only occurs on an ice cold code path to begin with. So can we rip out this layer and just use GZIP directly (or just use LZMA if we are obsessed with compression ratio)? I am aware that we will need some kind of transition period where we need to support existing records compressed with other compression types, but I don't think that is something we'd need to sustain for more than a year, right?
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index ee50812fdd2e..c10bfb8346fe 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -98,7 +98,7 @@ MODULE_PARM_DESC(kmsg_bytes, "amount of kernel log to snapshot (in bytes)"); static struct crypto_comp *tfm; struct pstore_zbackend { - int (*zbufsize)(size_t size); + unsigned int (*zbufsize)(size_t size); const char *name; }; @@ -169,9 +169,9 @@ static bool pstore_cannot_block_path(enum kmsg_dump_reason reason) } #if IS_ENABLED(CONFIG_PSTORE_DEFLATE_COMPRESS) -static int zbufsize_deflate(size_t size) +static unsigned int zbufsize_deflate(size_t size) { - size_t cmpr; + unsigned int cmpr; switch (size) { /* buffer range for efivars */ @@ -198,28 +198,28 @@ static int zbufsize_deflate(size_t size) #endif #if IS_ENABLED(CONFIG_PSTORE_LZO_COMPRESS) -static int zbufsize_lzo(size_t size) +static unsigned int zbufsize_lzo(size_t size) { return lzo1x_worst_compress(size); } #endif #if IS_ENABLED(CONFIG_PSTORE_LZ4_COMPRESS) || IS_ENABLED(CONFIG_PSTORE_LZ4HC_COMPRESS) -static int zbufsize_lz4(size_t size) +static unsigned int zbufsize_lz4(size_t size) { - return LZ4_compressBound(size); + return (unsigned int)LZ4_compressBound(size); } #endif #if IS_ENABLED(CONFIG_PSTORE_842_COMPRESS) -static int zbufsize_842(size_t size) +static unsigned int zbufsize_842(size_t size) { return size; } #endif #if IS_ENABLED(CONFIG_PSTORE_ZSTD_COMPRESS) -static int zbufsize_zstd(size_t size) +static unsigned int zbufsize_zstd(size_t size) { return zstd_compress_bound(size); } @@ -267,27 +267,27 @@ static const struct pstore_zbackend zbackends[] = { { } }; -static int pstore_compress(const void *in, void *out, - unsigned int inlen, unsigned int outlen) +static bool pstore_compress(const void *in, void *out, + unsigned int inlen, unsigned int *outlen) { int ret; if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS)) return -EINVAL; - ret = crypto_comp_compress(tfm, in, inlen, out, &outlen); + ret = crypto_comp_compress(tfm, in, inlen, out, outlen); if (ret) { pr_err("crypto_comp_compress failed, ret = %d!\n", ret); - return ret; + return false; } - return outlen; + return true; } static void allocate_buf_for_compression(void) { struct crypto_comp *ctx; - int size; + unsigned int size; char *buf; /* Skip if not built-in or compression backend not selected yet. */ @@ -304,15 +304,15 @@ static void allocate_buf_for_compression(void) } size = zbackend->zbufsize(psinfo->bufsize); - if (size <= 0) { - pr_err("Invalid compression size for %s: %d\n", + if (!size) { + pr_err("Invalid compression size for %s: %u\n", zbackend->name, size); return; } buf = kmalloc(size, GFP_KERNEL); if (!buf) { - pr_err("Failed %d byte compression buffer allocation for: %s\n", + pr_err("Failed %u byte compression buffer allocation for: %s\n", size, zbackend->name); return; } @@ -414,7 +414,6 @@ static void pstore_dump(struct kmsg_dumper *dumper, char *dst; size_t dst_size; int header_size; - int zipped_len = -1; size_t dump_size; struct pstore_record record; @@ -444,13 +443,10 @@ static void pstore_dump(struct kmsg_dumper *dumper, break; if (big_oops_buf) { - zipped_len = pstore_compress(dst, psinfo->buf, - header_size + dump_size, - psinfo->bufsize); - - if (zipped_len > 0) { + record.size = psinfo->bufsize; + if (pstore_compress(dst, psinfo->buf, + header_size + dump_size, (unsigned int *)&record.size)) { record.compressed = true; - record.size = zipped_len; } else { record.size = copy_kmsg_to_buffer(header_size, dump_size); @@ -677,7 +673,7 @@ EXPORT_SYMBOL_GPL(pstore_unregister); static void decompress_record(struct pstore_record *record) { int ret; - int unzipped_len; + unsigned int unzipped_len; char *unzipped, *workspace; if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !record->compressed)
The pstore infrastructure is capable of compressing collected logs, relying for that in many compression "libraries" present on kernel. Happens that the (de)compression code in pstore performs many implicit conversions from unsigned int/size_t to int, and vice-versa. Specially in the compress buffer size calculation, we notice that even the libs are not consistent, some of them return int, most of them unsigned int and others rely on preprocessor calculation. Here is an attempt to make it consistent: since we're talking about buffer sizes, let's go with unsigned types, since negative sizes don't make sense. While at it, improve pstore_compress() return semantic too. This function returns either some potential error or the size of the compressed buffer. Such output size is a parameter, so changing it to a pointer makes sense, even follows the compression libraries API (that does modify its parameter with the output size). In order to remove such ambiguity in the return of the function, was required to validate that all compression libraries return either 0 on success or some negative value on error - our analysis showed that all compress libraries potentially used by pstore do respect that [kudos to sw842_compress() as the most convoluted return semantic among them all!]. Cc: Ard Biesheuvel <ardb@kernel.org> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- fs/pstore/platform.c | 46 ++++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 25 deletions(-)