| Message ID | 20220106234319.2067842-4-atomlin@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | module: core code clean up | expand |
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
Petr Mladek <pmladek@suse.com> wrote on Wed [2022-Jan-12 17:53:56 +0100]: > 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 > <snip> > #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. +1 -- this seems like a cleaner approach. - David
Thanks for doing this refactor. +1 to doing this, though Petr had some suggestions in another thread that I'll wait on before Acking. Aaron Tomlin <atomlin@redhat.com> wrote on Thu [2022-Jan-06 23:43:09 +0000]: > diff --git a/kernel/module/livepatch.c b/kernel/module/livepatch.c > new file mode 100644 > index 000000000000..e147f5418327 > --- /dev/null > +++ b/kernel/module/livepatch.c > @@ -0,0 +1,75 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * kernel/module/livepatch.c - module livepatch support > + * > + * Copyright (C) 2016 Jessica Yu <jeyu@redhat.com> > + */ Should the copyright year (and possibly author) be updated? Or just removed entirely? - David
On Wed 2022-01-12 10:54 -0800, David Vernet wrote: > Thanks for doing this refactor. +1 to doing this, though Petr had some > suggestions in another thread that I'll wait on before Acking. No problem and yes, I will make the suggested modifications then add you on Cc. > Aaron Tomlin <atomlin@redhat.com> wrote on Thu [2022-Jan-06 23:43:09 +0000]: > > diff --git a/kernel/module/livepatch.c b/kernel/module/livepatch.c > > new file mode 100644 > > index 000000000000..e147f5418327 > > --- /dev/null > > +++ b/kernel/module/livepatch.c > > @@ -0,0 +1,75 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * kernel/module/livepatch.c - module livepatch support > > + * > > + * Copyright (C) 2016 Jessica Yu <jeyu@redhat.com> > > + */ > > Should the copyright year (and possibly author) be updated? Or just removed > entirely? Who should I specify? I'm not entirely sure. If I understand correctly, Jessica was the original author of the majority. Kind regards,
Aaron Tomlin <atomlin@atomlin.com> wrote on Thu [2022-Jan-13 10:35:31 +0000]: > Who should I specify? I'm not entirely sure. If I understand correctly, > Jessica was the original author of the majority. Personally I would just remove the whole copyright as the SPDX identifier should be sufficient. I'll let Luis weigh in here as the Modules maintainer, though.
On Thu, Jan 13, 2022 at 06:16:54AM -0800, David Vernet wrote: > Aaron Tomlin <atomlin@atomlin.com> wrote on Thu [2022-Jan-13 10:35:31 +0000]: > > Who should I specify? I'm not entirely sure. If I understand correctly, > > Jessica was the original author of the majority. > > Personally I would just remove the whole copyright as the SPDX identifier > should be sufficient. I'll let Luis weigh in here as the Modules > maintainer, though. Technically every contributor to the file has a copyright entry added, one does not need to add the name to the header, that's just common practice though. If someone goes through the work of at least adding the name of the main author, that a nice effort. Tons of files don't have authors listed, so I'd be fine with that too and it keeps things simple. What we should not do ever is remove the copyright notice so code which Rusty wrote should be maintained with the notice. Luis
On Wed 2022-01-12 17:53 +0100, Petr Mladek wrote: > 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 Petr, I agree. I'll incorporate this approach into RFC PATCH v3. Kind regards,
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) { } +#endif /* CONFIG_LIVEPATCH */ diff --git a/kernel/module/livepatch.c b/kernel/module/livepatch.c new file mode 100644 index 000000000000..e147f5418327 --- /dev/null +++ b/kernel/module/livepatch.c @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * kernel/module/livepatch.c - module livepatch support + * + * Copyright (C) 2016 Jessica Yu <jeyu@redhat.com> + */ + +#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); +} 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 @@ -2043,81 +2043,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) { /* @@ -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")) {
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