[PATCH 0/2] pstore: Replace crypto API compression with zlib calls

[PATCH 0/2] pstore: Replace crypto API compression with zlib calls

[Ard Biesheuvel]

The pstore layer implements support for compression of kernel log
output, using a variety of compressions algorithms provided by the
[deprecated] crypto API 'comp' interface.

This appears to have been somebody's pet project rather than a solution
to a real problem: the original deflate compression is reasonably fast,
compressed well and is comparatively small in terms of code footprint,
and so the flexibility that the crypto API integration provides does
little more than complicate the code for no reason.

So let's get rid of this complexity, and switch back to zlib deflate
using the library interface.

Cc: Kees Cook <keescook@chromium.org>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>

Ard Biesheuvel (1):
  pstore: Replace crypto API compression with zlib_deflate library calls

Kees Cook (1):
  pstore: Remove worst-case compression size logic

 fs/pstore/Kconfig    | 100 +-------
 fs/pstore/platform.c | 249 +++++---------------
 2 files changed, 72 insertions(+), 277 deletions(-)

-- 
2.39.2

[PATCH 1/2] pstore: Remove worst-case compression size logic

[Ard Biesheuvel]

From: Kees Cook <keescook@chromium.org>

The worst case compression size gives an upper bound for how much the
data might inadvertently *grow* due to encapsulation overhead if the
input is not compressible at all.

The kernel log is ASCII text so it should generally compress rather
well. This means that the probability that the kernel log grows beyond
the uncompressed size after compression is astronomically low, and in
such cases (i.e., dmesg filled with perfect entropy) we won't be able to
make sense of it anyway.

So let's just drop this logic, and use the uncompressed size as the
worst case instead.

Co-developed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 fs/pstore/platform.c | 153 ++------------------
 1 file changed, 9 insertions(+), 144 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index cbc0b468c1ab6ca1..6741ec4347a3eea9 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -16,15 +16,6 @@
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pstore.h>
-#if IS_ENABLED(CONFIG_PSTORE_LZO_COMPRESS)
-#include <linux/lzo.h>
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_LZ4_COMPRESS) || IS_ENABLED(CONFIG_PSTORE_LZ4HC_COMPRESS)
-#include <linux/lz4.h>
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_ZSTD_COMPRESS)
-#include <linux/zstd.h>
-#endif
 #include <linux/crypto.h>
 #include <linux/string.h>
 #include <linux/timer.h>
@@ -97,11 +88,6 @@ MODULE_PARM_DESC(kmsg_bytes, "amount of kernel log to snapshot (in bytes)");
 /* Compression parameters */
 static struct crypto_comp *tfm;
 
-struct pstore_zbackend {
-	int (*zbufsize)(size_t size);
-	const char *name;
-};
-
 static char *big_oops_buf;
 static size_t big_oops_buf_sz;
 
@@ -168,105 +154,6 @@ 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)
-{
-	size_t cmpr;
-
-	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;
-}
-#endif
-
-#if IS_ENABLED(CONFIG_PSTORE_LZO_COMPRESS)
-static 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)
-{
-	return LZ4_compressBound(size);
-}
-#endif
-
-#if IS_ENABLED(CONFIG_PSTORE_842_COMPRESS)
-static int zbufsize_842(size_t size)
-{
-	return size;
-}
-#endif
-
-#if IS_ENABLED(CONFIG_PSTORE_ZSTD_COMPRESS)
-static int zbufsize_zstd(size_t size)
-{
-	return zstd_compress_bound(size);
-}
-#endif
-
-static const struct pstore_zbackend *zbackend __ro_after_init;
-
-static const struct pstore_zbackend zbackends[] = {
-#if IS_ENABLED(CONFIG_PSTORE_DEFLATE_COMPRESS)
-	{
-		.zbufsize	= zbufsize_deflate,
-		.name		= "deflate",
-	},
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_LZO_COMPRESS)
-	{
-		.zbufsize	= zbufsize_lzo,
-		.name		= "lzo",
-	},
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_LZ4_COMPRESS)
-	{
-		.zbufsize	= zbufsize_lz4,
-		.name		= "lz4",
-	},
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_LZ4HC_COMPRESS)
-	{
-		.zbufsize	= zbufsize_lz4,
-		.name		= "lz4hc",
-	},
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_842_COMPRESS)
-	{
-		.zbufsize	= zbufsize_842,
-		.name		= "842",
-	},
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_ZSTD_COMPRESS)
-	{
-		.zbufsize	= zbufsize_zstd,
-		.name		= "zstd",
-	},
-#endif
-	{ }
-};
-
 static int pstore_compress(const void *in, void *out,
 			   unsigned int inlen, unsigned int outlen)
 {
@@ -291,36 +178,31 @@ static void allocate_buf_for_compression(void)
 	char *buf;
 
 	/* Skip if not built-in or compression backend not selected yet. */
-	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend)
+	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !compress)
 		return;
 
 	/* Skip if no pstore backend yet or compression init already done. */
 	if (!psinfo || tfm)
 		return;
 
