[PATCH] Allow direct access relocations referencing a protected function symbol

[PATCH] Allow direct access relocations referencing a protected function symbol

[Fangrui Song]

This fixes the bogus "relocation R_* against protected symbol `foo'
can not be used when making a shared object" for function symbols for
at least aarch64/i386/x86-64.

The controversial "copy relocations on protected data symbols" (which has some
fragile glibc support) is irrelevant to function symbols.

This patch makes the following program buildable with gcc -fpic -shared
-fuse-ld=bfd:

    __attribute__((visibility("protected"))) void *foo () {
      return (void *)foo;
    }

gold has always been good.

    PR ld/27973
    * ld-i386/protected1.d: Update.
    * ld-x86-64/protected1.d: Likewise.
    * ld-x86-64/protected7a.d: Likewise.
---
 bfd/elflink.c                        | 8 ++++++--
 ld/testsuite/ld-i386/protected1.d    | 4 +++-
 ld/testsuite/ld-x86-64/protected1.d  | 4 +++-
 ld/testsuite/ld-x86-64/protected7a.d | 4 +++-
 4 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/bfd/elflink.c b/bfd/elflink.c
index c9a6e78be79..8e349af68e3 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -3294,9 +3294,13 @@ _bfd_elf_symbol_refs_local_p (struct elf_link_hash_entry *h,
   if (h == NULL)
     return true;
 
-  /* STV_HIDDEN or STV_INTERNAL ones must be local.  */
+  /* Non-STV_DEFAULT symbols must be local. However, STV_PROTECTED data symbols
+   * may cause controversial copy relocations on x86 which has some fragile
+   * support in glibc.  */
   if (ELF_ST_VISIBILITY (h->other) == STV_HIDDEN
-      || ELF_ST_VISIBILITY (h->other) == STV_INTERNAL)
+      || ELF_ST_VISIBILITY (h->other) == STV_INTERNAL
+      || (ELF_ST_VISIBILITY (h->other) == STV_PROTECTED
+	  && h->type == STT_FUNC))
     return true;
 
   /* Forced local symbols resolve locally.  */
diff --git a/ld/testsuite/ld-i386/protected1.d b/ld/testsuite/ld-i386/protected1.d
index a3cb5cef140..bbf5d77784d 100644
--- a/ld/testsuite/ld-i386/protected1.d
+++ b/ld/testsuite/ld-i386/protected1.d
@@ -1,3 +1,5 @@
 #as: --32
 #ld: -shared -melf_i386
-#error: .*relocation R_386_GOTOFF against protected function `foo' can not be used when making a shared object
+#readelf: -r
+
+There are no relocations in this file.
diff --git a/ld/testsuite/ld-x86-64/protected1.d b/ld/testsuite/ld-x86-64/protected1.d
index 783b85a1a6f..63f8d990370 100644
--- a/ld/testsuite/ld-x86-64/protected1.d
+++ b/ld/testsuite/ld-x86-64/protected1.d
@@ -1,3 +1,5 @@
 #as: --64
 #ld: -shared -melf_x86_64
-#error: .*relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
+#readelf: -r
+
+There are no relocations in this file.
diff --git a/ld/testsuite/ld-x86-64/protected7a.d b/ld/testsuite/ld-x86-64/protected7a.d
index 3082084a7b8..d675d5460ff 100644
--- a/ld/testsuite/ld-x86-64/protected7a.d
+++ b/ld/testsuite/ld-x86-64/protected7a.d
@@ -1,4 +1,6 @@
 #source: protected7.s
 #as: --64
 #ld: -shared -melf_x86_64
-#error: .*relocation R_X86_64_GOTOFF64 against protected function `foo' can not be used when making a shared object
+#readelf: -r
+
+There are no relocations in this file.
-- 
2.32.0.272.g935e593368-goog

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Alan Modra]

On Sun, Jun 13, 2021 at 02:54:00PM -0700, Fangrui Song via Binutils wrote:
> This fixes the bogus "relocation R_* against protected symbol `foo'
> can not be used when making a shared object" for function symbols for
> at least aarch64/i386/x86-64.
> 
> The controversial "copy relocations on protected data symbols" (which has some
> fragile glibc support) is irrelevant to function symbols.

No, this patch doesn't do that.  What you are doing here will disable
dynamic relocations on protected function symbols in shared libraries.
That will break function pointer comparison for architectures that
implement non-pic executables, where a function that is undefined in
the executable is given a fixed address in the executable, that of its
plt call code.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Michael Matz]

Hello,

On Mon, 14 Jun 2021, Alan Modra via Binutils wrote:

> On Sun, Jun 13, 2021 at 02:54:00PM -0700, Fangrui Song via Binutils wrote:
> > This fixes the bogus "relocation R_* against protected symbol `foo'
> > can not be used when making a shared object" for function symbols for
> > at least aarch64/i386/x86-64.
> > 
> > The controversial "copy relocations on protected data symbols" (which has some
> > fragile glibc support) is irrelevant to function symbols.
> 
> No, this patch doesn't do that.  What you are doing here will disable
> dynamic relocations on protected function symbols in shared libraries.
> That will break function pointer comparison for architectures that
> implement non-pic executables, where a function that is undefined in
> the executable is given a fixed address in the executable, that of its
> plt call code.

Correct.  But I'm oscillating between thinking that this would be a 
problem and thinking the opposite :-/

One could always say that function addresses of protected functions aren't 
comparable.  Taking an address and using it as indirect call target will 
always work, you just wouldn't be able to compare them usefully.  Or one 
could require address references from outside components to a protected 
(function) symbol to always be via a GOT(-like structure).

