This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Downgrade linker error on protected symbols in .dynbss to a warning


On Fri, Apr 10, 2015 at 4:52 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Apr 10, 2015 at 11:00 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Fri, Apr 10, 2015 at 7:59 AM, Alan Modra <amodra@gmail.com> wrote:
>>> On Fri, Apr 10, 2015 at 06:39:10AM -0700, H.J. Lu wrote:
>>>> On Fri, Apr 10, 2015 at 6:21 AM, Alan Modra <amodra@gmail.com> wrote:
>>>> > On Fri, Apr 10, 2015 at 05:49:05AM -0700, H.J. Lu wrote:
>>>> >> GCC 5 is no more wrong than before with older glibc.
>>>> >
>>>> > This is false.  Your gcc5 change with glibc 2.22 fixes just one
>>>> > particular use of protected visibility variables, a definition in a
>>>> > shared library and a reference in a non-PIC executable.
>>>> >
>>>> > Previous versions of gcc work properly with glibc-2.21 and earlier for
>>>> > - more than one shared library with definitions of a protected
>>>> >   visibility variable,
>>>> > - a shared library with a definition of a protected variable, and an
>>>> >   executable with a definition of the same variable.
>>>> > Those cases are both broken with gcc-5 and glibc-2.21, on x86.
>>>>
>>>> It is broken only if a shared library expects that the protected definition
>>>> won't be preempted by another definition.
>>>
>>> If you don't require protected visibility variables to behave as they
>>> are supposed to behave, then of course everything is fine.
>>>
>>>> > Note that I'm not against the overall idea of your changes.  In fact,
>>>> > I think the idea is quite reasonable.  What I'm complaining about is
>>>> > the complete lack of any checking for older glibc in the case of the
>>>> > gcc change, and lack of checks for older gcc in the binutils change.
>>>>
>>>> We need to start somewhere.  Otherwise, it will never be fixed.
>>>
>>> Maybe, but you should admit that without the checks you have broken
>>> the toolchain.  When one part of the toolchain requires other parts to
>>> change for correct behaviour we usually at least want configure
>>> checks.
>>>
>>> Your gcc change ought to have emitted an attribute or somesuch into
>>> relocatable object files marking them as having protected visibility
>>> variables **with code accessing them no different than that for
>>> default visibility variables**.  That change requires glibc-2.22 for
>>> correctness.
>>
>> GCC, glibc and binutils were never correct before.  If applications
>> depend on the correct behavior of protected data symbol,  they should
>> update GCC, glibc and binutils.
>>
>>> The attribute could then be copied to a note when building a shared
>>> library, and also be used by the linker to warn if linking with an
>>> older ld.so.  The note then could be used by the linker to disable
>>> "copy reloc against protected <symbol> is dangerous".
>>
>> This won't work since link-time shared library != run-time shared
>> library.
>>
>>> Ideally, it would also be possible to disable your gcc change with
>>> configury and command line options, for people with older systems who
>>> want to upgrade gcc but not glibc.
>>>
>>
>> If they care about correctness of protected data symbol, they
>> should update GCC, glibc and binutils.
>
> I will add a linker switch, -z extern-protected-data, to control
> linker behavior.
>

Here is a patch to add such an option.  OK for master?

Thanks.

-- 
H.J.
From 4edbf4492560418a8981389f2a121f5d413c2956 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sat, 11 Apr 2015 07:34:49 -0700
Subject: [PATCH] Add -z [no]extern-protected-data to ld

Address of protected data defined in the shared library may be external,
i.e., due to copy relocation.  By default, linker backend checks if
relocations against protected data symbols are valid for building shared
library and issues an error if relocation isn't allowed.  The new options
override linker backend default.  When -z noextern-protected-data is used,
updates on protected data symbols by another module may not be visibile
to the resulting shared library.

bfd/

	* elflink.c (_bfd_elf_adjust_dynamic_copy): Check
	info->extern_protected_data when warning copy relocs against
	protected symbols.
	(_bfd_elf_symbol_refs_local_p): Check info->extern_protected_data
	when checking protected non-function symbols.

include/

	* bfdlink.h (bfd_link_info): Add extern_protected_data.

