| Message ID | 20220209171118.3269581-3-atomlin@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | module: core code clean up | expand |
Le 09/02/2022 à 18:11, Aaron Tomlin a écrit : > No functional change. > > This patch migrates module version support out of core code into > kernel/module/version.c. In addition simple code refactoring to > make this possible. > > Signed-off-by: Aaron Tomlin <atomlin@redhat.com> > --- > kernel/module/Makefile | 1 + > kernel/module/internal.h | 50 +++++++++++++ > kernel/module/main.c | 150 +-------------------------------------- > kernel/module/version.c | 110 ++++++++++++++++++++++++++++ > 4 files changed, 163 insertions(+), 148 deletions(-) > create mode 100644 kernel/module/version.c Sparse reports: CHECK kernel/module/version.c kernel/module/version.c:103:6: warning: symbol 'module_layout' was not declared. Should it be static? Checkpatch: total: 0 errors, 2 warnings, 3 checks, 337 lines checked Note that everywhere you get a warning for alignment not matching open parenthesis, first action should be to check if we really need it to be on two lines. When it's shorted than 100 chars it is usually better to keep it on a single line. > > diff --git a/kernel/module/Makefile b/kernel/module/Makefile > index c30141c37eb3..1f111aa47e88 100644 > --- a/kernel/module/Makefile > +++ b/kernel/module/Makefile > @@ -15,4 +15,5 @@ obj-$(CONFIG_DEBUG_KMEMLEAK) += debug_kmemleak.o > obj-$(CONFIG_KALLSYMS) += kallsyms.o > obj-$(CONFIG_PROC_FS) += procfs.o > obj-$(CONFIG_SYSFS) += sysfs.o > +obj-$(CONFIG_MODVERSIONS) += version.o > endif > diff --git a/kernel/module/internal.h b/kernel/module/internal.h > index c49b4900b30b..475a66aa42f2 100644 > --- a/kernel/module/internal.h > +++ b/kernel/module/internal.h > @@ -71,7 +71,31 @@ struct load_info { > } index; > }; > > +struct symsearch { > + const struct kernel_symbol *start, *stop; > + const s32 *crcs; > + enum mod_license { > + NOT_GPL_ONLY, > + GPL_ONLY, > + } license; > +}; Why don't leave this in main.c ? > + > +struct find_symbol_arg { > + /* Input */ > + const char *name; > + bool gplok; > + bool warn; > + > + /* Output */ > + struct module *owner; > + const s32 *crc; > + const struct kernel_symbol *sym; > + enum mod_license license; > +}; > + > int mod_verify_sig(const void *mod, struct load_info *info); > +int try_to_force_load(struct module *mod, const char *reason); > +bool find_symbol(struct find_symbol_arg *fsa); > struct module *find_module_all(const char *name, size_t len, bool even_unformed); > unsigned long kernel_symbol_value(const struct kernel_symbol *sym); > int cmp_name(const void *name, const void *sym); > @@ -231,3 +255,29 @@ static inline void module_remove_modinfo_attrs(struct module *mod, int end) { } > static inline void del_usage_links(struct module *mod) { } > static inline void init_param_lock(struct module *mod) { } > #endif /* CONFIG_SYSFS */ > + > +#ifdef CONFIG_MODVERSIONS > +int check_version(const struct load_info *info, > + const char *symname, struct module *mod, const s32 *crc); > +int check_modstruct_version(const struct load_info *info, struct module *mod); > +int same_magic(const char *amagic, const char *bmagic, bool has_crcs); > +#else /* !CONFIG_MODVERSIONS */ > +static inline int check_version(const struct load_info *info, > + const char *symname, > + struct module *mod, > + const s32 *crc) > +{ > + return 1; > +} > + > +static inline int check_modstruct_version(const struct load_info *info, > + struct module *mod) > +{ > + return 1; > +} > + > +static inline int same_magic(const char *amagic, const char *bmagic, bool has_crcs) > +{ > + return strcmp(amagic, bmagic) == 0; > +} > +#endif /* CONFIG_MODVERSIONS */ > diff --git a/kernel/module/version.c b/kernel/module/version.c > new file mode 100644 > index 000000000000..10a1490d1b9e > --- /dev/null > +++ b/kernel/module/version.c > @@ -0,0 +1,110 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Module version support > + * > + * Copyright (C) 2008 Rusty Russell > + */ > + > +#include <linux/module.h> > +#include <linux/string.h> > +#include <linux/printk.h> > +#include "internal.h" > + > +static u32 resolve_rel_crc(const s32 *crc) > +{ > + return *(u32 *)((void *)crc + *crc); > +} > + > +int check_version(const struct load_info *info, > + const char *symname, > + struct module *mod, > + const s32 *crc) > +{ > + Elf_Shdr *sechdrs = info->sechdrs; > + unsigned int versindex = info->index.vers; > + unsigned int i, num_versions; > + struct modversion_info *versions; > + > + /* Exporting module didn't supply crcs? OK, we're already tainted. */ > + if (!crc) > + return 1; > + > + /* No versions at all? modprobe --force does this. */ > + if (versindex == 0) > + return try_to_force_load(mod, symname) == 0; > + > + versions = (void *) sechdrs[versindex].sh_addr; > + num_versions = sechdrs[versindex].sh_size > + / sizeof(struct modversion_info); > + > + for (i = 0; i < num_versions; i++) { > + u32 crcval; > + > + if (strcmp(versions[i].name, symname) != 0) > + continue; > + > + if (IS_ENABLED(CONFIG_MODULE_REL_CRCS)) > + crcval = resolve_rel_crc(crc); > + else > + crcval = *crc; > + if (versions[i].crc == crcval) > + return 1; > + pr_debug("Found checksum %X vs module %lX\n", > + crcval, versions[i].crc); > + goto bad_version; > + } > + > + /* Broken toolchain. Warn once, then let it go.. */ > + pr_warn_once("%s: no symbol version for %s\n", info->name, symname); > + return 1; > + > +bad_version: > + pr_warn("%s: disagrees about version of symbol %s\n", > + info->name, symname); > + return 0; > +} > + > +inline int check_modstruct_version(const struct load_info *info, > + struct module *mod) inline is pointless for a non static function > +{ > + struct find_symbol_arg fsa = { > + .name = "module_layout", > + .gplok = true, > + }; > + > + /* > + * Since this should be found in kernel (which can't be removed), no > + * locking is necessary -- use preempt_disable() to placate lockdep. > + */ > + preempt_disable(); > + if (!find_symbol(&fsa)) { > + preempt_enable(); > + BUG(); > + } > + preempt_enable(); > + return check_version(info, "module_layout", mod, fsa.crc); > +} > + > +/* First part is kernel version, which we ignore if module has crcs. */ > +inline int same_magic(const char *amagic, const char *bmagic, > + bool has_crcs) Same, not point for inline keyword here. > +{ > + if (has_crcs) { > + amagic += strcspn(amagic, " "); > + bmagic += strcspn(bmagic, " "); > + } > + return strcmp(amagic, bmagic) == 0; > +} > + > +/* > + * Generate the signature for all relevant module structures here. > + * If these change, we don't want to try to parse the module. > + */ > +void module_layout(struct module *mod, > + struct modversion_info *ver, > + struct kernel_param *kp, > + struct kernel_symbol *ks, > + struct tracepoint * const *tp) > +{ > +} > +EXPORT_SYMBOL(module_layout);
On Thu 2022-02-10 14:28 +0000, Christophe Leroy wrote: > > > Le 09/02/2022 à 18:11, Aaron Tomlin a écrit : > > No functional change. > > > > This patch migrates module version support out of core code into > > kernel/module/version.c. In addition simple code refactoring to > > make this possible. > > > > Signed-off-by: Aaron Tomlin <atomlin@redhat.com> > > --- > > kernel/module/Makefile | 1 + > > kernel/module/internal.h | 50 +++++++++++++ > > kernel/module/main.c | 150 +-------------------------------------- > > kernel/module/version.c | 110 ++++++++++++++++++++++++++++ > > 4 files changed, 163 insertions(+), 148 deletions(-) > > create mode 100644 kernel/module/version.c > > Sparse reports: > > CHECK kernel/module/version.c > kernel/module/version.c:103:6: warning: symbol 'module_layout' was not > declared. Should it be static? The function module_layout() does not appear to be used. So, I've decided to remove it. > Checkpatch: > > total: 0 errors, 2 warnings, 3 checks, 337 lines checked Ok. > > +struct symsearch { > > + const struct kernel_symbol *start, *stop; > > + const s32 *crcs; > > + enum mod_license { > > + NOT_GPL_ONLY, > > + GPL_ONLY, > > + } license; > > +}; > > Why don't leave this in main.c ? Yes, struct 'symsearch' is not used outside of kernel/module/main.c. > > +inline int check_modstruct_version(const struct load_info *info, > > + struct module *mod) > > inline is pointless for a non static function This was an unfortunate oversight. > > +inline int same_magic(const char *amagic, const char *bmagic, > > + bool has_crcs) > > Same, not point for inline keyword here. Agreed. Kind regards,
Le 13/02/2022 à 19:03, Aaron Tomlin a écrit : > On Thu 2022-02-10 14:28 +0000, Christophe Leroy wrote: >> >> >> Le 09/02/2022 à 18:11, Aaron Tomlin a écrit : >>> No functional change. >>> >>> This patch migrates module version support out of core code into >>> kernel/module/version.c. In addition simple code refactoring to >>> make this possible. >>> >>> Signed-off-by: Aaron Tomlin <atomlin@redhat.com> >>> --- >>> kernel/module/Makefile | 1 + >>> kernel/module/internal.h | 50 +++++++++++++ >>> kernel/module/main.c | 150 +-------------------------------------- >>> kernel/module/version.c | 110 ++++++++++++++++++++++++++++ >>> 4 files changed, 163 insertions(+), 148 deletions(-) >>> create mode 100644 kernel/module/version.c >> >> Sparse reports: >> >> CHECK kernel/module/version.c >> kernel/module/version.c:103:6: warning: symbol 'module_layout' was not >> declared. Should it be static? > > The function module_layout() does not appear to be used. So, I've decided > to remove it. I'm not sure you can do that. From commit 8c8ef42aee8f ("module: include other structures in module version check") I understand that module_layout is there for some signature. > >> Checkpatch: >> >> total: 0 errors, 2 warnings, 3 checks, 337 lines checked > > Ok. > >>> +struct symsearch { >>> + const struct kernel_symbol *start, *stop; >>> + const s32 *crcs; >>> + enum mod_license { >>> + NOT_GPL_ONLY, >>> + GPL_ONLY, >>> + } license; >>> +}; >> >> Why don't leave this in main.c ? > > Yes, struct 'symsearch' is not used outside of kernel/module/main.c. > >>> +inline int check_modstruct_version(const struct load_info *info, >>> + struct module *mod) >> >> inline is pointless for a non static function > > This was an unfortunate oversight. > >>> +inline int same_magic(const char *amagic, const char *bmagic, >>> + bool has_crcs) >> >> Same, not point for inline keyword here. > > Agreed. > > > Kind regards, >
On Sun 2022-02-13 18:29 +0000, Christophe Leroy wrote: > I'm not sure you can do that. > > From commit 8c8ef42aee8f ("module: include other structures in module > version check") I understand that module_layout is there for some signature. Hi Christophe, I see. In which case, if I understand correctly, we'd have to ignore the report from Sparse. I will add a comment to hopefully suggest against removal. Kind regards,
Le 15/02/2022 à 16:05, Aaron Tomlin a écrit : > On Sun 2022-02-13 18:29 +0000, Christophe Leroy wrote: >> I'm not sure you can do that. >> >> From commit 8c8ef42aee8f ("module: include other structures in module >> version check") I understand that module_layout is there for some signature. > > Hi Christophe, > > I see. In which case, if I understand correctly, we'd have to ignore the > report from Sparse. I will add a comment to hopefully suggest against > removal. > > I would add the function's prototype in internal.h to shut up sparse. Christophe
> > > +struct symsearch { > > > + const struct kernel_symbol *start, *stop; > > > + const s32 *crcs; > > > + enum mod_license { > > > + NOT_GPL_ONLY, > > > + GPL_ONLY, > > > + } license; > > > +}; > > > > Why don't leave this in main.c ? > > Yes, struct 'symsearch' is not used outside of kernel/module/main.c. It is not, but "struct find_symbol_arg", which you moved, uses "enum mod_license" defined above, so you can either leave it as it is, or carve "enum mod_license" definition out. Miroslav
On Thu 2022-02-17 16:51 +0100, Miroslav Benes wrote: > It is not, but "struct find_symbol_arg", which you moved, uses "enum > mod_license" defined above, so you can either leave it as it is, or carve > "enum mod_license" definition out. Hi Miroslav, I've done this already for v6. I hope to share the latest version for review shortly. Kind regards,
diff --git a/kernel/module/Makefile b/kernel/module/Makefile index c30141c37eb3..1f111aa47e88 100644 --- a/kernel/module/Makefile +++ b/kernel/module/Makefile @@ -15,4 +15,5 @@ obj-$(CONFIG_DEBUG_KMEMLEAK) += debug_kmemleak.o obj-$(CONFIG_KALLSYMS) += kallsyms.o obj-$(CONFIG_PROC_FS) += procfs.o obj-$(CONFIG_SYSFS) += sysfs.o +obj-$(CONFIG_MODVERSIONS) += version.o endif diff --git a/kernel/module/internal.h b/kernel/module/internal.h index c49b4900b30b..475a66aa42f2 100644 --- a/kernel/module/internal.h +++ b/kernel/module/internal.h @@ -71,7 +71,31 @@ struct load_info { } index; }; +struct symsearch { + const struct kernel_symbol *start, *stop; + const s32 *crcs; + enum mod_license { + NOT_GPL_ONLY, + GPL_ONLY, + } license; +}; + +struct find_symbol_arg { + /* Input */ + const char *name; + bool gplok; + bool warn; + + /* Output */ + struct module *owner; + const s32 *crc; + const struct kernel_symbol *sym; + enum mod_license license; +}; + int mod_verify_sig(const void *mod, struct load_info *info); +int try_to_force_load(struct module *mod, const char *reason); +bool find_symbol(struct find_symbol_arg *fsa); struct module *find_module_all(const char *name, size_t len, bool even_unformed); unsigned long kernel_symbol_value(const struct kernel_symbol *sym); int cmp_name(const void *name, const void *sym); @@ -231,3 +255,29 @@ static inline void module_remove_modinfo_attrs(struct module *mod, int end) { } static inline void del_usage_links(struct module *mod) { } static inline void init_param_lock(struct module *mod) { } #endif /* CONFIG_SYSFS */ + +#ifdef CONFIG_MODVERSIONS +int check_version(const struct load_info *info, + const char *symname, struct module *mod, const s32 *crc); +int check_modstruct_version(const struct load_info *info, struct module *mod); +int same_magic(const char *amagic, const char *bmagic, bool has_crcs); +#else /* !CONFIG_MODVERSIONS */ +static inline int check_version(const struct load_info *info, + const char *symname, + struct module *mod, + const s32 *crc) +{ + return 1; +} + +static inline int check_modstruct_version(const struct load_info *info, + struct module *mod) +{ + return 1; +} + +static inline int same_magic(const char *amagic, const char *bmagic, bool has_crcs) +{ + return strcmp(amagic, bmagic) == 0; +} +#endif /* CONFIG_MODVERSIONS */ diff --git a/kernel/module/main.c b/kernel/module/main.c index 519c5335f7a6..b65bf5f7d474 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -241,28 +241,6 @@ static __maybe_unused void *any_section_objs(const struct load_info *info, #define symversion(base, idx) ((base != NULL) ? ((base) + (idx)) : NULL) #endif -struct symsearch { - const struct kernel_symbol *start, *stop; - const s32 *crcs; - enum mod_license { - NOT_GPL_ONLY, - GPL_ONLY, - } license; -}; - -struct find_symbol_arg { - /* Input */ - const char *name; - bool gplok; - bool warn; - - /* Output */ - struct module *owner; - const s32 *crc; - const struct kernel_symbol *sym; - enum mod_license license; -}; - static bool check_exported_symbol(const struct symsearch *syms, struct module *owner, unsigned int symnum, void *data) @@ -333,7 +311,7 @@ static bool find_exported_symbol_in_section(const struct symsearch *syms, * Find an exported symbol and return it, along with, (optional) crc and * (optional) module which owns it. Needs preempt disabled or module_mutex. */ -static bool find_symbol(struct find_symbol_arg *fsa) +bool find_symbol(struct find_symbol_arg *fsa) { static const struct symsearch arr[] = { { __start___ksymtab, __stop___ksymtab, __start___kcrctab, @@ -1007,7 +985,7 @@ size_t modinfo_attrs_count = ARRAY_SIZE(modinfo_attrs); static const char vermagic[] = VERMAGIC_STRING; -static int try_to_force_load(struct module *mod, const char *reason) +int try_to_force_load(struct module *mod, const char *reason) { #ifdef CONFIG_MODULE_FORCE_LOAD if (!test_taint(TAINT_FORCED_MODULE)) @@ -1019,115 +997,6 @@ static int try_to_force_load(struct module *mod, const char *reason) #endif } -#ifdef CONFIG_MODVERSIONS - -static u32 resolve_rel_crc(const s32 *crc) -{ - return *(u32 *)((void *)crc + *crc); -} - -static int check_version(const struct load_info *info, - const char *symname, - struct module *mod, - const s32 *crc) -{ - Elf_Shdr *sechdrs = info->sechdrs; - unsigned int versindex = info->index.vers; - unsigned int i, num_versions; - struct modversion_info *versions; - - /* Exporting module didn't supply crcs? OK, we're already tainted. */ - if (!crc) - return 1; - - /* No versions at all? modprobe --force does this. */ - if (versindex == 0) - return try_to_force_load(mod, symname) == 0; - - versions = (void *) sechdrs[versindex].sh_addr; - num_versions = sechdrs[versindex].sh_size - / sizeof(struct modversion_info); - - for (i = 0; i < num_versions; i++) { - u32 crcval; - - if (strcmp(versions[i].name, symname) != 0) - continue; - - if (IS_ENABLED(CONFIG_MODULE_REL_CRCS)) - crcval = resolve_rel_crc(crc); - else - crcval = *crc; - if (versions[i].crc == crcval) - return 1; - pr_debug("Found checksum %X vs module %lX\n", - crcval, versions[i].crc); - goto bad_version; - } - - /* Broken toolchain. Warn once, then let it go.. */ - pr_warn_once("%s: no symbol version for %s\n", info->name, symname); - return 1; - -bad_version: - pr_warn("%s: disagrees about version of symbol %s\n", - info->name, symname); - return 0; -} - -static inline int check_modstruct_version(const struct load_info *info, - struct module *mod) -{ - struct find_symbol_arg fsa = { - .name = "module_layout", - .gplok = true, - }; - - /* - * Since this should be found in kernel (which can't be removed), no - * locking is necessary -- use preempt_disable() to placate lockdep. - */ - preempt_disable(); - if (!find_symbol(&fsa)) { - preempt_enable(); - BUG(); - } - preempt_enable(); - return check_version(info, "module_layout", mod, fsa.crc); -} - -/* First part is kernel version, which we ignore if module has crcs. */ -static inline int same_magic(const char *amagic, const char *bmagic, - bool has_crcs) -{ - if (has_crcs) { - amagic += strcspn(amagic, " "); - bmagic += strcspn(bmagic, " "); - } - return strcmp(amagic, bmagic) == 0; -} -#else -static inline int check_version(const struct load_info *info, - const char *symname, - struct module *mod, - const s32 *crc) -{ - return 1; -} - -static inline int check_modstruct_version(const struct load_info *info, - struct module *mod) -{ - return 1; -} - -static inline int same_magic(const char *amagic, const char *bmagic, - bool has_crcs) -{ - return strcmp(amagic, bmagic) == 0; -} -#endif /* CONFIG_MODVERSIONS */ - static char *get_modinfo(const struct load_info *info, const char *tag); static char *get_next_modinfo(const struct load_info *info, const char *tag, char *prev); @@ -3263,18 +3132,3 @@ void print_modules(void) pr_cont(" [last unloaded: %s]", last_unloaded_module); pr_cont("\n"); } - -#ifdef CONFIG_MODVERSIONS -/* - * Generate the signature for all relevant module structures here. - * If these change, we don't want to try to parse the module. - */ -void module_layout(struct module *mod, - struct modversion_info *ver, - struct kernel_param *kp, - struct kernel_symbol *ks, - struct tracepoint * const *tp) -{ -} -EXPORT_SYMBOL(module_layout); -#endif diff --git a/kernel/module/version.c b/kernel/module/version.c new file mode 100644 index 000000000000..10a1490d1b9e --- /dev/null +++ b/kernel/module/version.c @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Module version support + * + * Copyright (C) 2008 Rusty Russell + */ + +#include <linux/module.h> +#include <linux/string.h> +#include <linux/printk.h> +#include "internal.h" + +static u32 resolve_rel_crc(const s32 *crc) +{ + return *(u32 *)((void *)crc + *crc); +} + +int check_version(const struct load_info *info, + const char *symname, + struct module *mod, + const s32 *crc) +{ + Elf_Shdr *sechdrs = info->sechdrs; + unsigned int versindex = info->index.vers; + unsigned int i, num_versions; + struct modversion_info *versions; + + /* Exporting module didn't supply crcs? OK, we're already tainted. */ + if (!crc) + return 1; + + /* No versions at all? modprobe --force does this. */ + if (versindex == 0) + return try_to_force_load(mod, symname) == 0; + + versions = (void *) sechdrs[versindex].sh_addr; + num_versions = sechdrs[versindex].sh_size + / sizeof(struct modversion_info); + + for (i = 0; i < num_versions; i++) { + u32 crcval; + + if (strcmp(versions[i].name, symname) != 0) + continue; + + if (IS_ENABLED(CONFIG_MODULE_REL_CRCS)) + crcval = resolve_rel_crc(crc); + else + crcval = *crc; + if (versions[i].crc == crcval) + return 1; + pr_debug("Found checksum %X vs module %lX\n", + crcval, versions[i].crc); + goto bad_version; + } + + /* Broken toolchain. Warn once, then let it go.. */ + pr_warn_once("%s: no symbol version for %s\n", info->name, symname); + return 1; + +bad_version: + pr_warn("%s: disagrees about version of symbol %s\n", + info->name, symname); + return 0; +} + +inline int check_modstruct_version(const struct load_info *info, + struct module *mod) +{ + struct find_symbol_arg fsa = { + .name = "module_layout", + .gplok = true, + }; + + /* + * Since this should be found in kernel (which can't be removed), no + * locking is necessary -- use preempt_disable() to placate lockdep. + */ + preempt_disable(); + if (!find_symbol(&fsa)) { + preempt_enable(); + BUG(); + } + preempt_enable(); + return check_version(info, "module_layout", mod, fsa.crc); +} + +/* First part is kernel version, which we ignore if module has crcs. */ +inline int same_magic(const char *amagic, const char *bmagic, + bool has_crcs) +{ + if (has_crcs) { + amagic += strcspn(amagic, " "); + bmagic += strcspn(bmagic, " "); + } + return strcmp(amagic, bmagic) == 0; +} + +/* + * Generate the signature for all relevant module structures here. + * If these change, we don't want to try to parse the module. + */ +void module_layout(struct module *mod, + struct modversion_info *ver, + struct kernel_param *kp, + struct kernel_symbol *ks, + struct tracepoint * const *tp) +{ +} +EXPORT_SYMBOL(module_layout);
No functional change. This patch migrates module version support out of core code into kernel/module/version.c. In addition simple code refactoring to make this possible. Signed-off-by: Aaron Tomlin <atomlin@redhat.com> --- kernel/module/Makefile | 1 + kernel/module/internal.h | 50 +++++++++++++ kernel/module/main.c | 150 +-------------------------------------- kernel/module/version.c | 110 ++++++++++++++++++++++++++++ 4 files changed, 163 insertions(+), 148 deletions(-) create mode 100644 kernel/module/version.c