Or (the other extremum of my oscillations) one says that function address 
comparisons absolutely need to work even in absence of 
indirection-via-GOT, at which point we basically talk about the same 
problem like protected data symbols and copy relocations.  If you then 
don't have relocations differing between calls and address taking, you 
effectively throw out the usefullness of protected visibility.

The problem with the latter stance is that it's not really justifiable 
from the ELF gABI: nothing says that getting at "the" function address 
must be possible without a GOT indirection.

(Adding to that is that what actually works in practice changed over time 
and depends on the architecture (and well, yeah, compiler and linker) so 
that now it's impossible to say "look there, that's how it should work 
and how everyone expects it to work" :-/ )


Ciao,
Michael.

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[H.J. Lu]

On Monday, June 14, 2021, Michael Matz <matz@suse.de> wrote:

> Hello,
>
> On Mon, 14 Jun 2021, Alan Modra via Binutils wrote:
>
> > On Sun, Jun 13, 2021 at 02:54:00PM -0700, Fangrui Song via Binutils
> wrote:
> > > This fixes the bogus "relocation R_* against protected symbol `foo'
> > > can not be used when making a shared object" for function symbols for
> > > at least aarch64/i386/x86-64.
> > >
> > > The controversial "copy relocations on protected data symbols" (which
> has some
> > > fragile glibc support) is irrelevant to function symbols.
> >
> > No, this patch doesn't do that.  What you are doing here will disable
> > dynamic relocations on protected function symbols in shared libraries.
> > That will break function pointer comparison for architectures that
> > implement non-pic executables, where a function that is undefined in
> > the executable is given a fixed address in the executable, that of its
> > plt call code.
>
> Correct.  But I'm oscillating between thinking that this would be a
> problem and thinking the opposite :-/
>
> One could always say that function addresses of protected functions aren't
> comparable.  Taking an address and using it as indirect call target will
> always work, you just wouldn't be able to compare them usefully.  Or one
> could require address references from outside components to a protected
> (function) symbol to always be via a GOT(-like structure).
>
> Or (the other extremum of my oscillations) one says that function address
> comparisons absolutely need to work even in absence of
> indirection-via-GOT, at which point we basically talk about the same
> problem like protected data symbols and copy relocations.  If you then
> don't have relocations differing between calls and address taking, you
> effectively throw out the usefullness of protected visibility.
>
> The problem with the latter stance is that it's not really justifiable
> from the ELF gABI: nothing says that getting at "the" function address
> must be possible without a GOT indirection.
>
> (Adding to that is that what actually works in practice changed over time
> and depends on the architecture (and well, yeah, compiler and linker) so
> that now it's impossible to say "look there, that's how it should work
> and how everyone expects it to work" :-/ )
>
>
> Ciao,
> Michael.
>

I'd to use GOT for function address and remove copy
relocation in both PIE and non-PIE.   The question is
that if we should detect binary incompatibility at link-time
and run-time.



-- 
H.J.

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Fangrui Song]

I shall add a note that this patch will restore the behavior before late
2015 or early 2016.

Cary had a good summary
https://sourceware.org/legacy-ml/binutils/2016-03/msg00331.html
(The original thread was about: Swift folks moved from GNU ld to gold
because they noticed that STV_PROTECTED had been broken in late 2015 or
early 2016.)

On 2021-06-14, H.J. Lu via Binutils wrote:
>On Monday, June 14, 2021, Michael Matz <matz@suse.de> wrote:
>
>> Hello,
>>
>> On Mon, 14 Jun 2021, Alan Modra via Binutils wrote:
>>
>> > On Sun, Jun 13, 2021 at 02:54:00PM -0700, Fangrui Song via Binutils
>> wrote:
>> > > This fixes the bogus "relocation R_* against protected symbol `foo'
>> > > can not be used when making a shared object" for function symbols for
>> > > at least aarch64/i386/x86-64.
>> > >
>> > > The controversial "copy relocations on protected data symbols" (which
>> has some
>> > > fragile glibc support) is irrelevant to function symbols.
>> >
>> > No, this patch doesn't do that.  What you are doing here will disable
>> > dynamic relocations on protected function symbols in shared libraries.
>> > That will break function pointer comparison for architectures that
>> > implement non-pic executables, where a function that is undefined in
>> > the executable is given a fixed address in the executable, that of its
>> > plt call code.
>>
>> Correct.  But I'm oscillating between thinking that this would be a
>> problem and thinking the opposite :-/
>>
>> One could always say that function addresses of protected functions aren't
>> comparable.  Taking an address and using it as indirect call target will
>> always work, you just wouldn't be able to compare them usefully.  Or one
>> could require address references from outside components to a protected
>> (function) symbol to always be via a GOT(-like structure).
>>
>> Or (the other extremum of my oscillations) one says that function address
>> comparisons absolutely need to work even in absence of
>> indirection-via-GOT, at which point we basically talk about the same
>> problem like protected data symbols and copy relocations.  If you then
>> don't have relocations differing between calls and address taking, you
>> effectively throw out the usefullness of protected visibility.
>>
>> The problem with the latter stance is that it's not really justifiable
>> from the ELF gABI: nothing says that getting at "the" function address
>> must be possible without a GOT indirection.
>>
>> (Adding to that is that what actually works in practice changed over time
>> and depends on the architecture (and well, yeah, compiler and linker) so
>> that now it's impossible to say "look there, that's how it should work
>> and how everyone expects it to work" :-/ )
>>
>>
>> Ciao,
>> Michael.
>>


Add to what Michael said,

The ELF spec says:
"STV_PROTECTED - A symbol defined in the current component is protected if it is visible in other components but not preemptable, meaning that any reference to such a symbol from within the defining component must be resolved to the definition in that component, even if there is a definition in another component that would preempt by the default rules."

Not producing a dynamic relocation (gold) is correct. GNU ld's dynamic
relocation causes unnecessary complexity in ld.so.
(https://sourceware.org/legacy-ml/binutils/2016-03/msg00331.html)

A STV_PROTECTED definition in a DSO is indeed incompatible with (for STT_OBJECT symbols) copy relocations and (for STT_FUNC symbols) canonical PLT entries (a PLT in the main executable whose address is considered canonical; st_shndx=0 && st_value!=0).
Keep STV_PROTECTED because that is the whole purpose behind the optimization (https://sourceware.org/legacy-ml/binutils/2016-03/msg00331.html)

The linker rule should be simple. If a no-pic object file (which is linked into the executable) will need copy relocations or canonical PLT entries, warn/error as Solaris does (https://groups.google.com/g/generic-abi/c/waK1dGiUGvM/m/8mMOmzjiIgAJ).
gold reports an error at link time (https://sourceware.org/bugzilla/show_bug.cgi?id=19823).

    % cc -o main.2 main.o -R. libfoo.so.2
    ld: warning: relocation warning: R_386_COPY: file libfoo.so.2: \
        symbol readonly_str: relocation bound to a symbol with \
        STV_PROTECTED visibility
    ld: warning: relocation warning: R_386_COPY: file libfoo.so.2: \
        symbol writable_str: relocation bound to a symbol with \
        STV_PROTECTED visibility
    % elfdump -r main.2 | grep _str
       [0] R_386_COPY  0x8060d5c   0   .bss    writable_str
       [1] R_386_COPY  0x8060d6f    0   .bss    readonly_str

>I'd to use GOT for function address and remove copy
>relocation in both PIE and non-PIE.   The question is
>that if we should detect binary incompatibility at link-time
>and run-time.

Function and variable cases are different.

I focus on the function case because fixing the problems for functions
can improve performance while fixing variables has less immediate
benefit.

Plus, the copy relocations on protected data symbols has some
complexity in glibc. While I agree with Michael that adding various
GNU_PROPERTY is over-engineering, I know some folks may prefer that.
By separating function cases from variable cases, I hope folks can
notice that many changes can be applied unconditionally. No link-time
detection, no runtime-time detection, it is just restore of the old
goold behavior.

* This patch
* eliminate copy relocations for x86-64 -fpie https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570139.html
* eliminate canonical PLT entries for -fno-pic (all architectures) https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100593

https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#summary has a summary.

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Alan Modra]

On Mon, Jun 14, 2021 at 10:43:36AM -0700, Fangrui Song wrote:
> I shall add a note that this patch will restore the behavior before late
> 2015 or early 2016.

Also add a note that the patch you posted is incorrect.

BTW, I think it was earlier than 2015 that HJ made access to protected
symbols in shared libraries slower in order to make access from the
executable faster.  You are way too late to try to reverse that
change in GNU ld and gcc.  (I was too late too.  By the time I raised
objections HJ had code in gcc and binutils, and it seems no one on the
gcc side cared about the fact that protected symbols in shared
libraries were made virtually useless from an optimisation perspective
on x86 and other architectures like arm that followed x86.)

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Fangrui Song]

On 2021-06-15, Alan Modra wrote:
>On Mon, Jun 14, 2021 at 10:43:36AM -0700, Fangrui Song wrote:
>> I shall add a note that this patch will restore the behavior before late
>> 2015 or early 2016.
>
>Also add a note that the patch you posted is incorrect.

Only the description needs an update. The code is correct.

     Treat protected function symbols local to the component like hidden/internal visibilities.
     
     This allows direct access relocations referencing a protected function symbol...

>BTW, I think it was earlier than 2015 that HJ made access to protected
>symbols in shared libraries slower in order to make access from the
>executable faster.  

There were binutils complaints in 2016 from GCC side https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248
but no GCC maintainer has paid enough attention to this issue.

>You are way too late to try to reverse that
>change in GNU ld and gcc.  (I was too late too.  By the time I raised
>objections HJ had code in gcc and binutils, and it seems no one on the
>gcc side cared about the fact that protected symbols in shared
>libraries were made virtually useless from an optimisation perspective
>on x86 and other architectures like arm that followed x86.)

I do not see why it is too late to fix GNU ld.

The GCC behavior on protected function symbols never regresses.

   __attribute__((visibility("protected"))) void *addr() { return (void *)addr; }
   
   __attribute__((visibility("protected"))) int var;
   int load() { return var; }
   
   // inlinable, i.e. no need for -fno-semantic-interposition in -fpic mode.
   __attribute__((visibility("protected"))) int inlinable(int x) { return x + x; }
   int bar(int x) { return inlinable(x); }

I have compiled the code with gcc 4.4, 4.9.4, 5.1, and 11.1, -fno-pic,-fpie and -fpic.
The only regressed case is for `load` (variable case), gcc 5 started to use unnecessary GOT.
This isn't a big deal - global variable accesses should not be in a
performance bottleneck.

Note that `inlinable` is inlinable. It doesn't need to have a branch
relocation which may lead to a JUMP_SLOT.


The main point of the patch is to make this work:

   __attribute__((visibility("protected"))) void *foo () {
     return (void *)foo;
   }

The new GNU ld behavior does suppress dynamic relocations for protected
symbols, which is a bonus.
The GCC behavior will be compatible with all of the pre-2015/2016
behavior, the 2015/2016~2021 behavior, and the future behavior.

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Alan Modra]

On Mon, Jun 14, 2021 at 08:19:32PM -0700, Fangrui Song wrote:
> On 2021-06-15, Alan Modra wrote:
> > On Mon, Jun 14, 2021 at 10:43:36AM -0700, Fangrui Song wrote:
> > > I shall add a note that this patch will restore the behavior before late
> > > 2015 or early 2016.
> > 
> > Also add a note that the patch you posted is incorrect.
> 
> Only the description needs an update. The code is correct.

How would you respond if I submitted a patch to lld that you could see
was wrong, but I kept arrogantly claiming was correct?

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Fangrui Song]

On 2021-06-16, Alan Modra wrote:
>On Mon, Jun 14, 2021 at 08:19:32PM -0700, Fangrui Song wrote:
>> On 2021-06-15, Alan Modra wrote:
>> > On Mon, Jun 14, 2021 at 10:43:36AM -0700, Fangrui Song wrote:
>> > > I shall add a note that this patch will restore the behavior before late
>> > > 2015 or early 2016.
>> >
>> > Also add a note that the patch you posted is incorrect.
>>
>> Only the description needs an update. The code is correct.
>
>How would you respond if I submitted a patch to lld that you could see
>was wrong, but I kept arrogantly claiming was correct?
>
>-- 
>Alan Modra
>Australia Development Lab, IBM

If you have found a mistake, please point out the mistake. No euphemism or
sarcasm is needed.

I said "Treat protected function symbols local to the component like hidden/internal visibilities."
That is certainly not my invention of the semantics of STV_PROTECTED.
Multiple people agreed on this. https://groups.google.com/g/generic-abi/c/waK1dGiUGvM/m/7Uid2HH4IQAJ
(which I have linked to previously)

Solaris ld agrees with this (from the dump of the generic-abi discussion).
gold agrees with this: https://sourceware.org/bugzilla/show_bug.cgi?id=19823#c1


I have heard multiple folks said "protected symbols are broken."
Why?  As-is,

__attribute__((visibility("protected"))) void *foo () {
   return (void *)foo;
}

gcc -fpic -shared b.c -fuse-ld=bfd b.c  is broken.

The code generation does have inferior optimization in certain places but they
are not correctness issues.

My intention was to fix the ld problem. I have answered that there is no
compatibility issue.

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Alan Modra]

On Tue, Jun 15, 2021 at 09:42:17PM -0700, Fangrui Song wrote:
> On 2021-06-16, Alan Modra wrote:
> > On Mon, Jun 14, 2021 at 08:19:32PM -0700, Fangrui Song wrote:
> > > On 2021-06-15, Alan Modra wrote:
> > > > On Mon, Jun 14, 2021 at 10:43:36AM -0700, Fangrui Song wrote:
> > > > > I shall add a note that this patch will restore the behavior before late
> > > > > 2015 or early 2016.
> > > >
> > > > Also add a note that the patch you posted is incorrect.
> > > 
> > > Only the description needs an update. The code is correct.
> > 
> > How would you respond if I submitted a patch to lld that you could see
> > was wrong, but I kept arrogantly claiming was correct?
> 
> If you have found a mistake, please point out the mistake. No euphemism or
> sarcasm is needed.

I already told you exactly what was wrong with your patch in my first
email in this thread.  _bfd_elf_symbol_refs_local_p is a function used
by many ELF targets.  You cannot lightly make a change to this
function.  A change that even ignores comments in the function!  That
was your first mistake.

Furthermore, you did not test the change very well.  That was your
second mistake.  Not an uncommon mistake, of course.  I fully expect
your change will cause failures in some part of the toolchain
(gcc+glibc+binutils) testsuites even on x86, particularly when using
older gcc.  Also, if you had tested the change more widely you would
have found a regression just in the binutils testsuite on a less
popular target.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Fangrui Song]

