On Tue, 20 Dec 2022 23:05:45 +0100
Ricardo Ribalda <ribalda@xxxxxxxxxxxx> wrote:
I hate to be the grammar police, but..
> Add two parameter to specify how many times a kexec kernel can be loaded.
"parameters"
>
> The sysadmin can set different limits for kexec panic and kexec reboot
> kernels.
>
> The value can be modified at runtime via sysfs, but only with a value
> smaller than the current one (except -1).
>
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 14 ++++
> include/linux/kexec.h | 2 +-
> kernel/kexec.c | 2 +-
> kernel/kexec_core.c | 91 ++++++++++++++++++++++++-
> kernel/kexec_file.c | 2 +-
> 5 files changed, 106 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 42af9ca0127e..2b37d6a20747 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2374,6 +2374,20 @@
> for Movable pages. "nn[KMGTPE]", "nn%", and "mirror"
> are exclusive, so you cannot specify multiple forms.
>
> + kexec_core.load_limit_reboot=
> + kexec_core.load_limit_panic=
> + [KNL]
> + This parameter specifies a limit to the number of times
> + a kexec kernel can be loaded.
> + Format: <int>
> + -1 = Unlimited.
> + int = Number of times kexec can be called.
> +
> + During runtime, this parameter can be modified with a
> + value smaller than the current one (but not -1).
Perhaps state:
smaller positive value than the current one or if
current is currently -1.
> +
> + Default: -1
> +
> kgdbdbgp= [KGDB,HW] kgdb over EHCI usb debug port.
> Format: <Controller#>[,poll interval]
> The controller # is the number of the ehci usb debug
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 182e0c11b87b..5daf9990d5b8 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -407,7 +407,7 @@ extern int kimage_crash_copy_vmcoreinfo(struct kimage *image);
> extern struct kimage *kexec_image;
> extern struct kimage *kexec_crash_image;
>
> -bool kexec_load_permitted(void);
> +bool kexec_load_permitted(bool crash_image);
>
> #ifndef kexec_flush_icache_page
> #define kexec_flush_icache_page(page)
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index ce1bca874a8d..7aefd134e319 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -193,7 +193,7 @@ static inline int kexec_load_check(unsigned long nr_segments,
> int result;
>
> /* We only trust the superuser with rebooting the system. */
> - if (!kexec_load_permitted())
> + if (!kexec_load_permitted(flags & KEXEC_ON_CRASH))
Note, here we have KEXEC_ON_CRASH (see bottom).
> return -EPERM;
>
> /* Permit LSMs and IMA to fail the kexec */
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index a1efc70f4158..adf71f2be3ff 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -952,13 +952,100 @@ static int __init kexec_core_sysctl_init(void)
> late_initcall(kexec_core_sysctl_init);
> #endif
>
> -bool kexec_load_permitted(void)
> +struct kexec_load_limit {
> + /* Mutex protects the limit count. */
> + struct mutex mutex;
> + int limit;
> +};
> +
> +struct kexec_load_limit load_limit_reboot = {
Perhaps make the above static?
> + .mutex = __MUTEX_INITIALIZER(load_limit_reboot.mutex),
> + .limit = -1,
> +};
> +
> +struct kexec_load_limit load_limit_panic = {
static?
> + .mutex = __MUTEX_INITIALIZER(load_limit_panic.mutex),
> + .limit = -1,
> +};
> +
> +static int param_get_limit(char *buffer, const struct kernel_param *kp)
> {
> + int ret;
> + struct kexec_load_limit *limit = kp->arg;
Looks better if "int ret;" is after the "limit".
> +
> + mutex_lock(&limit->mutex);
> + ret = scnprintf(buffer, PAGE_SIZE, "%i\n", limit->limit);
The above string can be at most "-2147483648\n\0"
Which is 13 characters. Why use PAGE_SIZE. Or scnprintf(), and not just
state:
/* buffer is PAGE_SIZE, much larger than what %i can be */
ret = sprintf(buffer, "%i\n", limit->limit);
> + mutex_unlock(&limit->mutex);
> +
> + return ret;
> +}
> +
> +static int param_set_limit(const char *buffer, const struct kernel_param *kp)
> +{
> + int ret;
> + struct kexec_load_limit *limit = kp->arg;
> + int new_val;
> +
> + ret = kstrtoint(buffer, 0, &new_val);
> + if (ret)
> + return ret;
> +
> + new_val = max(-1, new_val);
I wonder if anything less than -1 should be invalid.
> +
> + mutex_lock(&limit->mutex);
> +
> + if (new_val == -1 && limit->limit != -1) {
If -1 can't change the value, why allow it to be passed in to begin with.
Perhaps we should only allow sysctl to set positive values? Would make the
code simpler.
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + if (limit->limit != -1 && new_val > limit->limit) {
Since the above documentation said "small than" perhaps ">="?
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + limit->limit = new_val;
> +
> +done:
> + mutex_unlock(&limit->mutex);
> +
> + return ret;
> +}
> +
> +static const struct kernel_param_ops load_limit_ops = {
> + .get = param_get_limit,
> + .set = param_set_limit,
> +};
> +
> +module_param_cb(load_limit_reboot, &load_limit_ops, &load_limit_reboot, 0644);
> +MODULE_PARM_DESC(load_limit_reboot, "Maximum attempts to load a kexec reboot kernel");
> +
> +module_param_cb(load_limit_panic, &load_limit_ops, &load_limit_panic, 0644);
> +MODULE_PARM_DESC(load_limit_reboot, "Maximum attempts to load a kexec panic kernel");
Wait, why the module params if this can not be a module?
The kernel/kexec.c is decided via CONFIG_KEXEC_CORE which is bool. Either
builtin or not at all. No module selection possible.
For kernel parameters, we should just use __setup(), right?
> +
> +bool kexec_load_permitted(bool crash_image)
> +{
> + struct kexec_load_limit *limit;
> +
> /*
> * Only the superuser can use the kexec syscall and if it has not
> * been disabled.
> */
> - return capable(CAP_SYS_BOOT) && !kexec_load_disabled;
> + if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> + return false;
> +
> + /* Check limit counter and decrease it.*/
> + limit = crash_image ? &load_limit_panic : &load_limit_reboot;
> + mutex_lock(&limit->mutex);
> + if (!limit->limit) {
> + mutex_unlock(&limit->mutex);
> + return false;
> + }
> + if (limit->limit != -1)
> + limit->limit--;
> + mutex_unlock(&limit->mutex);
> +
> + return true;
> }
>
> /*
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 29efa43ea951..6a1d4b07635e 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -330,7 +330,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> struct kimage **dest_image, *image;
>
> /* We only trust the superuser with rebooting the system. */
> - if (!kexec_load_permitted())
> + if (!kexec_load_permitted(flags & KEXEC_FILE_FLAGS))
Here we have KEXEC_FILE_FLAGS, where above it was KEXCE_FILE_CRASH.
This is confusing to what denotes the "crash_image" boolean. Can we just
pass in flags and figure it out in the kexec_load_permitted() function?
-- Steve
> return -EPERM;
>
> /* Make sure we have a legal set of flags */
>
