[PATCH 0/8] Some pstore improvements

[PATCH 0/8] Some pstore improvements

[Guilherme G. Piccoli]

Hi pstore maintainers, this is a small series with some improvements
overall. Most of them are minors, but the implicit conversion thing
is a bit more "relevant" in a sense it's more invasive and would fit
more as a "fix".

The code is based on v6.0, and it was tested with multiple compression
algorithms (zstd, deflate, lz4, lzo, 842) and two backends (ramoops and
efi_pstore) - I've used a QEMU UEFI guest and Steam Deck for this goal.

My plan is to also work some ramoops improvements (for example, related
to [0]); they're more complex so it's deferred for a second series,
specific for that.

Reviews and comments are greatly appreciated!
Thanks in advance,

Guilherme


[0] https://lore.kernel.org/lkml/a21201cf-1e5f-fed1-356d-42c83a66fa57@igalia.com/


Guilherme G. Piccoli (8):
  pstore: Improve error reporting in case of backend overlap
  pstore: Expose kmsg_bytes as a module parameter
  pstore: Inform unregistered backend names as well
  pstore: Alert on backend write error
  pstore: Fix long-term implicit conversions in the compression routines
  MAINTAINERS: Add a mailing-list for the pstore infrastructure
  efi: pstore: Follow convention for the efi-pstore backend name
  efi: pstore: Add module parameter for setting the record size

 MAINTAINERS                       |  1 +
 drivers/firmware/efi/efi-pstore.c | 17 +++++---
 fs/pstore/platform.c              | 64 ++++++++++++++++---------------
 3 files changed, 46 insertions(+), 36 deletions(-)

-- 
2.38.0

[PATCH 1/8] pstore: Improve error reporting in case of backend overlap

[Guilherme G. Piccoli]

