Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190221230641.8797-1-aaro.koskinen@iki.fi>
In-Reply-To: <20190221230641.8797-1-aaro.koskinen@iki.fi>
From:   Kees Cook <keescook@chromium.org>
Date:   Thu, 21 Feb 2019 15:23:29 -0800
Message-ID: <CAGXu5jLFwHSCVqJTGbr=HjeaWsW-TZ_1k=ubjEYgDQBxOV9nJg@mail.gmail.com>
Subject: Re: [PATCH v3] panic/reboot: allow specifying reboot_mode for panic only
To:     Aaro Koskinen <aaro.koskinen@iki.fi>,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Aaro Koskinen <aaro.koskinen@nokia.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Feb 21, 2019 at 3:06 PM Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
>
> From: Aaro Koskinen <aaro.koskinen@nokia.com>
>
> Allow specifying reboot_mode for panic only. This is needed on systems
> where ramoops is used to store panic logs, and user wants to use warm
> reset to preserve those, while still having cold reset on normal reboots.
>
> Signed-off-by: Aaro Koskinen <aaro.koskinen@nokia.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>
>         v3: Move REBOOT_UNDEFINED before REBOOT_COLD.
>
>         v2: https://marc.info/?t=155078129700004&r=1&w=2
>
>             Use REBOOT_UNDEFINED instead of -1
>
>         v1: https://marc.info/?t=154957416600005&r=1&w=2
>
>  .../admin-guide/kernel-parameters.txt         |  4 +++-
>  include/linux/reboot.h                        |  2 ++
>  kernel/panic.c                                |  2 ++
>  kernel/reboot.c                               | 20 ++++++++++++++-----
>  4 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 858b6c0b9a15..5705b4689565 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3949,7 +3949,9 @@
>                                 [[,]s[mp]#### \
>                                 [[,]b[ios] | a[cpi] | k[bd] | t[riple] | e[fi] | p[ci]] \
>                                 [[,]f[orce]
> -                       Where reboot_mode is one of warm (soft) or cold (hard) or gpio,
> +                       Where reboot_mode is one of warm (soft) or cold (hard) or gpio
> +                                       (prefix with 'panic_' to set mode for panic
> +                                       reboot only),
>                               reboot_type is one of bios, acpi, kbd, triple, efi, or pci,
>                               reboot_force is either force or not specified,
>                               reboot_cpu is s[mp]#### with #### being the processor
> diff --git a/include/linux/reboot.h b/include/linux/reboot.h
> index e63799a6e895..3734cd8f38a8 100644
> --- a/include/linux/reboot.h
> +++ b/include/linux/reboot.h
> @@ -14,6 +14,7 @@ struct device;
>  #define SYS_POWER_OFF  0x0003  /* Notify of system power off */
>
>  enum reboot_mode {
> +       REBOOT_UNDEFINED = -1,
>         REBOOT_COLD = 0,
>         REBOOT_WARM,
>         REBOOT_HARD,
> @@ -21,6 +22,7 @@ enum reboot_mode {
>         REBOOT_GPIO,
>  };
>  extern enum reboot_mode reboot_mode;
> +extern enum reboot_mode panic_reboot_mode;
>
>  enum reboot_type {
>         BOOT_TRIPLE     = 't',
> diff --git a/kernel/panic.c b/kernel/panic.c
> index f121e6ba7e11..a0d839f883b7 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -306,6 +306,8 @@ void panic(const char *fmt, ...)
>                  * shutting down.  But if there is a chance of
>                  * rebooting the system it will be rebooted.
>                  */
> +               if (panic_reboot_mode != REBOOT_UNDEFINED)
> +                       reboot_mode = panic_reboot_mode;
>                 emergency_restart();
>         }
>  #ifdef __sparc__
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index e1b79b6a2735..b9e79e8c7226 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -31,6 +31,7 @@ EXPORT_SYMBOL(cad_pid);
>  #define DEFAULT_REBOOT_MODE
>  #endif
>  enum reboot_mode reboot_mode DEFAULT_REBOOT_MODE;
> +enum reboot_mode panic_reboot_mode = REBOOT_UNDEFINED;
>
>  /*
>   * This variable is used privately to keep track of whether or not
> @@ -519,6 +520,8 @@ EXPORT_SYMBOL_GPL(orderly_reboot);
>  static int __init reboot_setup(char *str)
>  {
>         for (;;) {
> +               enum reboot_mode *mode;
> +
>                 /*
>                  * Having anything passed on the command line via
>                  * reboot= will cause us to disable DMI checking
> @@ -526,17 +529,24 @@ static int __init reboot_setup(char *str)
>                  */
>                 reboot_default = 0;
>
> +               if (!strncmp(str, "panic_", 6)) {
> +                       mode = &panic_reboot_mode;
> +                       str += 6;
> +               } else {
> +                       mode = &reboot_mode;
> +               }
> +
>                 switch (*str) {
>                 case 'w':
> -                       reboot_mode = REBOOT_WARM;
> +                       *mode = REBOOT_WARM;
>                         break;
>
>                 case 'c':
> -                       reboot_mode = REBOOT_COLD;
> +                       *mode = REBOOT_COLD;
>                         break;
>
>                 case 'h':
> -                       reboot_mode = REBOOT_HARD;
> +                       *mode = REBOOT_HARD;
>                         break;
>
>                 case 's':
> @@ -553,11 +563,11 @@ static int __init reboot_setup(char *str)
>                                 if (rc)
>                                         return rc;
>                         } else
> -                               reboot_mode = REBOOT_SOFT;
> +                               *mode = REBOOT_SOFT;
>                         break;
>                 }
>                 case 'g':
> -                       reboot_mode = REBOOT_GPIO;
> +                       *mode = REBOOT_GPIO;
>                         break;
>
>                 case 'b':
> --
> 2.17.0
>


-- 
Kees Cook