[ELF] Split RW PT_LOAD on the PT_GNU_RELRO boundary

[MaskRay]

Summary

Based on Peter Collingbourne's suggestion in D56828.

Old: PT_LOAD(PT_GNU_RELRO(.data.rel.ro .bss.rel.ro) .data .bss)
New: PT_LOAD(PT_GNU_RELRO(.data.rel.ro .bss.rel.ro)) PT_LOAD(.data. .bss)

The new layout reflects the runtime memory mappings.
By having two PT_LOAD segments, we can utilize the NOBITS part of the
first PT_LOAD and save bytes for .bss.rel.ro.

.bss.rel.ro is currently small and only used by copy relocations of
symbols in read-only segments, but it can be used for other purposes in
the future, e.g. if a relro section's statically relocated data is all
zeros, we can move it to .bss.rel.ro.

[MaskRay]

.

[MaskRay]

This is independent from D56828.

[pcc]

Looks good modulo tests.

ELF/Writer.cpp

805

This comment should now be moved to line 814.

[MaskRay]

Add tests. Most offsets are off by 56 bytes as we add one more PT_LOAD.

[ruiu]

LGTM. Splitting a PT_LOAD into two should be harmless for this case.

ELF/Writer.cpp

1991

Remove the blank line.

[MaskRay]

Remove the blank line.

[pcc]

Is there a test that shows that the behavior is correct if a .bss.rel.ro is large enough to require another page?

lld/trunk/test/ELF/pack-dyn-relocs-loop.s

12 ↗

