Re: [PATCH v3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

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


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