[2/5] elf: Avoid unnecessary slowdown from profiling with audit (BZ#15533)
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
The rtld-audit interfaces introduce a slowdown due to enabling profiling
instrumentation (as if LD_AUDIT implied LD_PROFILE). However, instrumenting
is only necessary if one of audit libraries provides PLT (la_plt{enter,exit}
symbols) or symbol bynding (la_symbind{32,64})hooks. Otherwise, the
slowdown can be avoided.
The following patch adjusts the logic that enables profiling to iterate
over all audit modules and check if any of those provides a PLT hook.
Co-authored-by: Alexander Monakov <amonakov@ispras.dot.ru>
---
elf/Makefile | 13 +++++++++++-
elf/dl-reloc.c | 13 +++++++++++-
elf/rtld.c | 8 +-------
elf/tst-audit18a.c | 39 +++++++++++++++++++++++++++++++++++
elf/tst-audit18b.c | 30 +++++++++++++++++++++++++++
elf/tst-audit18bmod.c | 22 ++++++++++++++++++++
elf/tst-auditmod18a.c | 25 ++++++++++++++++++++++
elf/tst-auditmod18b.c | 48 +++++++++++++++++++++++++++++++++++++++++++
include/link.h | 2 ++
9 files changed, 191 insertions(+), 9 deletions(-)
create mode 100644 elf/tst-audit18a.c
create mode 100644 elf/tst-audit18b.c
create mode 100644 elf/tst-audit18bmod.c
create mode 100644 elf/tst-auditmod18a.c
create mode 100644 elf/tst-auditmod18b.c
Comments
* 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.
@@ -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 \
@@ -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
@@ -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"
new file mode 100644
@@ -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>
new file mode 100644
@@ -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>
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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;
+}
+
new file mode 100644
@@ -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;
+}
@@ -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