| 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: <20201109141628.GL1602@alley>
Date: Mon, 9 Nov 2020 15:16:28 +0100
From: Petr Mladek <pmladek@...e.com>
To: Matteo Croce <mcroce@...ux.microsoft.com>
Cc: linux-kernel@...r.kernel.org, Mike Rapoport <rppt@...nel.org>,
Guenter Roeck <linux@...ck-us.net>,
Arnd Bergmann <arnd@...db.de>,
Pavel Tatashin <pasha.tatashin@...een.com>,
Kees Cook <keescook@...omium.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Tyler Hicks <tyhicks@...ux.microsoft.com>
Subject: Re: [PATCH v2] reboot: allow to specify reboot mode via sysfs
On Fri 2020-11-06 21:07:04, Matteo Croce wrote:
> From: Matteo Croce <mcroce@...rosoft.com>
>
> The kernel cmdline reboot= option offers some sort of control
> on how the reboot is issued.
> Add handles in sysfs to allow setting these reboot options, so they
> can be changed when the system is booted, other than at boot time.
>
> The handlers are under <sysfs>/kernel/reboot, can be read to
> get the current configuration and written to alter it.
>
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-kernel-reboot
> @@ -0,0 +1,26 @@
> +What: /sys/kernel/reboot
> +Date: October 2020
> +KernelVersion: 5.11
> +Contact: Matteo Croce <mcroce@...rosoft.com>
> +Description: Interface to set the kernel reboot mode, similarly to
> + what can be done via the reboot= cmdline option.
> + (see Documentation/admin-guide/kernel-parameters.txt)
> +
> +What: /sys/kernel/reboot/mode
> +What: /sys/kernel/reboot/type
> +What: /sys/kernel/reboot/cpu
> +What: /sys/kernel/reboot/force
I do not see any file where it is accumulated this way.
It seems that each path is always described separately.
I am not sure if it is really needed. But it might be needed
when processing the API documentation.
Please, split it.
> +
> +Date: October 2020
> +Contact: Matteo Croce <mcroce@...rosoft.com>
> +Description: Tune reboot parameters.
> +
> + mode: Reboot mode. Valid values are:
> + cold warm hard soft gpio
> +
> + type: Reboot type. Valid values are:
> + bios acpi kbd triple efi pci
> +
> + cpu: CPU number to use to reboot.
> +
> + force: Force an immediate reboot.
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index e7b78d5ae1ab..b9e607517ae3 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -594,3 +594,196 @@ static int __init reboot_setup(char *str)
> return 1;
> }
> __setup("reboot=", reboot_setup);
> +
> +#ifdef CONFIG_SYSFS
> +
> +#define STARTS_WITH(s, sc) (!strncmp(s, sc, sizeof(sc)-1))
> +
> +static ssize_t mode_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> + const char *val;
> +
> + switch (reboot_mode) {
> + case REBOOT_COLD:
> + val = "cold\n";
Using "\n" everywhere is weird. Also the same strings are
repeated in the next functions.
I suggest to define them only once, e.g.
#define REBOOT_COLD_STR "cold"
#define REBOOT_WARM_STR "warm"
and use here:
case REBOOT_COLD:
val = REBOOT_COLD_STR;
and then at the end
return sprintf(buf, "%s\n", val);
> + break;
> + case REBOOT_WARM:
> + val = "warm\n";
> + break;
> + case REBOOT_HARD:
> + val = "hard\n";
> + break;
> + case REBOOT_SOFT:
> + val = "soft\n";
> + break;
> + case REBOOT_GPIO:
> + val = "gpio\n";
> + break;
> + default:
> + val = "undefined\n";
> + }
> +
> + return strscpy(buf, val, 10);
"undefined\n" needs 11 bytes to store also the trailing '\0'.
Anyway, the buffer should be big enough for all variants.
> +}
> +static ssize_t mode_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + if (!capable(CAP_SYS_BOOT))
> + return -EPERM;
> +
> + if (STARTS_WITH(buf, "cold"))
> + reboot_mode = REBOOT_COLD;
I would prefer to open code this and use strlen(). It will be obvious
what the code does immediately. And I am sure that compilators
will optimize out the strlen().
if (strncmp(buf, REBOOT_COLD_STR, strlen(REBOOT_COLD_STR)) == 0)
reboot_mode = REBOOT_COLD;
else if (strncmp(buf, REBOOT_WARM_STR, strlen(REBOOT_WARM_STR) == 0)
reboot_mode = REBOOT_WARM;
...
> + else if (STARTS_WITH(buf, "warm"))
> + reboot_mode = REBOOT_WARM;
> + else if (STARTS_WITH(buf, "hard"))
> + reboot_mode = REBOOT_HARD;
> + else if (STARTS_WITH(buf, "soft"))
> + reboot_mode = REBOOT_SOFT;
> + else if (STARTS_WITH(buf, "gpio"))
> + reboot_mode = REBOOT_GPIO;
> + else
> + return -EINVAL;
> +
> + return count;
> +}
> +static struct kobj_attribute reboot_mode_attr = __ATTR_RW(mode);
> +
> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> + const char *val;
> +
> + switch (reboot_type) {
> + case BOOT_TRIPLE:
> + val = "triple\n";
Same here
var = BOOT_TRIPLE_STR;
> + break;
> + case BOOT_KBD:
> + val = "kbd\n";
> + break;
> + case BOOT_BIOS:
> + val = "bios\n";
> + break;
> + case BOOT_ACPI:
> + val = "acpi\n";
> + break;
> + case BOOT_EFI:
> + val = "efi\n";
> + break;
> + case BOOT_CF9_FORCE:
> + val = "cf9_force\n";
> + break;
> + case BOOT_CF9_SAFE:
> + val = "cf9_safe\n";
> + break;
> + default:
> + val = "undefined\n";
> + }
> +
> + return strscpy(buf, val, 10);
> +}
> +static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + if (!capable(CAP_SYS_BOOT))
> + return -EPERM;
> +
> + if (STARTS_WITH(buf, "triple"))
> + reboot_type = BOOT_TRIPLE;
and here:
if (strncmp(buf, REBOOT_TRIPLE_STR, strlen(REBOOT_TRIPLE_STR)) == 0)
reboot_type = REBOOT_TRIPLE;
> + else if (STARTS_WITH(buf, "kbd"))
> + reboot_type = BOOT_KBD;
> + else if (STARTS_WITH(buf, "bios"))
> + reboot_type = BOOT_BIOS;
> + else if (STARTS_WITH(buf, "acpi"))
> + reboot_type = BOOT_ACPI;
> + else if (STARTS_WITH(buf, "efi"))
> + reboot_type = BOOT_EFI;
> + else if (STARTS_WITH(buf, "cf9_force"))
> + reboot_type = BOOT_CF9_FORCE;
> + else if (STARTS_WITH(buf, "cf9_safe"))
> + reboot_type = BOOT_CF9_SAFE;
> + else
> + return -EINVAL;
> +
> + return count;
> +}
> +static struct kobj_attribute reboot_type_attr = __ATTR_RW(type);
> +
> +static ssize_t force_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%d\n", reboot_force);
> +}
> +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + if (!capable(CAP_SYS_BOOT))
> + return -EPERM;
> +
> + if (buf[0] != '0' && buf[0] != '1')
> + return -EINVAL;
Please use kstrtobool() that supports also other boolean values,
for example, 'Y', 'n'.
> + rc = kstrtouint(buf, 0, &cpunum);
> +
> + reboot_force = buf[0] - '0';
> +
> + return count;
> +}
> +static int __init reboot_ksysfs_init(void)
> +{
> + struct kobject *reboot_kobj;
> + int ret;
> +
> + reboot_kobj = kobject_create_and_add("reboot", kernel_kobj);
> + if (!reboot_kobj)
> + return -ENOMEM;
> +
> + ret = sysfs_create_group(reboot_kobj, &reboot_attr_group);
> + if (ret) {
> + kobject_put(reboot_kobj);
> + return ret;
> + }
> +
> + return 0;
> +}
> +core_initcall(reboot_ksysfs_init);
There is no need to create the sysfs interface this early. In fact, it
might even break because the parent "kernel" node is defined
as core_initcall() as well. The order is not defined in this case.
I would do it as sybsys_initcall() like or even late_initcall().
Best Regards,
Petr
Powered by blists - more mailing lists