-	if (!crypto_has_comp(zbackend->name, 0, 0)) {
-		pr_err("Unknown compression: %s\n", zbackend->name);
-		return;
-	}
-
-	size = zbackend->zbufsize(psinfo->bufsize);
-	if (size <= 0) {
-		pr_err("Invalid compression size for %s: %d\n",
-		       zbackend->name, size);
+	if (!crypto_has_comp(compress, 0, 0)) {
+		pr_err("Unknown compression: %s\n", compress);
 		return;
 	}
 
+	/* Worst-case compression should never be more than uncompressed. */
+	size = psinfo->bufsize;
 	buf = kmalloc(size, GFP_KERNEL);
 	if (!buf) {
 		pr_err("Failed %d byte compression buffer allocation for: %s\n",
-		       size, zbackend->name);
+		       size, compress);
 		return;
 	}
 
-	ctx = crypto_alloc_comp(zbackend->name, 0, 0);
+	ctx = crypto_alloc_comp(compress, 0, 0);
 	if (IS_ERR_OR_NULL(ctx)) {
 		kfree(buf);
-		pr_err("crypto_alloc_comp('%s') failed: %ld\n", zbackend->name,
+		pr_err("crypto_alloc_comp('%s') failed: %ld\n", compress,
 		       PTR_ERR(ctx));
 		return;
 	}
@@ -330,7 +212,7 @@ static void allocate_buf_for_compression(void)
 	big_oops_buf_sz = size;
 	big_oops_buf = buf;
 
-	pr_info("Using crash dump compression: %s\n", zbackend->name);
+	pr_info("Using crash dump compression: %s\n", compress);
 }
 
 static void free_buf_for_compression(void)
@@ -818,27 +700,10 @@ static void pstore_timefunc(struct timer_list *unused)
 	pstore_timer_kick();
 }
 
