aarch64: Handle fewer relocations for RTLD_BOOTSTRAP

Message ID 20220606043709.3065173-1-maskray@google.com
State Committed
Commit e89913d0aa36597e5824baec870dfcec525fab1a
Headers
Series aarch64: Handle fewer relocations for RTLD_BOOTSTRAP |

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

Commit Message

Fangrui Song June 6, 2022, 4:37 a.m. UTC
  The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.

Tested on aarch64-linux-gnu.
---
 sysdeps/aarch64/dl-machine.h | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)
  

Comments

Fangrui Song June 11, 2022, 2:32 a.m. UTC | #1
On 2022-06-05, Fangrui Song wrote:
>The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
>needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.
>
>Tested on aarch64-linux-gnu.
>---
> sysdeps/aarch64/dl-machine.h | 33 +++++++++++++++------------------
> 1 file changed, 15 insertions(+), 18 deletions(-)
>
>diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
>index 292abe5152..ae8b14425a 100644
>--- a/sysdeps/aarch64/dl-machine.h
>+++ b/sysdeps/aarch64/dl-machine.h
>@@ -178,7 +178,9 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>       return;
>   else
>     {
>+# ifndef RTLD_BOOTSTRAP
>       const ElfW(Sym) *const refsym = sym;
>+# endif
>       struct link_map *sym_map = RESOLVE_MAP (map, scope, &sym, version,
> 					      r_type);
>       ElfW(Addr) value = SYMBOL_ADDRESS (sym_map, sym, true);
>@@ -191,6 +193,18 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
>
>       switch (r_type)
> 	{
>+	case AARCH64_R(GLOB_DAT):
>+	case AARCH64_R(JUMP_SLOT):
>+	  *reloc_addr = value + reloc->r_addend;
>+	  break;
>+
>+# ifndef RTLD_BOOTSTRAP
>+	case AARCH64_R(ABS32):
>+#  ifdef __LP64__
>+	case AARCH64_R(ABS64):
>+#  endif
>+	  *reloc_addr = value + reloc->r_addend;
>+	  break;
> 	case AARCH64_R(COPY):
> 	  if (sym == NULL)
> 	      break;
>@@ -210,30 +224,17 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
> 		  ? sym->st_size : refsym->st_size);
> 	  break;
>
>-	case AARCH64_R(RELATIVE):
>-	case AARCH64_R(GLOB_DAT):
>-	case AARCH64_R(JUMP_SLOT):
>-	case AARCH64_R(ABS32):
>-#ifdef __LP64__
>-	case AARCH64_R(ABS64):
>-#endif
>-	  *reloc_addr = value + reloc->r_addend;
>-	  break;
>-
> 	case AARCH64_R(TLSDESC):
> 	  {
> 	    struct tlsdesc volatile *td =
> 	      (struct tlsdesc volatile *)reloc_addr;
>-#ifndef RTLD_BOOTSTRAP
> 	    if (! sym)
> 	      {
> 		td->arg = (void*)reloc->r_addend;
> 		td->entry = _dl_tlsdesc_undefweak;
> 	      }
> 	    else
>-#endif
> 	      {
>-#ifndef RTLD_BOOTSTRAP
> # ifndef SHARED
> 		CHECK_STATIC_TLS (map, sym_map);
> # else
>@@ -245,7 +246,6 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
> 		  }
> 		else
> # endif
>-#endif
> 		  {
> 		    td->arg = (void*)(sym->st_value + sym_map->l_tls_offset
> 				      + reloc->r_addend);
>@@ -256,14 +256,10 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
> 	  }
>
> 	case AARCH64_R(TLS_DTPMOD):
>-#ifdef RTLD_BOOTSTRAP
>-	  *reloc_addr = 1;
>-#else
> 	  if (sym_map != NULL)
> 	    {
> 	      *reloc_addr = sym_map->l_tls_modid;
> 	    }
>-#endif
> 	  break;
>
> 	case AARCH64_R(TLS_DTPREL):
>@@ -286,6 +282,7 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
> 	    value = elf_ifunc_invoke (value);
> 	  *reloc_addr = value;
> 	  break;
>+# endif /* !RTLD_BOOTSTRAP */
>
> 	default:
> 	  _dl_reloc_bad_type (map, r_type, 0);
>-- 
>2.36.1.255.ge46751e96f-goog

Cc machine maintainers
  
Szabolcs Nagy June 13, 2022, 8:33 a.m. UTC | #2
The 06/10/2022 19:32, Fangrui Song wrote:
> On 2022-06-05, Fangrui Song wrote:
> > The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
> > needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.

the motivation is not clear to me.

is there an issue with other relocations or is this just a cleanup patch?
  
Fangrui Song June 14, 2022, 12:02 a.m. UTC | #3
On 2022-06-13, Szabolcs Nagy wrote:
>The 06/10/2022 19:32, Fangrui Song wrote:
>> On 2022-06-05, Fangrui Song wrote:
>> > The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
>> > needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.
>
>the motivation is not clear to me.
>
>is there an issue with other relocations or is this just a cleanup patch?

It is a cleanup: remove dead code paths.
  
Florian Weimer June 14, 2022, 5:07 a.m. UTC | #4
* Fangrui Song via Libc-alpha:

> On 2022-06-13, Szabolcs Nagy wrote:
>>The 06/10/2022 19:32, Fangrui Song wrote:
>>> On 2022-06-05, Fangrui Song wrote:
>>> > The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
>>> > needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.
>>
>>the motivation is not clear to me.
>>
>>is there an issue with other relocations or is this just a cleanup patch?
>
> It is a cleanup: remove dead code paths.

I think it could be paired (later) with a test that checks that ld.so
uses just the relocation types implemented for self-relocation.

I do not think it's possible in general to use the same code for
self-relocation and subsequent library loading, especially on
HIDDEN_VAR_NEEDS_DYNAMIC_RELOC architectures (I like the new name, by
the way).

Thanks,
Florian
  
Szabolcs Nagy June 14, 2022, 7:49 a.m. UTC | #5
The 06/14/2022 07:07, Florian Weimer wrote:
> * Fangrui Song via Libc-alpha:
> > On 2022-06-13, Szabolcs Nagy wrote:
> >>The 06/10/2022 19:32, Fangrui Song wrote:
> >>> On 2022-06-05, Fangrui Song wrote:
> >>> > The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
> >>> > needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.
> >>
> >>the motivation is not clear to me.
> >>
> >>is there an issue with other relocations or is this just a cleanup patch?
> >
> > It is a cleanup: remove dead code paths.
> 
> I think it could be paired (later) with a test that checks that ld.so
> uses just the relocation types implemented for self-relocation.
> 
> I do not think it's possible in general to use the same code for
> self-relocation and subsequent library loading, especially on
> HIDDEN_VAR_NEEDS_DYNAMIC_RELOC architectures (I like the new name, by
> the way).

i see. cleanup is fine.

note that there is static-pie too, which requires a 3rd
way to do relocations (and ideally it would be split to
separate RELATIVE and IRELATIVE reloc processing given
that ifunc requires complex setup like dynamic allocation
for tunables handling. the split is also needed if we want
to support static-pie with HIDDEN_VAR_NEEDS_DYNAMIC_RELOC).
  
Fangrui Song June 14, 2022, 8:19 a.m. UTC | #6
On 2022-06-14, Szabolcs Nagy wrote:
>The 06/14/2022 07:07, Florian Weimer wrote:
>> * Fangrui Song via Libc-alpha:
>> > On 2022-06-13, Szabolcs Nagy wrote:
>> >>The 06/10/2022 19:32, Fangrui Song wrote:
>> >>> On 2022-06-05, Fangrui Song wrote:
>> >>> > The RTLD_BOOTSTRAP branch is used to relocate ld.so itself.  It only
>> >>> > needs to handle RELATIVE, GLOB_DAT, and JUMP_SLOT.
>> >>
>> >>the motivation is not clear to me.
>> >>
>> >>is there an issue with other relocations or is this just a cleanup patch?
>> >
>> > It is a cleanup: remove dead code paths.
>>
>> I think it could be paired (later) with a test that checks that ld.so
>> uses just the relocation types implemented for self-relocation.
>>
>> I do not think it's possible in general to use the same code for
>> self-relocation and subsequent library loading, especially on
>> HIDDEN_VAR_NEEDS_DYNAMIC_RELOC architectures (I like the new name, by
>> the way).
>
>i see. cleanup is fine.
>
>note that there is static-pie too, which requires a 3rd
>way to do relocations (and ideally it would be split to
>separate RELATIVE and IRELATIVE reloc processing given
>that ifunc requires complex setup like dynamic allocation
>for tunables handling. the split is also needed if we want
>to support static-pie with HIDDEN_VAR_NEEDS_DYNAMIC_RELOC).

dl-reloc-static-pie.c uses the !RTLD_BOOTSTRAP branch, like dl-reloc.c.
I have tested that --enable-static-pie is correct.

I agree that it will be nice to have a readelf -rW test later.
For most ports, .rela?.plt and .rela?.dyn should only contain
RELATIVE|JU?MP_SLOT|GLOB_DAT.
There are a few less popular ports that I am unclear, though.
  

Patch

diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
index 292abe5152..ae8b14425a 100644
--- a/sysdeps/aarch64/dl-machine.h
+++ b/sysdeps/aarch64/dl-machine.h
@@ -178,7 +178,9 @@  elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
       return;
   else
     {
+# ifndef RTLD_BOOTSTRAP
       const ElfW(Sym) *const refsym = sym;
+# endif
       struct link_map *sym_map = RESOLVE_MAP (map, scope, &sym, version,
 					      r_type);
       ElfW(Addr) value = SYMBOL_ADDRESS (sym_map, sym, true);
@@ -191,6 +193,18 @@  elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
 
       switch (r_type)
 	{
+	case AARCH64_R(GLOB_DAT):
+	case AARCH64_R(JUMP_SLOT):
+	  *reloc_addr = value + reloc->r_addend;
+	  break;
+
+# ifndef RTLD_BOOTSTRAP
+	case AARCH64_R(ABS32):
+#  ifdef __LP64__
+	case AARCH64_R(ABS64):
+#  endif
+	  *reloc_addr = value + reloc->r_addend;
+	  break;
 	case AARCH64_R(COPY):
 	  if (sym == NULL)
 	      break;
@@ -210,30 +224,17 @@  elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
 		  ? sym->st_size : refsym->st_size);
 	  break;
 
-	case AARCH64_R(RELATIVE):
-	case AARCH64_R(GLOB_DAT):
-	case AARCH64_R(JUMP_SLOT):
-	case AARCH64_R(ABS32):
-#ifdef __LP64__
-	case AARCH64_R(ABS64):
-#endif
-	  *reloc_addr = value + reloc->r_addend;
-	  break;
-
 	case AARCH64_R(TLSDESC):
 	  {
 	    struct tlsdesc volatile *td =
 	      (struct tlsdesc volatile *)reloc_addr;
-#ifndef RTLD_BOOTSTRAP
 	    if (! sym)
 	      {
 		td->arg = (void*)reloc->r_addend;
 		td->entry = _dl_tlsdesc_undefweak;
 	      }
 	    else
-#endif
 	      {
-#ifndef RTLD_BOOTSTRAP
 # ifndef SHARED
 		CHECK_STATIC_TLS (map, sym_map);
 # else
@@ -245,7 +246,6 @@  elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
 		  }
 		else
 # endif
-#endif
 		  {
 		    td->arg = (void*)(sym->st_value + sym_map->l_tls_offset
 				      + reloc->r_addend);
@@ -256,14 +256,10 @@  elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
 	  }
 
 	case AARCH64_R(TLS_DTPMOD):
-#ifdef RTLD_BOOTSTRAP
-	  *reloc_addr = 1;
-#else
 	  if (sym_map != NULL)
 	    {
 	      *reloc_addr = sym_map->l_tls_modid;
 	    }
-#endif
 	  break;
 
 	case AARCH64_R(TLS_DTPREL):
@@ -286,6 +282,7 @@  elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
 	    value = elf_ifunc_invoke (value);
 	  *reloc_addr = value;
 	  break;
+# endif /* !RTLD_BOOTSTRAP */
 
 	default:
 	  _dl_reloc_bad_type (map, r_type, 0);