ld/

	* ld.texinfo: Document "-z [no]extern-protected-data".
	* ldmain.c (main): Initialize link_info.extern_protected_data
	to -1.
	* lexsup.c (elf_shlib_list_options): Add
	"-z [no]extern-protected-data".
	* emultempl/elf32.em (gld${EMULATION_NAME}_handle_option): Handle
	"-z [no]extern-protected-data".

ld/testsuite/

	* ld-i386/i386.exp: Run protected6b.
	* ld-i386/protected6b.d: New file.
	* ld-x86-64/protected6b.d: Likewise.
	* ld-x86-64/x86-64.exp:  Run protected6b.
---
 bfd/elflink.c                        |  9 +++++++--
 include/bfdlink.h                    |  5 +++++
 ld/emultempl/elf32.em                |  4 ++++
 ld/ld.texinfo                        | 13 +++++++++++++
 ld/ldmain.c                          |  1 +
 ld/lexsup.c                          |  4 ++++
 ld/testsuite/ld-i386/i386.exp        |  1 +
 ld/testsuite/ld-i386/protected6b.d   |  6 ++++++
 ld/testsuite/ld-x86-64/protected6b.d |  6 ++++++
 ld/testsuite/ld-x86-64/x86-64.exp    |  1 +
 10 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100644 ld/testsuite/ld-i386/protected6b.d
 create mode 100644 ld/testsuite/ld-x86-64/protected6b.d

diff --git a/bfd/elflink.c b/bfd/elflink.c
index 98d3108..5d45687 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -2675,7 +2675,9 @@ _bfd_elf_adjust_dynamic_copy (struct bfd_link_info *info,
 
   /* No error if extern_protected_data is true.  */
   if (h->protected_def
-      && !get_elf_backend_data (dynbss->owner)->extern_protected_data)
+      && (!info->extern_protected_data
+	  || (info->extern_protected_data < 0
+	      && !get_elf_backend_data (dynbss->owner)->extern_protected_data)))
     info->callbacks->einfo
       (_("%P: copy reloc against protected `%T' is dangerous\n"),
        h->root.root.string);
