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/[email protected]/
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
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 <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
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
On Thu, 13 Oct 2022 at 23:11, Guilherme G. Piccoli <[email protected]> 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 <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
> ---
>
>
> 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
>
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
On Thu, 13 Oct 2022 at 23:11, Guilherme G. Piccoli <[email protected]> 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 <[email protected]>
Acked-by: Ard Biesheuvel <[email protected]>
> ---
>
>
> 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
>
On 14/10/2022 12:00, Ard Biesheuvel wrote:
> On Fri, 14 Oct 2022 at 16:58, Guilherme G. Piccoli <[email protected]> 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
On Fri, 14 Oct 2022 at 16:58, Guilherme G. Piccoli <[email protected]> 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
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 <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
With the var length change recommended by Ard, yeah, looks good to me.
:)
Thanks!
-Kees
--
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