public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling
@ 2022-05-01  6:06 Fangrui Song
  2022-05-01  6:06 ` [PATCH 1/3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for non-DL_EXTERN_PROTECTED_DATA ports Fangrui Song
                   ` (3 more replies)
  0 siblings, 4 replies; 32+ messages in thread
From: Fangrui Song @ 2022-05-01  6:06 UTC (permalink / raw)
  To: libc-alpha, Adhemerval Zanella, Szabolcs Nagy

Say both a.so and b.so define protected var and the executable copy
relocates var. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange
semantics: a.so accesses the copy in the executable while b.so accesses
its own. This behavior requires that (a) the compiler emits
GOT-generating relocations (b) the linker produces GLOB_DAT instead of
RELATIVE.

Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
will bind to the executable (normal behavior).

For aarch64/arm it makes sense to restore the original behavior and don't
pay the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA cost. The behavior is very
unlikely used by anyone.

* Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
  no GOT-generating relocation in the first place.
* gold and lld reject copy relocation on a STV_PROTECTED symbol.
* Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
  GOT-generating relocation when accessing an default visibility
  external symbol which avoids copy relocation.

Fangrui Song (3):
  elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for
    non-DL_EXTERN_PROTECTED_DATA ports
  Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  Revert "[ARM][BZ #17711] Fix extern protected data handling"

 elf/dl-lookup.c              | 46 ++++++++++++------------------------
 sysdeps/aarch64/dl-machine.h | 13 +++++-----
 sysdeps/aarch64/dl-sysdep.h  |  2 --
 sysdeps/arm/dl-machine.h     | 10 +++-----
 sysdeps/arm/dl-sysdep.h      |  2 --
 5 files changed, 24 insertions(+), 49 deletions(-)

