[PATCH 0/5] pstore: Use zstd directly by default for compression

[PATCH 0/5] pstore: Use zstd directly by default for compression

[Kees Cook]

Hi,

Okay, here is a very lightly tested version of using zstd directly, which
should save 128KB per CPU compare to using crypto API. This leaves the
crypto API available, though, if someone wants to use it instead. Even
supporting both, this is a net reduction in code, due to dropping all
the zbufsize logic.

How does this look?

-Kees

Kees Cook (5):
  pstore: Remove worse-case compression size logic
  pstore: Allow for arbitrary compression algorithm
  pstore: Use size_t for compress/decompression type widths
  pstore: Refactor compression initialization
  pstore: Use zstd directly by default for compression

 fs/pstore/Kconfig    | 131 +++++-----------
 fs/pstore/platform.c | 358 ++++++++++++++++++++-----------------------
 2 files changed, 209 insertions(+), 280 deletions(-)

-- 
2.34.1

[PATCH 1/5] pstore: Remove worse-case compression size logic

[Kees Cook]

The worst case compression size is always the size of the uncompressed
data itself so avoid perfectly optimizing the oops buffer size. Hugely
simplifies the code.

Cc: Tony Luck <tony.luck@intel.com>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Cc: Nick Terrell <terrelln@fb.com>
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c | 144 +++----------------------------------------
 1 file changed, 9 insertions(+), 135 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index cbc0b468c1ab..ef0bc3ae161b 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -97,11 +97,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 +163,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 +187,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 +221,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 +709,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.34.1

[PATCH 2/5] pstore: Allow for arbitrary compression algorithm

[Kees Cook]

Since there is no longer a need for figuring out worst-case compression
size, there is no reason to limit the compression algorithm choice.
Greatly simplifies Kconfig.

Cc: Tony Luck <tony.luck@intel.com>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Cc: Nick Terrell <terrelln@fb.com>
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/Kconfig    | 122 ++++++++++++-------------------------------
 fs/pstore/platform.c |  39 ++++++--------
 2 files changed, 47 insertions(+), 114 deletions(-)

diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
index 8adabde685f1..a95b3981cb0e 100644
--- a/fs/pstore/Kconfig
+++ b/fs/pstore/Kconfig
@@ -22,99 +22,41 @@ 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
-	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
-	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
-
+	prompt "Panic dump compression"
+	depends on PSTORE
+	default PSTORE_COMPRESS_CRYPTO
+	help
+	  Choose whether and how to compress the panic dump output. This
+	  is usually only needed for very storage-constrained backends.
+
+	config PSTORE_COMPRESS_CRYPTO
+		bool "Use an arbitrary compression algorithm via the Crypto API"
+		help
+		  If the default compression algorithm from PSTORE_COMPRESS
+		  is not desired, an arbitrary one can be chosen if it is
+		  available to from the Crypto API. Note that this may reserve
+		  non-trivial amounts of per-CPU memory.
+
+	config PSTORE_COMPRESS_NONE
+		bool "Do not compress panic dumps"
+		help
+		  Do not compress the panic dump output. This leave the
+		  output easily readable in memory, if non-pstore forensics
+		  tools want to examine the contents easily.
 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
+config PSTORE_COMPRESS_CRYPTO_DEFAULT
+	string "Crypto API compression algorithm"
+	depends on PSTORE_COMPRESS_CRYPTO
+	default "zstd"
+	help
+	  This option chooses the default active compression algorithm,
+	  and can be changed at boot with "pstore.compress=..." on the
+	  kernel command line. The chosen compression algorithm needs to
+	  be available to the crypto subsystem for it to be usable by
+	  pstore. For example, "zstd" needs CONFIG_CRYPTO_ZSTD, "deflate"
+	  needs CONFIG_CRYPTO_DEFLATE, etc.
 
 config PSTORE_CONSOLE
 	bool "Log kernel console messages"
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index ef0bc3ae161b..1f01c4b904fc 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>
@@ -80,9 +71,9 @@ 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;
+static char *compress __ro_after_init =
+#ifdef CONFIG_PSTORE_COMPRESS_CRYPTO_DEFAULT
+		CONFIG_PSTORE_COMPRESS_CRYPTO_DEFAULT;
 #else
 		NULL;
 #endif
