Page MenuHomePhabricator

[ELF][PPC] Allow PT_LOAD to have overlapping p_offset ranges
ClosedPublic

Authored by MaskRay on Jul 18 2019, 12:51 AM.

Details

Summary

This change affects the non-linker script case (precisely, when the
SECTIONS command is not used). It deletes 3 alignments at PT_LOAD
boundaries for the default case: the size of a powerpc64 binary can be
decreased by at most 192kb. The technique can be ported to other
targets.

Let me demonstrate the idea with a maxPageSize=65536 example:

When assigning the address to the first output section of a new PT_LOAD,
if the end p_vaddr of the previous PT_LOAD is 0x10020, we advance to
the next multiple of maxPageSize: 0x20000. The new PT_LOAD will thus
have p_vaddr=0x20000. Because p_offset and p_vaddr are congruent modulo
maxPageSize, p_offset will be 0x20000, leaving a p_offset gap [0x10020,
0x20000) in the output.

Alternatively, if we advance the position to 0x20020, the new PT_LOAD
will have p_vaddr=0x20020. We can pick either 0x10020 or 0x20020 for p_offset!
Obviously 0x10020 is the choice because it leaves no gap.
At runtime, p_vaddr will be rounded down by pagesize
(65536 if pagesize=maxPageSize). This PT_LOAD will load additional
initial contents from p_offset ranges [0x10000,0x10020), which will also
be loaded by the previous PT_LOAD. This is fine if -z noseparate-code is
in effect or if we are not transiting between executable and
non-executable segments.

ld.bfd -z noseparate-code leverages this technique to keep output small.
This patch implements the technique in lld, which is mostly effective on
targets with large defaultMaxPageSize (AArch64/MIPS/PPC: 65536). The 3
removed alignments can save almost 3*65536 bytes.

Two places that rely on p_vaddr%pagesize = 0 have to be updated.

  1. We used to round p_memsz(PT_GNU_RELRO) up to commonPageSize (defaults to 4096 on all targets). Now p_vaddr%commonPageSize may be non-zero. The updated formula takes account of that factor.
  2. Our TP offsets formulae are only correct if p_vaddr%p_align = 0. Fix them. See the updated comments in InputSection.cpp for details.

    On targets that we enable the technique (only PPC64 now), we can potentially make p_vaddr(PT_TLS)%p_align(PT_TLS) != 0 if sh_addralign(.tdata) < sh_addralign(.tbss)

    This exposes many problems in ld.so implementations, especially the offsets of dynamic TLS blocks. Known issues:

    FreeBSD 13.0-CURRENT rtld-elf (i386/amd64/powerpc/arm64) glibc (HEAD) i386 and x86_64 https://sourceware.org/bugzilla/show_bug.cgi?id=24606 musl<=1.1.22 on TLS Variant I architectures (aarch64/powerpc64/...)

    So, force p_vaddr%p_align = 0 by rounding dot up to p_align(PT_TLS).