-- 
2.36.0.464.gb9c8b46e94-goog


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 1/3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for non-DL_EXTERN_PROTECTED_DATA ports
  2022-05-01  6:06 [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling Fangrui Song
@ 2022-05-01  6:06 ` Fangrui Song
  2022-05-01  6:06 ` [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling" Fangrui Song
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 32+ messages in thread
From: Fangrui Song @ 2022-05-01  6:06 UTC (permalink / raw)
  To: libc-alpha, Adhemerval Zanella, Szabolcs Nagy

A port not defining DL_EXTERN_PROTECTED_DATA should not incur the
overhead. In addition, taking the address of a protected function never
works (either breaks pointer equality or rejected by ld). Just remove
the code related to functions.
---
 elf/dl-lookup.c | 46 +++++++++++++++-------------------------------
 1 file changed, 15 insertions(+), 31 deletions(-)

diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index 989b073e4f..1bb80688ad 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -863,40 +863,24 @@ _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))
+  if (ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA
+      && __glibc_unlikely (
+	  *ref && ELFW (ST_VISIBILITY) ((*ref)->st_other) == STV_PROTECTED
+	  && ELFW (ST_TYPE) ((*ref)->st_info) == STT_OBJECT))
     {
-      /* 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 };
+      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;
+      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, NULL)
+	    != 0)
+	  break;
 
-	  if (protected_value.s != NULL && protected_value.m != undef_map)
-	    {
-	      current_value.s = *ref;
-	      current_value.m = undef_map;
-	    }
+      if (protected_value.s != NULL && protected_value.m != undef_map)
+	{
+	  current_value.s = *ref;
+	  current_value.m = undef_map;
 	}
     }
 
-- 
2.36.0.464.gb9c8b46e94-goog


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-01  6:06 [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling Fangrui Song
  2022-05-01  6:06 ` [PATCH 1/3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for non-DL_EXTERN_PROTECTED_DATA ports Fangrui Song
@ 2022-05-01  6:06 ` Fangrui Song
  2022-05-23 20:10   ` Szabolcs Nagy
  2022-05-01  6:06 ` [PATCH 3/3] Revert "[ARM][BZ " Fangrui Song
  2022-05-09 21:26 ` [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling H.J. Lu
  3 siblings, 1 reply; 32+ messages in thread
From: Fangrui Song @ 2022-05-01  6:06 UTC (permalink / raw)
  To: libc-alpha, Adhemerval Zanella, Szabolcs Nagy

This reverts commit 0910702c4d2cf9e8302b35c9519548726e1ac489.

Say both a.so and b.so define protected var and the executable copy
relocates var. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange
semantics: a.so accesses the copy in the executable while b.so accesses
its own. This behavior requires that (a) the compiler emits
GOT-generating relocations (b) the linker produces GLOB_DAT instead of
RELATIVE.

Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
will bind to the executable (normal behavior).

For aarch64 it makes sense to restore the original behavior and don't
pay the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA cost. The behavior is very
unlikely used by anyone.

* Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
  no GOT-generating relocation in the first place.
* gold and lld reject copy relocation on a STV_PROTECTED symbol.
* Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
  GOT-generating relocation when accessing an default visibility
  external symbol which avoids copy relocation.
---
 sysdeps/aarch64/dl-machine.h | 13 ++++++-------
 sysdeps/aarch64/dl-sysdep.h  |  2 --
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
index b40050a981..530952a736 100644
--- a/sysdeps/aarch64/dl-machine.h
+++ b/sysdeps/aarch64/dl-machine.h
@@ -182,13 +182,12 @@ _dl_start_user:								\n\
 ");
 
 #define elf_machine_type_class(type)					\
-  ((((type) == AARCH64_R(JUMP_SLOT)					\
-     || (type) == AARCH64_R(TLS_DTPMOD)					\
-     || (type) == AARCH64_R(TLS_DTPREL)					\
-     || (type) == AARCH64_R(TLS_TPREL)					\
-     || (type) == AARCH64_R(TLSDESC)) * ELF_RTYPE_CLASS_PLT)		\
-   | (((type) == AARCH64_R(COPY)) * ELF_RTYPE_CLASS_COPY)		\
-   | (((type) == AARCH64_R(GLOB_DAT)) * ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA))
+  ((((type) == R_AARCH64_JUMP_SLOT ||					\
+     (type) == R_AARCH64_TLS_DTPMOD ||					\
+     (type) == R_AARCH64_TLS_DTPREL ||					\
+     (type) == R_AARCH64_TLS_TPREL ||					\
+     (type) == R_AARCH64_TLSDESC) * ELF_RTYPE_CLASS_PLT)		\
+   | (((type) == R_AARCH64_COPY) * ELF_RTYPE_CLASS_COPY))
 
 #define ELF_MACHINE_JMP_SLOT	AARCH64_R(JUMP_SLOT)
 
diff --git a/sysdeps/aarch64/dl-sysdep.h b/sysdeps/aarch64/dl-sysdep.h
index 667786671c..ac69f414f3 100644
--- a/sysdeps/aarch64/dl-sysdep.h
+++ b/sysdeps/aarch64/dl-sysdep.h
@@ -21,5 +21,3 @@
 /* _dl_argv cannot be attribute_relro, because _dl_start_user
    might write into it after _dl_start returns.  */
 #define DL_ARGV_NOT_RELRO 1
-
-#define DL_EXTERN_PROTECTED_DATA
-- 
2.36.0.464.gb9c8b46e94-goog


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 3/3] Revert "[ARM][BZ #17711] Fix extern protected data handling"
  2022-05-01  6:06 [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling Fangrui Song
  2022-05-01  6:06 ` [PATCH 1/3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for non-DL_EXTERN_PROTECTED_DATA ports Fangrui Song
  2022-05-01  6:06 ` [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling" Fangrui Song
@ 2022-05-01  6:06 ` Fangrui Song
  2022-05-23 20:11   ` Szabolcs Nagy
  2022-05-09 21:26 ` [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling H.J. Lu
  3 siblings, 1 reply; 32+ messages in thread
From: Fangrui Song @ 2022-05-01  6:06 UTC (permalink / raw)
  To: libc-alpha, Adhemerval Zanella, Szabolcs Nagy

This reverts commit 3bcea719ddd6ce399d7bccb492c40af77d216e42.
---
 sysdeps/arm/dl-machine.h | 10 +++-------
 sysdeps/arm/dl-sysdep.h  |  2 --
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
index a7898bf420..2a7f795e46 100644
--- a/sysdeps/arm/dl-machine.h
+++ b/sysdeps/arm/dl-machine.h
@@ -212,22 +212,18 @@ _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 against
-   protected data whose address may be external due to copy relocation.  */
+   of the main executable's symbols, as for a COPY reloc.  */
 #ifndef RTLD_BOOTSTRAP
 # define elf_machine_type_class(type) \
   ((((type) == R_ARM_JUMP_SLOT || (type) == R_ARM_TLS_DTPMOD32		\
      || (type) == R_ARM_TLS_DTPOFF32 || (type) == R_ARM_TLS_TPOFF32	\
      || (type) == R_ARM_TLS_DESC)					\
     * ELF_RTYPE_CLASS_PLT)						\
-   | (((type) == R_ARM_COPY) * ELF_RTYPE_CLASS_COPY)			\
-   | (((type) == R_ARM_GLOB_DAT) * ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA))
+   | (((type) == R_ARM_COPY) * ELF_RTYPE_CLASS_COPY))
 #else
 #define elf_machine_type_class(type) \
   ((((type) == R_ARM_JUMP_SLOT) * ELF_RTYPE_CLASS_PLT)	\
-   | (((type) == R_ARM_COPY) * ELF_RTYPE_CLASS_COPY)	\
-   | (((type) == R_ARM_GLOB_DAT) * ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA))
+   | (((type) == R_ARM_COPY) * ELF_RTYPE_CLASS_COPY))
 #endif
 
 /* A reloc type used for ld.so cmdline arg lookups to reject PLT entries.  */
diff --git a/sysdeps/arm/dl-sysdep.h b/sysdeps/arm/dl-sysdep.h
index ce7a84a7de..3099ee419f 100644
--- a/sysdeps/arm/dl-sysdep.h
+++ b/sysdeps/arm/dl-sysdep.h
@@ -21,5 +21,3 @@
 /* _dl_argv cannot be attribute_relro, because _dl_start_user
    might write into it after _dl_start returns.  */
 #define DL_ARGV_NOT_RELRO 1
-
-#define DL_EXTERN_PROTECTED_DATA
-- 
2.36.0.464.gb9c8b46e94-goog


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling
  2022-05-01  6:06 [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling Fangrui Song
                   ` (2 preceding siblings ...)
  2022-05-01  6:06 ` [PATCH 3/3] Revert "[ARM][BZ " Fangrui Song
@ 2022-05-09 21:26 ` H.J. Lu
  2022-05-10  7:42   ` Fangrui Song
  3 siblings, 1 reply; 32+ messages in thread
From: H.J. Lu @ 2022-05-09 21:26 UTC (permalink / raw)
  To: Fangrui Song; +Cc: libc-alpha, Adhemerval Zanella, Szabolcs Nagy

On Sat, Apr 30, 2022 at 11:06 PM Fangrui Song via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> Say both a.so and b.so define protected var and the executable copy
> relocates var. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange
> semantics: a.so accesses the copy in the executable while b.so accesses
> its own. This behavior requires that (a) the compiler emits
> GOT-generating relocations (b) the linker produces GLOB_DAT instead of
> RELATIVE.
>
> Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
> will bind to the executable (normal behavior).
>
> For aarch64/arm it makes sense to restore the original behavior and don't
> pay the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA cost. The behavior is very
> unlikely used by anyone.
>
> * Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
>   no GOT-generating relocation in the first place.
> * gold and lld reject copy relocation on a STV_PROTECTED symbol.
> * Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
>   GOT-generating relocation when accessing an default visibility
>   external symbol which avoids copy relocation.
>
> Fangrui Song (3):
>   elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for
>     non-DL_EXTERN_PROTECTED_DATA ports
>   Revert "[AArch64][BZ #17711] Fix extern protected data handling"
>   Revert "[ARM][BZ #17711] Fix extern protected data handling"
>
>  elf/dl-lookup.c              | 46 ++++++++++++------------------------
>  sysdeps/aarch64/dl-machine.h | 13 +++++-----
>  sysdeps/aarch64/dl-sysdep.h  |  2 --
>  sysdeps/arm/dl-machine.h     | 10 +++-----
>  sysdeps/arm/dl-sysdep.h      |  2 --
>  5 files changed, 24 insertions(+), 49 deletions(-)
>
> --
> 2.36.0.464.gb9c8b46e94-goog
>

Given that protected symbols never work properly with copy relocation,
can we change protected symbol handling to simply warn copy relocation
against protected data symbol and non-zero symbol values of undefined
symbols against protected function symbols?


-- 
H.J.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling
  2022-05-09 21:26 ` [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling H.J. Lu
@ 2022-05-10  7:42   ` Fangrui Song
  2022-05-10 15:02     ` H.J. Lu
  0 siblings, 1 reply; 32+ messages in thread
From: Fangrui Song @ 2022-05-10  7:42 UTC (permalink / raw)
  To: H.J. Lu; +Cc: libc-alpha, Adhemerval Zanella, Szabolcs Nagy

On 2022-05-09, H.J. Lu wrote:
>On Sat, Apr 30, 2022 at 11:06 PM Fangrui Song via Libc-alpha
><libc-alpha@sourceware.org> wrote:
>>
>> Say both a.so and b.so define protected var and the executable copy
>> relocates var. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange
>> semantics: a.so accesses the copy in the executable while b.so accesses
>> its own. This behavior requires that (a) the compiler emits
>> GOT-generating relocations (b) the linker produces GLOB_DAT instead of
>> RELATIVE.
>>
>> Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
>> will bind to the executable (normal behavior).
>>
>> For aarch64/arm it makes sense to restore the original behavior and don't
>> pay the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA cost. The behavior is very
>> unlikely used by anyone.
>>
>> * Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
>>   no GOT-generating relocation in the first place.
>> * gold and lld reject copy relocation on a STV_PROTECTED symbol.
>> * Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
>>   GOT-generating relocation when accessing an default visibility
>>   external symbol which avoids copy relocation.
>>
>> Fangrui Song (3):
>>   elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for
>>     non-DL_EXTERN_PROTECTED_DATA ports
>>   Revert "[AArch64][BZ #17711] Fix extern protected data handling"
>>   Revert "[ARM][BZ #17711] Fix extern protected data handling"
>>
>>  elf/dl-lookup.c              | 46 ++++++++++++------------------------
>>  sysdeps/aarch64/dl-machine.h | 13 +++++-----
>>  sysdeps/aarch64/dl-sysdep.h  |  2 --
>>  sysdeps/arm/dl-machine.h     | 10 +++-----
>>  sysdeps/arm/dl-sysdep.h      |  2 --
>>  5 files changed, 24 insertions(+), 49 deletions(-)
>>
>> --
>> 2.36.0.464.gb9c8b46e94-goog
>>
>
>Given that protected symbols never work properly with copy relocation,
>can we change protected symbol handling to simply warn copy relocation
>against protected data symbol and non-zero symbol values of undefined
>symbols against protected function symbols?

"protected symbols never work properly with copy relocation" generally
applies to all non-x86 architectures (arm/aarch64 has some guarantee,
but not reliable). My goal is indeed to remove relevant code for all
non-x86 architectures. They should not even bother testing
STV_PROTECTED.

On x86 there is some guarantee. The gcc>=5 -fpie
HAVE_LD_PIE_COPYRELOC behavior makes the problem trigger in some cases.
Having a warning in x86 specific files (e.g.
sysdeps/x86_64/dl-machine.h R_X86_64_COPY) seems appropriate.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling
  2022-05-10  7:42   ` Fangrui Song
@ 2022-05-10 15:02     ` H.J. Lu
  2022-05-12  4:56       ` Fangrui Song
  0 siblings, 1 reply; 32+ messages in thread
From: H.J. Lu @ 2022-05-10 15:02 UTC (permalink / raw)
  To: Fangrui Song; +Cc: GNU C Library, Adhemerval Zanella, Szabolcs Nagy

On Tue, May 10, 2022 at 12:42 AM Fangrui Song <maskray@google.com> wrote:
>
> On 2022-05-09, H.J. Lu wrote:
> >On Sat, Apr 30, 2022 at 11:06 PM Fangrui Song via Libc-alpha
> ><libc-alpha@sourceware.org> wrote:
> >>
> >> Say both a.so and b.so define protected var and the executable copy
> >> relocates var. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange
> >> semantics: a.so accesses the copy in the executable while b.so accesses
> >> its own. This behavior requires that (a) the compiler emits
> >> GOT-generating relocations (b) the linker produces GLOB_DAT instead of
> >> RELATIVE.
> >>
> >> Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
> >> will bind to the executable (normal behavior).
> >>
> >> For aarch64/arm it makes sense to restore the original behavior and don't
> >> pay the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA cost. The behavior is very
> >> unlikely used by anyone.
> >>
> >> * Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
> >>   no GOT-generating relocation in the first place.
> >> * gold and lld reject copy relocation on a STV_PROTECTED symbol.
> >> * Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
> >>   GOT-generating relocation when accessing an default visibility
> >>   external symbol which avoids copy relocation.
> >>
> >> Fangrui Song (3):
> >>   elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for
> >>     non-DL_EXTERN_PROTECTED_DATA ports
> >>   Revert "[AArch64][BZ #17711] Fix extern protected data handling"
> >>   Revert "[ARM][BZ #17711] Fix extern protected data handling"
> >>
> >>  elf/dl-lookup.c              | 46 ++++++++++++------------------------
> >>  sysdeps/aarch64/dl-machine.h | 13 +++++-----
> >>  sysdeps/aarch64/dl-sysdep.h  |  2 --
> >>  sysdeps/arm/dl-machine.h     | 10 +++-----
> >>  sysdeps/arm/dl-sysdep.h      |  2 --
> >>  5 files changed, 24 insertions(+), 49 deletions(-)
> >>
> >> --
> >> 2.36.0.464.gb9c8b46e94-goog
> >>
> >
> >Given that protected symbols never work properly with copy relocation,
> >can we change protected symbol handling to simply warn copy relocation
> >against protected data symbol and non-zero symbol values of undefined
> >symbols against protected function symbols?
>
> "protected symbols never work properly with copy relocation" generally
> applies to all non-x86 architectures (arm/aarch64 has some guarantee,
> but not reliable). My goal is indeed to remove relevant code for all
> non-x86 architectures. They should not even bother testing
> STV_PROTECTED.

Even on x86, protected symbols don't work as intended.   They don't
provide any performance benefits.  I think we should move away from
copy relocation on protected symbols.  We can either issue a warning
or an error.

> On x86 there is some guarantee. The gcc>=5 -fpie
> HAVE_LD_PIE_COPYRELOC behavior makes the problem trigger in some cases.
> Having a warning in x86 specific files (e.g.
> sysdeps/x86_64/dl-machine.h R_X86_64_COPY) seems appropriate.



-- 
H.J.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling
  2022-05-10 15:02     ` H.J. Lu
@ 2022-05-12  4:56       ` Fangrui Song
  2022-05-13 22:18         ` H.J. Lu
  0 siblings, 1 reply; 32+ messages in thread
From: Fangrui Song @ 2022-05-12  4:56 UTC (permalink / raw)
  To: H.J. Lu; +Cc: GNU C Library, Adhemerval Zanella, Szabolcs Nagy

On 2022-05-10, H.J. Lu wrote:
>On Tue, May 10, 2022 at 12:42 AM Fangrui Song <maskray@google.com> wrote:
>>
>> On 2022-05-09, H.J. Lu wrote:
>> >On Sat, Apr 30, 2022 at 11:06 PM Fangrui Song via Libc-alpha
>> ><libc-alpha@sourceware.org> wrote:
>> >>
>> >> Say both a.so and b.so define protected var and the executable copy
>> >> relocates var. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange
>> >> semantics: a.so accesses the copy in the executable while b.so accesses
>> >> its own. This behavior requires that (a) the compiler emits
>> >> GOT-generating relocations (b) the linker produces GLOB_DAT instead of
>> >> RELATIVE.
>> >>
>> >> Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
>> >> will bind to the executable (normal behavior).
>> >>
>> >> For aarch64/arm it makes sense to restore the original behavior and don't
>> >> pay the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA cost. The behavior is very
>> >> unlikely used by anyone.
>> >>
>> >> * Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
>> >>   no GOT-generating relocation in the first place.
>> >> * gold and lld reject copy relocation on a STV_PROTECTED symbol.
>> >> * Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
>> >>   GOT-generating relocation when accessing an default visibility
>> >>   external symbol which avoids copy relocation.
>> >>
>> >> Fangrui Song (3):
>> >>   elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for
>> >>     non-DL_EXTERN_PROTECTED_DATA ports
>> >>   Revert "[AArch64][BZ #17711] Fix extern protected data handling"
>> >>   Revert "[ARM][BZ #17711] Fix extern protected data handling"
>> >>
>> >>  elf/dl-lookup.c              | 46 ++++++++++++------------------------
>> >>  sysdeps/aarch64/dl-machine.h | 13 +++++-----
>> >>  sysdeps/aarch64/dl-sysdep.h  |  2 --
>> >>  sysdeps/arm/dl-machine.h     | 10 +++-----
>> >>  sysdeps/arm/dl-sysdep.h      |  2 --
>> >>  5 files changed, 24 insertions(+), 49 deletions(-)
>> >>
>> >> --
>> >> 2.36.0.464.gb9c8b46e94-goog
>> >>
>> >
>> >Given that protected symbols never work properly with copy relocation,
>> >can we change protected symbol handling to simply warn copy relocation
>> >against protected data symbol and non-zero symbol values of undefined
>> >symbols against protected function symbols?
>>
>> "protected symbols never work properly with copy relocation" generally
>> applies to all non-x86 architectures (arm/aarch64 has some guarantee,
>> but not reliable). My goal is indeed to remove relevant code for all
>> non-x86 architectures. They should not even bother testing
>> STV_PROTECTED.
>
>Even on x86, protected symbols don't work as intended.   They don't
>provide any performance benefits.  I think we should move away from
>copy relocation on protected symbols.  We can either issue a warning
>or an error.
>
>> On x86 there is some guarantee. The gcc>=5 -fpie
>> HAVE_LD_PIE_COPYRELOC behavior makes the problem trigger in some cases.
>> Having a warning in x86 specific files (e.g.
>> sysdeps/x86_64/dl-machine.h R_X86_64_COPY) seems appropriate.

How about adding a warning for R_X86_64_COPY like the following, then removing
all code paths of (ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA!=0)?

 From 3f905ff283178396840861a965acf35bd21cf5f0 Mon Sep 17 00:00:00 2001
From: Fangrui Song <maskray@google.com>
Date: Wed, 11 May 2022 21:54:02 -0700
Subject: [PATCH] x86: Warn for copy relocations on a protected symbol

---
  sysdeps/x86_64/dl-machine.h | 6 ++++++
  1 file changed, 6 insertions(+)

diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
index c70af7ab1e..48f08ecc0b 100644
--- a/sysdeps/x86_64/dl-machine.h
+++ b/sysdeps/x86_64/dl-machine.h
@@ -474,6 +474,12 @@ and creates an unsatisfiable circular dependency.\n",
             break;
           memcpy (reloc_addr_arg, (void *) value,
                   MIN (sym->st_size, refsym->st_size));
+         if (ELFW(ST_VISIBILITY) (sym->st_other) == STV_PROTECTED)
+           {
+             fmt = "\
+%s: Protected symbol `%s' is incompatible with copy relocations\n";
+             goto print_err;
+           }
           if (__glibc_unlikely (sym->st_size > refsym->st_size)
               || (__glibc_unlikely (sym->st_size < refsym->st_size)
                   && GLRO(dl_verbose)))

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling
  2022-05-12  4:56       ` Fangrui Song
@ 2022-05-13 22:18         ` H.J. Lu
  2022-05-13 23:09           ` Fangrui Song
  0 siblings, 1 reply; 32+ messages in thread
From: H.J. Lu @ 2022-05-13 22:18 UTC (permalink / raw)
  To: Fangrui Song; +Cc: GNU C Library, Adhemerval Zanella, Szabolcs Nagy

On Wed, May 11, 2022 at 9:56 PM Fangrui Song <maskray@google.com> wrote:
>
> On 2022-05-10, H.J. Lu wrote:
> >On Tue, May 10, 2022 at 12:42 AM Fangrui Song <maskray@google.com> wrote:
> >>
> >> On 2022-05-09, H.J. Lu wrote:
> >> >On Sat, Apr 30, 2022 at 11:06 PM Fangrui Song via Libc-alpha
> >> ><libc-alpha@sourceware.org> wrote:
> >> >>
> >> >> Say both a.so and b.so define protected var and the executable copy
> >> >> relocates var. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange
> >> >> semantics: a.so accesses the copy in the executable while b.so accesses
> >> >> its own. This behavior requires that (a) the compiler emits
> >> >> GOT-generating relocations (b) the linker produces GLOB_DAT instead of
> >> >> RELATIVE.
> >> >>
> >> >> Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
> >> >> will bind to the executable (normal behavior).
> >> >>
> >> >> For aarch64/arm it makes sense to restore the original behavior and don't
> >> >> pay the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA cost. The behavior is very
> >> >> unlikely used by anyone.
> >> >>
> >> >> * Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
> >> >>   no GOT-generating relocation in the first place.
> >> >> * gold and lld reject copy relocation on a STV_PROTECTED symbol.
> >> >> * Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
> >> >>   GOT-generating relocation when accessing an default visibility
> >> >>   external symbol which avoids copy relocation.
> >> >>
> >> >> Fangrui Song (3):
> >> >>   elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for
> >> >>     non-DL_EXTERN_PROTECTED_DATA ports
> >> >>   Revert "[AArch64][BZ #17711] Fix extern protected data handling"
> >> >>   Revert "[ARM][BZ #17711] Fix extern protected data handling"
> >> >>
> >> >>  elf/dl-lookup.c              | 46 ++++++++++++------------------------
> >> >>  sysdeps/aarch64/dl-machine.h | 13 +++++-----
> >> >>  sysdeps/aarch64/dl-sysdep.h  |  2 --
> >> >>  sysdeps/arm/dl-machine.h     | 10 +++-----
> >> >>  sysdeps/arm/dl-sysdep.h      |  2 --
> >> >>  5 files changed, 24 insertions(+), 49 deletions(-)
> >> >>
> >> >> --
> >> >> 2.36.0.464.gb9c8b46e94-goog
> >> >>
> >> >
> >> >Given that protected symbols never work properly with copy relocation,
> >> >can we change protected symbol handling to simply warn copy relocation
> >> >against protected data symbol and non-zero symbol values of undefined
> >> >symbols against protected function symbols?
> >>
> >> "protected symbols never work properly with copy relocation" generally
> >> applies to all non-x86 architectures (arm/aarch64 has some guarantee,
> >> but not reliable). My goal is indeed to remove relevant code for all
> >> non-x86 architectures. They should not even bother testing
> >> STV_PROTECTED.
> >
> >Even on x86, protected symbols don't work as intended.   They don't
> >provide any performance benefits.  I think we should move away from
> >copy relocation on protected symbols.  We can either issue a warning
> >or an error.
> >
> >> On x86 there is some guarantee. The gcc>=5 -fpie
> >> HAVE_LD_PIE_COPYRELOC behavior makes the problem trigger in some cases.
> >> Having a warning in x86 specific files (e.g.
> >> sysdeps/x86_64/dl-machine.h R_X86_64_COPY) seems appropriate.
>
> How about adding a warning for R_X86_64_COPY like the following, then removing
> all code paths of (ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA!=0)?
>
>  From 3f905ff283178396840861a965acf35bd21cf5f0 Mon Sep 17 00:00:00 2001
> From: Fangrui Song <maskray@google.com>
> Date: Wed, 11 May 2022 21:54:02 -0700
> Subject: [PATCH] x86: Warn for copy relocations on a protected symbol
>
> ---
>   sysdeps/x86_64/dl-machine.h | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> index c70af7ab1e..48f08ecc0b 100644
> --- a/sysdeps/x86_64/dl-machine.h
> +++ b/sysdeps/x86_64/dl-machine.h
> @@ -474,6 +474,12 @@ and creates an unsatisfiable circular dependency.\n",
>              break;
>            memcpy (reloc_addr_arg, (void *) value,
>                    MIN (sym->st_size, refsym->st_size));
> +         if (ELFW(ST_VISIBILITY) (sym->st_other) == STV_PROTECTED)
> +           {
> +             fmt = "\
> +%s: Protected symbol `%s' is incompatible with copy relocations\n";
> +             goto print_err;
> +           }
>            if (__glibc_unlikely (sym->st_size > refsym->st_size)
>                || (__glibc_unlikely (sym->st_size < refsym->st_size)
>                    && GLRO(dl_verbose)))

Should it be handled in sysdeps/generic/dl-protected.h?

BTW, I'd like to see a migration plan to remove copy relocation and
use protected
visibility in shared libraries.   Qt uses protected/default
visibilities to mark exported
and imported symbols with GCC 12:

https://github.com/qt/qtbase/commit/19b7f854a274812d9c95fc7aaf134a12530c105f

Should glibc provide some EXPORT and IMPORT macros to help this?

--
H.J.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling
  2022-05-13 22:18         ` H.J. Lu
@ 2022-05-13 23:09           ` Fangrui Song
  0 siblings, 0 replies; 32+ messages in thread
From: Fangrui Song @ 2022-05-13 23:09 UTC (permalink / raw)
  To: H.J. Lu; +Cc: GNU C Library, Adhemerval Zanella, Szabolcs Nagy

On 2022-05-13, H.J. Lu wrote:
>On Wed, May 11, 2022 at 9:56 PM Fangrui Song <maskray@google.com> wrote:
>>
>> On 2022-05-10, H.J. Lu wrote:
>> >On Tue, May 10, 2022 at 12:42 AM Fangrui Song <maskray@google.com> wrote:
>> >>
>> >> On 2022-05-09, H.J. Lu wrote:
>> >> >On Sat, Apr 30, 2022 at 11:06 PM Fangrui Song via Libc-alpha
>> >> ><libc-alpha@sourceware.org> wrote:
>> >> >>
>> >> >> Say both a.so and b.so define protected var and the executable copy
>> >> >> relocates var. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange
>> >> >> semantics: a.so accesses the copy in the executable while b.so accesses
>> >> >> its own. This behavior requires that (a) the compiler emits
>> >> >> GOT-generating relocations (b) the linker produces GLOB_DAT instead of
>> >> >> RELATIVE.
>> >> >>
>> >> >> Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
>> >> >> will bind to the executable (normal behavior).
>> >> >>
>> >> >> For aarch64/arm it makes sense to restore the original behavior and don't
>> >> >> pay the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA cost. The behavior is very
>> >> >> unlikely used by anyone.
>> >> >>
>> >> >> * Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
>> >> >>   no GOT-generating relocation in the first place.
>> >> >> * gold and lld reject copy relocation on a STV_PROTECTED symbol.
>> >> >> * Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
>> >> >>   GOT-generating relocation when accessing an default visibility
>> >> >>   external symbol which avoids copy relocation.
>> >> >>
>> >> >> Fangrui Song (3):
>> >> >>   elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for
>> >> >>     non-DL_EXTERN_PROTECTED_DATA ports
>> >> >>   Revert "[AArch64][BZ #17711] Fix extern protected data handling"
>> >> >>   Revert "[ARM][BZ #17711] Fix extern protected data handling"
>> >> >>
>> >> >>  elf/dl-lookup.c              | 46 ++++++++++++------------------------
>> >> >>  sysdeps/aarch64/dl-machine.h | 13 +++++-----
>> >> >>  sysdeps/aarch64/dl-sysdep.h  |  2 --
>> >> >>  sysdeps/arm/dl-machine.h     | 10 +++-----
>> >> >>  sysdeps/arm/dl-sysdep.h      |  2 --
>> >> >>  5 files changed, 24 insertions(+), 49 deletions(-)
>> >> >>
>> >> >> --
>> >> >> 2.36.0.464.gb9c8b46e94-goog
>> >> >>
>> >> >
>> >> >Given that protected symbols never work properly with copy relocation,
>> >> >can we change protected symbol handling to simply warn copy relocation
>> >> >against protected data symbol and non-zero symbol values of undefined
>> >> >symbols against protected function symbols?
>> >>
>> >> "protected symbols never work properly with copy relocation" generally
>> >> applies to all non-x86 architectures (arm/aarch64 has some guarantee,
>> >> but not reliable). My goal is indeed to remove relevant code for all
>> >> non-x86 architectures. They should not even bother testing
>> >> STV_PROTECTED.
>> >
>> >Even on x86, protected symbols don't work as intended.   They don't
>> >provide any performance benefits.  I think we should move away from
>> >copy relocation on protected symbols.  We can either issue a warning
>> >or an error.
>> >
>> >> On x86 there is some guarantee. The gcc>=5 -fpie
>> >> HAVE_LD_PIE_COPYRELOC behavior makes the problem trigger in some cases.
>> >> Having a warning in x86 specific files (e.g.
>> >> sysdeps/x86_64/dl-machine.h R_X86_64_COPY) seems appropriate.
>>
>> How about adding a warning for R_X86_64_COPY like the following, then removing
>> all code paths of (ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA!=0)?
>>
>>  From 3f905ff283178396840861a965acf35bd21cf5f0 Mon Sep 17 00:00:00 2001
>> From: Fangrui Song <maskray@google.com>
>> Date: Wed, 11 May 2022 21:54:02 -0700
>> Subject: [PATCH] x86: Warn for copy relocations on a protected symbol
>>
>> ---
>>   sysdeps/x86_64/dl-machine.h | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
>> index c70af7ab1e..48f08ecc0b 100644
>> --- a/sysdeps/x86_64/dl-machine.h
>> +++ b/sysdeps/x86_64/dl-machine.h
>> @@ -474,6 +474,12 @@ and creates an unsatisfiable circular dependency.\n",
>>              break;
>>            memcpy (reloc_addr_arg, (void *) value,
>>                    MIN (sym->st_size, refsym->st_size));
>> +         if (ELFW(ST_VISIBILITY) (sym->st_other) == STV_PROTECTED)
>> +           {
>> +             fmt = "\
>> +%s: Protected symbol `%s' is incompatible with copy relocations\n";
>> +             goto print_err;
>> +           }
>>            if (__glibc_unlikely (sym->st_size > refsym->st_size)
>>                || (__glibc_unlikely (sym->st_size < refsym->st_size)
>>                    && GLRO(dl_verbose)))
>
>Should it be handled in sysdeps/generic/dl-protected.h?

Yes, look like the suitable place. I think we should report a warning
regardless of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS, at least for
all non-x86 architectures.

For x86, gating the logic under GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS may be fine
but I think it may be a bit overengineering as well.

>BTW, I'd like to see a migration plan to remove copy relocation and
>use protected
>visibility in shared libraries.   

As mentioned, the elf/dl-lookup.c code is to handle the case when a
protected variable is defined in multiple shared objects and the
executable copy relocates it. This is a pathologic case and does not
have clear semantics.  So IMO we should just remove the
ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code that let the second shared
object access its own copy.

I sent the patch series keeping ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA,
because I did not know whether you want to keep it for x86 :)

Note: the single protected data copy relocated by the executable does
not need any ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code to work. It
simply depends on whether the compiler and the linker cooperate and
produce a GLOB_DAT dynamic relocation.

>Qt uses protected/default
>visibilities to mark exported
>and imported symbols with GCC 12:
>
>https://github.com/qt/qtbase/commit/19b7f854a274812d9c95fc7aaf134a12530c105f

The Qt example belongs to the majority of cases where there is one
single definition for a protected variable.  Removing the dl-lookup.c
STV_PROTECTED code won't affect it.  If Qt ever defines the protected
variable in two shared objects, we can see that it is in a very broken
state.

And the original issue was really related to GCC 5's x86-64 decision to use
HAVE_LD_PIE_COPYRELOC. Other architectures don't have the problem.
In recent years most (if not all) distributions have switched to default
PIE. On all non-x86 architecture supporting shared objects, -fPIE
codegen uses an indirection for an external data symbol.

On the GCC side, we should just fix x86-64 -fpie by removing
HAVE_LD_PIE_COPYRELOC
(https://gcc.gnu.org/pipermail/gcc-patches/2021-November/582987.html).

>Should glibc provide some EXPORT and IMPORT macros to help this?

The -fno-pic codegen incompatibility with -Bsymbolic/protected/some
--dynamic-list has been there since like forever for architectures with
copy relocations. It doesn't help for glibc to tell GCC what to do.

Programs specifying -fno-pic likely rely on the existing behavior.
Changing GCC -fno-pic to default indirect access may be fine. That will
just let them opt out as they currently do for some security hardening
options pushed by Debian/Gentoo/RedHat/etc (e.g. -fno-stack-protector)

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-01  6:06 ` [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling" Fangrui Song
@ 2022-05-23 20:10   ` Szabolcs Nagy
  2022-05-23 20:17     ` Fangrui Song
  0 siblings, 1 reply; 32+ messages in thread
From: Szabolcs Nagy @ 2022-05-23 20:10 UTC (permalink / raw)
  To: Fangrui Song; +Cc: libc-alpha, Adhemerval Zanella

The 04/30/2022 23:06, Fangrui Song wrote:
> This reverts commit 0910702c4d2cf9e8302b35c9519548726e1ac489.
> 
> Say both a.so and b.so define protected var and the executable copy
> relocates var. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange
> semantics: a.so accesses the copy in the executable while b.so accesses
> its own. This behavior requires that (a) the compiler emits
> GOT-generating relocations (b) the linker produces GLOB_DAT instead of
> RELATIVE.
> 
> Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
> will bind to the executable (normal behavior).
> 
> For aarch64 it makes sense to restore the original behavior and don't
> pay the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA cost. The behavior is very
> unlikely used by anyone.
> 
> * Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
>   no GOT-generating relocation in the first place.
> * gold and lld reject copy relocation on a STV_PROTECTED symbol.
> * Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
>   GOT-generating relocation when accessing an default visibility
>   external symbol which avoids copy relocation.

this looks fine. i guess bfd ld should warn/reject copy relocs too
since it wont work well with protected visibility.

Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>

> --- a/sysdeps/aarch64/dl-sysdep.h
> +++ b/sysdeps/aarch64/dl-sysdep.h
> @@ -21,5 +21,3 @@
>  /* _dl_argv cannot be attribute_relro, because _dl_start_user
>     might write into it after _dl_start returns.  */
>  #define DL_ARGV_NOT_RELRO 1
> -
> -#define DL_EXTERN_PROTECTED_DATA

i think this file can be removed after rebase
(DL_ARGV_NOT_RELRO got removed)

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 3/3] Revert "[ARM][BZ #17711] Fix extern protected data handling"
  2022-05-01  6:06 ` [PATCH 3/3] Revert "[ARM][BZ " Fangrui Song
@ 2022-05-23 20:11   ` Szabolcs Nagy
  2022-05-23 20:45     ` Fangrui Song
  0 siblings, 1 reply; 32+ messages in thread
From: Szabolcs Nagy @ 2022-05-23 20:11 UTC (permalink / raw)
  To: Fangrui Song; +Cc: libc-alpha, Adhemerval Zanella

The 04/30/2022 23:06, Fangrui Song wrote:
> This reverts commit 3bcea719ddd6ce399d7bccb492c40af77d216e42.

i'd reference the previous commit here for details.

Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-23 20:10   ` Szabolcs Nagy
@ 2022-05-23 20:17     ` Fangrui Song
  2022-05-24  5:13       ` Fangrui Song
  0 siblings, 1 reply; 32+ messages in thread
From: Fangrui Song @ 2022-05-23 20:17 UTC (permalink / raw)
  To: Szabolcs Nagy; +Cc: libc-alpha, Adhemerval Zanella

On 2022-05-23, Szabolcs Nagy wrote:
>The 04/30/2022 23:06, Fangrui Song wrote:
>> This reverts commit 0910702c4d2cf9e8302b35c9519548726e1ac489.
>>
>> Say both a.so and b.so define protected var and the executable copy
>> relocates var. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange
>> semantics: a.so accesses the copy in the executable while b.so accesses
>> its own. This behavior requires that (a) the compiler emits
>> GOT-generating relocations (b) the linker produces GLOB_DAT instead of
>> RELATIVE.
>>
>> Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
>> will bind to the executable (normal behavior).
>>
>> For aarch64 it makes sense to restore the original behavior and don't
>> pay the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA cost. The behavior is very
>> unlikely used by anyone.
>>
>> * Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
>>   no GOT-generating relocation in the first place.
>> * gold and lld reject copy relocation on a STV_PROTECTED symbol.
>> * Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
>>   GOT-generating relocation when accessing an default visibility
>>   external symbol which avoids copy relocation.
>
>this looks fine. i guess bfd ld should warn/reject copy relocs too
>since it wont work well with protected visibility.
>
>Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>

Thanks for review. I'll check GNU ld's aarch64 port if I get time.

>> --- a/sysdeps/aarch64/dl-sysdep.h
>> +++ b/sysdeps/aarch64/dl-sysdep.h
>> @@ -21,5 +21,3 @@
>>  /* _dl_argv cannot be attribute_relro, because _dl_start_user
>>     might write into it after _dl_start returns.  */
>>  #define DL_ARGV_NOT_RELRO 1
>> -
>> -#define DL_EXTERN_PROTECTED_DATA
>
>i think this file can be removed after rebase
>(DL_ARGV_NOT_RELRO got removed)

Ack. Will remove this file.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 3/3] Revert "[ARM][BZ #17711] Fix extern protected data handling"
  2022-05-23 20:11   ` Szabolcs Nagy
@ 2022-05-23 20:45     ` Fangrui Song
  0 siblings, 0 replies; 32+ messages in thread
From: Fangrui Song @ 2022-05-23 20:45 UTC (permalink / raw)
  To: Szabolcs Nagy; +Cc: libc-alpha, Adhemerval Zanella


On 2022-05-23, Szabolcs Nagy wrote:
>The 04/30/2022 23:06, Fangrui Song wrote:
>> This reverts commit 3bcea719ddd6ce399d7bccb492c40af77d216e42.
>
>i'd reference the previous commit here for details.
>
>Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
>

Thanks:)

Added "Similar to commit e555954e026df1b85b8ef6c101d05f97b1520d7e for aarch64." to the commit message and pushed.
When I sent the patch series I did not have a commit id for this arm patch :)

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-23 20:17     ` Fangrui Song
@ 2022-05-24  5:13       ` Fangrui Song
  0 siblings, 0 replies; 32+ messages in thread
From: Fangrui Song @ 2022-05-24  5:13 UTC (permalink / raw)
  To: Szabolcs Nagy; +Cc: libc-alpha, Adhemerval Zanella

On 2022-05-23, Fangrui Song wrote:
>On 2022-05-23, Szabolcs Nagy wrote:
>>The 04/30/2022 23:06, Fangrui Song wrote:
>>>This reverts commit 0910702c4d2cf9e8302b35c9519548726e1ac489.
>>>
>>>Say both a.so and b.so define protected var and the executable copy
>>>relocates var. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange
>>>semantics: a.so accesses the copy in the executable while b.so accesses
>>>its own. This behavior requires that (a) the compiler emits
>>>GOT-generating relocations (b) the linker produces GLOB_DAT instead of
>>>RELATIVE.
>>>
>>>Without the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA code, b.so's GLOB_DAT
>>>will bind to the executable (normal behavior).
>>>
>>>For aarch64 it makes sense to restore the original behavior and don't
>>>pay the ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA cost. The behavior is very
>>>unlikely used by anyone.
>>>
>>>* Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
>>>  no GOT-generating relocation in the first place.
>>>* gold and lld reject copy relocation on a STV_PROTECTED symbol.
>>>* Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
>>>  GOT-generating relocation when accessing an default visibility
>>>  external symbol which avoids copy relocation.
>>
>>this looks fine. i guess bfd ld should warn/reject copy relocs too
>>since it wont work well with protected visibility.
>>
>>Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
>
>Thanks for review. I'll check GNU ld's aarch64 port if I get time.

I sent a GNU ld patch for review:
https://sourceware.org/pipermail/binutils/2022-May/120970.html
("[PATCH] aarch64: Disallow copy relocations on protected data")

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-31  7:49                   ` Fangrui Song
  2022-05-31  9:42                     ` Wilco Dijkstra
@ 2022-05-31 13:47                     ` H.J. Lu
  1 sibling, 0 replies; 32+ messages in thread
From: H.J. Lu @ 2022-05-31 13:47 UTC (permalink / raw)
  To: Fangrui Song; +Cc: Florian Weimer, Szabolcs Nagy, GNU C Library, Wilco Dijkstra

On Tue, May 31, 2022 at 12:49 AM Fangrui Song <maskray@google.com> wrote:
>
> On 2022-05-30, H.J. Lu via Libc-alpha wrote:
> >On Fri, May 27, 2022 at 5:43 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * Wilco Dijkstra:
> >>
> >> > Hi Florian,
> >> >
> >> > Sure, something basic like this shows the issues:
> >> >
> >> > int x;
> >> > int f(void) { return ++x; }
> >> > int main(void) { return f(); }
> >> >
> >> > compile with -O2 -fPIC -flto -shared:
> >> >
> >> > 00000000000004f0 <f@plt>:
> >> >  4f0:   b0000090        adrp    x16, 11000 <__cxa_finalize@GLIBC_2.17>
> >> >  4f4:   f9400611        ldr     x17, [x16, #8]
> >> >  4f8:   91002210        add     x16, x16, #0x8
> >> >  4fc:   d61f0220        br      x17
> >> >
> >> > 0000000000000510 <main>:
> >> >  510:   17fffff8        b       4f0 <f@plt>
> >> >
> >> > 0000000000000600 <f>:
> >> >  600:   90000081        adrp    x1, 10000 <__FRAME_END__+0xf8f8>
> >> >  604:   f947e821        ldr     x1, [x1, #4048]
> >> >  608:   b9400020        ldr     w0, [x1]
> >> >  60c:   11000400        add     w0, w0, #0x1
> >> >  610:   b9000020        str     w0, [x1]
> >> >  614:   d65f03c0        ret
> >>
> >> Can you link with a version script?
> >>
> >> { local: *; global: main; };
> >>
> >> Exporting symbols inhibits some optimizations even if interposition is
> >> assumed not to happen because the behavior of public entry points needs
> >> to be preserved.  With a version script, the set of entry points can be
> >> greatly reduced, enabling further optimizations.
> >>
> >> Thanks,
> >> Florian
> >>
> >
> >Should -fno-semantic-interposition imply -fvisibility=protected
> >-mno-direct-extern-access?
>
> Unfortunately, no.  -fvisibility=protected imposes stricter requirement
> than -fno-semantic-interposition.  See
> https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#protected-function-symbols-and-canonical-plt-entries
>
>    __attribute__((visibility("protected"))) void *foo() {
>      return (void *)foo;
>    }
>
> GNU ld does not support this on several architectures, including the common x86-32/x86-64/aarch64.
>
> % gcc -m32 -fpic -shared -fuse-ld=bfd b.c
> /usr/bin/ld.bfd: /tmp/cc3Ay0Gh.o: relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
> /usr/bin/ld.bfd: final link failed: bad value
> collect2: error: ld returned 1 exit status

Add -mno-direct-extern-access with binutils 2.39 or 2.38 branch works.

> % aarch64-linux-gnu-gcc -fpic -shared -fuse-ld=bfd b.c
> /usr/lib/gcc-cross/aarch64-linux-gnu/11/../../../../aarch64-linux-gnu/bin/ld.bfd: /tmp/ccJf24eh.o: relocation R_AARCH64_ADR_PREL_PG_HI21 against symbol `foo' which may bind externally can not be used when making a shared object; recompile with -fPIC
> /tmp/ccJf24eh.o: in function `foo':
> b.c:(.text+0x0): dangerous relocation: unsupported relocation
> collect2: error: ld returned 1 exit status
>
>


-- 
H.J.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-31  7:49                   ` Fangrui Song
@ 2022-05-31  9:42                     ` Wilco Dijkstra
  2022-05-31 13:47                     ` H.J. Lu
  1 sibling, 0 replies; 32+ messages in thread
From: Wilco Dijkstra @ 2022-05-31  9:42 UTC (permalink / raw)
  To: Fangrui Song, H.J. Lu; +Cc: Florian Weimer, Szabolcs Nagy, libc-alpha

Hi Fangrui,

>>Should -fno-semantic-interposition imply -fvisibility=protected
>>-mno-direct-extern-access?
>
> Unfortunately, no.  -fvisibility=protected imposes stricter requirement
> than -fno-semantic-interposition.  See
> https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#protected-function-symbols-and-canonical-plt-entries
>
>   __attribute__((visibility("protected"))) void *foo() {
>     return (void *)foo;
>  }
>
> GNU ld does not support this on several architectures, including the common x86-32/x86-64/aarch64.

So you're saying this should only give an error if the executable takes the address of foo
without a GOT indirection? In principle compilers could use a GOT indirection when taking
the address of function symbols to make this work (either in the executable or for protected
function symbols in shared objects).

Note in the executable case, the linker can relax the GOT relocations so it would have almost
zero overhead.

Wilco

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-31  2:03                 ` H.J. Lu
@ 2022-05-31  7:49                   ` Fangrui Song
  2022-05-31  9:42                     ` Wilco Dijkstra
  2022-05-31 13:47                     ` H.J. Lu
  0 siblings, 2 replies; 32+ messages in thread
From: Fangrui Song @ 2022-05-31  7:49 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Florian Weimer, Szabolcs Nagy, libc-alpha, Wilco Dijkstra

On 2022-05-30, H.J. Lu via Libc-alpha wrote:
>On Fri, May 27, 2022 at 5:43 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Wilco Dijkstra:
>>
>> > Hi Florian,
>> >
>> > Sure, something basic like this shows the issues:
>> >
>> > int x;
>> > int f(void) { return ++x; }
>> > int main(void) { return f(); }
>> >
>> > compile with -O2 -fPIC -flto -shared:
>> >
>> > 00000000000004f0 <f@plt>:
>> >  4f0:   b0000090        adrp    x16, 11000 <__cxa_finalize@GLIBC_2.17>
>> >  4f4:   f9400611        ldr     x17, [x16, #8]
>> >  4f8:   91002210        add     x16, x16, #0x8
>> >  4fc:   d61f0220        br      x17
>> >
>> > 0000000000000510 <main>:
>> >  510:   17fffff8        b       4f0 <f@plt>
>> >
>> > 0000000000000600 <f>:
>> >  600:   90000081        adrp    x1, 10000 <__FRAME_END__+0xf8f8>
>> >  604:   f947e821        ldr     x1, [x1, #4048]
>> >  608:   b9400020        ldr     w0, [x1]
>> >  60c:   11000400        add     w0, w0, #0x1
>> >  610:   b9000020        str     w0, [x1]
>> >  614:   d65f03c0        ret
>>
>> Can you link with a version script?
>>
>> { local: *; global: main; };
>>
>> Exporting symbols inhibits some optimizations even if interposition is
>> assumed not to happen because the behavior of public entry points needs
>> to be preserved.  With a version script, the set of entry points can be
>> greatly reduced, enabling further optimizations.
>>
>> Thanks,
>> Florian
>>
>
>Should -fno-semantic-interposition imply -fvisibility=protected
>-mno-direct-extern-access?

Unfortunately, no.  -fvisibility=protected imposes stricter requirement
than -fno-semantic-interposition.  See
https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#protected-function-symbols-and-canonical-plt-entries

   __attribute__((visibility("protected"))) void *foo() {
     return (void *)foo;
   }

GNU ld does not support this on several architectures, including the common x86-32/x86-64/aarch64.

% gcc -m32 -fpic -shared -fuse-ld=bfd b.c
/usr/bin/ld.bfd: /tmp/cc3Ay0Gh.o: relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
/usr/bin/ld.bfd: final link failed: bad value
collect2: error: ld returned 1 exit status
% aarch64-linux-gnu-gcc -fpic -shared -fuse-ld=bfd b.c
/usr/lib/gcc-cross/aarch64-linux-gnu/11/../../../../aarch64-linux-gnu/bin/ld.bfd: /tmp/ccJf24eh.o: relocation R_AARCH64_ADR_PREL_PG_HI21 against symbol `foo' which may bind externally can not be used when making a shared object; recompile with -fPIC
/tmp/ccJf24eh.o: in function `foo':
b.c:(.text+0x0): dangerous relocation: unsupported relocation
collect2: error: ld returned 1 exit status



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-26 20:03             ` Wilco Dijkstra
  2022-05-26 21:27               ` H.J. Lu
  2022-05-27 12:43               ` Florian Weimer
@ 2022-05-31  7:42               ` Fangrui Song
  2 siblings, 0 replies; 32+ messages in thread
From: Fangrui Song @ 2022-05-31  7:42 UTC (permalink / raw)
  To: Wilco Dijkstra
  Cc: Florian Weimer, Szabolcs Nagy, Wilco Dijkstra via Libc-alpha


On 2022-05-26, Wilco Dijkstra via Libc-alpha wrote:
>Hi Florian,
>
>Sure, something basic like this shows the issues:
>
>int x;
>int f(void) { return ++x; }
>int main(void) { return f(); }
>
>compile with -O2 -fPIC -flto -shared:
>
>00000000000004f0 <f@plt>:
> 4f0:   b0000090        adrp    x16, 11000 <__cxa_finalize@GLIBC_2.17>
> 4f4:   f9400611        ldr     x17, [x16, #8]
> 4f8:   91002210        add     x16, x16, #0x8
> 4fc:   d61f0220        br      x17
>
>0000000000000510 <main>:
> 510:   17fffff8        b       4f0 <f@plt>
>
>0000000000000600 <f>:
> 600:   90000081        adrp    x1, 10000 <__FRAME_END__+0xf8f8>
> 604:   f947e821        ldr     x1, [x1, #4048]
> 608:   b9400020        ldr     w0, [x1]
> 60c:   11000400        add     w0, w0, #0x1
> 610:   b9000020        str     w0, [x1]
> 614:   d65f03c0        ret
>
>So f() does not get inlined into main, it is redirected via PLT, and it uses a GOT
>indirection to access the global. The underlying problem is that ELF assumes by
>default that you want interposition/export for all symbols. In reality that is
>almost never needed.
>

>LLVM makes -fno-semantic-interposition the default,
>which solves the PLT and inlining issue, but not the unnecessary GOT indirections.

LLVM/Clang's behavior is more complex, but stating "-fno-semantic-interposition the default" is incorrect.

See https://maskray.me/blog/2021-05-09-fno-semantic-interposition#clang--fno-semantic-interposition

https://fedoraproject.org/wiki/Changes/PythonNoSemanticInterpositionSpeedup#Detailed_Description
used to be incorrect, and I sent them the updates we can see today.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-27 12:43               ` Florian Weimer
@ 2022-05-31  2:03                 ` H.J. Lu
  2022-05-31  7:49                   ` Fangrui Song
  0 siblings, 1 reply; 32+ messages in thread
From: H.J. Lu @ 2022-05-31  2:03 UTC (permalink / raw)
  To: Florian Weimer
  Cc: Wilco Dijkstra, Wilco Dijkstra via Libc-alpha, Szabolcs Nagy

On Fri, May 27, 2022 at 5:43 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Wilco Dijkstra:
>
> > Hi Florian,
> >
> > Sure, something basic like this shows the issues:
> >
> > int x;
> > int f(void) { return ++x; }
> > int main(void) { return f(); }
> >
> > compile with -O2 -fPIC -flto -shared:
> >
> > 00000000000004f0 <f@plt>:
> >  4f0:   b0000090        adrp    x16, 11000 <__cxa_finalize@GLIBC_2.17>
> >  4f4:   f9400611        ldr     x17, [x16, #8]
> >  4f8:   91002210        add     x16, x16, #0x8
> >  4fc:   d61f0220        br      x17
> >
> > 0000000000000510 <main>:
> >  510:   17fffff8        b       4f0 <f@plt>
> >
> > 0000000000000600 <f>:
> >  600:   90000081        adrp    x1, 10000 <__FRAME_END__+0xf8f8>
> >  604:   f947e821        ldr     x1, [x1, #4048]
> >  608:   b9400020        ldr     w0, [x1]
> >  60c:   11000400        add     w0, w0, #0x1
> >  610:   b9000020        str     w0, [x1]
> >  614:   d65f03c0        ret
>
> Can you link with a version script?
>
> { local: *; global: main; };
>
> Exporting symbols inhibits some optimizations even if interposition is
> assumed not to happen because the behavior of public entry points needs
> to be preserved.  With a version script, the set of entry points can be
> greatly reduced, enabling further optimizations.
>
> Thanks,
> Florian
>

Should -fno-semantic-interposition imply -fvisibility=protected
-mno-direct-extern-access?

-- 
H.J.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-26 20:03             ` Wilco Dijkstra
  2022-05-26 21:27               ` H.J. Lu
@ 2022-05-27 12:43               ` Florian Weimer
  2022-05-31  2:03                 ` H.J. Lu
  2022-05-31  7:42               ` Fangrui Song
  2 siblings, 1 reply; 32+ messages in thread
From: Florian Weimer @ 2022-05-27 12:43 UTC (permalink / raw)
  To: Wilco Dijkstra; +Cc: H.J. Lu, Wilco Dijkstra via Libc-alpha, Szabolcs Nagy

* Wilco Dijkstra:

> Hi Florian,
>
> Sure, something basic like this shows the issues:
>
> int x;
> int f(void) { return ++x; }
> int main(void) { return f(); }
>
> compile with -O2 -fPIC -flto -shared:
>
> 00000000000004f0 <f@plt>:
>  4f0:   b0000090        adrp    x16, 11000 <__cxa_finalize@GLIBC_2.17>
>  4f4:   f9400611        ldr     x17, [x16, #8]
>  4f8:   91002210        add     x16, x16, #0x8
>  4fc:   d61f0220        br      x17
>
> 0000000000000510 <main>:
>  510:   17fffff8        b       4f0 <f@plt>
>
> 0000000000000600 <f>:
>  600:   90000081        adrp    x1, 10000 <__FRAME_END__+0xf8f8>
>  604:   f947e821        ldr     x1, [x1, #4048]
>  608:   b9400020        ldr     w0, [x1]
>  60c:   11000400        add     w0, w0, #0x1
>  610:   b9000020        str     w0, [x1]
>  614:   d65f03c0        ret

Can you link with a version script?

{ local: *; global: main; };

Exporting symbols inhibits some optimizations even if interposition is
assumed not to happen because the behavior of public entry points needs
to be preserved.  With a version script, the set of entry points can be
greatly reduced, enabling further optimizations.

Thanks,
Florian


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-26 20:03             ` Wilco Dijkstra
@ 2022-05-26 21:27               ` H.J. Lu
  2022-05-27 12:43               ` Florian Weimer
  2022-05-31  7:42               ` Fangrui Song
  2 siblings, 0 replies; 32+ messages in thread
From: H.J. Lu @ 2022-05-26 21:27 UTC (permalink / raw)
  To: Wilco Dijkstra
  Cc: Florian Weimer, Wilco Dijkstra via Libc-alpha, Szabolcs Nagy

On Thu, May 26, 2022 at 1:03 PM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>
> Hi Florian,
>
> Sure, something basic like this shows the issues:
>
> int x;
> int f(void) { return ++x; }
> int main(void) { return f(); }
>
> compile with -O2 -fPIC -flto -shared:

-fPIC is the issue.   x can be marked as "export" with the
protected visibility.  GCC 12 with

-fvisibility=protected -flto  -mno-direct-extern-access

works on x86-64:

0000000000001040 <main>:
    1040: 8b 05 d6 2f 00 00    mov    0x2fd6(%rip),%eax        # 401c <x>
    1046: 83 c0 01              add    $0x1,%eax
    1049: 89 05 cd 2f 00 00    mov    %eax,0x2fcd(%rip)        # 401c <x>
    104f: c3                    ret

0000000000001110 <f>:
    1110: 8b 05 06 2f 00 00    mov    0x2f06(%rip),%eax        # 401c <x>
    1116: 83 c0 01              add    $0x1,%eax
    1119: 89 05 fd 2e 00 00    mov    %eax,0x2efd(%rip)        # 401c <x>
    111f: c3                    ret

>
> 00000000000004f0 <f@plt>:
>  4f0:   b0000090        adrp    x16, 11000 <__cxa_finalize@GLIBC_2.17>
>  4f4:   f9400611        ldr     x17, [x16, #8]
>  4f8:   91002210        add     x16, x16, #0x8
>  4fc:   d61f0220        br      x17
>
> 0000000000000510 <main>:
>  510:   17fffff8        b       4f0 <f@plt>
>
> 0000000000000600 <f>:
>  600:   90000081        adrp    x1, 10000 <__FRAME_END__+0xf8f8>
>  604:   f947e821        ldr     x1, [x1, #4048]
>  608:   b9400020        ldr     w0, [x1]
>  60c:   11000400        add     w0, w0, #0x1
>  610:   b9000020        str     w0, [x1]
>  614:   d65f03c0        ret
>
> So f() does not get inlined into main, it is redirected via PLT, and it uses a GOT
> indirection to access the global. The underlying problem is that ELF assumes by
> default that you want interposition/export for all symbols. In reality that is
> almost never needed. LLVM makes -fno-semantic-interposition the default,
> which solves the PLT and inlining issue, but not the unnecessary GOT indirections.
>
> Cheers,
> Wilco



-- 
H.J.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-26 19:25           ` Florian Weimer
@ 2022-05-26 20:03             ` Wilco Dijkstra
  2022-05-26 21:27               ` H.J. Lu
                                 ` (2 more replies)
  0 siblings, 3 replies; 32+ messages in thread
From: Wilco Dijkstra @ 2022-05-26 20:03 UTC (permalink / raw)
  To: Florian Weimer; +Cc: H.J. Lu, Wilco Dijkstra via Libc-alpha, Szabolcs Nagy

Hi Florian,

Sure, something basic like this shows the issues:

int x;
int f(void) { return ++x; }
int main(void) { return f(); }

compile with -O2 -fPIC -flto -shared:

00000000000004f0 <f@plt>:
 4f0:   b0000090        adrp    x16, 11000 <__cxa_finalize@GLIBC_2.17>
 4f4:   f9400611        ldr     x17, [x16, #8]
 4f8:   91002210        add     x16, x16, #0x8
 4fc:   d61f0220        br      x17

0000000000000510 <main>:
 510:   17fffff8        b       4f0 <f@plt>

0000000000000600 <f>:
 600:   90000081        adrp    x1, 10000 <__FRAME_END__+0xf8f8>
 604:   f947e821        ldr     x1, [x1, #4048]
 608:   b9400020        ldr     w0, [x1]
 60c:   11000400        add     w0, w0, #0x1
 610:   b9000020        str     w0, [x1]
 614:   d65f03c0        ret

So f() does not get inlined into main, it is redirected via PLT, and it uses a GOT
indirection to access the global. The underlying problem is that ELF assumes by
default that you want interposition/export for all symbols. In reality that is
almost never needed. LLVM makes -fno-semantic-interposition the default,
which solves the PLT and inlining issue, but not the unnecessary GOT indirections.

Cheers,
Wilco

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-26 19:17         ` Wilco Dijkstra
@ 2022-05-26 19:25           ` Florian Weimer
  2022-05-26 20:03             ` Wilco Dijkstra
  0 siblings, 1 reply; 32+ messages in thread
From: Florian Weimer @ 2022-05-26 19:25 UTC (permalink / raw)
  To: Wilco Dijkstra; +Cc: H.J. Lu, Wilco Dijkstra via Libc-alpha, Szabolcs Nagy

* Wilco Dijkstra:

> Hi,
>
>>> What about -flto (with a linker plugin)?
>>
>>
>> LTO certainly works.  An undefined symbol will be imported from a shared
>> library.
>
> LTO allows you to determine which symbols are imported indeed, so the
> compiler can use a GOT indirection without requiring explicit import
> annotations.
>
> However LTO does not solve all the code quality issues of FPIC - there
> is still no inlining, internal calls indirect via the PLT and
> non-static globals use a GOT indirection.

Could you share a small example that exhibits these shortcomings?

Thanks,
Florian


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-25 20:44       ` H.J. Lu
@ 2022-05-26 19:17         ` Wilco Dijkstra
  2022-05-26 19:25           ` Florian Weimer
  0 siblings, 1 reply; 32+ messages in thread
From: Wilco Dijkstra @ 2022-05-26 19:17 UTC (permalink / raw)
  To: H.J. Lu, Florian Weimer; +Cc: Wilco Dijkstra via Libc-alpha, Szabolcs Nagy

Hi,

>> What about -flto (with a linker plugin)?
>
>
> LTO certainly works.  An undefined symbol will be imported from a shared
> library.

LTO allows you to determine which symbols are imported indeed, so the
compiler can use a GOT indirection without requiring explicit import
annotations.

However LTO does not solve all the code quality issues of FPIC - there is still no inlining, internal calls indirect via the PLT and non-static globals use a GOT
indirection. The goal here is to have a new option that generates efficient code
by default that just works for most projects.

Cheers,
Wilco

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-25 18:21     ` Florian Weimer
@ 2022-05-25 20:44       ` H.J. Lu
  2022-05-26 19:17         ` Wilco Dijkstra
  0 siblings, 1 reply; 32+ messages in thread
From: H.J. Lu @ 2022-05-25 20:44 UTC (permalink / raw)
  To: Florian Weimer
  Cc: Wilco Dijkstra via Libc-alpha, Wilco Dijkstra, Szabolcs Nagy

On Wed, May 25, 2022 at 11:21 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Wilco Dijkstra via Libc-alpha:
>
> > Hi H.J.,
> >
> >> All imported symbols can be marked with the default visibility and all
> >> exported symbols, in both executables and shared libraries, can be
> >> marked with the protected visibility.   These require code changes.
> >
> > I meant doing this automatically using an option so most code requires no
> > source changes. If commonly used libraries mark their exported symbols,

"export" describes what happens to a symbol at the library build time.  An
exported symbol can be imported to a library user.   Without LTO, compilers
can't tell if an external symbol will be imported or exported.

> > most code (PIC, PIE and non-PIE) could be compiled using this option and
> > produce efficient code without copy relocations and only using GOT
> > indirections when needed (ie. accessing an exported symbol in another .so).
> > It would also imply -fno-semantic-interposition for non-exported symbols.
> > Currently there is no way to achieve this using options (eg. -fvisibility only
> > affects definitions), and LLVM and GCC disagree on many details.
>
> What about -flto (with a linker plugin)?
>

LTO certainly works.  An undefined symbol will be imported from a shared
library.


-- 
H.J.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-25 17:13   ` Wilco Dijkstra
  2022-05-25 18:21     ` Florian Weimer
@ 2022-05-25 20:10     ` maskray
  1 sibling, 0 replies; 32+ messages in thread
From: maskray @ 2022-05-25 20:10 UTC (permalink / raw)
  To: Wilco Dijkstra; +Cc: H.J. Lu, Szabolcs Nagy, GNU C Library

On 2022-05-25, Wilco Dijkstra wrote:
>Hi H.J.,
>
>> All imported symbols can be marked with the default visibility and all
>> exported symbols, in both executables and shared libraries, can be
>> marked with the protected visibility.   These require code changes.
>
>I meant doing this automatically using an option so most code requires no
>source changes. If commonly used libraries mark their exported symbols,
>most code (PIC, PIE and non-PIE) could be compiled using this option and
>produce efficient code without copy relocations and only using GOT
>indirections when needed (ie. accessing an exported symbol in another .so).
>It would also imply -fno-semantic-interposition for non-exported symbols.
>Currently there is no way to achieve this using options (eg. -fvisibility only
>affects definitions), and LLVM and GCC disagree on many details.
>
>Cheers,
>Wilco

Working out-of-the-box with -fpic and -fpie is trivial.
Working out-of-the-box with -fno-pic is not, as some code may rely on
the traditional codegen behavior that absolute resolutions are used.
I think the situation is worse on x86-32.

For protected data symbols, it's not a problem:

(a) Exported data symbols from shared object are rare. This is discouraged
as exported data symbols is part of ABI and makes DSO upgrading
difficult.

(b) Making exported data symbols exported is even rarer.

(c) In addition, many aarch64 OSes (Android, Chrome OS, FreeBSD, some
musl+llvm based Linux distributions, etc) use lld which does not allow
copy relocation on protected data symbols.
I haven't heard they have found any breakage due to the lld linker error.

(d) -fno-pic is out of favor now with the adoption of default -fpie.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-25 17:13   ` Wilco Dijkstra
@ 2022-05-25 18:21     ` Florian Weimer
  2022-05-25 20:44       ` H.J. Lu
  2022-05-25 20:10     ` maskray
  1 sibling, 1 reply; 32+ messages in thread
From: Florian Weimer @ 2022-05-25 18:21 UTC (permalink / raw)
  To: Wilco Dijkstra via Libc-alpha; +Cc: H.J. Lu, Wilco Dijkstra, Szabolcs Nagy

* Wilco Dijkstra via Libc-alpha:

> Hi H.J.,
>
>> All imported symbols can be marked with the default visibility and all
>> exported symbols, in both executables and shared libraries, can be
>> marked with the protected visibility.   These require code changes.
>
> I meant doing this automatically using an option so most code requires no
> source changes. If commonly used libraries mark their exported symbols,
> most code (PIC, PIE and non-PIE) could be compiled using this option and
> produce efficient code without copy relocations and only using GOT
> indirections when needed (ie. accessing an exported symbol in another .so). 
> It would also imply -fno-semantic-interposition for non-exported symbols.
> Currently there is no way to achieve this using options (eg. -fvisibility only
> affects definitions), and LLVM and GCC disagree on many details.

What about -flto (with a linker plugin)?

Thanks,
Florian


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-24 21:58 ` H.J. Lu
@ 2022-05-25 17:13   ` Wilco Dijkstra
  2022-05-25 18:21     ` Florian Weimer
  2022-05-25 20:10     ` maskray
  0 siblings, 2 replies; 32+ messages in thread
From: Wilco Dijkstra @ 2022-05-25 17:13 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Szabolcs Nagy, maskray, GNU C Library

Hi H.J.,

> All imported symbols can be marked with the default visibility and all
> exported symbols, in both executables and shared libraries, can be
> marked with the protected visibility.   These require code changes.

I meant doing this automatically using an option so most code requires no
source changes. If commonly used libraries mark their exported symbols,
most code (PIC, PIE and non-PIE) could be compiled using this option and
produce efficient code without copy relocations and only using GOT
indirections when needed (ie. accessing an exported symbol in another .so). 
It would also imply -fno-semantic-interposition for non-exported symbols.
Currently there is no way to achieve this using options (eg. -fvisibility only
affects definitions), and LLVM and GCC disagree on many details.

Cheers,
Wilco

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-24 13:46 [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling" Wilco Dijkstra
  2022-05-24 17:28 ` maskray
@ 2022-05-24 21:58 ` H.J. Lu
  2022-05-25 17:13   ` Wilco Dijkstra
  1 sibling, 1 reply; 32+ messages in thread
From: H.J. Lu @ 2022-05-24 21:58 UTC (permalink / raw)
  To: Wilco Dijkstra; +Cc: Szabolcs Nagy, maskray, GNU C Library

On Tue, May 24, 2022 at 6:47 AM Wilco Dijkstra via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> Hi,
>
> >> * Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
> >>   no GOT-generating relocation in the first place.
>
> We should change GCC's behaviour to match this - is this something that
> applies to all targets?
>
> >> * gold and lld reject copy relocation on a STV_PROTECTED symbol.
> >> * Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
> >>   GOT-generating relocation when accessing an default visibility
> >>   external symbol which avoids copy relocation.
>
> Would it be reasonable to add a way to override settings for binaries?
> For example if all imported symbols are marked with the correct visibility,
> PIE binaries could avoid using GOT for default visibility external symbols to
> get better performance. And non-PIE binaries could force GOT accesses for
> non-default visibility to avoid copy relocations and support protected visibility.

All imported symbols can be marked with the default visibility and all
exported symbols, in both executables and shared libraries, can be
marked with the protected visibility.   These require code changes.

-- 
H.J.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
  2022-05-24 13:46 [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling" Wilco Dijkstra
@ 2022-05-24 17:28 ` maskray
  2022-05-24 21:58 ` H.J. Lu
  1 sibling, 0 replies; 32+ messages in thread
From: maskray @ 2022-05-24 17:28 UTC (permalink / raw)
  To: Wilco Dijkstra; +Cc: Szabolcs Nagy, 'GNU C Library'

On 2022-05-24, Wilco Dijkstra wrote:
>Hi,

Hi Wilco,

>>> * Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
>>>   no GOT-generating relocation in the first place.
>
>We should change GCC's behaviour to match this - is this something that
>applies to all targets?

I have a blog post on the topic about copy relocations and protected
symbols:
https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected

"The GCC pessimization applies to all ports with #define TARGET_BINDS_LOCAL_P default_binds_local_p_2."

The aarch64 pessimization was due to the side affect of a commit
attempting to fix another issue: commit
cbddf64c0243816b45e6680754a251c603245dbc (From-SVN: r222992)

>>> * gold and lld reject copy relocation on a STV_PROTECTED symbol.
>>> * Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
>>>   GOT-generating relocation when accessing an default visibility
>>>   external symbol which avoids copy relocation.
>
>Would it be reasonable to add a way to override settings for binaries?
>For example if all imported symbols are marked with the correct visibility,
>PIE binaries could avoid using GOT for default visibility external symbols to
>get better performance. And non-PIE binaries could force GOT accesses for
>non-default visibility to avoid copy relocations and support protected visibility.

In Clang, -fno-direct-access-external-data is such an option:)

---

Regarding function symbols, there is a bit which should be fixed:
"Protected function symbols and canonical PLT entries"

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling"
@ 2022-05-24 13:46 Wilco Dijkstra
  2022-05-24 17:28 ` maskray
  2022-05-24 21:58 ` H.J. Lu
  0 siblings, 2 replies; 32+ messages in thread
From: Wilco Dijkstra @ 2022-05-24 13:46 UTC (permalink / raw)
  To: Szabolcs Nagy, maskray; +Cc: 'GNU C Library'

Hi, 

>> * Clang code generation treats STV_PROTECTED the same way as STV_HIDDEN:
>>   no GOT-generating relocation in the first place.

We should change GCC's behaviour to match this - is this something that
applies to all targets?

>> * gold and lld reject copy relocation on a STV_PROTECTED symbol.
>> * Nowadays -fpie/-fpic modes are popular. GCC/Clang's codegen uses
>>   GOT-generating relocation when accessing an default visibility
>>   external symbol which avoids copy relocation.

Would it be reasonable to add a way to override settings for binaries?
For example if all imported symbols are marked with the correct visibility,
PIE binaries could avoid using GOT for default visibility external symbols to
get better performance. And non-PIE binaries could force GOT accesses for
non-default visibility to avoid copy relocations and support protected visibility.

Cheers,
Wilco

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2022-05-31 13:48 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-01  6:06 [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling Fangrui Song
2022-05-01  6:06 ` [PATCH 1/3] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA check for non-DL_EXTERN_PROTECTED_DATA ports Fangrui Song
2022-05-01  6:06 ` [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling" Fangrui Song
2022-05-23 20:10   ` Szabolcs Nagy
2022-05-23 20:17     ` Fangrui Song
2022-05-24  5:13       ` Fangrui Song
2022-05-01  6:06 ` [PATCH 3/3] Revert "[ARM][BZ " Fangrui Song
2022-05-23 20:11   ` Szabolcs Nagy
2022-05-23 20:45     ` Fangrui Song
2022-05-09 21:26 ` [PATCH 0/3] Simplify ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA and revert aarch64/arm's extern protected data handling H.J. Lu
2022-05-10  7:42   ` Fangrui Song
2022-05-10 15:02     ` H.J. Lu
2022-05-12  4:56       ` Fangrui Song
2022-05-13 22:18         ` H.J. Lu
2022-05-13 23:09           ` Fangrui Song
2022-05-24 13:46 [PATCH 2/3] Revert "[AArch64][BZ #17711] Fix extern protected data handling" Wilco Dijkstra
2022-05-24 17:28 ` maskray
2022-05-24 21:58 ` H.J. Lu
2022-05-25 17:13   ` Wilco Dijkstra
2022-05-25 18:21     ` Florian Weimer
2022-05-25 20:44       ` H.J. Lu
2022-05-26 19:17         ` Wilco Dijkstra
2022-05-26 19:25           ` Florian Weimer
2022-05-26 20:03             ` Wilco Dijkstra
2022-05-26 21:27               ` H.J. Lu
2022-05-27 12:43               ` Florian Weimer
2022-05-31  2:03                 ` H.J. Lu
2022-05-31  7:49                   ` Fangrui Song
2022-05-31  9:42                     ` Wilco Dijkstra
2022-05-31 13:47                     ` H.J. Lu
2022-05-31  7:42               ` Fangrui Song
2022-05-25 20:10     ` maskray

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).