diff mbox

RFC: PATCHES: Properly handle reference to protected data on x86

Message ID CAMe9rOoKS4BwBSd2T+bcchYOykZ7Gzh2jCMC5J6r0qyEX1u0_Q@mail.gmail.com
State Not Applicable
Headers show

Commit Message

H.J. Lu March 4, 2015, 11:26 p.m. UTC
Protected symbol means that it can't be pre-emptied.  It
doesn't mean its address won't be external.  This is true
for pointer to protected function.  With copy relocation,
address of protected data defined in the shared library may
also be external.  We only know that for sure at run-time.
Here are patches for glibc, binutils and GCC to handle it
properly.

Any comments?

Thanks.

Comments

Alan Modra March 6, 2015, 4:19 a.m. UTC | #1
On Wed, Mar 04, 2015 at 03:26:10PM -0800, H.J. Lu wrote:
> Protected symbol means that it can't be pre-emptied.  It
> doesn't mean its address won't be external.  This is true
> for pointer to protected function.  With copy relocation,
> address of protected data defined in the shared library may
> also be external.  We only know that for sure at run-time.
> Here are patches for glibc, binutils and GCC to handle it
> properly.
> 
> Any comments?

I'd like to see this pass some more tests.  For example

reference in non-PIC exe to var x
protected visibility definition of x in libA
protected visibility definition of x in libB

I suspect you don't have this case correct, but congratulations if you
do!  Assuming libA is first on the breadth first search for libraries,
then exe and libA ought to use the same x, but libB have its own x.

In fact it would be good to prove that all variations of either a
reference, a default visibility definition or a protected visibility
definition worked in the exe plus two libs case.
Szabolcs Nagy March 6, 2015, 11:01 a.m. UTC | #2
On 04/03/15 23:26, H.J. Lu wrote:
> Protected symbol means that it can't be pre-emptied.  It
> doesn't mean its address won't be external.  This is true
> for pointer to protected function.  With copy relocation,
> address of protected data defined in the shared library may
> also be external.  We only know that for sure at run-time.
> Here are patches for glibc, binutils and GCC to handle it
> properly.
> 
> Any comments?
> 

i think this fixes
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55012
H.J. Lu March 6, 2015, 1:04 p.m. UTC | #3
On Thu, Mar 5, 2015 at 8:19 PM, Alan Modra <amodra@gmail.com> wrote:
> On Wed, Mar 04, 2015 at 03:26:10PM -0800, H.J. Lu wrote:
>> Protected symbol means that it can't be pre-emptied.  It
>> doesn't mean its address won't be external.  This is true
>> for pointer to protected function.  With copy relocation,
>> address of protected data defined in the shared library may
>> also be external.  We only know that for sure at run-time.
>> Here are patches for glibc, binutils and GCC to handle it
>> properly.
>>
>> Any comments?
>
> I'd like to see this pass some more tests.  For example
>
> reference in non-PIC exe to var x
> protected visibility definition of x in libA
> protected visibility definition of x in libB
>
> I suspect you don't have this case correct, but congratulations if you
> do!  Assuming libA is first on the breadth first search for libraries,
> then exe and libA ought to use the same x, but libB have its own x.

I believe my new testcases on hjl/pr17711 branch at

https://sourceware.org/git/?p=glibc.git;a=summary

covers those and they work correctly.

> In fact it would be good to prove that all variations of either a
> reference, a default visibility definition or a protected visibility
> definition worked in the exe plus two libs case.
>

You can git my branch a try on PPC.  If PPC uses copy
relocation, it shouldn't be too hard to update PPC to make
it work.
Joseph Myers March 6, 2015, 6:29 p.m. UTC | #4
On Wed, 4 Mar 2015, H.J. Lu wrote:

> Protected symbol means that it can't be pre-emptied.  It
> doesn't mean its address won't be external.  This is true
> for pointer to protected function.  With copy relocation,
> address of protected data defined in the shared library may
> also be external.  We only know that for sure at run-time.
> Here are patches for glibc, binutils and GCC to handle it
> properly.
> 
> Any comments?