@@ -2837,7 +2839,10 @@ _bfd_elf_symbol_refs_local_p (struct elf_link_hash_entry *h,
 
   /* If extern_protected_data is false, STV_PROTECTED non-function
      symbols are local.  */
-  if (!bed->extern_protected_data && !bed->is_function_type (h->type))
+  if ((!info->extern_protected_data
+       || (info->extern_protected_data < 0
+	   && !bed->extern_protected_data))
+      && !bed->is_function_type (h->type))
     return TRUE;
 
   /* Function pointer equality tests may require that STV_PROTECTED
diff --git a/include/bfdlink.h b/include/bfdlink.h
index 6a02a3c..1b15826 100644
--- a/include/bfdlink.h
+++ b/include/bfdlink.h
@@ -517,6 +517,11 @@ struct bfd_link_info
      relaxation returning true in *AGAIN.  */
   int relax_trip;
 
+  /* > 0 to treat protected data defined in the shared library as
+     reference external.  0 to treat it as internal.  -1 to let
+     backend to decide.  */
+  int extern_protected_data;
+
   /* Non-zero if auto-import thunks for DATA items in pei386 DLLs
      should be generated/linked against.  Set to 1 if this feature
      is explicitly requested by the user, -1 if enabled by default.  */
diff --git a/ld/emultempl/elf32.em b/ld/emultempl/elf32.em
index 5db5a93..3f99035 100644
--- a/ld/emultempl/elf32.em
+++ b/ld/emultempl/elf32.em
@@ -2335,6 +2335,10 @@ fragment <<EOF
 	link_info.error_textrel = FALSE;
       else if (strcmp (optarg, "textoff") == 0)
 	link_info.error_textrel = FALSE;
+      else if (strcmp (optarg, "extern-protected-data") == 0)
+	link_info.extern_protected_data = 1;
+      else if (strcmp (optarg, "noextern-protected-data") == 0)
+	link_info.extern_protected_data = 0;
 EOF
 fi
 
diff --git a/ld/ld.texinfo b/ld/ld.texinfo
index 5384c98..62be2f9 100644
--- a/ld/ld.texinfo
+++ b/ld/ld.texinfo
@@ -1054,6 +1054,12 @@ shared libraries are still allowed.
 @item execstack
 Marks the object as requiring executable stack.
 
+@item extern-protected-data
+Treat protected data symbol as external when building shared library.
+Address of protected data defined in the shared library may be external,
+i.e., due to copy relocation.  This option overrides linker backend
+default.
+
 @item global
 This option is only meaningful when building a shared object.  It makes
 the symbols defined by this shared object available for symbol resolution
@@ -1107,6 +1113,13 @@ Marks the object can not be dumped by @code{dldump}.
 @item noexecstack
 Marks the object as not requiring executable stack.
 
+@item noextern-protected-data
+Don't treat protected data symbol as external when building shared
+library.  This option overrides linker backend default.  It can be used
+to workaround incorrect relocations against protected data symbols
+generated by compiler.  Updates on protected data symbols by another
+module aren't visibile to the resulting shared library.
+
 @item text
 Treat DT_TEXTREL in shared object as error.
 
diff --git a/ld/ldmain.c b/ld/ldmain.c
index 6674a80..2ecb92d 100644
--- a/ld/ldmain.c
+++ b/ld/ldmain.c
@@ -285,6 +285,7 @@ main (int argc, char **argv)
   link_info.init_function = "_init";
   link_info.fini_function = "_fini";
   link_info.relax_pass = 1;
+  link_info.extern_protected_data = -1;
   link_info.pei386_auto_import = -1;
   link_info.spare_dynamic_tags = 5;
   link_info.path_separator = ':';
diff --git a/ld/lexsup.c b/ld/lexsup.c
index 4a71ba4..adad7a8 100644
--- a/ld/lexsup.c
+++ b/ld/lexsup.c
@@ -1677,6 +1677,10 @@ elf_shlib_list_options (FILE *file)
   fprintf (file, _("\
   -z nocombreloc              Don't merge dynamic relocs into one section\n"));
   fprintf (file, _("\
+  -z extern-protected-data    Treat protected data symbol as external\n"));
+  fprintf (file, _("\
+  -z noextern-protected-data  Don't treat protected data symbol as external\n"));
+  fprintf (file, _("\
   -z global                   Make symbols in DSO available for subsequently\n\
 			       loaded objects\n"));
   fprintf (file, _("\
diff --git a/ld/testsuite/ld-i386/i386.exp b/ld/testsuite/ld-i386/i386.exp
index 0c0fd96..f214d89 100644
--- a/ld/testsuite/ld-i386/i386.exp
+++ b/ld/testsuite/ld-i386/i386.exp
@@ -237,6 +237,7 @@ run_dump_test "protected3"
 run_dump_test "protected4"
 run_dump_test "protected5"
 run_dump_test "protected6a"
+run_dump_test "protected6b"
 run_dump_test "tlspie1"
 run_dump_test "tlspie2"
 run_dump_test "nogot1"
diff --git a/ld/testsuite/ld-i386/protected6b.d b/ld/testsuite/ld-i386/protected6b.d
new file mode 100644
index 0000000..5642c60
--- /dev/null
+++ b/ld/testsuite/ld-i386/protected6b.d
@@ -0,0 +1,6 @@
+#source: protected6.s
+#as: --32
+#ld: -shared -melf_i386 -z noextern-protected-data
+#readelf: -r
+
+There are no relocations in this file.
diff --git a/ld/testsuite/ld-x86-64/protected6b.d b/ld/testsuite/ld-x86-64/protected6b.d
new file mode 100644
index 0000000..8b44331
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/protected6b.d
@@ -0,0 +1,6 @@
+#source: protected6.s
+#as: --64
+#ld: -shared -melf_x86_64 -z noextern-protected-data
+#readelf: -r
+
+There are no relocations in this file.
diff --git a/ld/testsuite/ld-x86-64/x86-64.exp b/ld/testsuite/ld-x86-64/x86-64.exp
index 213a4c0..8352ad9 100644
--- a/ld/testsuite/ld-x86-64/x86-64.exp
+++ b/ld/testsuite/ld-x86-64/x86-64.exp
@@ -221,6 +221,7 @@ run_dump_test "protected3-l1om"
 run_dump_test "protected4"
 run_dump_test "protected5"
 run_dump_test "protected6a"
+run_dump_test "protected6b"
 run_dump_test "protected7a"
 run_dump_test "protected7b"
 run_dump_test "tlsle1"
-- 
2.1.0


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]