RFC: PATCHES: Properly handle reference to protected data on x86
Commit Message
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
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.
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
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.
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).
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
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.
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.
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.
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?
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(-)
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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. */
@@ -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
@@ -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
@@ -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
@@ -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