@@ -166,18 +157,18 @@ 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)
 {
-	int ret;
+	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS_CRYPTO)) {
+		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 = crypto_comp_compress(tfm, in, inlen, out, &outlen);
+		if (ret) {
+			pr_err("crypto_comp_compress failed, ret = %d!\n", ret);
+			return ret;
+		}
+		return outlen;
 	}
 
-	return outlen;
+	return -EINVAL;
 }
 
 static void allocate_buf_for_compression(void)
@@ -187,7 +178,7 @@ 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) || !compress)
+	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS_NONE) || !compress)
 		return;
 
 	/* Skip if no pstore backend yet or compression init already done. */
@@ -226,7 +217,7 @@ static void allocate_buf_for_compression(void)
 
 static void free_buf_for_compression(void)
 {
-	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm) {
+	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS_CRYPTO) && tfm) {
 		crypto_free_comp(tfm);
 		tfm = NULL;
 	}
@@ -578,7 +569,7 @@ static void decompress_record(struct pstore_record *record)
 	int unzipped_len;
 	char *unzipped, *workspace;
 
-	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !record->compressed)
+	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS_NONE) || !record->compressed)
 		return;
 
 	/* Only PSTORE_TYPE_DMESG support compression. */
-- 
2.34.1

[PATCH 3/5] pstore: Use size_t for compress/decompression type widths

[Kees Cook]

In preparation for adding direct library calls for zstd, refactor
routines to use normalized compress/decompression type widths of size_t.

Cc: Tony Luck <tony.luck@intel.com>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c | 47 ++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 1f01c4b904fc..210a4224edb4 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -155,17 +155,24 @@ 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)
+			   size_t inlen, size_t *outlen)
 {
 	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS_CRYPTO)) {
+		unsigned int crypto_outlen;
 		int ret;
 
-		ret = crypto_comp_compress(tfm, in, inlen, out, &outlen);
+		if (inlen > UINT_MAX || *outlen > UINT_MAX)
+			return -EINVAL;
+
+		crypto_outlen = *outlen;
+		ret = crypto_comp_compress(tfm, in, inlen, out, &crypto_outlen);
 		if (ret) {
 			pr_err("crypto_comp_compress failed, ret = %d!\n", ret);
 			return ret;
 		}
-		return outlen;
+
+		*outlen = crypto_outlen;
+		return 0;
 	}
 
 	return -EINVAL;
@@ -297,7 +304,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;
 
@@ -327,11 +333,11 @@ 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);
+			size_t zipped_len = psinfo->bufsize;
 
-			if (zipped_len > 0) {
+			if (pstore_compress(dst, psinfo->buf,
+					    header_size + dump_size,
+					    &zipped_len) == 0) {
 				record.compressed = true;
 				record.size = zipped_len;
 			} else {
@@ -563,10 +569,25 @@ void pstore_unregister(struct pstore_info *psi)
 }
 EXPORT_SYMBOL_GPL(pstore_unregister);
 
-static void decompress_record(struct pstore_record *record)
+static int pstore_decompress_crypto(struct pstore_record *record, char *workspace, size_t *outlen)
 {
+	unsigned int crypto_outlen = *outlen;
 	int ret;
-	int unzipped_len;
+
+	ret = crypto_comp_decompress(tfm, record->buf, record->size,
+				     workspace, &crypto_outlen);
+	if (ret) {
+		pr_err("crypto_comp_decompress failed, ret = %d!\n", ret);
+		return 0;
+	}
+
+	*outlen = crypto_outlen;
+	return 0;
+}
+
+static void decompress_record(struct pstore_record *record)
+{
+	size_t unzipped_len;
 	char *unzipped, *workspace;
 
 	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS_NONE) || !record->compressed)
@@ -591,11 +612,7 @@ 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);
+	if (pstore_decompress_crypto(record, workspace, &unzipped_len) != 0) {
 		kfree(workspace);
 		return;
 	}
-- 
2.34.1