I don't see any testcases in the glibc patch; it seems critical to have 
sufficient testcases to make it easy for architecture maintainers to tell 
if there is an issue for their architecture (and the testcases need to 
have clear comments explaining any requirements on GCC and binutils for 
them to work - that is, comments referring to committed patches or 
releases rather than to anything uncommitted).
H.J. Lu March 6, 2015, 6:33 p.m. UTC | #5
On Fri, Mar 6, 2015 at 10:29 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Wed, 4 Mar 2015, H.J. Lu wrote:
>
>> Protected symbol means that it can't be pre-emptied.  It
>> doesn't mean its address won't be external.  This is true
>> for pointer to protected function.  With copy relocation,
>> address of protected data defined in the shared library may
>> also be external.  We only know that for sure at run-time.
>> Here are patches for glibc, binutils and GCC to handle it
>> properly.
>>
>> Any comments?
>
> I don't see any testcases in the glibc patch; it seems critical to have
> sufficient testcases to make it easy for architecture maintainers to tell
> if there is an issue for their architecture (and the testcases need to
> have clear comments explaining any requirements on GCC and binutils for
> them to work - that is, comments referring to committed patches or
> releases rather than to anything uncommitted).
>

https://sourceware.org/ml/libc-alpha/2015-03/msg00231.html
Alan Modra March 7, 2015, 12:15 a.m. UTC | #6
On Fri, Mar 06, 2015 at 05:04:56AM -0800, H.J. Lu wrote:
> On Thu, Mar 5, 2015 at 8:19 PM, Alan Modra <amodra@gmail.com> wrote:
> > On Wed, Mar 04, 2015 at 03:26:10PM -0800, H.J. Lu wrote:
> >> Protected symbol means that it can't be pre-emptied.  It
> >> doesn't mean its address won't be external.  This is true
> >> for pointer to protected function.  With copy relocation,
> >> address of protected data defined in the shared library may
> >> also be external.  We only know that for sure at run-time.
> >> Here are patches for glibc, binutils and GCC to handle it
> >> properly.
> >>
> >> Any comments?
> >
> > I'd like to see this pass some more tests.  For example
> >
> > reference in non-PIC exe to var x
> > protected visibility definition of x in libA
> > protected visibility definition of x in libB
> >
> > I suspect you don't have this case correct, but congratulations if you
> > do!  Assuming libA is first on the breadth first search for libraries,
> > then exe and libA ought to use the same x, but libB have its own x.
> 
> I believe my new testcases on hjl/pr17711 branch at
> 
> https://sourceware.org/git/?p=glibc.git;a=summary
> 
> covers those and they work correctly.

The test that I see in commit 9ea148bb does not.  Please notice that
I'm asking you about two protected definitions in the libraries, not
one protected and one with default visibility.
H.J. Lu March 7, 2015, 1:03 a.m. UTC | #7
On Fri, Mar 6, 2015 at 4:15 PM, Alan Modra <amodra@gmail.com> wrote:
> On Fri, Mar 06, 2015 at 05:04:56AM -0800, H.J. Lu wrote:
>> On Thu, Mar 5, 2015 at 8:19 PM, Alan Modra <amodra@gmail.com> wrote:
>> > On Wed, Mar 04, 2015 at 03:26:10PM -0800, H.J. Lu wrote:
>> >> Protected symbol means that it can't be pre-emptied.  It
>> >> doesn't mean its address won't be external.  This is true
>> >> for pointer to protected function.  With copy relocation,
>> >> address of protected data defined in the shared library may
>> >> also be external.  We only know that for sure at run-time.
>> >> Here are patches for glibc, binutils and GCC to handle it
>> >> properly.
>> >>
>> >> Any comments?
>> >
>> > I'd like to see this pass some more tests.  For example
>> >
>> > reference in non-PIC exe to var x
>> > protected visibility definition of x in libA
>> > protected visibility definition of x in libB
>> >
>> > I suspect you don't have this case correct, but congratulations if you
>> > do!  Assuming libA is first on the breadth first search for libraries,
>> > then exe and libA ought to use the same x, but libB have its own x.
>>
>> I believe my new testcases on hjl/pr17711 branch at
>>
>> https://sourceware.org/git/?p=glibc.git;a=summary
>>
>> covers those and they work correctly.
>
> The test that I see in commit 9ea148bb does not.  Please notice that
> I'm asking you about two protected definitions in the libraries, not
> one protected and one with default visibility.

My patch extends my work for protected function pointer to
cover protected data.  There is no reason why it shouldn't work.
I updated the testcase to

commit 88af4693bd32e3658206b73c121de9a36c510f6b