On 2021-06-16, Alan Modra wrote:
>On Tue, Jun 15, 2021 at 09:42:17PM -0700, Fangrui Song wrote:
>> On 2021-06-16, Alan Modra wrote:
>> > On Mon, Jun 14, 2021 at 08:19:32PM -0700, Fangrui Song wrote:
>> > > On 2021-06-15, Alan Modra wrote:
>> > > > On Mon, Jun 14, 2021 at 10:43:36AM -0700, Fangrui Song wrote:
>> > > > > I shall add a note that this patch will restore the behavior before late
>> > > > > 2015 or early 2016.
>> > > >
>> > > > Also add a note that the patch you posted is incorrect.
>> > >
>> > > Only the description needs an update. The code is correct.
>> >
>> > How would you respond if I submitted a patch to lld that you could see
>> > was wrong, but I kept arrogantly claiming was correct?
>>
>> If you have found a mistake, please point out the mistake. No euphemism or
>> sarcasm is needed.
>
>I already told you exactly what was wrong with your patch in my first
>email in this thread.  _bfd_elf_symbol_refs_local_p is a function used
>by many ELF targets.  You cannot lightly make a change to this
>function.  A change that even ignores comments in the function!  That
>was your first mistake.

I think you mean

"What you are doing here will disable dynamic relocations on protected
function symbols in shared libraries.  That will break function pointer
comparison for architectures that implement non-pic executables, where a
function that is undefined in the executable is given a fixed address in
the executable, that of its plt call code."

