| Message ID | 20220222141303.1392190-5-atomlin@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | module: core code clean up | expand |
Le 22/02/2022 à 15:12, 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. > > Signed-off-by: Aaron Tomlin <atomlin@redhat.com> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > include/linux/module.h | 9 ++-- > kernel/module/Makefile | 1 + > kernel/module/internal.h | 22 ++++++++ > kernel/module/livepatch.c | 74 +++++++++++++++++++++++++++ > kernel/module/main.c | 102 ++++---------------------------------- > 5 files changed, 110 insertions(+), 98 deletions(-) > create mode 100644 kernel/module/livepatch.c > > diff --git a/include/linux/module.h b/include/linux/module.h > index 1e135fd5c076..7ec9715de7dc 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -663,17 +663,14 @@ static inline bool module_requested_async_probing(struct module *module) > return module && module->async_probe_requested; > } > > -#ifdef CONFIG_LIVEPATCH > static inline bool is_livepatch_module(struct module *mod) > { > +#ifdef CONFIG_LIVEPATCH > return mod->klp; > -} > -#else /* !CONFIG_LIVEPATCH */ > -static inline bool is_livepatch_module(struct module *mod) > -{ > +#else > return false; > +#endif > } > -#endif /* CONFIG_LIVEPATCH */ > > bool is_module_sig_enforced(void); > void set_module_sig_enforced(void); > diff --git a/kernel/module/Makefile b/kernel/module/Makefile > index cdd5c61b8c7f..ed3aacb04f17 100644 > --- a/kernel/module/Makefile > +++ b/kernel/module/Makefile > @@ -10,3 +10,4 @@ KCOV_INSTRUMENT_module.o := n > obj-y += main.o > obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o > obj-$(CONFIG_MODULE_SIG) += signing.o > +obj-$(CONFIG_LIVEPATCH) += livepatch.o > diff --git a/kernel/module/internal.h b/kernel/module/internal.h > index e0775e66bcf7..ad7a444253ed 100644 > --- a/kernel/module/internal.h > +++ b/kernel/module/internal.h > @@ -57,6 +57,28 @@ 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); > +#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 */ > + > +static inline bool set_livepatch_module(struct module *mod) > +{ > +#ifdef CONFIG_LIVEPATCH > + mod->klp = true; > + return true; > +#else > + return false; > +#endif > +} > + > #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..486d4ff92719 > --- /dev/null > +++ b/kernel/module/livepatch.c > @@ -0,0 +1,74 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * 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) > + 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) { > + 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) { > + 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 5f5e21f972dd..3596ebf3a6c3 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) > { > /* > @@ -3092,30 +3017,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) > {
On Tue 2022-02-22 14:12:54, 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> > diff --git a/kernel/module/livepatch.c b/kernel/module/livepatch.c > new file mode 100644 > index 000000000000..486d4ff92719 > --- /dev/null > +++ b/kernel/module/livepatch.c > @@ -0,0 +1,74 @@ > + * 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) > + 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) { > + 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) { > + 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; This code include several other well hidden changes: --- del.p 2022-02-24 16:55:26.570054922 +0100 +++ add.p 2022-02-24 16:56:04.766781394 +0100 @@ -3,14 +3,14 @@ * 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) +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) + if (!mod->klp_info) return -ENOMEM; /* Elf header */ @@ -20,7 +20,7 @@ static int copy_module_elf(struct module /* 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) { + if (!mod->klp_info->sechdrs) { ret = -ENOMEM; goto free_info; } @@ -28,7 +28,7 @@ static int copy_module_elf(struct module /* 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) { + if (!mod->klp_info->secstrings) { ret = -ENOMEM; goto free_sechdrs; } @@ -43,8 +43,7 @@ static int copy_module_elf(struct module * 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; + mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)mod->core_kallsyms.symtab; return 0; Please do not do these small coding style changes. It complicates the review and increases the risk of regressions. Different people have different preferences. Just imagine that every half a year someone update style of a code by his personal preferences. The real changes will then get lost in a lot of noise. Coding style changes might be acceptable only when the code is reworked or when it significantly improves readability. That said. I reviewed and tested this patch and did not find any problem. Feel free to use: Reviewed-by: Petr Mladek <pmladek@suse.com> Tested-by: Petr Mladek <pmladek@suse.com> Please, take the above as an advice for your future work. Best Regards, Petr
On Fri 2022-02-25 10:34 +0100, Petr Mladek wrote: > This code include several other well hidden changes: > > --- del.p 2022-02-24 16:55:26.570054922 +0100 > +++ add.p 2022-02-24 16:56:04.766781394 +0100 > @@ -3,14 +3,14 @@ > * 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) > +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) > + if (!mod->klp_info) > return -ENOMEM; > > /* Elf header */ > @@ -20,7 +20,7 @@ static int copy_module_elf(struct module > /* 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) { > + if (!mod->klp_info->sechdrs) { > ret = -ENOMEM; > goto free_info; > } > @@ -28,7 +28,7 @@ static int copy_module_elf(struct module > /* 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) { > + if (!mod->klp_info->secstrings) { > ret = -ENOMEM; > goto free_sechdrs; > } > @@ -43,8 +43,7 @@ static int copy_module_elf(struct module > * 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; > + mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)mod->core_kallsyms.symtab; > > return 0; > > > Please do not do these small coding style changes. It complicates the > review and increases the risk of regressions. Different people > have different preferences. Just imagine that every half a year > someone update style of a code by his personal preferences. The > real changes will then get lost in a lot of noise. > > Coding style changes might be acceptable only when the code is > reworked or when it significantly improves readability. > > > That said. I reviewed and tested this patch and did not find any > problem. Feel free to use: > > Reviewed-by: Petr Mladek <pmladek@suse.com> > Tested-by: Petr Mladek <pmladek@suse.com> > > Please, take the above as an advice for your future work. Hi Petr, Firstly, thank you for your feedback. Fair enough and noted. Kind regards,
Le 25/02/2022 à 10:34, Petr Mladek a écrit : > On Tue 2022-02-22 14:12:54, 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> > >> diff --git a/kernel/module/livepatch.c b/kernel/module/livepatch.c >> new file mode 100644 >> index 000000000000..486d4ff92719 >> --- /dev/null >> +++ b/kernel/module/livepatch.c >> @@ -0,0 +1,74 @@ >> + * 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) >> + 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) { >> + 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) { >> + 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; > > This code include several other well hidden changes: > > --- del.p 2022-02-24 16:55:26.570054922 +0100 > +++ add.p 2022-02-24 16:56:04.766781394 +0100 > @@ -3,14 +3,14 @@ > * 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) > +int copy_module_elf(struct module *mod, struct load_info *info) That's not a hidden change. That's part of the move, that's required. > { > unsigned int size, symndx; > int ret; > > size = sizeof(*mod->klp_info); > mod->klp_info = kmalloc(size, GFP_KERNEL); > - if (mod->klp_info == NULL) > + if (!mod->klp_info) > return -ENOMEM; > > /* Elf header */ > @@ -20,7 +20,7 @@ static int copy_module_elf(struct module > /* 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) { > + if (!mod->klp_info->sechdrs) { > ret = -ENOMEM; > goto free_info; > } > @@ -28,7 +28,7 @@ static int copy_module_elf(struct module > /* 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) { > + if (!mod->klp_info->secstrings) { > ret = -ENOMEM; > goto free_sechdrs; > } > @@ -43,8 +43,7 @@ static int copy_module_elf(struct module > * 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; > + mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)mod->core_kallsyms.symtab; > > return 0; > > > Please do not do these small coding style changes. It complicates the > review and increases the risk of regressions. Different people > have different preferences. Just imagine that every half a year > someone update style of a code by his personal preferences. The > real changes will then get lost in a lot of noise. I disagree here. We are not talking about people's preference here but compliance with documented Linux kernel Codying Style and handling of official checkpatch.pl script reports. You are right that randomly updating the style every half a year would be a nightmare and would kill blamability of changes. However when moving big peaces of code like this, blamability is broken anyway and this is a very good opportunity to increase compliance of kernel code to its own codying style. But doing it in several steps increases code churn and has no real added value. > > Coding style changes might be acceptable only when the code is > reworked or when it significantly improves readability. When code is moved around it is also a good opportunity. > > > That said. I reviewed and tested this patch and did not find any > problem. Feel free to use: > > Reviewed-by: Petr Mladek <pmladek@suse.com> > Tested-by: Petr Mladek <pmladek@suse.com> > > Please, take the above as an advice for your future work. > Thanks Christophe
On Fri 2022-02-25 16:49:31, Christophe Leroy wrote: > > > Le 25/02/2022 à 10:34, Petr Mladek a écrit : > > On Tue 2022-02-22 14:12:54, 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. > >> > > --- del.p 2022-02-24 16:55:26.570054922 +0100 > > +++ add.p 2022-02-24 16:56:04.766781394 +0100 > > @@ -3,14 +3,14 @@ > > * 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) > > +int copy_module_elf(struct module *mod, struct load_info *info) > > That's not a hidden change. That's part of the move, that's required. Sure. I was not talking about this line. I kept it to show the context. > > { > > unsigned int size, symndx; > > int ret; > > > > size = sizeof(*mod->klp_info); > > mod->klp_info = kmalloc(size, GFP_KERNEL); > > - if (mod->klp_info == NULL) > > + if (!mod->klp_info) > > return -ENOMEM; > > > > /* Elf header */ > > @@ -20,7 +20,7 @@ static int copy_module_elf(struct module > > /* 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) { > > + if (!mod->klp_info->sechdrs) { > > ret = -ENOMEM; > > goto free_info; > > } > > @@ -28,7 +28,7 @@ static int copy_module_elf(struct module > > /* 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) { > > + if (!mod->klp_info->secstrings) { > > ret = -ENOMEM; > > goto free_sechdrs; > > } > > @@ -43,8 +43,7 @@ static int copy_module_elf(struct module > > * 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; > > + mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)mod->core_kallsyms.symtab; > > > > return 0; > > > > > > Please do not do these small coding style changes. It complicates the > > review and increases the risk of regressions. Different people > > have different preferences. Just imagine that every half a year > > someone update style of a code by his personal preferences. The > > real changes will then get lost in a lot of noise. > > I disagree here. We are not talking about people's preference here but > compliance with documented Linux kernel Codying Style and handling of > official checkpatch.pl script reports. Really? 1. I restored + if (mod->klp_info->secstrings == NULL) { and checkpatch.pl is happy. 2. I do not see anythinkg about if (xxx == NULL) checks in Documentation/process/coding-style.rst 3. $> git grep "if (.* == NULL" | wc -l 15041 4. The result of - mod->klp_info->sechdrs[symndx].sh_addr = \ - (unsigned long) mod->core_kallsyms.symtab; + mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)mod->core_kallsyms.symtab; is 90 characeters long and Documentation/process/coding-style.rst says: 2) Breaking long lines and strings ---------------------------------- Coding style is all about readability and maintainability using commonly available tools. The preferred limit on the length of a single line is 80 columns. Statements longer than 80 columns should be broken into sensible chunks, unless exceeding 80 columns significantly increases readability and does not hide information. checkpatch.pl accepts lines up to 100 columns but 80 are still preferred. > You are right that randomly updating the style every half a year would > be a nightmare and would kill blamability of changes. > > However when moving big peaces of code like this, blamability is broken > anyway and this is a very good opportunity to increase compliance of > kernel code to its own codying style. But doing it in several steps > increases code churn and has no real added value. From Documentation/process/submitting-patches.rst: One significant exception is when moving code from one file to another -- in this case you should not modify the moved code at all in the same patch which moves it. This clearly delineates the act of moving the code and your changes. This greatly aids review of the actual differences and allows tools to better track the history of the code itself. > > > > Coding style changes might be acceptable only when the code is > > reworked or when it significantly improves readability. > > When code is moved around it is also a good opportunity. No! I would not have complained if it did not complicate my review. But it did! Best Regards, Petr
Le 28/02/2022 à 11:56, Petr Mladek a écrit : > On Fri 2022-02-25 16:49:31, Christophe Leroy wrote: >> >> >> Le 25/02/2022 à 10:34, Petr Mladek a écrit : >>> >>> Please do not do these small coding style changes. It complicates the >>> review and increases the risk of regressions. Different people >>> have different preferences. Just imagine that every half a year >>> someone update style of a code by his personal preferences. The >>> real changes will then get lost in a lot of noise. >> >> I disagree here. We are not talking about people's preference here but >> compliance with documented Linux kernel Codying Style and handling of >> official checkpatch.pl script reports. > > Really? > > 1. I restored > > + if (mod->klp_info->secstrings == NULL) { > > and checkpatch.pl is happy. On mainline's kernel/module.c checkpatch.pl tells me: CHECK: Comparison to NULL could be written "!mod->klp_info->secstrings" #2092: FILE: kernel/module.c:2092: + if (mod->klp_info->secstrings == NULL) { > > > 2. I do not see anythinkg about if (xxx == NULL) checks in > Documentation/process/coding-style.rst > > 3. $> git grep "if (.* == NULL" | wc -l > 15041 Commit b75ac618df75 ("checkpatch: add --strict "pointer comparison to NULL" test") > > 4. The result of > - mod->klp_info->sechdrs[symndx].sh_addr = \ > - (unsigned long) mod->core_kallsyms.symtab; > + mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)mod->core_kallsyms.symtab; > > is 90 characeters long and Documentation/process/coding-style.rst says: Probably a misinterpretation of: WARNING: Avoid unnecessary line continuations #2107: FILE: kernel/module.c:2107: + mod->klp_info->sechdrs[symndx].sh_addr = \ > > 2) Breaking long lines and strings > ---------------------------------- > > Coding style is all about readability and maintainability using commonly > available tools. > > The preferred limit on the length of a single line is 80 columns. > > Statements longer than 80 columns should be broken into sensible chunks, > unless exceeding 80 columns significantly increases readability and does > not hide information. > > checkpatch.pl accepts lines up to 100 columns but 80 are still > preferred. > > >> You are right that randomly updating the style every half a year would >> be a nightmare and would kill blamability of changes. >> >> However when moving big peaces of code like this, blamability is broken >> anyway and this is a very good opportunity to increase compliance of >> kernel code to its own codying style. But doing it in several steps >> increases code churn and has no real added value. > > From Documentation/process/submitting-patches.rst: > > One significant exception is when moving code from one file to > another -- in this case you should not modify the moved code at all in > the same patch which moves it. This clearly delineates the act of > moving the code and your changes. This greatly aids review of the > actual differences and allows tools to better track the history of > the code itself. > > >>> >>> Coding style changes might be acceptable only when the code is >>> reworked or when it significantly improves readability. >> >> When code is moved around it is also a good opportunity. > > No! By the way some maintainers require checkpatch' clean patches even when this is only code move. I remember being requested to do that in the past, so now I almost always do it with my own patches. > > I would not have complained if it did not complicate my review. > But it did! > Reviewing partial code move is not easy anyway, git is not very userfriendly with that. Christophe
On Mon 2022-02-28 11:46:33, Christophe Leroy wrote: > > > Le 28/02/2022 à 11:56, Petr Mladek a écrit : > > On Fri 2022-02-25 16:49:31, Christophe Leroy wrote: > >> Le 25/02/2022 à 10:34, Petr Mladek a écrit : > >>> > >>> Please do not do these small coding style changes. It complicates the > >>> review and increases the risk of regressions. Different people > >>> have different preferences. Just imagine that every half a year > >>> someone update style of a code by his personal preferences. The > >>> real changes will then get lost in a lot of noise. > >> > >> I disagree here. We are not talking about people's preference here but > >> compliance with documented Linux kernel Codying Style and handling of > >> official checkpatch.pl script reports. > > > > Really? > > > > 1. I restored > > > > + if (mod->klp_info->secstrings == NULL) { > > > > and checkpatch.pl is happy. > > On mainline's kernel/module.c checkpatch.pl tells me: > > CHECK: Comparison to NULL could be written "!mod->klp_info->secstrings" > #2092: FILE: kernel/module.c:2092: > + if (mod->klp_info->secstrings == NULL) { Only with --strict option. Alias of this option is --subjective... > By the way some maintainers require checkpatch' clean patches even when > this is only code move. I remember being requested to do that in the > past, so now I almost always do it with my own patches. I see. From my POV, checkpatch is an useful tool for finding obvious mistakes. But it is just a best effort approach. It has false positives. And some complains are controversial. BTW: I have never heard about --strict/--subjective option. I wonder if some maintainer requires it. > > I would not have complained if it did not complicate my review. > > But it did! > > Reviewing partial code move is not easy anyway, git is not very > userfriendly with that. Exactly. It is a real pain to find changes in moved functions. It is much easier when the author just shuffled the code. Anyway, the less changes the better. Best Regards, Petr
diff --git a/include/linux/module.h b/include/linux/module.h index 1e135fd5c076..7ec9715de7dc 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -663,17 +663,14 @@ static inline bool module_requested_async_probing(struct module *module) return module && module->async_probe_requested; } -#ifdef CONFIG_LIVEPATCH static inline bool is_livepatch_module(struct module *mod) { +#ifdef CONFIG_LIVEPATCH return mod->klp; -} -#else /* !CONFIG_LIVEPATCH */ -static inline bool is_livepatch_module(struct module *mod) -{ +#else return false; +#endif } -#endif /* CONFIG_LIVEPATCH */ bool is_module_sig_enforced(void); void set_module_sig_enforced(void); diff --git a/kernel/module/Makefile b/kernel/module/Makefile index cdd5c61b8c7f..ed3aacb04f17 100644 --- a/kernel/module/Makefile +++ b/kernel/module/Makefile @@ -10,3 +10,4 @@ KCOV_INSTRUMENT_module.o := n obj-y += main.o obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o obj-$(CONFIG_MODULE_SIG) += signing.o +obj-$(CONFIG_LIVEPATCH) += livepatch.o diff --git a/kernel/module/internal.h b/kernel/module/internal.h index e0775e66bcf7..ad7a444253ed 100644 --- a/kernel/module/internal.h +++ b/kernel/module/internal.h @@ -57,6 +57,28 @@ 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); +#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 */ + +static inline bool set_livepatch_module(struct module *mod) +{ +#ifdef CONFIG_LIVEPATCH + mod->klp = true; + return true; +#else + return false; +#endif +} + #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..486d4ff92719 --- /dev/null +++ b/kernel/module/livepatch.c @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * 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) + 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) { + 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) { + 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 5f5e21f972dd..3596ebf3a6c3 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) { /* @@ -3092,30 +3017,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) {
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> --- include/linux/module.h | 9 ++-- kernel/module/Makefile | 1 + kernel/module/internal.h | 22 ++++++++ kernel/module/livepatch.c | 74 +++++++++++++++++++++++++++ kernel/module/main.c | 102 ++++---------------------------------- 5 files changed, 110 insertions(+), 98 deletions(-) create mode 100644 kernel/module/livepatch.c