Re: [PATCH 0/4] Section alignment issues?

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/21/23 16:42, Masahiro Yamada wrote:
On Thu, Dec 21, 2023 at 10:40 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:

On Thu, Nov 23, 2023 at 7:18 AM <deller@xxxxxxxxxx> wrote:

From: Helge Deller <deller@xxxxxx>

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





[Index of Archives]     [Linux Kernel]     [Kernel Newbies]     [x86 Platform Driver]     [Netdev]     [Linux Wireless]     [Netfilter]     [Bugtraq]     [Linux Filesystems]     [Yosemite Discussion]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]

  Powered by Linux