[PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[Kees Cook]

Hi!

This is my stab at rearranging a few things based on Pavel's series. Most
things remain the same; I just tweaked how defaults are arranged and
detected and expanded the wording in a few places. Pavel, how does this
v3 look to you?

Pavel's original cover letter:

pstore /mnt/console-ramoops-0 outputs only messages below the console
loglevel, and our console loglevel is set to 3 due to slowness of
serial console. Which means only errors and worse types of messages
are recorded. There is no way to have different log levels for
different consoles.

This patch series adds a new option to ramoops: max_reason that enables
it to collect kmdesg dumps for other reasons beside oops and panics.

How to quickly test:

virtme-run --mods=auto --kdir --mods=auto --kdir . \
	-a memmap=1G$8G -a ramoops.mem_address=0x200000000 \
	-a ramoops.mem_size=0x100000 -a ramoops.record_size=32768 \
	-a ramoops.max_reason=5 -a quiet --qemu-opts -m 8G
..
# reboot -f

After VM is back:

# mount -t pstore pstore /mnt
# head /mnt/dmesg-ramoops-0
Restart#1 Part1
...


Changelog:

v3:
 - expanded several comments and commit logs
 - move max_reason member earlier in the structure
 - refactored DT parsing to allow setting defaults
 - changed how deprecated dump_oops fields are detected and parsed
 - cleaned up some module param permissions
v2: https://lore.kernel.org/lkml/20200505154510.93506-1-pasha.tatashin@soleen.com
v1: https://lore.kernel.org/lkml/20200502143555.543636-1-pasha.tatashin@soleen.com


Thanks!

-Kees


Kees Cook (2):
  pstore/ram: Refactor DT size parsing
  pstore/ram: Adjust module param permissions to reflect reality

Pavel Tatashin (4):
  printk: honor the max_reason field in kmsg_dumper
  pstore/platform: Pass max_reason to kmesg dump
  pstore/ram: Introduce max_reason and convert dump_oops
  ramoops: Add max_reason optional field to ramoops DT node

 Documentation/admin-guide/ramoops.rst         | 14 +++-
 .../bindings/reserved-memory/ramoops.txt      | 13 ++-
 drivers/platform/chrome/chromeos_pstore.c     |  2 +-
 fs/pstore/platform.c                          |  4 +-
 fs/pstore/ram.c                               | 83 ++++++++++++-------
 include/linux/kmsg_dump.h                     |  1 +
 include/linux/pstore.h                        |  7 ++
 include/linux/pstore_ram.h                    |  2 +-
 kernel/printk/printk.c                        | 15 +++-
 9 files changed, 97 insertions(+), 44 deletions(-)

-- 
2.20.1

[PATCH v3 1/6] printk: honor the max_reason field in kmsg_dumper

[Kees Cook]

From: Pavel Tatashin <pasha.tatashin@soleen.com>

kmsg_dump() allows to dump kmesg buffer for various system events: oops,
panic, reboot, etc. It provides an interface to register a callback call
for clients, and in that callback interface there is a field "max_reason"
which gets ignored unless always_kmsg_dump is passed as kernel parameter.

Allow clients to decide max_reason, and keep the current behavior when
max_reason is not set.

Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
Link: https://lore.kernel.org/r/20200505154510.93506-2-pasha.tatashin@soleen.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/kmsg_dump.h |  1 +
 kernel/printk/printk.c    | 15 +++++++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index 2e7a1e032c71..cfc042066be7 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -28,6 +28,7 @@ enum kmsg_dump_reason {
 	KMSG_DUMP_RESTART,
 	KMSG_DUMP_HALT,
 	KMSG_DUMP_POWEROFF,
+	KMSG_DUMP_MAX
 };
 
 /**
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9a9b6156270b..1aab69a8a2bf 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3157,12 +3157,19 @@ void kmsg_dump(enum kmsg_dump_reason reason)
 	struct kmsg_dumper *dumper;
 	unsigned long flags;
 
-	if ((reason > KMSG_DUMP_OOPS) && !always_kmsg_dump)
-		return;
-
 	rcu_read_lock();
 	list_for_each_entry_rcu(dumper, &dump_list, list) {
-		if (dumper->max_reason && reason > dumper->max_reason)
+		enum kmsg_dump_reason cur_reason = dumper->max_reason;
+
+		/*
+		 * If client has not provided a specific max_reason, default
+		 * to KMSG_DUMP_OOPS, unless always_kmsg_dump was set.
+		 */
+		if (cur_reason == KMSG_DUMP_UNDEF) {
+			cur_reason = always_kmsg_dump ? KMSG_DUMP_MAX :
+							KMSG_DUMP_OOPS;
+		}
+		if (reason > cur_reason)
 			continue;
 
 		/* initialize iterator with data about the stored records */
-- 
2.20.1

[PATCH v3 2/6] pstore/platform: Pass max_reason to kmesg dump

[Kees Cook]

From: Pavel Tatashin <pasha.tatashin@soleen.com>

Add a new member to struct pstore_info for passing information about
kmesg dump maximum reason. This allows a finer control of what kmesg
dumps are sent to pstore storage backends.

Those backends that do not explicitly set this field (keeping it equal to
0), get the default behavior: store only Oopses and Panics, or everything
if the printk.always_kmsg_dump boot param is set.

Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
Link: https://lore.kernel.org/r/20200505154510.93506-3-pasha.tatashin@soleen.com
Co-developed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c   | 4 +++-
 include/linux/pstore.h | 7 +++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 6fb526187953..3a3906173534 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -606,8 +606,10 @@ int pstore_register(struct pstore_info *psi)
 
 	pstore_get_records(0);
 
-	if (psi->flags & PSTORE_FLAGS_DMESG)
+	if (psi->flags & PSTORE_FLAGS_DMESG) {
+		pstore_dumper.max_reason = psinfo->max_reason;
 		pstore_register_kmsg();
+	}
 	if (psi->flags & PSTORE_FLAGS_CONSOLE)
 		pstore_register_console();
 	if (psi->flags & PSTORE_FLAGS_FTRACE)
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index e779441e6d26..e78d5c29aa8b 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -96,6 +96,12 @@ struct pstore_record {
  *
  * @read_mutex:	serializes @open, @read, @close, and @erase callbacks
  * @flags:	bitfield of frontends the backend can accept writes for
+ * @max_reason:	Used when PSTORE_FLAGS_DMESG is set. Contains the
+ *		kmsg_dump_reason enum value. KMSG_DUMP_UNDEF means
+ *		"use existing kmsg_dump() filtering, based on the
+ *		printk.always_kmsg_dump boot param" (which is either
+ *		KMSG_DUMP_OOPS when false, or KMSG_DUMP_MAX when
+ *		tree); see printk.always_kmsg_dump for more details.
  * @data:	backend-private pointer passed back during callbacks
  *
  * Callbacks:
@@ -179,6 +185,7 @@ struct pstore_info {
 	struct mutex	read_mutex;
 
 	int		flags;
+	int		max_reason;
 	void		*data;
 
 	int		(*open)(struct pstore_info *psi);
-- 
2.20.1

[PATCH v3 3/6] pstore/ram: Refactor DT size parsing

[Kees Cook]

Refactor device tree size parsing routines to be able to pass a non-zero
default value for providing a configurable default for the coming
"max_reason" field.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/ram.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 795622190c01..c2f76b650f91 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -645,18 +645,23 @@ static int ramoops_init_prz(const char *name,
 }
 
 static int ramoops_parse_dt_size(struct platform_device *pdev,
-				 const char *propname, u32 *value)
+				 const char *propname,
+				 u32 default_value, u32 *value)
 {
 	u32 val32 = 0;
 	int ret;
 
 	ret = of_property_read_u32(pdev->dev.of_node, propname, &val32);
-	if (ret < 0 && ret != -EINVAL) {
+	if (ret == -EINVAL) {
+		/* field is missing, use default value. */
+		val32 = default_value;
+	} else if (ret < 0) {
 		dev_err(&pdev->dev, "failed to parse property %s: %d\n",
 			propname, ret);
 		return ret;
 	}
 
+	/* Sanity check our results. */
 	if (val32 > INT_MAX) {
 		dev_err(&pdev->dev, "%s %u > INT_MAX\n", propname, val32);
 		return -EOVERFLOW;
@@ -689,19 +694,20 @@ static int ramoops_parse_dt(struct platform_device *pdev,
 	pdata->mem_type = of_property_read_bool(of_node, "unbuffered");
 	pdata->dump_oops = !of_property_read_bool(of_node, "no-dump-oops");
 
-#define parse_size(name, field) {					\
-		ret = ramoops_parse_dt_size(pdev, name, &value);	\
+#define parse_size(name, field, default_value) {			\
+		ret = ramoops_parse_dt_size(pdev, name, default_value,	\
+					    &value);			\
 		if (ret < 0)						\
 			return ret;					\
 		field = value;						\
 	}
 
-	parse_size("record-size", pdata->record_size);
-	parse_size("console-size", pdata->console_size);
-	parse_size("ftrace-size", pdata->ftrace_size);
-	parse_size("pmsg-size", pdata->pmsg_size);
-	parse_size("ecc-size", pdata->ecc_info.ecc_size);
-	parse_size("flags", pdata->flags);
+	parse_size("record-size", pdata->record_size, 0);
+	parse_size("console-size", pdata->console_size, 0);
+	parse_size("ftrace-size", pdata->ftrace_size, 0);
+	parse_size("pmsg-size", pdata->pmsg_size, 0);
+	parse_size("ecc-size", pdata->ecc_info.ecc_size, 0);
+	parse_size("flags", pdata->flags, 0);
 
 #undef parse_size
 
-- 
2.20.1

[PATCH v3 4/6] pstore/ram: Introduce max_reason and convert dump_oops

[Kees Cook]

From: Pavel Tatashin <pasha.tatashin@soleen.com>

Now that pstore_register() can correctly pass max_reason to the kmesg
dump facility, introduce a new "max_reason" module parameter and
"max-reason" Device Tree field.

The "dump_oops" module parameter and "dump-oops" Device
Tree field are now considered deprecated, but are now automatically
converted to their corresponding max_reason values when present, though
the new max_reason setting has precedence.

For struct ramoops_platform_data, the "dump_oops" member is entirely
replaced by a new "max_reason" member, with the only existing user
updated in place.

Additionally remove the "reason" filter logic from ramoops_pstore_write(),
as that is not specifically needed anymore, though technically
this is a change in behavior for any ramoops users also setting the
printk.always_kmsg_dump boot param, which will cause ramoops to behave as
if max_reason was set to KMSG_DUMP_MAX.

Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
Link: https://lore.kernel.org/r/20200505154510.93506-4-pasha.tatashin@soleen.com
Link: https://lore.kernel.org/r/20200505154510.93506-5-pasha.tatashin@soleen.com
Co-developed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 Documentation/admin-guide/ramoops.rst     | 14 +++++--
 drivers/platform/chrome/chromeos_pstore.c |  2 +-
 fs/pstore/ram.c                           | 51 +++++++++++++++--------
 include/linux/pstore_ram.h                |  2 +-
 4 files changed, 45 insertions(+), 24 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
index 6dbcc5481000..a60a96218ba9 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -32,11 +32,17 @@ memory to be mapped strongly ordered, and atomic operations on strongly ordered
 memory are implementation defined, and won't work on many ARMs such as omaps.
 
 The memory area is divided into ``record_size`` chunks (also rounded down to
-power of two) and each oops/panic writes a ``record_size`` chunk of
+power of two) and each kmesg dump writes a ``record_size`` chunk of
 information.
 
-Dumping both oopses and panics can be done by setting 1 in the ``dump_oops``
-variable while setting 0 in that variable dumps only the panics.
+Limiting which kinds of kmsg dumps are stored can be controlled via
+the ``max_reason`` value, as defined in include/linux/kmsg_dump.h's
+``enum kmsg_dump_reason``. For example, to store both Oopses and Panics,
+``max_reason`` should be set to 2 (KMSG_DUMP_OOPS), to store only Panics
+``max_reason`` should be set to 1 (KMSG_DUMP_PANIC). Setting this to 0
+(KMSG_DUMP_UNDEF), means the reason filtering will be controlled by the
+``printk.always_kmsg_dump`` boot param: if unset, it'll be KMSG_DUMP_OOPS,
+otherwise KMSG_DUMP_MAX.
 
 The module uses a counter to record multiple dumps but the counter gets reset
 on restart (i.e. new dumps after the restart will overwrite old ones).
@@ -90,7 +96,7 @@ Setting the ramoops parameters can be done in several different manners:
         .mem_address            = <...>,
         .mem_type               = <...>,
         .record_size            = <...>,
-        .dump_oops              = <...>,
+        .max_reason             = <...>,
         .ecc                    = <...>,
   };
 
diff --git a/drivers/platform/chrome/chromeos_pstore.c b/drivers/platform/chrome/chromeos_pstore.c
index d13770785fb5..fa51153688b4 100644
--- a/drivers/platform/chrome/chromeos_pstore.c
+++ b/drivers/platform/chrome/chromeos_pstore.c
@@ -57,7 +57,7 @@ static struct ramoops_platform_data chromeos_ramoops_data = {
 	.record_size	= 0x40000,
 	.console_size	= 0x20000,
 	.ftrace_size	= 0x20000,
-	.dump_oops	= 1,
+	.max_reason	= KMSG_DUMP_OOPS,
 };
 
 static struct platform_device chromeos_ramoops = {
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index c2f76b650f91..b8dac1d04e96 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -57,10 +57,15 @@ module_param(mem_type, uint, 0600);
 MODULE_PARM_DESC(mem_type,
 		"set to 1 to try to use unbuffered memory (default 0)");
 
-static int dump_oops = 1;
-module_param(dump_oops, int, 0600);
+static int ramoops_dump_oops = -1;
+module_param_named(dump_oops, ramoops_dump_oops, int, 0400);
 MODULE_PARM_DESC(dump_oops,
-		"set to 1 to dump oopses, 0 to only dump panics (default 1)");
+		 "set to 1 to dump oopses & panics, 0 to only dump panics (deprecated: use max_reason instead)");
+
+static int ramoops_max_reason = KMESG_DUMP_OOPS;
+module_param_named(max_reason, ramoops_max_reason, int, 0400);
+MODULE_PARM_DESC(max_reason,
+		 "maximum reason for kmsg dump (default 2: Oops and Panic) ");
 
 static int ramoops_ecc;
 module_param_named(ecc, ramoops_ecc, int, 0600);
@@ -81,7 +86,6 @@ struct ramoops_context {
 	size_t console_size;
 	size_t ftrace_size;
 	size_t pmsg_size;
-	int dump_oops;
 	u32 flags;
 	struct persistent_ram_ecc_info ecc_info;
 	unsigned int max_dump_cnt;
@@ -382,16 +386,14 @@ static int notrace ramoops_pstore_write(struct pstore_record *record)
 		return -EINVAL;
 
 	/*
-	 * Out of the various dmesg dump types, ramoops is currently designed
-	 * to only store crash logs, rather than storing general kernel logs.
+	 * We could filter on record->reason here if we wanted to (which
+	 * would duplicate what happened before the "max_reason" setting
+	 * was added), but that would defeat the purpose of a system
+	 * changing printk.always_kmsg_dump, so instead log everything that
+	 * the kmsg dumper sends us, since it should be doing the filtering
+	 * based on the combination of printk.always_kmsg_dump and our
+	 * requested "max_reason".
 	 */
-	if (record->reason != KMSG_DUMP_OOPS &&
-	    record->reason != KMSG_DUMP_PANIC)
-		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.
@@ -692,7 +694,14 @@ static int ramoops_parse_dt(struct platform_device *pdev,
 	pdata->mem_size = resource_size(res);
 	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");
+	/*
+	 * Setting "no-dump-oops" is deprecated and will be ignored if
+	 * "max_reason" is also specified.
+	 */
+	if (of_property_read_bool(of_node, "no-dump-oops"))
+		pdata->max_reason = KMSG_DUMP_PANIC;
+	else
+		pdata->max_reason = KMSG_DUMP_OOPS;
 
 #define parse_size(name, field, default_value) {			\
 		ret = ramoops_parse_dt_size(pdev, name, default_value,	\
@@ -708,6 +717,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
 	parse_size("pmsg-size", pdata->pmsg_size, 0);
 	parse_size("ecc-size", pdata->ecc_info.ecc_size, 0);
 	parse_size("flags", pdata->flags, 0);
+	parse_size("max-reason", pdata->max_reason, pdata->max_reason);
 
 #undef parse_size
 
@@ -791,7 +801,6 @@ static int ramoops_probe(struct platform_device *pdev)
 	cxt->console_size = pdata->console_size;
 	cxt->ftrace_size = pdata->ftrace_size;
 	cxt->pmsg_size = pdata->pmsg_size;
-	cxt->dump_oops = pdata->dump_oops;
 	cxt->flags = pdata->flags;
 	cxt->ecc_info = pdata->ecc_info;
 
@@ -834,8 +843,10 @@ 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;
+		cxt->pstore.max_reason = pdata->max_reason;
+	}
 	if (cxt->console_size)
 		cxt->pstore.flags |= PSTORE_FLAGS_CONSOLE;
 	if (cxt->max_ftrace_cnt)
@@ -871,7 +882,7 @@ static int ramoops_probe(struct platform_device *pdev)
 	mem_size = pdata->mem_size;
 	mem_address = pdata->mem_address;
 	record_size = pdata->record_size;
-	dump_oops = pdata->dump_oops;
+	ramoops_max_reason = pdata->max_reason;
 	ramoops_console_size = pdata->console_size;
 	ramoops_pmsg_size = pdata->pmsg_size;
 	ramoops_ftrace_size = pdata->ftrace_size;
@@ -954,7 +965,11 @@ static void __init ramoops_register_dummy(void)
 	pdata.console_size = ramoops_console_size;
 	pdata.ftrace_size = ramoops_ftrace_size;
 	pdata.pmsg_size = ramoops_pmsg_size;
-	pdata.dump_oops = dump_oops;
+	/* Parse deprecated module param "dump_oops" into "max_reason". */
+	if (ramoops_dump_oops != -1)
+		pdata.max_reason = ramoops_dump_oops ? KMSG_DUMP_OOPS
+						     : KMSG_DUMP_PANIC;
+	pdata.max_reason = ramoops_max_reason;
 	pdata.flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
 
 	/*
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 9cb9b9067298..9f16afec7290 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -133,7 +133,7 @@ struct ramoops_platform_data {
 	unsigned long	console_size;
 	unsigned long	ftrace_size;
 	unsigned long	pmsg_size;
-	int		dump_oops;
+	int		max_reason;
 	u32		flags;
 	struct persistent_ram_ecc_info ecc_info;
 };
-- 
2.20.1

[PATCH v3 5/6] ramoops: Add max_reason optional field to ramoops DT node

[Kees Cook]

From: Pavel Tatashin <pasha.tatashin@soleen.com>

Currently, it is possible to dump kmsges for panic, or oops.
With max_reason it is possible to dump messages for other
kmesg_dump events, for example reboot, halt, shutdown, kexec.

Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
Link: https://lore.kernel.org/r/20200505154510.93506-6-pasha.tatashin@soleen.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 .../devicetree/bindings/reserved-memory/ramoops.txt | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
index 0eba562fe5c6..b7886fea368c 100644
--- a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
+++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
@@ -30,7 +30,7 @@ Optional properties:
 - ecc-size: enables ECC support and specifies ECC buffer size in bytes
   (defaults to 0: no ECC)
 
-- record-size: maximum size in bytes of each dump done on oops/panic
+- record-size: maximum size in bytes of each kmsg dump.
   (defaults to 0: disabled)
 
 - console-size: size in bytes of log buffer reserved for kernel messages
@@ -45,7 +45,16 @@ Optional properties:
 - unbuffered: if present, use unbuffered mappings to map the reserved region
   (defaults to buffered mappings)
 
-- no-dump-oops: if present, only dump panics (defaults to panics and oops)
+- max-reason: if present, sets maximum type of kmsg dump reasons to store
+  (defaults to 2: log Oopses and Panics). This can be set to INT_MAX to
+  store all kmsg dumps. See include/linux/kmsg_dump.h KMSG_DUMP_* for other
+  kmsg dump reason values. Setting this to 0 (KMSG_DUMP_UNDEF), means the
+  reason filtering will be controlled by the printk.always_kmsg_dump boot
+  param: if unset, it will be KMSG_DUMP_OOPS, otherwise KMSG_DUMP_MAX.
+
+- no-dump-oops: deprecated, use max_reason instead. If present, and
+  max_reason is not specified, it is equivalent to max_reason = 1
+  (KMSG_DUMP_PANIC).
 
 - flags: if present, pass ramoops behavioral flags (defaults to 0,
   see include/linux/pstore_ram.h RAMOOPS_FLAG_* for flag values).
-- 
2.20.1

[PATCH v3 6/6] pstore/ram: Adjust module param permissions to reflect reality

[Kees Cook]

A couple module parameters had 0600 permissions, but changing them would
have no impact on ramoops, so switch these to 0400 to reflect reality.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/ram.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index b8dac1d04e96..1de9d68d5c24 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -53,7 +53,7 @@ MODULE_PARM_DESC(mem_size,
 		"size of reserved RAM used to store oops/panic logs");
 
 static unsigned int mem_type;
-module_param(mem_type, uint, 0600);
+module_param(mem_type, uint, 0400);
 MODULE_PARM_DESC(mem_type,
 		"set to 1 to try to use unbuffered memory (default 0)");
 
@@ -68,7 +68,7 @@ MODULE_PARM_DESC(max_reason,
 		 "maximum reason for kmsg dump (default 2: Oops and Panic) ");
 
 static int ramoops_ecc;
-module_param_named(ecc, ramoops_ecc, int, 0600);
+module_param_named(ecc, ramoops_ecc, int, 0400);
 MODULE_PARM_DESC(ramoops_ecc,
 		"if non-zero, the option enables ECC support and specifies "
 		"ECC buffer size in bytes (1 is a special value, means 16 "
-- 
2.20.1

Re: [PATCH v3 4/6] pstore/ram: Introduce max_reason and convert dump_oops

[Kees Cook]

On Wed, May 06, 2020 at 02:15:21PM -0700, Kees Cook wrote:
> From: Pavel Tatashin <pasha.tatashin@soleen.com>
> 
> Now that pstore_register() can correctly pass max_reason to the kmesg
> dump facility, introduce a new "max_reason" module parameter and
> "max-reason" Device Tree field.
> 
> The "dump_oops" module parameter and "dump-oops" Device
> Tree field are now considered deprecated, but are now automatically
> converted to their corresponding max_reason values when present, though
> the new max_reason setting has precedence.
> 
> For struct ramoops_platform_data, the "dump_oops" member is entirely
> replaced by a new "max_reason" member, with the only existing user
> updated in place.
> 
> Additionally remove the "reason" filter logic from ramoops_pstore_write(),
> as that is not specifically needed anymore, though technically
> this is a change in behavior for any ramoops users also setting the
> printk.always_kmsg_dump boot param, which will cause ramoops to behave as
> if max_reason was set to KMSG_DUMP_MAX.
> 
> Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> Link: https://lore.kernel.org/r/20200505154510.93506-4-pasha.tatashin@soleen.com
> Link: https://lore.kernel.org/r/20200505154510.93506-5-pasha.tatashin@soleen.com
> Co-developed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/admin-guide/ramoops.rst     | 14 +++++--
>  drivers/platform/chrome/chromeos_pstore.c |  2 +-
>  fs/pstore/ram.c                           | 51 +++++++++++++++--------
>  include/linux/pstore_ram.h                |  2 +-
>  4 files changed, 45 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
> index 6dbcc5481000..a60a96218ba9 100644
> --- a/Documentation/admin-guide/ramoops.rst
> +++ b/Documentation/admin-guide/ramoops.rst
> @@ -32,11 +32,17 @@ memory to be mapped strongly ordered, and atomic operations on strongly ordered
>  memory are implementation defined, and won't work on many ARMs such as omaps.
>  
>  The memory area is divided into ``record_size`` chunks (also rounded down to
> -power of two) and each oops/panic writes a ``record_size`` chunk of
> +power of two) and each kmesg dump writes a ``record_size`` chunk of
>  information.
>  
> -Dumping both oopses and panics can be done by setting 1 in the ``dump_oops``
> -variable while setting 0 in that variable dumps only the panics.
> +Limiting which kinds of kmsg dumps are stored can be controlled via
> +the ``max_reason`` value, as defined in include/linux/kmsg_dump.h's
> +``enum kmsg_dump_reason``. For example, to store both Oopses and Panics,
> +``max_reason`` should be set to 2 (KMSG_DUMP_OOPS), to store only Panics
> +``max_reason`` should be set to 1 (KMSG_DUMP_PANIC). Setting this to 0
> +(KMSG_DUMP_UNDEF), means the reason filtering will be controlled by the
> +``printk.always_kmsg_dump`` boot param: if unset, it'll be KMSG_DUMP_OOPS,
> +otherwise KMSG_DUMP_MAX.
>  
>  The module uses a counter to record multiple dumps but the counter gets reset
>  on restart (i.e. new dumps after the restart will overwrite old ones).
> @@ -90,7 +96,7 @@ Setting the ramoops parameters can be done in several different manners:
>          .mem_address            = <...>,
>          .mem_type               = <...>,
>          .record_size            = <...>,
> -        .dump_oops              = <...>,
> +        .max_reason             = <...>,
>          .ecc                    = <...>,
>    };
>  
> diff --git a/drivers/platform/chrome/chromeos_pstore.c b/drivers/platform/chrome/chromeos_pstore.c
> index d13770785fb5..fa51153688b4 100644
> --- a/drivers/platform/chrome/chromeos_pstore.c
> +++ b/drivers/platform/chrome/chromeos_pstore.c
> @@ -57,7 +57,7 @@ static struct ramoops_platform_data chromeos_ramoops_data = {
>  	.record_size	= 0x40000,
>  	.console_size	= 0x20000,
>  	.ftrace_size	= 0x20000,
> -	.dump_oops	= 1,
> +	.max_reason	= KMSG_DUMP_OOPS,
>  };
>  
>  static struct platform_device chromeos_ramoops = {
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index c2f76b650f91..b8dac1d04e96 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -57,10 +57,15 @@ module_param(mem_type, uint, 0600);
>  MODULE_PARM_DESC(mem_type,
>  		"set to 1 to try to use unbuffered memory (default 0)");
>  
> -static int dump_oops = 1;
> -module_param(dump_oops, int, 0600);
> +static int ramoops_dump_oops = -1;
> +module_param_named(dump_oops, ramoops_dump_oops, int, 0400);
>  MODULE_PARM_DESC(dump_oops,
> -		"set to 1 to dump oopses, 0 to only dump panics (default 1)");
> +		 "set to 1 to dump oopses & panics, 0 to only dump panics (deprecated: use max_reason instead)");
> +
> +static int ramoops_max_reason = KMESG_DUMP_OOPS;

This is what I get for a "quick change right before sending". This
is a typo and should be KMSG_DUMP_OOPS. :|

-- 
Kees Cook

Re: [PATCH v3 2/6] pstore/platform: Pass max_reason to kmesg dump

[Joe Perches]

On Wed, 2020-05-06 at 14:15 -0700, Kees Cook wrote:
> From: Pavel Tatashin <pasha.tatashin@soleen.com>
> 
> Add a new member to struct pstore_info for passing information about
> kmesg dump maximum reason. This allows a finer control of what kmesg
> dumps are sent to pstore storage backends.

trivia:

> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
[]
> @@ -96,6 +96,12 @@ struct pstore_record {
>   *
>   * @read_mutex:	serializes @open, @read, @close, and @erase callbacks
>   * @flags:	bitfield of frontends the backend can accept writes for
> + * @max_reason:	Used when PSTORE_FLAGS_DMESG is set. Contains the
> + *		kmsg_dump_reason enum value. KMSG_DUMP_UNDEF means
> + *		"use existing kmsg_dump() filtering, based on the
> + *		printk.always_kmsg_dump boot param" (which is either
> + *		KMSG_DUMP_OOPS when false, or KMSG_DUMP_MAX when
> + *		tree); see printk.always_kmsg_dump for more details.

s/tree/true/

Re: [PATCH v3 2/6] pstore/platform: Pass max_reason to kmesg dump

[Kees Cook]

On Wed, May 06, 2020 at 02:25:41PM -0700, Joe Perches wrote:
> On Wed, 2020-05-06 at 14:15 -0700, Kees Cook wrote:
> > From: Pavel Tatashin <pasha.tatashin@soleen.com>
> > 
> > Add a new member to struct pstore_info for passing information about
> > kmesg dump maximum reason. This allows a finer control of what kmesg
> > dumps are sent to pstore storage backends.
> 
> trivia:
> 
> > diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> []
> > @@ -96,6 +96,12 @@ struct pstore_record {
> >   *
> >   * @read_mutex:	serializes @open, @read, @close, and @erase callbacks
> >   * @flags:	bitfield of frontends the backend can accept writes for
> > + * @max_reason:	Used when PSTORE_FLAGS_DMESG is set. Contains the
> > + *		kmsg_dump_reason enum value. KMSG_DUMP_UNDEF means
> > + *		"use existing kmsg_dump() filtering, based on the
> > + *		printk.always_kmsg_dump boot param" (which is either
> > + *		KMSG_DUMP_OOPS when false, or KMSG_DUMP_MAX when
> > + *		tree); see printk.always_kmsg_dump for more details.
> 
> s/tree/true/

Eek, thanks. I'll fix my typo. :)

-- 
Kees Cook

Re: [PATCH v3 3/6] pstore/ram: Refactor DT size parsing

[Pavel Tatashin]

On Wed, May 6, 2020 at 5:15 PM Kees Cook <keescook@chromium.org> wrote:
>
> Refactor device tree size parsing routines to be able to pass a non-zero
> default value for providing a configurable default for the coming
> "max_reason" field.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

ramoops_parse_dt_size
parse_size

Are used to parse flags, and max-reason properties, so the "size" in
their names become outdated. How about:
ramoops_parse_dt_prop
parse_prop

Otherwise it looks good.

Thank you,
Pasha

Re: [PATCH v3 3/6] pstore/ram: Refactor DT size parsing

[Kees Cook]

On Thu, May 07, 2020 at 08:57:50AM -0400, Pavel Tatashin wrote:
> On Wed, May 6, 2020 at 5:15 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > Refactor device tree size parsing routines to be able to pass a non-zero
> > default value for providing a configurable default for the coming
> > "max_reason" field.
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> ramoops_parse_dt_size
> parse_size
> 
> Are used to parse flags, and max-reason properties, so the "size" in
> their names become outdated. How about:
> ramoops_parse_dt_prop
> parse_prop

Yeah, I struggled with that thought too.

> Otherwise it looks good.

Okay, great, I'll find a better name and apply this series. Thanks!

-- 
Kees Cook

Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[Petr Mladek]

On Wed 2020-05-06 14:15:17, Kees Cook wrote:
> Hi!
> 
> This is my stab at rearranging a few things based on Pavel's series. Most
> things remain the same; I just tweaked how defaults are arranged and
> detected and expanded the wording in a few places. Pavel, how does this
> v3 look to you?
> 
> Pavel's original cover letter:
> 
> pstore /mnt/console-ramoops-0 outputs only messages below the console
> loglevel, and our console loglevel is set to 3 due to slowness of
> serial console. Which means only errors and worse types of messages
> are recorded. There is no way to have different log levels for
> different consoles.
> 
> This patch series adds a new option to ramoops: max_reason that enables
> it to collect kmdesg dumps for other reasons beside oops and panics.

I was a bit confused by the above explanation. It talks about two
different numbering schemes:

   + console loglevels: emerg, alert, crit, err, warning, ...
   + dump reason: panic, oops, emerg, restart, halt, poweroff

This difference and also the jump from consoles to ramoops is far from
obvious.

My understanding is the following:

It is not possible to set loglevel per console. The global value must
be set by the slowest one. This prevents seeing all messages even
on fast consoles.

Alternative solution is to dump all messages using ramoops. The
problem is that it currently works only during Oops and panic
situation. This is solved by this patchset.


OK, I personally see this as two separate problems:

   1. Missing support to set loglevel per console.
   2. Missing support to dump messages for other reasons.

I would remove the paragraph about console log levels completely.
It is your reason to use ramoops. But it is not reason to modify
the logic about max_reason.


Now, the max_reason logic makes sense only when all the values
have some ordering. Is this the case?

I see it as two distinct sets:

   + panic, oops, emerg: describe how critical is an error situation
   + restart, halt, poweroff: describe behavior when the system goes down

Let's say that panic is more critical than oops. Is restart more
critical than halt?

If you want the dump during restart. Does it mean that you want it
also during emergency situation?

My fear is that this patchset is going to introduce user interface
(max_reason) with a weird logic. IMHO, max_reason is confusing even
in the code and we should not spread this to users.

Is there any reason why the existing printk.always_kmsg_dump option
is not enough for you?

Best Regards,
Petr

Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[Pavel Tatashin]

Hi Petr,

> Alternative solution is to dump all messages using ramoops. The
> problem is that it currently works only during Oops and panic
> situation. This is solved by this patchset.
>
>
> OK, I personally see this as two separate problems:
>
>    1. Missing support to set loglevel per console.
>    2. Missing support to dump messages for other reasons.
>
> I would remove the paragraph about console log levels completely.

OK, I see your point, this paragraph can be removed, however, I think
it makes it clear to understand the rationale for this change. As I
understand, the per console loglevel has been proposed but were never
accepted.

> It is your reason to use ramoops. But it is not reason to modify
> the logic about max_reason.
>
>
> Now, the max_reason logic makes sense only when all the values
> have some ordering. Is this the case?
>
> I see it as two distinct sets:
>
>    + panic, oops, emerg: describe how critical is an error situation
>    + restart, halt, poweroff: describe behavior when the system goes down
>
> Let's say that panic is more critical than oops. Is restart more
> critical than halt?
>
> If you want the dump during restart. Does it mean that you want it
> also during emergency situation?
>
> My fear is that this patchset is going to introduce user interface
> (max_reason) with a weird logic. IMHO, max_reason is confusing even
> in the code and we should not spread this to users.
>
> Is there any reason why the existing printk.always_kmsg_dump option
> is not enough for you?

printk.always_kmsg_dump is not working for me because ramoops has its
own filtering based on dump_oops boolean, and ignores everything but
panics and conditionally oops.
max_reason makes the ramoops internal logic cleaner compared to using dump_oops.

I agree, the reasons in kmsg_dump_reason do not order well  (I
actually want to add another reason for kexec type reboots, and where
do I put it?), so how about if we change the ordering list to
bitfield/flags, and instead of max_reason provide: "reasons" bitset?

Thank you,
Pasha

Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[Petr Mladek]

On Tue 2020-05-12 10:03:44, Pavel Tatashin wrote:
> > OK, I personally see this as two separate problems:
> >
> >    1. Missing support to set loglevel per console.
> >    2. Missing support to dump messages for other reasons.
> >
> > I would remove the paragraph about console log levels completely.
> 
> OK, I see your point, this paragraph can be removed, however, I think
> it makes it clear to understand the rationale for this change. As I
> understand, the per console loglevel has been proposed but were never
> accepted.

The proposal was not accepted because there were more requirements:

   + add console device into sysfs so that it can be modified there
   + make a reasonable backward compatible behavior

I guess that the sysfs interface discouraged the author to continue
on it.

Note that console loglevel handling is very complicated. There are
already four variables, see console_printk array in
kernel/printk/printk.c. Per console loglevel would make it even
more complicated.

It is a nighmare. And introducing max_reason parameter goes the same way.

> > Now, the max_reason logic makes sense only when all the values
> > have some ordering. Is this the case?
> >
> > I see it as two distinct sets:
> >
> >    + panic, oops, emerg: describe how critical is an error situation
> >    + restart, halt, poweroff: describe behavior when the system goes down
> >
> > Let's say that panic is more critical than oops. Is restart more
> > critical than halt?
> >
> > If you want the dump during restart. Does it mean that you want it
> > also during emergency situation?
> >
> > My fear is that this patchset is going to introduce user interface
> > (max_reason) with a weird logic. IMHO, max_reason is confusing even
> > in the code and we should not spread this to users.
> >
> > Is there any reason why the existing printk.always_kmsg_dump option
> > is not enough for you?
> 
> printk.always_kmsg_dump is not working for me because ramoops has its
> own filtering based on dump_oops boolean, and ignores everything but
> panics and conditionally oops.
> max_reason makes the ramoops internal logic cleaner compared to using dump_oops.

I see. Just to be sure. Is the main reason to add max_reason parameter
to keep complatibility of the deprecated dump_oops parameter? Or is
there any real use case for this granularity?

I made some arecheology. ramoops.dump_oops parameter was added in 2010 by the
initial commit 56d611a04fb2db77334e ("char drivers: RAM oops/panic
logger."

Note that the initial implementation printed Oops reason only when
dump_oops was set. It printed all other reasons otherwise. It seems
that there were only the two reasons at that time.

Now, printk.always_kmsg_dump parameter was added later in 2012 by
the commit c22ab332902333f8376601 ("kmsg_dump: don't run on non-error
paths by default").

IMHO, the later commit actually fixed the default behavior of ramoops.

I wonder if anyone is actually using the ramoops.dump_oops parameter
in reality. I would personally make it deprecated and change the
default behavior to work according to printk.always_kmsg_dump parameter.

IMHO, ramoops.dump_oops just increases complexity and should not have
been introduced at all. I would try hard to avoid introducing even bigger
complecity and mess.

I know that there is the "do not break existing userspace" rule. The
question is if there is any user and if it is worth it.

> I agree, the reasons in kmsg_dump_reason do not order well  (I
> actually want to add another reason for kexec type reboots, and where
> do I put it?), so how about if we change the ordering list to
> bitfield/flags, and instead of max_reason provide: "reasons" bitset?

It looks too complicated. I would really try hard to avoid the
parameter at all.

Best Regards,
Petr

Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[Steven Rostedt]

On Tue, 12 May 2020 17:52:07 +0200
Petr Mladek <pmladek@suse.com> wrote:

> I know that there is the "do not break existing userspace" rule. The
> question is if there is any user and if it is worth it.

If you break user space, and nobody is around to notice it, did you really
break it?

The answer is "No" ;-)

-- Steve

Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[Pavel Tatashin]

On Tue, May 12, 2020 at 11:52 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Tue 2020-05-12 10:03:44, Pavel Tatashin wrote:
> > > OK, I personally see this as two separate problems:
> > >
> > >    1. Missing support to set loglevel per console.
> > >    2. Missing support to dump messages for other reasons.
> > >
> > > I would remove the paragraph about console log levels completely.
> >
> > OK, I see your point, this paragraph can be removed, however, I think
> > it makes it clear to understand the rationale for this change. As I
> > understand, the per console loglevel has been proposed but were never
> > accepted.
>
> The proposal was not accepted because there were more requirements:
>
>    + add console device into sysfs so that it can be modified there
>    + make a reasonable backward compatible behavior
>
> I guess that the sysfs interface discouraged the author to continue
> on it.
>
> Note that console loglevel handling is very complicated. There are
> already four variables, see console_printk array in
> kernel/printk/printk.c. Per console loglevel would make it even
> more complicated.
>
> It is a nighmare. And introducing max_reason parameter goes the same way.
>
> > > Now, the max_reason logic makes sense only when all the values
> > > have some ordering. Is this the case?
> > >
> > > I see it as two distinct sets:
> > >
> > >    + panic, oops, emerg: describe how critical is an error situation
> > >    + restart, halt, poweroff: describe behavior when the system goes down
> > >
> > > Let's say that panic is more critical than oops. Is restart more
> > > critical than halt?
> > >
> > > If you want the dump during restart. Does it mean that you want it
> > > also during emergency situation?
> > >
> > > My fear is that this patchset is going to introduce user interface
> > > (max_reason) with a weird logic. IMHO, max_reason is confusing even
> > > in the code and we should not spread this to users.
> > >
> > > Is there any reason why the existing printk.always_kmsg_dump option
> > > is not enough for you?
> >
> > printk.always_kmsg_dump is not working for me because ramoops has its
> > own filtering based on dump_oops boolean, and ignores everything but
> > panics and conditionally oops.
> > max_reason makes the ramoops internal logic cleaner compared to using dump_oops.
>
> I see. Just to be sure. Is the main reason to add max_reason parameter
> to keep complatibility of the deprecated dump_oops parameter? Or is
> there any real use case for this granularity?
>
> I made some arecheology. ramoops.dump_oops parameter was added in 2010 by the
> initial commit 56d611a04fb2db77334e ("char drivers: RAM oops/panic
> logger."
>
> Note that the initial implementation printed Oops reason only when
> dump_oops was set. It printed all other reasons otherwise. It seems
> that there were only the two reasons at that time.
>
> Now, printk.always_kmsg_dump parameter was added later in 2012 by
> the commit c22ab332902333f8376601 ("kmsg_dump: don't run on non-error
> paths by default").
>
> IMHO, the later commit actually fixed the default behavior of ramoops.
>
> I wonder if anyone is actually using the ramoops.dump_oops parameter
> in reality. I would personally make it deprecated and change the
> default behavior to work according to printk.always_kmsg_dump parameter.

This sounds alright to me with one slight problem. I am doing this
work for an embedded arm64 SoC, so controlling everything via device
tree is preferable compared to having some settings via device tree
and others via kernel parameters, especially because the kernel
parameters are hardcoded by firmware that we try not to update too
often for uptime reasons.

>
> IMHO, ramoops.dump_oops just increases complexity and should not have
> been introduced at all. I would try hard to avoid introducing even bigger
> complecity and mess.

I agree, amoops.dump_oops should be depricated with or without
max_reason change.

>
> I know that there is the "do not break existing userspace" rule. The
> question is if there is any user and if it is worth it.
>
> > I agree, the reasons in kmsg_dump_reason do not order well  (I
> > actually want to add another reason for kexec type reboots, and where
> > do I put it?), so how about if we change the ordering list to
> > bitfield/flags, and instead of max_reason provide: "reasons" bitset?
>
> It looks too complicated. I would really try hard to avoid the
> parameter at all.

OK. Should we remove max_reason from struct kmsg_dumper and also
remove the misleading comment about kmsg_dump_reason ordering?

Thank you,
Pasha

Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[Kees Cook]

On Tue, May 12, 2020 at 05:52:07PM +0200, Petr Mladek wrote:
> On Tue 2020-05-12 10:03:44, Pavel Tatashin wrote:
> > > OK, I personally see this as two separate problems:
> > >
> > >    1. Missing support to set loglevel per console.
> > >    2. Missing support to dump messages for other reasons.
> > >
> > > I would remove the paragraph about console log levels completely.
> > 
> > OK, I see your point, this paragraph can be removed, however, I think
> > it makes it clear to understand the rationale for this change. As I
> > understand, the per console loglevel has been proposed but were never
> > accepted.

I understood Pavel's rationale as an output from my questions in the v1
series, that went like this, paraphrased:

Pavel: "I need to have other kmsg dump reasons available to pstore."
Kees:  "Why can't you just use the pstore console dumper?"
Pavel: "It's too much for the slow device; we only need to know about
        specific events that are already provided by kmsg dump."
Kees:  "Ah! Sounds good, max_reasons it is."

So, AIUI, loglevel remains orthogonal to this, and it's my fault for
even causing to be be brought up. Please disregard! :)

> > printk.always_kmsg_dump is not working for me because ramoops has its
> > own filtering based on dump_oops boolean, and ignores everything but
> > panics and conditionally oops.
> > max_reason makes the ramoops internal logic cleaner compared to using dump_oops.
> 
> I see. Just to be sure. Is the main reason to add max_reason parameter
> to keep complatibility of the deprecated dump_oops parameter? Or is
> there any real use case for this granularity?

In my mind it seemed like a nice mapping, so it was an easy port.

> I wonder if anyone is actually using the ramoops.dump_oops parameter
> in reality. I would personally make it deprecated and change the
> default behavior to work according to printk.always_kmsg_dump parameter.

Yes. For things I'm aware of: ARM devices with very tiny persistent RAM
were using ramoops and setting dump_oops to 0 (specifically, setting
the DT "no-dump-oops" to 1), and larger Android and Chrome OS devices
using ramoops were setting to dump_oops to 1[1].

The logic built into pstore recognizes a difference between panic and
non-panic dumps as well, as the expectation is that there is little to
no kernel infrastructure available for use during a panic kmsg.

> IMHO, ramoops.dump_oops just increases complexity and should not have
> been introduced at all. I would try hard to avoid introducing even bigger
> complecity and mess.

I think dump_oops was the wrong implementation, but granularity control
is still needed. It is an old parameter, and is baked into many device
trees on systems, so I can't just drop it. (In fact, I've had to support
some other DT compat issues[2] as well.)

> I know that there is the "do not break existing userspace" rule. The
> question is if there is any user and if it is worth it.

For dump_oops, yes, there is unfortunately.

> > I agree, the reasons in kmsg_dump_reason do not order well  (I
> > actually want to add another reason for kexec type reboots, and where
> > do I put it?), so how about if we change the ordering list to
> > bitfield/flags, and instead of max_reason provide: "reasons" bitset?
> 
> It looks too complicated. I would really try hard to avoid the
> parameter at all.

Here are the problems I see being solved by this:

- lifting kmsg dump reason filtering out of the individual pstore
  backends and making it part of the "infrastructure", so that
  there is a central place to set expectations. Right now there
  is a mix of explicit and implicit kmsg dump handling:

  - arch/powerpc/kernel/nvram_64.c has a hard-coded list
  - drivers/firmware/efi/efi-pstore.c doesn't expect anything but
    OOPS and PANIC.
  - drivers/mtd/mtdoops.c tries to filter using its own dump_oops
    and doesn't expect anything but OOPS and PANIC.
  - fs/pstore/ram.c: has a hard-coded list and uses its own
    dump_oops.
  - drivers/mtd/mtdpstore.c (under development[3]) expected only
    OOPS and PANIC and had its own dump_oops.

- providing a way for backends that can deal with all kmsg dump reasons
  to do so without breaking existing default behavior (i.e. getting
  Pavel what he's interested in).

So, that said, I'm totally fine with a bit field. I just need a way to
map the kmsg dump reasons onto the existing backend expectations and to
have Pavel's needs addressed.

-Kees

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/chrome/chromeos_pstore.c?h=v5.6#n60
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/pstore/ram.c?h=v5.6#n708
[3] https://lore.kernel.org/lkml/20200511233229.27745-11-keescook@chromium.org/

-- 
Kees Cook

Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[Kees Cook]

On Tue, May 12, 2020 at 12:49:10PM -0400, Pavel Tatashin wrote:
> On Tue, May 12, 2020 at 11:52 AM Petr Mladek <pmladek@suse.com> wrote:
> > I wonder if anyone is actually using the ramoops.dump_oops parameter
> > in reality. I would personally make it deprecated and change the
> > default behavior to work according to printk.always_kmsg_dump parameter.
> 
> This sounds alright to me with one slight problem. I am doing this
> work for an embedded arm64 SoC, so controlling everything via device
> tree is preferable compared to having some settings via device tree
> and others via kernel parameters, especially because the kernel
> parameters are hardcoded by firmware that we try not to update too
> often for uptime reasons.

I'm entirely convinced that this area of pstore needs to be cleaned up
and I want to have the pstore backends be able to declare their kmsg
dump reason filters in a configurable fashion. So at least on the pstore
end, I intend to have some way to do this.

> > IMHO, ramoops.dump_oops just increases complexity and should not have
> > been introduced at all. I would try hard to avoid introducing even bigger
> > complecity and mess.
> 
> I agree, amoops.dump_oops should be depricated with or without
> max_reason change.

Yup. dump_oops will be deprecated in favor of whatever we settle on here.

> > I know that there is the "do not break existing userspace" rule. The
> > question is if there is any user and if it is worth it.
> >
> > > I agree, the reasons in kmsg_dump_reason do not order well  (I
> > > actually want to add another reason for kexec type reboots, and where
> > > do I put it?), so how about if we change the ordering list to
> > > bitfield/flags, and instead of max_reason provide: "reasons" bitset?
> >
> > It looks too complicated. I would really try hard to avoid the
> > parameter at all.
> 
> OK. Should we remove max_reason from struct kmsg_dumper and also
> remove the misleading comment about kmsg_dump_reason ordering?

I'm also fine with this. I can have pstore infrastructure doing the
filtering if kmsg dump doesn't want to. Given the existence of
printk.always_kmsg_dump, though, it seemed like it was better to have
kmsg dump do this filtering instead.

At this point my preference is to switch to a bit field -- I don't see a
reason for ordering. The only cases that remain "special" appear to be
PANIC and EMERG (which, again, aren't ordered adjacent).

-Kees

-- 
Kees Cook

Re: [PATCH v3 4/6] pstore/ram: Introduce max_reason and convert dump_oops

[Tyler Hicks]

On 2020-05-06 14:15:21, Kees Cook wrote:
> From: Pavel Tatashin <pasha.tatashin@soleen.com>
> 
> Now that pstore_register() can correctly pass max_reason to the kmesg
> dump facility, introduce a new "max_reason" module parameter and
> "max-reason" Device Tree field.
> 
> The "dump_oops" module parameter and "dump-oops" Device
> Tree field are now considered deprecated, but are now automatically
> converted to their corresponding max_reason values when present, though
> the new max_reason setting has precedence.
> 
> For struct ramoops_platform_data, the "dump_oops" member is entirely
> replaced by a new "max_reason" member, with the only existing user
> updated in place.
> 
> Additionally remove the "reason" filter logic from ramoops_pstore_write(),
> as that is not specifically needed anymore, though technically
> this is a change in behavior for any ramoops users also setting the
> printk.always_kmsg_dump boot param, which will cause ramoops to behave as
> if max_reason was set to KMSG_DUMP_MAX.
> 
> Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> Link: https://lore.kernel.org/r/20200505154510.93506-4-pasha.tatashin@soleen.com
> Link: https://lore.kernel.org/r/20200505154510.93506-5-pasha.tatashin@soleen.com
> Co-developed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/admin-guide/ramoops.rst     | 14 +++++--
>  drivers/platform/chrome/chromeos_pstore.c |  2 +-
>  fs/pstore/ram.c                           | 51 +++++++++++++++--------
>  include/linux/pstore_ram.h                |  2 +-
>  4 files changed, 45 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
> index 6dbcc5481000..a60a96218ba9 100644
> --- a/Documentation/admin-guide/ramoops.rst
> +++ b/Documentation/admin-guide/ramoops.rst
> @@ -32,11 +32,17 @@ memory to be mapped strongly ordered, and atomic operations on strongly ordered
>  memory are implementation defined, and won't work on many ARMs such as omaps.
>  
>  The memory area is divided into ``record_size`` chunks (also rounded down to
> -power of two) and each oops/panic writes a ``record_size`` chunk of
> +power of two) and each kmesg dump writes a ``record_size`` chunk of
>  information.
>  
> -Dumping both oopses and panics can be done by setting 1 in the ``dump_oops``
> -variable while setting 0 in that variable dumps only the panics.
> +Limiting which kinds of kmsg dumps are stored can be controlled via
> +the ``max_reason`` value, as defined in include/linux/kmsg_dump.h's
> +``enum kmsg_dump_reason``. For example, to store both Oopses and Panics,
> +``max_reason`` should be set to 2 (KMSG_DUMP_OOPS), to store only Panics
> +``max_reason`` should be set to 1 (KMSG_DUMP_PANIC). Setting this to 0
> +(KMSG_DUMP_UNDEF), means the reason filtering will be controlled by the
> +``printk.always_kmsg_dump`` boot param: if unset, it'll be KMSG_DUMP_OOPS,
> +otherwise KMSG_DUMP_MAX.
>  
>  The module uses a counter to record multiple dumps but the counter gets reset
>  on restart (i.e. new dumps after the restart will overwrite old ones).
> @@ -90,7 +96,7 @@ Setting the ramoops parameters can be done in several different manners:
>          .mem_address            = <...>,
>          .mem_type               = <...>,
>          .record_size            = <...>,
> -        .dump_oops              = <...>,
> +        .max_reason             = <...>,
>          .ecc                    = <...>,
>    };
>  
> diff --git a/drivers/platform/chrome/chromeos_pstore.c b/drivers/platform/chrome/chromeos_pstore.c
> index d13770785fb5..fa51153688b4 100644
> --- a/drivers/platform/chrome/chromeos_pstore.c
> +++ b/drivers/platform/chrome/chromeos_pstore.c
> @@ -57,7 +57,7 @@ static struct ramoops_platform_data chromeos_ramoops_data = {
>  	.record_size	= 0x40000,
>  	.console_size	= 0x20000,
>  	.ftrace_size	= 0x20000,
> -	.dump_oops	= 1,
> +	.max_reason	= KMSG_DUMP_OOPS,
>  };
>  
>  static struct platform_device chromeos_ramoops = {
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index c2f76b650f91..b8dac1d04e96 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -57,10 +57,15 @@ module_param(mem_type, uint, 0600);
>  MODULE_PARM_DESC(mem_type,
>  		"set to 1 to try to use unbuffered memory (default 0)");
>  
> -static int dump_oops = 1;
> -module_param(dump_oops, int, 0600);
> +static int ramoops_dump_oops = -1;
> +module_param_named(dump_oops, ramoops_dump_oops, int, 0400);
>  MODULE_PARM_DESC(dump_oops,
> -		"set to 1 to dump oopses, 0 to only dump panics (default 1)");
> +		 "set to 1 to dump oopses & panics, 0 to only dump panics (deprecated: use max_reason instead)");
> +
> +static int ramoops_max_reason = KMESG_DUMP_OOPS;
> +module_param_named(max_reason, ramoops_max_reason, int, 0400);
> +MODULE_PARM_DESC(max_reason,
> +		 "maximum reason for kmsg dump (default 2: Oops and Panic) ");
>  
>  static int ramoops_ecc;
>  module_param_named(ecc, ramoops_ecc, int, 0600);
> @@ -81,7 +86,6 @@ struct ramoops_context {
>  	size_t console_size;
>  	size_t ftrace_size;
>  	size_t pmsg_size;
> -	int dump_oops;
>  	u32 flags;
>  	struct persistent_ram_ecc_info ecc_info;
>  	unsigned int max_dump_cnt;
> @@ -382,16 +386,14 @@ static int notrace ramoops_pstore_write(struct pstore_record *record)
>  		return -EINVAL;
>  
>  	/*
> -	 * Out of the various dmesg dump types, ramoops is currently designed
> -	 * to only store crash logs, rather than storing general kernel logs.
> +	 * We could filter on record->reason here if we wanted to (which
> +	 * would duplicate what happened before the "max_reason" setting
> +	 * was added), but that would defeat the purpose of a system
> +	 * changing printk.always_kmsg_dump, so instead log everything that
> +	 * the kmsg dumper sends us, since it should be doing the filtering
> +	 * based on the combination of printk.always_kmsg_dump and our
> +	 * requested "max_reason".
>  	 */
> -	if (record->reason != KMSG_DUMP_OOPS &&
> -	    record->reason != KMSG_DUMP_PANIC)
> -		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.
> @@ -692,7 +694,14 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>  	pdata->mem_size = resource_size(res);
>  	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");
> +	/*
> +	 * Setting "no-dump-oops" is deprecated and will be ignored if
> +	 * "max_reason" is also specified.
> +	 */
> +	if (of_property_read_bool(of_node, "no-dump-oops"))
> +		pdata->max_reason = KMSG_DUMP_PANIC;
> +	else
> +		pdata->max_reason = KMSG_DUMP_OOPS;
>  
>  #define parse_size(name, field, default_value) {			\
>  		ret = ramoops_parse_dt_size(pdev, name, default_value,	\
> @@ -708,6 +717,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>  	parse_size("pmsg-size", pdata->pmsg_size, 0);
>  	parse_size("ecc-size", pdata->ecc_info.ecc_size, 0);
>  	parse_size("flags", pdata->flags, 0);
> +	parse_size("max-reason", pdata->max_reason, pdata->max_reason);
>  
>  #undef parse_size
>  
> @@ -791,7 +801,6 @@ static int ramoops_probe(struct platform_device *pdev)
>  	cxt->console_size = pdata->console_size;
>  	cxt->ftrace_size = pdata->ftrace_size;
>  	cxt->pmsg_size = pdata->pmsg_size;
> -	cxt->dump_oops = pdata->dump_oops;
>  	cxt->flags = pdata->flags;
>  	cxt->ecc_info = pdata->ecc_info;
>  
> @@ -834,8 +843,10 @@ 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;
> +		cxt->pstore.max_reason = pdata->max_reason;
> +	}
>  	if (cxt->console_size)
>  		cxt->pstore.flags |= PSTORE_FLAGS_CONSOLE;
>  	if (cxt->max_ftrace_cnt)
> @@ -871,7 +882,7 @@ static int ramoops_probe(struct platform_device *pdev)
>  	mem_size = pdata->mem_size;
>  	mem_address = pdata->mem_address;
>  	record_size = pdata->record_size;
> -	dump_oops = pdata->dump_oops;
> +	ramoops_max_reason = pdata->max_reason;
>  	ramoops_console_size = pdata->console_size;
>  	ramoops_pmsg_size = pdata->pmsg_size;
>  	ramoops_ftrace_size = pdata->ftrace_size;
> @@ -954,7 +965,11 @@ static void __init ramoops_register_dummy(void)
>  	pdata.console_size = ramoops_console_size;
>  	pdata.ftrace_size = ramoops_ftrace_size;
>  	pdata.pmsg_size = ramoops_pmsg_size;
> -	pdata.dump_oops = dump_oops;
> +	/* Parse deprecated module param "dump_oops" into "max_reason". */
> +	if (ramoops_dump_oops != -1)
> +		pdata.max_reason = ramoops_dump_oops ? KMSG_DUMP_OOPS
> +						     : KMSG_DUMP_PANIC;
> +	pdata.max_reason = ramoops_max_reason;

This isn't quite right. We're conditionally assigning pdata.max_reason
and then immediately re-assigning it.

IIUC, we're just missing an else block and it should look like this:

	/* Parse deprecated module param "dump_oops" into "max_reason". */
	if (ramoops_dump_oops != -1)
		pdata.max_reason = ramoops_dump_oops ? KMSG_DUMP_OOPS
						     : KMSG_DUMP_PANIC;
	else
		pdata.max_reason = ramoops_max_reason;

Tyler

>  	pdata.flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
>  
>  	/*
> diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> index 9cb9b9067298..9f16afec7290 100644
> --- a/include/linux/pstore_ram.h
> +++ b/include/linux/pstore_ram.h
> @@ -133,7 +133,7 @@ struct ramoops_platform_data {
>  	unsigned long	console_size;
>  	unsigned long	ftrace_size;
>  	unsigned long	pmsg_size;
> -	int		dump_oops;
> +	int		max_reason;
>  	u32		flags;
>  	struct persistent_ram_ecc_info ecc_info;
>  };
> -- 
> 2.20.1
> 

Re: [PATCH v3 4/6] pstore/ram: Introduce max_reason and convert dump_oops

[Kees Cook]

On Tue, May 12, 2020 at 06:35:04PM -0500, Tyler Hicks wrote:
> On 2020-05-06 14:15:21, Kees Cook wrote:
> > From: Pavel Tatashin <pasha.tatashin@soleen.com>
> > 
> > Now that pstore_register() can correctly pass max_reason to the kmesg
> > dump facility, introduce a new "max_reason" module parameter and
> > "max-reason" Device Tree field.
> > 
> > The "dump_oops" module parameter and "dump-oops" Device
> > Tree field are now considered deprecated, but are now automatically
> > converted to their corresponding max_reason values when present, though
> > the new max_reason setting has precedence.
> > 
> > For struct ramoops_platform_data, the "dump_oops" member is entirely
> > replaced by a new "max_reason" member, with the only existing user
> > updated in place.
> > 
> > Additionally remove the "reason" filter logic from ramoops_pstore_write(),
> > as that is not specifically needed anymore, though technically
> > this is a change in behavior for any ramoops users also setting the
> > printk.always_kmsg_dump boot param, which will cause ramoops to behave as
> > if max_reason was set to KMSG_DUMP_MAX.
> > 
> > Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> > Link: https://lore.kernel.org/r/20200505154510.93506-4-pasha.tatashin@soleen.com
> > Link: https://lore.kernel.org/r/20200505154510.93506-5-pasha.tatashin@soleen.com
> > Co-developed-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  Documentation/admin-guide/ramoops.rst     | 14 +++++--
> >  drivers/platform/chrome/chromeos_pstore.c |  2 +-
> >  fs/pstore/ram.c                           | 51 +++++++++++++++--------
> >  include/linux/pstore_ram.h                |  2 +-
> >  4 files changed, 45 insertions(+), 24 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/ramoops.rst b/Documentation/admin-guide/ramoops.rst
> > index 6dbcc5481000..a60a96218ba9 100644
> > --- a/Documentation/admin-guide/ramoops.rst
> > +++ b/Documentation/admin-guide/ramoops.rst
> > @@ -32,11 +32,17 @@ memory to be mapped strongly ordered, and atomic operations on strongly ordered
> >  memory are implementation defined, and won't work on many ARMs such as omaps.
> >  
> >  The memory area is divided into ``record_size`` chunks (also rounded down to
> > -power of two) and each oops/panic writes a ``record_size`` chunk of
> > +power of two) and each kmesg dump writes a ``record_size`` chunk of
> >  information.
> >  
> > -Dumping both oopses and panics can be done by setting 1 in the ``dump_oops``
> > -variable while setting 0 in that variable dumps only the panics.
> > +Limiting which kinds of kmsg dumps are stored can be controlled via
> > +the ``max_reason`` value, as defined in include/linux/kmsg_dump.h's
> > +``enum kmsg_dump_reason``. For example, to store both Oopses and Panics,
> > +``max_reason`` should be set to 2 (KMSG_DUMP_OOPS), to store only Panics
> > +``max_reason`` should be set to 1 (KMSG_DUMP_PANIC). Setting this to 0
> > +(KMSG_DUMP_UNDEF), means the reason filtering will be controlled by the
> > +``printk.always_kmsg_dump`` boot param: if unset, it'll be KMSG_DUMP_OOPS,
> > +otherwise KMSG_DUMP_MAX.
> >  
> >  The module uses a counter to record multiple dumps but the counter gets reset
> >  on restart (i.e. new dumps after the restart will overwrite old ones).
> > @@ -90,7 +96,7 @@ Setting the ramoops parameters can be done in several different manners:
> >          .mem_address            = <...>,
> >          .mem_type               = <...>,
> >          .record_size            = <...>,
> > -        .dump_oops              = <...>,
> > +        .max_reason             = <...>,
> >          .ecc                    = <...>,
> >    };
> >  
> > diff --git a/drivers/platform/chrome/chromeos_pstore.c b/drivers/platform/chrome/chromeos_pstore.c
> > index d13770785fb5..fa51153688b4 100644
> > --- a/drivers/platform/chrome/chromeos_pstore.c
> > +++ b/drivers/platform/chrome/chromeos_pstore.c
> > @@ -57,7 +57,7 @@ static struct ramoops_platform_data chromeos_ramoops_data = {
> >  	.record_size	= 0x40000,
> >  	.console_size	= 0x20000,
> >  	.ftrace_size	= 0x20000,
> > -	.dump_oops	= 1,
> > +	.max_reason	= KMSG_DUMP_OOPS,
> >  };
> >  
> >  static struct platform_device chromeos_ramoops = {
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index c2f76b650f91..b8dac1d04e96 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -57,10 +57,15 @@ module_param(mem_type, uint, 0600);
> >  MODULE_PARM_DESC(mem_type,
> >  		"set to 1 to try to use unbuffered memory (default 0)");
> >  
> > -static int dump_oops = 1;
> > -module_param(dump_oops, int, 0600);
> > +static int ramoops_dump_oops = -1;
> > +module_param_named(dump_oops, ramoops_dump_oops, int, 0400);
> >  MODULE_PARM_DESC(dump_oops,
> > -		"set to 1 to dump oopses, 0 to only dump panics (default 1)");
> > +		 "set to 1 to dump oopses & panics, 0 to only dump panics (deprecated: use max_reason instead)");
> > +
> > +static int ramoops_max_reason = KMESG_DUMP_OOPS;
> > +module_param_named(max_reason, ramoops_max_reason, int, 0400);
> > +MODULE_PARM_DESC(max_reason,
> > +		 "maximum reason for kmsg dump (default 2: Oops and Panic) ");
> >  
> >  static int ramoops_ecc;
> >  module_param_named(ecc, ramoops_ecc, int, 0600);
> > @@ -81,7 +86,6 @@ struct ramoops_context {
> >  	size_t console_size;
> >  	size_t ftrace_size;
> >  	size_t pmsg_size;
> > -	int dump_oops;
> >  	u32 flags;
> >  	struct persistent_ram_ecc_info ecc_info;
> >  	unsigned int max_dump_cnt;
> > @@ -382,16 +386,14 @@ static int notrace ramoops_pstore_write(struct pstore_record *record)
> >  		return -EINVAL;
> >  
> >  	/*
> > -	 * Out of the various dmesg dump types, ramoops is currently designed
> > -	 * to only store crash logs, rather than storing general kernel logs.
> > +	 * We could filter on record->reason here if we wanted to (which
> > +	 * would duplicate what happened before the "max_reason" setting
> > +	 * was added), but that would defeat the purpose of a system
> > +	 * changing printk.always_kmsg_dump, so instead log everything that
> > +	 * the kmsg dumper sends us, since it should be doing the filtering
> > +	 * based on the combination of printk.always_kmsg_dump and our
> > +	 * requested "max_reason".
> >  	 */
> > -	if (record->reason != KMSG_DUMP_OOPS &&
> > -	    record->reason != KMSG_DUMP_PANIC)
> > -		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.
> > @@ -692,7 +694,14 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> >  	pdata->mem_size = resource_size(res);
> >  	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");
> > +	/*
> > +	 * Setting "no-dump-oops" is deprecated and will be ignored if
> > +	 * "max_reason" is also specified.
> > +	 */
> > +	if (of_property_read_bool(of_node, "no-dump-oops"))
> > +		pdata->max_reason = KMSG_DUMP_PANIC;
> > +	else
> > +		pdata->max_reason = KMSG_DUMP_OOPS;
> >  
> >  #define parse_size(name, field, default_value) {			\
> >  		ret = ramoops_parse_dt_size(pdev, name, default_value,	\
> > @@ -708,6 +717,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> >  	parse_size("pmsg-size", pdata->pmsg_size, 0);
> >  	parse_size("ecc-size", pdata->ecc_info.ecc_size, 0);
> >  	parse_size("flags", pdata->flags, 0);
> > +	parse_size("max-reason", pdata->max_reason, pdata->max_reason);
> >  
> >  #undef parse_size
> >  
> > @@ -791,7 +801,6 @@ static int ramoops_probe(struct platform_device *pdev)
> >  	cxt->console_size = pdata->console_size;
> >  	cxt->ftrace_size = pdata->ftrace_size;
> >  	cxt->pmsg_size = pdata->pmsg_size;
> > -	cxt->dump_oops = pdata->dump_oops;
> >  	cxt->flags = pdata->flags;
> >  	cxt->ecc_info = pdata->ecc_info;
> >  
> > @@ -834,8 +843,10 @@ 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;
> > +		cxt->pstore.max_reason = pdata->max_reason;
> > +	}
> >  	if (cxt->console_size)
> >  		cxt->pstore.flags |= PSTORE_FLAGS_CONSOLE;
> >  	if (cxt->max_ftrace_cnt)
> > @@ -871,7 +882,7 @@ static int ramoops_probe(struct platform_device *pdev)
> >  	mem_size = pdata->mem_size;
> >  	mem_address = pdata->mem_address;
> >  	record_size = pdata->record_size;
> > -	dump_oops = pdata->dump_oops;
> > +	ramoops_max_reason = pdata->max_reason;
> >  	ramoops_console_size = pdata->console_size;
> >  	ramoops_pmsg_size = pdata->pmsg_size;
> >  	ramoops_ftrace_size = pdata->ftrace_size;
> > @@ -954,7 +965,11 @@ static void __init ramoops_register_dummy(void)
> >  	pdata.console_size = ramoops_console_size;
> >  	pdata.ftrace_size = ramoops_ftrace_size;
> >  	pdata.pmsg_size = ramoops_pmsg_size;
> > -	pdata.dump_oops = dump_oops;
> > +	/* Parse deprecated module param "dump_oops" into "max_reason". */
> > +	if (ramoops_dump_oops != -1)
> > +		pdata.max_reason = ramoops_dump_oops ? KMSG_DUMP_OOPS
> > +						     : KMSG_DUMP_PANIC;
> > +	pdata.max_reason = ramoops_max_reason;
> 
> This isn't quite right. We're conditionally assigning pdata.max_reason
> and then immediately re-assigning it.
> 
> IIUC, we're just missing an else block and it should look like this:
> 
> 	/* Parse deprecated module param "dump_oops" into "max_reason". */
> 	if (ramoops_dump_oops != -1)
> 		pdata.max_reason = ramoops_dump_oops ? KMSG_DUMP_OOPS
> 						     : KMSG_DUMP_PANIC;
> 	else
> 		pdata.max_reason = ramoops_max_reason;

Oops, yes. ramoops_max_reason needs to also have an "unset" value so
this can determine which was set... I'll get this fixed. Thanks for
double-checking this!

-- 
Kees Cook

Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[Petr Mladek]

On Tue 2020-05-12 11:45:54, Kees Cook wrote:
> Here are the problems I see being solved by this:
> 
> - lifting kmsg dump reason filtering out of the individual pstore
>   backends and making it part of the "infrastructure", so that
>   there is a central place to set expectations. Right now there
>   is a mix of explicit and implicit kmsg dump handling:
> 
>   - arch/powerpc/kernel/nvram_64.c has a hard-coded list

It handles restart, halt, poweroff the same way.  I wonder if anyone
would want to distinguish them.

>   - drivers/firmware/efi/efi-pstore.c doesn't expect anything but
>     OOPS and PANIC.
>   - drivers/mtd/mtdoops.c tries to filter using its own dump_oops
>     and doesn't expect anything but OOPS and PANIC.
>   - fs/pstore/ram.c: has a hard-coded list and uses its own
>     dump_oops.
>   - drivers/mtd/mtdpstore.c (under development[3]) expected only
>     OOPS and PANIC and had its own dump_oops.

The others handle only panic or oops.

What about splitting the reason into two variables? One for severity
and other for shutdown behavior. I mean:

  + reason: panic, oops, emergency, shutdown    (ordered by severity)
  + handling: restart, halt, poweroff

Or we might just replace KMSG_DUMP_RESTART, KMSG_DUMP_HALT,
KMSG_DUMP_POWEROFF with a single KMSG_DUMP_SHUTDOWN.

Then the max reason variable would make sense.

Best Regards,
Petr

Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[Kees Cook]

On Wed, May 13, 2020 at 09:34:49AM +0200, Petr Mladek wrote:
> On Tue 2020-05-12 11:45:54, Kees Cook wrote:
> > Here are the problems I see being solved by this:
> > 
> > - lifting kmsg dump reason filtering out of the individual pstore
> >   backends and making it part of the "infrastructure", so that
> >   there is a central place to set expectations. Right now there
> >   is a mix of explicit and implicit kmsg dump handling:
> > 
> >   - arch/powerpc/kernel/nvram_64.c has a hard-coded list
> 
> It handles restart, halt, poweroff the same way.  I wonder if anyone
> would want to distinguish them.
> 
> >   - drivers/firmware/efi/efi-pstore.c doesn't expect anything but
> >     OOPS and PANIC.
> >   - drivers/mtd/mtdoops.c tries to filter using its own dump_oops
> >     and doesn't expect anything but OOPS and PANIC.
> >   - fs/pstore/ram.c: has a hard-coded list and uses its own
> >     dump_oops.
> >   - drivers/mtd/mtdpstore.c (under development[3]) expected only
> >     OOPS and PANIC and had its own dump_oops.
> 
> The others handle only panic or oops.
> 
> What about splitting the reason into two variables? One for severity
> and other for shutdown behavior. I mean:
> 
>   + reason: panic, oops, emergency, shutdown    (ordered by severity)
>   + handling: restart, halt, poweroff
> 
> Or we might just replace KMSG_DUMP_RESTART, KMSG_DUMP_HALT,
> KMSG_DUMP_POWEROFF with a single KMSG_DUMP_SHUTDOWN.
> 
> Then the max reason variable would make sense.

That would work for me, yeah. Pavel, is that enough granularity for you?

-- 
Kees Cook

Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[Pavel Tatashin]

> >   + reason: panic, oops, emergency, shutdown    (ordered by severity)
> >   + handling: restart, halt, poweroff
> >
> > Or we might just replace KMSG_DUMP_RESTART, KMSG_DUMP_HALT,
> > KMSG_DUMP_POWEROFF with a single KMSG_DUMP_SHUTDOWN.
> >
> > Then the max reason variable would make sense.
>
> That would work for me, yeah. Pavel, is that enough granularity for you?
>

Yes, I like the second approach: where we combine all shutdown type
events into a single type.
max_reason will have 4 levels:

   KMSG_DUMP_PANIC,
   KMSG_DUMP_OOPS,
   KMSG_DUMP_EMERG,
   KMSG_DUMP_SHUTDOWN,

If needed it is possible to determine from dmesg logs what kind of
shutdown was taken, because there is a message logged right before
every kmsg_dump() for these events:

249   if (!cmd)
250   pr_emerg("Restarting system\n");
251   else
252   pr_emerg("Restarting system with command '%s'\n", cmd);
253   kmsg_dump(KMSG_DUMP_RESTART);

276   pr_emerg("System halted\n");
277   kmsg_dump(KMSG_DUMP_HALT);

294   pr_emerg("Power down\n");
295   kmsg_dump(KMSG_DUMP_POWEROFF);

Kees, I will submit a new series with these changes soon.

Thank you,
Pasha

Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

[Kees Cook]

On Wed, May 13, 2020 at 10:35:16AM -0400, Pavel Tatashin wrote:
> > >   + reason: panic, oops, emergency, shutdown    (ordered by severity)
> > >   + handling: restart, halt, poweroff
> > >
> > > Or we might just replace KMSG_DUMP_RESTART, KMSG_DUMP_HALT,
> > > KMSG_DUMP_POWEROFF with a single KMSG_DUMP_SHUTDOWN.
> > >
> > > Then the max reason variable would make sense.
> >
> > That would work for me, yeah. Pavel, is that enough granularity for you?
> >
> 
> Yes, I like the second approach: where we combine all shutdown type
> events into a single type.
> max_reason will have 4 levels:
> 
>    KMSG_DUMP_PANIC,
>    KMSG_DUMP_OOPS,
>    KMSG_DUMP_EMERG,
>    KMSG_DUMP_SHUTDOWN,
> 
> If needed it is possible to determine from dmesg logs what kind of
> shutdown was taken, because there is a message logged right before
> every kmsg_dump() for these events:
> 
> 249   if (!cmd)
> 250   pr_emerg("Restarting system\n");
> 251   else
> 252   pr_emerg("Restarting system with command '%s'\n", cmd);
> 253   kmsg_dump(KMSG_DUMP_RESTART);
> 
> 276   pr_emerg("System halted\n");
> 277   kmsg_dump(KMSG_DUMP_HALT);
> 
> 294   pr_emerg("Power down\n");
> 295   kmsg_dump(KMSG_DUMP_POWEROFF);
> 
> Kees, I will submit a new series with these changes soon.

Great; thanks!

-Kees

-- 
Kees Cook

Re: [PATCH v3 4/6] pstore/ram: Introduce max_reason and convert dump_oops

[Kees Cook]

On Tue, May 12, 2020 at 06:35:04PM -0500, Tyler Hicks wrote:
> On 2020-05-06 14:15:21, Kees Cook wrote:
> > @@ -954,7 +965,11 @@ static void __init ramoops_register_dummy(void)
> >  	pdata.console_size = ramoops_console_size;
> >  	pdata.ftrace_size = ramoops_ftrace_size;
> >  	pdata.pmsg_size = ramoops_pmsg_size;
> > -	pdata.dump_oops = dump_oops;
> > +	/* Parse deprecated module param "dump_oops" into "max_reason". */
> > +	if (ramoops_dump_oops != -1)
> > +		pdata.max_reason = ramoops_dump_oops ? KMSG_DUMP_OOPS
> > +						     : KMSG_DUMP_PANIC;
> > +	pdata.max_reason = ramoops_max_reason;
> 
> This isn't quite right. We're conditionally assigning pdata.max_reason
> and then immediately re-assigning it.
> 
> IIUC, we're just missing an else block and it should look like this:
> 
> 	/* Parse deprecated module param "dump_oops" into "max_reason". */
> 	if (ramoops_dump_oops != -1)
> 		pdata.max_reason = ramoops_dump_oops ? KMSG_DUMP_OOPS
> 						     : KMSG_DUMP_PANIC;
> 	else
> 		pdata.max_reason = ramoops_max_reason;

Whoops, I forgot to CC you Tyler! This should be fixed now here:
https://lore.kernel.org/lkml/20200515184434.8470-6-keescook@chromium.org/

-- 
Kees Cook