Page MenuHomePhabricator

[ELF] Split RW PT_LOAD on the PT_GNU_RELRO boundary
ClosedPublic

Authored by MaskRay on Sun, Mar 3, 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.

Diff Detail

Repository
rL LLVM

Event Timeline

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

.

MaskRay updated this revision to Diff 189440.Tue, Mar 5, 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.Wed, Mar 13, 5:11 PM

Looks good modulo tests.

ELF/Writer.cpp
805 ↗(On Diff #189440)

This comment should now be moved to line 814.

MaskRay marked an inline comment as done.Wed, Mar 13, 6:45 PM
MaskRay updated this revision to Diff 190570.Wed, Mar 13, 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.Thu, Mar 14, 6:08 PM

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

ELF/Writer.cpp
1990 ↗(On Diff #190570)

Remove the blank line.

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

Remove the blank line.

This revision was automatically updated to reflect the committed changes.
pcc added a comment.Thu, Mar 14, 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

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.Thu, Mar 14, 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

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.Thu, Mar 14, 8:21 PM
MaskRay added inline comments.
lld/trunk/test/ELF/pack-dyn-relocs-loop.s
12

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