The technique will be enabled (with updated tests) for other targets in
subsequent patches.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
peter.smith added inline comments.Jul 19 2019, 3:20 AM
ELF/Writer.cpp
2211 ↗(On Diff #210780)

Does this comment need updating. What does page aligned mean now? Moreover does the comment about PT_GNU_RELRO make sense?

2215 ↗(On Diff #210780)

I suggest prev or prevPhdr rather than last, or perhaps lastSeen. At a glance last on its own can imply the final Phdr.

I would like to do a bit more research about RELRO, as I can't see from this patch alone. I think it is fine if RELRO is double mapped into an RO page. However if RELRO is adjacent to RW segments I think it could be a bad idea to have something like

VA [0x10000, 0x10020).data.rel.roPA [0x10000, 0x10020)
VA [0x20020, ...).dataPA [0x10020, ...)

As in theory (I'm not sure about how this works in the OS/loader so I could have this wrong) if the physical contents of .data was mapped RW from 0x10000 -> 0x20000 we'd have an ability to write to the .data.rel.ro via .data.

Is there some other part of the code that prevents this or does some other mechanism in the loader/OS prevent this from happening?

To answer my own question https://sourceware.org/binutils/docs-2.32/ld/Builtin-Functions.html has DATA_SEGMENT_RELRO_END which mentions:

DATA_SEGMENT_ALIGN is padded so that exp + offset is aligned to the commonpagesize argument given to DATA_SEGMENT_ALIGN

There is also the comment in DATA_SEGMENT_ALIGN

commonpagesize should be less or equal to maxpagesize and should be the system page size the object wants to be optimized for while still running on system page sizes up to maxpagesize. Note however that ‘-z relro’ protection will not be effective if the system page size is larger than commonpagesize.

So this implies that if you are on a linux distro with a 64k page size and you want full relro protection you must increase the common page size to match the max page size.

MaskRay updated this revision to Diff 210798.Jul 19 2019, 4:33 AM

Address review comments

MaskRay updated this revision to Diff 210814.Jul 19 2019, 6:14 AM
MaskRay edited the summary of this revision. (Show Details)
This comment was removed by MaskRay.

I would like to do a bit more research about RELRO, as I can't see from this patch alone. I think it is fine if RELRO is double mapped into an RO page.

Yes, the RELRO region may be double mapped.

As in theory (I'm not sure about how this works in the OS/loader so I could have this wrong) if the physical contents of .data was mapped RW from 0x10000 -> 0x20000 we'd have an ability to write to the .data.rel.ro via .data.

This should not be a concern. PT_LOAD segments are mapped with the MAP_PRIVATE flag. The contents are copy-on-write and not shared between two maps:

MAP_PRIVATE
   Create  a  private copy-on-write mapping.  Updates to the mapping are not visible to other pro‐
   cesses mapping the same file, and are not carried  through  to  the  underlying  file.   It  is
   unspecified  whether  changes  made to the file after the mmap() call are visible in the mapped
   region.

To answer my own question https://sourceware.org/binutils/docs-2.32/ld/Builtin-Functions.html has DATA_SEGMENT_RELRO_END which mentions:

There is also the comment in DATA_SEGMENT_ALIGN

Thanks for the reference! I see that document partly answered my point 5 above (https://reviews.llvm.org/D64906#1592854). Their choice is to avoid maxpagesize alignment at the end of PT_GNU_RELRO, but there can still be a commonpagesize alignment at DATA_SEGMENT_ALIGN. The downside is that the last page may not be protected (common case on PPC: commonpagesize=4k, pagesz=maxpagesize=64k).

Since D58892, we have two RW segments: RW(relro) + RW(non-relro). By allowing double mapped RELRO contents, we don't have an alignment. (Our .got and .got.plt are separate - which has been the case for a long time. There seems no issue with it.)

MaskRay marked 3 inline comments as done.Jul 19 2019, 6:17 AM
MaskRay updated this revision to Diff 210956.Jul 20 2019, 5:28 AM
MaskRay edited the summary of this revision. (Show Details)

Align the RW PT_LOAD which includes PT_TLS, and add ppc64-tls-vaddr-align.s

A stage2 check-llvm check-clang check-lld passed before this revision. But
when I test this patch (the x86_64 version) in our internal code base, I
noticed PT_TLS may not satisfy p_vaddr%p_align=0. This update fixes the issue.

MaskRay updated this revision to Diff 210964.Jul 20 2019, 10:04 AM
MaskRay edited the summary of this revision. (Show Details)

Fix TP offset computation if p_vaddr % p_align != 0

MaskRay updated this revision to Diff 211021.Jul 21 2019, 10:51 PM

Fix gcc 8 -Wparentheses

Passed stage 2 check-llvm check-clang check-lld on a powerpc64le machine

MaskRay updated this revision to Diff 211406.Jul 23 2019, 8:41 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay added reviewers: dalias, nsz.

Comment about static TLS blocks Variants 1 and 2.

MaskRay marked an inline comment as done.Jul 23 2019, 8:44 PM
MaskRay added inline comments.
ELF/InputSection.cpp
611 ↗(On Diff #211406)

@peter.smith Moved the comment here because this mostly applies on other Variant I targets (PPC,RISC-V,...).

MaskRay updated this revision to Diff 211410.Jul 23 2019, 9:22 PM

Reword the comment about TLS

MaskRay updated this revision to Diff 212493.Jul 30 2019, 7:47 PM

Add a p_vaddr%p_align = 0 hack to work around some ld.so bugs

MaskRay updated this revision to Diff 212494.Jul 30 2019, 8:02 PM

Edit a comment: FreeBSD rtld-elf amd64 has the same bug as glibc(i386 x86-64) https://sourceware.org/bugzilla/show_bug.cgi?id=24606

D64903 is committed. This patch is ready now. It can delete 3 alignments at PT_LOAD boundaries (because -z noseparate-code is the default): it can decrease the size of a powerpc64 executable/shared object by at most 192kb (it can save more than 96kb on average).

I'm happy to go ahead as this is a pre-requisite for supporting AArch64.

MaskRay updated this revision to Diff 212775.Aug 1 2019, 3:39 AM
MaskRay edited the summary of this revision. (Show Details)

Update description to mention ld.so bugs (why we have to add a workaround)

MaskRay edited the summary of this revision. (Show Details)Aug 1 2019, 3:42 AM
MaskRay updated this revision to Diff 212779.Aug 1 2019, 4:29 AM

Move AArch64 TLS formula here

🥳It would be nice to get this in. Recently I learned that Brandon Bergren at FreeBSD used a local patch to fix the powerpc64 size regression caused by lld:

  // We need 64K pages (at least under glibc/Linux, the loader won't
  // set different permissions on a finer granularity than that).
-  defaultMaxPageSize = 65536;
+ defaultMaxPageSize = 4096;

(It works for them because they use 4k pagesize, but a general fix (this patch) will be preferable.) D64906 is also a prerequisite for aarch64 support, which many more people may want.

It turns out I need the proper fix after all in my local tree, because lately I have been working on getting a working Petitboot loader binary, and that means I'm technically cross compiling code for ppc64le Linux. So yeah, it would be very nice to get this in.

ruiu accepted this revision.Aug 20 2019, 12:48 AM

LGTM

This revision is now accepted and ready to land.Aug 20 2019, 12:48 AM
MaskRay updated this revision to Diff 216062.Aug 20 2019, 1:17 AM

Rebase. 3 ppc tests have changed recently.