-static void __init pstore_choose_compression(void)
-{
-	const struct pstore_zbackend *step;
-
-	if (!compress)
-		return;
-
-	for (step = zbackends; step->name; step++) {
-		if (!strcmp(compress, step->name)) {
-			zbackend = step;
-			return;
-		}
-	}
-}
-
 static int __init pstore_init(void)
 {
 	int ret;
 
-	pstore_choose_compression();
-
 	/*
 	 * Check if any pstore backends registered earlier but did not
 	 * initialize compression because crypto was not ready. If so,
-- 
2.39.2

[PATCH 2/2] pstore: Replace crypto API compression with zlib_deflate library calls

[Ard Biesheuvel]

Pstore supports compression using a variety of algorithms exposed by the
crypto API. This uses the deprecated comp (as opposed to scomp/acomp)
API, and so we should stop using that, and either move to the new API,
or switch to a different approach entirely.

Given that we only compress ASCII text in pstore, and considering that
this happens when the system is likely to be in a highly fragile state,
the flexibility that the complex crypto API provides does not outweigh
its impact on the risk that we might encounter additional problems when
trying to commit the kernel log contents to the pstore backend.

So let's switch [back] to the zlib deflate library API, and remove all
the complexity that really has no place in a low-level diagnostic
facility. Note that, while more modern compression algorithms have been
added to the kernel in recent years, the code size of zlib deflate is
substantially smaller, while its performance in terms of compression
ratio is perfectly acceptable, and speed is irrelevant in this context
(unless panic() is a performance bottleneck in your workload).

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 fs/pstore/Kconfig    | 100 ++----------------
 fs/pstore/platform.c | 108 +++++++++++---------
 2 files changed, 69 insertions(+), 139 deletions(-)

diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
index c49d554cc9ae9f80..3acc38600cd1a2d0 100644
--- a/fs/pstore/Kconfig
+++ b/fs/pstore/Kconfig
@@ -1,7 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config PSTORE
 	tristate "Persistent store support"
-	select CRYPTO if PSTORE_COMPRESS
 	default n
 	help
 	   This option enables generic access to platform level
@@ -22,99 +21,18 @@ config PSTORE_DEFAULT_KMSG_BYTES
 	  Defines default size of pstore kernel log storage.
 	  Can be enlarged if needed, not recommended to shrink it.
 
-config PSTORE_DEFLATE_COMPRESS
-	tristate "DEFLATE (ZLIB) compression"
-	default y
-	depends on PSTORE
-	select CRYPTO_DEFLATE
-	help
-	  This option enables DEFLATE (also known as ZLIB) compression
-	  algorithm support.
-
-config PSTORE_LZO_COMPRESS
-	tristate "LZO compression"
-	depends on PSTORE
-	select CRYPTO_LZO
-	help
-	  This option enables LZO compression algorithm support.
-
-config PSTORE_LZ4_COMPRESS
-	tristate "LZ4 compression"
-	depends on PSTORE
-	select CRYPTO_LZ4
-	help
-	  This option enables LZ4 compression algorithm support.
-
-config PSTORE_LZ4HC_COMPRESS
-	tristate "LZ4HC compression"
-	depends on PSTORE
-	select CRYPTO_LZ4HC
-	help
-	  This option enables LZ4HC (high compression) mode algorithm.
-
-config PSTORE_842_COMPRESS
-	bool "842 compression"
-	depends on PSTORE
-	select CRYPTO_842
-	help
-	  This option enables 842 compression algorithm support.
-
-config PSTORE_ZSTD_COMPRESS
-	bool "zstd compression"
-	depends on PSTORE
-	select CRYPTO_ZSTD
-	help
-	  This option enables zstd compression algorithm support.
-
 config PSTORE_COMPRESS
-	def_bool y
+	bool "Pstore compression (deflate)"
 	depends on PSTORE
-	depends on PSTORE_DEFLATE_COMPRESS || PSTORE_LZO_COMPRESS ||	\
-		   PSTORE_LZ4_COMPRESS || PSTORE_LZ4HC_COMPRESS ||	\
-		   PSTORE_842_COMPRESS || PSTORE_ZSTD_COMPRESS
-
-choice
-	prompt "Default pstore compression algorithm"
-	depends on PSTORE_COMPRESS
+	select ZLIB_INFLATE
+	select ZLIB_DEFLATE
+	default y
 	help
-	  This option chooses the default active compression algorithm.
-	  This change be changed at boot with "pstore.compress=..." on
-	  the kernel command line.
-
-	  Currently, pstore has support for 6 compression algorithms:
-	  deflate, lzo, lz4, lz4hc, 842 and zstd.
-
-	  The default compression algorithm is deflate.
-
-	config PSTORE_DEFLATE_COMPRESS_DEFAULT
-		bool "deflate" if PSTORE_DEFLATE_COMPRESS
-
-	config PSTORE_LZO_COMPRESS_DEFAULT
-		bool "lzo" if PSTORE_LZO_COMPRESS
-
-	config PSTORE_LZ4_COMPRESS_DEFAULT
-		bool "lz4" if PSTORE_LZ4_COMPRESS
-
-	config PSTORE_LZ4HC_COMPRESS_DEFAULT
-		bool "lz4hc" if PSTORE_LZ4HC_COMPRESS
-
-	config PSTORE_842_COMPRESS_DEFAULT
-		bool "842" if PSTORE_842_COMPRESS
-
-	config PSTORE_ZSTD_COMPRESS_DEFAULT
-		bool "zstd" if PSTORE_ZSTD_COMPRESS
-
-endchoice
-
-config PSTORE_COMPRESS_DEFAULT
-	string
-	depends on PSTORE_COMPRESS
-	default "deflate" if PSTORE_DEFLATE_COMPRESS_DEFAULT
-	default "lzo" if PSTORE_LZO_COMPRESS_DEFAULT
-	default "lz4" if PSTORE_LZ4_COMPRESS_DEFAULT
-	default "lz4hc" if PSTORE_LZ4HC_COMPRESS_DEFAULT
-	default "842" if PSTORE_842_COMPRESS_DEFAULT
-	default "zstd" if PSTORE_ZSTD_COMPRESS_DEFAULT
+	  Whether pstore records should be compressed before being written to
+	  the backing store. This is implemented using the zlib 'deflate'
+	  algorithm, using the library implementation instead of using the full
+	  blown crypto API. This reduces the risk of secondary oopses or other
+	  problems while pstore is recording panic metadata.
 
 config PSTORE_CONSOLE
 	bool "Log kernel console messages"
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 6741ec4347a3eea9..27a045b730eaf1c4 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -16,13 +16,14 @@
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pstore.h>
-#include <linux/crypto.h>
 #include <linux/string.h>
 #include <linux/timer.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/jiffies.h>
+#include <linux/vmalloc.h>
 #include <linux/workqueue.h>
+#include <linux/zlib.h>
 
 #include "internal.h"
 
@@ -71,12 +72,7 @@ static char *backend;
 module_param(backend, charp, 0444);
 MODULE_PARM_DESC(backend, "specific backend to use");
 
-static char *compress =
-#ifdef CONFIG_PSTORE_COMPRESS_DEFAULT
-		CONFIG_PSTORE_COMPRESS_DEFAULT;
-#else
-		NULL;
-#endif
+static char *compress = "deflate";
 module_param(compress, charp, 0444);
 MODULE_PARM_DESC(compress, "compression to use");
 
@@ -85,8 +81,7 @@ unsigned long kmsg_bytes = CONFIG_PSTORE_DEFAULT_KMSG_BYTES;
 module_param(kmsg_bytes, ulong, 0444);
 MODULE_PARM_DESC(kmsg_bytes, "amount of kernel log to snapshot (in bytes)");
 
-/* Compression parameters */
-static struct crypto_comp *tfm;
+static void *compress_workspace;
 
 static char *big_oops_buf;
 static size_t big_oops_buf_sz;
@@ -157,37 +152,46 @@ static bool pstore_cannot_block_path(enum kmsg_dump_reason reason)
 static int pstore_compress(const void *in, void *out,
 			   unsigned int inlen, unsigned int outlen)
 {
+	struct z_stream_s zstream = {
+		.next_in	= in,
+		.avail_in	= inlen,
+		.next_out	= out,
+		.avail_out	= outlen,
+		.workspace	= compress_workspace,
+	};
 	int ret;
 
 	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS))
 		return -EINVAL;
 