Please check it out.
Alan Modra March 7, 2015, noon UTC | #8
On Fri, Mar 06, 2015 at 05:03:21PM -0800, H.J. Lu wrote:
> I updated the testcase to

Thanks, that's good to see.
H.J. Lu March 7, 2015, 12:51 p.m. UTC | #9
On Sat, Mar 7, 2015 at 4:00 AM, Alan Modra <amodra@gmail.com> wrote:
> On Fri, Mar 06, 2015 at 05:03:21PM -0800, H.J. Lu wrote:
>> I updated the testcase to
>
> Thanks, that's good to see.
>

Did you make it to work on PPC?
diff mbox

Patch

From ced505c47a961691e3f1e252ba5ea2aec4152397 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 4 Mar 2015 08:57:30 -0800
Subject: [PATCH] Add extern_protected_data and set it for x86

With copy relocation, address of protected data defined in the shared
library may be external.  This patch adds extern_protected_data and
changes _bfd_elf_symbol_refs_local_p to return false for protecteddata
if extern_protected_data is true.
---
 bfd/elf-bfd.h                       | 4 ++++
 bfd/elf32-i386.c                    | 1 +
 bfd/elf64-x86-64.c                  | 1 +
 bfd/elflink.c                       | 9 ++++++---
 bfd/elfxx-target.h                  | 6 +++++-
 ld/testsuite/ld-i386/protected3.d   | 3 ++-
 ld/testsuite/ld-i386/protected3.s   | 3 ++-
 ld/testsuite/ld-x86-64/protected3.d | 3 ++-
 ld/testsuite/ld-x86-64/protected3.s | 3 ++-
 9 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index 156eec7..13c32e0 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -1359,6 +1359,10 @@  struct elf_backend_data
      in length rather than sec->size in length, if sec->rawsize is
      non-zero and smaller than sec->size.  */
   unsigned caches_rawsize : 1;
+
+  /* Address of protected data defined in the shared library may be
+     external, i.e., due to copy relocation.   */
+  unsigned extern_protected_data : 1;
 };
 
 /* Information about reloc sections associated with a bfd_elf_section_data
diff --git a/bfd/elf32-i386.c b/bfd/elf32-i386.c
index 3f16fc1..52f4d33 100644
--- a/bfd/elf32-i386.c
+++ b/bfd/elf32-i386.c
@@ -5292,6 +5292,7 @@  elf_i386_add_symbol_hook (bfd * abfd,
 #define elf_backend_want_plt_sym	0
 #define elf_backend_got_header_size	12
 #define elf_backend_plt_alignment	4
+#define elf_backend_extern_protected_data 1
 
 /* Support RELA for objdump of prelink objects.  */
 #define elf_info_to_howto		      elf_i386_info_to_howto_rel
diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
index a4974ce..74d1d06 100644
--- a/bfd/elf64-x86-64.c
+++ b/bfd/elf64-x86-64.c
@@ -5868,6 +5868,7 @@  static const struct bfd_elf_special_section
 #define elf_backend_got_header_size	    (GOT_ENTRY_SIZE*3)
 #define elf_backend_rela_normal		    1
 #define elf_backend_plt_alignment           4
+#define elf_backend_extern_protected_data   1
 
 #define elf_info_to_howto		    elf_x86_64_info_to_howto
 
