On Thu, 31 Aug 2023 at 23:01, Kees Cook <[email protected]> wrote:
>
> From: Ard Biesheuvel <[email protected]>
>
> Commit 1756ddea6916 ("pstore: Remove worst-case compression size logic")
> removed some clunky per-algorithm worst case size estimation routines on
> the basis that we can always store pstore records uncompressed, and
> these worst case estimations are about how much the size might
> inadvertently *increase* due to encapsulation overhead when the input
> cannot be compressed at all. So if compression results in a size
> increase, we just store the original data instead.
>
> However, it seems that the original code was misinterpreting these
> calculations as an estimation of how much uncompressed data might fit
> into a compressed buffer of a given size, and it was using the results
> to consume the input data in larger chunks than the pstore record size,
> relying on the compression to ensure that what ultimately gets stored
> fits into the available space.
>
> One result of this, as observed and reported by Linus, is that upgrading
> to a newer kernel that includes the given commit may result in pstore
> decompression errors reported in the kernel log. This is due to the fact
> that the existing records may unexpectedly decompress to a size that is
> larger than the pstore record size.
>
> Another potential problem caused by this change is that we may
> underutilize the fixed sized records on pstore backends such as ramoops.
> And on pstore backends with variable sized records such as EFI, we will
> end up creating many more entries than before to store the same amount
> of compressed data.
>
> So let's fix both issues, by bringing back the typical case estimation of
> how much ASCII text captured from the dmesg log might fit into a pstore
> record of a given size after compression. The original implementation
> used the computation given below for zlib:
>
> switch (size) {
> /* buffer range for efivars */
> case 1000 ... 2000:
> cmpr = 56;
> break;
> case 2001 ... 3000:
> cmpr = 54;
> break;
> case 3001 ... 3999:
> cmpr = 52;
> break;
> /* buffer range for nvram, erst */
> case 4000 ... 10000:
> cmpr = 45;
> break;
> default:
> cmpr = 60;
> break;
> }
>
> return (size * 100) / cmpr;
>
> We will use the previous worst-case of 60% for compression. For
> decompression go extra large (3x) so we make sure there's enough space
> for anything.
>
> While at it, rate limit the error message so we don't flood the log
> unnecessarily on systems that have accumulated a lot of pstore history.
>
> Cc: Linus Torvalds <[email protected]>
> Cc: Eric Biggers <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Co-developed-by: Kees Cook <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> v2:
> - reduce compression buffer size to 1.67x from 2x
> - raise decompression buffer size to 3x
LGTM
Thanks for picking this up.
> v1: https://lore.kernel.org/all/[email protected]
> ---
> fs/pstore/platform.c | 34 +++++++++++++++++++++++++++-------
> 1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 62356d542ef6..e5bca9a004cc 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -98,7 +98,14 @@ MODULE_PARM_DESC(kmsg_bytes, "amount of kernel log to snapshot (in bytes)");
>
> static void *compress_workspace;
>
> +/*
> + * Compression is only used for dmesg output, which consists of low-entropy
> + * ASCII text, and so we can assume worst-case 60%.
> + */
> +#define DMESG_COMP_PERCENT 60
> +
> static char *big_oops_buf;
> +static size_t max_compressed_size;
>
> void pstore_set_kmsg_bytes(int bytes)
> {
> @@ -196,6 +203,7 @@ static int pstore_compress(const void *in, void *out,
>
> static void allocate_buf_for_compression(void)
> {
> + size_t compressed_size;
> char *buf;
>
> /* Skip if not built-in or compression disabled. */
> @@ -216,7 +224,8 @@ static void allocate_buf_for_compression(void)
> * uncompressed record size, since any record that would be expanded by
> * compression is just stored uncompressed.
> */
> - buf = kvzalloc(psinfo->bufsize, GFP_KERNEL);
> + compressed_size = (psinfo->bufsize * 100) / DMESG_COMP_PERCENT;
> + buf = kvzalloc(compressed_size, GFP_KERNEL);
> if (!buf) {
> pr_err("Failed %zu byte compression buffer allocation for: %s\n",
> psinfo->bufsize, compress);
> @@ -233,6 +242,7 @@ static void allocate_buf_for_compression(void)
>
> /* A non-NULL big_oops_buf indicates compression is available. */
> big_oops_buf = buf;
> + max_compressed_size = compressed_size;
>
> pr_info("Using crash dump compression: %s\n", compress);
> }
> @@ -246,6 +256,7 @@ static void free_buf_for_compression(void)
>
> kvfree(big_oops_buf);
> big_oops_buf = NULL;
> + max_compressed_size = 0;
> }
>
> void pstore_record_init(struct pstore_record *record,
> @@ -305,7 +316,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> record.buf = psinfo->buf;
>
> dst = big_oops_buf ?: psinfo->buf;
> - dst_size = psinfo->bufsize;
> + dst_size = max_compressed_size ?: psinfo->bufsize;
>
> /* Write dump header. */
> header_size = snprintf(dst, dst_size, "%s#%d Part%u\n", why,
> @@ -326,8 +337,15 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> record.compressed = true;
> record.size = zipped_len;
> } else {
> - record.size = header_size + dump_size;
> - memcpy(psinfo->buf, dst, record.size);
> + /*
> + * Compression failed, so the buffer is most
> + * likely filled with binary data that does not
> + * compress as well as ASCII text. Copy as much
> + * of the uncompressed data as possible into
> + * the pstore record, and discard the rest.
> + */
> + record.size = psinfo->bufsize;
> + memcpy(psinfo->buf, dst, psinfo->bufsize);
> }
> } else {
> record.size = header_size + dump_size;
> @@ -560,6 +578,7 @@ static void decompress_record(struct pstore_record *record,
> int ret;
> int unzipped_len;
> char *unzipped, *workspace;
> + size_t max_uncompressed_size;
>
> if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !record->compressed)
> return;
> @@ -583,7 +602,8 @@ static void decompress_record(struct pstore_record *record,
> }
>
> /* Allocate enough space to hold max decompression and ECC. */
> - workspace = kvzalloc(psinfo->bufsize + record->ecc_notice_size,
> + max_uncompressed_size = 3 * psinfo->bufsize;
> + workspace = kvzalloc(max_uncompressed_size + record->ecc_notice_size,
> GFP_KERNEL);
> if (!workspace)
> return;
> @@ -591,11 +611,11 @@ static void decompress_record(struct pstore_record *record,
> zstream->next_in = record->buf;
> zstream->avail_in = record->size;
> zstream->next_out = workspace;
> - zstream->avail_out = psinfo->bufsize;
> + zstream->avail_out = max_uncompressed_size;
>
> ret = zlib_inflate(zstream, Z_FINISH);
> if (ret != Z_STREAM_END) {
> - pr_err("zlib_inflate() failed, ret = %d!\n", ret);
> + pr_err_ratelimited("zlib_inflate() failed, ret = %d!\n", ret);
> kvfree(workspace);
> return;
> }
> --
> 2.34.1
>