-	ret = crypto_comp_compress(tfm, in, inlen, out, &outlen);
-	if (ret) {
-		pr_err("crypto_comp_compress failed, ret = %d!\n", ret);
-		return ret;
-	}
+	ret = zlib_deflateInit2(&zstream, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
+				-MAX_WBITS, DEF_MEM_LEVEL, Z_DEFAULT_STRATEGY);
+	if (ret != Z_OK)
+		return -EINVAL;
 
-	return outlen;
+	ret = zlib_deflate(&zstream, Z_FINISH);
+	if (ret != Z_STREAM_END)
+		return -EINVAL;
+
+	return zstream.total_out;
 }
 
 static void allocate_buf_for_compression(void)
 {
-	struct crypto_comp *ctx;
 	int size;
 	char *buf;
 
-	/* Skip if not built-in or compression backend not selected yet. */
-	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !compress)
+	/* Skip if not built-in or compression disabled. */
+	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !compress ||
+	    !strcmp(compress, "none")) {
+		compress = NULL;
 		return;
+	}
 
-	/* Skip if no pstore backend yet or compression init already done. */
-	if (!psinfo || tfm)
-		return;
-
-	if (!crypto_has_comp(compress, 0, 0)) {
-		pr_err("Unknown compression: %s\n", compress);
-		return;
+	if (strcmp(compress, "deflate")) {
+		pr_err("Unsupported compression '%s', falling back to deflate\n",
+		       compress);
+		compress = "deflate";
 	}
 
 	/* Worst-case compression should never be more than uncompressed. */
@@ -199,16 +203,15 @@ static void allocate_buf_for_compression(void)
 		return;
 	}
 
