diff mbox series

[v2,2/6] elf: Avoid unnecessary slowdown from profiling with audit (BZ#15533)

Message ID 20210719143309.2848878-3-adhemerval.zanella@linaro.org
State Superseded
Headers
Series Some rtld-audit fixes |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella July 19, 2021, 2:33 p.m. UTC 
  The rtld-audit interfaces introduces 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).  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>
---
 NEWS                 |  4 ++++
 elf/Makefile         |  6 +++++-
 elf/dl-reloc.c       | 12 +++++++++++-
 elf/rtld.c           |  8 +-------
 elf/tst-audit18.c    | 39 +++++++++++++++++++++++++++++++++++++++
 elf/tst-auditmod18.c | 24 ++++++++++++++++++++++++
 include/link.h       |  2 ++
 7 files changed, 86 insertions(+), 9 deletions(-)
 create mode 100644 elf/tst-audit18.c
 create mode 100644 elf/tst-auditmod18.c

Comments

Alexander Monakov July 20, 2021, 8:31 a.m. UTC | #1 
On Mon, 19 Jul 2021, Adhemerval Zanella via Libc-alpha wrote:

> The rtld-audit interfaces introduces a slowdown due to enabling profiling

"interface" (my original message had "using the audit interfaces introduces").

> 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).  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>

Please use my correct email here and in the Cc list (it's ispras.ru, not
ispras.dot.ru).

> ---
>  NEWS                 |  4 ++++
>  elf/Makefile         |  6 +++++-
>  elf/dl-reloc.c       | 12 +++++++++++-
>  elf/rtld.c           |  8 +-------
>  elf/tst-audit18.c    | 39 +++++++++++++++++++++++++++++++++++++++
>  elf/tst-auditmod18.c | 24 ++++++++++++++++++++++++
>  include/link.h       |  2 ++
>  7 files changed, 86 insertions(+), 9 deletions(-)
>  create mode 100644 elf/tst-audit18.c
>  create mode 100644 elf/tst-auditmod18.c
> 
> diff --git a/NEWS b/NEWS
> index 13ffe627da..8fde312ec6 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -76,6 +76,10 @@ Major new features:
>    equal to a giver integer.  This function is a GNU extension, although
>    Solaris also provides a similar function.
>  
> +* The audit libraries will avoid unnecessary slowdown if it is not required

"required for" or "... by"?

> +  either PLT tracking or symbol binding profiling (enabled with LA_FLG_BINDFROM
> +  or LA_FLG_BINDTO from la_objopen() callback).

It seems the part about binding needs to be updated, because the commit message
and the patch itself does not change anything for binding hooks? My
understanding always was that no change is necessary w.r.t la_symbind
callbacks: the loader invokes them when doing symbol resolution, and presence
of PLT does not matter.

Alexander
Adhemerval Zanella July 20, 2021, 6:37 p.m. UTC | #2 
On 20/07/2021 05:31, Alexander Monakov wrote:
> On Mon, 19 Jul 2021, Adhemerval Zanella via Libc-alpha wrote:
> 
>> The rtld-audit interfaces introduces a slowdown due to enabling profiling
> 
> "interface" (my original message had "using the audit interfaces introduces").
> 
>> 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).  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>
> 
> Please use my correct email here and in the Cc list (it's ispras.ru, not
> ispras.dot.ru).

Ack, I used the one on previous submission. I will update with the correct
one

