| 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: <Z9wan08CpbvddHhc@smile.fi.intel.com>
Date: Thu, 20 Mar 2025 15:39:43 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Pasha Tatashin <pasha.tatashin@...een.com>
Cc: changyuanl@...gle.com, graf@...zon.com, rppt@...nel.org,
rientjes@...gle.com, corbet@....net, rdunlap@...radead.org,
ilpo.jarvinen@...ux.intel.com, kanie@...ux.alibaba.com,
ojeda@...nel.org, aliceryhl@...gle.com, masahiroy@...nel.org,
akpm@...ux-foundation.org, tj@...nel.org, yoann.congal@...le.fr,
mmaurer@...gle.com, roman.gushchin@...ux.dev, chenridong@...wei.com,
axboe@...nel.dk, mark.rutland@....com, jannh@...gle.com,
vincent.guittot@...aro.org, hannes@...xchg.org,
dan.j.williams@...el.com, david@...hat.com,
joel.granados@...nel.org, rostedt@...dmis.org,
anna.schumaker@...cle.com, song@...nel.org, zhangguopeng@...inos.cn,
linux@...ssschuh.net, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, linux-mm@...ck.org,
gregkh@...uxfoundation.org, tglx@...utronix.de, mingo@...hat.com,
bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org,
hpa@...or.com, rafael@...nel.org, dakr@...nel.org,
bartosz.golaszewski@...aro.org, cw00.choi@...sung.com,
myungjoo.ham@...sung.com, yesanishhere@...il.com,
Jonathan.Cameron@...wei.com, quic_zijuhu@...cinc.com,
aleksander.lobakin@...el.com, ira.weiny@...el.com, leon@...nel.org,
lukas@...ner.de, bhelgaas@...gle.com, wagi@...nel.org,
djeffery@...hat.com, stuart.w.hayes@...il.com, jgowans@...zon.com,
jgg@...dia.com
Subject: Re: [RFC v1 1/3] luo: Live Update Orchestrator
On Thu, Mar 20, 2025 at 02:40:09AM +0000, Pasha Tatashin wrote:
> Introduces the Live Update Orchestrator (LUO), a new kernel subsystem
> designed to facilitate live updates. Live update is a method to reboot
> the kernel while attempting to keep selected devices alive across the
> reboot boundary, minimizing downtime.
>
> The primary use case is cloud environments, allowing hypervisor updates
> without fully disrupting running virtual machines. VMs can be suspended
> while the hypervisor kernel reboots, and devices attached to these VM
> are kept operational by the LUO.
>
> Features introduced:
>
> - Core orchestration logic for managing the live update process.
> - A state machine (NORMAL, PREPARED, UPDATED, *_FAILED) to track
> the progress of live updates.
> - Notifier chains for subsystems (device layer, interrupts, KVM, IOMMU,
> etc.) to register callbacks for different live update events:
> - LIVEUPDATE_PREPARE: Prepare for reboot (before blackout).
> - LIVEUPDATE_REBOOT: Final serialization before kexec (blackout).
> - LIVEUPDATE_FINISH: Cleanup after update (after blackout).
> - LIVEUPDATE_CANCEL: Rollback actions on failure or user request.
> - A sysfs interface (/sys/kernel/liveupdate/) for user-space control:
> - `prepare`: Initiate preparation (write 1) or reset (write 0).
> - `finish`: Finalize update in new kernel (write 1).
> - `cancel`: Abort ongoing preparation or reboot (write 1).
> - `reset`: Force state back to normal (write 1).
> - `state`: Read-only view of the current LUO state.
> - `enabled`: Read-only view of whether live update is enabled.
> - Integration with KHO to pass orchestrator state to the new kernel.
> - Version checking during startup of the new kernel to ensure
> compatibility with the previous kernel's live update state.
>
> This infrastructure allows various kernel subsystems to coordinate and
> participate in the live update process, serializing and restoring device
> state across a kernel reboot.
...
> +Date: March 2025
> +KernelVersion: 6.14.0
This is way too optimistic, it even won't make v6.15.
And date can be chosen either v6.16-rc1 or v6.16 release
in accordance with prediction tool
...
> +#ifndef _LINUX_LIVEUPDATE_H
> +#define _LINUX_LIVEUPDATE_H
> +#include <linux/compiler.h>
> +#include <linux/notifier.h>
This is semi-random list of inclusions. Try to follow IWYU principle.
See below.
...
> +bool liveupdate_state_updated(void);
Where bool is defined?
...
> +/**
> + * LIVEUPDATE_DECLARE_NOTIFIER - Declare a live update notifier with default
> + * structure.
> + * @_name: A base name used to generate the names of the notifier block
> + * (e.g., ``_name##_liveupdate_notifier_block``) and the callback function
> + * (e.g., ``_name##_liveupdate``).
> + * @_priority: The priority of the notifier, specified using the
> + * ``enum liveupdate_cb_priority`` values
> + * (e.g., ``LIVEUPDATE_CB_PRIO_BEFORE_DEVICES``).
> + *
> + * This macro declares a static struct notifier_block and a corresponding
> + * notifier callback function for use with the live update orchestrator.
> + * It simplifies the process by automatically handling the dispatching of
> + * live update events to separate handler functions for prepare, reboot,
> + * finish, and cancel.
> + *
> + * This macro expects the following functions to be defined:
> + *
> + * ``_name##_liveupdate_prepare()``: Called on LIVEUPDATE_PREPARE.
> + * ``_name##_liveupdate_reboot()``: Called on LIVEUPDATE_REBOOT.
> + * ``_name##_liveupdate_finish()``: Called on LIVEUPDATE_FINISH.
> + * ``_name##_liveupdate_cancel()``: Called on LIVEUPDATE_CANCEL.
> + *
> + * The generated callback function handles the switch statement for the
> + * different live update events and calls the appropriate handler function.
> + * It also includes warnings if the finish or cancel handlers return an error.
> + *
> + * For example, declartion can look like this:
> + *
> + * ``static int foo_liveupdate_prepare(void) { ... }``
> + *
> + * ``static int foo_liveupdate_reboot(void) { ... }``
> + *
> + * ``static int foo_liveupdate_finish(void) { ... }``
> + *
> + * ``static int foo_liveupdate_cancel(void) { ... }``
> + *
> + * ``LIVEUPDATE_DECLARE_NOTIFIER(foo, LIVEUPDATE_CB_PRIO_WITH_DEVICES);``
> + *
Hmm... Have you run kernel-doc validator? There is missing Return section and
it will warn about that.
> + */
...
> + WARN_ONCE(rv, "cancel failed[%d]\n", rv); \
+ bug.h
...
> + #undef pr_fmt
> + #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Leftover from the development?
> +#undef pr_fmt
Not needed as long as pr_fmt9) is at the top of the file.
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
...
> +#include <linux/kernel.h>
What for? Can you follow IWYU, please? Here again semi-random list of
inclusions.
> +#include <linux/sysfs.h>
> +#include <linux/string.h>
> +#include <linux/rwsem.h>
> +#include <linux/err.h>
> +#include <linux/liveupdate.h>
> +#include <linux/cpufreq.h>
> +#include <linux/kexec_handover.h>
Can you keep them ordered which will be easier to read and maintain?
...
> +static int __init early_liveupdate_param(char *buf)
> +{
> + return kstrtobool(buf, &luo_enabled);
> +}
> +
Redundant blank line.
> +early_param("liveupdate", early_liveupdate_param);
...
> +/* Show the current live update state */
> +static ssize_t state_show(struct kobject *kobj,
> + struct kobj_attribute *attr,
It is still enough room even for the strict 80 limit case.
> + char *buf)
> +{
> + return sysfs_emit(buf, "%s\n", LUO_STATE_STR);
> +}
...
> + ret = blocking_notifier_call_chain(&luo_notify_list,
> + event,
> + NULL);
There is room on the previous lines. Ditto for the similar cases all over
the code.
...
> +{
> + int ret;
> +
> + if (down_write_killable(&luo_state_rwsem)) {
> + pr_warn(" %s, change state canceled by user\n", __func__);
Why __func__ is so important in _this_ message? And why leading space?
Ditto for the similar cases.
> + return -EAGAIN;
> + }
> +
> + if (!IS_STATE(LIVEUPDATE_STATE_NORMAL)) {
> + pr_warn("Can't switch to [%s] from [%s] state\n",
> + luo_state_str[LIVEUPDATE_STATE_PREPARED],
> + LUO_STATE_STR);
> + up_write(&luo_state_rwsem);
> +
> + return -EINVAL;
> + }
> +
> + ret = luo_notify(LIVEUPDATE_PREPARE);
> + if (!ret)
> + luo_set_state(LIVEUPDATE_STATE_PREPARED);
> +
> + up_write(&luo_state_rwsem);
> +
> + return ret;
> +}
...
> +static ssize_t prepare_store(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + ssize_t ret;
> + long val;
> + if (kstrtol(buf, 0, &val) < 0)
> + return -EINVAL;
Shadower error code.
> + if (val != 1 && val != 0)
> + return -EINVAL;
What's wrong with using kstrtobool() from the beginning?
> +
> + if (val)
> + ret = luo_prepare();
> + else
> + ret = luo_cancel();
> + if (!ret)
> + ret = count;
> +
> + return ret;
Can we go with the usual pattern "check for errors first"?
if (ret)
return ret;
...
> +}
...
> +static ssize_t finish_store(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf,
> + size_t count)
Same comments as per above.
...
> +static struct attribute *luo_attrs[] = {
> + &state_attribute.attr,
> + &prepare_attribute.attr,
> + &finish_attribute.attr,
> + NULL,
No comma for the terminator entry.
> +};
...
> +static int __init luo_init(void)
> +{
> + int ret;
> +
> + if (!luo_enabled || !kho_is_enabled()) {
> + pr_info("disabled by user\n");
> + luo_enabled = false;
> +
> + return 0;
> + }
Can be written like
if (!kho_is_enabled())
luo_enabled = false;
if (!luo_enabled) {
pr_info("disabled by user\n");
return 0;
}
> + ret = sysfs_create_group(kernel_kobj, &luo_attr_group);
> + if (ret)
> + pr_err("Failed to create group\n");
> +
> + luo_sysfs_initialized = true;
> + pr_info("Initialized\n");
> +
> + return ret;
Something is odd here between (non-)checking for errors and printed messages.
> +}
...
> +EXPORT_SYMBOL_GPL(liveupdate_state_normal);
No namespace?
...
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -18,6 +18,7 @@
> #include <linux/syscalls.h>
> #include <linux/syscore_ops.h>
> #include <linux/uaccess.h>
> +#include <linux/liveupdate.h>
Can oyu preserve order (with given context at least)?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists