Re: [PATCH v6 10/20] elf: Avoid unnecessary slowdown from profiling with audit (BZ#15533)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 18/12/2021 15:45, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> 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 callbacks (
>> la_pltenter or la_pltexit symbols).  Otherwise, the slowdown can be avoided.
> 
> Awkward linebreak after (.

Ack.

> 
>> To keep la_symbind() to work even without PLT callbacks, _dl_fixup now
> 
>> +* The audit libraries will avoid unnecessary slowdown if it is not required
>> +  PLT tracking (by not implementing the la_pltenter() or la_pltexit()
>> +  callbacks).
> 
> I think we don't use the () markers?  (Although I said before I wouldn't
> point this out anymore.)

I though I have removed all of them, I will fix this as well.

> 
>> diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c
>> index 0d5b727c64..64a96c36e8 100644
>> --- a/elf/dl-reloc.c
>> +++ b/elf/dl-reloc.c
>> @@ -205,12 +205,28 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>>    int skip_ifunc = reloc_mode & __RTLD_NOIFUNC;
> 
>>  #ifndef PROF
>> -    if (__glibc_unlikely (consider_profiling)
>> +    if (consider_profiling | consider_symbind
>>  	&& l->l_info[DT_PLTRELSZ] != NULL)
>>        {
>>  	/* Allocate the array which will contain the already found
> 
> Please add parentheses around the | expression (which should perhaps
> use || too).
> 

Ack.

>> diff --git a/elf/dl-runtime.c b/elf/dl-runtime.c
>> index 03da689503..9a38eea7cc 100644
>> --- a/elf/dl-runtime.c
>> +++ b/elf/dl-runtime.c
>> @@ -131,6 +131,37 @@ _dl_fixup (
> 
>> -	  /* Determine whether any of the two participating DSOs is
>> -	     interested in auditing.  */
>> -	  if ((l->l_audit_any_plt | result->l_audit_any_plt) != 0)
>> -	    {
>> -	      unsigned int flags = 0;
>> -	      struct audit_ifaces *afct = GLRO(dl_audit);
>> -	      /* Synthesize a symbol record where the st_value field is
>> -		 the result.  */
>> -	      ElfW(Sym) sym = *defsym;
>> -	      sym.st_value = DL_FIXUP_VALUE_ADDR (value);
> 
> All this was copied over in patch 06/20 of the series to dl-audit.c.

Ack, I have moved this to the 06/20 patch.

> 
>> diff --git a/elf/tst-audit19a.c b/elf/tst-audit19a.c
>> new file mode 100644
>> index 0000000000..36b781f9be
>> --- /dev/null
>> +++ b/elf/tst-audit19a.c
> 
>> +#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)
> 
> I don't think this works.  _dl_runtime_profile is not an exported
> symbol, so it can't be interposed.
> 
> Maybe you can check for an allocated l_reloc_result instead.  If it's
> not there, profiling isn't possible.  It's not entirely equivalent, but
> it provides at least some testing.

Indeed, I am not sure why I haven't see it that removing the segfault
does not make the test fail.  I have added a l_reloc_result check
instead.

> 
>> diff --git a/elf/tst-auditmod19a.c b/elf/tst-auditmod19a.c
>> new file mode 100644
>> index 0000000000..2296382a1c
>> --- /dev/null
>> +++ b/elf/tst-auditmod19a.c
>> @@ -0,0 +1,23 @@
> 
>> +unsigned int
>> +la_version (unsigned int version)
>> +{
>> +  return version;
>> +}
> 
> This should use LAV_CURRENT.
> 

Ack.

> 
>> diff --git a/elf/tst-auditmod19b.c b/elf/tst-auditmod19b.c
>> new file mode 100644
>> index 0000000000..52bb88c7d7
>> --- /dev/null
>> +++ b/elf/tst-auditmod19b.c
> 
>> +unsigned int
>> +la_version (unsigned int version)
>> +{
>> +  return version;
>> +}
> 
> Likweise.

Ack.

> 
> Thanks,
> Florian
>