-	ctx = crypto_alloc_comp(compress, 0, 0);
-	if (IS_ERR_OR_NULL(ctx)) {
+	compress_workspace =
+		vmalloc(zlib_deflate_workspacesize(MAX_WBITS, DEF_MEM_LEVEL));
+	if (!compress_workspace) {
+		pr_err("Failed to allocate zlib deflate workspace\n");
 		kfree(buf);
-		pr_err("crypto_alloc_comp('%s') failed: %ld\n", compress,
-		       PTR_ERR(ctx));
 		return;
 	}
 
 	/* A non-NULL big_oops_buf indicates compression is available. */
-	tfm = ctx;
 	big_oops_buf_sz = size;
 	big_oops_buf = buf;
 
@@ -217,10 +220,6 @@ static void allocate_buf_for_compression(void)
 
 static void free_buf_for_compression(void)
 {
-	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm) {
-		crypto_free_comp(tfm);
-		tfm = NULL;
-	}
 	kfree(big_oops_buf);
 	big_oops_buf = NULL;
 	big_oops_buf_sz = 0;
@@ -563,7 +562,8 @@ void pstore_unregister(struct pstore_info *psi)
 }
 EXPORT_SYMBOL_GPL(pstore_unregister);
 
-static void decompress_record(struct pstore_record *record)
+static void decompress_record(struct pstore_record *record,
+			      struct z_stream_s *zstream)
 {
 	int ret;
 	int unzipped_len;
@@ -584,6 +584,12 @@ static void decompress_record(struct pstore_record *record)
 		return;
 	}
 
