Support build-id generation from compressed ELF files #653
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
…ation File extension based heuristics only work so far at best, and break completely on compressed files with arbitrary .gz/.xz etc extension. This isn't supposed to change any behavior as such, only provide more reliable detection of kernel modules.
…175) Use dwelf_elf_begin() for reading ELF files for build-id generation on versions that have it to support compressed ELF files such as kernel modules (RhBug:1650072,1650074). Note that debugedit still cannot handle compressed files, this is only for build-id generation.
| @@ -487,6 +487,10 @@ AS_IF([test "$WITH_LIBELF" = yes],[ | |||
| # If possible we also want the strtab functions from elfutils 0.167. | |||
| # But we can fall back on the (unsupported) ebl alternatives if not. | |||
| AC_CHECK_LIB(dw, dwelf_strtab_init, [HAVE_LIBDW_STRTAB=yes]) | |||
| # whether libdw supports compressed ELF objects | |||
| AC_CHECK_LIB(dw, dwelf_elf_begin, [ | |||
| AC_DEFINE(HAVE_DWELF_ELF_BEGIN, 1, [Have dwelf_elf_begin?]) | |||
There was a problem hiding this comment.
should we also change previous condition to use AC_DEFINE? just to be consistent. Because it uses AM_CONDITIONAL few lines below.
There was a problem hiding this comment.
Nah, it ain't broken and it ain't mine, I'm not touching it. Not in autocrap land ;)
|
This should probably be reviewed by Mark Wielaard or some kernel person. This is all gibberish to me. Feel free to merge if you got a positive response. |
|
On Tue, 2019-04-09 at 09:31 +0000, Florian Festi wrote:
This should probably be reviewed by Mark Wielaard or some kernel
person. This is all gibberish to me.
It isn't that bad is it?
TLDR; Looks good to me.
+static int haveModinfo(Elf *elf)
+{
+ Elf_Scn * scn = NULL;
+ size_t shstrndx;
+ int have_modinfo = 0;
+ const char *sname;
+
+ if (elf_getshdrstrndx(elf, &shstrndx) == 0) {
+ while ((scn = elf_nextscn(elf, scn)) != NULL) {
+ GElf_Shdr shdr_mem, *shdr = gelf_getshdr(scn, &shdr_mem);
+ if (shdr == NULL)
+ continue;
+ sname = elf_strptr(elf, shstrndx, shdr->sh_name);
+ if (sname && rstreq(sname, ".modinfo")) {
+ have_modinfo = 1;
+ break;
+ }
+ }
+ }
+ return have_modinfo;
+}
Looks like a reasonable way to check the file has a .modinfo section.
You could add some more sanity checks, but it is unlikely that anything
else has a .modinfo section anyway. And you do another check below.
@@ -1803,15 +1825,14 @@ static int generateBuildIDs(FileList fl, ARGV_t *files)
int fd = open (flp->diskPath, O_RDONLY);
if (fd >= 0) {
/* Only real ELF files, that are ET_EXEC, ET_DYN or
- kernel modules (ET_REL files with names ending in .ko)
+ kernel modules (ET_REL files with .modinfo section)
should have build-ids. */
GElf_Ehdr ehdr;
Elf *elf = elf_begin (fd, ELF_C_READ, NULL);
if (elf != NULL && elf_kind(elf) == ELF_K_ELF
&& gelf_getehdr(elf, &ehdr) != NULL
&& (ehdr.e_type == ET_EXEC || ehdr.e_type == ET_DYN
- || (ehdr.e_type == ET_REL
- && rpmFileHasSuffix (flp->diskPath, ".ko")))) {
+ || (ehdr.e_type == ET_REL && haveModinfo(elf)))) {
Yes, definitely a better check that the "file ends with .ko" one.
+#if HAVE_DWELF_ELF_BEGIN
+ Elf *elf = dwelf_elf_begin(fd);
+#else
Elf *elf = elf_begin (fd, ELF_C_READ, NULL);
+#endif
That is indeed how dwelf_elf_begin was intended to be used. As a drop
in replacement for elf_begin (fd, ELF_C_READ, NULL) that "magically"
handles compressed ELF files.
+ # whether libdw supports compressed ELF objects
+ AC_CHECK_LIB(dw, dwelf_elf_begin, [
+ AC_DEFINE(HAVE_DWELF_ELF_BEGIN, 1, [Have dwelf_elf_begin?])
Looks like the correct check.
|
|
On Tue, 2019-04-02 at 06:32 -0700, Igor Gnatenko wrote:
> @@ -487,6 +487,10 @@ AS_IF([test "$WITH_LIBELF" = yes],[
# If possible we also want the strtab functions from elfutils
0.167.
# But we can fall back on the (unsupported) ebl alternatives
if not.
AC_CHECK_LIB(dw, dwelf_strtab_init, [HAVE_LIBDW_STRTAB=yes])
+ # whether libdw supports compressed ELF objects
+ AC_CHECK_LIB(dw, dwelf_elf_begin, [
+ AC_DEFINE(HAVE_DWELF_ELF_BEGIN, 1, [Have
dwelf_elf_begin?])
should we also change previous condition to use AC_DEFINE? just to be
consistent. Because it uses AM_CONDITIONAL few lines below.
No. The AC_DEFINE is for use is C source code and defines a macro.
An AM_CONDITIONAL defines a conditional for use in an automake file.
The automake conditionals are used to decide how to link, the defines
are used to determine how/which functions to call in the code.
Cheers,
Mark
|
Elf internals are a bit daunting for the uninitiated such as ourselves 😁 Anyway, thanks for the review Mark. |
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.
The prime use-case is build-id generation for compressed kernel modules, see https://bugzilla.redhat.com/show_bug.cgi?id=1650072 for background.
The first commit is independent in that it simply improves the detection of kernel modules, the second part actually enables compressed ELF via use of dwelf_elf_begin().
Caveat: I haven't actually tested it with a kernel module due to not having a sane reproducer (rebuilding the entire kernel just for this is not an appetizing thought).