PR25384, PowerPC64 ELFv1 copy relocs against function symbols

Message ID 20200115024252.GW4433@bubble.grove.modra.org
State New
Headers show
Series
  • PR25384, PowerPC64 ELFv1 copy relocs against function symbols
Related show

Commit Message

Alan Modra Jan. 15, 2020, 2:42 a.m.
Function symbols of course don't normally want .dynbss copies but
with some old versions of gcc they are needed to copy the function
descriptor.  This patch restricts the cases where they are useful to
compilers using dot-symbols, and enables the warning regardless of
whether a PLT entry is emitted in the executable.  PLTs in shared
libraries are affected by a .dynbss copy in the executable.

bfd/
	PR 25384
	* elf64-ppc.c (ELIMINATE_COPY_RELOCS): Update comment.
	(ppc64_elf_adjust_dynamic_symbol): Don't allow .dynbss copies
	of function symbols unless dot symbols are present.  Do warn
	whenever one is created, regardles of whether a PLT entry is
	also emitted for the function symbol.
ld/
	* testsuite/ld-powerpc/ambiguousv1b.d: Adjust expected output.
	* testsuite/ld-powerpc/funref.s: Align func_tab.
	* testsuite/ld-powerpc/funref2.s: Likewise.
	* testsuite/ld-powerpc/funv1.s: Add dot symbols.


-- 
Alan Modra
Australia Development Lab, IBM

Comments

Carlos O'Donell Jan. 15, 2020, 1:17 p.m. | #1
On Tue, Jan 14, 2020 at 9:49 PM Alan Modra <amodra@gmail.com> wrote:
>

> Function symbols of course don't normally want .dynbss copies but

> with some old versions of gcc they are needed to copy the function

> descriptor.  This patch restricts the cases where they are useful to

> compilers using dot-symbols, and enables the warning regardless of

> whether a PLT entry is emitted in the executable.  PLTs in shared

> libraries are affected by a .dynbss copy in the executable.


Thanks for expanding the fix for this.

This patch is certainly an improvement over what we have today.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>


> bfd/

>         PR 25384

>         * elf64-ppc.c (ELIMINATE_COPY_RELOCS): Update comment.

>         (ppc64_elf_adjust_dynamic_symbol): Don't allow .dynbss copies

>         of function symbols unless dot symbols are present.  Do warn

>         whenever one is created, regardles of whether a PLT entry is

>         also emitted for the function symbol.

> ld/

>         * testsuite/ld-powerpc/ambiguousv1b.d: Adjust expected output.

>         * testsuite/ld-powerpc/funref.s: Align func_tab.

>         * testsuite/ld-powerpc/funref2.s: Likewise.

>         * testsuite/ld-powerpc/funv1.s: Add dot symbols.

>

> diff --git a/bfd/elf64-ppc.c b/bfd/elf64-ppc.c

> index a451c4753a..8319a21940 100644

> --- a/bfd/elf64-ppc.c

> +++ b/bfd/elf64-ppc.c

> @@ -2788,20 +2788,20 @@ must_be_dyn_reloc (struct bfd_link_info *info,

>  }

>

>  /* If ELIMINATE_COPY_RELOCS is non-zero, the linker will try to avoid

> -   copying dynamic variables from a shared lib into an app's dynbss

> +   copying dynamic variables from a shared lib into an app's .dynbss

>     section, and instead use a dynamic relocation to point into the

> -   shared lib.  With code that gcc generates, it's vital that this be

> -   enabled;  In the PowerPC64 ABI, the address of a function is actually

> -   the address of a function descriptor, which resides in the .opd

> -   section.  gcc uses the descriptor directly rather than going via the

> -   GOT as some other ABI's do, which means that initialized function

> -   pointers must reference the descriptor.  Thus, a function pointer

> -   initialized to the address of a function in a shared library will

> -   either require a copy reloc, or a dynamic reloc.  Using a copy reloc

> -   redefines the function descriptor symbol to point to the copy.  This

> -   presents a problem as a plt entry for that function is also

> -   initialized from the function descriptor symbol and the copy reloc

> -   may not be initialized first.  */

> +   shared lib.  With code that gcc generates it is vital that this be

> +   enabled;  In the PowerPC64 ELFv1 ABI the address of a function is

> +   actually the address of a function descriptor which resides in the

> +   .opd section.  gcc uses the descriptor directly rather than going

> +   via the GOT as some other ABIs do, which means that initialized

> +   function pointers reference the descriptor.  Thus, a function

> +   pointer initialized to the address of a function in a shared

> +   library will either require a .dynbss copy and a copy reloc, or a

> +   dynamic reloc.  Using a .dynbss copy redefines the function