[Some folks call this a "canonical PLT entry". I know it is not a widely used term.]

Both Michael and H.J. have replied. I think their replies match Cary's/my thought:

If a -fno-pic program uses an absolute/PC-relative relocation to take the
address of a function which is defined as protected in a shared object, the
linker can report an error or break pointer equality.
(Rafael made this an error in lld
https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/protected-function-access.s and
users can suppress the error with --ignore-function-address-equality)

If the compiler wants to keep pointer equality working, use GOT when taking the
address of an undefined function in -fno-pic mode (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100593)

(
Is such function pointer equality useful or leveraged in practice?
It is super rare. My analysis https://maskray.me/blog/2021-05-16-elf-interposition-and-bsymbolic#pointer-equality-for-functions
)

To gold, the STT_OBJECT diagnostic probably should apply to STT_FUNC as well
https://sourceware.org/bugzilla/show_bug.cgi?id=19823#c7

>Furthermore, you did not test the change very well.  That was your
>second mistake.  Not an uncommon mistake, of course.  I fully expect
>your change will cause failures in some part of the toolchain
>(gcc+glibc+binutils) testsuites even on x86, particularly when using
>older gcc.  Also, if you had tested the change more widely you would
>have found a regression just in the binutils testsuite on a less
>popular target.

I cannot find documentation on how to properly test binutils. I think that may
be the reason that (as I have watched binutils for quite some time now)
frequently someone caused breakage for some less popular targets and then you
kindly fixed them...

