[systemd-devel] [RFC] pstore: options to enable kernel writing into the pstore

[Zbigniew Jędrzejewski-Szmek]

On Thu, Mar 26, 2020 at 01:05:22PM -0500, Eric DeVolder wrote:
> >Below is a proposal for adding a couple of settings to the systemd pstore
> >service so that it can enable the kernel parameters that allow the
> >kernel to write into the pstore.

Hi,

please submit this as PR.

> > From 837d716c6e7ed02518a399356df95bf7c47e1772 Mon Sep 17 00:00:00 2001
> >From: Eric DeVolder <eric.devolder at oracle.com>
> >Date: Wed, 11 Mar 2020 14:11:03 -0500
> >Subject: [RFC] pstore: options to enable kernel writing into the pstore
> >
> >The systemd pstore service archives the contents of /sys/fs/pstore
> >upon boot so that there is room for a subsequent dump. The pstore is
> >usually backed by flash memory typically in the vicinity of 64KB.  The
> >pstore can contain post-mortem debug information even if kdump fails
> >or is not enabld.
> >
> >The issue is that while the service is present, the kernel still needs
> >to be configured to write data into the pstore. It has two parameters,
> >crash_kexec_post_notifiers and printk.always_kmsg_dump, that control
> >writes into pstore.
> >
> >The crash_kexec_post_notifiers parameter enables the kernel to write
> >dmesg (including stack trace) into pstore upon a panic, and
> >printk.always_kmsg_dump parameter enables the kernel to write dmesg upon
> >a shutdown (shutdown, reboot, halt).
> >
> >As it stands today, these parameters are not managed/manipulated by the
> >systemd pstore service, and are solely reliant upon the user [to have
> >the foresight] to set them on the kernel command line at boot, or post
> >boot via sysfs. Furthermore, the user would need to set these parameters
> >in a persistent fashion so that that they are enabled on subsequent
> >reboots.
> >
> >This patch allows the user to set these parameters via the systemd
> >pstore service, and forget about it. This patch introduces two new
> >settings in the pstore.conf, 'kmsg' and 'crash'. If either of these
> >is set to true, then the corresponding parameter is enabled in the
> >kernel. If the setting is false, then the parameter is not touched,
> >thus preserving whatever behavior the user may have previously
> >chosen.
> >---
> >  src/pstore/pstore.c | 36 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 35 insertions(+), 1 deletion(-)
> >
> >diff --git a/src/pstore/pstore.c b/src/pstore/pstore.c
> >index 5c812b5d5b..02bd94751f 100644
> >--- a/src/pstore/pstore.c
> >+++ b/src/pstore/pstore.c
> >@@ -68,6 +68,10 @@ static DEFINE_CONFIG_PARSE_ENUM(config_parse_pstore_storage, pstore_storage, PSt
> >  static PStoreStorage arg_storage = PSTORE_STORAGE_EXTERNAL;
> >
> >  static bool arg_unlink = true;
> >+static bool arg_kmsg = false;
> >+static bool arg_crash = false;
> >+static const char *arg_kmsg_path = "/sys/module/printk/parameters/always_kmsg_dump";
> >+static const char *arg_crash_path = "/sys/module/kernel/parameters/crash_kexec_post_notifiers";
> >  static const char *arg_sourcedir = "/sys/fs/pstore";
> >  static const char *arg_archivedir = "/var/lib/systemd/pstore";
> >
> >@@ -75,6 +79,8 @@ static int parse_config(void) {
> >          static const ConfigTableItem items[] = {
> >                  { "PStore", "Unlink",  config_parse_bool,           0, &arg_unlink },
> >                  { "PStore", "Storage", config_parse_pstore_storage, 0, &arg_storage },
> >+                { "PStore", "kmsg",    config_parse_bool,           0, &arg_kmsg },
> >+                { "PStore", "crash",   config_parse_bool,           0, &arg_crash },

This also needs a man page update.

Config keys are generally capitalized. But they should be named in a
way that indicates what they're actually configuring.

> >                  {}
> >          };
> >
> >@@ -363,7 +369,7 @@ static int list_files(PStoreList *list, const char *sourcepath) {
> >
> >  static int run(int argc, char *argv[]) {
> >          _cleanup_(pstore_entries_reset) PStoreList list = {};
> >-        int r;
> >+        int fd, r;
> >
> >          log_setup_service();
> >
> >@@ -380,6 +386,34 @@ static int run(int argc, char *argv[]) {
> >          log_debug("Selected storage: %s.", pstore_storage_to_string(arg_storage));
> >          log_debug("Selected unlink: %s.", yes_no(arg_unlink));
> >
> >+        if (arg_kmsg) {
> >+                /* Only enable if requested; otherwise do not touch the parameter */
> >+                /* NOTE: These errors are not fatal */
> >+                fd = open(arg_kmsg_path, O_WRONLY|O_CLOEXEC);
> >+                if (fd < 0)
> >+                        log_error_errno(r, "Failed to open %s: %m", arg_kmsg_path);
> >+                r = write(fd, "Y", 1);
> >+                if (r != 1)
> >+                        log_error_errno(r, "Failed to write: %m");
> >+                else
> >+                        log_debug("Set printk.always_kmsg_dump.");
> >+                close(fd);
> >+        }
> >+
> >+        if (arg_crash) {
> >+                /* Only enable if requested; otherwise do not touch the parameter */
> >+                /* NOTE: These errors are not fatal */
> >+                fd = open(arg_crash_path, O_WRONLY|O_CLOEXEC);
> >+                if (fd < 0)
> >+                        log_error_errno(r, "Failed to open %s: %m", arg_crash_path);
> >+                r = write(fd, "Y", 1);
> >+                if (r != 1)
> >+                        log_error_errno(r, "Failed to write: %m");
> >+                else
> >+                        log_debug("Set crash_kexec_post_notifiers.");
> >+                close(fd);
> >+        }
> >+
> >          if (arg_storage == PSTORE_STORAGE_NONE)
> >                  /* Do nothing, intentionally, leaving pstore untouched */
> >                  return 0;

Zbyszek