Re: [PATCH v5 04/13] module: Move livepatch support to a separate file

From: Christophe Leroy
Date: Wed Feb 09 2022 - 13:55:18 EST

Next message: Jiri Olsa: "Re: [PATCH] perf trace: Avoid early exit due SIGCHLD from non-workload processes"
Previous message: Rob Herring: "Re: [PATCH V5 2/6] dt-bindings: mfd: pm8008: Add regulators node"
In reply to: Aaron Tomlin: "[PATCH v5 04/13] module: Move livepatch support to a separate file"
Next in thread: Christophe Leroy: "Re: [PATCH v5 04/13] module: Move livepatch support to a separate file"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Le 09/02/2022 à 18:03, Aaron Tomlin a écrit :
> 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.

LD .tmp_vmlinux.kallsyms1
powerpc64-linux-ld: kernel/livepatch/core.o: in function `klp_enable_patch':
(.text+0x1a0c): undefined reference to `is_livepatch_module'
powerpc64-linux-ld: kernel/module/main.o: in function `free_module':
main.c:(.text+0x50f4): undefined reference to `is_livepatch_module'
powerpc64-linux-ld: kernel/module/main.o: in function `load_module':
main.c:(.text+0x6f20): undefined reference to `is_livepatch_module'
powerpc64-linux-ld: main.c:(.text+0x8390): undefined reference to
`is_livepatch_module'
powerpc64-linux-ld: main.c:(.text+0x8d6c): undefined reference to
`is_livepatch_module'
make: *** [Makefile:1155 : vmlinux] Erreur 1

I don't understand why you are uninlining that so simple function.
Such a function is likely a single instruction in assembly, it is
definitely not worth inlining.

Christophe