Good documentation can make the process smoother... 

Configure
binutils multiple times with --target=aarch64, --target=powerpc64le,
--target=x86_64 and numerous less popular tests, make all-ld, and then make check-ld?

(
I tested (1)x86_64 (2)aarch64 (3)default with --enable-targets=all
but I don't know what less popular target doesn't like the change
)



While (I think Cary/Michael) and I prefer that STV_PROTECTED STT_FUNC symbols don't have unneeded dynamic relocations in -shared mode,
I don't mind if GNU ld keeps producing them.
Anyhow the important thing is the following spurious error:

   % gcc -fpic -shared -fuse-ld=bfd a.s
   /usr/bin/ld.bfd: /tmp/ccWPJCLw.o: relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
   __attribute__((visibility("protected"))) void *foo () {
     return (void *)foo;
   }

If you decide to take over, fixing the error will be really great and will be an important step fixing protected symbols.

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Michael Matz]

Hello,

On Wed, 16 Jun 2021, Fangrui Song wrote:

> >I already told you exactly what was wrong with your patch in my first
> >email in this thread.  _bfd_elf_symbol_refs_local_p is a function used
> >by many ELF targets.  You cannot lightly make a change to this
> >function.  A change that even ignores comments in the function!  That
> >was your first mistake.
> 
> I think you mean
> 
> "What you are doing here will disable dynamic relocations on protected
> function symbols in shared libraries.  That will break function pointer
> comparison for architectures that implement non-pic executables, where a
> function that is undefined in the executable is given a fixed address in
> the executable, that of its plt call code."
> 
> [Some folks call this a "canonical PLT entry". I know it is not a widely used
> term.]
> 
> Both Michael and H.J. have replied. I think their replies match Cary's/my
> thought:

Sure, but I wasn't talking about the specific patch, only the overall 
effects it has on some targets (and what I think would or would not be 
desirable).

If you look again at the function you changed you will see that it already 
contains handling for protected symbols, basically the whole second half 
of that function deals with that.  And it does so based on command line 
and target settings; your change will effectively make the function ignore 
these settings, which cannot be simply done; certainly not without 
explicitely removing the then dead handling and adjusting all comments to 
those effects.  (But some targets will probably want to continue having 
their canonical plt-slot handling, so all that code really can't just be 
removed)


Ciao,
Michael.

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Alan Modra]

On Wed, Jun 16, 2021 at 01:11:43AM -0700, Fangrui Song wrote:
> While (I think Cary/Michael) and I prefer that STV_PROTECTED STT_FUNC symbols don't have unneeded dynamic relocations in -shared mode,

I strongly prefer that too.  The only thing I object to is incorrect
fixes..

> I don't mind if GNU ld keeps producing them.
> Anyhow the important thing is the following spurious error:
> 
>   % gcc -fpic -shared -fuse-ld=bfd a.s
>   /usr/bin/ld.bfd: /tmp/ccWPJCLw.o: relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
>   __attribute__((visibility("protected"))) void *foo () {
>     return (void *)foo;
>   }

Agreed, that is an x86 linker bug.

Here is the result of your testcase on powerpc64le, power10 to get
pc-relative cpu support, and linked without startup files so just the
relocs in this code are seen.

powerpc64le-linux-gcc -c -O2 -fPIC -mcpu=power10 prot2.c
powerpc64le-linux-ld -shared -o prot2 prot2.o
readelf -r prot2
There are no relocations in this file.

> 
> If you decide to take over, fixing the error will be really great and will be an important step fixing protected symbols.

It would be inappropriate for me to take over.  This is a target
issue, caused by a deliberate choice on the part of HJ to optimise in
the executable at the expense of shared library optimisation (*).  The
fix needs to be in the x86 target code, and it's a very long time
since I was an x86 binutils maintainer.

*) I'll note that at the time, when non-pic executables were more
common, the trick HJ used made quite a lot of sense from an
optimisation viewpoint, but frankly I'm amazed that he managed to get
it all working.  In fact, the optimisation makes even more sense on
powerpc prior to power10 but I was unwilling to go against the ELF
gABI (when narrowly considering the resulting shared libraries in
isolation) just to shave off cycles in micro-benchmarks.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Fangrui Song]