> +   descriptor symbol to point to the copy.  This presents a problem as

> +   a PLT entry for that function is also initialized from the function

> +   descriptor symbol and the copy may not be initialized first.  */


OK. I like this new text and it's much clearer than what we had before.

>  #define ELIMINATE_COPY_RELOCS 1

>

>  /* Section name for stubs is the associated section name plus this

> @@ -6462,13 +6462,23 @@ ppc64_elf_adjust_dynamic_symbol (struct bfd_link_info *info,

>        || h->protected_def)

>      return TRUE;

>

> -  if (h->plt.plist != NULL)

> +  if (h->type == STT_FUNC

> +      || h->type == STT_GNU_IFUNC)


OK. You expand the scope of what is being handled (regardless of a PLT
entry being created).

>      {

> -      /* We should never get here, but unfortunately there are versions

> -        of gcc out there that improperly (for this ABI) put initialized

> -        function pointers, vtable refs and suchlike in read-only

> -        sections.  Allow them to proceed, but warn that this might

> -        break at runtime.  */

> +      /* .dynbss copies of function symbols only work if we have

> +        ELFv1 dot-symbols.  ELFv1 compilers since 2004 default to not

> +        use dot-symbols and set the function symbol size to the text

> +        size of the function rather than the size of the descriptor.

> +        That's wrong for copying a descriptor.  */

> +      if (((struct ppc_link_hash_entry *) h)->oh == NULL

> +         || !(h->size == 24 || h->size == 16))

> +       return TRUE;


OK. Though I wonder if we can't have a text size that's 24-bytes ;-)

> +

> +      /* We should never get here, but unfortunately there are old

> +        versions of gcc (circa gcc-3.2) that improperly for the

> +        ELFv1 ABI put initialized function pointers, vtable refs and

> +        suchlike in read-only sections.  Allow them to proceed, but

> +        warn that this might break at runtime.  */

>        info->callbacks->einfo

>         (_("%P: copy reloc against `%pT' requires lazy plt linking; "

>            "avoid setting LD_BIND_NOW=1 or upgrade gcc\n"),

> diff --git a/ld/testsuite/ld-powerpc/ambiguousv1b.d b/ld/testsuite/ld-powerpc/ambiguousv1b.d

> index 9be1371e5e..205f7ea46f 100644

> --- a/ld/testsuite/ld-powerpc/ambiguousv1b.d

> +++ b/ld/testsuite/ld-powerpc/ambiguousv1b.d

> @@ -3,6 +3,7 @@

>  #as: -a64

>  #ld: -melf64ppc --emit-stub-syms

>  #ld_after_inputfiles: tmpdir/funv1.so

> +#warning: .*requires lazy plt linking.*


OK.

>  #readelf: -rs --wide

>  # Check that we do the right thing with funref2.s that doesn't have

>  # anything to mark it as ELFv1 or ELFv2.  Since my_func address is

> @@ -15,9 +16,9 @@ Relocation section .* contains 1 entry:

>

>  Symbol table '\.dynsym' contains 2 entries:

>  #...

> -.*: 0*[1-9a-f][0-9a-f]*     4 FUNC    GLOBAL DEFAULT   1[23] my_func

> +.*: 0*[1-9a-f][0-9a-f]* +24 FUNC +GLOBAL DEFAULT +1[23] my_func


OK.

>  #...

>  Symbol table '\.symtab' contains .* entries:

>  #...

> -.*: 0*[1-9a-f][0-9a-f]*     4 FUNC    GLOBAL DEFAULT   1[23] my_func

> +.*: 0*[1-9a-f][0-9a-f]* +24 FUNC +GLOBAL DEFAULT +1[23] my_func


OK.

>  #pass

> diff --git a/ld/testsuite/ld-powerpc/funref.s b/ld/testsuite/ld-powerpc/funref.s

> index 3f7de479ce..27c1bcf6b1 100644

> --- a/ld/testsuite/ld-powerpc/funref.s

> +++ b/ld/testsuite/ld-powerpc/funref.s

> @@ -1,4 +1,5 @@

>   .data

>   .globl func_tab

> + .p2align 3


OK.

>  func_tab:

>   .dc.a my_func

> diff --git a/ld/testsuite/ld-powerpc/funref2.s b/ld/testsuite/ld-powerpc/funref2.s

> index a2bf949126..14c58f0123 100644

> --- a/ld/testsuite/ld-powerpc/funref2.s

> +++ b/ld/testsuite/ld-powerpc/funref2.s

> @@ -1,4 +1,5 @@

>   .section .rodata,"a",@progbits

>   .globl func_tab

> + .p2align 3


OK.

>  func_tab:

>   .dc.a my_func

> diff --git a/ld/testsuite/ld-powerpc/funv1.s b/ld/testsuite/ld-powerpc/funv1.s

> index e79009d1d2..988ad0d8c1 100644

> --- a/ld/testsuite/ld-powerpc/funv1.s

> +++ b/ld/testsuite/ld-powerpc/funv1.s

> @@ -1,10 +1,12 @@

> - .globl my_func

> - .type my_func,@function

> - .section .opd,"aw",@progbits

> +# old style ELFv1, with dot-symbols


OK.

> + .globl my_func, .my_func

> + .type .my_func, @function

> + .section .opd, "aw", @progbits

>  my_func:

> - .quad .Lmy_func, .TOC.@tocbase

> + .quad .my_func, .TOC.@tocbase, 0

> + .size my_func, . - my_func

>

>   .text

> -.Lmy_func:

> +.my_func:

>   blr

> - .size my_func,.-.Lmy_func

> + .size .my_func, . - .my_func

>

> --

> Alan Modra

> Australia Development Lab, IBM

>
Alan Modra Jan. 15, 2020, 1:45 p.m. | #2
On Wed, Jan 15, 2020 at 08:17:41AM -0500, Carlos O'Donell wrote:
> On Tue, Jan 14, 2020 at 9:49 PM Alan Modra <amodra@gmail.com> wrote:

> > +      /* .dynbss copies of function symbols only work if we have

> > +        ELFv1 dot-symbols.  ELFv1 compilers since 2004 default to not

> > +        use dot-symbols and set the function symbol size to the text

> > +        size of the function rather than the size of the descriptor.

> > +        That's wrong for copying a descriptor.  */

> > +      if (((struct ppc_link_hash_entry *) h)->oh == NULL

> > +         || !(h->size == 24 || h->size == 16))

> > +       return TRUE;

> 

> OK. Though I wonder if we can't have a text size that's 24-bytes ;-)


