Message ID | 20210707182610.3940620-3-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | Some rtld-audit fixes | expand |
* Adhemerval Zanella via Libc-alpha: > diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c > index e13a672ade..998cfef099 100644 > --- a/elf/dl-reloc.c > +++ b/elf/dl-reloc.c > @@ -181,7 +181,18 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[], > #ifdef SHARED > /* If we are auditing, install the same handlers we need for profiling. */ > if ((reloc_mode & __RTLD_AUDIT) == 0) > - consider_profiling |= GLRO(dl_audit) != NULL; > + { > + struct audit_ifaces *afct = GLRO(dl_audit); > + for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt) > + { > + /* Profiling is needed only if PLT hooks are provided. */ > + if (afct->symbind != NULL > + || afct->ARCH_LA_PLTENTER != NULL > + || afct->ARCH_LA_PLTEXIT != NULL) > + consider_profiling = 1; > + afct = afct->next; > + } > + } Is the afct->symbind check really necessary? Looking at _dl_fixup, it should be safe to call symbind with just the standard trampoline. I think this needs a NEWS entry, describing how to activate this optimization. Thanks, Florian
On 07/07/2021 16:20, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c >> index e13a672ade..998cfef099 100644 >> --- a/elf/dl-reloc.c >> +++ b/elf/dl-reloc.c >> @@ -181,7 +181,18 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[], >> #ifdef SHARED >> /* If we are auditing, install the same handlers we need for profiling. */ >> if ((reloc_mode & __RTLD_AUDIT) == 0) >> - consider_profiling |= GLRO(dl_audit) != NULL; >> + { >> + struct audit_ifaces *afct = GLRO(dl_audit); >> + for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt) >> + { >> + /* Profiling is needed only if PLT hooks are provided. */ >> + if (afct->symbind != NULL >> + || afct->ARCH_LA_PLTENTER != NULL >> + || afct->ARCH_LA_PLTEXIT != NULL) >> + consider_profiling = 1; >> + afct = afct->next; >> + } >> + } > > Is the afct->symbind check really necessary? Looking at _dl_fixup, it > should be safe to call symbind with just the standard trampoline. Yes, elf/tst-audit18b check specifically for this. Without it, returning LA_FLG_BINDFROM | LA_FLG_BINDTO from la_objopen() won't trigger a la_symbind{32,64}. > > I think this needs a NEWS entry, describing how to activate this > optimization. I can add a NEW entry, although it is not really a 'new feature'. What about: * The audit libraries will avoid unnecessary slowdown if it is not required either PLT tracking or symbol binding profiling (enabled with LA_FLG_BINDFROM or LA_FLG_BINDTO from la_objopen() callback).
* Adhemerval Zanella: > On 07/07/2021 16:20, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c >>> index e13a672ade..998cfef099 100644 >>> --- a/elf/dl-reloc.c >>> +++ b/elf/dl-reloc.c >>> @@ -181,7 +181,18 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[], >>> #ifdef SHARED >>> /* If we are auditing, install the same handlers we need for profiling. */ >>> if ((reloc_mode & __RTLD_AUDIT) == 0) >>> - consider_profiling |= GLRO(dl_audit) != NULL; >>> + { >>> + struct audit_ifaces *afct = GLRO(dl_audit); >>> + for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt) >>> + { >>> + /* Profiling is needed only if PLT hooks are provided. */ >>> + if (afct->symbind != NULL >>> + || afct->ARCH_LA_PLTENTER != NULL >>> + || afct->ARCH_LA_PLTEXIT != NULL) >>> + consider_profiling = 1; >>> + afct = afct->next; >>> + } >>> + } >> >> Is the afct->symbind check really necessary? Looking at _dl_fixup, it >> should be safe to call symbind with just the standard trampoline. > > Yes, elf/tst-audit18b check specifically for this. Without it, > returning LA_FLG_BINDFROM | LA_FLG_BINDTO from la_objopen() won't > trigger a la_symbind{32,64}. Hmm, I'm surprised we get a la_symbind call with BIND_NOW at all. And for BIND_NOW the choice of trampoline really should not matter anyway. Anyway, I think we have a feature request that without la_ltenter/la_pltexit defined, the presence of la_symbind should not incur the overhead from the profiling trampoline. The afct->symbind check defeats that. Thanks, Florian
Forian is correct that what the HPCToolkit team wants is to use la_symbind without la_ltenter/la_pltexit defined. We don’t want the overhead from the profiling trampoline just to get the la_symbind call. -- John Mellor-Crummey Professor Dept of Computer Science Rice University email: johnmc@rice.edu phone: 713-348-5179 > On Jul 7, 2021, at 3:15 PM, Florian Weimer <fweimer@redhat.com> wrote: > > * Adhemerval Zanella: > >> On 07/07/2021 16:20, Florian Weimer wrote: >>> * Adhemerval Zanella via Libc-alpha: >>> >>>> diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c >>>> index e13a672ade..998cfef099 100644 >>>> --- a/elf/dl-reloc.c >>>> +++ b/elf/dl-reloc.c >>>> @@ -181,7 +181,18 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[], >>>> #ifdef SHARED >>>> /* If we are auditing, install the same handlers we need for profiling. */ >>>> if ((reloc_mode & __RTLD_AUDIT) == 0) >>>> - consider_profiling |= GLRO(dl_audit) != NULL; >>>> + { >>>> + struct audit_ifaces *afct = GLRO(dl_audit); >>>> + for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt) >>>> + { >>>> + /* Profiling is needed only if PLT hooks are provided. */ >>>> + if (afct->symbind != NULL >>>> + || afct->ARCH_LA_PLTENTER != NULL >>>> + || afct->ARCH_LA_PLTEXIT != NULL) >>>> + consider_profiling = 1; >>>> + afct = afct->next; >>>> + } >>>> + } >>> >>> Is the afct->symbind check really necessary? Looking at _dl_fixup, it >>> should be safe to call symbind with just the standard trampoline. >> >> Yes, elf/tst-audit18b check specifically for this. Without it, >> returning LA_FLG_BINDFROM | LA_FLG_BINDTO from la_objopen() won't >> trigger a la_symbind{32,64}. > > Hmm, I'm surprised we get a la_symbind call with BIND_NOW at all. And > for BIND_NOW the choice of trampoline really should not matter anyway. > > Anyway, I think we have a feature request that without > la_ltenter/la_pltexit defined, the presence of la_symbind should not > incur the overhead from the profiling trampoline. The afct->symbind > check defeats that. > > Thanks, > Florian >
On 07/07/2021 17:15, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 07/07/2021 16:20, Florian Weimer wrote: >>> * Adhemerval Zanella via Libc-alpha: >>> >>>> diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c >>>> index e13a672ade..998cfef099 100644 >>>> --- a/elf/dl-reloc.c >>>> +++ b/elf/dl-reloc.c >>>> @@ -181,7 +181,18 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[], >>>> #ifdef SHARED >>>> /* If we are auditing, install the same handlers we need for profiling. */ >>>> if ((reloc_mode & __RTLD_AUDIT) == 0) >>>> - consider_profiling |= GLRO(dl_audit) != NULL; >>>> + { >>>> + struct audit_ifaces *afct = GLRO(dl_audit); >>>> + for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt) >>>> + { >>>> + /* Profiling is needed only if PLT hooks are provided. */ >>>> + if (afct->symbind != NULL >>>> + || afct->ARCH_LA_PLTENTER != NULL >>>> + || afct->ARCH_LA_PLTEXIT != NULL) >>>> + consider_profiling = 1; >>>> + afct = afct->next; >>>> + } >>>> + } >>> >>> Is the afct->symbind check really necessary? Looking at _dl_fixup, it >>> should be safe to call symbind with just the standard trampoline. >> >> Yes, elf/tst-audit18b check specifically for this. Without it, >> returning LA_FLG_BINDFROM | LA_FLG_BINDTO from la_objopen() won't >> trigger a la_symbind{32,64}. > > Hmm, I'm surprised we get a la_symbind call with BIND_NOW at all. And > for BIND_NOW the choice of trampoline really should not matter anyway. > > Anyway, I think we have a feature request that without > la_ltenter/la_pltexit defined, the presence of la_symbind should not > incur the overhead from the profiling trampoline. The afct->symbind > check defeats that. Indeed, we will need to track the la_symbind for bind-now in a different fix then. I will update the patch.
diff --git a/elf/Makefile b/elf/Makefile index 5214196de6..a26f5ce320 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -219,7 +219,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \ tst-dlopen-self tst-auditmany tst-initfinilazyfail tst-dlopenfail \ tst-dlopenfail-2 \ tst-filterobj tst-filterobj-dlopen tst-auxobj tst-auxobj-dlopen \ - tst-audit14 tst-audit15 tst-audit16 tst-audit17 \ + tst-audit14 tst-audit15 tst-audit16 tst-audit17 tst-audit18a \ + tst-audit18b \ tst-single_threaded tst-single_threaded-pthread \ tst-tls-ie tst-tls-ie-dlmopen argv0test \ tst-glibc-hwcaps tst-glibc-hwcaps-prepend tst-glibc-hwcaps-mask \ @@ -294,6 +295,7 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \ tst-unique1mod1 tst-unique1mod2 \ tst-unique2mod1 tst-unique2mod2 \ tst-auditmod9a tst-auditmod9b \ + tst-auditmod18a tst-audit18bmod tst-auditmod18b \ $(if $(CXX),tst-unique3lib tst-unique3lib2 tst-unique4lib \ tst-nodelete-uniquemod tst-nodelete-rtldmod \ tst-nodelete-zmod \ @@ -1478,6 +1480,15 @@ $(objpfx)tst-auditmod17.so: $(objpfx)tst-auditmod17.os mv -f $@.new $@ tst-audit17-ENV = LD_AUDIT=$(objpfx)tst-auditmod17.so +$(objpfx)tst-audit18a.out: $(objpfx)tst-auditmod18a.so +tst-audit18a-ENV = LD_AUDIT=$(objpfx)tst-auditmod18a.so +$(objpfx)tst-audit18b.out: $(objpfx)tst-auditmod18b.so \ + $(objpfx)tst-audit18bmod.so +$(objpfx)tst-audit18b: $(objpfx)tst-audit18bmod.so +LDFLAGS-tst-audit18bmod.so = -Wl,-z,now +LDFLAGS-tst-audit18b = -Wl,-z,now +tst-audit18b-ENV = LD_AUDIT=$(objpfx)tst-auditmod18b.so + # tst-sonamemove links against an older implementation of the library. LDFLAGS-tst-sonamemove-linkmod1.so = \ -Wl,--version-script=tst-sonamemove-linkmod1.map \ diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c index e13a672ade..998cfef099 100644 --- a/elf/dl-reloc.c +++ b/elf/dl-reloc.c @@ -181,7 +181,18 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[], #ifdef SHARED /* If we are auditing, install the same handlers we need for profiling. */ if ((reloc_mode & __RTLD_AUDIT) == 0) - consider_profiling |= GLRO(dl_audit) != NULL; + { + struct audit_ifaces *afct = GLRO(dl_audit); + for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt) + { + /* Profiling is needed only if PLT hooks are provided. */ + if (afct->symbind != NULL + || afct->ARCH_LA_PLTENTER != NULL + || afct->ARCH_LA_PLTEXIT != NULL) + consider_profiling = 1; + afct = afct->next; + } + } #elif defined PROF /* Never use dynamic linker profiling for gprof profiling code. */ # define consider_profiling 0 diff --git a/elf/rtld.c b/elf/rtld.c index fbbd60b446..bf9eb5a33e 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -1014,13 +1014,7 @@ ERROR: audit interface '%s' requires version %d (maximum supported version %d); "la_objsearch\0" "la_objopen\0" "la_preinit\0" -#if __ELF_NATIVE_CLASS == 32 - "la_symbind32\0" -#elif __ELF_NATIVE_CLASS == 64 - "la_symbind64\0" -#else -# error "__ELF_NATIVE_CLASS must be defined" -#endif + LA_SYMBIND "\0" #define STRING(s) __STRING (s) "la_" STRING (ARCH_LA_PLTENTER) "\0" "la_" STRING (ARCH_LA_PLTEXIT) "\0" diff --git a/elf/tst-audit18a.c b/elf/tst-audit18a.c new file mode 100644 index 0000000000..36b781f9be --- /dev/null +++ b/elf/tst-audit18a.c @@ -0,0 +1,39 @@ +/* Check if DT_AUDIT a module without la_plt{enter,exit} symbols does not incur + in profiling (BZ#15533). + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <link.h> +#include <stdio.h> + +/* We interpose the profile resolver and if it is called it means profiling is + enabled. */ +void +_dl_runtime_profile (ElfW(Word) addr) +{ + volatile int *p = NULL; + *p = 0; +} + +static int +do_test (void) +{ + printf ("..."); + return 0; +} + +#include <support/test-driver.c> diff --git a/elf/tst-audit18b.c b/elf/tst-audit18b.c new file mode 100644 index 0000000000..21381fe950 --- /dev/null +++ b/elf/tst-audit18b.c @@ -0,0 +1,30 @@ +/* Check if DT_AUDIT a module built with bind-now does trigger la_symbind. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +extern int foo (void); + +int +do_test (void) +{ + foo (); + /* If audit la_symbind() callback is called it will exit the test with + EXIT_SUCCESS. */ + return 1; +} + +#include <support/test-driver.c> diff --git a/elf/tst-audit18bmod.c b/elf/tst-audit18bmod.c new file mode 100644 index 0000000000..91f171b5bc --- /dev/null +++ b/elf/tst-audit18bmod.c @@ -0,0 +1,22 @@ +/* Check if DT_AUDIT a module built with bind-now does trigger la_symbind. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +int foo (void) +{ + return 0; +} diff --git a/elf/tst-auditmod18a.c b/elf/tst-auditmod18a.c new file mode 100644 index 0000000000..333db0077c --- /dev/null +++ b/elf/tst-auditmod18a.c @@ -0,0 +1,25 @@ +/* Check if DT_ADIT a module without la_plt{enter,exit} symbols does not incur + in profiling (BZ#15533). + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +unsigned int +la_version (unsigned int version) +{ + return version; +} + diff --git a/elf/tst-auditmod18b.c b/elf/tst-auditmod18b.c new file mode 100644 index 0000000000..40ebcd5be2 --- /dev/null +++ b/elf/tst-auditmod18b.c @@ -0,0 +1,48 @@ +/* Check if DT_AUDIT a module built with bind-now does trigger la_symbind. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <link.h> +#include <stdlib.h> +#include <string.h> + +unsigned int +la_version (unsigned int version) +{ + return version; +} + +unsigned int +la_objopen (struct link_map* map, Lmid_t lmid, uintptr_t* cookie) +{ + return LA_FLG_BINDFROM | LA_FLG_BINDTO; +} + +uintptr_t +#if __ELF_NATIVE_CLASS == 32 +la_symbind32 (Elf32_Sym *sym, unsigned int ndx, uintptr_t *refcook, + uintptr_t *defcook, unsigned int *flags, const char *symname) +#else +la_symbind64 (Elf64_Sym *sym, unsigned int ndx, uintptr_t *refcook, + uintptr_t *defcook, unsigned int *flags, const char *symname) +#endif +{ + /* Filter out glibc own symbols libc.so is not built with -z,now. */ + if (strcmp (symname, "foo") == 0) + exit (EXIT_SUCCESS); + return sym->st_value; +} diff --git a/include/link.h b/include/link.h index 4af16cb596..ebd0f511e2 100644 --- a/include/link.h +++ b/include/link.h @@ -355,8 +355,10 @@ struct auditstate #if __ELF_NATIVE_CLASS == 32 # define symbind symbind32 +# define LA_SYMBIND "la_symbind32" #elif __ELF_NATIVE_CLASS == 64 # define symbind symbind64 +# define LA_SYMBIND "la_symbind64" #else # error "__ELF_NATIVE_CLASS must be defined" #endif