[Issue]
Currently, efi_pstore driver simply overwrites existing panic messages in NVRAM.
So, in the following scenario, we will lose 1st panic messages.
1. kernel panics.
2. efi_pstore is kicked and writes panic messages to NVRAM.
3. system reboots.
4. kernel panics again before a user checks the 1st panic messages in NVRAM.
[Solution]
A reasonable solution to fix the issue is just holding multiple logs without erasing
existing entries.
This patch removes a logic erasing existing entries in a write callback
because the logic is not needed in the write callback to support holding multiple logs.
Signed-off-by: Seiji Aguchi <[email protected]>
---
drivers/firmware/efivars.c | 39 ++-------------------------------------
1 files changed, 2 insertions(+), 37 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index bee14cc..fbe9202 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -701,18 +701,13 @@ static int efi_pstore_write(enum pstore_type_id type,
unsigned int part, size_t size, struct pstore_info *psi)
{
char name[DUMP_NAME_LEN];
- char stub_name[DUMP_NAME_LEN];
efi_char16_t efi_name[DUMP_NAME_LEN];
efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
struct efivars *efivars = psi->data;
- struct efivar_entry *entry, *found = NULL;
int i, ret = 0;
u64 storage_space, remaining_space, max_variable_size;
efi_status_t status = EFI_NOT_FOUND;
- sprintf(stub_name, "dump-type%u-%u-", type, part);
- sprintf(name, "%s%lu", stub_name, get_seconds());
-
spin_lock(&efivars->lock);
/*
@@ -730,35 +725,8 @@ static int efi_pstore_write(enum pstore_type_id type,
return -ENOSPC;
}
- for (i = 0; i < DUMP_NAME_LEN; i++)
- efi_name[i] = stub_name[i];
-
- /*
- * Clean up any entries with the same name
- */
-
- list_for_each_entry(entry, &efivars->list, list) {
- get_var_data_locked(efivars, &entry->var);
-
- if (efi_guidcmp(entry->var.VendorGuid, vendor))
- continue;
- if (utf16_strncmp(entry->var.VariableName, efi_name,
- utf16_strlen(efi_name)))
- continue;
- /* Needs to be a prefix */
- if (entry->var.VariableName[utf16_strlen(efi_name)] == 0)
- continue;
-
- /* found */
- found = entry;
- efivars->ops->set_variable(entry->var.VariableName,
- &entry->var.VendorGuid,
- PSTORE_EFI_ATTRIBUTES,
- 0, NULL);
- }
-
- if (found)
- list_del(&found->list);
+ sprintf(name, "dump-type%u-%u-%lu", type, part,
+ get_seconds());
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name[i] = name[i];
@@ -768,9 +736,6 @@ static int efi_pstore_write(enum pstore_type_id type,
spin_unlock(&efivars->lock);
- if (found)
- efivar_unregister(found);
-
if (size)
ret = efivar_create_sysfs_entry(efivars,
utf16_strsize(efi_name,
-- 1.7.1
Acked-by: Mike Waychison <[email protected]>
On Wed, Oct 24, 2012 at 6:54 PM, Seiji Aguchi <[email protected]> wrote:
> [Issue]
>
> Currently, efi_pstore driver simply overwrites existing panic messages in NVRAM.
> So, in the following scenario, we will lose 1st panic messages.
>
> 1. kernel panics.
> 2. efi_pstore is kicked and writes panic messages to NVRAM.
> 3. system reboots.
> 4. kernel panics again before a user checks the 1st panic messages in NVRAM.
>
> [Solution]
>
> A reasonable solution to fix the issue is just holding multiple logs without erasing
> existing entries.
> This patch removes a logic erasing existing entries in a write callback
> because the logic is not needed in the write callback to support holding multiple logs.
>
> Signed-off-by: Seiji Aguchi <[email protected]>
> ---
> drivers/firmware/efivars.c | 39 ++-------------------------------------
> 1 files changed, 2 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index bee14cc..fbe9202 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -701,18 +701,13 @@ static int efi_pstore_write(enum pstore_type_id type,
> unsigned int part, size_t size, struct pstore_info *psi)
> {
> char name[DUMP_NAME_LEN];
> - char stub_name[DUMP_NAME_LEN];
> efi_char16_t efi_name[DUMP_NAME_LEN];
> efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
> struct efivars *efivars = psi->data;
> - struct efivar_entry *entry, *found = NULL;
> int i, ret = 0;
> u64 storage_space, remaining_space, max_variable_size;
> efi_status_t status = EFI_NOT_FOUND;
>
> - sprintf(stub_name, "dump-type%u-%u-", type, part);
> - sprintf(name, "%s%lu", stub_name, get_seconds());
> -
> spin_lock(&efivars->lock);
>
> /*
> @@ -730,35 +725,8 @@ static int efi_pstore_write(enum pstore_type_id type,
> return -ENOSPC;
> }
>
> - for (i = 0; i < DUMP_NAME_LEN; i++)
> - efi_name[i] = stub_name[i];
> -
> - /*
> - * Clean up any entries with the same name
> - */
> -
> - list_for_each_entry(entry, &efivars->list, list) {
> - get_var_data_locked(efivars, &entry->var);
> -
> - if (efi_guidcmp(entry->var.VendorGuid, vendor))
> - continue;
> - if (utf16_strncmp(entry->var.VariableName, efi_name,
> - utf16_strlen(efi_name)))
> - continue;
> - /* Needs to be a prefix */
> - if (entry->var.VariableName[utf16_strlen(efi_name)] == 0)
> - continue;
> -
> - /* found */
> - found = entry;
> - efivars->ops->set_variable(entry->var.VariableName,
> - &entry->var.VendorGuid,
> - PSTORE_EFI_ATTRIBUTES,
> - 0, NULL);
> - }
> -
> - if (found)
> - list_del(&found->list);
> + sprintf(name, "dump-type%u-%u-%lu", type, part,
> + get_seconds());
Could you please also update Documentation/ABI/testing/pstore ? It
seems it was never updated with the "dump*" file prefix.
>
> for (i = 0; i < DUMP_NAME_LEN; i++)
> efi_name[i] = name[i];
> @@ -768,9 +736,6 @@ static int efi_pstore_write(enum pstore_type_id type,
>
> spin_unlock(&efivars->lock);
>
> - if (found)
> - efivar_unregister(found);
> -
> if (size)
> ret = efivar_create_sysfs_entry(efivars,
> utf16_strsize(efi_name,
> -- 1.7.1
On Wed, Oct 24, 2012 at 6:54 PM, Seiji Aguchi <[email protected]> wrote:
> [Issue]
>
> Currently, efi_pstore driver simply overwrites existing panic messages in NVRAM.
> So, in the following scenario, we will lose 1st panic messages.
>
> 1. kernel panics.
> 2. efi_pstore is kicked and writes panic messages to NVRAM.
> 3. system reboots.
> 4. kernel panics again before a user checks the 1st panic messages in NVRAM.
>
> [Solution]
>
> A reasonable solution to fix the issue is just holding multiple logs without erasing
> existing entries.
> This patch removes a logic erasing existing entries in a write callback
> because the logic is not needed in the write callback to support holding multiple logs.
>
> Signed-off-by: Seiji Aguchi <[email protected]>
> ---
> drivers/firmware/efivars.c | 39 ++-------------------------------------
> 1 files changed, 2 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index bee14cc..fbe9202 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -701,18 +701,13 @@ static int efi_pstore_write(enum pstore_type_id type,
> unsigned int part, size_t size, struct pstore_info *psi)
> {
> char name[DUMP_NAME_LEN];
> - char stub_name[DUMP_NAME_LEN];
> efi_char16_t efi_name[DUMP_NAME_LEN];
> efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
> struct efivars *efivars = psi->data;
> - struct efivar_entry *entry, *found = NULL;
> int i, ret = 0;
> u64 storage_space, remaining_space, max_variable_size;
> efi_status_t status = EFI_NOT_FOUND;
>
> - sprintf(stub_name, "dump-type%u-%u-", type, part);
> - sprintf(name, "%s%lu", stub_name, get_seconds());
> -
> spin_lock(&efivars->lock);
>
> /*
> @@ -730,35 +725,8 @@ static int efi_pstore_write(enum pstore_type_id type,
> return -ENOSPC;
> }
>
> - for (i = 0; i < DUMP_NAME_LEN; i++)
> - efi_name[i] = stub_name[i];
> -
> - /*
> - * Clean up any entries with the same name
> - */
> -
> - list_for_each_entry(entry, &efivars->list, list) {
> - get_var_data_locked(efivars, &entry->var);
> -
> - if (efi_guidcmp(entry->var.VendorGuid, vendor))
> - continue;
> - if (utf16_strncmp(entry->var.VariableName, efi_name,
> - utf16_strlen(efi_name)))
> - continue;
> - /* Needs to be a prefix */
> - if (entry->var.VariableName[utf16_strlen(efi_name)] == 0)
> - continue;
> -
> - /* found */
> - found = entry;
> - efivars->ops->set_variable(entry->var.VariableName,
> - &entry->var.VendorGuid,
> - PSTORE_EFI_ATTRIBUTES,
> - 0, NULL);
> - }
> -
> - if (found)
> - list_del(&found->list);
> + sprintf(name, "dump-type%u-%u-%lu", type, part,
> + get_seconds());
Actually, nothing seems to be ensuring that the value used here ends
up being the same as the ctime :\ Consider what happens if the
get_seconds() call here happens at second N, and the i_ctime is
collected at time N+1. The object wouldn't be able to be deleted after
patch 4/5 is applied. How can we better ensure that the i_ctime is
the same value as serialized into the efi var name here?
>
> for (i = 0; i < DUMP_NAME_LEN; i++)
> efi_name[i] = name[i];
> @@ -768,9 +736,6 @@ static int efi_pstore_write(enum pstore_type_id type,
>
> spin_unlock(&efivars->lock);
>
> - if (found)
> - efivar_unregister(found);
> -
> if (size)
> ret = efivar_create_sysfs_entry(efivars,
> utf16_strsize(efi_name,
> -- 1.7.1
> > - list_del(&found->list);
> > + sprintf(name, "dump-type%u-%u-%lu", type, part,
> > + get_seconds());
>
> Actually, nothing seems to be ensuring that the value used here ends up being the same as the ctime :\ Consider what happens if the
> get_seconds() call here happens at second N, and the i_ctime is collected at time N+1. The object wouldn't be able to be deleted after
> patch 4/5 is applied. How can we better ensure that the i_ctime is the same value as serialized into the efi var name here?
i_ctime of pstore file system is different from normal file systems.
As I said in patch v1, i_ctime of pstore means the date that the record was "originally" stored.
I_ctime is always set to N.
Here is the scenario.
1. writing time
- get_seconds() call here happens at second N
- "N" is stored to a variable name
2. System reboots...
3. System boots up...
4. reading time
- pstore calls a read callback, efi_pstore_read()
- efi_pstore_read extracts ctime, "N", from the virable name
- efi_pstore passes "N" to pstore file system
- pstore create a file, /dev/pstore/dmesg* and set "N" to i_ctime
Pstore file system doesn't set i_ctime by itself but set ctime passed by platform drivers.
Seiji
On Thu, Oct 25, 2012 at 1:03 PM, Seiji Aguchi <[email protected]> wrote:
>> > - list_del(&found->list);
>> > + sprintf(name, "dump-type%u-%u-%lu", type, part,
>> > + get_seconds());
>>
>> Actually, nothing seems to be ensuring that the value used here ends up being the same as the ctime :\ Consider what happens if the
>> get_seconds() call here happens at second N, and the i_ctime is collected at time N+1. The object wouldn't be able to be deleted after
>> patch 4/5 is applied. How can we better ensure that the i_ctime is the same value as serialized into the efi var name here?
>
> i_ctime of pstore file system is different from normal file systems.
> As I said in patch v1, i_ctime of pstore means the date that the record was "originally" stored.
>
> I_ctime is always set to N.
> Here is the scenario.
>
> 1. writing time
> - get_seconds() call here happens at second N
> - "N" is stored to a variable name
>
> 2. System reboots...
> 3. System boots up...
>
> 4. reading time
> - pstore calls a read callback, efi_pstore_read()
> - efi_pstore_read extracts ctime, "N", from the virable name
Ah, I accidentally glossed over this bit. Thanks for correcting me :)
> - efi_pstore passes "N" to pstore file system
> - pstore create a file, /dev/pstore/dmesg* and set "N" to i_ctime
>
> Pstore file system doesn't set i_ctime by itself but set ctime passed by platform drivers.
>
So doesn't this break erasing of existing dumps in pstore-efivars?
Unless efi_pstore_read still understands the older variable name
formats, users will be stranded with dumps consuming space in efivars
that aren't exported via pstore anymore.
> So doesn't this break erasing of existing dumps in pstore-efivars?
> Unless efi_pstore_read still understands the older variable name formats, users will be stranded with dumps consuming space in
> efivars that aren't exported via pstore anymore.
Patch 2/5, 3/5 and 4/5 don't change an existing format of a variable name and don't break anything.
The existing format consists of type, id and ctime. And it is not moditied by these patches.
Here is a variable name (and guid) of efi_pstore in curret linus-tree.
ls /sys/firmware/efi/vars/
dump-type0-1-1351113059-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
dump-type0-2-1351113059-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
dump-type0-3-1351113059-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
Variable Name: dump-type0-1-1351113059
GUID: cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
On the other hand, patch 5/5 changes the format by adding sequence counter.
But efi_pstore_read is modied to work correctly in it.
dump-type0-1-1-1351113059-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
Variable Name: dump-type0-1-1-1351113059
GUID: cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
If I need to elaborate more, please feel free to ask me:)
I'm not sure if I understand your question completely.
Seiji
On Thu, Oct 25, 2012 at 1:51 PM, Seiji Aguchi <[email protected]> wrote:
>> So doesn't this break erasing of existing dumps in pstore-efivars?
>> Unless efi_pstore_read still understands the older variable name formats, users will be stranded with dumps consuming space in
>> efivars that aren't exported via pstore anymore.
>
> Patch 2/5, 3/5 and 4/5 don't change an existing format of a variable name and don't break anything.
> The existing format consists of type, id and ctime. And it is not moditied by these patches.
> Here is a variable name (and guid) of efi_pstore in curret linus-tree.
>
> ls /sys/firmware/efi/vars/
> dump-type0-1-1351113059-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
> dump-type0-2-1351113059-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
> dump-type0-3-1351113059-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
>
> Variable Name: dump-type0-1-1351113059
> GUID: cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
Okay, this alleviates most of my concerns for ctime.
>
> On the other hand, patch 5/5 changes the format by adding sequence counter.
> But efi_pstore_read is modied to work correctly in it.
>
> dump-type0-1-1-1351113059-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
>
> Variable Name: dump-type0-1-1-1351113059
> GUID: cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
>
> If I need to elaborate more, please feel free to ask me:)
> I'm not sure if I understand your question completely.
In this case, I think efi_pstore_read in patch 5/5 should probably
fall-back to trying to do a sscanf(...) == 3 if the sscanf(...) == 4
test fails so that dumps for older dumps from previous kernels are
still visible to users, no? They can perhaps get a default count of
0? efi_pstore_erase would have to be updated to understand this as
well.
> > On the other hand, patch 5/5 changes the format by adding sequence counter.
> > But efi_pstore_read is modied to work correctly in it.
> >
> > dump-type0-1-1-1351113059-cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
> >
> > Variable Name: dump-type0-1-1-1351113059
> > GUID: cfc8fc79-be2e-4ddc-97f0-9f98bfe298a0
> >
> > If I need to elaborate more, please feel free to ask me:) I'm not sure
> > if I understand your question completely.
>
> In this case, I think efi_pstore_read in patch 5/5 should probably fall-back to trying to do a sscanf(...) == 3 if the sscanf(...) == 4 test fails
> so that dumps for older dumps from previous kernels are still visible to users, no? They can perhaps get a default count of 0?
> efi_pstore_erase would have to be updated to understand this as well.
OK. I understand your concern.
I will update my patch to work with an old format too.
In addition to efi_pstore_read(), efi_pstore_erase should be modified.
Anyway, I will post a patch v3 later.
Thank you for reviewing.
Seiji