[PATCH 4/5] pstore: Refactor compression initialization

[Kees Cook]

In preparation for calling zstd library compression routines, split the
crypto-specific initialization into a separate init routine.

Cc: Tony Luck <tony.luck@intel.com>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c | 71 +++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 28 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 210a4224edb4..4d883dc2e8a7 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -178,48 +178,29 @@ static int pstore_compress(const void *in, void *out,
 	return -EINVAL;
 }
 
-static void allocate_buf_for_compression(void)
+static int allocate_crypto_buf(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_NONE) || !compress)
-		return;
-
-	/* Skip if no pstore backend yet or compression init already done. */
-	if (!psinfo || tfm)
-		return;
+	/* Skip if compression init already done. */
+	if (tfm)
+		return 0;
 
 	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, compress);
-		return;
+		return -EINVAL;
 	}
 
 	ctx = crypto_alloc_comp(compress, 0, 0);
 	if (IS_ERR_OR_NULL(ctx)) {
-		kfree(buf);
 		pr_err("crypto_alloc_comp('%s') failed: %ld\n", compress,
 		       PTR_ERR(ctx));
-		return;
+		return -ENOMEM;
 	}
-
-	/* A non-NULL big_oops_buf indicates compression is available. */
 	tfm = ctx;
-	big_oops_buf_sz = size;
-	big_oops_buf = buf;
 
-	pr_info("Using crash dump compression: %s\n", compress);
+	pr_info("Using crash dump compression: crypto API %s\n", compress);
+	return 0;
 }
 
 static void free_buf_for_compression(void)
@@ -233,6 +214,38 @@ static void free_buf_for_compression(void)
 	big_oops_buf_sz = 0;
 }
 
+static void allocate_buf_for_compression(void)
+{
+	char *buf;
+	int rc;
+
+	/* Skip if not built-in or compression backend not selected yet. */
+	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS_NONE) || !compress)
+		return;
+
+	/* Skip if no pstore backend yet. */
+	if (!psinfo)
+		return;
+
+	/* Initialize compression routines. */
+	rc = allocate_crypto_buf();
+	if (rc)
+		goto fail;
+
+	/* Create common buffer for compression work. */
+	buf = kmalloc(psinfo->bufsize, GFP_KERNEL);
+	if (!buf)
+		goto fail;
+
+	/* A non-NULL big_oops_buf indicates compression is available. */
+	big_oops_buf_sz = psinfo->bufsize;
+	big_oops_buf = buf;
+	return;
+
+fail:
+	free_buf_for_compression();
+}
+
 /*
  * Called when compression fails, since the printk buffer
  * would be fetched for compression calling it again when
@@ -589,6 +602,7 @@ static void decompress_record(struct pstore_record *record)
 {
 	size_t unzipped_len;
 	char *unzipped, *workspace;
+	int rc;
 
 	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS_NONE) || !record->compressed)
 		return;
@@ -612,7 +626,8 @@ static void decompress_record(struct pstore_record *record)
 	if (!workspace)
 		return;
 
-	if (pstore_decompress_crypto(record, workspace, &unzipped_len) != 0) {
+	rc = pstore_decompress_crypto(record, workspace, &unzipped_len);
+	if (rc) {
 		kfree(workspace);
 		return;
 	}
-- 
2.34.1

[PATCH 5/5] pstore: Use zstd directly by default for compression

[Kees Cook]

If compression is desired, use zstd directly to avoid Crypto API
overhead.

Cc: Tony Luck <tony.luck@intel.com>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Cc: Nick Terrell <terrelln@fb.com>
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/Kconfig    | 11 +++++-
 fs/pstore/platform.c | 93 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 97 insertions(+), 7 deletions(-)

diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
index a95b3981cb0e..1f05312c7479 100644
--- a/fs/pstore/Kconfig
+++ b/fs/pstore/Kconfig
@@ -25,7 +25,7 @@ config PSTORE_DEFAULT_KMSG_BYTES
 choice
 	prompt "Panic dump compression"
 	depends on PSTORE
-	default PSTORE_COMPRESS_CRYPTO
+	default PSTORE_COMPRESS
 	help
 	  Choose whether and how to compress the panic dump output. This
 	  is usually only needed for very storage-constrained backends.
@@ -38,6 +38,15 @@ choice
 		  available to from the Crypto API. Note that this may reserve
 		  non-trivial amounts of per-CPU memory.
 
+	config PSTORE_COMPRESS
+		bool "Use recommended best compression algorithm"
+		select CRYPTO_ZSTD
+		help
+		  Use the compression routines currently deemed best suited
+		  for panic dump compression. Currently, this is "zstd".
+		  As this compression is used directly through its library
+		  interface, no per-CPU memory is allocated by the Crypto API.
+
 	config PSTORE_COMPRESS_NONE
 		bool "Do not compress panic dumps"
 		help
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 4d883dc2e8a7..51d2801fc880 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -23,6 +23,7 @@
 #include <linux/uaccess.h>
 #include <linux/jiffies.h>
 #include <linux/workqueue.h>
+#include <linux/zstd.h>
 
 #include "internal.h"
 
@@ -72,13 +73,17 @@ module_param(backend, charp, 0444);
 MODULE_PARM_DESC(backend, "specific backend to use");
 
 static char *compress __ro_after_init =
-#ifdef CONFIG_PSTORE_COMPRESS_CRYPTO_DEFAULT
-		CONFIG_PSTORE_COMPRESS_CRYPTO_DEFAULT;
+#ifdef CONFIG_PSTORE_COMPRESS
+		"zstd";
 #else
-		NULL;
-#endif
+# ifdef CONFIG_PSTORE_COMPRESS_CRYPTO_DEFAULT
+		CONFIG_PSTORE_COMPRESS_CRYPTO_DEFAULT;
 module_param(compress, charp, 0444);
 MODULE_PARM_DESC(compress, "compression to use");
+# else
+		NULL;
+# endif
+#endif
 
 /* How much of the kernel log to snapshot */
 unsigned long kmsg_bytes = CONFIG_PSTORE_DEFAULT_KMSG_BYTES;
