Re: [PATCH v6 18/20] elf: Issue la_symbind() for bind-now (BZ #23734)

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

On 20/12/2021 16:18, Florian Weimer wrote:
> I believe this change needs to update elf/sotruss-lib.c:
> 
> uintptr_t
> la_symbind (Elf_Sym *sym, unsigned int ndx, uintptr_t *refcook,
> 	    uintptr_t *defcook, unsigned int *flags, const char *symname)
> {
>   if (!do_exit)
>     *flags = LA_SYMB_NOPLTEXIT;
> 
>   return sym->st_value;
> }



> 
> * Adhemerval Zanella:
> 
>> diff --git a/elf/Makefile b/elf/Makefile
>> index ee4daae0fc..acce163078 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
> 
>> +# The test check if a library without .gnu.version correctly calls the
>> +# audit callbacks.  So it uses an explicit link rule to avoid linking
>> +# against libc.so.
>> +$(objpfx)tst-audit24bmod1.so: $(objpfx)tst-audit24bmod1.os
>> +	$(CC) -nostdlib -nostartfiles -shared -o $@.new $(objpfx)tst-audit24bmod1.os \
>> +	  -Wl,-z,now
>> +	$(call after-link,$@.new)
>> +	mv -f $@.new $@
> 
> I think you may have to update modules-names-nobuild as well.

Ack.

> 
>>  # 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-audit.c b/elf/dl-audit.c
>> index c3569cb357..f080591596 100644
>> --- a/elf/dl-audit.c
>> +++ b/elf/dl-audit.c
>> @@ -190,7 +190,7 @@ rtld_hidden_def (_dl_audit_symbind_alt)
>>  void
>>  _dl_audit_symbind (struct link_map *l, struct reloc_result *reloc_result,
>>  		   const ElfW(Sym) *defsym, DL_FIXUP_VALUE_TYPE *value,
>> -		   lookup_t result)
>> +		   lookup_t result, bool bindnow)
>>  {
> 
> Should “bindow“ be called “for_jmp_slot”?

Ok, it does make sense if the idea would be extend not only for functions
calls.

> 
>> @@ -229,16 +230,16 @@ _dl_audit_symbind (struct link_map *l, struct reloc_result *reloc_result,
>>  	{
>>  	  if (afct->symbind != NULL)
>>  	    {
>> -	      uintptr_t new_value = afct->symbind (&sym,
>> -						   reloc_result->boundndx,
>> -						   &l_state->cookie,
>> -						   &result_state->cookie,
>> -						   &flags,
>> -						   strtab2 + defsym->st_name);
>> +	      flags |= bindnow ? LA_SYMB_NOPLTENTER | LA_SYMB_NOPLTEXIT : 0;
>> +	      new_value = afct->symbind (&sym, reloc_result->boundndx,
>> +					 &l_state->cookie,
>> +					 &result_state->cookie, &flags,
>> +					 strtab2 + defsym->st_name);
>>  	      if (new_value != (uintptr_t) sym.st_value)
>>  		{
>>  		  flags |= LA_SYMB_ALTVALUE;
>> -		  sym.st_value = new_value;
>> +		  sym.st_value = bindnow
>> +		    ? DL_FIXUP_BINDNOW_ADDR_VALUE (new_value) : new_value;
>>  		}
>>  	    }
> 
> We could error out if the audit callback clears the LA_SYMB_NOPLT* flags
> we've set (after fixing sotruss).

I don't oppose bumping LAV_CURRENT, it is large change than I envisioned
in this change but it makes sense.  

> 
> And then we'll have to make this change conditional on the la_version
> return value, I think.

My understanding from previous discussion is we don't want to support mixed
lav versions, so bumping the minimal version should be suffice.  It would
required older modules to be rebuild against a newer glibc, but I think
it should be ok.

> 
>> --- a/elf/dl-reloc.c
>> +++ b/elf/dl-reloc.c
>> @@ -234,9 +234,7 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>>  
>>    /* If DT_BIND_NOW is set relocate all references in this object.  We
>>       do not do this if we are profiling, of course.  */
>> -  // XXX Correct for auditing?
>> -  if (!consider_profiling
>> -      && __builtin_expect (l->l_info[DT_BIND_NOW] != NULL, 0))
>> +  if (!consider_profiling && l->l_info[DT_BIND_NOW] != NULL)
>>      lazy = 0;
>>  
>>    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_RELOC))
>> @@ -285,8 +283,6 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>>    {
>>      /* Do the actual relocation of the object's GOT and other data.  */
>>  
>> -    ELF_DYNAMIC_RELOCATE (l, scope, lazy, consider_profiling, skip_ifunc);
>> -
>>  #ifndef PROF
>>      if (consider_profiling | consider_symbind
>>  	&& l->l_info[DT_PLTRELSZ] != NULL)
>> @@ -309,6 +305,8 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>>  	  }
>>        }
>>  #endif
>> +
>> +    ELF_DYNAMIC_RELOCATE (l, scope, lazy, consider_profiling, skip_ifunc);
>>    }
> 
> I don't understand why this is needed.  Can we indeed use the profiling
> trampoline with a BIND_NOW binding?  If not, then we shouldn't need the
> allocation of l_reloc_result, either.  We should patch in the changed
> symbol variable directly, not going through l_reloc_result.

I does not indeed, I just did it to avoid adjusting _dl_audit_symbind. But
I agree that not requiring allocate l_reloc_results for bind-now is a better
strategy. I will fix it.