| Message ID | 20231122221814.139916-1-deller@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 22, 2023 at 11:18:10PM +0100, deller@kernel.org wrote: > From: Helge Deller <deller@gmx.de> > My questions: > - Am I wrong with my analysis? This would typically of course depend on the arch, but whether it helps is what I would like to see with real numbers rather then speculation. Howeer, I don't expect some of these are hot paths except maybe the table lookups. So could you look at some perf stat differences without / with alignment ? Other than bootup live patching would be a good test case. We have selftest for modules, the script in selftests tools/testing/selftests/kmod/kmod.sh is pretty aggressive, but the live patching tests might be better suited. > - What does people see on other architectures? > - Does it make sense to add a compile- and runtime-check, like the patch below, to the kernel? The chatty aspects really depend on the above results. Aren't there some archs where an unaligned access would actually crash? Why hasn't that happened? Luis
On Tue, Dec 19, 2023 at 01:26:49PM -0800, Luis Chamberlain wrote: > On Wed, Nov 22, 2023 at 11:18:10PM +0100, deller@kernel.org wrote: > > From: Helge Deller <deller@gmx.de> > > My questions: > > - Am I wrong with my analysis? > > This would typically of course depend on the arch, but whether it helps > is what I would like to see with real numbers rather then speculation. > Howeer, I don't expect some of these are hot paths except maybe the > table lookups. So could you look at some perf stat differences > without / with alignment ? Other than bootup live patching would be > a good test case. We have selftest for modules, the script in selftests > tools/testing/selftests/kmod/kmod.sh is pretty aggressive, but the live > patching tests might be better suited. > > > - What does people see on other architectures? > > - Does it make sense to add a compile- and runtime-check, like the patch below, to the kernel? > > The chatty aspects really depend on the above results. > > Aren't there some archs where an unaligned access would actually crash? > Why hasn't that happened? I've gone down through memory lane and we have discussed this before. It would seem this misalignment should not affect performance, and this should not be an issue unless you have a buggy exception hanlder. We actually ran into one before. Please refer to merge commit e74acdf55da6649dd30be5b621a93b71cbe7f3f9 Luis
On Thu, Nov 23, 2023 at 7:18 AM <deller@kernel.org> wrote: > > From: Helge Deller <deller@gmx.de> > > While working on the 64-bit parisc kernel, I noticed that the __ksymtab[] > table was not correctly 64-bit aligned in many modules. > The following patches do fix some of those issues in the generic code. > > But further investigation shows that multiple sections in the kernel and in > modules are possibly not correctly aligned, and thus may lead to performance > degregations at runtime (small on x86, huge on parisc, sparc and others which > need exception handlers). Sometimes wrong alignments may also be simply hidden > by the linker or kernel module loader which pulls in the sections by luck with > a correct alignment (e.g. because the previous section was aligned already). > > An objdump on a x86 module shows e.g.: > > ./kernel/net/netfilter/nf_log_syslog.ko: file format elf64-x86-64 > Sections: > Idx Name Size VMA LMA File off Algn > 0 .text 00001fdf 0000000000000000 0000000000000000 00000040 2**4 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > 1 .init.text 000000f6 0000000000000000 0000000000000000 00002020 2**4 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > 2 .exit.text 0000005c 0000000000000000 0000000000000000 00002120 2**4 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > 3 .rodata.str1.8 000000dc 0000000000000000 0000000000000000 00002180 2**3 > CONTENTS, ALLOC, LOAD, READONLY, DATA > 4 .rodata.str1.1 0000030a 0000000000000000 0000000000000000 0000225c 2**0 > CONTENTS, ALLOC, LOAD, READONLY, DATA > 5 .rodata 000000b0 0000000000000000 0000000000000000 00002580 2**5 > CONTENTS, ALLOC, LOAD, READONLY, DATA > 6 .modinfo 0000019e 0000000000000000 0000000000000000 00002630 2**0 > CONTENTS, ALLOC, LOAD, READONLY, DATA > 7 .return_sites 00000034 0000000000000000 0000000000000000 000027ce 2**0 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA > 8 .call_sites 0000029c 0000000000000000 0000000000000000 00002802 2**0 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA > > In this example I believe the ".return_sites" and ".call_sites" should have > an alignment of at least 32-bit (4 bytes). > > On other architectures or modules other sections like ".altinstructions" or > "__ex_table" may show up wrongly instead. > > In general I think it would be beneficial to search for wrong alignments at > link time, and maybe at runtime. > > The patch at the end of this cover letter > - adds compile time checks to the "modpost" tool, and > - adds a runtime check to the kernel module loader at runtime. > And it will possibly show false positives too (!!!) > I do understand that some of those sections are not performce critical > and thus any alignment is OK. > > The modpost patch will emit at compile time such warnings (on x86-64 kernel build): > > WARNING: modpost: vmlinux: section .initcall7.init (type 1, flags 2) has alignment of 1, expected at least 4. > Maybe you need to add ALIGN() to the modules.lds file (or fix modpost) ? > WARNING: modpost: vmlinux: section .altinstructions (type 1, flags 2) has alignment of 1, expected at least 2. > WARNING: modpost: vmlinux: section .initcall6.init (type 1, flags 2) has alignment of 1, expected at least 4. > WARNING: modpost: vmlinux: section .initcallearly.init (type 1, flags 2) has alignment of 1, expected at least 4. > WARNING: modpost: vmlinux: section .rodata.cst2 (type 1, flags 18) has alignment of 2, expected at least 64. > WARNING: modpost: vmlinux: section .static_call_tramp_key (type 1, flags 2) has alignment of 1, expected at least 8. > WARNING: modpost: vmlinux: section .con_initcall.init (type 1, flags 2) has alignment of 1, expected at least 8. > WARNING: modpost: vmlinux: section __bug_table (type 1, flags 3) has alignment of 1, expected at least 4. > ... modpost acts on vmlinux.o instead of vmlinux. vmlinux.o is a relocatable ELF, which is not a real layout because no linker script has been considered yet at this point. vmlinux is an executable ELF, produced by a linker, with the linker script taken into consideration to determine the final section/symbol layout. So, checking this in modpost is meaningless. I did not check the module checking code, but modules are also relocatable ELF. > > while the kernel module loader may show at runtime: > > ** WARNING **: module: efivarfs, dest=0xffffffffc02d08d2, section=.retpoline_sites possibly not correctly aligned. > ** WARNING **: module: efivarfs, dest=0xffffffffc02d0bae, section=.ibt_endbr_seal possibly not correctly aligned. > ** WARNING **: module: efivarfs, dest=0xffffffffc02d0be6, section=.orc_unwind possibly not correctly aligned. > ** WARNING **: module: efivarfs, dest=0xffffffffc02d12a6, section=.orc_unwind_ip possibly not correctly aligned. > ** WARNING **: module: efivarfs, dest=0xffffffffc02d178c, section=.note.Linux possibly not correctly aligned. > ** WARNING **: module: efivarfs, dest=0xffffffffc02d17bc, section=.orc_header possibly not correctly aligned. > ** WARNING **: module: xt_addrtype, dest=0xffffffffc01ef18a, size=0x00000020, section=.return_sites > > My questions: > - Am I wrong with my analysis? > - What does people see on other architectures? > - Does it make sense to add a compile- and runtime-check, like the patch below, to the kernel? > > Helge > > --- > > From: Helge Deller <deller@gmx.de> > Subject: [PATCH] MODPOST: Detect and report possible section alignment issues > > IMPORTANT: THIS PATCH IS NOT INTENDED TO BE APPLIED !!!! > > The main idea here is to check at compile time (during modpost run) and at > runtime (when loading a module) if the sections in kernel modules (and > vmlinux) are correctly aligned in memory and report if a wrong alignment is > suspected. > > It WILL report false positives. Many section names still need to be added to > the various tables. > But even at this stage it gives some insight at the various possibly wrong > section alignments. > > Signed-off-by: Helge Deller <deller@gmx.de> > > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 98fedfdb8db5..88201d6b0c17 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -2277,6 +2277,10 @@ static int move_module(struct module *mod, struct load_info *info) > ret = -ENOEXEC; > goto out_enomem; > } > + if (((uintptr_t)dest) & (BITS_PER_LONG/8 - 1)) { > + printk("** WARNING **: \tmodule: %s, dest=0x%lx, section=%s possibly not correctly aligned.\n", > + mod->name, (long)dest, info->secstrings + shdr->sh_name); > + } > memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size); > } > /* > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost > index 739402f45509..2add144a05e3 100644 > --- a/scripts/Makefile.modpost > +++ b/scripts/Makefile.modpost > @@ -49,6 +49,8 @@ modpost-args = \ > $(if $(KBUILD_NSDEPS),-d $(MODULES_NSDEPS)) \ > $(if $(CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS)$(KBUILD_NSDEPS),-N) \ > $(if $(findstring 1, $(KBUILD_EXTRA_WARN)),-W) \ > + $(if $(CONFIG_64BIT),-6) \ > + $(if $(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS),-R) \ > -o $@ > > modpost-deps := $(MODPOST) > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index cb6406f485a9..70c69db6a17c 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -43,6 +43,10 @@ static bool ignore_missing_files; > /* If set to 1, only warn (instead of error) about missing ns imports */ > static bool allow_missing_ns_imports; > > +/* is target a 64-bit platform and has it prel32 relocation support? */ > +static bool target_64bit; > +static bool target_prel32_relocations; > + > static bool error_occurred; > > static bool extra_warn; > @@ -1493,6 +1497,76 @@ static void check_sec_ref(struct module *mod, struct elf_info *elf) > } > } > > +/** > + * Check alignment of sections in modules. > + **/ > +static void check_sec_alignment(struct module *mod, struct elf_info *elf) > +{ > + /* sections that may use PREL32 relocations and only need 4-byte alignment */ > + static const char *const prel32_sec_list[] = { > + "__tracepoints_ptrs", > + "__ksymtab", > + "__bug_table", > + ".smp_locks", > + NULL > + }; > + /* sections that are fine with any/1-byte alignment */ > + static const char *const byte_sec_list[] = { > + ".modinfo", > + ".init.ramfs", > + NULL > + }; > + /* sections with special alignment */ > + static struct { int align; const char *name; } const special_list[] = { > + { 64, ".rodata.cst2" }, > + { 0, NULL } > + }; > + > + int i; > + > + /* ignore vmlinux for now? */ > + // if (mod->is_vmlinux) return; > + > + /* Walk through all sections */ > + for (i = 0; i < elf->num_sections; i++) { > + Elf_Shdr *sechdr = &elf->sechdrs[i]; > + const char *sec = sech_name(elf, sechdr); > + const char *modname = mod->name; > + const int is_shalign = sechdr->sh_addralign; > + int should_shalign; > + int k; > + > + /* ignore some sections */ > + if ((sechdr->sh_type == SHT_NULL) || > + !(sechdr->sh_flags & SHF_ALLOC)) > + continue; > + > + /* default alignment is 8 for 64-bit and 4 for 32-bit targets * */ > + should_shalign = target_64bit ? 8 : 4; > + if (match(sec, prel32_sec_list)) > + should_shalign = target_prel32_relocations ? 4 : should_shalign; > + else if (strstr(sec, "text")) /* assume text segments to be 4-byte aligned */ > + should_shalign = 4; > + else if (match(sec, byte_sec_list)) /* any alignment is OK. */ > + continue; > + else { > + /* search in list with special alignment */ > + k = 0; > + while (special_list[k].align && strstarts(sec, special_list[k].name)) { > + should_shalign = special_list[k].align; > + break; > + } > + } > + > + if (is_shalign >= should_shalign) > + continue; > + > + warn("%s: section %s (type %d, flags %lu) has alignment of %d, expected at least %d.\n" > + "Maybe you need to add ALIGN() to the modules.lds file (or fix modpost) ?\n", > + modname, sec, sechdr->sh_type, sechdr->sh_flags, is_shalign, should_shalign); > + } > +} > + > static char *remove_dot(char *s) > { > size_t n = strcspn(s, "."); > @@ -1653,6 +1727,8 @@ static void read_symbols(const char *modname) > handle_moddevtable(mod, &info, sym, symname); > } > > + check_sec_alignment(mod, &info); > + > check_sec_ref(mod, &info); > > if (!mod->is_vmlinux) { > @@ -2183,7 +2259,7 @@ int main(int argc, char **argv) > LIST_HEAD(dump_lists); > struct dump_list *dl, *dl2; > > - while ((opt = getopt(argc, argv, "ei:MmnT:to:au:WwENd:")) != -1) { > + while ((opt = getopt(argc, argv, "ei:MmnT:to:au:WwENd:6R")) != -1) { > switch (opt) { > case 'e': > external_module = true; > @@ -2232,6 +2308,12 @@ int main(int argc, char **argv) > case 'd': > missing_namespace_deps = optarg; > break; > + case '6': > + target_64bit = true; > + break; > + case 'R': > + target_prel32_relocations = true; > + break; > default: > exit(1); > } > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index 70c69db6a17c..b09ab081dc03 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -1510,15 +1510,24 @@ static void check_sec_alignment(struct module *mod, struct elf_info *elf) > ".smp_locks", > NULL > }; > - /* sections that are fine with any/1-byte alignment */ > + /* sections that are fine with any alignment */ > static const char *const byte_sec_list[] = { > ".modinfo", > ".init.ramfs", > NULL > }; > - /* sections with special alignment */ > + /* sections with special given minimal alignment. Checked with > + * startswith(). */ > static struct { int align; const char *name; } const special_list[] = { > { 64, ".rodata.cst2" }, > + { 2, ".altinstructions" }, /* at least on x86 */ > + { 1, ".altinstr_replacement" }, > + { 1, ".altinstr_aux" }, > + { 4, ".call_sites" }, > + { 4, ".return_sites" }, > + { 1, ".note.Linux" }, /* correct? */ > + { 4, "__ex_table" }, > + { 4, ".initcall" }, /* at least 4 ? */ > { 0, NULL } > }; > > @@ -1545,16 +1554,17 @@ static void check_sec_alignment(struct module *mod, struct elf_info *elf) > should_shalign = target_64bit ? 8 : 4; > if (match(sec, prel32_sec_list)) > should_shalign = target_prel32_relocations ? 4 : should_shalign; > - else if (strstr(sec, "text")) /* assume text segments to be 4-byte aligned */ > - should_shalign = 4; > else if (match(sec, byte_sec_list)) /* any alignment is OK. */ > continue; > + else if (strstr(sec, "text")) /* assume text segments to be 4-byte aligned */ > + should_shalign = 4; > else { > /* search in list with special alignment */ > - k = 0; > - while (special_list[k].align && strstarts(sec, special_list[k].name)) { > - should_shalign = special_list[k].align; > - break; > + for (k = 0; special_list[k].align; k++) { > + if (strstarts(sec, special_list[k].name)) { > + should_shalign = special_list[k].align; > + break; > + } > } > } > > Helge Deller (4): > linux/export: Fix alignment for 64-bit ksymtab entries > modules: Ensure 64-bit alignment on __ksymtab_* sections > vmlinux.lds.h: Fix alignment for __ksymtab*, __kcrctab_* and > .pci_fixup sections > modules: Add missing entry for __ex_table > > include/asm-generic/vmlinux.lds.h | 5 +++++ > include/linux/export-internal.h | 5 ++++- > scripts/module.lds.S | 9 +++++---- > 3 files changed, 14 insertions(+), 5 deletions(-) > > -- > 2.41.0 > -- Best Regards Masahiro Yamada
On Thu, Dec 21, 2023 at 10:40 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Thu, Nov 23, 2023 at 7:18 AM <deller@kernel.org> wrote: > > > > From: Helge Deller <deller@gmx.de> > > > > While working on the 64-bit parisc kernel, I noticed that the __ksymtab[] > > table was not correctly 64-bit aligned in many modules. > > The following patches do fix some of those issues in the generic code. > > > > But further investigation shows that multiple sections in the kernel and in > > modules are possibly not correctly aligned, and thus may lead to performance > > degregations at runtime (small on x86, huge on parisc, sparc and others which > > need exception handlers). Sometimes wrong alignments may also be simply hidden > > by the linker or kernel module loader which pulls in the sections by luck with > > a correct alignment (e.g. because the previous section was aligned already). > > > > An objdump on a x86 module shows e.g.: > > > > ./kernel/net/netfilter/nf_log_syslog.ko: file format elf64-x86-64 > > Sections: > > Idx Name Size VMA LMA File off Algn > > 0 .text 00001fdf 0000000000000000 0000000000000000 00000040 2**4 > > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > > 1 .init.text 000000f6 0000000000000000 0000000000000000 00002020 2**4 > > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > > 2 .exit.text 0000005c 0000000000000000 0000000000000000 00002120 2**4 > > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > > 3 .rodata.str1.8 000000dc 0000000000000000 0000000000000000 00002180 2**3 > > CONTENTS, ALLOC, LOAD, READONLY, DATA > > 4 .rodata.str1.1 0000030a 0000000000000000 0000000000000000 0000225c 2**0 > > CONTENTS, ALLOC, LOAD, READONLY, DATA > > 5 .rodata 000000b0 0000000000000000 0000000000000000 00002580 2**5 > > CONTENTS, ALLOC, LOAD, READONLY, DATA > > 6 .modinfo 0000019e 0000000000000000 0000000000000000 00002630 2**0 > > CONTENTS, ALLOC, LOAD, READONLY, DATA > > 7 .return_sites 00000034 0000000000000000 0000000000000000 000027ce 2**0 > > CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA > > 8 .call_sites 0000029c 0000000000000000 0000000000000000 00002802 2**0 > > CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA > > > > In this example I believe the ".return_sites" and ".call_sites" should have > > an alignment of at least 32-bit (4 bytes). > > > > On other architectures or modules other sections like ".altinstructions" or > > "__ex_table" may show up wrongly instead. > > > > In general I think it would be beneficial to search for wrong alignments at > > link time, and maybe at runtime. > > > > The patch at the end of this cover letter > > - adds compile time checks to the "modpost" tool, and > > - adds a runtime check to the kernel module loader at runtime. > > And it will possibly show false positives too (!!!) > > I do understand that some of those sections are not performce critical > > and thus any alignment is OK. > > > > The modpost patch will emit at compile time such warnings (on x86-64 kernel build): > > > > WARNING: modpost: vmlinux: section .initcall7.init (type 1, flags 2) has alignment of 1, expected at least 4. > > Maybe you need to add ALIGN() to the modules.lds file (or fix modpost) ? > > WARNING: modpost: vmlinux: section .altinstructions (type 1, flags 2) has alignment of 1, expected at least 2. > > WARNING: modpost: vmlinux: section .initcall6.init (type 1, flags 2) has alignment of 1, expected at least 4. > > WARNING: modpost: vmlinux: section .initcallearly.init (type 1, flags 2) has alignment of 1, expected at least 4. > > WARNING: modpost: vmlinux: section .rodata.cst2 (type 1, flags 18) has alignment of 2, expected at least 64. > > WARNING: modpost: vmlinux: section .static_call_tramp_key (type 1, flags 2) has alignment of 1, expected at least 8. > > WARNING: modpost: vmlinux: section .con_initcall.init (type 1, flags 2) has alignment of 1, expected at least 8. > > WARNING: modpost: vmlinux: section __bug_table (type 1, flags 3) has alignment of 1, expected at least 4. > > ... > > > > > modpost acts on vmlinux.o instead of vmlinux. > > > vmlinux.o is a relocatable ELF, which is not a real layout > because no linker script has been considered yet at this > point. > > > vmlinux is an executable ELF, produced by a linker, > with the linker script taken into consideration > to determine the final section/symbol layout. > > > So, checking this in modpost is meaningless. > > > > I did not check the module checking code, but > modules are also relocatable ELF. Sorry, I replied too early. (Actually I replied without reading your modpost code). Now, I understand what your checker is doing. I did not test how many false positives are produced, but it catches several suspicious mis-alignments. However, I am not convinced with this warning. + warn("%s: section %s (type %d, flags %lu) has alignment of %d, expected at least %d.\n" + "Maybe you need to add ALIGN() to the modules.lds file (or fix modpost) ?\n", + modname, sec, sechdr->sh_type, sechdr->sh_flags, is_shalign, should_shalign); + } Adding ALGIN() hides the real problem. I think the real problem is that not enough alignment was requested in the code. For example, the right fix for ".initcall7.init" should be this: diff --git a/include/linux/init.h b/include/linux/init.h index 3fa3f6241350..650311e4b215 100644 --- a/include/linux/init.h +++ b/include/linux/init.h @@ -264,6 +264,7 @@ extern struct module __this_module; #define ____define_initcall(fn, __stub, __name, __sec) \ __define_initcall_stub(__stub, fn) \ asm(".section \"" __sec "\", \"a\" \n" \ + ".balign 4 \n" \ __stringify(__name) ": \n" \ ".long " __stringify(__stub) " - . \n" \ ".previous \n"); \ Then, "this section requires at least 4 byte alignment" is recorded in the sh_addralign field. Then, the rest is the linker's job. We should not tweak the linker script. -- Best Regards Masahiro Yamada
On 12/21/23 16:42, Masahiro Yamada wrote: > On Thu, Dec 21, 2023 at 10:40 PM Masahiro Yamada <masahiroy@kernel.org> wrote: >> >> On Thu, Nov 23, 2023 at 7:18 AM <deller@kernel.org> wrote: >>> >>> From: Helge Deller <deller@gmx.de> >>> >>> While working on the 64-bit parisc kernel, I noticed that the __ksymtab[] >>> table was not correctly 64-bit aligned in many modules. >>> The following patches do fix some of those issues in the generic code. >>> >>> But further investigation shows that multiple sections in the kernel and in >>> modules are possibly not correctly aligned, and thus may lead to performance >>> degregations at runtime (small on x86, huge on parisc, sparc and others which >>> need exception handlers). Sometimes wrong alignments may also be simply hidden >>> by the linker or kernel module loader which pulls in the sections by luck with >>> a correct alignment (e.g. because the previous section was aligned already). >>> >>> An objdump on a x86 module shows e.g.: >>> >>> ./kernel/net/netfilter/nf_log_syslog.ko: file format elf64-x86-64 >>> Sections: >>> Idx Name Size VMA LMA File off Algn >>> 0 .text 00001fdf 0000000000000000 0000000000000000 00000040 2**4 >>> CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE >>> 1 .init.text 000000f6 0000000000000000 0000000000000000 00002020 2**4 >>> CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE >>> 2 .exit.text 0000005c 0000000000000000 0000000000000000 00002120 2**4 >>> CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE >>> 3 .rodata.str1.8 000000dc 0000000000000000 0000000000000000 00002180 2**3 >>> CONTENTS, ALLOC, LOAD, READONLY, DATA >>> 4 .rodata.str1.1 0000030a 0000000000000000 0000000000000000 0000225c 2**0 >>> CONTENTS, ALLOC, LOAD, READONLY, DATA >>> 5 .rodata 000000b0 0000000000000000 0000000000000000 00002580 2**5 >>> CONTENTS, ALLOC, LOAD, READONLY, DATA >>> 6 .modinfo 0000019e 0000000000000000 0000000000000000 00002630 2**0 >>> CONTENTS, ALLOC, LOAD, READONLY, DATA >>> 7 .return_sites 00000034 0000000000000000 0000000000000000 000027ce 2**0 >>> CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA >>> 8 .call_sites 0000029c 0000000000000000 0000000000000000 00002802 2**0 >>> CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA >>> >>> In this example I believe the ".return_sites" and ".call_sites" should have >>> an alignment of at least 32-bit (4 bytes). >>> >>> On other architectures or modules other sections like ".altinstructions" or >>> "__ex_table" may show up wrongly instead. >>> >>> In general I think it would be beneficial to search for wrong alignments at >>> link time, and maybe at runtime. >>> >>> The patch at the end of this cover letter >>> - adds compile time checks to the "modpost" tool, and >>> - adds a runtime check to the kernel module loader at runtime. >>> And it will possibly show false positives too (!!!) >>> I do understand that some of those sections are not performce critical >>> and thus any alignment is OK. >>> >>> The modpost patch will emit at compile time such warnings (on x86-64 kernel build): >>> >>> WARNING: modpost: vmlinux: section .initcall7.init (type 1, flags 2) has alignment of 1, expected at least 4. >>> Maybe you need to add ALIGN() to the modules.lds file (or fix modpost) ? >>> WARNING: modpost: vmlinux: section .altinstructions (type 1, flags 2) has alignment of 1, expected at least 2. >>> WARNING: modpost: vmlinux: section .initcall6.init (type 1, flags 2) has alignment of 1, expected at least 4. >>> WARNING: modpost: vmlinux: section .initcallearly.init (type 1, flags 2) has alignment of 1, expected at least 4. >>> WARNING: modpost: vmlinux: section .rodata.cst2 (type 1, flags 18) has alignment of 2, expected at least 64. >>> WARNING: modpost: vmlinux: section .static_call_tramp_key (type 1, flags 2) has alignment of 1, expected at least 8. >>> WARNING: modpost: vmlinux: section .con_initcall.init (type 1, flags 2) has alignment of 1, expected at least 8. >>> WARNING: modpost: vmlinux: section __bug_table (type 1, flags 3) has alignment of 1, expected at least 4. >>> ... >> >> >> >> >> modpost acts on vmlinux.o instead of vmlinux. >> >> >> vmlinux.o is a relocatable ELF, which is not a real layout >> because no linker script has been considered yet at this >> point. >> >> >> vmlinux is an executable ELF, produced by a linker, >> with the linker script taken into consideration >> to determine the final section/symbol layout. >> >> >> So, checking this in modpost is meaningless. >> >> >> >> I did not check the module checking code, but >> modules are also relocatable ELF. > > > > Sorry, I replied too early. > (Actually I replied without reading your modpost code). > > Now, I understand what your checker is doing. > > > I did not test how many false positives are produced, > but it catches several suspicious mis-alignments. Yes. > However, I am not convinced with this warning. > > > + warn("%s: section %s (type %d, flags %lu) has > alignment of %d, expected at least %d.\n" > + "Maybe you need to add ALIGN() to the modules.lds > file (or fix modpost) ?\n", > + modname, sec, sechdr->sh_type, sechdr->sh_flags, > is_shalign, should_shalign); > + } > > > Adding ALGIN() hides the real problem. Right. It took me some time to understand the effects here too. See below... > I think the real problem is that not enough alignment was requested > in the code. > > For example, the right fix for ".initcall7.init" should be this: > > diff --git a/include/linux/init.h b/include/linux/init.h > index 3fa3f6241350..650311e4b215 100644 > --- a/include/linux/init.h > +++ b/include/linux/init.h > @@ -264,6 +264,7 @@ extern struct module __this_module; > #define ____define_initcall(fn, __stub, __name, __sec) \ > __define_initcall_stub(__stub, fn) \ > asm(".section \"" __sec "\", \"a\" \n" \ > + ".balign 4 \n" \ > __stringify(__name) ": \n" \ > ".long " __stringify(__stub) " - . \n" \ > ".previous \n"); \ > > Then, "this section requires at least 4 byte alignment" > is recorded in the sh_addralign field. Yes, this is the important part. > Then, the rest is the linker's job. > > We should not tweak the linker script. That's right, but let's phrase it slightly different... There is *no need* to tweak the linker script, *if* the alignment gets correctly assigned by the inline assembly (like your initcall patch above). But on some platforms (e.g. on parisc) I noticed that this .balign was missing for some other sections, in which case the other (not preferred) possible option is to tweak the linker script. So I think we agree that fixing the inline assembly is the right way to go? Either way, a link-time check like the proposed modpost patch may catch section issue for upcoming/newly added sections too. Helge
On 12/20/23 20:40, Luis Chamberlain wrote: > On Tue, Dec 19, 2023 at 01:26:49PM -0800, Luis Chamberlain wrote: >> On Wed, Nov 22, 2023 at 11:18:10PM +0100, deller@kernel.org wrote: >>> From: Helge Deller <deller@gmx.de> >>> My questions: >>> - Am I wrong with my analysis? >> >> This would typically of course depend on the arch, but whether it helps >> is what I would like to see with real numbers rather then speculation. >> Howeer, I don't expect some of these are hot paths except maybe the >> table lookups. So could you look at some perf stat differences >> without / with alignment ? Other than bootup live patching would be >> a good test case. We have selftest for modules, the script in selftests >> tools/testing/selftests/kmod/kmod.sh is pretty aggressive, but the live >> patching tests might be better suited. >> >>> - What does people see on other architectures? >>> - Does it make sense to add a compile- and runtime-check, like the patch below, to the kernel? >> >> The chatty aspects really depend on the above results. >> >> Aren't there some archs where an unaligned access would actually crash? >> Why hasn't that happened? > > I've gone down through memory lane and we have discussed this before. > > It would seem this misalignment should not affect performance, and this > should not be an issue unless you have a buggy exception hanlder. We > actually ran into one before. Please refer to merge commit > > e74acdf55da6649dd30be5b621a93b71cbe7f3f9 Yes, this is the second time I stumbled over this issue. But let's continue discussing in the other mail thread... Helge
... > diff --git a/include/linux/init.h b/include/linux/init.h > index 3fa3f6241350..650311e4b215 100644 > --- a/include/linux/init.h > +++ b/include/linux/init.h > @@ -264,6 +264,7 @@ extern struct module __this_module; > #define ____define_initcall(fn, __stub, __name, __sec) \ > __define_initcall_stub(__stub, fn) \ > asm(".section \"" __sec "\", \"a\" \n" \ > + ".balign 4 \n" \ > __stringify(__name) ": \n" \ > ".long " __stringify(__stub) " - . \n" \ > ".previous \n"); \ > > > > Then, "this section requires at least 4 byte alignment" > is recorded in the sh_addralign field. Perhaps one of the headers should contain (something like): #ifdef CONFIG_64 #define BALIGN_PTR ".balign 8\n" #else #define BALIGN_PTR ".balign 4\n" #endif to make it all easier (although that example doesn't need it). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Dec 22, 2023 at 5:23 PM Helge Deller <deller@gmx.de> wrote: > > On 12/21/23 16:42, Masahiro Yamada wrote: > > On Thu, Dec 21, 2023 at 10:40 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > >> > >> On Thu, Nov 23, 2023 at 7:18 AM <deller@kernel.org> wrote: > >>> > >>> From: Helge Deller <deller@gmx.de> > >>> > >>> While working on the 64-bit parisc kernel, I noticed that the __ksymtab[] > >>> table was not correctly 64-bit aligned in many modules. > >>> The following patches do fix some of those issues in the generic code. > >>> > >>> But further investigation shows that multiple sections in the kernel and in > >>> modules are possibly not correctly aligned, and thus may lead to performance > >>> degregations at runtime (small on x86, huge on parisc, sparc and others which > >>> need exception handlers). Sometimes wrong alignments may also be simply hidden > >>> by the linker or kernel module loader which pulls in the sections by luck with > >>> a correct alignment (e.g. because the previous section was aligned already). > >>> > >>> An objdump on a x86 module shows e.g.: > >>> > >>> ./kernel/net/netfilter/nf_log_syslog.ko: file format elf64-x86-64 > >>> Sections: > >>> Idx Name Size VMA LMA File off Algn > >>> 0 .text 00001fdf 0000000000000000 0000000000000000 00000040 2**4 > >>> CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > >>> 1 .init.text 000000f6 0000000000000000 0000000000000000 00002020 2**4 > >>> CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > >>> 2 .exit.text 0000005c 0000000000000000 0000000000000000 00002120 2**4 > >>> CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > >>> 3 .rodata.str1.8 000000dc 0000000000000000 0000000000000000 00002180 2**3 > >>> CONTENTS, ALLOC, LOAD, READONLY, DATA > >>> 4 .rodata.str1.1 0000030a 0000000000000000 0000000000000000 0000225c 2**0 > >>> CONTENTS, ALLOC, LOAD, READONLY, DATA > >>> 5 .rodata 000000b0 0000000000000000 0000000000000000 00002580 2**5 > >>> CONTENTS, ALLOC, LOAD, READONLY, DATA > >>> 6 .modinfo 0000019e 0000000000000000 0000000000000000 00002630 2**0 > >>> CONTENTS, ALLOC, LOAD, READONLY, DATA > >>> 7 .return_sites 00000034 0000000000000000 0000000000000000 000027ce 2**0 > >>> CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA > >>> 8 .call_sites 0000029c 0000000000000000 0000000000000000 00002802 2**0 > >>> CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA > >>> > >>> In this example I believe the ".return_sites" and ".call_sites" should have > >>> an alignment of at least 32-bit (4 bytes). > >>> > >>> On other architectures or modules other sections like ".altinstructions" or > >>> "__ex_table" may show up wrongly instead. > >>> > >>> In general I think it would be beneficial to search for wrong alignments at > >>> link time, and maybe at runtime. > >>> > >>> The patch at the end of this cover letter > >>> - adds compile time checks to the "modpost" tool, and > >>> - adds a runtime check to the kernel module loader at runtime. > >>> And it will possibly show false positives too (!!!) > >>> I do understand that some of those sections are not performce critical > >>> and thus any alignment is OK. > >>> > >>> The modpost patch will emit at compile time such warnings (on x86-64 kernel build): > >>> > >>> WARNING: modpost: vmlinux: section .initcall7.init (type 1, flags 2) has alignment of 1, expected at least 4. > >>> Maybe you need to add ALIGN() to the modules.lds file (or fix modpost) ? > >>> WARNING: modpost: vmlinux: section .altinstructions (type 1, flags 2) has alignment of 1, expected at least 2. > >>> WARNING: modpost: vmlinux: section .initcall6.init (type 1, flags 2) has alignment of 1, expected at least 4. > >>> WARNING: modpost: vmlinux: section .initcallearly.init (type 1, flags 2) has alignment of 1, expected at least 4. > >>> WARNING: modpost: vmlinux: section .rodata.cst2 (type 1, flags 18) has alignment of 2, expected at least 64. > >>> WARNING: modpost: vmlinux: section .static_call_tramp_key (type 1, flags 2) has alignment of 1, expected at least 8. > >>> WARNING: modpost: vmlinux: section .con_initcall.init (type 1, flags 2) has alignment of 1, expected at least 8. > >>> WARNING: modpost: vmlinux: section __bug_table (type 1, flags 3) has alignment of 1, expected at least 4. > >>> ... > >> > >> > >> > >> > >> modpost acts on vmlinux.o instead of vmlinux. > >> > >> > >> vmlinux.o is a relocatable ELF, which is not a real layout > >> because no linker script has been considered yet at this > >> point. > >> > >> > >> vmlinux is an executable ELF, produced by a linker, > >> with the linker script taken into consideration > >> to determine the final section/symbol layout. > >> > >> > >> So, checking this in modpost is meaningless. > >> > >> > >> > >> I did not check the module checking code, but > >> modules are also relocatable ELF. > > > > > > > > Sorry, I replied too early. > > (Actually I replied without reading your modpost code). > > > > Now, I understand what your checker is doing. > > > > > > I did not test how many false positives are produced, > > but it catches several suspicious mis-alignments. > > Yes. > > > However, I am not convinced with this warning. > > > > > > + warn("%s: section %s (type %d, flags %lu) has > > alignment of %d, expected at least %d.\n" > > + "Maybe you need to add ALIGN() to the modules.lds > > file (or fix modpost) ?\n", > > + modname, sec, sechdr->sh_type, sechdr->sh_flags, > > is_shalign, should_shalign); > > + } > > > > > > Adding ALGIN() hides the real problem. > > Right. > It took me some time to understand the effects here too. > See below... > > > I think the real problem is that not enough alignment was requested > > in the code. > > > > For example, the right fix for ".initcall7.init" should be this: > > > > diff --git a/include/linux/init.h b/include/linux/init.h > > index 3fa3f6241350..650311e4b215 100644 > > --- a/include/linux/init.h > > +++ b/include/linux/init.h > > @@ -264,6 +264,7 @@ extern struct module __this_module; > > #define ____define_initcall(fn, __stub, __name, __sec) \ > > __define_initcall_stub(__stub, fn) \ > > asm(".section \"" __sec "\", \"a\" \n" \ > > + ".balign 4 \n" \ > > __stringify(__name) ": \n" \ > > ".long " __stringify(__stub) " - . \n" \ > > ".previous \n"); \ > > > > Then, "this section requires at least 4 byte alignment" > > is recorded in the sh_addralign field. > > Yes, this is the important part. > > > Then, the rest is the linker's job. > > > > We should not tweak the linker script. > > That's right, but let's phrase it slightly different... > There is *no need* to tweak the linker script, *if* the alignment > gets correctly assigned by the inline assembly (like your > initcall patch above). > But on some platforms (e.g. on parisc) I noticed that this .balign > was missing for some other sections, in which case the other (not preferred) > possible option is to tweak the linker script. > > So I think we agree that fixing the inline assembly is the right > way to go? Yes, I think so. > Either way, a link-time check like the proposed modpost patch > may catch section issue for upcoming/newly added sections too. Yes. This check seems to be useful.
diff --git a/kernel/module/main.c b/kernel/module/main.c index 98fedfdb8db5..88201d6b0c17 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2277,6 +2277,10 @@ static int move_module(struct module *mod, struct load_info *info) ret = -ENOEXEC; goto out_enomem; } + if (((uintptr_t)dest) & (BITS_PER_LONG/8 - 1)) { + printk("** WARNING **: \tmodule: %s, dest=0x%lx, section=%s possibly not correctly aligned.\n", + mod->name, (long)dest, info->secstrings + shdr->sh_name); + } memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size); } /* diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost index 739402f45509..2add144a05e3 100644 --- a/scripts/Makefile.modpost +++ b/scripts/Makefile.modpost @@ -49,6 +49,8 @@ modpost-args = \ $(if $(KBUILD_NSDEPS),-d $(MODULES_NSDEPS)) \ $(if $(CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS)$(KBUILD_NSDEPS),-N) \ $(if $(findstring 1, $(KBUILD_EXTRA_WARN)),-W) \ + $(if $(CONFIG_64BIT),-6) \ + $(if $(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS),-R) \ -o $@ modpost-deps := $(MODPOST) diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index cb6406f485a9..70c69db6a17c 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -43,6 +43,10 @@ static bool ignore_missing_files; /* If set to 1, only warn (instead of error) about missing ns imports */ static bool allow_missing_ns_imports; +/* is target a 64-bit platform and has it prel32 relocation support? */ +static bool target_64bit; +static bool target_prel32_relocations; + static bool error_occurred; static bool extra_warn; @@ -1493,6 +1497,76 @@ static void check_sec_ref(struct module *mod, struct elf_info *elf) } } +/** + * Check alignment of sections in modules. + **/ +static void check_sec_alignment(struct module *mod, struct elf_info *elf) +{ + /* sections that may use PREL32 relocations and only need 4-byte alignment */ + static const char *const prel32_sec_list[] = { + "__tracepoints_ptrs", + "__ksymtab", + "__bug_table", + ".smp_locks", + NULL + }; + /* sections that are fine with any/1-byte alignment */ + static const char *const byte_sec_list[] = { + ".modinfo", + ".init.ramfs", + NULL + }; + /* sections with special alignment */ + static struct { int align; const char *name; } const special_list[] = { + { 64, ".rodata.cst2" }, + { 0, NULL } + }; + + int i; + + /* ignore vmlinux for now? */ + // if (mod->is_vmlinux) return; + + /* Walk through all sections */ + for (i = 0; i < elf->num_sections; i++) { + Elf_Shdr *sechdr = &elf->sechdrs[i]; + const char *sec = sech_name(elf, sechdr); + const char *modname = mod->name; + const int is_shalign = sechdr->sh_addralign; + int should_shalign; + int k; + + /* ignore some sections */ + if ((sechdr->sh_type == SHT_NULL) || + !(sechdr->sh_flags & SHF_ALLOC)) + continue; + + /* default alignment is 8 for 64-bit and 4 for 32-bit targets * */ + should_shalign = target_64bit ? 8 : 4; + if (match(sec, prel32_sec_list)) + should_shalign = target_prel32_relocations ? 4 : should_shalign; + else if (strstr(sec, "text")) /* assume text segments to be 4-byte aligned */ + should_shalign = 4; + else if (match(sec, byte_sec_list)) /* any alignment is OK. */ + continue; + else { + /* search in list with special alignment */ + k = 0; + while (special_list[k].align && strstarts(sec, special_list[k].name)) { + should_shalign = special_list[k].align; + break; + } + } + + if (is_shalign >= should_shalign) + continue; + + warn("%s: section %s (type %d, flags %lu) has alignment of %d, expected at least %d.\n" + "Maybe you need to add ALIGN() to the modules.lds file (or fix modpost) ?\n", + modname, sec, sechdr->sh_type, sechdr->sh_flags, is_shalign, should_shalign); + } +} + static char *remove_dot(char *s) { size_t n = strcspn(s, "."); @@ -1653,6 +1727,8 @@ static void read_symbols(const char *modname) handle_moddevtable(mod, &info, sym, symname); } + check_sec_alignment(mod, &info); + check_sec_ref(mod, &info); if (!mod->is_vmlinux) { @@ -2183,7 +2259,7 @@ int main(int argc, char **argv) LIST_HEAD(dump_lists); struct dump_list *dl, *dl2; - while ((opt = getopt(argc, argv, "ei:MmnT:to:au:WwENd:")) != -1) { + while ((opt = getopt(argc, argv, "ei:MmnT:to:au:WwENd:6R")) != -1) { switch (opt) { case 'e': external_module = true; @@ -2232,6 +2308,12 @@ int main(int argc, char **argv) case 'd': missing_namespace_deps = optarg; break; + case '6': + target_64bit = true; + break; + case 'R': + target_prel32_relocations = true; + break; default: exit(1); } diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index 70c69db6a17c..b09ab081dc03 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -1510,15 +1510,24 @@ static void check_sec_alignment(struct module *mod, struct elf_info *elf) ".smp_locks", NULL }; - /* sections that are fine with any/1-byte alignment */ + /* sections that are fine with any alignment */ static const char *const byte_sec_list[] = { ".modinfo", ".init.ramfs", NULL }; - /* sections with special alignment */ + /* sections with special given minimal alignment. Checked with + * startswith(). */ static struct { int align; const char *name; } const special_list[] = { { 64, ".rodata.cst2" }, + { 2, ".altinstructions" }, /* at least on x86 */ + { 1, ".altinstr_replacement" }, + { 1, ".altinstr_aux" }, + { 4, ".call_sites" }, + { 4, ".return_sites" }, + { 1, ".note.Linux" }, /* correct? */ + { 4, "__ex_table" }, + { 4, ".initcall" }, /* at least 4 ? */ { 0, NULL } }; @@ -1545,16 +1554,17 @@ static void check_sec_alignment(struct module *mod, struct elf_info *elf) should_shalign = target_64bit ? 8 : 4; if (match(sec, prel32_sec_list)) should_shalign = target_prel32_relocations ? 4 : should_shalign; - else if (strstr(sec, "text")) /* assume text segments to be 4-byte aligned */ - should_shalign = 4; else if (match(sec, byte_sec_list)) /* any alignment is OK. */ continue; + else if (strstr(sec, "text")) /* assume text segments to be 4-byte aligned */ + should_shalign = 4; else { /* search in list with special alignment */ - k = 0; - while (special_list[k].align && strstarts(sec, special_list[k].name)) { - should_shalign = special_list[k].align; - break; + for (k = 0; special_list[k].align; k++) { + if (strstarts(sec, special_list[k].name)) { + should_shalign = special_list[k].align; + break; + } } }