* Re: [RFC PATCH v2 03/13] module: Move livepatch support to a separate file
[not found] ` <20220106234319.2067842-4-atomlin@redhat.com>
@ 2022-01-12 16:53 ` Petr Mladek
2022-01-12 18:40 ` David Vernet
2022-01-14 9:14 ` Aaron Tomlin
0 siblings, 2 replies; 3+ messages in thread
From: Petr Mladek @ 2022-01-12 16:53 UTC (permalink / raw)
To: Aaron Tomlin
Cc: mcgrof, cl, mbenes, akpm, jeyu, linux-kernel, linux-modules,
atomlin, ghalat, allen.lkml, live-patching
On Thu 2022-01-06 23:43:09, Aaron Tomlin wrote:
> No functional change.
>
> This patch migrates livepatch support (i.e. used during module
> add/or load and remove/or deletion) from core module code into
> kernel/module/livepatch.c. At the moment it contains code to
> persist Elf information about a given livepatch module, only.
>
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
> kernel/module/Makefile | 1 +
> kernel/module/internal.h | 12 ++++++
> kernel/module/livepatch.c | 75 +++++++++++++++++++++++++++++++++
> kernel/module/main.c | 89 +--------------------------------------
> 4 files changed, 89 insertions(+), 88 deletions(-)
> create mode 100644 kernel/module/livepatch.c
>
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index a9cf6e822075..47d70bb18da3 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -6,3 +6,4 @@
> obj-$(CONFIG_MODULES) += main.o
> obj-$(CONFIG_MODULE_SIG) += signing.o
> obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
> +obj-$(CONFIG_LIVEPATCH) += livepatch.o
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index ffc50df010a7..91ef152aeffb 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -51,3 +51,15 @@ struct load_info {
> };
>
> extern int mod_verify_sig(const void *mod, struct load_info *info);
> +
> +#ifdef CONFIG_LIVEPATCH
> +extern int copy_module_elf(struct module *mod, struct load_info *info);
> +extern void free_module_elf(struct module *mod);
> +extern int check_modinfo_livepatch(struct module *mod, struct load_info *info);
> +#else /* !CONFIG_LIVEPATCH */
> +static inline int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> + return 0;
> +}
> +static inline void free_module_elf(struct module *mod) { }
It looks like there is no check_modinfo_livepatch() variant when
CONFIG_LIPATCH is disabled.
> +#endif /* CONFIG_LIVEPATCH */
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 2a6b859716c0..9bcaf251e109 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3052,19 +2977,7 @@ static int copy_chunked_from_user(void *dst, const void __user *usrc, unsigned l
> return 0;
> }
>
> -#ifdef CONFIG_LIVEPATCH
> -static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
> -{
> - if (get_modinfo(info, "livepatch")) {
> - mod->klp = true;
> - add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
> - pr_notice_once("%s: tainting kernel with TAINT_LIVEPATCH\n",
> - mod->name);
> - }
> -
> - return 0;
> -}
> -#else /* !CONFIG_LIVEPATCH */
> +#ifndef CONFIG_LIVEPATCH
> static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
> {
> if (get_modinfo(info, "livepatch")) {
But it exist here.
It would be better to have the two variants close each other. I mean
to have it somewhere like:
#ifdef CONFIG_LIVEPATCH
variant A
#else
variant B
#endif
A solution would be to do it a similar way like in
check_modinfo_retpoline(). Have a generic:
static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
{
if (!get_modinfo(info, "livepatch"))
return 0;
if (set_livepatch_module(mod)) {
add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
pr_notice_once("%s: tainting kernel with TAINT_LIVEPATCH\n",
mod->name);
return 0;
}
pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
mod->name);
return -ENOEXEC;
}
, where set_livepatch_module(mod) might be defined inline
similar way like is_livepatch_module():
#ifdef CONFIG_LIVEPATCH
static inline bool set_livepatch_module(struct module *mod)
{
mod->klp = true;
return true;
}
#else /* !CONFIG_LIVEPATCH */
static inline bool set_livepatch_module(struct module *mod)
{
return false;
}
#endif /* CONFIG_LIVEPATCH */
Well, it might be matter of taste. Others might prefer another solution.
Adding live-patching mailing list into Cc.
Anyway, if we do any code refactoring, we should do it in a separate
preparatory patch.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 3+ messages in thread