[v3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA

Message ID 20220601175633.2407189-1-maskray@google.com
State New
Headers
Series [v3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686
redhat-pt-bot/TryBot-still_applies warning Patch no longer applies to master

Commit Message

Fangrui Song June 1, 2022, 5:56 p.m. UTC
  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.

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
  

Comments

Szabolcs Nagy June 7, 2022, 1:24 p.m. UTC | #1
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.
  
Fangrui Song June 7, 2022, 5:49 p.m. UTC | #2
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?
  
H.J. Lu June 7, 2022, 5:49 p.m. UTC | #3
On Tue, Jun 7, 2022 at 6:25 AM Szabolcs Nagy via Libc-alpha
<libc-alpha@sourceware.org> 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
>

I am OK to remove support of copy relocation against protected
symbols since it doesn't work properly.  My only question is if
ld.so should issue a warning or an error when seeing a copy
relocation against a protected symbol.   Copy relocation against
protected symbol defeats the purpose of protected symbol.

> > 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.
  
Fangrui Song June 7, 2022, 6:21 p.m. UTC | #4
On 2022-06-07, H.J. Lu wrote:
>On Tue, Jun 7, 2022 at 6:25 AM Szabolcs Nagy via Libc-alpha
><libc-alpha@sourceware.org> 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
>>
>
>I am OK to remove support of copy relocation against protected
>symbols since it doesn't work properly.  

Thanks.

>My only question is if
>ld.so should issue a warning or an error when seeing a copy
>relocation against a protected symbol.   Copy relocation against
>protected symbol defeats the purpose of protected symbol.

The check already exists (_dl_check_protected_symbol) but currently
relies on GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (only implemented
for x86, and adoption is low on x86).

For ELF_RTYPE_CLASS_COPY, I think the GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
check can be removed.
(
Since GCC 5, x86-64 -fpie has HAVE_LD_PIE_COPYRELOC.
When neither -m[no]direct-extern-access is specified, HAVE_LD_PIE_COPYRELOC takes effect.
The executable does not have GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
but the incompatibility exists.
It just kinda works because GCC and GNU ld cooperate to produce a GLOB_DAT in the DSO.
)

For ELF_RTYPE_CLASS_PLT, the pointer equality does not matter much in
practice:

* protected visibility adoption is very low due to various problems.
* Taking a function address in the executable and expecting it to match the address in a DSO is rare.
* Many users use ICF and by and large don't care about function addresses to some extent.

I think having the warning under GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS is fine.
(
* x86-32 -fno-pic uses R_386_PC32 as a jump instruction, which is
   indistinguishable from an address-taken operation
   https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#branch-instructions-on-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.
>
>
>
>-- 
>H.J.
  
H.J. Lu June 7, 2022, 7:21 p.m. UTC | #5
On Tue, Jun 7, 2022 at 11:21 AM Fangrui Song <maskray@google.com> wrote:
>
> On 2022-06-07, H.J. Lu wrote:
> >On Tue, Jun 7, 2022 at 6:25 AM Szabolcs Nagy via Libc-alpha
> ><libc-alpha@sourceware.org> 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
> >>
> >
> >I am OK to remove support of copy relocation against protected
> >symbols since it doesn't work properly.
>
> Thanks.
>
> >My only question is if
> >ld.so should issue a warning or an error when seeing a copy
> >relocation against a protected symbol.   Copy relocation against
> >protected symbol defeats the purpose of protected symbol.
>
> The check already exists (_dl_check_protected_symbol) but currently
> relies on GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (only implemented
> for x86, and adoption is low on x86).
>
> For ELF_RTYPE_CLASS_COPY, I think the GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
> check can be removed.

Will removal of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
check cause many run-time errors?

> (
> Since GCC 5, x86-64 -fpie has HAVE_LD_PIE_COPYRELOC.
> When neither -m[no]direct-extern-access is specified, HAVE_LD_PIE_COPYRELOC takes effect.
> The executable does not have GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
> but the incompatibility exists.
> It just kinda works because GCC and GNU ld cooperate to produce a GLOB_DAT in the DSO.
> )
>
> For ELF_RTYPE_CLASS_PLT, the pointer equality does not matter much in
> practice:
>
> * protected visibility adoption is very low due to various problems.
> * Taking a function address in the executable and expecting it to match the address in a DSO is rare.
> * Many users use ICF and by and large don't care about function addresses to some extent.
>
> I think having the warning under GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS is fine.
> (
> * x86-32 -fno-pic uses R_386_PC32 as a jump instruction, which is
>    indistinguishable from an address-taken operation
>    https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#branch-instructions-on-x86
> )

An error with GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
and a warning without?

>
> >> > 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.
> >
> >
> >
> >--
> >H.J.
  
Fangrui Song June 7, 2022, 8 p.m. UTC | #6
On 2022-06-07, H.J. Lu wrote:
>On Tue, Jun 7, 2022 at 11:21 AM Fangrui Song <maskray@google.com> wrote:
>>
>> On 2022-06-07, H.J. Lu wrote:
>> >On Tue, Jun 7, 2022 at 6:25 AM Szabolcs Nagy via Libc-alpha
>> ><libc-alpha@sourceware.org> 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
>> >>
>> >
>> >I am OK to remove support of copy relocation against protected
>> >symbols since it doesn't work properly.
>>
>> Thanks.
>>
>> >My only question is if
>> >ld.so should issue a warning or an error when seeing a copy
>> >relocation against a protected symbol.   Copy relocation against
>> >protected symbol defeats the purpose of protected symbol.
>>
>> The check already exists (_dl_check_protected_symbol) but currently
>> relies on GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (only implemented
>> for x86, and adoption is low on x86).
>>
>> For ELF_RTYPE_CLASS_COPY, I think the GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> check can be removed.
>
>Will removal of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>check cause many run-time errors?
>> (
>> Since GCC 5, x86-64 -fpie has HAVE_LD_PIE_COPYRELOC.
>> When neither -m[no]direct-extern-access is specified, HAVE_LD_PIE_COPYRELOC takes effect.
>> The executable does not have GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> but the incompatibility exists.
>> It just kinda works because GCC and GNU ld cooperate to produce a GLOB_DAT in the DSO.
>> )
>>
>> For ELF_RTYPE_CLASS_PLT, the pointer equality does not matter much in
>> practice:
>>
>> * protected visibility adoption is very low due to various problems.
>> * Taking a function address in the executable and expecting it to match the address in a DSO is rare.
>> * Many users use ICF and by and large don't care about function addresses to some extent.
>>
>> I think having the warning under GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS is fine.
>> (
>> * x86-32 -fno-pic uses R_386_PC32 as a jump instruction, which is
>>    indistinguishable from an address-taken operation
>>    https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#branch-instructions-on-x86
>> )
>
>An error with GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>and a warning without?

This plan sounds good, when we create a separate patch enhancing the
diagnostics.

x86-32 may need a exception (i.e. no warning) for ELF_RTYPE_CLASS_PLT to handle R_386_PC32.

>> >> > 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.
>> >
>> >
>> >
>> >--
>> >H.J.
>
>
>
>-- 
>H.J.
  
H.J. Lu June 7, 2022, 9:02 p.m. UTC | #7
On Tue, Jun 7, 2022 at 1:00 PM Fangrui Song <maskray@google.com> wrote:
>
> On 2022-06-07, H.J. Lu wrote:
> >On Tue, Jun 7, 2022 at 11:21 AM Fangrui Song <maskray@google.com> wrote:
> >>
> >> On 2022-06-07, H.J. Lu wrote:
> >> >On Tue, Jun 7, 2022 at 6:25 AM Szabolcs Nagy via Libc-alpha
> >> ><libc-alpha@sourceware.org> 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
> >> >>
> >> >
> >> >I am OK to remove support of copy relocation against protected
> >> >symbols since it doesn't work properly.
> >>
> >> Thanks.
> >>
> >> >My only question is if
> >> >ld.so should issue a warning or an error when seeing a copy
> >> >relocation against a protected symbol.   Copy relocation against
> >> >protected symbol defeats the purpose of protected symbol.
> >>
> >> The check already exists (_dl_check_protected_symbol) but currently
> >> relies on GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (only implemented
> >> for x86, and adoption is low on x86).
> >>
> >> For ELF_RTYPE_CLASS_COPY, I think the GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
> >> check can be removed.
> >
> >Will removal of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
> >check cause many run-time errors?
> >> (
> >> Since GCC 5, x86-64 -fpie has HAVE_LD_PIE_COPYRELOC.
> >> When neither -m[no]direct-extern-access is specified, HAVE_LD_PIE_COPYRELOC takes effect.
> >> The executable does not have GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
> >> but the incompatibility exists.
> >> It just kinda works because GCC and GNU ld cooperate to produce a GLOB_DAT in the DSO.
> >> )
> >>
> >> For ELF_RTYPE_CLASS_PLT, the pointer equality does not matter much in
> >> practice:
> >>
> >> * protected visibility adoption is very low due to various problems.
> >> * Taking a function address in the executable and expecting it to match the address in a DSO is rare.
> >> * Many users use ICF and by and large don't care about function addresses to some extent.
> >>
> >> I think having the warning under GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS is fine.
> >> (
> >> * x86-32 -fno-pic uses R_386_PC32 as a jump instruction, which is
> >>    indistinguishable from an address-taken operation
> >>    https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#branch-instructions-on-x86
> >> )
> >
> >An error with GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
> >and a warning without?
>
> This plan sounds good, when we create a separate patch enhancing the
> diagnostics.
>
> x86-32 may need a exception (i.e. no warning) for ELF_RTYPE_CLASS_PLT to handle R_386_PC32.

Linker sets non-zero symbol values for undefined function symbols in
executable only when their addresses are taken.  R_386_PC32 shouldn't
matter.

> >> >> > 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.
> >> >
> >> >
> >> >
> >> >--
> >> >H.J.
> >
> >
> >
> >--
> >H.J.
  
Fangrui Song June 7, 2022, 11:57 p.m. UTC | #8
On 2022-06-07, H.J. Lu wrote:
>On Tue, Jun 7, 2022 at 1:00 PM Fangrui Song <maskray@google.com> wrote:
>>
>> On 2022-06-07, H.J. Lu wrote:
>> >On Tue, Jun 7, 2022 at 11:21 AM Fangrui Song <maskray@google.com> wrote:
>> >>
>> >> On 2022-06-07, H.J. Lu wrote:
>> >> >On Tue, Jun 7, 2022 at 6:25 AM Szabolcs Nagy via Libc-alpha
>> >> ><libc-alpha@sourceware.org> 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
>> >> >>
>> >> >
>> >> >I am OK to remove support of copy relocation against protected
>> >> >symbols since it doesn't work properly.
>> >>
>> >> Thanks.
>> >>
>> >> >My only question is if
>> >> >ld.so should issue a warning or an error when seeing a copy
>> >> >relocation against a protected symbol.   Copy relocation against
>> >> >protected symbol defeats the purpose of protected symbol.
>> >>
>> >> The check already exists (_dl_check_protected_symbol) but currently
>> >> relies on GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (only implemented
>> >> for x86, and adoption is low on x86).
>> >>
>> >> For ELF_RTYPE_CLASS_COPY, I think the GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> >> check can be removed.
>> >
>> >Will removal of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> >check cause many run-time errors?
>> >> (
>> >> Since GCC 5, x86-64 -fpie has HAVE_LD_PIE_COPYRELOC.
>> >> When neither -m[no]direct-extern-access is specified, HAVE_LD_PIE_COPYRELOC takes effect.
>> >> The executable does not have GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> >> but the incompatibility exists.
>> >> It just kinda works because GCC and GNU ld cooperate to produce a GLOB_DAT in the DSO.
>> >> )
>> >>
>> >> For ELF_RTYPE_CLASS_PLT, the pointer equality does not matter much in
>> >> practice:
>> >>
>> >> * protected visibility adoption is very low due to various problems.
>> >> * Taking a function address in the executable and expecting it to match the address in a DSO is rare.
>> >> * Many users use ICF and by and large don't care about function addresses to some extent.
>> >>
>> >> I think having the warning under GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS is fine.
>> >> (
>> >> * x86-32 -fno-pic uses R_386_PC32 as a jump instruction, which is
>> >>    indistinguishable from an address-taken operation
>> >>    https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#branch-instructions-on-x86
>> >> )
>> >
>> >An error with GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> >and a warning without?
>>
>> This plan sounds good, when we create a separate patch enhancing the
>> diagnostics.

Created
https://sourceware.org/pipermail/libc-alpha/2022-June/139552.html
([PATCH] elf: Refine direct extern access diagnostics to protected symbol).

>> x86-32 may need a exception (i.e. no warning) for ELF_RTYPE_CLASS_PLT to handle R_386_PC32.
>
>Linker sets non-zero symbol values for undefined function symbols in
>executable only when their addresses are taken.  R_386_PC32 shouldn't
>matter.

OK, I believe GNU ld distinguishes branch/address-taken usages of
R_386_PC32 by poking at the instruction opcode.  That works.
ld.lld doesn't check the opcode, and just reports "error: cannot preempt
symbol:" in an example I crafted.

>> >> >> > 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.
>> >> >
>> >> >
>> >> >
>> >> >--
>> >> >H.J.
>> >
>> >
>> >
>> >--
>> >H.J.
>
>
>
>-- 
>H.J.
  
H.J. Lu June 8, 2022, 1:51 a.m. UTC | #9
On Tue, Jun 7, 2022 at 4:57 PM Fangrui Song <maskray@google.com> wrote:
>
>
> On 2022-06-07, H.J. Lu wrote:
> >On Tue, Jun 7, 2022 at 1:00 PM Fangrui Song <maskray@google.com> wrote:
> >>
> >> On 2022-06-07, H.J. Lu wrote:
> >> >On Tue, Jun 7, 2022 at 11:21 AM Fangrui Song <maskray@google.com> wrote:
> >> >>
> >> >> On 2022-06-07, H.J. Lu wrote:
> >> >> >On Tue, Jun 7, 2022 at 6:25 AM Szabolcs Nagy via Libc-alpha
> >> >> ><libc-alpha@sourceware.org> 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
> >> >> >>
> >> >> >
> >> >> >I am OK to remove support of copy relocation against protected
> >> >> >symbols since it doesn't work properly.
> >> >>
> >> >> Thanks.
> >> >>
> >> >> >My only question is if
> >> >> >ld.so should issue a warning or an error when seeing a copy
> >> >> >relocation against a protected symbol.   Copy relocation against
> >> >> >protected symbol defeats the purpose of protected symbol.
> >> >>
> >> >> The check already exists (_dl_check_protected_symbol) but currently
> >> >> relies on GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (only implemented
> >> >> for x86, and adoption is low on x86).
> >> >>
> >> >> For ELF_RTYPE_CLASS_COPY, I think the GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
> >> >> check can be removed.
> >> >
> >> >Will removal of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
> >> >check cause many run-time errors?
> >> >> (
> >> >> Since GCC 5, x86-64 -fpie has HAVE_LD_PIE_COPYRELOC.
> >> >> When neither -m[no]direct-extern-access is specified, HAVE_LD_PIE_COPYRELOC takes effect.
> >> >> The executable does not have GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
> >> >> but the incompatibility exists.
> >> >> It just kinda works because GCC and GNU ld cooperate to produce a GLOB_DAT in the DSO.
> >> >> )
> >> >>
> >> >> For ELF_RTYPE_CLASS_PLT, the pointer equality does not matter much in
> >> >> practice:
> >> >>
> >> >> * protected visibility adoption is very low due to various problems.
> >> >> * Taking a function address in the executable and expecting it to match the address in a DSO is rare.
> >> >> * Many users use ICF and by and large don't care about function addresses to some extent.
> >> >>
> >> >> I think having the warning under GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS is fine.
> >> >> (
> >> >> * x86-32 -fno-pic uses R_386_PC32 as a jump instruction, which is
> >> >>    indistinguishable from an address-taken operation
> >> >>    https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#branch-instructions-on-x86
> >> >> )
> >> >
> >> >An error with GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
> >> >and a warning without?
> >>
> >> This plan sounds good, when we create a separate patch enhancing the
> >> diagnostics.
>
> Created
> https://sourceware.org/pipermail/libc-alpha/2022-June/139552.html
> ([PATCH] elf: Refine direct extern access diagnostics to protected symbol).
>
> >> x86-32 may need a exception (i.e. no warning) for ELF_RTYPE_CLASS_PLT to handle R_386_PC32.
> >
> >Linker sets non-zero symbol values for undefined function symbols in
> >executable only when their addresses are taken.  R_386_PC32 shouldn't
> >matter.
>
> OK, I believe GNU ld distinguishes branch/address-taken usages of
> R_386_PC32 by poking at the instruction opcode.  That works.
> ld.lld doesn't check the opcode, and just reports "error: cannot preempt
> symbol:" in an example I crafted.

Does lld always set non-zero symbol values for undefined function symbols?

> >> >> >> > 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.
> >> >> >
> >> >> >
> >> >> >
> >> >> >--
> >> >> >H.J.
> >> >
> >> >
> >> >
> >> >--
> >> >H.J.
> >
> >
> >
> >--
> >H.J.
  
Fangrui Song June 8, 2022, 3:42 a.m. UTC | #10
On 2022-06-07, H.J. Lu wrote:
>On Tue, Jun 7, 2022 at 4:57 PM Fangrui Song <maskray@google.com> wrote:
>>
>>
>> On 2022-06-07, H.J. Lu wrote:
>> >On Tue, Jun 7, 2022 at 1:00 PM Fangrui Song <maskray@google.com> wrote:
>> >>
>> >> On 2022-06-07, H.J. Lu wrote:
>> >> >On Tue, Jun 7, 2022 at 11:21 AM Fangrui Song <maskray@google.com> wrote:
>> >> >>
>> >> >> On 2022-06-07, H.J. Lu wrote:
>> >> >> >On Tue, Jun 7, 2022 at 6:25 AM Szabolcs Nagy via Libc-alpha
>> >> >> ><libc-alpha@sourceware.org> 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
>> >> >> >>
>> >> >> >
>> >> >> >I am OK to remove support of copy relocation against protected
>> >> >> >symbols since it doesn't work properly.
>> >> >>
>> >> >> Thanks.
>> >> >>
>> >> >> >My only question is if
>> >> >> >ld.so should issue a warning or an error when seeing a copy
>> >> >> >relocation against a protected symbol.   Copy relocation against
>> >> >> >protected symbol defeats the purpose of protected symbol.
>> >> >>
>> >> >> The check already exists (_dl_check_protected_symbol) but currently
>> >> >> relies on GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (only implemented
>> >> >> for x86, and adoption is low on x86).
>> >> >>
>> >> >> For ELF_RTYPE_CLASS_COPY, I think the GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> >> >> check can be removed.
>> >> >
>> >> >Will removal of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> >> >check cause many run-time errors?
>> >> >> (
>> >> >> Since GCC 5, x86-64 -fpie has HAVE_LD_PIE_COPYRELOC.
>> >> >> When neither -m[no]direct-extern-access is specified, HAVE_LD_PIE_COPYRELOC takes effect.
>> >> >> The executable does not have GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> >> >> but the incompatibility exists.
>> >> >> It just kinda works because GCC and GNU ld cooperate to produce a GLOB_DAT in the DSO.
>> >> >> )
>> >> >>
>> >> >> For ELF_RTYPE_CLASS_PLT, the pointer equality does not matter much in
>> >> >> practice:
>> >> >>
>> >> >> * protected visibility adoption is very low due to various problems.
>> >> >> * Taking a function address in the executable and expecting it to match the address in a DSO is rare.
>> >> >> * Many users use ICF and by and large don't care about function addresses to some extent.
>> >> >>
>> >> >> I think having the warning under GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS is fine.
>> >> >> (
>> >> >> * x86-32 -fno-pic uses R_386_PC32 as a jump instruction, which is
>> >> >>    indistinguishable from an address-taken operation
>> >> >>    https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#branch-instructions-on-x86
>> >> >> )
>> >> >
>> >> >An error with GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS
>> >> >and a warning without?
>> >>
>> >> This plan sounds good, when we create a separate patch enhancing the
>> >> diagnostics.
>>
>> Created
>> https://sourceware.org/pipermail/libc-alpha/2022-June/139552.html
>> ([PATCH] elf: Refine direct extern access diagnostics to protected symbol).
>>
>> >> x86-32 may need a exception (i.e. no warning) for ELF_RTYPE_CLASS_PLT to handle R_386_PC32.
>> >
>> >Linker sets non-zero symbol values for undefined function symbols in
>> >executable only when their addresses are taken.  R_386_PC32 shouldn't
>> >matter.
>>
>> OK, I believe GNU ld distinguishes branch/address-taken usages of
>> R_386_PC32 by poking at the instruction opcode.  That works.
>> ld.lld doesn't check the opcode, and just reports "error: cannot preempt
>> symbol:" in an example I crafted.
>
>Does lld always set non-zero symbol values for undefined function symbols?

For a direct access relocation which is neither a link-time constant nor
a dynamic relocation (e.g. R_386_32 in a writable section), lld always
sets non-zero st_value for an undefined symbol referencing STT_OBJECT.

lld doesn't know R_386_PC32 may be used with call/jmp.  It simply
conservatively treats all R_386_PC32 as possibly address-taking.
BTW: This was why I opened https://sourceware.org/bugzilla/show_bug.cgi?id=27169
(i386: Emit R_386_PLT32 instead of R_386_PC32 for `call/jmp foo`) but I
can accept that we keep it unchanged.

>> >> >> >> > 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.
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> >--
>> >> >> >H.J.
>> >> >
>> >> >
>> >> >
>> >> >--
>> >> >H.J.
>> >
>> >
>> >
>> >--
>> >H.J.
>
>
>
>-- 
>H.J.
  
Szabolcs Nagy June 8, 2022, 9:15 a.m. UTC | #11
The 06/07/2022 10:49, Fangrui Song wrote:
> On 2022-06-07, Szabolcs Nagy wrote:
> > > -  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?

yes, i posted an example earlier that behaves differently.

object symbol defined in exe and dso, the dso one is protected
and has a GOT reloc for it.

with the extra logic the GOT is resolved to the definition in
the dso, without it the exe interposes the protected symbol.
  
Fangrui Song June 8, 2022, 5:16 p.m. UTC | #12
On 2022-06-08, Szabolcs Nagy wrote:
>The 06/07/2022 10:49, Fangrui Song wrote:
>> On 2022-06-07, Szabolcs Nagy wrote:
>> > > -  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?
>
>yes, i posted an example earlier that behaves differently.

OK. You meant the
https://sourceware.org/pipermail/libc-alpha/2022-May/139183.html
example with GNU ld as the linker.

With lld the behavior is the same with or without the code block.

>object symbol defined in exe and dso, the dso one is protected
>and has a GOT reloc for it.
>
>with the extra logic the GOT is resolved to the definition in
>the dso, without it the exe interposes the protected symbol.

Created
https://sourceware.org/pipermail/libc-alpha/2022-June/139574.html
I'd still wish that the code block is removed, but we can do that later.
I assume that once one port of GNU ld stops producing GLOB_DAT for
protected symbol, we can drop the code block for that port.
  
Szabolcs Nagy June 9, 2022, 8:12 a.m. UTC | #13
The 06/08/2022 10:16, Fangrui Song wrote:
> On 2022-06-08, Szabolcs Nagy wrote:
> > The 06/07/2022 10:49, Fangrui Song wrote:
> > > On 2022-06-07, Szabolcs Nagy wrote:
> > > > > -  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?
> > 
> > yes, i posted an example earlier that behaves differently.
> 
> OK. You meant the
> https://sourceware.org/pipermail/libc-alpha/2022-May/139183.html
> example with GNU ld as the linker.
> 
> With lld the behavior is the same with or without the code block.

well you need GOT reloc, obviously if lld locally binds
the symbol then there will be no interpositon.

but we must support GOT relocs for protected symbols,
that's perfectly valid (even if no linker generates it).

> 
> > object symbol defined in exe and dso, the dso one is protected
> > and has a GOT reloc for it.
> > 
> > with the extra logic the GOT is resolved to the definition in
> > the dso, without it the exe interposes the protected symbol.
> 
> Created
> https://sourceware.org/pipermail/libc-alpha/2022-June/139574.html
> I'd still wish that the code block is removed, but we can do that later.
> I assume that once one port of GNU ld stops producing GLOB_DAT for
> protected symbol, we can drop the code block for that port.

thanks.
  

Patch

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
@@ -456,59 +456,6 @@  do_lookup_x (const char *undef_name, unsigned int new_hash,
       if (sym != NULL)
 	{
 	found_it:
-	  /* 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
-	      && undef_map == NULL
-	      && map->l_type == lt_executable
-	      && type_class == ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA)
-	    {
-	      const ElfW(Sym) *s;
-	      unsigned int i;
-
-#if ! ELF_MACHINE_NO_RELA
-	      if (map->l_info[DT_RELA] != NULL
-		  && map->l_info[DT_RELASZ] != NULL
-		  && map->l_info[DT_RELASZ]->d_un.d_val != 0)
-		{
-		  const ElfW(Rela) *rela
-		    = (const ElfW(Rela) *) D_PTR (map, l_info[DT_RELA]);
-		  unsigned int rela_count
-		    = map->l_info[DT_RELASZ]->d_un.d_val / sizeof (*rela);
-
-		  for (i = 0; i < rela_count; i++, rela++)
-		    if (elf_machine_type_class (ELFW(R_TYPE) (rela->r_info))
-			== ELF_RTYPE_CLASS_COPY)
-		      {
-			s = &symtab[ELFW(R_SYM) (rela->r_info)];
-			if (!strcmp (strtab + s->st_name, undef_name))
-			  goto skip;
-		      }
-		}
-#endif
-#if ! ELF_MACHINE_NO_REL
-	      if (map->l_info[DT_REL] != NULL
-		  && map->l_info[DT_RELSZ] != NULL
-		  && map->l_info[DT_RELSZ]->d_un.d_val != 0)
-		{
-		  const ElfW(Rel) *rel
-		    = (const ElfW(Rel) *) D_PTR (map, l_info[DT_REL]);
-		  unsigned int rel_count
-		    = map->l_info[DT_RELSZ]->d_un.d_val / sizeof (*rel);
-
-		  for (i = 0; i < rel_count; i++, rel++)
-		    if (elf_machine_type_class (ELFW(R_TYPE) (rel->r_info))
-			== ELF_RTYPE_CLASS_COPY)
-		      {
-			s = &symtab[ELFW(R_SYM) (rel->r_info)];
-			if (!strcmp (strtab + s->st_name, undef_name))
-			  goto skip;
-		      }
-		}
-#endif
-	    }
-
 	  /* Hidden and internal symbols are local, ignore them.  */
 	  if (__glibc_unlikely (dl_symbol_visibility_binds_local_p (sym)))
 	    goto skip;
@@ -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;
-	    }
-	}
-    }
-
   /* We have to check whether this would bind UNDEF_MAP to an object
      in the global scope which was dynamically loaded.  In this case
      we have to prevent the latter from being unloaded unless the
diff --git a/sysdeps/arc/dl-sysdep.h b/sysdeps/arc/dl-sysdep.h
deleted file mode 100644
index cf4d160a73..0000000000
--- a/sysdeps/arc/dl-sysdep.h
+++ /dev/null
@@ -1,21 +0,0 @@ 
-/* System-specific settings for dynamic linker code.  ARC version.
-   Copyright (C) 2020-2022 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_next <dl-sysdep.h>
-
-#define DL_EXTERN_PROTECTED_DATA
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 6716e1f382..d3cbfd4f95 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -149,23 +149,13 @@  dl_symbol_visibility_binds_local_p (const ElfW(Sym) *sym)
    satisfied by any symbol in the executable.  Some architectures do
    not support copy relocations.  In this case we define the macro to
    zero so that the code for handling them gets automatically optimized
-   out.  ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA means address of protected
-   data defined in the shared library may be external, i.e., due to copy
-   relocation.  */
+   out.  */
 #define ELF_RTYPE_CLASS_PLT 1
 #ifndef DL_NO_COPY_RELOCS
 # define ELF_RTYPE_CLASS_COPY 2
 #else
 # define ELF_RTYPE_CLASS_COPY 0
 #endif
-/* If DL_EXTERN_PROTECTED_DATA is defined, address of protected data
-   defined in the shared library may be external, i.e., due to copy
-   relocation.   */
-#ifdef DL_EXTERN_PROTECTED_DATA
-# define ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA 4
-#else
-# define ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA 0
-#endif
 
 /* ELF uses the PF_x macros to specify the segment permissions, mmap
    uses PROT_xxx.  In most cases the three macros have the values 1, 2,
diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
index 1f8d734215..3311631a3e 100644
--- a/sysdeps/i386/dl-machine.h
+++ b/sysdeps/i386/dl-machine.h
@@ -203,8 +203,7 @@  _dl_start_user:\n\
      || (type) == R_386_TLS_DTPOFF32 || (type) == R_386_TLS_TPOFF32	      \
      || (type) == R_386_TLS_TPOFF || (type) == R_386_TLS_DESC)		      \
     * ELF_RTYPE_CLASS_PLT)						      \
-   | (((type) == R_386_COPY) * ELF_RTYPE_CLASS_COPY)			      \
-   | (((type) == R_386_GLOB_DAT) * ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA))
+   | (((type) == R_386_COPY) * ELF_RTYPE_CLASS_COPY))
 
 /* A reloc type used for ld.so cmdline arg lookups to reject PLT entries.  */
 #define ELF_MACHINE_JMP_SLOT	R_386_JMP_SLOT
diff --git a/sysdeps/nios2/dl-sysdep.h b/sysdeps/nios2/dl-sysdep.h
deleted file mode 100644
index 257b37c258..0000000000
--- a/sysdeps/nios2/dl-sysdep.h
+++ /dev/null
@@ -1,21 +0,0 @@ 
-/* System-specific settings for dynamic linker code.  Nios II version.
-   Copyright (C) 2009-2022 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_next <dl-sysdep.h>
-
-#define DL_EXTERN_PROTECTED_DATA
diff --git a/sysdeps/x86/dl-lookupcfg.h b/sysdeps/x86/dl-lookupcfg.h
index 18b3b49f6e..e136cc63af 100644
--- a/sysdeps/x86/dl-lookupcfg.h
+++ b/sysdeps/x86/dl-lookupcfg.h
@@ -20,10 +20,6 @@ 
 
 #include_next <dl-lookupcfg.h>
 
-/* Address of protected data defined in the shared library may be
-   external due to copy relocation.   */
-#define DL_EXTERN_PROTECTED_DATA
-
 struct link_map;
 
 extern void _dl_unmap (struct link_map *map) attribute_hidden;
diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
index 7f607f6dff..78aecfc9fd 100644
--- a/sysdeps/x86_64/dl-machine.h
+++ b/sysdeps/x86_64/dl-machine.h
@@ -181,10 +181,7 @@  _dl_start_user:\n\
    TLS variable, so undefined references should not be allowed to
    define the value.
    ELF_RTYPE_CLASS_COPY iff TYPE should not be allowed to resolve to one
-   of the main executable's symbols, as for a COPY reloc.
-   ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA iff TYPE describes relocation may
-   against protected data whose address be external due to copy relocation.
- */
+   of the main executable's symbols, as for a COPY reloc.  */
 #define elf_machine_type_class(type)					      \
   ((((type) == R_X86_64_JUMP_SLOT					      \
      || (type) == R_X86_64_DTPMOD64					      \
@@ -192,8 +189,7 @@  _dl_start_user:\n\
      || (type) == R_X86_64_TPOFF64					      \
      || (type) == R_X86_64_TLSDESC)					      \
     * ELF_RTYPE_CLASS_PLT)						      \
-   | (((type) == R_X86_64_COPY) * ELF_RTYPE_CLASS_COPY)			      \
-   | (((type) == R_X86_64_GLOB_DAT) * ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA))
+   | (((type) == R_X86_64_COPY) * ELF_RTYPE_CLASS_COPY))
 
 /* A reloc type used for ld.so cmdline arg lookups to reject PLT entries.  */
 #define ELF_MACHINE_JMP_SLOT	R_X86_64_JUMP_SLOT