+	ret = zlib_inflateReset(zstream);
+	if (ret != Z_OK) {
+		pr_err("zlib_inflateReset() failed, ret = %d!\n", ret);
+		return;
+	}
+
 	/* Allocate enough space to hold max decompression and ECC. */
 	unzipped_len = big_oops_buf_sz;
 	workspace = kmalloc(unzipped_len + record->ecc_notice_size,
@@ -591,15 +597,20 @@ static void decompress_record(struct pstore_record *record)
 	if (!workspace)
 		return;
 
-	/* After decompression "unzipped_len" is almost certainly smaller. */
-	ret = crypto_comp_decompress(tfm, record->buf, record->size,
-					  workspace, &unzipped_len);
-	if (ret) {
-		pr_err("crypto_comp_decompress failed, ret = %d!\n", ret);
+	zstream->next_in	= record->buf;
+	zstream->avail_in	= record->size;
+	zstream->next_out	= workspace;
+	zstream->avail_out	= unzipped_len;
+
+	ret = zlib_inflate(zstream, Z_SYNC_FLUSH);
+	if (ret != Z_STREAM_END) {
+		pr_err("zlib_inflate() failed, ret = %d!\n", ret);
 		kfree(workspace);
 		return;
 	}
 
+	unzipped_len = zstream->total_out;
+
 	/* Append ECC notice to decompressed buffer. */
 	memcpy(workspace + unzipped_len, record->buf + record->size,
 	       record->ecc_notice_size);
@@ -629,10 +640,17 @@ void pstore_get_backend_records(struct pstore_info *psi,
 {
 	int failed = 0;
 	unsigned int stop_loop = 65536;
+	struct z_stream_s zstream;
 
 	if (!psi || !root)
 		return;
 
+	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && compress) {
+		zstream.workspace = kvmalloc(zlib_inflate_workspacesize(),
+					     GFP_KERNEL);
+		zlib_inflateInit2(&zstream, -DEF_WBITS);
+	}
+
 	mutex_lock(&psi->read_mutex);
 	if (psi->open && psi->open(psi))
 		goto out;
@@ -661,7 +679,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
 			break;
 		}
 
-		decompress_record(record);
+		decompress_record(record, &zstream);
 		rc = pstore_mkfile(root, record);
 		if (rc) {
 			/* pstore_mkfile() did not take record, so free it. */
@@ -676,6 +694,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
 		psi->close(psi);
 out:
 	mutex_unlock(&psi->read_mutex);
+	kvfree(zstream.workspace);
 
 	if (failed)
 		pr_warn("failed to create %d record(s) from '%s'\n",
@@ -704,13 +723,6 @@ static int __init pstore_init(void)
 {
 	int ret;
 
-	/*
-	 * Check if any pstore backends registered earlier but did not
-	 * initialize compression because crypto was not ready. If so,
-	 * initialize compression now.
-	 */
-	allocate_buf_for_compression();
-
 	ret = pstore_init_fs();
 	if (ret)
 		free_buf_for_compression();
-- 
2.39.2

Re: [PATCH 0/2] pstore: Replace crypto API compression with zlib calls

[Kees Cook]

On July 4, 2023 6:52:09 AM PDT, Ard Biesheuvel <ardb@kernel.org> wrote:
>The pstore layer implements support for compression of kernel log
>output, using a variety of compressions algorithms provided by the
>[deprecated] crypto API 'comp' interface.
>
>This appears to have been somebody's pet project rather than a solution
>to a real problem: the original deflate compression is reasonably fast,
>compressed well and is comparatively small in terms of code footprint,
>and so the flexibility that the crypto API integration provides does
>little more than complicate the code for no reason.
>
>So let's get rid of this complexity, and switch back to zlib deflate
>using the library interface.
>
>Cc: Kees Cook <keescook@chromium.org>
>Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>

Thanks for picking this back up, it had fallen down on my to-do list. :)

>
>Ard Biesheuvel (1):
>  pstore: Replace crypto API compression with zlib_deflate library calls
>
>Kees Cook (1):
>  pstore: Remove worst-case compression size logic
>
> fs/pstore/Kconfig    | 100 +-------
> fs/pstore/platform.c | 249 +++++---------------
> 2 files changed, 72 insertions(+), 277 deletions(-)

My favorite kind of patch ratio! :)


-- 
Kees Cook

Re: [PATCH 2/2] pstore: Replace crypto API compression with zlib_deflate library calls

[Kees Cook]

On July 4, 2023 6:52:11 AM PDT, Ard Biesheuvel <ardb@kernel.org> wrote:
>@@ -217,10 +220,6 @@ static void allocate_buf_for_compression(void)
> 
> static void free_buf_for_compression(void)
> {
>-	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm) {
>-		crypto_free_comp(tfm);
>-		tfm = NULL;
>-	}
> 	kfree(big_oops_buf);
> 	big_oops_buf = NULL;
> 	big_oops_buf_sz = 0;

I think this is missing a free of compress_workspace? Everything else looks great! What kind of testing did you do for this series?


-- 
Kees Cook

Re: [PATCH 2/2] pstore: Replace crypto API compression with zlib_deflate library calls

[Ard Biesheuvel]

On Tue, 4 Jul 2023 at 18:48, Kees Cook <kees@kernel.org> wrote:
>
> On July 4, 2023 6:52:11 AM PDT, Ard Biesheuvel <ardb@kernel.org> wrote:
> >@@ -217,10 +220,6 @@ static void allocate_buf_for_compression(void)
> >
> > static void free_buf_for_compression(void)
> > {
> >-      if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm) {
> >-              crypto_free_comp(tfm);
> >-              tfm = NULL;
> >-      }
> >       kfree(big_oops_buf);
> >       big_oops_buf = NULL;
> >       big_oops_buf_sz = 0;
>
> I think this is missing a free of compress_workspace?

Indeed.

> Everything else looks great! What kind of testing did you do for this series?

I tested it using lkdtm PANIC in a arm64 vm, and after a reboot,
checking the contents of /sys/fs/pstore, and all looked as expected.

Re: [PATCH 1/2] pstore: Remove worst-case compression size logic

[Eric Biggers]

On Tue, Jul 04, 2023 at 03:52:10PM +0200, Ard Biesheuvel wrote:
> From: Kees Cook <keescook@chromium.org>
> 
> The worst case compression size gives an upper bound for how much the
> data might inadvertently *grow* due to encapsulation overhead if the
> input is not compressible at all.
> 
> The kernel log is ASCII text so it should generally compress rather
> well. This means that the probability that the kernel log grows beyond
> the uncompressed size after compression is astronomically low, and in
> such cases (i.e., dmesg filled with perfect entropy) we won't be able to
> make sense of it anyway.
> 
> So let's just drop this logic, and use the uncompressed size as the
> worst case instead.
> 
> Co-developed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

I think the reasoning about the kernel log being low-entropy, which is a bit
hand-wavy, is entirely unnecessary.  pstore records include a flag that
indicates whether they are compressed or not.  Therefore, any record that would
compress to more than its original size can and should be stored uncompressed.
There's nothing more to it than that.

>  static int pstore_compress(const void *in, void *out,
>  			   unsigned int inlen, unsigned int outlen)
>  {
> @@ -291,36 +178,31 @@ static void allocate_buf_for_compression(void)
>  	char *buf;
>  
>  	/* Skip if not built-in or compression backend not selected yet. */
> -	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend)
> +	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !compress)
>  		return;
>  
>  	/* Skip if no pstore backend yet or compression init already done. */
>  	if (!psinfo || tfm)
>  		return;
>  
> -	if (!crypto_has_comp(zbackend->name, 0, 0)) {
> -		pr_err("Unknown compression: %s\n", zbackend->name);
> -		return;
> -	}
> -
> -	size = zbackend->zbufsize(psinfo->bufsize);
> -	if (size <= 0) {
> -		pr_err("Invalid compression size for %s: %d\n",
> -		       zbackend->name, size);
> +	if (!crypto_has_comp(compress, 0, 0)) {
> +		pr_err("Unknown compression: %s\n", compress);
>  		return;
>  	}
>  
> +	/* Worst-case compression should never be more than uncompressed. */
> +	size = psinfo->bufsize;
>  	buf = kmalloc(size, GFP_KERNEL);
>  	if (!buf) {
>  		pr_err("Failed %d byte compression buffer allocation for: %s\n",
> -		       size, zbackend->name);
> +		       size, compress);
>  		return;
>  	}

The local variable 'size' should be removed, and psinfo->bufsize used directly.

> @@ -330,7 +212,7 @@ static void allocate_buf_for_compression(void)
>  	big_oops_buf_sz = size;

The static variable 'big_oops_buf_sz' should be removed, as it is redundant with
psinfo->bufsize.

>		if (big_oops_buf) {
>			dst = big_oops_buf;
>			dst_size = big_oops_buf_sz;
>		} else {
>			dst = psinfo->buf;
>			dst_size = psinfo->bufsize;
>		}

This can be simplified to:

		if (big_oops_buf)
			dst = big_oops_buf;
		else
			dst = psinfo->buf;
		dst_size = psinfo->bufsize;

>	unzipped_len = big_oops_buf_sz;
>	workspace = kmalloc(unzipped_len + record->ecc_notice_size,
>			    GFP_KERNEL);
>	if (!workspace)
>		return;

This can be simplified to:

	workspace = kmalloc(psinfo->bufsize + record->ecc_notice_size,
			    GFP_KERNEL);
	if (!workspace)
		return;

> /*
>  * Called when compression fails, since the printk buffer
>  * would be fetched for compression calling it again when
>  * compression fails would have moved the iterator of
>  * printk buffer which results in fetching old contents.
>  * Copy the recent messages from big_oops_buf to psinfo->buf
>  */
> static size_t copy_kmsg_to_buffer(int hsize, size_t len)
> {
> 	size_t total_len;
> 	size_t diff;
> 
> 	total_len = hsize + len;
> 
> 	if (total_len > psinfo->bufsize) {
> 		diff = total_len - psinfo->bufsize + hsize;
> 		memcpy(psinfo->buf, big_oops_buf, hsize);
> 		memcpy(psinfo->buf + hsize, big_oops_buf + diff,
> 					psinfo->bufsize - hsize);
> 		total_len = psinfo->bufsize;
> 	} else
> 		memcpy(psinfo->buf, big_oops_buf, total_len);
> 
> 	return total_len;
> }

This patch makes the 'total_len > psinfo->bufsize' case in the above function
unreachable.  That function should be removed, and its caller should just do:

			zipped_len = pstore_compress(dst, psinfo->buf,
						header_size + dump_size,
						psinfo->bufsize);

			if (zipped_len > 0) {
				record.compressed = true;
				record.size = zipped_len;
			} else {
				memcpy(psinfo->buf, dst,
				       header_size + dump_size);
				record.size = header_size + dump_size;
			}


- Eric

Re: [PATCH 0/2] pstore: Replace crypto API compression with zlib calls

[Guilherme G. Piccoli]

On 04/07/2023 10:52, Ard Biesheuvel wrote:
> The pstore layer implements support for compression of kernel log
> output, using a variety of compressions algorithms provided by the
> [deprecated] crypto API 'comp' interface.
> 
> This appears to have been somebody's pet project rather than a solution
> to a real problem: the original deflate compression is reasonably fast,
> compressed well and is comparatively small in terms of code footprint,
> and so the flexibility that the crypto API integration provides does
> little more than complicate the code for no reason.
> 
> So let's get rid of this complexity, and switch back to zlib deflate
> using the library interface.

Thanks Ard, very nice stuff - makes total sense for me!

I'll test it in the Steam Deck - waiting a V2 (since there are some code
comments from Eric and Kees).

Cheers,


Guilherme

Re: [PATCH 2/2] pstore: Replace crypto API compression with zlib_deflate library calls

[Eric Biggers]

On Tue, Jul 04, 2023 at 03:52:11PM +0200, Ard Biesheuvel wrote:
> Pstore supports compression using a variety of algorithms exposed by the
> crypto API. This uses the deprecated comp (as opposed to scomp/acomp)
> API, and so we should stop using that, and either move to the new API,
> or switch to a different approach entirely.
> 
> Given that we only compress ASCII text in pstore, and considering that
> this happens when the system is likely to be in a highly fragile state,
> the flexibility that the complex crypto API provides does not outweigh
> its impact on the risk that we might encounter additional problems when
> trying to commit the kernel log contents to the pstore backend.
> 
> So let's switch [back] to the zlib deflate library API, and remove all
> the complexity that really has no place in a low-level diagnostic
> facility. Note that, while more modern compression algorithms have been
> added to the kernel in recent years, the code size of zlib deflate is
> substantially smaller, while its performance in terms of compression
> ratio is perfectly acceptable, and speed is irrelevant in this context
> (unless panic() is a performance bottleneck in your workload).
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Actually, LZ4 and LZO both have slightly smaller code size than zlib.

Though, they are really intended for use cases that need high performance, which
as you mention is not important for the pstore use case.

So I think zlib is still fine here.  Just the above argument is a bit
misleading.

In any case, can the rationale for the choice of compression algorithm and API
be documented in the source code itself?  Otherwise I worry that someone will
want to "improve" this code again.

> @@ -157,37 +152,46 @@ static bool pstore_cannot_block_path(enum kmsg_dump_reason reason)
>  static int pstore_compress(const void *in, void *out,
>  			   unsigned int inlen, unsigned int outlen)
>  {
> +	struct z_stream_s zstream = {
> +		.next_in	= in,
> +		.avail_in	= inlen,
> +		.next_out	= out,
> +		.avail_out	= outlen,
> +		.workspace	= compress_workspace,
> +	};
>  	int ret;
>  
>  	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS))
>  		return -EINVAL;
>  
> -	ret = crypto_comp_compress(tfm, in, inlen, out, &outlen);
> -	if (ret) {
> -		pr_err("crypto_comp_compress failed, ret = %d!\n", ret);
> -		return ret;
> -	}
> +	ret = zlib_deflateInit2(&zstream, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
> +				-MAX_WBITS, DEF_MEM_LEVEL, Z_DEFAULT_STRATEGY);
> +	if (ret != Z_OK)
> +		return -EINVAL;
>  
> -	return outlen;
> +	ret = zlib_deflate(&zstream, Z_FINISH);
> +	if (ret != Z_STREAM_END)
> +		return -EINVAL;
> +
> +	return zstream.total_out;
>  }