(On Diff #190768)

I think you need to change the size of x here so that this test continues to test what it is intended to test.

[MaskRay]

Is there a test that shows that the behavior is correct if a .bss.rel.ro is large enough to require another page?

See https://reviews.llvm.org/D59404

lld/trunk/test/ELF/pack-dyn-relocs-loop.s

12 ↗

(On Diff #190768)

I think you mean https://reviews.llvm.org/D53003

As the description says, it is very sensitive to exact sizes and offsets. Now with the layout change, I'm not sure how to reproduce the oscillation after deleting if (RelocData.size() < OldSize) RelocData.append(OldSize - RelocData.size(), 0);...

I checked the following layout but it didn't fail.

[ 6] .text             PROGBITS        0000000000010000 010000 000000 00  AX  0   0  4                        
[ 7] .dynamic          DYNAMIC         0000000000010000 010000 0000b0 10  WA  4   0  8                        
[ 8] .data             PROGBITS        0000000000020000 020000 000004 00  WA  0   0  1                        
[ 9] x                 PROGBITS        0000000000020004 020004 00fff6 00  WA  0   0  1                        
[10] barr              PROGBITS        000000000002fffa 02fffa 00000a 00  WA  0   0  2                        
[11] foo               PROGBITS        0000000000030004 030004 00000c 00  WA  0   0  1

[MaskRay]

lld/trunk/test/ELF/pack-dyn-relocs-loop.s

12 ↗

(On Diff #190768)

OK, I can use -z norelro to counteract the layout change in D56828. Fixed by https://reviews.llvm.org/rLLD356229

[mgorny]

I know I'm late to the party but this change thoroughly destroyed NetBSD support. The NetBSD loader doesn't support having more than two PT_LOAD sections. -z norosegment helped with that so far but after this change practically everything fails to run.

[MaskRay]

In D58892#1783029, @mgorny wrote:

I know I'm late to the party but this change thoroughly destroyed NetBSD support. The NetBSD loader doesn't support having more than two PT_LOAD sections. -z norosegment helped with that so far but after this change practically everything fails to run.

This seems to be a very serious limitation. I have difficult to understand how such limitation could exist at all, but I think this should be straightforward to fix. I presume you meant --no-rosegment, not -z norosegment. The latter does not exist.

[mgorny]

In D58892#1783042, @MaskRay wrote:

In D58892#1783029, @mgorny wrote:

I know I'm late to the party but this change thoroughly destroyed NetBSD support. The NetBSD loader doesn't support having more than two PT_LOAD sections. -z norosegment helped with that so far but after this change practically everything fails to run.

This seems to be a very serious limitation. I have difficult to understand how such limitation could exist at all, but I think this should be straightforward to fix.

Yes, it is a serious limitation, and no, it's not straightforward to fix. The loader has making a lot of assumptions, especially what PT_LOAD segments to expect and in which order. I've previously made a patch that allowed third segment but it was rejected as apparently 'not doing it the right way'. Almost a year has passed, and I am entirely powerless to fix it.

I presume you meant --no-rosegment, not -z norosegment. The latter does not exist.

Yes, whichever exists, I meant it ;-).

[krytarowski]

We would like to get this PT_LOAD split as a tunable, ignoring whether tests are passed or not when it is disabled.

At some point of time (hopefully sooner than later) we will add a switch in lld that investigates Triplet, retrieves version of the OS and conditionally enables it. This way we will get support for older and newer versions of NetBSD. We want to narrow this knowledge to the linker (in a way like: https://reviews.llvm.org/D70048), rather than spread to GCC in base, several versions of GCC in pkgsrc and Portable C Compiler.

ELF/Writer.cpp

static unsigned getSectionRank(const OutputSection *Sec) {
   } else if (Sec->Type == SHT_PROGBITS) {
     // Make non-executable and non-writable PROGBITS sections (e.g .rodata
     // .eh_frame) closer to .text. They likely contain PC or GOT relative
     // relocations and there could be relocation overflow if other huge sections
     // (.dynstr .dynsym) were placed in between.
     Rank |= RF_RODATA;
   }
 
   // If we got here we know that both A and B are in the same PT_LOAD.
 
   // Place RelRo sections first. After considering SHT_NOBITS below, the
   // ordering is PT_LOAD(PT_GNU_RELRO(.data.rel.ro .bss.rel.ro) | .data .bss),
   // where | marks where page alignment happens. An alternative ordering is
   // PT_LOAD(.data | PT_GNU_RELRO( .data.rel.ro .bss.rel.ro) | .bss), but it may
   // waste more bytes due to 2 alignment places.
   if (!isRelroSection(Sec))
     Rank |= RF_NOT_RELRO;
template <class ELFT> std::vector<PhdrEntry *> Writer<ELFT>::createPhdrs() {
   // Add the first PT_LOAD segment for regular output sections.
   uint64_t Flags = computeFlags(PF_R);
   PhdrEntry *Load = AddHdr(PT_LOAD, Flags);
 
   // Add the headers. We will remove them if they don't fit.
   Load->add(Out::ElfHeader);
   Load->add(Out::ProgramHeaders);
 
+  // PT_GNU_RELRO includes all sections that should be marked as
+  // read-only by dynamic linker after proccessing relocations.
+  // Current dynamic loaders only support one PT_GNU_RELRO PHDR, give
+  // an error message if more than one PT_GNU_RELRO PHDR is required.
+  PhdrEntry *RelRo = make<PhdrEntry>(PT_GNU_RELRO, PF_R);
+  bool InRelroPhdr = false;
+  OutputSection *RelroEnd = nullptr;
+  for (OutputSection *Sec : OutputSections) {
+    if (!needsPtLoad(Sec))
+      continue;
+    if (isRelroSection(Sec)) {
+      InRelroPhdr = true;
+      if (!RelroEnd)
+        RelRo->add(Sec);
+      else
+        error("section: " + Sec->Name + " is not contiguous with other relro" +
+              " sections");
+    } else if (InRelroPhdr) {
+      InRelroPhdr = false;
+      RelroEnd = Sec;
+    }
+  }
+
   for (OutputSection *Sec : OutputSections) {
     if (!(Sec->Flags & SHF_ALLOC))
       break;
     if (!needsPtLoad(Sec))
       continue;
 
     // Segments are contiguous memory regions that has the same attributes
     // (e.g. executable or writable). There is one phdr for each segment.
     // Therefore, we need to create a new phdr when the next section has
     // different flags or is loaded at a discontiguous address or memory
     // region using AT or AT> linker script command, respectively. At the same
     // time, we don't want to create a separate load segment for the headers,
     // even if the first output section has an AT or AT> attribute.
     uint64_t NewFlags = computeFlags(Sec->getPhdrFlags());
     if (((Sec->LMAExpr ||
           (Sec->LMARegion && (Sec->LMARegion != Load->FirstSec->LMARegion))) &&
          Load->LastSec != Out::ProgramHeaders) ||
-        Sec->MemRegion != Load->FirstSec->MemRegion || Flags != NewFlags) {
+        Sec->MemRegion != Load->FirstSec->MemRegion || Flags != NewFlags ||
+        Sec == RelroEnd) {
 
       Load = AddHdr(PT_LOAD, NewFlags);
       Flags = NewFlags;
     }
 
     Load->add(Sec);
   }
 
   // Add a TLS segment if any.
   PhdrEntry *TlsHdr = make<PhdrEntry>(PT_TLS, PF_R);
   for (OutputSection *Sec : OutputSections)
     if (Sec->Flags & SHF_TLS)
       TlsHdr->add(Sec);
   if (TlsHdr->FirstSec)
     Ret.push_back(TlsHdr);
 
   // Add an entry for .dynamic.
   if (OutputSection *Sec = In.Dynamic->getParent())
     AddHdr(PT_DYNAMIC, Sec->getPhdrFlags())->add(Sec);
 
-  // PT_GNU_RELRO includes all sections that should be marked as
-  // read-only by dynamic linker after proccessing relocations.
-  // Current dynamic loaders only support one PT_GNU_RELRO PHDR, give
-  // an error message if more than one PT_GNU_RELRO PHDR is required.
-  PhdrEntry *RelRo = make<PhdrEntry>(PT_GNU_RELRO, PF_R);
-  bool InRelroPhdr = false;
-  bool IsRelroFinished = false;
-  for (OutputSection *Sec : OutputSections) {
-    if (!needsPtLoad(Sec))
-      continue;
-    if (isRelroSection(Sec)) {
-      InRelroPhdr = true;
-      if (!IsRelroFinished)
-        RelRo->add(Sec);
-      else
-        error("section: " + Sec->Name + " is not contiguous with other relro" +
-              " sections");
-    } else if (InRelroPhdr) {
-      InRelroPhdr = false;
-      IsRelroFinished = true;
-    }
-  }
   if (RelRo->FirstSec)
     Ret.push_back(RelRo);
 
   // PT_GNU_EH_FRAME is a special section pointing on .eh_frame_hdr.
   if (!In.EhFrame->empty() && In.EhFrameHdr && In.EhFrame->getParent() &&
       In.EhFrameHdr->getParent())
     AddHdr(PT_GNU_EH_FRAME, In.EhFrameHdr->getParent()->getPhdrFlags())
         ->add(In.EhFrameHdr->getParent());