>
> Signed-off-by: Aaron Tomlin <atomlin@xxxxxxxxxx>
> ---
> include/linux/module.h | 5 +-
> kernel/module/Makefile | 3 ++
> kernel/module/internal.h | 18 +++++++
> kernel/module/livepatch.c | 80 ++++++++++++++++++++++++++++++
> kernel/module/main.c | 102 ++++----------------------------------
> 5 files changed, 112 insertions(+), 96 deletions(-)
> create mode 100644 kernel/module/livepatch.c
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 1e135fd5c076..680b31ff57fa 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -664,10 +664,7 @@ static inline bool module_requested_async_probing(struct module *module)
> }
>
> #ifdef CONFIG_LIVEPATCH
> -static inline bool is_livepatch_module(struct module *mod)
> -{
> - return mod->klp;
> -}
> +bool is_livepatch_module(struct module *mod);
> #else /* !CONFIG_LIVEPATCH */
> static inline bool is_livepatch_module(struct module *mod)
> {
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index 2902fc7d0ef1..ee20d864ad19 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -7,3 +7,6 @@ obj-$(CONFIG_MODULES) += main.o
> obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o
> obj-$(CONFIG_MODULE_SIG) += signing.o
> obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
> +ifdef CONFIG_MODULES
> +obj-$(CONFIG_LIVEPATCH) += livepatch.o
> +endif
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 1cf5d6dabc97..d252e0af1c54 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -58,6 +58,24 @@ struct load_info {
>
> int mod_verify_sig(const void *mod, struct load_info *info);
>
> +#ifdef CONFIG_LIVEPATCH
> +int copy_module_elf(struct module *mod, struct load_info *info);
> +void free_module_elf(struct module *mod);
> +bool set_livepatch_module(struct module *mod);
> +#else /* !CONFIG_LIVEPATCH */
> +static inline int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> + return 0;
> +}
> +
> +static inline bool set_livepatch_module(struct module *mod)
> +{
> + return false;
> +}
> +
> +static inline void free_module_elf(struct module *mod) { }
> +#endif /* CONFIG_LIVEPATCH */
> +
> #ifdef CONFIG_MODULE_DECOMPRESS
> int module_decompress(struct load_info *info, const void *buf, size_t size);
> void module_decompress_cleanup(struct load_info *info);
> diff --git a/kernel/module/livepatch.c b/kernel/module/livepatch.c
> new file mode 100644
> index 000000000000..7e9cf530c3f0
> --- /dev/null
> +++ b/kernel/module/livepatch.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Module livepatch support
> + *
> + * Copyright (C) 2016 Jessica Yu <jeyu@xxxxxxxxxx>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include "internal.h"
> +
> +/*
> + * Persist Elf information about a module. Copy the Elf header,
> + * section header table, section string table, and symtab section
> + * index from info to mod->klp_info.
> + */
> +int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> + unsigned int size, symndx;
> + int ret;
> +
> + size = sizeof(*mod->klp_info);
> + mod->klp_info = kmalloc(size, GFP_KERNEL);
> + if (mod->klp_info == NULL)
> + return -ENOMEM;
> +
> + /* Elf header */
> + size = sizeof(mod->klp_info->hdr);
> + memcpy(&mod->klp_info->hdr, info->hdr, size);
> +
> + /* Elf section header table */
> + size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
> + mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
> + if (mod->klp_info->sechdrs == NULL) {
> + ret = -ENOMEM;
> + goto free_info;
> + }
> +
> + /* Elf section name string table */
> + size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
> + mod->klp_info->secstrings = kmemdup(info->secstrings, size, GFP_KERNEL);
> + if (mod->klp_info->secstrings == NULL) {
> + ret = -ENOMEM;
> + goto free_sechdrs;
> + }
> +
> + /* Elf symbol section index */
> + symndx = info->index.sym;
> + mod->klp_info->symndx = symndx;
> +
> + /*
> + * For livepatch modules, core_kallsyms.symtab is a complete
> + * copy of the original symbol table. Adjust sh_addr to point
> + * to core_kallsyms.symtab since the copy of the symtab in module
> + * init memory is freed at the end of do_init_module().
> + */
> + mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long) mod->core_kallsyms.symtab;
> +
> + return 0;
> +
> +free_sechdrs:
> + kfree(mod->klp_info->sechdrs);
> +free_info:
> + kfree(mod->klp_info);
> + return ret;
> +}
> +
> +void free_module_elf(struct module *mod)
> +{
> + kfree(mod->klp_info->sechdrs);
> + kfree(mod->klp_info->secstrings);
> + kfree(mod->klp_info);
> +}
> +
> +inline bool set_livepatch_module(struct module *mod)
> +{
> + mod->klp = true;
> + return true;
> +}
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 750e3ad28679..5f5bd7152b55 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2042,81 +2042,6 @@ static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
> }
> #endif /* CONFIG_STRICT_MODULE_RWX */
>
> -#ifdef CONFIG_LIVEPATCH
> -/*
> - * Persist Elf information about a module. Copy the Elf header,
> - * section header table, section string table, and symtab section
> - * index from info to mod->klp_info.
> - */
> -static int copy_module_elf(struct module *mod, struct load_info *info)
> -{
> - unsigned int size, symndx;
> - int ret;
> -
> - size = sizeof(*mod->klp_info);
> - mod->klp_info = kmalloc(size, GFP_KERNEL);
> - if (mod->klp_info == NULL)
> - return -ENOMEM;
> -
> - /* Elf header */
> - size = sizeof(mod->klp_info->hdr);
> - memcpy(&mod->klp_info->hdr, info->hdr, size);
> -
> - /* Elf section header table */
> - size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
> - mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
> - if (mod->klp_info->sechdrs == NULL) {
> - ret = -ENOMEM;
> - goto free_info;
> - }
> -
> - /* Elf section name string table */
> - size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
> - mod->klp_info->secstrings = kmemdup(info->secstrings, size, GFP_KERNEL);
> - if (mod->klp_info->secstrings == NULL) {
> - ret = -ENOMEM;
> - goto free_sechdrs;
> - }
> -
> - /* Elf symbol section index */
> - symndx = info->index.sym;
> - mod->klp_info->symndx = symndx;
> -
> - /*
> - * For livepatch modules, core_kallsyms.symtab is a complete
> - * copy of the original symbol table. Adjust sh_addr to point
> - * to core_kallsyms.symtab since the copy of the symtab in module
> - * init memory is freed at the end of do_init_module().
> - */
> - mod->klp_info->sechdrs[symndx].sh_addr = \
> - (unsigned long) mod->core_kallsyms.symtab;
> -
> - return 0;
> -
> -free_sechdrs:
> - kfree(mod->klp_info->sechdrs);
> -free_info:
> - kfree(mod->klp_info);
> - return ret;
> -}
> -
> -static void free_module_elf(struct module *mod)
> -{
> - kfree(mod->klp_info->sechdrs);
> - kfree(mod->klp_info->secstrings);
> - kfree(mod->klp_info);
> -}
> -#else /* !CONFIG_LIVEPATCH */
> -static int copy_module_elf(struct module *mod, struct load_info *info)
> -{
> - return 0;
> -}
> -
> -static void free_module_elf(struct module *mod)
> -{
> -}
> -#endif /* CONFIG_LIVEPATCH */
> -
> void __weak module_memfree(void *module_region)
> {
> /*
> @@ -3091,30 +3016,23 @@ 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;
> + if (!get_modinfo(info, "livepatch"))
> + /* Nothing more to do */
> + 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;
> -}
> -#else /* !CONFIG_LIVEPATCH */
> -static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
> -{
> - if (get_modinfo(info, "livepatch")) {
> - pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
> - mod->name);
> - return -ENOEXEC;
> + mod->name);
> + return 0;
> }
>
> - return 0;
> + pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
> + mod->name);
> + return -ENOEXEC;
> }
> -#endif /* CONFIG_LIVEPATCH */
>
> static void check_modinfo_retpoline(struct module *mod, struct load_info *info)
> {

Next message: Jiri Olsa: "Re: [PATCH] perf trace: Avoid early exit due SIGCHLD from non-workload processes"
Previous message: Rob Herring: "Re: [PATCH V5 2/6] dt-bindings: mfd: pm8008: Add regulators node"
In reply to: Aaron Tomlin: "[PATCH v5 04/13] module: Move livepatch support to a separate file"
Next in thread: Christophe Leroy: "Re: [PATCH v5 04/13] module: Move livepatch support to a separate file"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]