Linux: changed section flags for .note.Linux in arch/x86/entry/vdso/vdso-note.S

[tpimh]

arch/x86/entry/vdso/vdso-note.S:15:1: error: changed section flags for .note.Linux, expected: 0x2
.pushsection .note.Linux, "",@note ; .balign 4 ; .long 2f - 1f ; .long 4484f - 3f ; .long 0x100 ; 1:.asciz "Linux" ; 2:.balign 4 ; 3: .asciz "" ; 4484:.balign 4 ; .popsection ;
^

[nickdesaulniers]

That's not a great error message, llvm should print the flag rather than the hex constant. The "" likely needs a letter or two in there for the section flags.

[ihalip]

It seems that GNU as allows a second .pushsection with empty flags. Reproducer:

$ cat vdso-note.s
.pushsection .note.Linux, "a",@note ;
 .long 0x1234 ;
.popsection ;

.pushsection .note.Linux, "",@note ;
 .long 0x5678 ;
.popsection ;

$ as -o vdso-note.o vdso-note.s && objdump -D vdso-note.o
[...]
0000000000000000 <.note.Linux>:
   0:   34 12                   xor    $0x12,%al
   2:   00 00                   add    %al,(%rax)
   4:   78 56                   js     0x5c
    ...

$ clang-11 -cc1as -triple x86_64-pc-linux-gnu -o vdso-note.o vdso-note.s
vdso-note.s:5:1: error: changed section flags for .note.Linux, expected: 0x2
.pushsection .note.Linux, "",@note ;

Looks like the fix should be easy enough on the LLVM side - just reuse the flags of the existing section. I'll try to fix this and send a review.

[MaskRay]

See https://lists.llvm.org/pipermail/llvm-dev/2020-February/139390.html

https://reviews.llvm.org/D73999 added the error. I agree that the error message is not great but there is some technical difficulty there. I have noted we should improve it.

.section .foo,"ax",@progbits; .byte 1
.section .foo,"",@progbits; .byte 2
.section .foo,"a",@progbits; .byte 3

So GNU as does not error for the empty flags case.

a.s:3: Error: changed section attributes for .foo

I asked https://sourceware.org/ml/binutils/2020-03/msg00053.html whether GNU as wants to emit an error.

I lean toward an error for consistency. Yes, we should fix the kernel assembly.

[nickdesaulniers]

I agree that we should error; we can likely fix this in the Linux kernel source code.

[ihalip]

The ELFNOTE macros are used with the "a" flags throughout the kernel. But I don't think changing the macro to make this hardcoded or implicit is a good idea.
On the other hand, the BUILD_SALT macro is there to add a piece of data to the binary (to tie a vDSO build to the kernel build) and doesn't seem needed at runtime, so changing it to use the same flag also doesn't sit well with me.

What about moving the build salt into it's own section? Such a change would look like this:

diff --git a/include/linux/build-salt.h b/include/linux/build-salt.h
index bb007bd05e7a..95292c7b7e91 100644
--- a/include/linux/build-salt.h
+++ b/include/linux/build-salt.h
@@ -8,12 +8,12 @@
 #ifdef __ASSEMBLER__

 #define BUILD_SALT \
-       ELFNOTE(Linux, LINUX_ELFNOTE_BUILD_SALT, .asciz CONFIG_BUILD_SALT)
+       ELFNOTE(buildsalt, LINUX_ELFNOTE_BUILD_SALT, .asciz CONFIG_BUILD_SALT)

 #else

 #define BUILD_SALT \
-       ELFNOTE32("Linux", LINUX_ELFNOTE_BUILD_SALT, CONFIG_BUILD_SALT)
+       ELFNOTE32("buildsalt", LINUX_ELFNOTE_BUILD_SALT, CONFIG_BUILD_SALT)

 #endif

cc @masahir0y

[nickdesaulniers]

If I preprocess the assembly, it looks like:

.pushsection .note.Linux, "a",@note ; .balign 4 ; .long 2f - 1f ; .long 4484f - 3f ; .long 0 ; 1:.asciz "Linux" ; 2:.balign 4 ; 3:
.long 329216
4484:.balign 4 ; .popsection ;
.pushsection .note.Linux, "",@note ; .balign 4 ; .long 2f - 1f ; .long 4484f - 3f ; .long 0x100 ; 1:.asciz "Linux" ; 2:.balign 4 ; 3: .asciz "" ; 4484:.balign 4 ; .popsection ;

So we have:

.pushsection .note.Linux, "a",@note ;
.pushsection .note.Linux, "",@note ;

[nickdesaulniers]

Ah, so it looks like all instances of ELFNOTE_START in the kernel set "a", except for ELFNOTE. I think this would be the right fix:

