[PATCH V2 0/3] Some pstore improvements V2

[PATCH V2 0/3] Some pstore improvements V2

[Guilherme G. Piccoli]

Hi Kees / Ard and all pstore/efi folks,

this is the second iteration of the patchset with modifications
in some patches, one patch dropped and applied on top of the first
3 patches in the old series [0] (since they were already picked
by Kees).

I've tested this with QEMU guest using OVMF and both ramoops and
efi-pstore backends. Reviews and comments are greatly appreciated!
Cheers,


Guilherme


[0] https://lore.kernel.org/lkml/20221006224212.569555-1-gpiccoli@igalia.com/


Guilherme G. Piccoli (3):
  pstore: Alert on backend write error
  efi: pstore: Follow convention for the efi-pstore backend name
  efi: pstore: Add module parameter for setting the record size

 drivers/firmware/efi/efi-pstore.c | 25 ++++++++++++++++++-------
 fs/pstore/platform.c              | 10 ++++++++++
 2 files changed, 28 insertions(+), 7 deletions(-)

-- 
2.38.0

[PATCH V2 1/3] 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>
---


V2:
- Show error message late, outside of the critical region
(thanks Kees for the idea!).


 fs/pstore/platform.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 06c2c66af332..cbc0b468c1ab 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -393,6 +393,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 	const char	*why;
 	unsigned int	part = 1;
 	unsigned long	flags = 0;
+	int		saved_ret = 0;
 	int		ret;
 
 	why = kmsg_dump_reason_str(reason);
@@ -463,12 +464,21 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 		if (ret == 0 && reason == KMSG_DUMP_OOPS) {
 			pstore_new_entry = 1;
 			pstore_timer_kick();
+		} else {
+			/* Preserve only the first non-zero returned value. */
+			if (!saved_ret)
+				saved_ret = ret;
 		}
 
 		total += record.size;
 		part++;
 	}
 	spin_unlock_irqrestore(&psinfo->buf_lock, flags);
+
+	if (saved_ret) {
+		pr_err_once("backend (%s) writing error (%d)\n", psinfo->name,
+			    saved_ret);
+	}
 }
 
 static struct kmsg_dumper pstore_dumper = {
-- 
2.38.0

[PATCH V2 2/3] 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).

Acked-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---


V2:
- Added Ard's ACK - thanks!


 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 V2 3/3] 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. The historical reasons for that were discussed by
Ard in threads [0][1]:

"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.".

"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."

With that, and since there is not such a limit in the UEFI spec, we
have the confidence to hereby add a module parameter to enable advanced
users to change the UEFI record size for efi-pstore data collection,
this way allowing a much easier reading of the collected log, which is
not scattered anymore among many small files.

Through empirical analysis we observed that extreme low values (like 8
bytes) could eventually cause writing issues, so given that and the OVMF
default discussed, we limited the minimum value to 1024 bytes, which also
is still the default.

[0] https://lore.kernel.org/lkml/CAMj1kXF4UyRMh2Y_KakeNBHvkHhTtavASTAxXinDO1rhPe_wYg@mail.gmail.com/
[1] https://lore.kernel.org/lkml/CAMj1kXFy-2KddGu+dgebAdU9v2sindxVoiHLWuVhqYw+R=kqng@mail.gmail.com/

Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---


V2:
- Fixed a memory corruption bug in the code (that wasn't causing
trouble before due to the fixed sized of record_size), thanks
Ard for spotting this!

- Added Ard's archeology in the commit message plus a comment
with the reasoning behind the minimum value.


 drivers/firmware/efi/efi-pstore.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 97a9e84840a0..827e32427ddb 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,20 @@ static __init int efivars_pstore_init(void)
 	if (efivars_pstore_disable)
 		return 0;
 
-	efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
+	/*
+	 * Notice that 1024 is the minimum here to prevent issues with
+	 * decompression algorithms that were spotted during tests;
+	 * even in the case of not using compression, smaller values would
+	 * just pollute more the pstore FS with many small collected files.
+	 */
+	if (record_size < 1024)
+		record_size = 1024;
+
+	efi_pstore_info.buf = kmalloc(record_size, 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 V2 3/3] efi: pstore: Add module parameter for setting the record size

[Ard Biesheuvel]

On Thu, 13 Oct 2022 at 23:11, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>
> By default, the efi-pstore backend hardcode the UEFI variable size
> as 1024 bytes. The historical reasons for that were discussed by
> Ard in threads [0][1]:
>
> "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.".
>
> "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."
>
> With that, and since there is not such a limit in the UEFI spec, we
> have the confidence to hereby add a module parameter to enable advanced
> users to change the UEFI record size for efi-pstore data collection,
> this way allowing a much easier reading of the collected log, which is
> not scattered anymore among many small files.
>
> Through empirical analysis we observed that extreme low values (like 8
> bytes) could eventually cause writing issues, so given that and the OVMF
> default discussed, we limited the minimum value to 1024 bytes, which also
> is still the default.
>
> [0] https://lore.kernel.org/lkml/CAMj1kXF4UyRMh2Y_KakeNBHvkHhTtavASTAxXinDO1rhPe_wYg@mail.gmail.com/
> [1] https://lore.kernel.org/lkml/CAMj1kXFy-2KddGu+dgebAdU9v2sindxVoiHLWuVhqYw+R=kqng@mail.gmail.com/
>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
>
>
> V2:
> - Fixed a memory corruption bug in the code (that wasn't causing
> trouble before due to the fixed sized of record_size), thanks
> Ard for spotting this!
>
> - Added Ard's archeology in the commit message plus a comment
> with the reasoning behind the minimum value.
>
>
>  drivers/firmware/efi/efi-pstore.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
> index 97a9e84840a0..827e32427ddb 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;
>