diff --git a/bfd/elflink.c b/bfd/elflink.c
index ec1e4df..6ee6499 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -2671,7 +2671,9 @@  _bfd_elf_adjust_dynamic_copy (struct bfd_link_info *info,
   /* Increment the size of DYNBSS to make room for the symbol.  */
   dynbss->size += h->size;
 
-  if (h->protected_def)
+  /* No error if extern_protected_data is true.  */
+  if (h->protected_def
+      && !get_elf_backend_data (dynbss->owner)->extern_protected_data)
     {
       info->callbacks->einfo
 	(_("%P: copy reloc against protected `%T' is invalid\n"),
@@ -2835,8 +2837,9 @@  _bfd_elf_symbol_refs_local_p (struct elf_link_hash_entry *h,
 
   bed = get_elf_backend_data (hash_table->dynobj);
 
-  /* STV_PROTECTED non-function symbols are local.  */
-  if (!bed->is_function_type (h->type))
+  /* If extern_protected_data is false, STV_PROTECTED non-function
+     symbols are local.  */
+  if (!bed->extern_protected_data && !bed->is_function_type (h->type))
     return TRUE;
 
   /* Function pointer equality tests may require that STV_PROTECTED
diff --git a/bfd/elfxx-target.h b/bfd/elfxx-target.h
index 211c0a1..9760db4 100644
--- a/bfd/elfxx-target.h
+++ b/bfd/elfxx-target.h
@@ -117,6 +117,9 @@ 
 #ifndef elf_backend_caches_rawsize
 #define elf_backend_caches_rawsize 0
 #endif
+#ifndef elf_backend_extern_protected_data
+#define elf_backend_extern_protected_data 0
+#endif
 #ifndef elf_backend_stack_align
 #define elf_backend_stack_align 16
 #endif
@@ -801,7 +804,8 @@  static struct elf_backend_data elfNN_bed =
   elf_backend_want_dynbss,
   elf_backend_want_p_paddr_set_to_zero,
   elf_backend_default_execstack,
-  elf_backend_caches_rawsize
+  elf_backend_caches_rawsize,
+  elf_backend_extern_protected_data
 };
 
 /* Forward declaration for use when initialising alternative_target field.  */
diff --git a/ld/testsuite/ld-i386/protected3.d b/ld/testsuite/ld-i386/protected3.d
index aafa2d8..47ab4e1 100644
--- a/ld/testsuite/ld-i386/protected3.d
+++ b/ld/testsuite/ld-i386/protected3.d
@@ -8,6 +8,7 @@ 
 Disassembly of section .text:
 
 0+[a-f0-9]+ <bar>:
-[ 	]*[a-f0-9]+:	8b 81 [a-f0-9][a-f0-9] [a-f0-9][a-f0-9] 00 00    	mov    0x[a-f0-9]+\(%ecx\),%eax
+[ 	]*[a-f0-9]+:	8b 81 [a-f0-9][a-f0-9] [a-f0-9][a-f0-9] ff ff    	mov    -0x[a-f0-9]+\(%ecx\),%eax
+[ 	]*[a-f0-9]+:	8b 00                	mov    \(%eax\),%eax
 [ 	]*[a-f0-9]+:	c3                   	ret    
 #pass
diff --git a/ld/testsuite/ld-i386/protected3.s b/ld/testsuite/ld-i386/protected3.s
index 7a605a2..4dd2115 100644
--- a/ld/testsuite/ld-i386/protected3.s
+++ b/ld/testsuite/ld-i386/protected3.s
@@ -10,6 +10,7 @@  foo:
 .globl bar
 	.type	bar, @function
 bar:
-	movl	foo@GOTOFF(%ecx), %eax
+	movl	foo@GOT(%ecx), %eax
+	movl	(%eax), %eax
 	ret
 	.size	bar, .-bar
diff --git a/ld/testsuite/ld-x86-64/protected3.d b/ld/testsuite/ld-x86-64/protected3.d
index 22a36ac..d8f09da 100644
--- a/ld/testsuite/ld-x86-64/protected3.d
+++ b/ld/testsuite/ld-x86-64/protected3.d
@@ -8,6 +8,7 @@ 
 Disassembly of section .text:
 
 0+[a-f0-9]+ <bar>:
-[ 	]*[a-f0-9]+:	8b 05 ([0-9a-f]{2} ){4} *	mov    0x[a-f0-9]+\(%rip\),%eax        # [a-f0-9]+ <foo>
+[ 	]*[a-f0-9]+:	48 8b 05 ([0-9a-f]{2} ){4} *	mov    0x[a-f0-9]+\(%rip\),%rax        # [a-f0-9]+ <_DYNAMIC\+0x[a-f0-9]+>
+[ 	]*[a-f0-9]+:	8b 00                	mov    \(%rax\),%eax
 [ 	]*[a-f0-9]+:	c3                   	retq *
 #pass
diff --git a/ld/testsuite/ld-x86-64/protected3.s b/ld/testsuite/ld-x86-64/protected3.s
index e4af6e7..7538050 100644
--- a/ld/testsuite/ld-x86-64/protected3.s
+++ b/ld/testsuite/ld-x86-64/protected3.s
@@ -10,6 +10,7 @@  foo:
 .globl bar
 	.type	bar, @function
 bar:
-	movl	foo(%rip), %eax
+	movq	foo@GOTPCREL(%rip), %rax
+	movl	(%rax), %eax
 	ret
 	.size	bar, .-bar
-- 
1.9.3