Page MenuHomePhabricator

[ELF] Split RW PT_LOAD on the PT_GNU_RELRO boundary
ClosedPublic

Authored by MaskRay on Mar 3 2019, 10:41 PM.

Details

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.

Event Timeline

MaskRay created this revision.Mar 3 2019, 10:41 PM
MaskRay updated this revision to Diff 189245.Mar 4 2019, 5:33 PM
MaskRay edited the summary of this revision. (Show Details)

.

MaskRay updated this revision to Diff 189440.Mar 5 2019, 7:03 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay added reviewers: ruiu, pcc.

This is independent from D56828.

pcc added a comment.Mar 13 2019, 5:11 PM

Looks good modulo tests.

ELF/Writer.cpp
804–805

This comment should now be moved to line 814.

MaskRay marked an inline comment as done.Mar 13 2019, 6:45 PM
MaskRay updated this revision to Diff 190570.Mar 13 2019, 10:45 PM
MaskRay edited the summary of this revision. (Show Details)

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

ruiu accepted this revision.Mar 14 2019, 6:08 PM

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

ELF/Writer.cpp
1990

Remove the blank line.

This revision is now accepted and ready to land.Mar 14 2019, 6:08 PM
MaskRay updated this revision to Diff 190765.Mar 14 2019, 6:10 PM

Remove the blank line.

This revision was automatically updated to reflect the committed changes.
pcc added a comment.Mar 14 2019, 6:33 PM

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 marked an inline comment as done.Mar 14 2019, 7:52 PM

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 marked an inline comment as done.Mar 14 2019, 8:21 PM
MaskRay added inline comments.
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 added a subscriber: mgorny.Dec 12 2019, 11:03 PM

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.

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.

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 ;-).

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.