> 
>> ---
>>  NEWS                 |  4 ++++
>>  elf/Makefile         |  6 +++++-
>>  elf/dl-reloc.c       | 12 +++++++++++-
>>  elf/rtld.c           |  8 +-------
>>  elf/tst-audit18.c    | 39 +++++++++++++++++++++++++++++++++++++++
>>  elf/tst-auditmod18.c | 24 ++++++++++++++++++++++++
>>  include/link.h       |  2 ++
>>  7 files changed, 86 insertions(+), 9 deletions(-)
>>  create mode 100644 elf/tst-audit18.c
>>  create mode 100644 elf/tst-auditmod18.c
>>
>> diff --git a/NEWS b/NEWS
>> index 13ffe627da..8fde312ec6 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -76,6 +76,10 @@ Major new features:
>>    equal to a giver integer.  This function is a GNU extension, although
>>    Solaris also provides a similar function.
>>  
>> +* The audit libraries will avoid unnecessary slowdown if it is not required
> 
> "required for" or "... by"?
> 
>> +  either PLT tracking or symbol binding profiling (enabled with LA_FLG_BINDFROM
>> +  or LA_FLG_BINDTO from la_objopen() callback).
> 
> It seems the part about binding needs to be updated, because the commit message
> and the patch itself does not change anything for binding hooks? My
> understanding always was that no change is necessary w.r.t la_symbind
> callbacks: the loader invokes them when doing symbol resolution, and presence
> of PLT does not matter.

The NEWS entry needs update indeed.  For 'la_symbind', I was trying to handle a
different issue which in turn made this very change ineffective (issue la_symbind
for bind-now).  My plan is to address it in a different patch.
Adhemerval Zanella July 27, 2021, 4:12 p.m. UTC | #3 
On 19/07/2021 11:33, Adhemerval Zanella wrote:

> diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c
> index e13a672ade..2abcfc996f 100644
> --- a/elf/dl-reloc.c
> +++ b/elf/dl-reloc.c
> @@ -181,7 +181,17 @@ _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->ARCH_LA_PLTENTER != NULL
> +	      || afct->ARCH_LA_PLTEXIT != NULL)
> +	    consider_profiling = 1;
> +	  afct = afct->next;
> +	}
> +    }
>  #elif defined PROF

This is wrong since it disables la_symbind() for lazy resolution
if the audit modules does not provide the plt callbacks as well.
I will update the patch to now call la_symbind() on _dl_fixup() if
la_symbind() is set.
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 13ffe627da..8fde312ec6 100644
--- a/NEWS
+++ b/NEWS
@@ -76,6 +76,10 @@  Major new features:
   equal to a giver integer.  This function is a GNU extension, although
   Solaris also provides a similar function.
 
+* 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).
+
 Deprecated and removed features, and other changes affecting compatibility:
 
 * The function pthread_mutex_consistent_np has been deprecated; programs
diff --git a/elf/Makefile b/elf/Makefile
index 3216d67bb4..9e2d766c03 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -219,7 +219,7 @@  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-audit18 \
 	 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 \
@@ -301,6 +301,7 @@  modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-unique1mod1 tst-unique1mod2 \
 		tst-unique2mod1 tst-unique2mod2 \
 		tst-auditmod9a tst-auditmod9b \
+		tst-auditmod18 \
 		$(if $(CXX),tst-unique3lib tst-unique3lib2 tst-unique4lib \
 		  tst-nodelete-uniquemod tst-nodelete-rtldmod \
 		  tst-nodelete-zmod \
@@ -1489,6 +1490,9 @@  $(objpfx)tst-auditmod17.so: $(objpfx)tst-auditmod17.os
 CFLAGS-.os += $(call elide-stack-protector,.os,tst-auditmod17)
 tst-audit17-ENV = LD_AUDIT=$(objpfx)tst-auditmod17.so
 
+$(objpfx)tst-audit18.out: $(objpfx)tst-auditmod18.so
+tst-audit18-ENV = LD_AUDIT=$(objpfx)tst-auditmod18.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..2abcfc996f 100644
--- a/elf/dl-reloc.c
+++ b/elf/dl-reloc.c
@@ -181,7 +181,17 @@  _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->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 d733359eaf..374bf86a69 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -1013,13 +1013,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-audit18.c b/elf/tst-audit18.c
new file mode 100644
index 0000000000..36b781f9be
--- /dev/null
+++ b/elf/tst-audit18.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-auditmod18.c b/elf/tst-auditmod18.c
new file mode 100644
index 0000000000..6d1b55223e
--- /dev/null
+++ b/elf/tst-auditmod18.c
@@ -0,0 +1,24 @@ 
+/* 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/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