diff --git a/include/linux/elfnote.h b/include/linux/elfnote.h
index f236f5b931b2..7fdd7f355b52 100644
--- a/include/linux/elfnote.h
+++ b/include/linux/elfnote.h
@@ -54,7 +54,7 @@
 .popsection                            ;

 #define ELFNOTE(name, type, desc)              \
-       ELFNOTE_START(name, type, "")           \
+       ELFNOTE_START(name, type, "a")          \
                desc                    ;       \
        ELFNOTE_END

[nickdesaulniers]

(or better yet, I'd refactor the flag to be implicitly "a", and update all uses so they were no longer able to pass potentially different flags).

[ihalip]

(or better yet, I'd refactor the flag to be implicitly "a", and update all uses so they were no longer able to pass potentially different flags).

Agreed. It actually doesn't seem to matter what flags are set for .note.* sections, since the linker merges them all into a PT_NOTE section which is marked as SHF_ALLOC only. Let me spin up a patch.

[masahir0y]

On the other hand, the BUILD_SALT macro is there to add a piece of data to the binary (to tie a vDSO build to the kernel build) and doesn't seem needed at runtime, so changing it to use the same flag also doesn't sit well with me.

Right, we do not need BUILD_SALT at runtime, but
linker scripts puts (.note.*) into the same section.
So, ".note.buildsalt" would go to the same section after all.

The buildsalt is only a few bytes data, so this is not a big deal.

(or better yet, I'd refactor the flag to be implicitly "a", and update all uses so they were no longer able to pass potentially different flags).

I agree too.

[ihalip]

This turned into a series because I found another opportunity to do a minor change. See the last 2 commits: https://github.com/ihalip/linux/commits/elfnote
If you think these are OK, I can send them to LKML.

[nickdesaulniers]

First one LGTM, left comments on the second. Thank you for the fixes!

[masahir0y]

This turned into a series because I found another opportunity to do a minor change. See the last 2 commits: https://github.com/ihalip/linux/commits/elfnote
If you think these are OK, I can send them to LKML.

LGTM.
If there is no objection, please send them to ML.
Thanks.

[nickdesaulniers]

I sent https://lore.kernel.org/lkml/[email protected]/T/#u to the list. I'd like to rebase @ihalip changes and get those submitted, too, though treewide changes sometimes take longer to land.

[nathanchance]

Andrew Morton picked this up: https://marc.info/?l=linux-mm-commits&m=158724137902001&w=2

Should be here eventually: http://ozlabs.org/~akpm/mmotm/broken-out/elfnote-mark-all-note-sections-shf_alloc.patch

[MaskRay]

Andrew Morton picked this up: https://marc.info/?l=linux-mm-commits&m=158724137902001&w=2

Should be here eventually: http://ozlabs.org/~akpm/mmotm/broken-out/elfnote-mark-all-note-sections-shf_alloc.patch

Yeah.. I also received an email notification about this
http://ozlabs.org/~akpm/mmotm/broken-out/elfnote-mark-all-note-sections-shf_alloc.patch is currently 404...

Just out of curiosity, why isn't the patch in an x86 tree?

./scripts/get_maintainer.pl arch/x86/entry/vdso/vdso-note.S => [email protected] (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT))

[nathanchance]

Andrew Morton picked this up: https://marc.info/?l=linux-mm-commits&m=158724137902001&w=2
Should be here eventually: http://ozlabs.org/~akpm/mmotm/broken-out/elfnote-mark-all-note-sections-shf_alloc.patch

Yeah.. I also received an email notification about this
http://ozlabs.org/~akpm/mmotm/broken-out/elfnote-mark-all-note-sections-shf_alloc.patch is currently 404...

mmotm (-mm of the moment) is uploaded at random so it might take a bit to show up there. Andrew Morton does not use git, he uses quilt so getting the patches out is not as simple as typing git push.

Just out of curiosity, why isn't the patch in an x86 tree?

./scripts/get_maintainer.pl arch/x86/entry/vdso/vdso-note.S => [email protected] (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT))

The actual file modified is include/linux/elfnote.h which does not appear to have a formal maintainer:

$ ./scripts/get_maintainer.pl include/linux/elfnote.h
Vincenzo Frascino <[email protected]> (commit_signer:1/1=100%,authored:1/1=100%,added_lines:1/1=100%,removed_lines:1/1=100%)
Thomas Gleixner <[email protected]> (commit_signer:1/1=100%)
[email protected] (open list)

As a result, Andrew picks it up, since he's the catch all for files that do not have an entry in MAINTAINERS. Happens fairly often with header files.

[MaskRay]

Is the include/linux/elfnote.h change still not picked? :)

[nathanchance]

It is in -next, won’t be in mainline until the next merge window:

https://git.kernel.org/next/linux-next/c/0a1c42727be5d91b13699363701164c4050edc75

[nathanchance]

https://git.kernel.org/linus/51da9dfb7f20911ae4e79e9b412a9c2d4c373d4b