You can, but this is belt and braces.  The dot-symbol test h->oh ought
to be sufficient by itself to exclude ELFv2 and newer ELFv1 code.  I'm
just making really sure my pants don't fall off.  :-)

-- 
Alan Modra
Australia Development Lab, IBM

Patch

diff --git a/bfd/elf64-ppc.c b/bfd/elf64-ppc.c
index a451c4753a..8319a21940 100644
--- a/bfd/elf64-ppc.c
+++ b/bfd/elf64-ppc.c
@@ -2788,20 +2788,20 @@  must_be_dyn_reloc (struct bfd_link_info *info,
 }
 
 /* If ELIMINATE_COPY_RELOCS is non-zero, the linker will try to avoid
-   copying dynamic variables from a shared lib into an app's dynbss
+   copying dynamic variables from a shared lib into an app's .dynbss
    section, and instead use a dynamic relocation to point into the
-   shared lib.  With code that gcc generates, it's vital that this be
-   enabled;  In the PowerPC64 ABI, the address of a function is actually
-   the address of a function descriptor, which resides in the .opd
-   section.  gcc uses the descriptor directly rather than going via the
-   GOT as some other ABI's do, which means that initialized function
-   pointers must reference the descriptor.  Thus, a function pointer
-   initialized to the address of a function in a shared library will
-   either require a copy reloc, or a dynamic reloc.  Using a copy reloc
-   redefines the function descriptor symbol to point to the copy.  This
-   presents a problem as a plt entry for that function is also
-   initialized from the function descriptor symbol and the copy reloc
-   may not be initialized first.  */
+   shared lib.  With code that gcc generates it is vital that this be
+   enabled;  In the PowerPC64 ELFv1 ABI the address of a function is
+   actually the address of a function descriptor which resides in the
+   .opd section.  gcc uses the descriptor directly rather than going
+   via the GOT as some other ABIs do, which means that initialized
+   function pointers reference the descriptor.  Thus, a function
+   pointer initialized to the address of a function in a shared
+   library will either require a .dynbss copy and a copy reloc, or a
+   dynamic reloc.  Using a .dynbss copy redefines the function
+   descriptor symbol to point to the copy.  This presents a problem as
+   a PLT entry for that function is also initialized from the function
+   descriptor symbol and the copy may not be initialized first.  */
 #define ELIMINATE_COPY_RELOCS 1
 
 /* Section name for stubs is the associated section name plus this
@@ -6462,13 +6462,23 @@  ppc64_elf_adjust_dynamic_symbol (struct bfd_link_info *info,
       || h->protected_def)
     return TRUE;
 
-  if (h->plt.plist != NULL)
+  if (h->type == STT_FUNC
+      || h->type == STT_GNU_IFUNC)
     {
-      /* We should never get here, but unfortunately there are versions
-	 of gcc out there that improperly (for this ABI) put initialized
-	 function pointers, vtable refs and suchlike in read-only
-	 sections.  Allow them to proceed, but warn that this might
-	 break at runtime.  */
+      /* .dynbss copies of function symbols only work if we have
+	 ELFv1 dot-symbols.  ELFv1 compilers since 2004 default to not
+	 use dot-symbols and set the function symbol size to the text
+	 size of the function rather than the size of the descriptor.
+	 That's wrong for copying a descriptor.  */
+      if (((struct ppc_link_hash_entry *) h)->oh == NULL
+	  || !(h->size == 24 || h->size == 16))
+	return TRUE;
+
+      /* We should never get here, but unfortunately there are old
+	 versions of gcc (circa gcc-3.2) that improperly for the
+	 ELFv1 ABI put initialized function pointers, vtable refs and
+	 suchlike in read-only sections.  Allow them to proceed, but
+	 warn that this might break at runtime.  */
       info->callbacks->einfo
 	(_("%P: copy reloc against `%pT' requires lazy plt linking; "
 	   "avoid setting LD_BIND_NOW=1 or upgrade gcc\n"),
diff --git a/ld/testsuite/ld-powerpc/ambiguousv1b.d b/ld/testsuite/ld-powerpc/ambiguousv1b.d
index 9be1371e5e..205f7ea46f 100644
--- a/ld/testsuite/ld-powerpc/ambiguousv1b.d
+++ b/ld/testsuite/ld-powerpc/ambiguousv1b.d
@@ -3,6 +3,7 @@ 
 #as: -a64
 #ld: -melf64ppc --emit-stub-syms
 #ld_after_inputfiles: tmpdir/funv1.so
+#warning: .*requires lazy plt linking.*
 #readelf: -rs --wide
 # Check that we do the right thing with funref2.s that doesn't have
 # anything to mark it as ELFv1 or ELFv2.  Since my_func address is
@@ -15,9 +16,9 @@  Relocation section .* contains 1 entry:
 
 Symbol table '\.dynsym' contains 2 entries:
 #...
-.*: 0*[1-9a-f][0-9a-f]*     4 FUNC    GLOBAL DEFAULT   1[23] my_func
+.*: 0*[1-9a-f][0-9a-f]* +24 FUNC +GLOBAL DEFAULT +1[23] my_func
 #...
 Symbol table '\.symtab' contains .* entries:
 #...
-.*: 0*[1-9a-f][0-9a-f]*     4 FUNC    GLOBAL DEFAULT   1[23] my_func
+.*: 0*[1-9a-f][0-9a-f]* +24 FUNC +GLOBAL DEFAULT +1[23] my_func
 #pass
diff --git a/ld/testsuite/ld-powerpc/funref.s b/ld/testsuite/ld-powerpc/funref.s
index 3f7de479ce..27c1bcf6b1 100644
--- a/ld/testsuite/ld-powerpc/funref.s
+++ b/ld/testsuite/ld-powerpc/funref.s
@@ -1,4 +1,5 @@ 
  .data
  .globl func_tab
+ .p2align 3
 func_tab:
  .dc.a my_func
diff --git a/ld/testsuite/ld-powerpc/funref2.s b/ld/testsuite/ld-powerpc/funref2.s
index a2bf949126..14c58f0123 100644
--- a/ld/testsuite/ld-powerpc/funref2.s
+++ b/ld/testsuite/ld-powerpc/funref2.s
@@ -1,4 +1,5 @@ 
  .section .rodata,"a",@progbits
  .globl func_tab
+ .p2align 3
 func_tab:
  .dc.a my_func
diff --git a/ld/testsuite/ld-powerpc/funv1.s b/ld/testsuite/ld-powerpc/funv1.s
index e79009d1d2..988ad0d8c1 100644
--- a/ld/testsuite/ld-powerpc/funv1.s
+++ b/ld/testsuite/ld-powerpc/funv1.s
@@ -1,10 +1,12 @@ 
- .globl my_func
- .type my_func,@function
- .section .opd,"aw",@progbits
+# old style ELFv1, with dot-symbols
+ .globl my_func, .my_func
+ .type .my_func, @function
+ .section .opd, "aw", @progbits
 my_func:
- .quad .Lmy_func, .TOC.@tocbase
+ .quad .my_func, .TOC.@tocbase, 0
+ .size my_func, . - my_func
 
  .text
-.Lmy_func:
+.my_func:
  blr
- .size my_func,.-.Lmy_func
+ .size .my_func, . - .my_func