@@ -88,6 +93,12 @@ MODULE_PARM_DESC(kmsg_bytes, "amount of kernel log to snapshot (in bytes)");
 /* Compression parameters */
 static struct crypto_comp *tfm;
 
+static zstd_cctx *cctx;
+static zstd_dctx *dctx;
+static void *cwksp;
+static void *dwksp;
+zstd_parameters zparams;
+
 static char *big_oops_buf;
 static size_t big_oops_buf_sz;
 
@@ -175,6 +186,14 @@ static int pstore_compress(const void *in, void *out,
 		return 0;
 	}
 
+	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS)) {
+		*outlen = zstd_compress_cctx(cctx, out, *outlen, in, inlen,
+					    &zparams);
+		if (zstd_is_error(*outlen))
+			return -EINVAL;
+		return 0;
+	}
+
 	return -EINVAL;
 }
 
@@ -203,12 +222,56 @@ static int allocate_crypto_buf(void)
 	return 0;
 }
 
+static int allocate_zstd_buf(void)
+{
+	size_t csize, dsize;
+
+	/* Skip if compression init already done. */
+	if (cctx)
+		return 0;
+
+	zparams = zstd_get_params(3, 0);
+	csize = zstd_cctx_workspace_bound(&zparams.cParams);
+	dsize = zstd_dctx_workspace_bound();
+
+#define init_ctx(dir, name)	do {					\
+	dir##wksp = kzalloc(dir##size, GFP_KERNEL);			\
+	if (!dir##wksp) {						\
+		pr_err("Failed %zu byte %s " #name " allocation\n",	\
+		       dir##size, compress);				\
+		return -ENOMEM;						\
+	}								\
+	dir##ctx = zstd_init_##dir##ctx(dir##wksp, dir##size);		\
+	if (!dir##ctx) {						\
+		pr_err("Failed %s " #name " context init\n", compress);	\
+		return -EINVAL;						\
+	}								\
+} while (0)
+
+	init_ctx(c, compress);
+	init_ctx(d, decompress);
+
+#undef init_wksp
+
+	pr_info("Using crash dump compression: built-in %s\n", compress);
+	return 0;
+}
+
 static void free_buf_for_compression(void)
 {
 	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS_CRYPTO) && tfm) {
 		crypto_free_comp(tfm);
 		tfm = NULL;
 	}
