| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 5 Jul 2012 09:32:01 -0400
From: Don Zickus <dzickus@...hat.com>
To: Seiji Aguchi <seiji.aguchi@....com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Luck, Tony (tony.luck@...el.com)" <tony.luck@...el.com>,
"mikew@...gle.com" <mikew@...gle.com>,
"Matthew Garrett (mjg@...hat.com)" <mjg@...hat.com>,
"dle-develop@...ts.sourceforge.net"
<dle-develop@...ts.sourceforge.net>,
Satoru Moriya <satoru.moriya@....com>
Subject: Re: [RFC][PATCH 2/2] write callback: Check if existing entry is
erasable
On Tue, Jul 03, 2012 at 11:35:47PM +0000, Seiji Aguchi wrote:
> This patch introduces a following rule checking if an existing entry in NVRAM are erasable to avoid missing
> a critical message ,such as panic, before an user check it.
I am not a big fan of this policy.
>
> - In case where an existing entry is panic or emergency
> - It is not erasable because if panic/emergency event is lost, we have no way to detect
> the root cause. We shouldn't overwrite them for any reason.
Ok.
>
> - In case where an existing entry is oops/shutdown/halt/poweroff
> - It is erasable if an error ,panic, emergency or oops, happen in new event.
> - Oops is erasable because system may still be running and its message may be saved
> into /var/log/messages.
Then why save it to begin with?
> - Also, normal event ,shutdown/halt/poweroff, is erasable because error message is
> more important than normal message.
Then why happens if you are using these messages to debug a problem on
your system? It would seem one could be misled unless you set a flag
stating that a log was overwritten somewhere.
>
> - In case where an existing entry is unknown event
> - It is erasable because any event supported by pstore outweighs unsupported events.
Again, how does an 'unknown' event get recorded? Shouldn't all events be
known?
I would rather see no records overwritten and just make sure there is
enough space for a dozen or so records to buffer multiple panics before
userspace can run.
Implementing policy like this in the kernel seems like it would be a
constant battle between everyone's view point of what is important and not
important.
I would rather take the viewpoint, if it is important to log it in a space
limited NVRAM, then it is important enough not to overwrite until
userspace explicitly asks it to be deleted. Otherwise why log it, if it
is not important?
Cheers,
Don
>
> Signed-off-by: Seiji Aguchi <seiji.aguchi@....com>
> ---
> drivers/firmware/efivars.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
> fs/pstore/platform.c | 4 +-
> include/linux/pstore.h | 5 ++++
> 3 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index 4929254..54d9bc6 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -685,6 +685,45 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
> return 0;
> }
>
> +
> +static bool can_erase_entry(struct efivar_entry *entry, enum kmsg_dump_reason
> + new_reason)
> +{
> + int prev_reason = 0;
> + const char *prev_why;
> + bool is_erasable = 0;
> +
> + /* Get a reason of previous message */
> + while (1) {
> + prev_why = pstore_get_reason_str(prev_reason);
> + if (!strncmp(entry->var.Data, prev_why, strlen(prev_why)))
> + break;
> + prev_reason++;
> + }
> +
> + /* check if exisiting message is erasable */
> +
> + switch (prev_reason) {
> + case KMSG_DUMP_PANIC:
> + case KMSG_DUMP_EMERG:
> + /* Never erase panic or emergency message */
> + break;
> + case KMSG_DUMP_OOPS:
> + case KMSG_DUMP_RESTART:
> + case KMSG_DUMP_HALT:
> + case KMSG_DUMP_POWEROFF:
> + /* Can erase if new one is error message */
> + if (new_reason <= KMSG_DUMP_EMERG)
> + is_erasable = 1;
> + break;
> + default:
> + /* Can erase unknown message */
> + is_erasable = 1;
> + }
> +
> + return is_erasable;
> +}
> +
> static int efi_pstore_write(enum pstore_type_id type,
> enum kmsg_dump_reason reason, u64 *id,
> unsigned int part, size_t size, struct pstore_info *psi) @@ -706,7 +745,7 @@ static int efi_pstore_write(enum pstore_type_id type,
> efi_name[i] = stub_name[i];
>
> /*
> - * Clean up any entries with the same name
> + * Search for erasable entry
> */
>
> list_for_each_entry(entry, &efivars->list, list) { @@ -721,6 +760,13 @@ static int efi_pstore_write(enum pstore_type_id type,
> if (entry->var.VariableName[utf16_strlen(efi_name)] == 0)
> continue;
>
> + if (!can_erase_entry(entry, reason)) {
> + /* return without writing new entry */
> + spin_unlock(&efivars->lock);
> + *id = part;
> + return ret;
> + }
> +
> /* found */
> found = entry;
> efivars->ops->set_variable(entry->var.VariableName,
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 03ce7a9..32715eb 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -68,7 +68,7 @@ void pstore_set_kmsg_bytes(int bytes)
> /* Tag each group of saved records with a sequence number */
> static int oopscount;
>
> -static const char *get_reason_str(enum kmsg_dump_reason reason)
> +const char *pstore_get_reason_str(enum kmsg_dump_reason reason)
> {
> switch (reason) {
> case KMSG_DUMP_PANIC:
> @@ -104,7 +104,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> int is_locked = 0;
> int ret;
>
> - why = get_reason_str(reason);
> + why = pstore_get_reason_str(reason);
>
> if (in_nmi()) {
> is_locked = spin_trylock(&psinfo->buf_lock); diff --git a/include/linux/pstore.h b/include/linux/pstore.h index e1461e1..e9347e9 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -54,12 +54,17 @@ struct pstore_info {
>
> #ifdef CONFIG_PSTORE
> extern int pstore_register(struct pstore_info *);
> +extern const char *pstore_get_reason_str(enum kmsg_dump_reason reason);
> #else
> static inline int
> pstore_register(struct pstore_info *psi) {
> return -ENODEV;
> }
> +static const char *pstore_get_reason_str(enum kmsg_dump_reason reason)
> +{
> + return NULL;
> +}
> #endif
>
> #endif /*_LINUX_PSTORE_H*/
> --
> 1.7.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists