ldscript.powerpc*: Keep .data.rel.ro out of PT_DYNAMIC
Needs ReviewPublic
Actions

Authored by mchoo on Sat, Sep 12, 1:55 PM.

Details

Reviewers

jhibbits
adrian
jrtc27
jhb

Group Reviewers

PowerPC

Summary

When .got2 is empty, the orphan .data.rel.ro output section follows
.dynamic and inherits both its :kernel and :dynamic program-header
assignments. This makes PT_DYNAMIC cover .data.rel.ro as well as the
actual dynamic table. On powerpc64le, its resulting size may not even
be a multiple of Elf64_Dyn.

Describe .data.rel.ro explicitly and assign it only to :kernel,
terminating :dynamic inheritance on powerpc, powerpc64, and powerpc64le.

MFC after: 2 weeks
MFC to: stable/14, stable/15
Sponsored by: FreeBSD Foundation, Reliable Computer Systems Lab

Test Plan

readelf -l shows that .data.rel.ro does not appear in 01 (Dynamic) segment after this patch.
ppc64{be,le} boots. Not tested on ppc32.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

No Test Coverage

Build Status

Buildable 77415
Build 74298: arc lint + arc unit

Event Timeline

mchoo created this revision.Sat, Sep 12, 1:55 PM

Herald added a subscriber: imp. · View Herald TranscriptSat, Sep 12, 1:55 PM

mchoo requested review of this revision.Sat, Sep 12, 1:55 PM

Harbormaster completed remote builds in B76801: Diff 186523.Sat, Sep 12, 1:55 PM

mchoo added reviewers: PowerPC, jhibbits, adrian, jrtc27.Sat, Sep 12, 1:55 PM

mchoo added a reviewer: jhb.Sat, Sep 12, 2:01 PM

Would you mind adding a comment explaining this above the section name so we don't forget why ?

Reflect @adrian's suggestion

Harbormaster completed remote builds in B76807: Diff 186535.Sat, Sep 12, 6:35 PM

also just to be clear - did you boot test this on something? :-P :-)

In D59620#1368197, @adrian wrote:

also just to be clear - did you boot test this on something? :-P :-)

Tested on ppc64{be,le} on IBM LC Power 9. I didn't test on ppc32 although I'm pretty sure it will pass.

mchoo edited the summary of this revision. (Show Details)Sat, Sep 12, 11:59 PM

mchoo edited the test plan for this revision. (Show Details)Sun, Sep 13, 5:55 PM

I'm ok with it, let's wait for @jhibbits to chime in too.

This revision is now accepted and ready to land.Sun, Sep 13, 6:18 PM

jhibbits accepted this revision.Sun, Sep 13, 7:11 PM

ping @jhb for mentor approval

edit: Do I need another mentor approval for MFC?

ping @olce for mentor approval

I do not precisely understand what is going on here on the technical side. I have a small consistency question though (see inline comment).

Approved by: olce (mentor)

For MFC, not sure so personally always asked my mentors. MFC by default should be 2 weeks, unless you have some reason to expedite and the risk is very low. I'll approve for 2 weeks, and if you absolutely want something lower, let's just chat about it and/or see what the reviewers have to say.

sys/conf/ldscript.powerpc64le
122	Isn't the comment lacking some details compared to the commit message, which apparently states that `.data.rel.ro` follows `.dynamic` when `.got2` is empty? What happens if `.got2` is not empty?

Update comment

This revision now requires review to proceed.Mon, Sep 28, 3:36 PM

Harbormaster completed remote builds in B77415: Diff 187882.Mon, Sep 28, 3:36 PM

mchoo marked an inline comment as done.Mon, Sep 28, 3:36 PM

mchoo added inline comments.

sys/conf/ldscript.powerpc64le
122	You're right, I missed "when .got2 is empty" condition.

mchoo marked an inline comment as done.Mon, Sep 28, 3:36 PM

I wonder why we have explicit PHDRS in the powerpc ldscripts. None of the other kernel ldscripts do (though amd64's vdso script does for reasons I don't fully understand either, not clear at all why the vdso needs a custom linker script). Maybe @jrtc27 has thoughts. Certainly PT_DYNAMIC should only contain .dynamic and nothing else.

sys/conf/ldscript.powerpc64le
131	Is this for GCC 2.7.2? Yowzers.

The only reason to do have this singular RWX segment would be if whatever's loading the kernel cannot tolerate more than one PT_LOAD. I hope that's really not the case. Manually specifying the program headers really sucks as a thing to do (no PT_NOTE for NT_GNU_BUILD_ID, no PT_GNU_RELRO, no PT_INTERP with /red/herring, and any other helpful things your toolchain might normally add...).