| Message ID | 20220304113821.2366328-1-kkourt@kkourt.io (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | pahole: avoid segfault when parsing a problematic file | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-PR | fail | merge-conflict |
| netdev/tree_selection | success | Not a local patch |
Em Fri, Mar 04, 2022 at 12:38:21PM +0100, kkourt@kkourt.io escreveu: > From: Kornilios Kourtis <kornilios@isovalent.com> > > When trying to use btf encoding for an apparently problematic kernel file, > pahole segfaults. As can be seen below [1], the problem is that we are trying to > dereference a NULL decoder. > > Fix this by checking the return value of dwfl_getmodules which [2] whill return > -1 on errors or an offset if one of the modules did not return DWARF_CB_OK. (In > this specific case, it was __cus__load_debug_types that returnd > DWARF_CB_ABORT.) > > Also, ensure that we get a reasonable error by setting errno in > cus__load_files(). Otherwise, we get a "No such file or directory" error which > might be confusing. Can you break this into two patches, one for checking dwfl_getmodules() failure and the second setting errno? We should try to avoid these patches that do multiple fixes, as sometimes one of the fixes isn't really correct and we end up not being able to use 'git revert' which should be the case when we figure out that some previous fix wasn't correct. Thanks for working on this, lemme know if you're busy in which case I can do this myself. Best regards, - Arnaldo > After tha patch: > $ ./pahole -J vmlinux-5.3.18-24.102-default.debug > pahole: vmlinux-5.3.18-24.102-default.debug: Unknown error -22 > > [1]: > $ gdb -q --args ./pahole -J vmlinux-5.3.18-24.102-default.debug > Reading symbols from ./pahole... > (gdb) r > Starting program: /tmp/pahole/build/pahole -J vmlinux-5.3.18-24.102-default.debug > [Thread debugging using libthread_db enabled] > Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". > > Program received signal SIGSEGV, Segmentation fault. > 0x00007ffff7f4000e in gobuffer__size (gb=0x18) at /tmp/pahole/gobuffer.h:39 > 39 return gb->index; > (gdb) bt > (gdb) frame 1 > 1042 if (gobuffer__size(&encoder->percpu_secinfo) != 0) > (gdb) list > 1037 > 1038 int btf_encoder__encode(struct btf_encoder *encoder) > 1039 { > 1040 int err; > 1041 > 1042 if (gobuffer__size(&encoder->percpu_secinfo) != 0) > 1043 btf_encoder__add_datasec(encoder, PERCPU_SECTION); > 1044 > 1045 /* Empty file, nothing to do, so... done! */ > 1046 if (btf__get_nr_types(encoder->btf) == 0) > (gdb) print encoder > $1 = (struct btf_encoder *) 0x0 > > [2] https://sourceware.org/git/?p=elfutils.git;a=blob;f=libdwfl/libdwfl.h;h=f98f1d525d94bc7bcfc7c816890de5907ee4bd6d;hb=HEAD#l200 > > Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com> > --- > dwarf_loader.c | 5 ++++- > dwarves.c | 5 ++++- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/dwarf_loader.c b/dwarf_loader.c > index e30b03c..fecf711 100644 > --- a/dwarf_loader.c > +++ b/dwarf_loader.c > @@ -3235,7 +3235,10 @@ static int cus__process_file(struct cus *cus, struct conf_load *conf, int fd, > }; > > /* Process the one or more modules gleaned from this file. */ > - dwfl_getmodules(dwfl, cus__process_dwflmod, &parms, 0); > + int err = dwfl_getmodules(dwfl, cus__process_dwflmod, &parms, 0); > + if (err) { > + return -1; > + } > > // We can't call dwfl_end(dwfl) here, as we keep pointers to strings > // allocated by libdw that will be freed at dwfl_end(), so leave this for > diff --git a/dwarves.c b/dwarves.c > index 81fa47b..c5935ec 100644 > --- a/dwarves.c > +++ b/dwarves.c > @@ -2391,8 +2391,11 @@ int cus__load_files(struct cus *cus, struct conf_load *conf, > int i = 0; > > while (filenames[i] != NULL) { > - if (cus__load_file(cus, conf, filenames[i])) > + int err = cus__load_file(cus, conf, filenames[i]); > + if (err) { > + errno = err; > return -++i; > + } > ++i; > } > > -- > 2.25.1
Hi Arnaldo, Apologies for the delayed reply. On Sat, Mar 05, 2022 at 03:49:55PM -0300, Arnaldo Carvalho de Melo wrote: > Can you break this into two patches, one for checking dwfl_getmodules() > failure and the second setting errno? > > We should try to avoid these patches that do multiple fixes, as > sometimes one of the fixes isn't really correct and we end up not being > able to use 'git revert' which should be the case when we figure out > that some previous fix wasn't correct. Yap, makes perfect sense. I will reply to this email with the two patches momentarily.
diff --git a/dwarf_loader.c b/dwarf_loader.c index e30b03c..fecf711 100644 --- a/dwarf_loader.c +++ b/dwarf_loader.c @@ -3235,7 +3235,10 @@ static int cus__process_file(struct cus *cus, struct conf_load *conf, int fd, }; /* Process the one or more modules gleaned from this file. */ - dwfl_getmodules(dwfl, cus__process_dwflmod, &parms, 0); + int err = dwfl_getmodules(dwfl, cus__process_dwflmod, &parms, 0); + if (err) { + return -1; + } // We can't call dwfl_end(dwfl) here, as we keep pointers to strings // allocated by libdw that will be freed at dwfl_end(), so leave this for diff --git a/dwarves.c b/dwarves.c index 81fa47b..c5935ec 100644 --- a/dwarves.c +++ b/dwarves.c @@ -2391,8 +2391,11 @@ int cus__load_files(struct cus *cus, struct conf_load *conf, int i = 0; while (filenames[i] != NULL) { - if (cus__load_file(cus, conf, filenames[i])) + int err = cus__load_file(cus, conf, filenames[i]); + if (err) { + errno = err; return -++i; + } ++i; }