The pstore infrastructure supports one single backend at a time;
trying to load a another backend causes an error and displays a
message, introduced on commit 0d7cd09a3dbb ("pstore: Improve
register_pstore() error reporting").

Happens that this message is not really clear about the situation,
also the current error returned (-EPERM) isn't accurate, whereas
-EBUSY makes more sense. We have another place in the code that
relies in the -EBUSY return for a similar check.

So, make it consistent here by returning -EBUSY and using a
similar message in both scenarios.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 fs/pstore/platform.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 0c034ea39954..c32957e4b256 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -562,8 +562,9 @@ static int pstore_write_user_compat(struct pstore_record *record,
 int pstore_register(struct pstore_info *psi)
 {
 	if (backend && strcmp(backend, psi->name)) {
-		pr_warn("ignoring unexpected backend '%s'\n", psi->name);
-		return -EPERM;
+		pr_warn("backend '%s' already in use: ignoring '%s'\n",
+			backend, psi->name);
+		return -EBUSY;
 	}
 
 	/* Sanity check flags. */
-- 
2.38.0

[PATCH 2/8] pstore: Expose kmsg_bytes as a module parameter

[Guilherme G. Piccoli]

Currently this tuning is only exposed as a filesystem option,
but most Linux distros automatically mount pstore, hence changing
this setting requires remounting it. Also, if that mount option
wasn't explicitly set it doesn't show up in mount information,
so users cannot check what is the current value of kmsg_bytes.

Let's then expose it as a module parameter, allowing both user
visibility at all times (even if not manually set) and also the
possibility of setting that as a boot/module parameter.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 fs/pstore/platform.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index c32957e4b256..be05090076ce 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -89,6 +89,11 @@ static char *compress =
 module_param(compress, charp, 0444);
 MODULE_PARM_DESC(compress, "compression to use");
 
+/* How much of the kernel log to snapshot */
+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;
 
@@ -100,9 +105,6 @@ struct pstore_zbackend {
 static char *big_oops_buf;
 static size_t big_oops_buf_sz;
 
-/* How much of the console log to snapshot */
-unsigned long kmsg_bytes = CONFIG_PSTORE_DEFAULT_KMSG_BYTES;
-
 void pstore_set_kmsg_bytes(int bytes)
 {
 	kmsg_bytes = bytes;
-- 
2.38.0

[PATCH 3/8] pstore: Inform unregistered backend names as well

[Guilherme G. Piccoli]

Currently we only show the registered ones in the kernel
log; users that change backend for some reason require
first to unregister the current one, so let's confirm this
operation with a message in the log.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 fs/pstore/platform.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index be05090076ce..06c2c66af332 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -665,6 +665,8 @@ void pstore_unregister(struct pstore_info *psi)
 	psinfo = NULL;
 	kfree(backend);
 	backend = NULL;
+
+	pr_info("Unregistered %s as persistent store backend\n", psi->name);
 	mutex_unlock(&psinfo_lock);
 }
 EXPORT_SYMBOL_GPL(pstore_unregister);
-- 
2.38.0

[PATCH 4/8] pstore: Alert on backend write error

[Guilherme G. Piccoli]

The pstore dump function doesn't alert at all on errors - despite
pstore is usually a last resource and if it fails users won't be
able to read the kernel log, this is not the case for server users
with serial access, for example.

So, let's at least attempt to inform such advanced users on the first
backend writing error detected during the kmsg dump - this is also
very useful for pstore debugging purposes.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 fs/pstore/platform.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 06c2c66af332..ee50812fdd2e 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -463,6 +463,9 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 		if (ret == 0 && reason == KMSG_DUMP_OOPS) {
 			pstore_new_entry = 1;
 			pstore_timer_kick();
+		} else {
+			pr_err_once("backend (%s) writing error (%d)\n",
+				    psinfo->name, ret);
 		}
 
 		total += record.size;
-- 
2.38.0

[PATCH 5/8] pstore: Fix long-term implicit conversions in the compression routines

[Guilherme G. Piccoli]

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(-)

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)
-- 
2.38.0

[PATCH 6/8] MAINTAINERS: Add a mailing-list for the pstore infrastructure

[Guilherme G. Piccoli]

Currently, this entry contains only the maintainers name. Add hereby
a mailing-list as well, for archiving purposes.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---


Hi Kees / all, not sure if up to me doing that (apologies if not) and
maybe fsdevel is not the proper list, but I think worth having at least
one list explicitely mentioned in MAINTAINERS in order people use that
as a pstore archive of patches. If you prefer other list, lemme know.

Cheers,

Guilherme


 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 72b9654f764c..16a18125bf0a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16465,6 +16465,7 @@ M:	Kees Cook <keescook@chromium.org>
 M:	Anton Vorontsov <anton@enomsg.org>
 M:	Colin Cross <ccross@android.com>
 M:	Tony Luck <tony.luck@intel.com>
+L:	linux-fsdevel@vger.kernel.org
 S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
 F:	Documentation/admin-guide/ramoops.rst
-- 
2.38.0

[PATCH 7/8] efi: pstore: Follow convention for the efi-pstore backend name

[Guilherme G. Piccoli]

For some reason, the efi-pstore backend name (exposed through the
pstore infrastructure) is hardcoded as "efi", whereas all the other
backends follow a kind of convention in using the module name.

Let's do it here as well, to make user's life easier (they might
use this info for unloading the module backend, for example).

Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 drivers/firmware/efi/efi-pstore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 3bddc152fcd4..97a9e84840a0 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -207,7 +207,7 @@ static int efi_pstore_erase(struct pstore_record *record)
 
 static struct pstore_info efi_pstore_info = {
 	.owner		= THIS_MODULE,
-	.name		= "efi",
+	.name		= KBUILD_MODNAME,
 	.flags		= PSTORE_FLAGS_DMESG,
 	.open		= efi_pstore_open,
 	.close		= efi_pstore_close,
-- 
2.38.0

[PATCH 8/8] efi: pstore: Add module parameter for setting the record size

[Guilherme G. Piccoli]

By default, the efi-pstore backend hardcode the UEFI variable size
as 1024 bytes. That's not a big deal, but at the same time having
no way to change that in the kernel is a bit bummer for specialized
users - there is not such a limit in the UEFI specification.

So, add here a module parameter to enable advanced users to change
the UEFI record size for efi-pstore data collection.
Through empirical analysis we observed that extreme low values
(like 8 bytes) could eventually cause writing issues, so we limit
the low end to 1024 bytes (which is also the default).

Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 drivers/firmware/efi/efi-pstore.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 97a9e84840a0..78c27f6a83aa 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -10,7 +10,9 @@ MODULE_IMPORT_NS(EFIVAR);
 
 #define DUMP_NAME_LEN 66
 
-#define EFIVARS_DATA_SIZE_MAX 1024
+static unsigned int record_size = 1024;
+module_param(record_size, uint, 0444);
+MODULE_PARM_DESC(record_size, "size of each pstore UEFI var (in bytes, min/default=1024)");
 
 static bool efivars_pstore_disable =
 	IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE);
@@ -30,7 +32,7 @@ static int efi_pstore_open(struct pstore_info *psi)
 	if (err)
 		return err;
 
-	psi->data = kzalloc(EFIVARS_DATA_SIZE_MAX, GFP_KERNEL);
+	psi->data = kzalloc(record_size, GFP_KERNEL);
 	if (!psi->data)
 		return -ENOMEM;
 
@@ -52,7 +54,7 @@ static inline u64 generic_id(u64 timestamp, unsigned int part, int count)
 static int efi_pstore_read_func(struct pstore_record *record,
 				efi_char16_t *varname)
 {
-	unsigned long wlen, size = EFIVARS_DATA_SIZE_MAX;
+	unsigned long wlen, size = record_size;
 	char name[DUMP_NAME_LEN], data_type;
 	efi_status_t status;
 	int cnt;
@@ -133,7 +135,7 @@ static ssize_t efi_pstore_read(struct pstore_record *record)
 	efi_status_t status;
 
 	for (;;) {
-		varname_size = EFIVARS_DATA_SIZE_MAX;
+		varname_size = record_size;
 
 		/*
 		 * If this is the first read() call in the pstore enumeration,
@@ -224,11 +226,14 @@ static __init int efivars_pstore_init(void)
 	if (efivars_pstore_disable)
 		return 0;
 
+	if (record_size < 1024)
+		record_size = 1024;
+
 	efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
 	if (!efi_pstore_info.buf)
 		return -ENOMEM;
 
-	efi_pstore_info.bufsize = 1024;
+	efi_pstore_info.bufsize = record_size;
 
 	if (pstore_register(&efi_pstore_info)) {
 		kfree(efi_pstore_info.buf);
-- 
2.38.0

Re: [PATCH 8/8] efi: pstore: Add module parameter for setting the record size

[Kees Cook]

On Thu, Oct 06, 2022 at 07:42:12PM -0300, Guilherme G. Piccoli wrote:
> By default, the efi-pstore backend hardcode the UEFI variable size
> as 1024 bytes. That's not a big deal, but at the same time having
> no way to change that in the kernel is a bit bummer for specialized
> users - there is not such a limit in the UEFI specification.

It seems to have been added in commit
e0d59733f6b1 ("efivars, efi-pstore: Hold off deletion of sysfs entry until the scan is completed")

But I see no mention of why it was introduced or how it was chosen.

I remember hearing some horror stories from Matthew Garrett about older
EFI implementations bricking themselves when they stored large
variables, or something like that, but I don't know if that's meaningful
here at all.

I think it'd be great to make it configurable! Ard, do you have any
sense of what the max/min, etc, should be here?

-- 
Kees Cook

Re: [PATCH 7/8] efi: pstore: Follow convention for the efi-pstore backend name

[Kees Cook]

On Thu, Oct 06, 2022 at 07:42:11PM -0300, Guilherme G. Piccoli wrote:
> For some reason, the efi-pstore backend name (exposed through the
> pstore infrastructure) is hardcoded as "efi", whereas all the other
> backends follow a kind of convention in using the module name.
> 
> Let's do it here as well, to make user's life easier (they might
> use this info for unloading the module backend, for example).
> 
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Looks fine to me. Ard, if you don't object, I can carry this in the
pstore tree.

-- 
Kees Cook

Re: [PATCH 6/8] MAINTAINERS: Add a mailing-list for the pstore infrastructure

[Kees Cook]

On Thu, Oct 06, 2022 at 07:42:10PM -0300, Guilherme G. Piccoli wrote:
> Currently, this entry contains only the maintainers name. Add hereby

This likely need a general refresh, too.

Colin, you haven't sent anything pstore related since 2016. Please let
me know if you'd like to stay listed here.

Anton, same question for you (last I see is 2015).

Tony, I see your recent responses, but if you'd rather not be bothered
by pstore stuff any more, please let me know. :)

> a mailing-list as well, for archiving purposes.
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
> 
> 
> Hi Kees / all, not sure if up to me doing that (apologies if not) and
> maybe fsdevel is not the proper list, but I think worth having at least
> one list explicitely mentioned in MAINTAINERS in order people use that
> as a pstore archive of patches. If you prefer other list, lemme know.

I think that's a reasonable guess! :) Thanks for reminding me about
this. I think I'd rather use linux-hardening@vger.kernel.org, since
we've got a patchwork configured. It's not a _totally_ unreasonable
topic to have there. ;)

-- 
Kees Cook

Re: [PATCH 0/8] Some pstore improvements

[Kees Cook]

On Thu, 6 Oct 2022 19:42:04 -0300, Guilherme G. Piccoli wrote:
> overall. Most of them are minors, but the implicit conversion thing
> is a bit more "relevant" in a sense it's more invasive and would fit
> more as a "fix".
> 
> The code is based on v6.0, and it was tested with multiple compression
> algorithms (zstd, deflate, lz4, lzo, 842) and two backends (ramoops and
> efi_pstore) - I've used a QEMU UEFI guest and Steam Deck for this goal.
> 
> [...]

Applied to for-next/pstore, thanks!

[1/8] pstore: Improve error reporting in case of backend overlap
      https://git.kernel.org/kees/c/55dbe25ee4c8
[2/8] pstore: Expose kmsg_bytes as a module parameter
      https://git.kernel.org/kees/c/1af13c2b6324
[3/8] pstore: Inform unregistered backend names as well
      https://git.kernel.org/kees/c/a4f92789f799

-- 
Kees Cook

Re: [PATCH 4/8] pstore: Alert on backend write error

[Kees Cook]

On Thu, Oct 06, 2022 at 07:42:08PM -0300, Guilherme G. Piccoli wrote:
> The pstore dump function doesn't alert at all on errors - despite
> pstore is usually a last resource and if it fails users won't be
> able to read the kernel log, this is not the case for server users
> with serial access, for example.
> 
> So, let's at least attempt to inform such advanced users on the first
> backend writing error detected during the kmsg dump - this is also
> very useful for pstore debugging purposes.
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
>  fs/pstore/platform.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 06c2c66af332..ee50812fdd2e 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -463,6 +463,9 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>  		if (ret == 0 && reason == KMSG_DUMP_OOPS) {
>  			pstore_new_entry = 1;
>  			pstore_timer_kick();
> +		} else {
> +			pr_err_once("backend (%s) writing error (%d)\n",
> +				    psinfo->name, ret);

We're holding a spinlock here, so doing a pr_*() call isn't a great
idea. It's kind of not a great idea to try to write to the log in the
middle of a dump either, but we do attempt it at the start.

Perhaps keep a saved_ret or something and send it after the spin lock is
released?

-- 
Kees Cook

Re: [PATCH 1/8] pstore: Improve error reporting in case of backend overlap

[Kees Cook]

On Thu, Oct 06, 2022 at 07:42:05PM -0300, Guilherme G. Piccoli wrote:
> The pstore infrastructure supports one single backend at a time;
> trying to load a another backend causes an error and displays a
> message, introduced on commit 0d7cd09a3dbb ("pstore: Improve
> register_pstore() error reporting").
> 
> Happens that this message is not really clear about the situation,
> also the current error returned (-EPERM) isn't accurate, whereas
> -EBUSY makes more sense. We have another place in the code that
> relies in the -EBUSY return for a similar check.
> 
> So, make it consistent here by returning -EBUSY and using a
> similar message in both scenarios.
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
>  fs/pstore/platform.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 0c034ea39954..c32957e4b256 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -562,8 +562,9 @@ static int pstore_write_user_compat(struct pstore_record *record,
>  int pstore_register(struct pstore_info *psi)
>  {
>  	if (backend && strcmp(backend, psi->name)) {
> -		pr_warn("ignoring unexpected backend '%s'\n", psi->name);
> -		return -EPERM;
> +		pr_warn("backend '%s' already in use: ignoring '%s'\n",
> +			backend, psi->name);
> +		return -EBUSY;

Thank you! Yes, this has bothered me for a while. :)

-- 
Kees Cook

RE: [PATCH 6/8] MAINTAINERS: Add a mailing-list for the pstore infrastructure

[Luck, Tony]

> Tony, I see your recent responses, but if you'd rather not be bothered
> by pstore stuff any more, please let me know. :)

Kees,

Occasionally something catches my eye ... but in general I'm not looking at
pstore patches. You can drop me from the MAINTAINERS file.

-Tony

Re: [PATCH 2/8] pstore: Expose kmsg_bytes as a module parameter

[Kees Cook]

On Thu, Oct 06, 2022 at 07:42:06PM -0300, Guilherme G. Piccoli wrote:
> Currently this tuning is only exposed as a filesystem option,
> but most Linux distros automatically mount pstore, hence changing
> this setting requires remounting it. Also, if that mount option
> wasn't explicitly set it doesn't show up in mount information,
> so users cannot check what is the current value of kmsg_bytes.
> 
> Let's then expose it as a module parameter, allowing both user
> visibility at all times (even if not manually set) and also the
> possibility of setting that as a boot/module parameter.

I've been meaning to do this too. :)

> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
>  fs/pstore/platform.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index c32957e4b256..be05090076ce 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -89,6 +89,11 @@ static char *compress =
>  module_param(compress, charp, 0444);
>  MODULE_PARM_DESC(compress, "compression to use");
>  
> +/* How much of the kernel log to snapshot */
> +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;
>  
> @@ -100,9 +105,6 @@ struct pstore_zbackend {
>  static char *big_oops_buf;
>  static size_t big_oops_buf_sz;
>  
> -/* How much of the console log to snapshot */
> -unsigned long kmsg_bytes = CONFIG_PSTORE_DEFAULT_KMSG_BYTES;
> -
>  void pstore_set_kmsg_bytes(int bytes)
>  {
>  	kmsg_bytes = bytes;
> -- 
> 2.38.0

Doing a mount will override the result, so I wonder if there should be
two variables, etc... not a concern for the normal use case.

Also, I've kind of wanted to get rid of a "default" for this and instead
use a value based on the compression vs record sizes, etc. But I didn't
explore it.

-- 
Kees Cook

Re: [PATCH 4/8] pstore: Alert on backend write error

[Guilherme G. Piccoli]

On 06/10/2022 20:27, Kees Cook wrote:
> [...]
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -463,6 +463,9 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>>  		if (ret == 0 && reason == KMSG_DUMP_OOPS) {
>>  			pstore_new_entry = 1;
>>  			pstore_timer_kick();
>> +		} else {
>> +			pr_err_once("backend (%s) writing error (%d)\n",
>> +				    psinfo->name, ret);
> 
> We're holding a spinlock here, so doing a pr_*() call isn't a great
> idea. It's kind of not a great idea to try to write to the log in the
> middle of a dump either, but we do attempt it at the start.
> 
> Perhaps keep a saved_ret or something and send it after the spin lock is
> released?
> 

Hi Kees, thanks a lot for the very quick review!!

Agree with you, I'll rework this one.
Do you agree with showing only a single error? For me makes sense since
we just wanna hint advanced users (+ people-debugging-pstore heh) that
something went wrong.

Cheers,


Guilherme

Re: [PATCH 1/8] pstore: Improve error reporting in case of backend overlap

[Guilherme G. Piccoli]

On 06/10/2022 20:27, Kees Cook wrote:
> On Thu, Oct 06, 2022 at 07:42:05PM -0300, Guilherme G. Piccoli wrote:
>> The pstore infrastructure supports one single backend at a time;
>> trying to load a another backend causes an error and displays a
>> message, introduced on commit 0d7cd09a3dbb ("pstore: Improve
>> register_pstore() error reporting").
>>
>> Happens that this message is not really clear about the situation,
>> also the current error returned (-EPERM) isn't accurate, whereas
>> -EBUSY makes more sense. We have another place in the code that
>> relies in the -EBUSY return for a similar check.
>>
>> So, make it consistent here by returning -EBUSY and using a
>> similar message in both scenarios.
>>
>> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
>> ---
>>  fs/pstore/platform.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index 0c034ea39954..c32957e4b256 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -562,8 +562,9 @@ static int pstore_write_user_compat(struct pstore_record *record,
>>  int pstore_register(struct pstore_info *psi)
>>  {
>>  	if (backend && strcmp(backend, psi->name)) {
>> -		pr_warn("ignoring unexpected backend '%s'\n", psi->name);
>> -		return -EPERM;
>> +		pr_warn("backend '%s' already in use: ignoring '%s'\n",
>> +			backend, psi->name);
>> +		return -EBUSY;
> 
> Thank you! Yes, this has bothered me for a while. :)

Heheh ditto! Thank you for the great and fast review =)

Re: [PATCH 5/8] pstore: Fix long-term implicit conversions in the compression routines

[Kees Cook]

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.

-- 
Kees Cook

Re: [PATCH 6/8] MAINTAINERS: Add a mailing-list for the pstore infrastructure

[Kees Cook]

On Thu, Oct 06, 2022 at 11:29:02PM +0000, Luck, Tony wrote:
> > Tony, I see your recent responses, but if you'd rather not be bothered
> > by pstore stuff any more, please let me know. :)
> 
> Kees,
> 
> Occasionally something catches my eye ... but in general I'm not looking at
> pstore patches. You can drop me from the MAINTAINERS file.

Do you mind if I leave you? It's nice to have extra eyes on it when it
does happen! :)

-- 
Kees Cook

Re: [PATCH 4/8] pstore: Alert on backend write error

[Kees Cook]

On Thu, Oct 06, 2022 at 08:34:44PM -0300, Guilherme G. Piccoli wrote:
> On 06/10/2022 20:27, Kees Cook wrote:
> > [...]
> >> --- a/fs/pstore/platform.c
> >> +++ b/fs/pstore/platform.c
> >> @@ -463,6 +463,9 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> >>  		if (ret == 0 && reason == KMSG_DUMP_OOPS) {
> >>  			pstore_new_entry = 1;
> >>  			pstore_timer_kick();
> >> +		} else {
> >> +			pr_err_once("backend (%s) writing error (%d)\n",
> >> +				    psinfo->name, ret);
> > 
> > We're holding a spinlock here, so doing a pr_*() call isn't a great
> > idea. It's kind of not a great idea to try to write to the log in the
> > middle of a dump either, but we do attempt it at the start.
> > 
> > Perhaps keep a saved_ret or something and send it after the spin lock is
> > released?
> > 
> 
> Hi Kees, thanks a lot for the very quick review!!
> 
> Agree with you, I'll rework this one.
> Do you agree with showing only a single error? For me makes sense since
> we just wanna hint advanced users (+ people-debugging-pstore heh) that
> something went wrong.

Yeah, I agree -- it's going to be for folks working on pstore code. :)

-- 
Kees Cook

Re: [PATCH 5/8] pstore: Fix long-term implicit conversions in the compression routines

[Ard Biesheuvel]

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?

Re: [PATCH 7/8] efi: pstore: Follow convention for the efi-pstore backend name

[Ard Biesheuvel]

On Fri, 7 Oct 2022 at 01:16, Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Oct 06, 2022 at 07:42:11PM -0300, Guilherme G. Piccoli wrote:
> > For some reason, the efi-pstore backend name (exposed through the
> > pstore infrastructure) is hardcoded as "efi", whereas all the other
> > backends follow a kind of convention in using the module name.
> >
> > Let's do it here as well, to make user's life easier (they might
> > use this info for unloading the module backend, for example).
> >
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
>
> Looks fine to me. Ard, if you don't object, I can carry this in the
> pstore tree.
>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

Re: [PATCH 8/8] efi: pstore: Add module parameter for setting the record size

[Ard Biesheuvel]

On Fri, 7 Oct 2022 at 01:16, Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Oct 06, 2022 at 07:42:12PM -0300, Guilherme G. Piccoli wrote:
> > By default, the efi-pstore backend hardcode the UEFI variable size
> > as 1024 bytes. That's not a big deal, but at the same time having
> > no way to change that in the kernel is a bit bummer for specialized
> > users - there is not such a limit in the UEFI specification.
>
> It seems to have been added in commit
> e0d59733f6b1 ("efivars, efi-pstore: Hold off deletion of sysfs entry until the scan is completed")
>

Yeah fortunately that kludge is gone now.

> But I see no mention of why it was introduced or how it was chosen.
>

There is some cargo cult from prehistoric EFI times going on here, it
seems. Or maybe just misinterpretation of the maximum size for the
variable *name* vs the variable itself.

> I remember hearing some horror stories from Matthew Garrett about older
> EFI implementations bricking themselves when they stored large
> variables, or something like that, but I don't know if that's meaningful
> here at all.
>

This was related to filling up the variable store to the point where
SetVariable() calls in the firmware itself would start failing,
bricking the system in the process.

The efivars layer below efi-pstore will take care of this, by ensuring
upfront that sufficient space is available.

> I think it'd be great to make it configurable! Ard, do you have any
> sense of what the max/min, etc, should be here?
>

Given that dbx on an arbitrary EFI system with secure boot enabled is
already almost 4k, that seems like a reasonable default. As for the
upper bound, there is no way to know what weird firmware bugs you
might tickle by choosing highly unusual values here.

If you need to store lots of data, you might want to look at [0] for
some work done in the past on using capsule update for preserving data
across a reboot. In the general case, this is not as useful, as the
capsule is only delivered to the firmware after invoking the
ResetSystem() EFI runtime service (as opposed to SetVariable() calls
taking effect immediately). However, if you need to capture large
amounts of data, and can tolerate the uncertainty involved in the
capsule approach, it might be a reasonable option.

[0] https://lore.kernel.org/all/20200312011335.70750-1-qiuxu.zhuo@intel.com/

Re: [PATCH 8/8] efi: pstore: Add module parameter for setting the record size

[Guilherme G. Piccoli]

First of all, thanks Ard for the historical explanation!


On 07/10/2022 06:11, Ard Biesheuvel wrote:
> [...]
>> I think it'd be great to make it configurable! Ard, do you have any
>> sense of what the max/min, etc, should be here?
>>
> 
> Given that dbx on an arbitrary EFI system with secure boot enabled is
> already almost 4k, that seems like a reasonable default. As for the
> upper bound, there is no way to know what weird firmware bugs you
> might tickle by choosing highly unusual values here.
> 
> If you need to store lots of data, you might want to look at [0] for
> some work done in the past on using capsule update for preserving data
> across a reboot. In the general case, this is not as useful, as the
> capsule is only delivered to the firmware after invoking the
> ResetSystem() EFI runtime service (as opposed to SetVariable() calls
> taking effect immediately). However, if you need to capture large
> amounts of data, and can tolerate the uncertainty involved in the
> capsule approach, it might be a reasonable option.
> 
> [0] https://lore.kernel.org/all/20200312011335.70750-1-qiuxu.zhuo@intel.com/

So, you mean 4K as the default? I can change, but I would try to not
mess with the current users, is there a case you can imagine something
like 4k would fail? Maybe 2K is safer?

As for the maximum, I've tested with many values, and when it's larger
than ~30000 for edk2/ovmf, it fails with EFI_OUT_OF_RESOURCES and
doesn't collect the log; other than that, no issues observed.

When set to ~24000, the interesting is that we have fewer big logs in
/sys/fs/pstore, so it's nice to see compared to the bunch of 1K files heheh

Anyway, let's agree on the default and then I can resubmit that, I'm
glad you both consider that it's a good idea =)

Thanks,


Guilherme

Re: [PATCH 8/8] efi: pstore: Add module parameter for setting the record size

[Ard Biesheuvel]

On Fri, 7 Oct 2022 at 15:00, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>
> First of all, thanks Ard for the historical explanation!
>
>
> On 07/10/2022 06:11, Ard Biesheuvel wrote:
> > [...]
> >> I think it'd be great to make it configurable! Ard, do you have any
> >> sense of what the max/min, etc, should be here?
> >>
> >
> > Given that dbx on an arbitrary EFI system with secure boot enabled is
> > already almost 4k, that seems like a reasonable default. As for the
> > upper bound, there is no way to know what weird firmware bugs you
> > might tickle by choosing highly unusual values here.
> >
> > If you need to store lots of data, you might want to look at [0] for
> > some work done in the past on using capsule update for preserving data
> > across a reboot. In the general case, this is not as useful, as the
> > capsule is only delivered to the firmware after invoking the
> > ResetSystem() EFI runtime service (as opposed to SetVariable() calls
> > taking effect immediately). However, if you need to capture large
> > amounts of data, and can tolerate the uncertainty involved in the
> > capsule approach, it might be a reasonable option.
> >
> > [0] https://lore.kernel.org/all/20200312011335.70750-1-qiuxu.zhuo@intel.com/
>
> So, you mean 4K as the default? I can change, but I would try to not
> mess with the current users, is there a case you can imagine something
> like 4k would fail? Maybe 2K is safer?
>

Reducing the number of writes 4x on systems that can support this has
its own advantages, so I am willing to risk it.

> As for the maximum, I've tested with many values, and when it's larger
> than ~30000 for edk2/ovmf, it fails with EFI_OUT_OF_RESOURCES and
> doesn't collect the log; other than that, no issues observed.
>

OVMF has

OvmfPkg/OvmfPkgX64.dsc:
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
OvmfPkg/OvmfPkgX64.dsc:
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400

where the first one is without secure boot and the second with secure boot.

Interestingly, the default is

gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x400

so this is probably where this 1k number comes from. So perhaps it is
better to leave it at 1k after all :-(

> When set to ~24000, the interesting is that we have fewer big logs in
> /sys/fs/pstore, so it's nice to see compared to the bunch of 1K files heheh
>
> Anyway, let's agree on the default and then I can resubmit that, I'm
> glad you both consider that it's a good idea =)
>
> Thanks,
>
>
> Guilherme

Re: [PATCH 8/8] efi: pstore: Add module parameter for setting the record size

[Guilherme G. Piccoli]

On 07/10/2022 10:19, Ard Biesheuvel wrote:
> [...]
> 
> OVMF has
> 
> OvmfPkg/OvmfPkgX64.dsc:
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> OvmfPkg/OvmfPkgX64.dsc:
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> 
> where the first one is without secure boot and the second with secure boot.
> 
> Interestingly, the default is
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x400
> 
> so this is probably where this 1k number comes from. So perhaps it is
> better to leave it at 1k after all :-(
> 

Oh darn...

So, let's stick with 1024 then? If so, no need for re-submitting right?
Thanks again,


Guilherme

Re: [PATCH 8/8] efi: pstore: Add module parameter for setting the record size

[Ard Biesheuvel]

On Fri, 7 Oct 2022 at 15:46, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>
> On 07/10/2022 10:19, Ard Biesheuvel wrote:
> > [...]
> >
> > OVMF has
> >
> > OvmfPkg/OvmfPkgX64.dsc:
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> > OvmfPkg/OvmfPkgX64.dsc:
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> >
> > where the first one is without secure boot and the second with secure boot.
> >
> > Interestingly, the default is
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x400
> >
> > so this is probably where this 1k number comes from. So perhaps it is
> > better to leave it at 1k after all :-(
> >
>
> Oh darn...
>
> So, let's stick with 1024 then? If so, no need for re-submitting right?

Well, I did spot this oddity

        efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
        if (!efi_pstore_info.buf)
                return -ENOMEM;

       efi_pstore_info.bufsize = 1024;

So that hardcoded 4096 looks odd, but at least it is larger than the
default 1024. So what happens if you increase the record size to >
4096?

RE: [PATCH 6/8] MAINTAINERS: Add a mailing-list for the pstore infrastructure

[Luck, Tony]

>> Occasionally something catches my eye ... but in general I'm not looking at
>> pstore patches. You can drop me from the MAINTAINERS file.
>
> Do you mind if I leave you? It's nice to have extra eyes on it when it
> does happen! :)

That's ok too. pstore traffic isn't a significant fraction of my e-mail flow.

-Tony

Re: [PATCH 6/8] MAINTAINERS: Add a mailing-list for the pstore infrastructure

[Colin Cross]

On Thu, Oct 6, 2022 at 4:22 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Oct 06, 2022 at 07:42:10PM -0300, Guilherme G. Piccoli wrote:
> > Currently, this entry contains only the maintainers name. Add hereby
>
> This likely need a general refresh, too.
>
> Colin, you haven't sent anything pstore related since 2016. Please let
> me know if you'd like to stay listed here.

You can remove me.

Re: [PATCH 6/8] MAINTAINERS: Add a mailing-list for the pstore infrastructure

[Guilherme G. Piccoli]

On 06/10/2022 20:22, Kees Cook wrote:
> On Thu, Oct 06, 2022 at 07:42:10PM -0300, Guilherme G. Piccoli wrote:
>> Currently, this entry contains only the maintainers name. Add hereby
> 
> This likely need a general refresh, too.
> 
> Colin, you haven't sent anything pstore related since 2016. Please let
> me know if you'd like to stay listed here.
> 
> Anton, same question for you (last I see is 2015).
> 
> Tony, I see your recent responses, but if you'd rather not be bothered
> by pstore stuff any more, please let me know. :)
> 

Hi Kees, in case you want an extra pair of eyes to review/test pstore,
you can add me as reviewer - since we're using pstore in the Steam Deck
now and I have some improvements/fixes planned, I could help testing and
reviewing the stuff as well.

Feel free to ignore that as well if you think it's not pertinent!
Thanks,


Guilherme

Re: [PATCH 8/8] efi: pstore: Add module parameter for setting the record size

[Guilherme G. Piccoli]

On 07/10/2022 12:06, Ard Biesheuvel wrote:
> [...]
> Well, I did spot this oddity
> 
>         efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
>         if (!efi_pstore_info.buf)
>                 return -ENOMEM;
> 
>        efi_pstore_info.bufsize = 1024;
> 
> So that hardcoded 4096 looks odd, but at least it is larger than the
> default 1024. So what happens if you increase the record size to >
> 4096?

This is a very good finding, thanks a bunch Ard and apologies for this
mistake!

Before this patch it was "safe" doing this way since the allocation was
4096 whereas the size value was 1024. Now, with my change this is not
valid anymore, and my feeling is that it worked fine in my tests because
I'm testing panic (which is a single CPU/no-IRQ scenario), so basically
we're corrupting memory...but nothing broke in my tests due to panic
scenario.

Thanks again, I'll fix that - need to allocate record_size.


Guilherme

Re: [PATCH 6/8] MAINTAINERS: Add a mailing-list for the pstore infrastructure

[Kees Cook]

On Fri, Oct 07, 2022 at 01:32:57PM -0300, Guilherme G. Piccoli wrote:
> On 06/10/2022 20:22, Kees Cook wrote:
> > On Thu, Oct 06, 2022 at 07:42:10PM -0300, Guilherme G. Piccoli wrote:
> >> Currently, this entry contains only the maintainers name. Add hereby
> > 
> > This likely need a general refresh, too.
> > 
> > Colin, you haven't sent anything pstore related since 2016. Please let
> > me know if you'd like to stay listed here.
> > 
> > Anton, same question for you (last I see is 2015).
> > 
> > Tony, I see your recent responses, but if you'd rather not be bothered
> > by pstore stuff any more, please let me know. :)
> > 
> 
> Hi Kees, in case you want an extra pair of eyes to review/test pstore,
> you can add me as reviewer - since we're using pstore in the Steam Deck
> now and I have some improvements/fixes planned, I could help testing and
> reviewing the stuff as well.

Sure! That'd be lovely. :)

-- 
Kees Cook

Re: [PATCH 8/8] efi: pstore: Add module parameter for setting the record size

[Kees Cook]

On Fri, Oct 07, 2022 at 10:45:33AM -0300, Guilherme G. Piccoli wrote:
> On 07/10/2022 10:19, Ard Biesheuvel wrote:
> > [...]
> > 
> > OVMF has
> > 
> > OvmfPkg/OvmfPkgX64.dsc:
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> > OvmfPkg/OvmfPkgX64.dsc:
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> > 
> > where the first one is without secure boot and the second with secure boot.
> > 
> > Interestingly, the default is
> > 
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x400
> > 
> > so this is probably where this 1k number comes from. So perhaps it is
> > better to leave it at 1k after all :-(
> > 
> 
> Oh darn...
> 
> So, let's stick with 1024 then? If so, no need for re-submitting right?

Given OVMF showing this as a max, it doesn't seem right to also make
this a minimum? Perhaps choose a different minimum to be enforced.

Also, can you update the commit log with Ard's archeology on
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ?

-- 
Kees Cook

Re: [PATCH 5/8] pstore: Fix long-term implicit conversions in the compression routines

[Kees Cook]

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/

-- 
Kees Cook

Re: [PATCH 8/8] efi: pstore: Add module parameter for setting the record size

[Guilherme G. Piccoli]

On 07/10/2022 16:32, Kees Cook wrote:
> [...]
> Given OVMF showing this as a max, it doesn't seem right to also make
> this a minimum? Perhaps choose a different minimum to be enforced.

Hi Kees! Through my tests, I've noticed low values tend to cause issues
(didn't go further in the investigation), IIRC even 512 caused problems
on "deflate" (worked in the others).

I'll try again 512 to see how it goes, but I'm not so sure what would be
the use of such low values, it does truncate a lot and "pollute" the
pstore fs with many small files. But I can go with any value you/Ard
think is appropriate (given it works with all compression algorithms
heh) - currently the minimum of 1024 is enforced in the patch.

> 
> Also, can you update the commit log with Ard's archeology on
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ?
> 

Sure, of course!
Cheers,


Guilherme

Re: [PATCH 8/8] efi: pstore: Add module parameter for setting the record size

[Kees Cook]

On October 7, 2022 4:29:55 PM PDT, "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote:
>On 07/10/2022 16:32, Kees Cook wrote:
>> [...]
>> Given OVMF showing this as a max, it doesn't seem right to also make
>> this a minimum? Perhaps choose a different minimum to be enforced.
>
>Hi Kees! Through my tests, I've noticed low values tend to cause issues
>(didn't go further in the investigation), IIRC even 512 caused problems
>on "deflate" (worked in the others).
>
>I'll try again 512 to see how it goes, but I'm not so sure what would be
>the use of such low values, it does truncate a lot and "pollute" the
>pstore fs with many small files. But I can go with any value you/Ard
>think is appropriate (given it works with all compression algorithms
>heh) - currently the minimum of 1024 is enforced in the patch.

Right, but not everyone uses compression. On the other hand, this was never configurable before, so, sure, let's do 1k as a minimum. (And a comment in the source.)


-- 
Kees Cook

Re: [PATCH 5/8] pstore: Fix long-term implicit conversions in the compression routines

[Guilherme G. Piccoli]

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

Re: [PATCH 5/8] pstore: Fix long-term implicit conversions in the compression routines

[Ard Biesheuvel]

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?

Re: [PATCH 5/8] pstore: Fix long-term implicit conversions in the compression routines

[Guilherme G. Piccoli]

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

Re: [PATCH 5/8] pstore: Fix long-term implicit conversions in the compression routines

[Ard Biesheuvel]

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?

Re: [PATCH 5/8] pstore: Fix long-term implicit conversions in the compression routines

[Guilherme G. Piccoli]

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

Re: [PATCH 5/8] pstore: Fix long-term implicit conversions in the compression routines

[Kees Cook]

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.

-- 
Kees Cook

Re: [PATCH 5/8] pstore: Fix long-term implicit conversions in the compression routines

[Ard Biesheuvel]

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?

Re: [PATCH 2/8] pstore: Expose kmsg_bytes as a module parameter

[Guilherme G. Piccoli]

On 06/10/2022 20:32, Kees Cook wrote:
> [...]
> Doing a mount will override the result, so I wonder if there should be
> two variables, etc... not a concern for the normal use case.
> 
> Also, I've kind of wanted to get rid of a "default" for this and instead
> use a value based on the compression vs record sizes, etc. But I didn't
> explore it.
> 

For some reason I forgot to respond that, sorry!

I didn't understand exactly how the mount would override things; I've
done some tests:

(1) booted with the new kmsg_bytes module parameter set to 64k, and it
was preserved across multiple mount/umount cycles.

(2) When I manually had "-o kmsg_bytes=16k" set during the mount
operation, it worked as expected, setting the thing to 16k (and
reflecting in the module parameter, as observed in /sys/modules).

Maybe I'm missing something?

Now, regarding the idea of setting that as a function of
compression/record_sizes, I feel it makes sense and could be worked,
like a heuristic right?

In the end, if you think properly, what is the purpose of kmsg_bytes?
Wouldn't make sense to just fill the record_size with the maximum amount
of data it can handle? Of course there is the partitioning thing, but in
the end kmsg_bytes seems a mechanism to _restrict_ the data collection,
so maybe the default would be a value that means "save whatever you can
handle" (maybe 0), and if the parameter/mount option is set, then pstore
would restrict the saved size.
Thoughts?

Thanks,


Guilherme

Re: [PATCH 0/8] Some pstore improvements

[Guilherme G. Piccoli]

On 06/10/2022 20:24, Kees Cook wrote:
> On Thu, 6 Oct 2022 19:42:04 -0300, Guilherme G. Piccoli wrote:
>> overall. Most of them are minors, but the implicit conversion thing
>> is a bit more "relevant" in a sense it's more invasive and would fit
>> more as a "fix".
>>
>> The code is based on v6.0, and it was tested with multiple compression
>> algorithms (zstd, deflate, lz4, lzo, 842) and two backends (ramoops and
>> efi_pstore) - I've used a QEMU UEFI guest and Steam Deck for this goal.
>>
>> [...]
> 
> Applied to for-next/pstore, thanks!
> 
> [1/8] pstore: Improve error reporting in case of backend overlap
>       https://git.kernel.org/kees/c/55dbe25ee4c8
> [2/8] pstore: Expose kmsg_bytes as a module parameter
>       https://git.kernel.org/kees/c/1af13c2b6324
> [3/8] pstore: Inform unregistered backend names as well
>       https://git.kernel.org/kees/c/a4f92789f799
> 

Thanks Kees! just a heads-up on how I'll proceed.

(a) Patches 1-3 were added already.

(b) MAINTAINERS patch was reworked by yourself in the other series, so
I'll discard my version.

(c) I'll rework patches 4 and 8 and re-submit them plus patch 7
(including the ACK from Ard).

(d) Gonna discard for now patch 5, planning to test a new version on top
of the crypto acomp interface V2 from Ard/you.

Cheers,


Guilherme

Re: [PATCH 2/8] pstore: Expose kmsg_bytes as a module parameter

[Kees Cook]

On Wed, Oct 12, 2022 at 12:33:36PM -0300, Guilherme G. Piccoli wrote:
> On 06/10/2022 20:32, Kees Cook wrote:
> > [...]
> > Doing a mount will override the result, so I wonder if there should be
> > two variables, etc... not a concern for the normal use case.
> > 
> > Also, I've kind of wanted to get rid of a "default" for this and instead
> > use a value based on the compression vs record sizes, etc. But I didn't
> > explore it.
> > 
> 
> For some reason I forgot to respond that, sorry!
> 
> I didn't understand exactly how the mount would override things; I've
> done some tests:
> 
> (1) booted with the new kmsg_bytes module parameter set to 64k, and it
> was preserved across multiple mount/umount cycles.
> 
> (2) When I manually had "-o kmsg_bytes=16k" set during the mount
> operation, it worked as expected, setting the thing to 16k (and
> reflecting in the module parameter, as observed in /sys/modules).

What I was imagining was the next step:

(3) umount, unload the backend, load a new backend, and mount it
    without kmsg_bytes specified -- kmsg_bytes will be 16k, not 64k.

It's a pretty extreme corner-case, I realize. :) However, see below...

> In the end, if you think properly, what is the purpose of kmsg_bytes?
> Wouldn't make sense to just fill the record_size with the maximum amount
> of data it can handle? Of course there is the partitioning thing, but in
> the end kmsg_bytes seems a mechanism to _restrict_ the data collection,
> so maybe the default would be a value that means "save whatever you can
> handle" (maybe 0), and if the parameter/mount option is set, then pstore
> would restrict the saved size.

Right, kmsg_bytes is the maximum size to save from the console on a
crash. The design of the ram backend was to handle really small amounts
of persistent RAM -- if a single crash would eat all of it and possibly
wrap around, it could write over useful parts at the end (since it's
written from the end to the front). However, I think somewhere along
the way, stricter logic was added to the ram backend:

        /*
         * Explicitly only take the first part of any new crash.
         * If our buffer is larger than kmsg_bytes, this can never happen,
         * and if our buffer is smaller than kmsg_bytes, we don't want the
         * report split across multiple records.
         */
        if (record->part != 1)
                return -ENOSPC;

This limits it to just a single record.

However, this does _not_ exist for other backends, so they will see up
to kmsg_bytes-size dumps split across psinfo->bufsize many records. For
the backends, this record size is not always fixed:

- efi uses 1024, even though it allocates 4096 (as was pointed out earlier)
- zone uses kmsg_bytes
- acpi-erst uses some ACPI value from ACPI_ERST_GET_ERROR_LENGTH
- ppc-nvram uses the configured size of nvram partition

Honestly, it seems like the 64k default is huge, but I don't think it
should be "unlimited" given the behaviors of ppc-nvram, and acpi-erst.
For ram and efi, it's effectively unlimited because of the small bufsizes
(and the "only 1 record" logic in ram).

Existing documentation I can find online seem to imply making it smaller
(8000 bytes[1], 16000 bytes), but without justification. Even the "main"
documentation[2] doesn't mention it.

-Kees

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/pstore
[2] https://docs.kernel.org/admin-guide/ramoops.html

-- 
Kees Cook

Re: [PATCH 0/8] Some pstore improvements

[Kees Cook]

On Wed, Oct 12, 2022 at 12:50:50PM -0300, Guilherme G. Piccoli wrote:
> On 06/10/2022 20:24, Kees Cook wrote:
> > On Thu, 6 Oct 2022 19:42:04 -0300, Guilherme G. Piccoli wrote:
> >> overall. Most of them are minors, but the implicit conversion thing
> >> is a bit more "relevant" in a sense it's more invasive and would fit
> >> more as a "fix".
> >>
> >> The code is based on v6.0, and it was tested with multiple compression
> >> algorithms (zstd, deflate, lz4, lzo, 842) and two backends (ramoops and
> >> efi_pstore) - I've used a QEMU UEFI guest and Steam Deck for this goal.
> >>
> >> [...]
> > 
> > Applied to for-next/pstore, thanks!
> > 
> > [1/8] pstore: Improve error reporting in case of backend overlap
> >       https://git.kernel.org/kees/c/55dbe25ee4c8
> > [2/8] pstore: Expose kmsg_bytes as a module parameter
> >       https://git.kernel.org/kees/c/1af13c2b6324
> > [3/8] pstore: Inform unregistered backend names as well
> >       https://git.kernel.org/kees/c/a4f92789f799
> > 
> 
> Thanks Kees! just a heads-up on how I'll proceed.
> 
> (a) Patches 1-3 were added already.
> 
> (b) MAINTAINERS patch was reworked by yourself in the other series, so
> I'll discard my version.
> 
> (c) I'll rework patches 4 and 8 and re-submit them plus patch 7
> (including the ACK from Ard).
> 
> (d) Gonna discard for now patch 5, planning to test a new version on top
> of the crypto acomp interface V2 from Ard/you.

Sounds good; thanks!

-- 
Kees Cook

Re: (subset) [PATCH 7/8] efi: pstore: Follow convention for the efi-pstore backend name

[Kees Cook]

On Thu, 6 Oct 2022 19:42:11 -0300, Guilherme G. Piccoli wrote:
> For some reason, the efi-pstore backend name (exposed through the
> pstore infrastructure) is hardcoded as "efi", whereas all the other
> backends follow a kind of convention in using the module name.
> 
> Let's do it here as well, to make user's life easier (they might
> use this info for unloading the module backend, for example).
> 
> [...]

Applied to for-next/pstore, thanks!

[7/8] efi: pstore: Follow convention for the efi-pstore backend name
      https://git.kernel.org/kees/c/39bae0ee0656

-- 
Kees Cook

Re: [PATCH 2/8] pstore: Expose kmsg_bytes as a module parameter

[Guilherme G. Piccoli]

Hi Kees, my apologies for the (big) delay in answering that! I kept it
marked to respond after tests, ended-up forgetting, but now did all the
tests and finally I'm able to respond.


On 12/10/2022 14:58, Kees Cook wrote:
> [...]
>> I didn't understand exactly how the mount would override things; I've
>> done some tests:
>>
>> (1) booted with the new kmsg_bytes module parameter set to 64k, and it
>> was preserved across multiple mount/umount cycles.
>>
>> (2) When I manually had "-o kmsg_bytes=16k" set during the mount
>> operation, it worked as expected, setting the thing to 16k (and
>> reflecting in the module parameter, as observed in /sys/modules).
> 
> What I was imagining was the next step:
> 
> (3) umount, unload the backend, load a new backend, and mount it
>     without kmsg_bytes specified -- kmsg_bytes will be 16k, not 64k.
> 
> It's a pretty extreme corner-case, I realize. :) However, see below...

Oh okay, thanks for pointing that! Indeed, in your test-case I've faced
the issue of the retained kmsg_bytes...although, I agree it's an extreme
corner-case heheh



> [...]
> Right, kmsg_bytes is the maximum size to save from the console on a
> crash. The design of the ram backend was to handle really small amounts
> of persistent RAM -- if a single crash would eat all of it and possibly
> wrap around, it could write over useful parts at the end (since it's
> written from the end to the front). However, I think somewhere along
> the way, stricter logic was added to the ram backend:
> 
>         /*
>          * Explicitly only take the first part of any new crash.
>          * If our buffer is larger than kmsg_bytes, this can never happen,
>          * and if our buffer is smaller than kmsg_bytes, we don't want the
>          * report split across multiple records.
>          */
>         if (record->part != 1)
>                 return -ENOSPC;
> 
> This limits it to just a single record.

Indeed, and I already considered that in the past...why was that
restricted to a single record, right? I had plans to change it, lemme
know your thoughts. (Reference:
https://lore.kernel.org/linux-fsdevel/a21201cf-1e5f-fed1-356d-42c83a66fa57@igalia.com/)



> However, this does _not_ exist for other backends, so they will see up
> to kmsg_bytes-size dumps split across psinfo->bufsize many records. For
> the backends, this record size is not always fixed:
> 
> - efi uses 1024, even though it allocates 4096 (as was pointed out earlier)
> - zone uses kmsg_bytes
> - acpi-erst uses some ACPI value from ACPI_ERST_GET_ERROR_LENGTH
> - ppc-nvram uses the configured size of nvram partition
> 
> Honestly, it seems like the 64k default is huge, but I don't think it
> should be "unlimited" given the behaviors of ppc-nvram, and acpi-erst.
> For ram and efi, it's effectively unlimited because of the small bufsizes
> (and the "only 1 record" logic in ram).
> 
> Existing documentation I can find online seem to imply making it smaller
> (8000 bytes[1], 16000 bytes), but without justification. Even the "main"
> documentation[2] doesn't mention it.

Right! Also, on top of that, there is a kind of "tricky" logic in which
this value is not always respected. For example, in the Steam Deck case
we have a region of ~10MB, and set record size of the ramoops backend to
2MB. This is the amount collected, it doesn't respect kmsg_bytes, since
it checks the amount dumped vs kmsg_bytes effectively _after_ the first
part is written (which in the ramoops case, it's a single write), hence
this check is never "respected" there. I don't consider that as a bug,
more a flexibility for the ramoops case heh

In any way, lemme know if you want to have a "revamp" in the meaning of
kmsg_bytes, I'd be glad in discuss/work on that =)

Thanks,


Guilherme