On 2021-06-17, Alan Modra wrote:
>On Wed, Jun 16, 2021 at 01:11:43AM -0700, Fangrui Song wrote:
>> While (I think Cary/Michael) and I prefer that STV_PROTECTED STT_FUNC symbols don't have unneeded dynamic relocations in -shared mode,
>
>I strongly prefer that too.  The only thing I object to is incorrect
>fixes..

Thanks...

>> I don't mind if GNU ld keeps producing them.
>> Anyhow the important thing is the following spurious error:
>>
>>   % gcc -fpic -shared -fuse-ld=bfd a.s
>>   /usr/bin/ld.bfd: /tmp/ccWPJCLw.o: relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
>>   __attribute__((visibility("protected"))) void *foo () {
>>     return (void *)foo;
>>   }
>
>Agreed, that is an x86 linker bug.

Sent https://sourceware.org/pipermail/binutils/2021-June/116985.html for
x86-64.

Hope some aarch64 folks in the CC list can fix aarch64...

>Here is the result of your testcase on powerpc64le, power10 to get
>pc-relative cpu support, and linked without startup files so just the
>relocs in this code are seen.
>
>powerpc64le-linux-gcc -c -O2 -fPIC -mcpu=power10 prot2.c
>powerpc64le-linux-ld -shared -o prot2 prot2.o
>readelf -r prot2
>There are no relocations in this file.

So x86 and aarch64 have major issues. arm has a minor issue.
other ports are probably good.

x86-64: broken direct access relocation, unneeded GLOB_DAT
aarch64: broken direct access relocation, unneeded GLOB_DAT
arm: unneeded GLOB_DAT
ppc64le: good
mips64el: good
riscv64: good

>>
>> If you decide to take over, fixing the error will be really great and will be an important step fixing protected symbols.
>
>It would be inappropriate for me to take over.  This is a target
>issue, caused by a deliberate choice on the part of HJ to optimise in
>the executable at the expense of shared library optimisation (*).  The
>fix needs to be in the x86 target code, and it's a very long time
>since I was an x86 binutils maintainer.

nvm, I sent  https://sourceware.org/pipermail/binutils/2021-June/116985.html 

>*) I'll note that at the time, when non-pic executables were more
>common, the trick HJ used made quite a lot of sense from an
>optimisation viewpoint, but frankly I'm amazed that he managed to get
>it all working.  In fact, the optimisation makes even more sense on
>powerpc prior to power10 but I was unwilling to go against the ELF
>gABI (when narrowly considering the resulting shared libraries in
>isolation) just to shave off cycles in micro-benchmarks.

Hmm, I think those changes don't improve -fno-pic or -fpie performance.
ELF -fno-pic has always been using absolute relocations for
data/functions. For -fpie, HAVE_LD_PIE_COPYRELOC has no benefit but a
tiny bit of size decrease.  ppc64 ELFv2 toc-indirection to toc-relative
optimization renders the GOT optimization mostly useless, as well. 
HAVE_LD_PIE_COPYRELOC, if implemented, may have a tiny bit of effects on
other arches, but I am of the opinion that toolchain developers do not
necessarily show mercy to applications where global variable access is a
bottleneck.  The applications should fix their global variable usage:
hidden/protected/local alias. Either can improve global variable access
if they indeed decide to care. Unfortunately protected data accesses
have unneeded GOT indirection for all GCC arches in -fpic mode.

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[H.J. Lu]

On Wed, Jun 16, 2021 at 9:24 PM Fangrui Song <i@maskray.me> wrote:
>
> On 2021-06-17, Alan Modra wrote:
> >On Wed, Jun 16, 2021 at 01:11:43AM -0700, Fangrui Song wrote:
> >> While (I think Cary/Michael) and I prefer that STV_PROTECTED STT_FUNC symbols don't have unneeded dynamic relocations in -shared mode,
> >
> >I strongly prefer that too.  The only thing I object to is incorrect
> >fixes..
>
> Thanks...
>
> >> I don't mind if GNU ld keeps producing them.
> >> Anyhow the important thing is the following spurious error:
> >>
> >>   % gcc -fpic -shared -fuse-ld=bfd a.s
> >>   /usr/bin/ld.bfd: /tmp/ccWPJCLw.o: relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
> >>   __attribute__((visibility("protected"))) void *foo () {
> >>     return (void *)foo;
> >>   }
> >
> >Agreed, that is an x86 linker bug.
>
> Sent https://sourceware.org/pipermail/binutils/2021-June/116985.html for
> x86-64.
>
> Hope some aarch64 folks in the CC list can fix aarch64...
>
> >Here is the result of your testcase on powerpc64le, power10 to get
> >pc-relative cpu support, and linked without startup files so just the
> >relocs in this code are seen.
> >
> >powerpc64le-linux-gcc -c -O2 -fPIC -mcpu=power10 prot2.c
> >powerpc64le-linux-ld -shared -o prot2 prot2.o
> >readelf -r prot2
> >There are no relocations in this file.
>
> So x86 and aarch64 have major issues. arm has a minor issue.
> other ports are probably good.
>
> x86-64: broken direct access relocation, unneeded GLOB_DAT
> aarch64: broken direct access relocation, unneeded GLOB_DAT
> arm: unneeded GLOB_DAT
> ppc64le: good
> mips64el: good
> riscv64: good
>
> >>
> >> If you decide to take over, fixing the error will be really great and will be an important step fixing protected symbols.
> >
> >It would be inappropriate for me to take over.  This is a target
> >issue, caused by a deliberate choice on the part of HJ to optimise in
> >the executable at the expense of shared library optimisation (*).  The
> >fix needs to be in the x86 target code, and it's a very long time
> >since I was an x86 binutils maintainer.
>
> nvm, I sent  https://sourceware.org/pipermail/binutils/2021-June/116985.html
>
> >*) I'll note that at the time, when non-pic executables were more
> >common, the trick HJ used made quite a lot of sense from an
> >optimisation viewpoint, but frankly I'm amazed that he managed to get
> >it all working.  In fact, the optimisation makes even more sense on
> >powerpc prior to power10 but I was unwilling to go against the ELF
> >gABI (when narrowly considering the resulting shared libraries in
> >isolation) just to shave off cycles in micro-benchmarks.
>
> Hmm, I think those changes don't improve -fno-pic or -fpie performance.
> ELF -fno-pic has always been using absolute relocations for
> data/functions. For -fpie, HAVE_LD_PIE_COPYRELOC has no benefit but a
> tiny bit of size decrease.  ppc64 ELFv2 toc-indirection to toc-relative
> optimization renders the GOT optimization mostly useless, as well.
> HAVE_LD_PIE_COPYRELOC, if implemented, may have a tiny bit of effects on
> other arches, but I am of the opinion that toolchain developers do not
> necessarily show mercy to applications where global variable access is a
> bottleneck.  The applications should fix their global variable usage:
> hidden/protected/local alias. Either can improve global variable access
> if they indeed decide to care. Unfortunately protected data accesses
> have unneeded GOT indirection for all GCC arches in -fpic mode.

