| 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
| ||
|
Message-ID: <202005041222.4A870DFEC@keescook>
Date: Mon, 4 May 2020 12:29:18 -0700
From: Kees Cook <keescook@...omium.org>
To: Pavel Tatashin <pasha.tatashin@...een.com>
Cc: jmorris@...ei.org, sashal@...nel.org, linux-kernel@...r.kernel.org,
pmladek@...e.com, sergey.senozhatsky@...il.com,
rostedt@...dmis.org, anton@...msg.org, ccross@...roid.com,
tony.luck@...el.com, robh+dt@...nel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v1 2/3] pstore/ram: allow to dump kmesg during regular
reboot
On Sat, May 02, 2020 at 10:35:54AM -0400, Pavel Tatashin wrote:
> Currently, ramoops is capable to collect dmesg buffer only during
> panic and oops events. However, it is desirable to optionally allow
> collecting dmesg buffers during other events as well: reboot, kexec,
> emergency reboot etc.
>
> While, a similar functionality is provided by pstore console it is not the
> same. Often, console message level is reduced in production due to baud
> rate limitation of serial consoles. Having a noisy console reduces the
> boot performance.
>
> Thus, if the shutdown dmesg buffer is needed to study the shutdown
> performance, it is currently not possible to do so with console as by
> increasing the console output the shutdown time substantially increases
> as well.
>
> Signed-off-by: Pavel Tatashin <pasha.tatashin@...een.com>
> ---
> fs/pstore/platform.c | 6 ++++--
> fs/pstore/ram.c | 38 +++++++++++++++++++++++++++-----------
> include/linux/pstore.h | 1 +
> include/linux/pstore_ram.h | 1 +
> 4 files changed, 33 insertions(+), 13 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 408277ee3cdb..d0393799fe6c 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -476,8 +476,10 @@ static struct kmsg_dumper pstore_dumper = {
> /*
> * Register with kmsg_dump to save last part of console log on panic.
> */
> -static void pstore_register_kmsg(void)
> +static void pstore_register_kmsg(int dmesg_all)
> {
> + if (dmesg_all)
> + pstore_dumper.max_reason = KMSG_DUMP_MAX;
So, I'd like to avoid any new arguments in the API and instead add a new
field to struct pstore_info, which will be valid when PSTORE_FLAGS_DMESG
is set, and the max kdump reason can be set there by the pstore backends.
> kmsg_dump_register(&pstore_dumper);
> }
>
> @@ -603,7 +605,7 @@ int pstore_register(struct pstore_info *psi)
> pstore_get_records(0);
>
> if (psi->flags & PSTORE_FLAGS_DMESG)
> - pstore_register_kmsg();
> + pstore_register_kmsg(psi->flags & PSTORE_FLAGS_DMESG_ALL);
> if (psi->flags & PSTORE_FLAGS_CONSOLE)
> pstore_register_console();
> if (psi->flags & PSTORE_FLAGS_FTRACE)
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 795622190c01..9d2d1b299225 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -62,6 +62,12 @@ module_param(dump_oops, int, 0600);
> MODULE_PARM_DESC(dump_oops,
> "set to 1 to dump oopses, 0 to only dump panics (default 1)");
>
> +static int dump_all;
> +module_param(dump_all, int, 0600);
> +MODULE_PARM_DESC(dump_all,
> + "set to 1 to record all kernel kmesg dump events (default 0) "
> + "otherwise only panics and oops are recorded");
And instead of using a "dump_all" here, let's add a new field that is
the max reason. When both "max_reason" and "dump_oops" are defined,
"max_reason" should win.
> +
> static int ramoops_ecc;
> module_param_named(ecc, ramoops_ecc, int, 0600);
> MODULE_PARM_DESC(ramoops_ecc,
> @@ -82,6 +88,7 @@ struct ramoops_context {
> size_t ftrace_size;
> size_t pmsg_size;
> int dump_oops;
> + int dump_all;
> u32 flags;
> struct persistent_ram_ecc_info ecc_info;
> unsigned int max_dump_cnt;
> @@ -381,17 +388,19 @@ static int notrace ramoops_pstore_write(struct pstore_record *record)
> if (record->type != PSTORE_TYPE_DMESG)
> return -EINVAL;
>
> - /*
> - * Out of the various dmesg dump types, ramoops is currently designed
> - * to only store crash logs, rather than storing general kernel logs.
> - */
> - if (record->reason != KMSG_DUMP_OOPS &&
> - record->reason != KMSG_DUMP_PANIC)
> - return -EINVAL;
> + if (!cxt->dump_all) {
> + /*
> + * By default only store crash logs, rather than storing general
> + * kernel logs.
> + */
> + if (record->reason != KMSG_DUMP_OOPS &&
> + record->reason != KMSG_DUMP_PANIC)
> + return -EINVAL;
Then all these tests can be collapsed much more cleanly, etc.
>
> - /* Skip Oopes when configured to do so. */
> - if (record->reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
> - return -EINVAL;
> + /* Skip Oopes when configured to do so. */
> + if (record->reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
> + return -EINVAL;
> + }
>
> /*
> * Explicitly only take the first part of any new crash.
> @@ -688,6 +697,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> pdata->mem_address = res->start;
> pdata->mem_type = of_property_read_bool(of_node, "unbuffered");
> pdata->dump_oops = !of_property_read_bool(of_node, "no-dump-oops");
> + pdata->dump_all = of_property_read_bool(of_node, "dump-all");
>
> #define parse_size(name, field) { \
> ret = ramoops_parse_dt_size(pdev, name, &value); \
> @@ -786,6 +796,7 @@ static int ramoops_probe(struct platform_device *pdev)
> cxt->ftrace_size = pdata->ftrace_size;
> cxt->pmsg_size = pdata->pmsg_size;
> cxt->dump_oops = pdata->dump_oops;
> + cxt->dump_all = pdata->dump_all;
> cxt->flags = pdata->flags;
> cxt->ecc_info = pdata->ecc_info;
>
> @@ -828,8 +839,11 @@ static int ramoops_probe(struct platform_device *pdev)
> * the single region size is how to check.
> */
> cxt->pstore.flags = 0;
> - if (cxt->max_dump_cnt)
> + if (cxt->max_dump_cnt) {
> cxt->pstore.flags |= PSTORE_FLAGS_DMESG;
> + if (cxt->dump_all)
> + cxt->pstore.flags |= PSTORE_FLAGS_DMESG_ALL;
> + }
> if (cxt->console_size)
> cxt->pstore.flags |= PSTORE_FLAGS_CONSOLE;
> if (cxt->max_ftrace_cnt)
> @@ -866,6 +880,7 @@ static int ramoops_probe(struct platform_device *pdev)
> mem_address = pdata->mem_address;
> record_size = pdata->record_size;
> dump_oops = pdata->dump_oops;
> + dump_all = pdata->dump_all;
> ramoops_console_size = pdata->console_size;
> ramoops_pmsg_size = pdata->pmsg_size;
> ramoops_ftrace_size = pdata->ftrace_size;
> @@ -949,6 +964,7 @@ static void __init ramoops_register_dummy(void)
> pdata.ftrace_size = ramoops_ftrace_size;
> pdata.pmsg_size = ramoops_pmsg_size;
> pdata.dump_oops = dump_oops;
> + pdata.dump_all = dump_all;
> pdata.flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
>
> /*
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index e779441e6d26..32092c2d7224 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -195,6 +195,7 @@ struct pstore_info {
> #define PSTORE_FLAGS_CONSOLE BIT(1)
> #define PSTORE_FLAGS_FTRACE BIT(2)
> #define PSTORE_FLAGS_PMSG BIT(3)
> +#define PSTORE_FLAGS_DMESG_ALL BIT(4)
>
> extern int pstore_register(struct pstore_info *);
> extern void pstore_unregister(struct pstore_info *);
> diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> index 9cb9b9067298..f23c29cbd205 100644
> --- a/include/linux/pstore_ram.h
> +++ b/include/linux/pstore_ram.h
> @@ -134,6 +134,7 @@ struct ramoops_platform_data {
> unsigned long ftrace_size;
> unsigned long pmsg_size;
> int dump_oops;
> + int dump_all;
> u32 flags;
> struct persistent_ram_ecc_info ecc_info;
> };
> --
> 2.25.1
>
Thanks!
--
Kees Cook
Powered by blists - more mailing lists