+	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && cctx) {
+		cctx = NULL;
+		dctx = NULL;
+		kfree(cwksp);
+		cwksp = NULL;
+		kfree(dwksp);
+		dwksp = NULL;
+
+	}
 	kfree(big_oops_buf);
 	big_oops_buf = NULL;
 	big_oops_buf_sz = 0;
@@ -228,7 +291,10 @@ static void allocate_buf_for_compression(void)
 		return;
 
 	/* Initialize compression routines. */
-	rc = allocate_crypto_buf();
+	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS_CRYPTO))
+		rc = allocate_crypto_buf();
+	else
+		rc = allocate_zstd_buf();
 	if (rc)
 		goto fail;
 
@@ -598,6 +664,16 @@ static int pstore_decompress_crypto(struct pstore_record *record, char *workspac
 	return 0;
 }
 
+static int pstore_decompress_zstd(struct pstore_record *record,
+				  char *workspace, size_t *outlen)
+{
+	*outlen = zstd_decompress_dctx(dctx, workspace, *outlen,
+				       record->buf, record->size);
+	if (zstd_is_error(*outlen))
+		return -EINVAL;
+	return 0;
+}
+
 static void decompress_record(struct pstore_record *record)
 {
 	size_t unzipped_len;
@@ -626,7 +702,12 @@ static void decompress_record(struct pstore_record *record)
 	if (!workspace)
 		return;
 
-	rc = pstore_decompress_crypto(record, workspace, &unzipped_len);
+	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS_CRYPTO))
+		rc = pstore_decompress_crypto(record, workspace,
+					      &unzipped_len);
+	else
+		rc = pstore_decompress_zstd(record, workspace,
+					    &unzipped_len);
 	if (rc) {
 		kfree(workspace);
 		return;
-- 
2.34.1

Re: [PATCH 0/5] pstore: Use zstd directly by default for compression

[Ard Biesheuvel]

On Tue, 18 Oct 2022 at 04:08, Kees Cook <keescook@chromium.org> wrote:
>
> Hi,
>
> Okay, here is a very lightly tested version of using zstd directly, which
> should save 128KB per CPU compare to using crypto API. This leaves the
> crypto API available, though, if someone wants to use it instead. Even
> supporting both, this is a net reduction in code, due to dropping all
> the zbufsize logic.
>
> How does this look?
>

The code changes all look correct and reasonable to me.

As for supporting both the library and the crypto API interface: I did
a little digging, and it turns out all additional compression modes
were added by the same contributor, with no justification other than
'this might be useful to some people' (see below)

However, I did a quick experiment with the output of dmesg on my
workstation, and these are the results I am getting

Input file:
-rw-r--r-- 1 ard ard 111792 Oct 18 09:23 uncompressed

Default compression
-rw-r--r-- 1 ard ard  21810 Oct 18 09:23 uncompressed.gz
-rw-r--r-- 1 ard ard  33923 Oct 18 09:23 uncompressed.lz4
-rw-r--r-- 1 ard ard  34238 Oct 18 09:23 uncompressed.lzo
-rw-r--r-- 1 ard ard  21599 Oct 18 09:23 uncompressed.zst

Max compression (-9)
-rw-r--r-- 1 ard ard  21589 Oct 18 09:23 uncompressed.gz
-rw-r--r-- 1 ard ard  28848 Oct 18 09:25 uncompressed.lz4 (== lz4hc?)
-rw-r--r-- 1 ard ard  26917 Oct 18 09:23 uncompressed.lzo
-rw-r--r-- 1 ard ard  19883 Oct 18 09:23 uncompressed.zst

So the patches in question don't actually fulfill their claim of
improving the compression ratio. Only zstd, which was added later,
improves upon zlib, but not significantly unless you override the
compression level (which we don't).

So I seriously doubt that those patches were inspired by the need to
solve an actual problem anyone was experiencing at the time, given
that they don't. It just seems that nobody bothered to ask the 'why?'
question.

So again, I suggest to simply drop this non-feature, and standardize
on either zlib or zstd using the library interface exclusively. If
someone present a compelling use case, we can always consider adding
it back in some form.

As for the choice of algorithm, given the equal performance using the
default compression level, and the difference in code size, I don't
see why zstd should be preferred here. If anything, it only increases
the likelihood of hitting another error if we are panicking due to
some memory corruption issue.

$ size  lib/zstd/zstd_compress.ko lib/zlib_deflate/zlib_deflate.ko
   text    data     bss     dec     hex filename
 218411     768       0 219179   3582b lib/zstd/zstd_compress.ko
  16231     876    2288   19395    4bc3 lib/zlib_deflate/zlib_deflate.ko






commit 8cfc8ddc99df9509a46043b14af81f5c6a223eab
Author:     Geliang Tang <geliangtang@163.com>
AuthorDate: Thu Feb 18 22:04:22 2016 +0800
Commit:     Kees Cook <keescook@chromium.org>
CommitDate: Thu Jun 2 10:59:31 2016 -0700

    pstore: add lzo/lz4 compression support

    Like zlib compression in pstore, this patch added lzo and lz4
    compression support so that users can have more options and better
    compression ratio.

commit 239b716199d9aff0d09444b0086e23aacd6bd445
Author:     Geliang Tang <geliangtang@gmail.com>
AuthorDate: Tue Feb 13 14:40:39 2018 +0800
Commit:     Kees Cook <keescook@chromium.org>
CommitDate: Tue Mar 6 15:06:11 2018 -0800

    pstore: Add lz4hc and 842 compression support


>
> Kees Cook (5):
>   pstore: Remove worse-case compression size logic
>   pstore: Allow for arbitrary compression algorithm
>   pstore: Use size_t for compress/decompression type widths
>   pstore: Refactor compression initialization
>   pstore: Use zstd directly by default for compression
>
>  fs/pstore/Kconfig    | 131 +++++-----------
>  fs/pstore/platform.c | 358 ++++++++++++++++++++-----------------------
>  2 files changed, 209 insertions(+), 280 deletions(-)
>
> --
> 2.34.1
>

Re: [PATCH 0/5] pstore: Use zstd directly by default for compression

[Guilherme G. Piccoli]

On 18/10/2022 05:20, Ard Biesheuvel wrote:
> [...]
> So again, I suggest to simply drop this non-feature, and standardize
> on either zlib or zstd using the library interface exclusively. If
> someone present a compelling use case, we can always consider adding
> it back in some form.
> 
> As for the choice of algorithm, given the equal performance using the
> default compression level, and the difference in code size, I don't
> see why zstd should be preferred here. If anything, it only increases
> the likelihood of hitting another error if we are panicking due to
> some memory corruption issue.

I think it's a good argument - would zlib be simpler in code than zstd?
I've checked the zstd patch from Kees - not complex per se, but would be
great if we could have a simple mechanism, without the need of the ifdef
there for example...

Cheers,


Guilherme

Re: [PATCH 0/5] pstore: Use zstd directly by default for compression

[Ard Biesheuvel]

On Tue, 18 Oct 2022 at 16:11, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>
> On 18/10/2022 05:20, Ard Biesheuvel wrote:
> > [...]
> > So again, I suggest to simply drop this non-feature, and standardize
> > on either zlib or zstd using the library interface exclusively. If
> > someone present a compelling use case, we can always consider adding
> > it back in some form.
> >
> > As for the choice of algorithm, given the equal performance using the
> > default compression level, and the difference in code size, I don't
> > see why zstd should be preferred here. If anything, it only increases
> > the likelihood of hitting another error if we are panicking due to
> > some memory corruption issue.
>
> I think it's a good argument - would zlib be simpler in code than zstd?

I think it should be rather straight-forward. Note that this is what
we had before 2016 when all the 'features' were starting to get added.

> I've checked the zstd patch from Kees - not complex per se, but would be
> great if we could have a simple mechanism, without the need of the ifdef
> there for example...
>
> Cheers,
>
>
> Guilherme