Re: [PATCH v3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA

[Fangrui Song <maskray@google.com>]

On 2022-06-07, Szabolcs Nagy wrote:
>The 06/01/2022 10:56, Fangrui Song wrote:
>> If an executable has copy relocations for extern protected data, that
>> can only work if the library containing the definition is built with
>> assumptions (a) the compiler emits GOT-generating relocations (b) the
>> linker produces R_*_GLOB_DAT instead of R_*_RELATIVE.  Otherwise the
>> library uses its own definition directly and the executable accesses a
>> stale copy.  Note: the GOT relocations defeat the purpose of protected
>> visibility as an optimization, but allow rtld to make the executable and
>> library use the same copy when copy relocations are present, but it
>> turns out this never worked perfectly.
>>
>> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange semantics when both
>> a.so and b.so define protected var and the executable copy relocates
>> var: b.so accesses its own copy even with GLOB_DAT.  The behavior change
>> is from commit 62da1e3b00b51383ffa7efc89d8addda0502e107 (x86) and then
>> copied to nios2 (ae5eae7cfc9c4a8297ff82ec6b794faca1976ecc) and arc
>> (0e7d930c4c11de896fe807f67fa1eb756c9c1e05).
>>
>> Without ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA, b.so accesses the copy
>> relocated data like a.so.
>>
>> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has another effect in the absence
>> of copy relocations: when a protected data symbol is defined in multiple
>> objects, the code tries to bind the relocation locally.  Without
>> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA, STV_PROTECTED is handled in the
>> same way as STV_DEFAULT: if ld produces GLOB_DAT (some ports of GNU ld),
>> the relocation will bind to the first definition; otherwise (e.g.
>> ld.lld) ld does the binding locally and ld.so doesn't help.
>>
>
>i think we should not change the interposition semantics.
>we should go back to the old behaviour where only copy
>relocs were broken (and there was an expensive workaround
>to deal with protected symbol interposition).
>
>i think you want to revert the elf/dl-lookup.c changes of
>
>  commit 62da1e3b00b51383ffa7efc89d8addda0502e107
>  Author:     H.J. Lu <hjl.tools@gmail.com>
>  CommitDate: 2015-03-31 05:16:57 -0700
>
>  Add ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA to x86

Thanks for taking a look.

The elf/dl-lookup.c change is removed by this patch:
```
-         /* When UNDEF_MAP is NULL, which indicates we are called from
-            do_lookup_x on relocation against protected data, we skip
-            the data definion in the executable from copy reloc.  */
-         if (ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA
...
```

>
>> It's extremely unlikely anyone relies on the
>> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA behavior, so let's remove it: this
>> removes a check in the symbol lookup code.
>>
>> --
>> Changes from v1:
>> * Reword commit message as suggested by Szabolcs Nagy
>>
>> Changes from v2:
>> * Explain interposition behavior
>> ---
>>  elf/dl-lookup.c             | 90 -------------------------------------
>>  sysdeps/arc/dl-sysdep.h     | 21 ---------
>>  sysdeps/generic/ldsodefs.h  | 12 +----
>>  sysdeps/i386/dl-machine.h   |  3 +-
>>  sysdeps/nios2/dl-sysdep.h   | 21 ---------
>>  sysdeps/x86/dl-lookupcfg.h  |  4 --
>>  sysdeps/x86_64/dl-machine.h |  8 +---
>>  7 files changed, 4 insertions(+), 155 deletions(-)
>>  delete mode 100644 sysdeps/arc/dl-sysdep.h
>>  delete mode 100644 sysdeps/nios2/dl-sysdep.h
>>
>> diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
>> index a42f6d5390..41d108e0b8 100644
>> --- a/elf/dl-lookup.c
>> +++ b/elf/dl-lookup.c
>...
>> @@ -854,43 +801,6 @@ _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map,
>>        return 0;
>>      }
>>
>> -  int protected = (*ref
>> -		   && ELFW(ST_VISIBILITY) ((*ref)->st_other) == STV_PROTECTED);
>> -  if (__glibc_unlikely (protected != 0))
>> -    {
>> -      /* It is very tricky.  We need to figure out what value to
>> -	 return for the protected symbol.  */
>> -      if (type_class == ELF_RTYPE_CLASS_PLT)
>> -	{
>> -	  if (current_value.s != NULL && current_value.m != undef_map)
>> -	    {
>> -	      current_value.s = *ref;
>> -	      current_value.m = undef_map;
>> -	    }
>> -	}
>> -      else
>> -	{
>> -	  struct sym_val protected_value = { NULL, NULL };
>> -
>> -	  for (scope = symbol_scope; *scope != NULL; i = 0, ++scope)
>> -	    if (do_lookup_x (undef_name, new_hash, &old_hash, *ref,
>> -			     &protected_value, *scope, i, version, flags,
>> -			     skip_map,
>> -			     (ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA
>> -			      && ELFW(ST_TYPE) ((*ref)->st_info) == STT_OBJECT
>> -			      && type_class == ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA)
>> -			     ? ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA
>> -			     : ELF_RTYPE_CLASS_PLT, NULL) != 0)
>> -	      break;
>> -
>> -	  if (protected_value.s != NULL && protected_value.m != undef_map)
>> -	    {
>> -	      current_value.s = *ref;
>> -	      current_value.m = undef_map;
>> -	    }
>> -	}
>> -    }
>> -
>
>i think we should keep this part without the
>ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA bit.

I have played a bit but do not find any difference (with some examples using
"canonical PLT entries") if I simply remove the whole if statement.  Do you
find anything I may have missed?