The above code looks weird to anyone familiar with the zlib API, since it is
missing the call to zlib_deflateEnd().  It looks like the in-kernel zlib doesn't
really need it, since the in-kernel zlib has been customized to manage memory
differently from the real zlib.  But I recommend including it.

> @@ -629,10 +640,17 @@ void pstore_get_backend_records(struct pstore_info *psi,
>  {
>  	int failed = 0;
>  	unsigned int stop_loop = 65536;
> +	struct z_stream_s zstream;
>  
>  	if (!psi || !root)
>  		return;
>  
> +	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && compress) {
> +		zstream.workspace = kvmalloc(zlib_inflate_workspacesize(),
> +					     GFP_KERNEL);
> +		zlib_inflateInit2(&zstream, -DEF_WBITS);
> +	}
> +
>  	mutex_lock(&psi->read_mutex);
>  	if (psi->open && psi->open(psi))
>  		goto out;
> @@ -661,7 +679,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
>  			break;
>  		}
>  
> -		decompress_record(record);
> +		decompress_record(record, &zstream);
>  		rc = pstore_mkfile(root, record);
>  		if (rc) {
>  			/* pstore_mkfile() did not take record, so free it. */
> @@ -676,6 +694,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
>  		psi->close(psi);
>  out:
>  	mutex_unlock(&psi->read_mutex);
> +	kvfree(zstream.workspace);

Similarly above: zlib_inflateEnd() isn't being called.  It should happen
alongside freeing 'zstream.workspace'.

- Eric