This is an archive of the discontinued LLVM Phabricator instance.

[FreeBSD] avoid marking __stack_chk_guard symbol as dso_local on FreeBSD
ClosedPublic

Authored by adalava on Sep 1 2021, 1:57 PM.

Details

Summary

Binaries compiled with any optization model plus -fstack-protector-strong crash with segmentation on llvm12 and later due to bad pointer to "__stack_chk_guard" . This is known to affect powerpc64 (both BE and LE) on FreeBSD 13 and later.

Source of regression was traced to 2518433f861fcb877d0a7bdd9aec1aec1f77505a but it's not clear that this commit is the "real root cause", it probably uncovered a bug in PowerPC64 backend since other platforms generate a working binary. In any case on FreeBSD the "__stack_chk_guard" symbol is defined on external DSO (libc.so.7), so make sense not marking it as dso_local..

Linux doesn't follow the same path since it uses a different approach (LOAD_STACK_GUARD)

*Reported in bugzilla: https://bugs.llvm.org/show_bug.cgi?id=51590
*Similar patch was merged to FreeBSD source tree: https://cgit.freebsd.org/src/commit/?h=stable/13&id=e8e5d75e6a9676e76c3bfd6d1d52561ffbb40846

Test case:

int main(int argc, char *argv[]) 
{ 
  char name[10]; 
  if (ttyname_r(0, name, 10)) 
    err(1, "capsicum"); return 0;
};

build parameters:

-target powerpc64-unknown-freebsd13 -O2 -fstack-protector-strong

Diff Detail

Event Timeline

adalava created this revision.Sep 1 2021, 1:57 PM
adalava requested review of this revision.Sep 1 2021, 1:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2021, 1:57 PM
adalava retitled this revision from Amends LLVM commit 2518433f861fcb877d0a7bdd9aec1aec1f77505a that was pointed as the source of regression on LLVM12. to [PPC64] fix/workaround runtime crash on FreeBSD with llvm12 and later.Sep 1 2021, 2:14 PM
adalava edited the summary of this revision. (Show Details)
adalava updated this revision to Diff 370058.Sep 1 2021, 2:22 PM
adalava added a reviewer: MaskRay.

rebased from stable/13.x to main branch

arichardson added a comment.EditedSep 1 2021, 4:18 PM

This does not seem like the right approach to me. Have you looked into why the pointer is not correct. Does it affect static binaries or dynamic ones? Maybe this can be fixed in rtld instead? As it works for all other architectures (and PPC Linux), I feel like that is the more likely cause

This does not seem like the right approach to me. Have you looked into why the pointer is not correct. Does it affect static binaries or dynamic ones? Maybe this can be fixed in rtld instead? As it works for all other architectures (and PPC Linux), I feel like that is the more likely cause

Agreed, it looks like "GV->setDSOLocal(true)" is only exposing the problem. I confirmed it affects only dynamic binaries, static ones are fine. On clang11 it points to something like 0x81030fad8, and on clang12 0xffffffffffff8ee8. I noticed PPC64LE Linux binaries are using a different approach since "stack_chk_guard" isn't defined on glibc, according to https://reviews.llvm.org/D17740. I used the patch bellow to force the Linux approach and it builds sane binaries but it's not correct since FreeBSD has "stack_chk_guard" defined on libc.

