From 5e8ed280dab9eeabc1ba0b2db5dbe9fe6debb6b5 Mon Sep 17 00:00:00 2001 From: Miroslav Benes Date: Tue, 27 Oct 2020 15:03:36 +0100 Subject: [PATCH 01/15] module: set MODULE_STATE_GOING state when a module fails to load If a module fails to load due to an error in prepare_coming_module(), the following error handling in load_module() runs with MODULE_STATE_COMING in module's state. Fix it by correctly setting MODULE_STATE_GOING under "bug_cleanup" label. Signed-off-by: Miroslav Benes Signed-off-by: Jessica Yu --- kernel/module.c | 1 + 1 file changed, 1 insertion(+) diff --git a/kernel/module.c b/kernel/module.c index a4fa44a652a7..b34235082394 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3991,6 +3991,7 @@ static int load_module(struct load_info *info, const char __user *uargs, MODULE_STATE_GOING, mod); klp_module_going(mod); bug_cleanup: + mod->state = MODULE_STATE_GOING; /* module_bug_cleanup needs module_mutex protection */ mutex_lock(&module_mutex); module_bug_cleanup(mod); From 705e9195187d85249fbb0eaa844b1604a98fbc9a Mon Sep 17 00:00:00 2001 From: Sergey Shtylyov Date: Sat, 31 Oct 2020 23:06:45 +0300 Subject: [PATCH 02/15] module: merge repetitive strings in module_sig_check() The 'reason' variable in module_sig_check() points to 3 strings across the *switch* statement, all needlessly starting with the same text. Let's put the starting text into the pr_notice() call -- it saves 21 bytes of the object code (x86 gcc 10.2.1). Suggested-by: Joe Perches Reviewed-by: Miroslav Benes Signed-off-by: Sergey Shtylyov Signed-off-by: Jessica Yu --- kernel/module.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index b34235082394..0e54d58babac 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2907,16 +2907,17 @@ static int module_sig_check(struct load_info *info, int flags) * enforcing, certain errors are non-fatal. */ case -ENODATA: - reason = "Loading of unsigned module"; + reason = "unsigned module"; goto decide; case -ENOPKG: - reason = "Loading of module with unsupported crypto"; + reason = "module with unsupported crypto"; goto decide; case -ENOKEY: - reason = "Loading of module with unavailable key"; + reason = "module with unavailable key"; decide: if (is_module_sig_enforced()) { - pr_notice("%s: %s is rejected\n", info->name, reason); + pr_notice("%s: loading of %s is rejected\n", + info->name, reason); return -EKEYREJECTED; } From 10ccd1abb808599a6dc7c9389560016ea3568085 Mon Sep 17 00:00:00 2001 From: Sergey Shtylyov Date: Sat, 31 Oct 2020 23:09:31 +0300 Subject: [PATCH 03/15] module: avoid *goto*s in module_sig_check() Let's move the common handling of the non-fatal errors after the *switch* statement -- this avoids *goto*s inside that *switch*... Suggested-by: Joe Perches Reviewed-by: Miroslav Benes Signed-off-by: Sergey Shtylyov Signed-off-by: Jessica Yu --- kernel/module.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index 0e54d58babac..02b87bc84a42 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2908,20 +2908,13 @@ static int module_sig_check(struct load_info *info, int flags) */ case -ENODATA: reason = "unsigned module"; - goto decide; + break; case -ENOPKG: reason = "module with unsupported crypto"; - goto decide; + break; case -ENOKEY: reason = "module with unavailable key"; - decide: - if (is_module_sig_enforced()) { - pr_notice("%s: loading of %s is rejected\n", - info->name, reason); - return -EKEYREJECTED; - } - - return security_locked_down(LOCKDOWN_MODULE_SIGNATURE); + break; /* All other errors are fatal, including nomem, unparseable * signatures and signature check failures - even if signatures @@ -2930,6 +2923,13 @@ static int module_sig_check(struct load_info *info, int flags) default: return err; } + + if (is_module_sig_enforced()) { + pr_notice("%s: loading of %s is rejected\n", info->name, reason); + return -EKEYREJECTED; + } + + return security_locked_down(LOCKDOWN_MODULE_SIGNATURE); } #else /* !CONFIG_MODULE_SIG */ static int module_sig_check(struct load_info *info, int flags) From 076aa52e402185e1e347bf5c62c61c6388fce4c7 Mon Sep 17 00:00:00 2001 From: Sergey Shtylyov Date: Sat, 31 Oct 2020 23:10:28 +0300 Subject: [PATCH 04/15] module: only handle errors with the *switch* statement in module_sig_check() Let's handle the successful call of mod_verify_sig() right after that call, making the *switch* statement only handle the real errors, and then move the comment from the first *case* before *switch* itself and the comment before *default* after it. Fix the comment style, add article/comma/dash, spell out "nomem" as "lack of memory" in these comments, while at it... Suggested-by: Joe Perches Reviewed-by: Miroslav Benes Signed-off-by: Sergey Shtylyov Signed-off-by: Jessica Yu --- kernel/module.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index 02b87bc84a42..948d4bbbceb5 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2895,17 +2895,18 @@ static int module_sig_check(struct load_info *info, int flags) /* We truncate the module to discard the signature */ info->len -= markerlen; err = mod_verify_sig(mod, info); + if (!err) { + info->sig_ok = true; + return 0; + } } + /* + * We don't permit modules to be loaded into the trusted kernels + * without a valid signature on them, but if we're not enforcing, + * certain errors are non-fatal. + */ switch (err) { - case 0: - info->sig_ok = true; - return 0; - - /* We don't permit modules to be loaded into trusted kernels - * without a valid signature on them, but if we're not - * enforcing, certain errors are non-fatal. - */ case -ENODATA: reason = "unsigned module"; break; @@ -2916,11 +2917,12 @@ static int module_sig_check(struct load_info *info, int flags) reason = "module with unavailable key"; break; - /* All other errors are fatal, including nomem, unparseable - * signatures and signature check failures - even if signatures - * aren't required. - */ default: + /* + * All other errors are fatal, including lack of memory, + * unparseable signatures, and signature check failures -- + * even if signatures aren't required. + */ return err; } From 24389b610be31536328c655ae0a2cb0ef94be2c8 Mon Sep 17 00:00:00 2001 From: Sergey Shtylyov Date: Wed, 4 Nov 2020 23:34:59 +0300 Subject: [PATCH 05/15] module: fix up 'kernel-doc' comments Some 'kernel-doc' function comments do not fully comply with the specified format due to: - missing () after the function name; - "RETURNS:"/"Returns:" instead of "Return:" when documenting the function's result. - empty line before describing the function's arguments. Signed-off-by: Sergey Shtylyov Signed-off-by: Jessica Yu --- kernel/module.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index 948d4bbbceb5..98b9e2ba8c3d 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -727,13 +727,12 @@ bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr) } /** - * is_module_percpu_address - test whether address is from module static percpu + * is_module_percpu_address() - test whether address is from module static percpu * @addr: address to test * * Test whether @addr belongs to module static percpu area. * - * RETURNS: - * %true if @addr is from module static percpu area + * Return: %true if @addr is from module static percpu area */ bool is_module_percpu_address(unsigned long addr) { @@ -957,11 +956,10 @@ static int try_stop_module(struct module *mod, int flags, int *forced) } /** - * module_refcount - return the refcount or -1 if unloading - * + * module_refcount() - return the refcount or -1 if unloading * @mod: the module we're checking * - * Returns: + * Return: * -1 if the module is in the process of unloading * otherwise the number of references in the kernel to the module */ From 2541743e99c301f9b9659d0928bd8b22708d59df Mon Sep 17 00:00:00 2001 From: Sergey Shtylyov Date: Wed, 4 Nov 2020 23:35:51 +0300 Subject: [PATCH 06/15] module: add more 'kernel-doc' comments Some functions have the proper 'kernel-doc' comments but these don't start with proper /** -- fix that, along with adding () to the function name on the following lines to fully comply with the 'kernel-doc' format. Signed-off-by: Sergey Shtylyov Signed-off-by: Jessica Yu --- kernel/module.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index 98b9e2ba8c3d..0310c80b90a3 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -4491,8 +4491,8 @@ out: return e; } -/* - * is_module_address - is this address inside a module? +/** + * is_module_address() - is this address inside a module? * @addr: the address to check. * * See is_module_text_address() if you simply want to see if the address @@ -4509,8 +4509,8 @@ bool is_module_address(unsigned long addr) return ret; } -/* - * __module_address - get the module which contains an address. +/** + * __module_address() - get the module which contains an address. * @addr: the address. * * Must be called with preempt disabled or module mutex held so that @@ -4534,8 +4534,8 @@ struct module *__module_address(unsigned long addr) return mod; } -/* - * is_module_text_address - is this address inside module code? +/** + * is_module_text_address() - is this address inside module code? * @addr: the address to check. * * See is_module_address() if you simply want to see if the address is @@ -4553,8 +4553,8 @@ bool is_module_text_address(unsigned long addr) return ret; } -/* - * __module_text_address - get the module whose code contains an address. +/** + * __module_text_address() - get the module whose code contains an address. * @addr: the address. * * Must be called with preempt disabled or module mutex held so that From 24b9f0d22081455b6fd739c8365958c207a69973 Mon Sep 17 00:00:00 2001 From: Sergey Shtylyov Date: Sat, 7 Nov 2020 23:20:52 +0300 Subject: [PATCH 07/15] module: fix comment style Many comments in this module do not comply with the preferred multi-line comment style as reported by 'scripts/checkpatch.pl': WARNING: Block comments use * on subsequent lines WARNING: Block comments use a trailing */ on a separate line Fix those comments, along with (unreported for some reason?) the starts of the multi-line comments not being /* on their own line... Signed-off-by: Sergey Shtylyov Signed-off-by: Jessica Yu --- kernel/module.c | 117 ++++++++++++++++++++++++++++++------------------ 1 file changed, 74 insertions(+), 43 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index 0310c80b90a3..a40ec708f8f2 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1,9 +1,8 @@ // SPDX-License-Identifier: GPL-2.0-or-later /* - Copyright (C) 2002 Richard Henderson - Copyright (C) 2001 Rusty Russell, 2002, 2010 Rusty Russell IBM. - -*/ + * Copyright (C) 2002 Richard Henderson + * Copyright (C) 2001 Rusty Russell, 2002, 2010 Rusty Russell IBM. + */ #define INCLUDE_VERMAGIC @@ -86,7 +85,8 @@ * 1) List of modules (also safely readable with preempt_disable), * 2) module_use links, * 3) module_addr_min/module_addr_max. - * (delete and add uses RCU list operations). */ + * (delete and add uses RCU list operations). + */ DEFINE_MUTEX(module_mutex); EXPORT_SYMBOL_GPL(module_mutex); static LIST_HEAD(modules); @@ -586,8 +586,10 @@ static bool find_exported_symbol_in_section(const struct symsearch *syms, return false; } -/* Find an exported symbol and return it, along with, (optional) crc and - * (optional) module which owns it. Needs preempt disabled or module_mutex. */ +/* + * Find an exported symbol and return it, along with, (optional) crc and + * (optional) module which owns it. Needs preempt disabled or module_mutex. + */ static const struct kernel_symbol *find_symbol(const char *name, struct module **owner, const s32 **crc, @@ -1644,8 +1646,10 @@ static void remove_sect_attrs(struct module *mod) if (mod->sect_attrs) { sysfs_remove_group(&mod->mkobj.kobj, &mod->sect_attrs->grp); - /* We are positive that no one is using any sect attrs - * at this point. Deallocate immediately. */ + /* + * We are positive that no one is using any sect attrs + * at this point. Deallocate immediately. + */ free_sect_attrs(mod->sect_attrs); mod->sect_attrs = NULL; } @@ -2216,8 +2220,10 @@ static void free_module(struct module *mod) mod_sysfs_teardown(mod); - /* We leave it in list to prevent duplicate loads, but make sure - * that noone uses it while it's being deconstructed. */ + /* + * We leave it in list to prevent duplicate loads, but make sure + * that noone uses it while it's being deconstructed. + */ mutex_lock(&module_mutex); mod->state = MODULE_STATE_UNFORMED; mutex_unlock(&module_mutex); @@ -2334,8 +2340,10 @@ static int simplify_symbols(struct module *mod, const struct load_info *info) if (!strncmp(name, "__gnu_lto", 9)) break; - /* We compiled with -fno-common. These are not - supposed to happen. */ + /* + * We compiled with -fno-common. These are not + * supposed to happen. + */ pr_debug("Common symbol: %s\n", name); pr_warn("%s: please compile with -fno-common\n", mod->name); @@ -2438,16 +2446,20 @@ static long get_offset(struct module *mod, unsigned int *size, return ret; } -/* Lay out the SHF_ALLOC sections in a way not dissimilar to how ld - might -- code, read-only data, read-write data, small data. Tally - sizes, and place the offsets into sh_entsize fields: high bit means it - belongs in init. */ +/* + * Lay out the SHF_ALLOC sections in a way not dissimilar to how ld + * might -- code, read-only data, read-write data, small data. Tally + * sizes, and place the offsets into sh_entsize fields: high bit means it + * belongs in init. + */ static void layout_sections(struct module *mod, struct load_info *info) { static unsigned long const masks[][2] = { - /* NOTE: all executable code must be the first section + /* + * NOTE: all executable code must be the first section * in this array; otherwise modify the text_size - * finder in the two loops below */ + * finder in the two loops below + */ { SHF_EXECINSTR | SHF_ALLOC, ARCH_SHF_SMALL }, { SHF_ALLOC, SHF_WRITE | ARCH_SHF_SMALL }, { SHF_RO_AFTER_INIT | SHF_ALLOC, ARCH_SHF_SMALL }, @@ -3062,8 +3074,10 @@ static int rewrite_section_headers(struct load_info *info, int flags) return -ENOEXEC; } - /* Mark all sections sh_addr with their address in the - temporary image. */ + /* + * Mark all sections sh_addr with their address in the + * temporary image. + */ shdr->sh_addr = (size_t)info->hdr + shdr->sh_offset; #ifndef CONFIG_MODULE_UNLOAD @@ -3494,9 +3508,11 @@ static struct module *layout_and_allocate(struct load_info *info, int flags) if (ndx) info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT; - /* Determine total sizes, and put offsets in sh_entsize. For now - this is done generically; there doesn't appear to be any - special cases for the architectures. */ + /* + * Determine total sizes, and put offsets in sh_entsize. For now + * this is done generically; there doesn't appear to be any + * special cases for the architectures. + */ layout_sections(info->mod, info); layout_symtab(info->mod, info); @@ -3780,8 +3796,10 @@ static int complete_formation(struct module *mod, struct load_info *info) module_enable_nx(mod); module_enable_x(mod); - /* Mark state as coming so strong_try_module_get() ignores us, - * but kallsyms etc. can see us. */ + /* + * Mark state as coming so strong_try_module_get() ignores us, + * but kallsyms etc. can see us. + */ mod->state = MODULE_STATE_COMING; mutex_unlock(&module_mutex); @@ -3828,8 +3846,10 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname, return 0; } -/* Allocate and load the module: note that size of section 0 is always - zero, and we rely on this for optional sections. */ +/* + * Allocate and load the module: note that size of section 0 is always + * zero, and we rely on this for optional sections. + */ static int load_module(struct load_info *info, const char __user *uargs, int flags) { @@ -3903,8 +3923,10 @@ static int load_module(struct load_info *info, const char __user *uargs, init_param_lock(mod); - /* Now we've got everything in the final locations, we can - * find optional sections. */ + /* + * Now we've got everything in the final locations, we can + * find optional sections. + */ err = find_module_sections(mod, info); if (err) goto free_unload; @@ -4118,8 +4140,10 @@ static const char *find_kallsyms_symbol(struct module *mod, bestval = kallsyms_symbol_value(&kallsyms->symtab[best]); - /* Scan for closest preceding symbol, and next symbol. (ELF - starts real symbols at 1). */ + /* + * Scan for closest preceding symbol, and next symbol. (ELF + * starts real symbols at 1). + */ for (i = 1; i < kallsyms->num_symtab; i++) { const Elf_Sym *sym = &kallsyms->symtab[i]; unsigned long thisval = kallsyms_symbol_value(sym); @@ -4127,8 +4151,10 @@ static const char *find_kallsyms_symbol(struct module *mod, if (sym->st_shndx == SHN_UNDEF) continue; - /* We ignore unnamed symbols: they're uninformative - * and inserted at a whim. */ + /* + * We ignore unnamed symbols: they're uninformative + * and inserted at a whim. + */ if (*kallsyms_symbol_name(kallsyms, i) == '\0' || is_arm_mapping_symbol(kallsyms_symbol_name(kallsyms, i))) continue; @@ -4158,8 +4184,10 @@ void * __weak dereference_module_function_descriptor(struct module *mod, return ptr; } -/* For kallsyms to ask for address resolution. NULL means not found. Careful - * not to lock to avoid deadlock on oopses, simply disable preemption. */ +/* + * For kallsyms to ask for address resolution. NULL means not found. Careful + * not to lock to avoid deadlock on oopses, simply disable preemption. + */ const char *module_address_lookup(unsigned long addr, unsigned long *size, unsigned long *offset, @@ -4417,11 +4445,12 @@ static int m_show(struct seq_file *m, void *p) return 0; } -/* Format: modulename size refcount deps address - - Where refcount is a number or -, and deps is a comma-separated list - of depends or -. -*/ +/* + * Format: modulename size refcount deps address + * + * Where refcount is a number or -, and deps is a comma-separated list + * of depends or -. + */ static const struct seq_operations modules_op = { .start = m_start, .next = m_next, @@ -4593,8 +4622,10 @@ void print_modules(void) } #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. */ +/* + * 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, From 0801a0073f86e020987acbbd96b50f9c85d79de8 Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Mon, 23 Nov 2020 11:23:14 +0100 Subject: [PATCH 08/15] module: drop version-attribute alignment Commit 98562ad8cb03 ("module: explicitly align module_version_attribute structure") added an alignment attribute to the struct module_version_attribute type in order to fix an alignment issue on m68k where the structure is 2-byte aligned while MODULE_VERSION() forced the __modver section entries to be 4-byte aligned (sizeof(void *)). This was essentially an alternative fix to the problem addressed by b4bc842802db ("module: deal with alignment issues in built-in module versions") which used the array-of-pointer trick to prevent gcc from increasing alignment of the version attribute entries. And with the pointer indirection in place there's no need to increase the alignment of the type. Link: https://lore.kernel.org/lkml/20201103175711.10731-1-johan@kernel.org Signed-off-by: Johan Hovold Signed-off-by: Jessica Yu --- include/linux/module.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/module.h b/include/linux/module.h index 7ccdf87f376f..293250958512 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -66,7 +66,7 @@ struct module_version_attribute { struct module_attribute mattr; const char *module_name; const char *version; -} __attribute__ ((__aligned__(sizeof(void *)))); +}; extern ssize_t __modver_version_show(struct module_attribute *, struct module_kobject *, char *); From b112082c8930e7aa72422484b2d31d3aa06f58bc Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Mon, 23 Nov 2020 11:23:15 +0100 Subject: [PATCH 09/15] module: simplify version-attribute handling Instead of using the array-of-pointers trick to avoid having gcc mess up the built-in module-version array stride, specify type alignment when declaring entries to prevent gcc from increasing alignment. This is essentially an alternative (one-line) fix to the problem addressed by commit b4bc842802db ("module: deal with alignment issues in built-in module versions"). gcc can increase the alignment of larger objects with static extent as an optimisation, but this can be suppressed by using the aligned attribute when declaring variables. Note that we have been relying on this behaviour for kernel parameters for 16 years and it indeed hasn't changed since the introduction of the aligned attribute in gcc-3.1. Link: https://lore.kernel.org/lkml/20201103175711.10731-1-johan@kernel.org Signed-off-by: Johan Hovold Signed-off-by: Jessica Yu --- include/linux/module.h | 26 +++++++++++++------------- kernel/params.c | 10 ++++------ 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/include/linux/module.h b/include/linux/module.h index 293250958512..5958075ea3f4 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -266,20 +266,20 @@ extern typeof(name) __mod_##type##__##name##_device_table \ #else #define MODULE_VERSION(_version) \ MODULE_INFO(version, _version); \ - static struct module_version_attribute ___modver_attr = { \ - .mattr = { \ - .attr = { \ - .name = "version", \ - .mode = S_IRUGO, \ + static struct module_version_attribute __modver_attr \ + __used __section("__modver") \ + __aligned(__alignof__(struct module_version_attribute)) \ + = { \ + .mattr = { \ + .attr = { \ + .name = "version", \ + .mode = S_IRUGO, \ + }, \ + .show = __modver_version_show, \ }, \ - .show = __modver_version_show, \ - }, \ - .module_name = KBUILD_MODNAME, \ - .version = _version, \ - }; \ - static const struct module_version_attribute \ - __used __section("__modver") \ - * __moduleparam_const __modver_attr = &___modver_attr + .module_name = KBUILD_MODNAME, \ + .version = _version, \ + }; #endif /* Optional firmware file (or files) needed by the module diff --git a/kernel/params.c b/kernel/params.c index 3835fb82c64b..aa7d6f2213f1 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -843,18 +843,16 @@ ssize_t __modver_version_show(struct module_attribute *mattr, return scnprintf(buf, PAGE_SIZE, "%s\n", vattr->version); } -extern const struct module_version_attribute *__start___modver[]; -extern const struct module_version_attribute *__stop___modver[]; +extern const struct module_version_attribute __start___modver[]; +extern const struct module_version_attribute __stop___modver[]; static void __init version_sysfs_builtin(void) { - const struct module_version_attribute **p; + const struct module_version_attribute *vattr; struct module_kobject *mk; int err; - for (p = __start___modver; p < __stop___modver; p++) { - const struct module_version_attribute *vattr = *p; - + for (vattr = __start___modver; vattr < __stop___modver; vattr++) { mk = locate_module_kobject(vattr->module_name); if (mk) { err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr); From 8d6615f1fccc4f39d7d3dcf286b33e8a1e833d2b Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Mon, 23 Nov 2020 11:23:17 +0100 Subject: [PATCH 10/15] params: drop redundant "unused" attributes Drop the redundant "unused" attributes from module-parameter structures already marked "used". Link: https://lore.kernel.org/lkml/20201103175711.10731-1-johan@kernel.org Signed-off-by: Johan Hovold Signed-off-by: Jessica Yu --- include/linux/moduleparam.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h index 6388eb9734a5..742074ad9f6e 100644 --- a/include/linux/moduleparam.h +++ b/include/linux/moduleparam.h @@ -22,7 +22,7 @@ #define __MODULE_INFO(tag, name, info) \ static const char __UNIQUE_ID(name)[] \ - __used __section(".modinfo") __attribute__((unused, aligned(1))) \ + __used __section(".modinfo") __attribute__((aligned(1))) \ = __MODULE_INFO_PREFIX __stringify(tag) "=" info #define __MODULE_PARM_TYPE(name, _type) \ @@ -289,7 +289,7 @@ struct kparam_array static const char __param_str_##name[] = prefix #name; \ static struct kernel_param __moduleparam_const __param_##name \ __used \ - __section("__param") __attribute__ ((unused, aligned(sizeof(void *)))) \ + __section("__param") __attribute__ ((aligned(sizeof(void *)))) \ = { __param_str_##name, THIS_MODULE, ops, \ VERIFY_OCTAL_PERMISSIONS(perm), level, flags, { arg } } From fe2f4fe139b321a38daafc715aeb7d21d9e8e5ad Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Mon, 23 Nov 2020 11:23:18 +0100 Subject: [PATCH 11/15] params: use type alignment for kernel parameters Specify type alignment for kernel parameters instead of sizeof(void *). The alignment attribute is used to prevent gcc from increasing the alignment of objects with static extent as an optimisation, something which would mess up the __param array stride. Using __alignof__(struct kernel_param) rather than sizeof(void *) is preferred since it better indicates why it is there and doesn't break should the type size or alignment change. Note that on m68k the alignment of struct kernel_param is actually two and that adding a 1- or 2-byte field to the 20-byte struct would cause a breakage with the current 4-byte alignment. Link: https://lore.kernel.org/lkml/20201103175711.10731-1-johan@kernel.org Signed-off-by: Johan Hovold Signed-off-by: Jessica Yu --- include/linux/moduleparam.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h index 742074ad9f6e..15ecc6cc3a3b 100644 --- a/include/linux/moduleparam.h +++ b/include/linux/moduleparam.h @@ -288,8 +288,8 @@ struct kparam_array /* Default value instead of permissions? */ \ static const char __param_str_##name[] = prefix #name; \ static struct kernel_param __moduleparam_const __param_##name \ - __used \ - __section("__param") __attribute__ ((aligned(sizeof(void *)))) \ + __used __section("__param") \ + __aligned(__alignof__(struct kernel_param)) \ = { __param_str_##name, THIS_MODULE, ops, \ VERIFY_OCTAL_PERMISSIONS(perm), level, flags, { arg } } From 2aec389e19150ed3bf67ab708f2435563f76050f Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Mon, 23 Nov 2020 11:23:19 +0100 Subject: [PATCH 12/15] params: clean up module-param macros Clean up the module-param macros by adding some indentation and using the __aligned() macro to improve readability. Link: https://lore.kernel.org/lkml/20201103175711.10731-1-johan@kernel.org Signed-off-by: Johan Hovold Signed-off-by: Jessica Yu --- include/linux/moduleparam.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h index 15ecc6cc3a3b..eed280fae433 100644 --- a/include/linux/moduleparam.h +++ b/include/linux/moduleparam.h @@ -21,12 +21,12 @@ #define MAX_PARAM_PREFIX_LEN (64 - sizeof(unsigned long)) #define __MODULE_INFO(tag, name, info) \ -static const char __UNIQUE_ID(name)[] \ - __used __section(".modinfo") __attribute__((aligned(1))) \ - = __MODULE_INFO_PREFIX __stringify(tag) "=" info + static const char __UNIQUE_ID(name)[] \ + __used __section(".modinfo") __aligned(1) \ + = __MODULE_INFO_PREFIX __stringify(tag) "=" info #define __MODULE_PARM_TYPE(name, _type) \ - __MODULE_INFO(parmtype, name##type, #name ":" _type) + __MODULE_INFO(parmtype, name##type, #name ":" _type) /* One for each parameter, describing how to use it. Some files do multiple of these per line, so can't just use MODULE_INFO. */ From 147ad605dc12c515c97136899ccb5c70e6c674e1 Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Mon, 23 Nov 2020 11:23:16 +0100 Subject: [PATCH 13/15] init: use type alignment for kernel parameters Specify type alignment for kernel parameters instead of sizeof(long). The alignment attribute is used to prevent gcc from increasing the alignment of objects with static extent as an optimisation, something which would mess up the __setup array stride. Using __alignof__(struct obs_kernel_param) rather than sizeof(long) is preferred since it better indicates why it is there and doesn't break should the type size or alignment change. Note that on m68k the alignment of struct obs_kernel_param is actually two and that adding a 1- or 2-byte field to the 12-byte struct would cause a breakage with the current 4-byte alignment. Link: https://lore.kernel.org/lkml/20201103175711.10731-1-johan@kernel.org Signed-off-by: Johan Hovold Signed-off-by: Jessica Yu --- include/linux/init.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/init.h b/include/linux/init.h index 7b53cb3092ee..e668832ef66a 100644 --- a/include/linux/init.h +++ b/include/linux/init.h @@ -255,7 +255,7 @@ struct obs_kernel_param { __aligned(1) = str; \ static struct obs_kernel_param __setup_##unique_id \ __used __section(".init.setup") \ - __attribute__((aligned((sizeof(long))))) \ + __aligned(__alignof__(struct obs_kernel_param)) \ = { __setup_str_##unique_id, fn, early } #define __setup(str, fn) \ From 2d26c716fc49f41a63e1efe8f1f772b0adeaacef Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Mon, 7 Dec 2020 10:13:08 +0100 Subject: [PATCH 14/15] module: drop semicolon from version macro Drop the trailing semicolon from the MODULE_VERSION() macro definition which was left when removing the array-of-pointer indirection. Signed-off-by: Johan Hovold Signed-off-by: Jessica Yu --- include/linux/module.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/module.h b/include/linux/module.h index 5958075ea3f4..e7a619c2457e 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -279,7 +279,7 @@ extern typeof(name) __mod_##type##__##name##_device_table \ }, \ .module_name = KBUILD_MODNAME, \ .version = _version, \ - }; + } #endif /* Optional firmware file (or files) needed by the module From 38dc717e97153e46375ee21797aa54777e5498f3 Mon Sep 17 00:00:00 2001 From: Jessica Yu Date: Fri, 27 Nov 2020 10:09:39 +0100 Subject: [PATCH 15/15] module: delay kobject uevent until after module init call Apparently there has been a longstanding race between udev/systemd and the module loader. Currently, the module loader sends a uevent right after sysfs initialization, but before the module calls its init function. However, some udev rules expect that the module has initialized already upon receiving the uevent. This race has been triggered recently (see link in references) in some systemd mount unit files. For instance, the configfs module creates the /sys/kernel/config mount point in its init function, however the module loader issues the uevent before this happens. sys-kernel-config.mount expects to be able to mount /sys/kernel/config upon receipt of the module loading uevent, but if the configfs module has not called its init function yet, then this directory will not exist and the mount unit fails. A similar situation exists for sys-fs-fuse-connections.mount, as the fuse sysfs mount point is created during the fuse module's init function. If udev is faster than module initialization then the mount unit would fail in a similar fashion. To fix this race, delay the module KOBJ_ADD uevent until after the module has finished calling its init routine. References: https://github.com/systemd/systemd/issues/17586 Reviewed-by: Greg Kroah-Hartman Tested-By: Nicolas Morey-Chaisemartin Signed-off-by: Jessica Yu --- kernel/module.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index a40ec708f8f2..e1dd0df57244 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1897,7 +1897,6 @@ static int mod_sysfs_init(struct module *mod) if (err) mod_kobject_put(mod); - /* delay uevent until full sysfs population */ out: return err; } @@ -1934,7 +1933,6 @@ static int mod_sysfs_setup(struct module *mod, add_sect_attrs(mod, info); add_notes_attrs(mod, info); - kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD); return 0; out_unreg_modinfo_attrs: @@ -3656,6 +3654,9 @@ static noinline int do_init_module(struct module *mod) blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_LIVE, mod); + /* Delay uevent until module has finished its init routine */ + kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD); + /* * We need to finish all async code before the module init sequence * is done. This has potential to deadlock. For example, a newly