On x86-64, function pointers in executable for external funtions may be
resolved to their PLT entries in executable.  If it happens, function
pointers of protected funtions in shared libraries must be resolved to
the PLT entries in executable, not addresses of protected funtions in
shared libraries.

I made a proposal to remove copy relocation and add canonical function
address:

https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues/8

It should address this issue.  But this is an ABI change.  The question
is if we should do it silently which can result in run-time failure.

-- 
H.J.

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Fangrui Song]

On 2021-06-17, H.J. Lu wrote:
>On Wed, Jun 16, 2021 at 9:24 PM Fangrui Song <i@maskray.me> wrote:
>>
>> On 2021-06-17, Alan Modra wrote:
>> >On Wed, Jun 16, 2021 at 01:11:43AM -0700, Fangrui Song wrote:
>> >> While (I think Cary/Michael) and I prefer that STV_PROTECTED STT_FUNC symbols don't have unneeded dynamic relocations in -shared mode,
>> >
>> >I strongly prefer that too.  The only thing I object to is incorrect
>> >fixes..
>>
>> Thanks...
>>
>> >> I don't mind if GNU ld keeps producing them.
>> >> Anyhow the important thing is the following spurious error:
>> >>
>> >>   % gcc -fpic -shared -fuse-ld=bfd a.s
>> >>   /usr/bin/ld.bfd: /tmp/ccWPJCLw.o: relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
>> >>   __attribute__((visibility("protected"))) void *foo () {
>> >>     return (void *)foo;
>> >>   }
>> >
>> >Agreed, that is an x86 linker bug.
>>
>> Sent https://sourceware.org/pipermail/binutils/2021-June/116985.html for
>> x86-64.
>>
>> Hope some aarch64 folks in the CC list can fix aarch64...
>>
>> >Here is the result of your testcase on powerpc64le, power10 to get
>> >pc-relative cpu support, and linked without startup files so just the
>> >relocs in this code are seen.
>> >
>> >powerpc64le-linux-gcc -c -O2 -fPIC -mcpu=power10 prot2.c
>> >powerpc64le-linux-ld -shared -o prot2 prot2.o
>> >readelf -r prot2
>> >There are no relocations in this file.
>>
>> So x86 and aarch64 have major issues. arm has a minor issue.
>> other ports are probably good.
>>
>> x86-64: broken direct access relocation, unneeded GLOB_DAT
>> aarch64: broken direct access relocation, unneeded GLOB_DAT
>> arm: unneeded GLOB_DAT
>> ppc64le: good
>> mips64el: good
>> riscv64: good
>>
>> >>
>> >> If you decide to take over, fixing the error will be really great and will be an important step fixing protected symbols.
>> >
>> >It would be inappropriate for me to take over.  This is a target
>> >issue, caused by a deliberate choice on the part of HJ to optimise in
>> >the executable at the expense of shared library optimisation (*).  The
>> >fix needs to be in the x86 target code, and it's a very long time
>> >since I was an x86 binutils maintainer.
>>
>> nvm, I sent  https://sourceware.org/pipermail/binutils/2021-June/116985.html
>>
>> >*) I'll note that at the time, when non-pic executables were more
>> >common, the trick HJ used made quite a lot of sense from an
>> >optimisation viewpoint, but frankly I'm amazed that he managed to get
>> >it all working.  In fact, the optimisation makes even more sense on
>> >powerpc prior to power10 but I was unwilling to go against the ELF
>> >gABI (when narrowly considering the resulting shared libraries in
>> >isolation) just to shave off cycles in micro-benchmarks.
>>
>> Hmm, I think those changes don't improve -fno-pic or -fpie performance.
>> ELF -fno-pic has always been using absolute relocations for
>> data/functions. For -fpie, HAVE_LD_PIE_COPYRELOC has no benefit but a
>> tiny bit of size decrease.  ppc64 ELFv2 toc-indirection to toc-relative
>> optimization renders the GOT optimization mostly useless, as well.
>> HAVE_LD_PIE_COPYRELOC, if implemented, may have a tiny bit of effects on
>> other arches, but I am of the opinion that toolchain developers do not
>> necessarily show mercy to applications where global variable access is a
>> bottleneck.  The applications should fix their global variable usage:
>> hidden/protected/local alias. Either can improve global variable access
>> if they indeed decide to care. Unfortunately protected data accesses
>> have unneeded GOT indirection for all GCC arches in -fpic mode.
>
>On x86-64, function pointers in executable for external funtions may be
>resolved to their PLT entries in executable.  If it happens, function
>pointers of protected funtions in shared libraries must be resolved to
>the PLT entries in executable, not addresses of protected funtions in
>shared libraries.