diff --git a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
index 7be3115bb7db..cb88af4c0821 100644
--- a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -16633,8 +16633,8 @@ SDValue PPCTargetLowering::getNegatedExpression(SDValue Op, SelectionDAG &DAG,

 // Override to enable LOAD_STACK_GUARD lowering on Linux.
 bool PPCTargetLowering::useLoadStackGuardNode() const {
-  if (!Subtarget.isTargetLinux())
-    return TargetLowering::useLoadStackGuardNode();
+  //if (!Subtarget.isTargetLinux())
+  //  return TargetLowering::useLoadStackGuardNode();
   return true;
 }

diff --git a/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp b/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
index c74b6bcf4287..d92ea1053201 100644
--- a/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
+++ b/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
@@ -3022,8 +3022,8 @@ bool PPCInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
     return true;
   }
   case TargetOpcode::LOAD_STACK_GUARD: {
-    assert(Subtarget.isTargetLinux() &&
-           "Only Linux target is expected to contain LOAD_STACK_GUARD");
+    //assert(Subtarget.isTargetLinux() &&
+    //       "Only Linux target is expected to contain LOAD_STACK_GUARD");
     const int64_t Offset = Subtarget.isPPC64() ? -0x7010 : -0x7008;
     const unsigned Reg = Subtarget.isPPC64() ? PPC::X13 : PPC::R2;
     MI.setDesc(get(Subtarget.isPPC64() ? PPC::LD : PPC::LWZ));
lkail added a subscriber: lkail.Sep 9 2021, 5:36 PM

It looks like this worked prior to 2518433f861f because shouldAssumeDSOLocal has this case:

// PPC has no copy relocations and cannot use a plt entry as a symbol address.
llvm::Triple::ArchType Arch = TT.getArch();
if (Arch == llvm::Triple::ppc || Arch == llvm::Triple::ppc64 ||
    Arch == llvm::Triple::ppc64le)
  return false;

(later changed to TT.isPPC64() but same effect)

MaskRay added a comment.EditedSep 17 2021, 12:17 PM

It looks like this worked prior to 2518433f861f because shouldAssumeDSOLocal has this case:

// PPC has no copy relocations and cannot use a plt entry as a symbol address.
llvm::Triple::ArchType Arch = TT.getArch();
if (Arch == llvm::Triple::ppc || Arch == llvm::Triple::ppc64 ||
    Arch == llvm::Triple::ppc64le)
  return false;

(later changed to TT.isPPC64() but same effect)

The ppc part was incorrect. ppc32 has copy relocations and GCC does leverage it. isPPC64 is because ELF v2 prefers TOC indirection for access to non-preemptible definitions, but the special condition was still spurious.

I am afraid the root cause is still unknown and this patch just papers over the issue.
I hope we have a good understanding why having dso_local will cause trouble on FreeBSD powerpc64's -fstack-protector* and fix the root cause instead.

I am afraid the root cause is still unknown and this patch just papers over the issue.
I hope we have a good understanding why having dso_local will cause trouble on FreeBSD powerpc64's -fstack-protector* and fix the root cause instead.

@MaskRay , since the binary works when compiled with -O0 and doesn't work with -O1, I used '-mllvm -opt-bisect-limit=NNN' as clang parameter and found that the binary stops working at the following optimization pass:

BISECT: running pass (80) PowerPC DAG->DAG Pattern Instruction Selection on function (main)

May it give some light?

@MaskRay According to @adalava the symbol __stack_chk_guard resides in libc.so on FreeBSD (i.e. a different DSO from the compiled program). Can you please elaborate on why you feel that marking such a symbol dso_local is the correct thing to do and there is an underlying problem elsewhere? Once we understand the logic there, we can dig into any deeper problems.

@sfertile Just wanted to add you here as well as you are quite familiar with dso_local and the various PPC relocations.

@adalava the symbol __stack_chk_guard resides in libc.so on FreeBSD (i.e. a different DSO from the compiled program).

If that is the case then isn't DSO local. Was the idea behind Make __stack_chk_guard dso_local if Reloc::Static that the linker would emit a copy-relocation in the case we don't statically link against libc?

llvm/lib/CodeGen/TargetLoweringBase.cpp
1994

Obviously fix the formatting, but otherwise this looks like the right fix to me.

adalava updated this revision to Diff 379589.Oct 13 2021, 7:56 PM
adalava edited the summary of this revision. (Show Details)

Update patch to avoid setting dso_local for any FreeBSD target since symbol is defined on libc.so.7.

It's now avoiding a copy relocation for '__stack_chk_guard':

Relocation section with addend (.rela.dyn):
r_offset     r_info       r_type              st_value         st_name + r_addend
000010020f88 000100000026 R_PPC64_ADDR64      0000000000000000 _init_tls + 0
000010020f80 000200000026 R_PPC64_ADDR64      0000000000000000 atexit + 0
000010020f98 000300000026 R_PPC64_ADDR64      0000000000000000 exit + 0
000010020fa8 000800000026 R_PPC64_ADDR64      0000000000000000 __stack_chk_guard + 0
..
..
Symbol table (.dynsym) contains 11 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND _init_tls@FBSD_1.0 (2)
     2: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND atexit@FBSD_1.0 (2)
     3: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND exit@FBSD_1.0 (2)
     4: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND _Jv_RegisterClasses
     5: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND ttyname_r@FBSD_1.0 (2)
     6: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND err@FBSD_1.0 (2)
     7: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND __stack_chk_fail@FBSD_1.0 (2)
     8: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  UND __stack_chk_guard@FBSD_1.0 (2)
     9: 0000000010030fb0     8 OBJECT  GLOBAL DEFAULT   24 __progname
    10: 0000000010030fc0     8 OBJECT  GLOBAL DEFAULT   26 environ
adalava marked an inline comment as done.Oct 13 2021, 7:59 PM
adalava updated this revision to Diff 379762.Oct 14 2021, 10:34 AM

add tests for FreeBSD

@sfertile is this last version still good for you? (after removing isPPC64 from if clause)

MaskRay added inline comments.Oct 14 2021, 11:04 AM
llvm/test/CodeGen/PowerPC/stack-protector.ll
15

Can be simplified as [[#]]

adalava updated this revision to Diff 380010.Oct 15 2021, 8:09 AM
  • Simplified register number check as suggested by @MaskRay
  • Added test for powerpcle (32 bits LE)
adalava marked an inline comment as done.Oct 15 2021, 8:09 AM
adalava retitled this revision from [PPC64] fix/workaround runtime crash on FreeBSD with llvm12 and later to [FreeBSD] avoid marking __stack_chk_guard symbol as dso_local on FreeBSD.Oct 15 2021, 8:25 AM
adalava edited the summary of this revision. (Show Details)
nemanjai accepted this revision.Oct 26 2021, 2:17 AM

LGTM.

This revision is now accepted and ready to land.Oct 26 2021, 2:17 AM
adalava edited the summary of this revision. (Show Details)Nov 4 2021, 10:49 AM
This revision was automatically updated to reflect the committed changes.
dim added a subscriber: pkubaj.Nov 6 2021, 6:00 AM

Hmm this seems to cause some trouble when building kernels and kernel modules on FreeBSD, which are built in -ffreestanding mode, and then you get weird relocations in kernel modules:

clang13  -O2 -pipe -fno-common  -fno-strict-aliasing -Werror -D_KERNEL -DKLD_MODULE -DKLD_TIED -nostdinc   -DHAVE_KERNEL_OPTION_HEADERS -include /usr/home/kostik/work/DEV/src/sys/amd64/compile/X/opt_global.h -I. -I/usr/home/kostik/work/DEV/src/sys -I/usr/home/kostik/work/DEV/src/sys/contrib/ck/include -fno-common -gdwarf-2 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fdebug-prefix-map=./machine=/usr/home/kostik/work/DEV/src/sys/amd64/include -fdebug-prefix-map=./x86=/usr/home/kostik/work/DEV/src/sys/x86/include -I/usr/home/kostik/work/DEV/src/sys/amd64/compile/X     -MD  -MF.depend.ufs_vnops.o -MTufs_vnops.o -mcmodel=kernel -mno-red-zone -mno-mmx -mno-sse -msoft-float  -fno-asynchronous-unwind-tables -ffreestanding -fwrapv -fstack-protector -Wall -Wredundant-decls -Wnested-externs -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wcast-qual -Wundef -Wno-pointer-sign -D__printf__=__freebsd_kprintf__ -Wmissing-include-dirs -fdiagnostics-show-option -Wno-unknown-pragmas -Wno-error=tautological-compare -Wno-error=empty-body -Wno-error=parentheses-equality -Wno-error=unused-function -Wno-error=pointer-sign -Wno-error=shift-negative-value -Wno-address-of-packed-member -Wno-error=unused-but-set-variable -Wno-format-zero-length   -mno-aes -mno-avx  -std=iso9899:1999 -c /usr/home/kostik/work/DEV/src/sys/ufs/ufs/ufs_vnops.c -o ufs_vnops.o

% readelf -a ../src/sys/amd64/compile/X/modules/usr/home/kostik/work/DEV/src/sys/modules/ufs/ufs_vnops.o | grep GOT | head -3
0000000001f7  004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4
00000000060d  004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4
0000000007c8  004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4

Maybe this should have an additional check for freestanding mode?

Hmm this seems to cause some trouble when building kernels and kernel modules on FreeBSD, which are built in -ffreestanding mode, and then you get weird relocations in kernel modules:

% readelf -a ../src/sys/amd64/compile/X/modules/usr/home/kostik/work/DEV/src/sys/modules/ufs/ufs_vnops.o | grep GOT | head -3
0000000001f7 004a0000002a R_X86_64_REX_GOTP 0000000000000000 stack_chk_guard - 4
00000000060d 004a0000002a R_X86_64_REX_GOTP 0000000000000000
stack_chk_guard - 4
0000000007c8 004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4

Maybe this should have an additional check for freestanding mode?

Hmm, something seems odd here. I don't claim to really understand what is going on or why these relocations are a problem, but it seems bad if there is a requirement for dso_local for correctness.
The way I understand this attribute is that it is useful for optimization only - i.e. it is not required for anything, but if something is actually local to a DSO, a more optimal code sequence is possible. Shouldn't it be conservatively correct to just omit dso_local from everything?

Hmm this seems to cause some trouble when building kernels and kernel modules on FreeBSD, which are built in -ffreestanding mode, and then you get weird relocations in kernel modules:

clang13  -O2 -pipe -fno-common  -fno-strict-aliasing -Werror -D_KERNEL -DKLD_MODULE -DKLD_TIED -nostdinc   -DHAVE_KERNEL_OPTION_HEADERS -include /usr/home/kostik/work/DEV/src/sys/amd64/compile/X/opt_global.h -I. -I/usr/home/kostik/work/DEV/src/sys -I/usr/home/kostik/work/DEV/src/sys/contrib/ck/include -fno-common -gdwarf-2 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fdebug-prefix-map=./machine=/usr/home/kostik/work/DEV/src/sys/amd64/include -fdebug-prefix-map=./x86=/usr/home/kostik/work/DEV/src/sys/x86/include -I/usr/home/kostik/work/DEV/src/sys/amd64/compile/X     -MD  -MF.depend.ufs_vnops.o -MTufs_vnops.o -mcmodel=kernel -mno-red-zone -mno-mmx -mno-sse -msoft-float  -fno-asynchronous-unwind-tables -ffreestanding -fwrapv -fstack-protector -Wall -Wredundant-decls -Wnested-externs -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wcast-qual -Wundef -Wno-pointer-sign -D__printf__=__freebsd_kprintf__ -Wmissing-include-dirs -fdiagnostics-show-option -Wno-unknown-pragmas -Wno-error=tautological-compare -Wno-error=empty-body -Wno-error=parentheses-equality -Wno-error=unused-function -Wno-error=pointer-sign -Wno-error=shift-negative-value -Wno-address-of-packed-member -Wno-error=unused-but-set-variable -Wno-format-zero-length   -mno-aes -mno-avx  -std=iso9899:1999 -c /usr/home/kostik/work/DEV/src/sys/ufs/ufs/ufs_vnops.c -o ufs_vnops.o

% readelf -a ../src/sys/amd64/compile/X/modules/usr/home/kostik/work/DEV/src/sys/modules/ufs/ufs_vnops.o | grep GOT | head -3
0000000001f7  004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4
00000000060d  004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4
0000000007c8  004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4

Maybe this should have an additional check for freestanding mode?

readelf -Wr/llvm-readobj -r prints the full name.

The relocation type name is R_X86_64_REX_GOTPCRELX, which is correct (if the definition resides in libc.so) to avoid copy relocations.
Ideally the relevant FreeBSD code should handle the relocation type. It can be handled the same way as R_X86_64_GOTPCREL if no optimization is planned.
-fno-pic programs may use GOT-generating relocations as well (https://maskray.me/blog/2021-08-29-all-about-global-offset-table#undefined%20symbols).

MaskRay added a comment.EditedNov 8 2021, 12:55 PM

Hmm this seems to cause some trouble when building kernels and kernel modules on FreeBSD, which are built in -ffreestanding mode, and then you get weird relocations in kernel modules:

% readelf -a ../src/sys/amd64/compile/X/modules/usr/home/kostik/work/DEV/src/sys/modules/ufs/ufs_vnops.o | grep GOT | head -3
0000000001f7 004a0000002a R_X86_64_REX_GOTP 0000000000000000 stack_chk_guard - 4
00000000060d 004a0000002a R_X86_64_REX_GOTP 0000000000000000
stack_chk_guard - 4
0000000007c8 004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4

Maybe this should have an additional check for freestanding mode?

Hmm, something seems odd here. I don't claim to really understand what is going on or why these relocations are a problem, but it seems bad if there is a requirement for dso_local for correctness.
The way I understand this attribute is that it is useful for optimization only - i.e. it is not required for anything, but if something is actually local to a DSO, a more optimal code sequence is possible. Shouldn't it be conservatively correct to just omit dso_local from everything?

This is also a good choice. Most popular targets (OS x arch) have switched TLS for the stack canary. Targets using the global variable may get a minor code size hit for -fno-pic mode, but that may be acceptable.
In the long term, this choice will also benefit elimination of copy relocations which some folks are fond of.

Hmm this seems to cause some trouble when building kernels and kernel modules on FreeBSD, which are built in -ffreestanding mode, and then you get weird relocations in kernel modules:

@dim , I created https://reviews.llvm.org/D113443 to restricting the logic to PPC64, where original regression was confirmed and proved a safe workaround. With this amend the logic will be the same as the temporary patch added to FreeBSD 14-CURRENT in August.