| 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: <CAFhGd8puRTCYw2MhecmgW4SQR-_YPyBSy_fbgvE_1c1Onh0veg@mail.gmail.com>
Date: Wed, 13 Sep 2023 12:30:45 -0700
From: Justin Stitt <justinstitt@...gle.com>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Steve Wahl <steve.wahl@....com>,
Justin Ernst <justin.ernst@....com>,
Kyle Meyer <kyle.meyer@....com>,
Dimitri Sivanich <dimitri.sivanich@....com>,
Russ Anderson <russ.anderson@....com>,
Darren Hart <dvhart@...radead.org>,
Andy Shevchenko <andy@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>,
"H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] x86/platform/uv: Rework NMI "action" modparam handling
On Wed, Sep 13, 2023 at 11:01 AM Hans de Goede <hdegoede@...hat.com> wrote:
>
> Rework NMI "action" modparam handling:
>
> 1. Replace the uv_nmi_action string with an enum; and
> 2. Use sysfs_match_string() for string parsing in param_set_action()
>
> Suggested-by: Steve Wahl <steve.wahl@....com>
> Cc: Justin Stitt <justinstitt@...gle.com>
> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
> ---
Reviewed-by: Justin Stitt <justinstitt@...gle.com>
> Changes in v2:
> - Also change uv_nmi_action to an enum to replace a bunch of
> strcmp() calls
> ---
> arch/x86/platform/uv/uv_nmi.c | 104 +++++++++++++++++++---------------
> 1 file changed, 57 insertions(+), 47 deletions(-)
>
> diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
> index 45d0c17ce77c..b92f1b4adeb0 100644
> --- a/arch/x86/platform/uv/uv_nmi.c
> +++ b/arch/x86/platform/uv/uv_nmi.c
> @@ -178,49 +178,57 @@ module_param_named(debug, uv_nmi_debug, int, 0644);
> } while (0)
>
> /* Valid NMI Actions */
> -#define ACTION_LEN 16
> -static struct nmi_action {
> - char *action;
> - char *desc;
> -} valid_acts[] = {
> - { "kdump", "do kernel crash dump" },
> - { "dump", "dump process stack for each cpu" },
> - { "ips", "dump Inst Ptr info for each cpu" },
> - { "kdb", "enter KDB (needs kgdboc= assignment)" },
> - { "kgdb", "enter KGDB (needs gdb target remote)" },
> - { "health", "check if CPUs respond to NMI" },
> +enum action_t {
> + nmi_act_kdump,
> + nmi_act_dump,
> + nmi_act_ips,
> + nmi_act_kdb,
> + nmi_act_kgdb,
> + nmi_act_health,
> };
> -typedef char action_t[ACTION_LEN];
> -static action_t uv_nmi_action = { "dump" };
> +
> +static const char * const actions[] = {
> + [nmi_act_kdump] = "kdump",
> + [nmi_act_dump] = "dump",
> + [nmi_act_ips] = "ips",
> + [nmi_act_kdb] = "kdb",
> + [nmi_act_kgdb] = "kgdb",
> + [nmi_act_health] = "health",
> +};
> +
> +static const char * const actions_desc[] = {
> + [nmi_act_kdump] = "do kernel crash dump",
> + [nmi_act_dump] = "dump process stack for each cpu",
> + [nmi_act_ips] = "dump Inst Ptr info for each cpu",
> + [nmi_act_kdb] = "enter KDB (needs kgdboc= assignment)",
> + [nmi_act_kgdb] = "enter KGDB (needs gdb target remote)",
> + [nmi_act_health] = "check if CPUs respond to NMI",
> +};
> +
> +static_assert(ARRAY_SIZE(actions) == ARRAY_SIZE(actions_desc));
> +
> +static enum action_t uv_nmi_action = nmi_act_dump;
>
> static int param_get_action(char *buffer, const struct kernel_param *kp)
> {
> - return sprintf(buffer, "%s\n", uv_nmi_action);
> + return sprintf(buffer, "%s\n", actions[uv_nmi_action]);
> }
>
> static int param_set_action(const char *val, const struct kernel_param *kp)
> {
> - int i;
> - int n = ARRAY_SIZE(valid_acts);
> - char arg[ACTION_LEN];
> + int i, n = ARRAY_SIZE(actions);
>
> - /* (remove possible '\n') */
> - strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1);
> -
> - for (i = 0; i < n; i++)
> - if (!strcmp(arg, valid_acts[i].action))
> - break;
> -
> - if (i < n) {
> - strscpy(uv_nmi_action, arg, sizeof(uv_nmi_action));
> - pr_info("UV: New NMI action:%s\n", uv_nmi_action);
> + i = sysfs_match_string(actions, val);
> + if (i >= 0) {
> + uv_nmi_action = i;
> + pr_info("UV: New NMI action:%s\n", actions[i]);
> return 0;
> }
>
> - pr_err("UV: Invalid NMI action:%s, valid actions are:\n", arg);
> + pr_err("UV: Invalid NMI action:%s, valid actions are:\n", val);
> for (i = 0; i < n; i++)
> - pr_err("UV: %-8s - %s\n",
> - valid_acts[i].action, valid_acts[i].desc);
> + pr_err("UV: %-8s - %s\n", actions[i], actions_desc[i]);
> +
> return -EINVAL;
> }
>
> @@ -228,15 +236,10 @@ static const struct kernel_param_ops param_ops_action = {
> .get = param_get_action,
> .set = param_set_action,
> };
> -#define param_check_action(name, p) __param_check(name, p, action_t)
> +#define param_check_action(name, p) __param_check(name, p, enum action_t)
>
> module_param_named(action, uv_nmi_action, action, 0644);
>
> -static inline bool uv_nmi_action_is(const char *action)
> -{
> - return (strncmp(uv_nmi_action, action, strlen(action)) == 0);
> -}
> -
> /* Setup which NMI support is present in system */
> static void uv_nmi_setup_mmrs(void)
> {
> @@ -727,10 +730,10 @@ static void uv_nmi_dump_state_cpu(int cpu, struct pt_regs *regs)
> if (cpu == 0)
> uv_nmi_dump_cpu_ip_hdr();
>
> - if (current->pid != 0 || !uv_nmi_action_is("ips"))
> + if (current->pid != 0 || uv_nmi_action != nmi_act_ips)
> uv_nmi_dump_cpu_ip(cpu, regs);
>
> - if (uv_nmi_action_is("dump")) {
> + if (uv_nmi_action == nmi_act_dump) {
> pr_info("UV:%sNMI process trace for CPU %d\n", dots, cpu);
> show_regs(regs);
> }
> @@ -798,7 +801,7 @@ static void uv_nmi_dump_state(int cpu, struct pt_regs *regs, int master)
> int saved_console_loglevel = console_loglevel;
>
> pr_alert("UV: tracing %s for %d CPUs from CPU %d\n",
> - uv_nmi_action_is("ips") ? "IPs" : "processes",
> + uv_nmi_action == nmi_act_ips ? "IPs" : "processes",
> atomic_read(&uv_nmi_cpus_in_nmi), cpu);
>
> console_loglevel = uv_nmi_loglevel;
> @@ -874,7 +877,7 @@ static inline int uv_nmi_kdb_reason(void)
> static inline int uv_nmi_kdb_reason(void)
> {
> /* Ensure user is expecting to attach gdb remote */
> - if (uv_nmi_action_is("kgdb"))
> + if (uv_nmi_action == nmi_act_kgdb)
> return 0;
>
> pr_err("UV: NMI error: KDB is not enabled in this kernel\n");
> @@ -950,28 +953,35 @@ static int uv_handle_nmi(unsigned int reason, struct pt_regs *regs)
> master = (atomic_read(&uv_nmi_cpu) == cpu);
>
> /* If NMI action is "kdump", then attempt to do it */
> - if (uv_nmi_action_is("kdump")) {
> + if (uv_nmi_action == nmi_act_kdump) {
> uv_nmi_kdump(cpu, master, regs);
>
> /* Unexpected return, revert action to "dump" */
> if (master)
> - strscpy(uv_nmi_action, "dump", sizeof(uv_nmi_action));
> + uv_nmi_action = nmi_act_dump;
> }
>
> /* Pause as all CPU's enter the NMI handler */
> uv_nmi_wait(master);
>
> /* Process actions other than "kdump": */
> - if (uv_nmi_action_is("health")) {
> + switch (uv_nmi_action) {
> + case nmi_act_health:
> uv_nmi_action_health(cpu, regs, master);
> - } else if (uv_nmi_action_is("ips") || uv_nmi_action_is("dump")) {
> + break;
> + case nmi_act_ips:
> + case nmi_act_dump:
> uv_nmi_dump_state(cpu, regs, master);
> - } else if (uv_nmi_action_is("kdb") || uv_nmi_action_is("kgdb")) {
> + break;
> + case nmi_act_kdb:
> + case nmi_act_kgdb:
> uv_call_kgdb_kdb(cpu, regs, master);
> - } else {
> + break;
> + default:
> if (master)
> - pr_alert("UV: unknown NMI action: %s\n", uv_nmi_action);
> + pr_alert("UV: unknown NMI action: %d\n", uv_nmi_action);
> uv_nmi_sync_exit(master);
> + break;
> }
>
> /* Clear per_cpu "in_nmi" flag */
> --
> 2.41.0
>
Powered by blists - more mailing lists