vdso: switch from DT_HASH to DT_GNU_HASH (aarch64) #2570
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@avagin any comments from your side. This looks like it should be @0x7f454c46 any comments from your side. The VDSO code seems to be mostly from you. |
|
Hmm, it completely fails on older aarch64 kernels. Because they do not have GNU hash style section. Only the sysv hash style section. So we need to do runtime detection. |
adrianreber
force-pushed
the
2025-01-22-gnu-hash
branch
from
eba2260 to
b5ce14d
Compare
|
Changed the PR to only look for DT_GNU_HASH on aarch64 if DT_HASH is not found. |
adrianreber
force-pushed
the
2025-01-22-gnu-hash
branch
from
b5ce14d to
5aa36b2
Compare
|
I'd say, you also need to differ them later on symbols parsing. Currently CRIU's And with this update, the new hash should be: (both are un-optimized simplistic version from Glibc). Maybe with small amount of vdso symbols it could work by luck, but yet probably look at check/dump logs to verify that kernel-provided symbols can be found after the patch. |
|
Also, thanks for this update! :-) |
adrianreber
force-pushed
the
2025-01-22-gnu-hash
branch
from
5aa36b2 to
d4b1aa5
Compare
|
@0x7f454c46 Thanks for the review and thanks for the hash function. I added that hash function. Does the latest change look correct to you? This is the vdso log output on the aarch64 test system I have: |
|
Heh, it seems my linker by default produces both: So, albeit I like the long comment you've written, but it seems slightly incorrect about only With some debug (probably can be a patch) before: when I ifdef-out So, I spent some time on this... |
The pritings for vdso functions' offsets went into the commit above, so this line was written when there wasn't a patch yet. |
|
Thanks for reworking the PR, that also works on my test system. Let's update this PR with your version. |
adrianreber
force-pushed
the
2025-01-22-gnu-hash
branch
from
d4b1aa5 to
552cb52
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Interesting CI result: |
adrianreber
force-pushed
the
2025-01-22-gnu-hash
branch
from
552cb52 to
1b0a12f
Compare
|
Pushed a version that should no longer have the buffer overflow. |
adrianreber
force-pushed
the
2025-01-22-gnu-hash
branch
from
1b0a12f to
aa129f7
Compare
rst0git
reviewed
Jan 30, 2025
adrianreber
force-pushed
the
2025-01-22-gnu-hash
branch
2 times, most recently
from
351973d to
8783bd0
Compare
Trying to run latest CRIU on CentOS Stream 10 or Ubuntu 24.04 (aarch64)
fails like this:
# criu/criu check -v4
[...]
(00.096460) vdso: Parsing at ffffb2e2a000 ffffb2e2c000
(00.096539) vdso: PT_LOAD p_vaddr: 0
(00.096567) vdso: DT_STRTAB: 1d0
(00.096592) vdso: DT_SYMTAB: 128
(00.096616) vdso: DT_STRSZ: 8a
(00.096640) vdso: DT_SYMENT: 18
(00.096663) Error (criu/pie-util-vdso.c:193): vdso: Not all dynamic entries are present
(00.096688) Error (criu/vdso.c:627): vdso: Failed to fill self vdso symtable
(00.096713) Error (criu/kerndat.c:1906): kerndat_vdso_fill_symtable failed when initializing kerndat.
(00.096812) Found mmap_min_addr 0x10000
(00.096881) files stat: fs/nr_open 1073741816
(00.096908) Error (criu/crtools.c:267): Could not initialize kernel features detection.
This seems to be related to the kernel (6.12.0-41.el10.aarch64). The
Ubuntu user-space is running in a container on the same kernel.
Looking at the kernel this seems to be related to:
commit 48f6430505c0b0498ee9020ce3cf9558b1caaaeb
Author: Fangrui Song <[email protected]>
Date: Thu Jul 18 10:34:23 2024 -0700
arm64/vdso: Remove --hash-style=sysv
glibc added support for .gnu.hash in 2006 and .hash has been obsoleted
for more than one decade in many Linux distributions. Using
--hash-style=sysv might imply unaddressed issues and confuse readers.
Just drop the option and rely on the linker default, which is likely
"both", or "gnu" when the distribution really wants to eliminate sysv
hash overhead.
Similar to commit 6b7e26547fad ("x86/vdso: Emit a GNU hash").
The commit basically does:
-ldflags-y := -shared -soname=linux-vdso.so.1 --hash-style=sysv \
+ldflags-y := -shared -soname=linux-vdso.so.1 \
Which results in only a GNU hash being added to the ELF header. This
change has been merged with 6.11.
Looking at the referenced x86 commit:
commit 6b7e26547fad7ace3dcb27a5babd2317fb9d1e12
Author: Andy Lutomirski <[email protected]>
Date: Thu Aug 6 14:45:45 2015 -0700
x86/vdso: Emit a GNU hash
Some dynamic loaders may be slightly faster if a GNU hash is
available. Strangely, this seems to have no effect at all on
the vdso size.
This is unlikely to have any measurable effect on the time it
takes to resolve vdso symbols (since there are so few of them).
In some contexts, it can be a win for a different reason: if
every DSO has a GNU hash section, then libc can avoid
calculating SysV hashes at all. Both musl and glibc appear to
have this optimization.
It's plausible that this breaks some ancient glibc version. If
so, then, depending on what glibc versions break, we could
either require COMPAT_VDSO for them or consider reverting.
Which is also a really simple change:
-VDSO_LDFLAGS = -fPIC -shared $(call cc-ldoption, -Wl$(comma)--hash-style=sysv) \
+VDSO_LDFLAGS = -fPIC -shared $(call cc-ldoption, -Wl$(comma)--hash-style=both) \
The big difference here is that for x86 both hash sections are
generated. For aarch64 only the newer GNU hash is generated. That is why
we only see this error on kernel >= 6.11 and aarch64.
Changing from DT_HASH to DT_GNU_HASH seems to work on aarch64. The test
suite runs without any errors.
Unfortunately I am not aware of all implication of this change and if a
successful test suite run means that it still works.
Looking at the kernel I see following hash styles for the VDSO:
aarch64: not specified (only GNU hash style)
arm: --hash-style=sysv
loongarch: --hash-style=sysv
mips: --hash-style=sysv
powerpc: --hash-style=both
riscv: --hash-style=both
s390: --hash-style=both
x86: --hash-style=both
Only aarch64 on kernels >= 6.11 is a problem right now, because all
other platforms provide the old style hashing.
Signed-off-by: Adrian Reber <[email protected]>
Co-developed-by: Dmitry Safonov <[email protected]>
Co-authored-by: Dmitry Safonov <[email protected]>
Signed-off-by: Dmitry Safonov <[email protected]>
|
One of the tests is failing with: The compiler cannot figure out, that those variables are not always needed. I think the code is correct. I am still initializing those variables to make the compiler happy. |
adrianreber
force-pushed
the
2025-01-22-gnu-hash
branch
from
8783bd0 to
ed2468f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trying to run latest CRIU on CentOS Stream 10 or Ubuntu 24.04 (aarch64) fails like this:
This seems to be related to the kernel (6.12.0-41.el10.aarch64). The Ubuntu user-space is running in a container on the same kernel.
Looking at the kernel this seems to be related to:
The commit basically does:
-ldflags-y := -shared -soname=linux-vdso.so.1 --hash-style=sysv \ +ldflags-y := -shared -soname=linux-vdso.so.1 \Which results in only a GNU hash being added to the ELF header. This change has been merged with 6.11.
Looking at the referenced x86 commit:
Which is also a really simple change:
-VDSO_LDFLAGS = -fPIC -shared $(call cc-ldoption, -Wl$(comma)--hash-style=sysv) \ +VDSO_LDFLAGS = -fPIC -shared $(call cc-ldoption, -Wl$(comma)--hash-style=both) \The big difference here is that for x86 both hash sections are generated. For aarch64 on the newer GNU hash is generated. That is why we only see this error on kernel >= 6.11 and aarch64.
Changing from DT_HASH to DT_GNU_HASH seems to work on aarch64. The test suite runs without any errors.
Unfortunately I am not aware of all implication of this change and if a successful test suite run means that it still works.
Looking at the kernel I see following hash styles for the VDSO:
aarch64: not specified (only GNU hash style)
arm: --hash-style=sysv
loongarch: --hash-style=sysv
mips: --hash-style=sysv
powerpc: --hash-style=both
riscv: --hash-style=both
s390: --hash-style=both
x86: --hash-style=both