[0/2] s390: Avoid relocation overflows on undefined weak symbols

Message ID 20240626152033.2567309-1-jremus@linux.ibm.com
Headers
Series s390: Avoid relocation overflows on undefined weak symbols |

Message

Jens Remus June 26, 2024, 3:20 p.m. UTC
  Patch 1 corrects commit 896a639babe2 ("s390: Avoid reloc overflows on
undefined weak symbols") not to replace Branch Relative on Count High
(brcth) referencing an undefined weak symbol definitively resolving to
zero by a trap, as it is not guaranteed that the conditional branch is
taken in any case.

Patch 2 complements commit 896a639babe2 ("s390: Avoid reloc overflows
on undefined weak symbols") by applying similar replacements of
instructions referencing undefined weak symbols that definitively
resolve to zero. This time for PLT32DBL relocations.

Regards,
Jens

Jens Remus (2):
  s390: Do not replace brcth referencing undefined weak symbol
  s390: Avoid reloc overflows on undefined weak symbols (cont)

 bfd/elf64-s390.c                    | 42 ++++++++++++++++++++++++++---
 ld/testsuite/ld-s390/s390.exp       |  5 +++-
 ld/testsuite/ld-s390/weakundef-1.dd |  6 ++---
 ld/testsuite/ld-s390/weakundef-1.s  |  1 -
 ld/testsuite/ld-s390/weakundef-2.dd | 17 ++++++++++++
 ld/testsuite/ld-s390/weakundef-2.s  | 17 ++++++++++++
 6 files changed, 80 insertions(+), 8 deletions(-)
 create mode 100644 ld/testsuite/ld-s390/weakundef-2.dd
 create mode 100644 ld/testsuite/ld-s390/weakundef-2.s
  

Comments

Fangrui Song June 26, 2024, 6:26 p.m. UTC | #1
On Wed, Jun 26, 2024 at 8:20 AM Jens Remus <jremus@linux.ibm.com> wrote:
>
> Patch 1 corrects commit 896a639babe2 ("s390: Avoid reloc overflows on
> undefined weak symbols") not to replace Branch Relative on Count High
> (brcth) referencing an undefined weak symbol definitively resolving to
> zero by a trap, as it is not guaranteed that the conditional branch is
> taken in any case.
>
> Patch 2 complements commit 896a639babe2 ("s390: Avoid reloc overflows
> on undefined weak symbols") by applying similar replacements of
> instructions referencing undefined weak symbols that definitively
> resolve to zero. This time for PLT32DBL relocations.
>
> Regards,
> Jens
>
> Jens Remus (2):
>   s390: Do not replace brcth referencing undefined weak symbol
>   s390: Avoid reloc overflows on undefined weak symbols (cont)
>
>  bfd/elf64-s390.c                    | 42 ++++++++++++++++++++++++++---
>  ld/testsuite/ld-s390/s390.exp       |  5 +++-
>  ld/testsuite/ld-s390/weakundef-1.dd |  6 ++---
>  ld/testsuite/ld-s390/weakundef-1.s  |  1 -
>  ld/testsuite/ld-s390/weakundef-2.dd | 17 ++++++++++++
>  ld/testsuite/ld-s390/weakundef-2.s  | 17 ++++++++++++
>  6 files changed, 80 insertions(+), 8 deletions(-)
>  create mode 100644 ld/testsuite/ld-s390/weakundef-2.dd
>  create mode 100644 ld/testsuite/ld-s390/weakundef-2.s
>
> --
> 2.40.1
>

Other ports have simpler strategies for unresolved undefined weak symbols
https://reviews.llvm.org/D103001

aarch64: GNU ld: rewrite the instruction to a NOP; ld.lld: branch to
the next instruction
mips: GNU ld: branch to the start of the text segment (?); ld.lld:
branch to zero
ppc32: GNU ld: rewrite the instruction to a NOP; ld.lld: branch to the
current instruction
ppc64: GNU ld: rewrite the instruction to a NOP; ld.lld: branch to the
current instruction
riscv: GNU ld: branch to the absolute zero address (with instruction rewriting)
i386/x86_64: GNU ld/ld.lld: branch to the link-time zero address

I haven't checked the patch closely, but it seems to add quite a few lines.
  
Jens Remus July 1, 2024, 2:33 p.m. UTC | #2
Hello Fangrui,

thank you for the feedback and hints at how the issue is dealt with by 
other architectures!

Am 26.06.2024 um 20:26 schrieb Fangrui Song:
> On Wed, Jun 26, 2024 at 8:20 AM Jens Remus <jremus@linux.ibm.com> wrote:
>> Patch 1 corrects commit 896a639babe2 ("s390: Avoid reloc overflows on
>> undefined weak symbols") not to replace Branch Relative on Count High
>> (brcth) referencing an undefined weak symbol definitively resolving to
>> zero by a trap, as it is not guaranteed that the conditional branch is
>> taken in any case.
>>
>> Patch 2 complements commit 896a639babe2 ("s390: Avoid reloc overflows
>> on undefined weak symbols") by applying similar replacements of
>> instructions referencing undefined weak symbols that definitively
>> resolve to zero. This time for PLT32DBL relocations.
...
>> Jens Remus (2):
>>    s390: Do not replace brcth referencing undefined weak symbol
>>    s390: Avoid reloc overflows on undefined weak symbols (cont)
>>
>>   bfd/elf64-s390.c                    | 42 ++++++++++++++++++++++++++---
>>   ld/testsuite/ld-s390/s390.exp       |  5 +++-
>>   ld/testsuite/ld-s390/weakundef-1.dd |  6 ++---
>>   ld/testsuite/ld-s390/weakundef-1.s  |  1 -
>>   ld/testsuite/ld-s390/weakundef-2.dd | 17 ++++++++++++
>>   ld/testsuite/ld-s390/weakundef-2.s  | 17 ++++++++++++
>>   6 files changed, 80 insertions(+), 8 deletions(-)
>>   create mode 100644 ld/testsuite/ld-s390/weakundef-2.dd
>>   create mode 100644 ld/testsuite/ld-s390/weakundef-2.s

> Other ports have simpler strategies for unresolved undefined weak symbols
> https://reviews.llvm.org/D103001
> 
> aarch64: GNU ld: rewrite the instruction to a NOP; ld.lld: branch to
> the next instruction
> mips: GNU ld: branch to the start of the text segment (?); ld.lld:
> branch to zero
> ppc32: GNU ld: rewrite the instruction to a NOP; ld.lld: branch to the
> current instruction
> ppc64: GNU ld: rewrite the instruction to a NOP; ld.lld: branch to the
> current instruction
> riscv: GNU ld: branch to the absolute zero address (with instruction rewriting)
> i386/x86_64: GNU ld/ld.lld: branch to the link-time zero address
> 
> I haven't checked the patch closely, but it seems to add quite a few lines.

I had a quick look at bfd.ld AArch64 and PPC64. As you listed they both 
replace branches with NOPs. Yet it is unclear to me how/where they deal 
with (function) pointers resolving to zero.

On s390x using a section start address greater than 4GB at link-time 
causes PC32DBL and PLT32DBL PC-relative relocations for undefined weak 
symbols that definitively resolve to zero to overflow.

So far we have identified three types of PC-relative instructions and 
decided to rewrite them as follows to not change the general behavior:
- PC-relative load address of zero are replaced by a non-PC-relative
   load address of zero. This ensures correctness of run-time checks
   for undef weak symbols that resolved to zero.
- PC-relative branches to zero are replaced by a trap. A branch to zero
   would result in an exception at run-time and so does a trap.
- PC-relative instructions that can be considered optional, such as 
PC-relative prefetch of zero, are replaced by a NOP.

While technically it might be ok to rewrite subject branches as a NOP, 
we think that using a trap is better for debugging, as a NOP might go 
undetected at run-time. Also the complexity of the logic to do so does 
not change whether to rewrite as NOP or trap.

Regards,
Jens
  
Alan Modra July 3, 2024, 12:43 a.m. UTC | #3
On Mon, Jul 01, 2024 at 04:33:20PM +0200, Jens Remus wrote:
> I had a quick look at bfd.ld AArch64 and PPC64. As you listed they both
> replace branches with NOPs. Yet it is unclear to me how/where they deal with
> (function) pointers resolving to zero.

ppc64 does nothing special.  As with most other targets, you need to
write
  if (foo)
    foo ();
when dealing with a weak function foo that may or may not be defined
at runtime.  However, if you know the function will be resolved at
link time (static linking, or -z nodynamic-undefined-weak) then you
can just write
  foo ();
and get the same behaviour as
  if (foo)
    foo ();

So replacing a branch and link with a nop is only useful in special
cases, and of course doesn't apply if you're calling via a function
pointer.
  
Jens Remus July 9, 2024, 3:22 p.m. UTC | #4
Am 03.07.2024 um 02:43 schrieb Alan Modra:
> On Mon, Jul 01, 2024 at 04:33:20PM +0200, Jens Remus wrote:
>> I had a quick look at bfd.ld AArch64 and PPC64. As you listed they both
>> replace branches with NOPs. Yet it is unclear to me how/where they deal with
>> (function) pointers resolving to zero.
> 
> ppc64 does nothing special.  As with most other targets, you need to
> write
>    if (foo)
>      foo ();

On s390x both the address taking in "if (foo)" and the call of "foo ()" 
have a PC-relative relocation that may overflow for undefined weak 
symbols that definitively resolve to zero. So both instructions need to 
be rewritten.

In the ppc64 code I think I found where the call of "foo ()" is 
apparently rewritten as a NOP in bfd/elf64-ppc.c 
ppc64_elf_relocate_section (): "NOP out calls to undefined weak 
functions. ...". I did not find where the address taking in "if (foo)" 
is rewritten with a load of zero.

> when dealing with a weak function foo that may or may not be defined
> at runtime.  However, if you know the function will be resolved at
> link time (static linking, or -z nodynamic-undefined-weak) then you
> can just write
>    foo ();
> and get the same behaviour as
>    if (foo)
>      foo ();
> 
> So replacing a branch and link with a nop is only useful in special
> cases, and of course doesn't apply if you're calling via a function
> pointer.

Thank you for the explanation!

Thanks and regards,
Jens
  
Jens Remus July 12, 2024, 2:58 p.m. UTC | #5
Am 26.06.2024 um 17:20 schrieb Jens Remus:
> Patch 1 corrects commit 896a639babe2 ("s390: Avoid reloc overflows on
> undefined weak symbols") not to replace Branch Relative on Count High
> (brcth) referencing an undefined weak symbol definitively resolving to
> zero by a trap, as it is not guaranteed that the conditional branch is
> taken in any case.
> 
> Patch 2 complements commit 896a639babe2 ("s390: Avoid reloc overflows
> on undefined weak symbols") by applying similar replacements of
> instructions referencing undefined weak symbols that definitively
> resolve to zero. This time for PLT32DBL relocations.
...

> Jens Remus (2):
>    s390: Do not replace brcth referencing undefined weak symbol
>    s390: Avoid reloc overflows on undefined weak symbols (cont)
...

Committed to mainline with Andreas' approval.

Regards,
Jens