I don't think we need this - this is the size of the variable name not
the variable itself.

>                 /*
>                  * If this is the first read() call in the pstore enumeration,
> @@ -224,11 +226,20 @@ static __init int efivars_pstore_init(void)
>         if (efivars_pstore_disable)
>                 return 0;
>
> -       efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
> +       /*
> +        * Notice that 1024 is the minimum here to prevent issues with
> +        * decompression algorithms that were spotted during tests;
> +        * even in the case of not using compression, smaller values would
> +        * just pollute more the pstore FS with many small collected files.
> +        */
> +       if (record_size < 1024)
> +               record_size = 1024;
> +
> +       efi_pstore_info.buf = kmalloc(record_size, 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 V2 1/3] pstore: Alert on backend write error

[Ard Biesheuvel]

On Thu, 13 Oct 2022 at 23:11, Guilherme G. Piccoli <gpiccoli@igalia.com> 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>

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

> ---
>
>
> V2:
> - Show error message late, outside of the critical region
> (thanks Kees for the idea!).
>
>
>  fs/pstore/platform.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 06c2c66af332..cbc0b468c1ab 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -393,6 +393,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>         const char      *why;
>         unsigned int    part = 1;
>         unsigned long   flags = 0;
> +       int             saved_ret = 0;
>         int             ret;
>
>         why = kmsg_dump_reason_str(reason);
> @@ -463,12 +464,21 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>                 if (ret == 0 && reason == KMSG_DUMP_OOPS) {
>                         pstore_new_entry = 1;
>                         pstore_timer_kick();
> +               } else {
> +                       /* Preserve only the first non-zero returned value. */
> +                       if (!saved_ret)
> +                               saved_ret = ret;
>                 }
>
>                 total += record.size;
>                 part++;
>         }
>         spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> +
> +       if (saved_ret) {
> +               pr_err_once("backend (%s) writing error (%d)\n", psinfo->name,
> +                           saved_ret);
> +       }
>  }
>
>  static struct kmsg_dumper pstore_dumper = {
> --
> 2.38.0
>

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

[Guilherme G. Piccoli]

On 14/10/2022 11:46, Ard Biesheuvel wrote:
> [...]
>>         for (;;) {
>> -               varname_size = EFIVARS_DATA_SIZE_MAX;
>> +               varname_size = record_size;
>>
> 
> I don't think we need this - this is the size of the variable name not
> the variable itself.
> 

Ugh, my bad. Do you want to stick with 1024 then?
Thanks,


Guilherme

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

[Ard Biesheuvel]

On Fri, 14 Oct 2022 at 16:58, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>
> On 14/10/2022 11:46, Ard Biesheuvel wrote:
> > [...]
> >>         for (;;) {
> >> -               varname_size = EFIVARS_DATA_SIZE_MAX;
> >> +               varname_size = record_size;
> >>
> >
> > I don't think we need this - this is the size of the variable name not
> > the variable itself.
> >
>
> Ugh, my bad. Do you want to stick with 1024 then?

Yes let's keep this at 1024

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

[Guilherme G. Piccoli]

On 14/10/2022 12:00, Ard Biesheuvel wrote:
> On Fri, 14 Oct 2022 at 16:58, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>>
>> On 14/10/2022 11:46, Ard Biesheuvel wrote:
>>> [...]
>>>>         for (;;) {
>>>> -               varname_size = EFIVARS_DATA_SIZE_MAX;
>>>> +               varname_size = record_size;
>>>>
>>>
>>> I don't think we need this - this is the size of the variable name not
>>> the variable itself.
>>>
>>
>> Ugh, my bad. Do you want to stick with 1024 then?
> 
> Yes let's keep this at 1024

Perfect, will re-send after we have more feedback on patches 1 and 2.
Thanks again,


Guilherme

Re: (subset) [PATCH V2 1/3] pstore: Alert on backend write error

[Kees Cook]

On Thu, 13 Oct 2022 18:06:46 -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.
> 
> [...]

Applied to for-next/pstore, thanks!

[1/3] pstore: Alert on backend write error
      https://git.kernel.org/kees/c/f181c1af1385

-- 
Kees Cook

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

[Kees Cook]

On Thu, Oct 13, 2022 at 06:06:48PM -0300, Guilherme G. Piccoli wrote:
> By default, the efi-pstore backend hardcode the UEFI variable size
> as 1024 bytes. The historical reasons for that were discussed by
> Ard in threads [0][1]:
> 
> "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.".
> 
> "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."
> 
> With that, and since there is not such a limit in the UEFI spec, we
> have the confidence to hereby add a module parameter to enable advanced
> users to change the UEFI record size for efi-pstore data collection,
> this way allowing a much easier reading of the collected log, which is
> not scattered anymore among many small files.
> 
> Through empirical analysis we observed that extreme low values (like 8
> bytes) could eventually cause writing issues, so given that and the OVMF
> default discussed, we limited the minimum value to 1024 bytes, which also
> is still the default.
> 
> [0] https://lore.kernel.org/lkml/CAMj1kXF4UyRMh2Y_KakeNBHvkHhTtavASTAxXinDO1rhPe_wYg@mail.gmail.com/
> [1] https://lore.kernel.org/lkml/CAMj1kXFy-2KddGu+dgebAdU9v2sindxVoiHLWuVhqYw+R=kqng@mail.gmail.com/
> 
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

With the var length change recommended by Ard, yeah, looks good to me.
:)

Thanks!

-Kees

-- 
Kees Cook