For C/C++, normally

(a) the function address taken in the executable
(b) the function address taken in the shared object 

should be identical. The intention of the diagnostic was to enforce it. However, 

   % gcc -fpic -shared -fuse-ld=bfd a.s
   __attribute__((visibility("protected"))) void *foo () {
     return (void *)foo;
   }

has been broken for so many years... This adds a proof that nobody uses (b).

Well, technically people can use inline asm this way

   // R_X86_64_REX_GOTPCRELX => R_X86_64_GLOB_DAT
   // gcc -fpic -shared -fuse-ld=bfd a.s
   void *addr() { return (void *)addr; }
   asm(".protected addr");

but I am really doubting any project is using this.

I think the GNU ld and GCC fixes
https://sourceware.org/pipermail/binutils/2021-June/116985.html
https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570139.html

should just be unconditionally applied.


If you think fixing the potential pointer equality issue for default visibility STT_FUNC symbols
is good (as I repeated many times, such code is rare, considering --icf=all
and Windows dllimport),
you can submit a patch unconditionally using GOT for -fno-pic function
address https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100593 ...

>I made a proposal to remove copy relocation and add canonical function
>address:
>
>https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues/8
>
>It should address this issue.  But this is an ABI change.  The question
>is if we should do it silently which can result in run-time failure.

I am with Michael. For many points it is over-engineering.  Though, as I
said, "I care more about the function case less about the variable case
(copy relocations)", if you want to introduce a GNU PROPERTY (I think
something like GNU_PROPERTY_NO_COPY_ON_PROTECTED already exists), I do
not necessarily stand in the way.

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[Alan Modra]

On Thu, Jun 17, 2021 at 10:25:09AM -0700, H.J. Lu wrote:
> On x86-64, function pointers in executable for external funtions may be
> resolved to their PLT entries in executable.  If it happens, function
> pointers of protected funtions in shared libraries must be resolved to
> the PLT entries in executable, not addresses of protected funtions in
> shared libraries.

Yes, but quite plainly we have a broken toolchain at the moment.  With
correct options for generating shared library code, gcc generates
objects that ld.bfd refuses to link.  That's BAD.  At a minimum the ld
error needs to be reduced to a warning, but preferably silenced.

What's more, function pointer comparisons work in many cases when the
executable is a PIE, even on x86.  For example

library:
void __attribute__ ((visibility ("protected")))
protfun (void) { __builtin_printf ("Address in lib is %lx\n", (long) &protfun); }

executable:
extern void protfun (void);
int main (void)
{
  __builtin_printf ("Address in main is %lx\n", (long) &protfun);
  protfun ();
  return 0;
}

And this variant of the executable

extern void protfun (void);
void *const addr = &protfun;

int main (void)
{
  __builtin_printf ("Address in main is %lx\n", (long) addr);
  protfun ();
  return 0;
}

only fails when PIE due to ld.bfd needlessly emitting a dynamic
protfun symbol with non-zero value.  It's not needed when the const
"addr" is in .data.rel.ro, a writable section when ld.so is
relocating.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Allow direct access relocations referencing a protected function symbol

[H.J. Lu]

On Thu, Jun 17, 2021 at 6:54 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Thu, Jun 17, 2021 at 10:25:09AM -0700, H.J. Lu wrote:
> > On x86-64, function pointers in executable for external funtions may be
> > resolved to their PLT entries in executable.  If it happens, function
> > pointers of protected funtions in shared libraries must be resolved to
> > the PLT entries in executable, not addresses of protected funtions in
> > shared libraries.
>
> Yes, but quite plainly we have a broken toolchain at the moment.  With
> correct options for generating shared library code, gcc generates
> objects that ld.bfd refuses to link.  That's BAD.  At a minimum the ld
> error needs to be reduced to a warning, but preferably silenced.
>
> What's more, function pointer comparisons work in many cases when the
> executable is a PIE, even on x86.  For example
>
> library:
> void __attribute__ ((visibility ("protected")))
> protfun (void) { __builtin_printf ("Address in lib is %lx\n", (long) &protfun); }
>
> executable:
> extern void protfun (void);
> int main (void)
> {
>   __builtin_printf ("Address in main is %lx\n", (long) &protfun);
>   protfun ();
>   return 0;
> }
>
> And this variant of the executable
>
> extern void protfun (void);
> void *const addr = &protfun;
>
> int main (void)
> {
>   __builtin_printf ("Address in main is %lx\n", (long) addr);
>   protfun ();
>   return 0;
> }
>
> only fails when PIE due to ld.bfd needlessly emitting a dynamic
> protfun symbol with non-zero value.  It's not needed when the const
> "addr" is in .data.rel.ro, a writable section when ld.so is
> relocating.

I want to address this once for all.

-- 
H.J.