From: Fangrui Song <i@maskray.me>
To: Alan Modra <amodra@gmail.com>
Cc: "H.J. Lu" <hjl.tools@gmail.com>, Binutils <binutils@sourceware.org>
Subject: Re: [PATCH] elf: Remove the 1-page gap before the RELRO segment
Date: Fri, 14 Jan 2022 01:37:45 -0800 [thread overview]
Message-ID: <20220114093745.rjjquetgtqtdlu3w@gmail.com> (raw)
In-Reply-To: <YeEwYD4XqfVVxJJ6@squeak.grove.modra.org>
On 2022-01-14, Alan Modra via Binutils wrote:
>On Thu, Jan 13, 2022 at 05:19:22AM -0800, H.J. Lu wrote:
>> On Thu, Jan 13, 2022 at 4:52 AM Alan Modra <amodra@gmail.com> wrote:
>> >
>> > On Mon, Jan 10, 2022 at 06:12:41PM -0800, H.J. Lu via Binutils wrote:
>> > > The existing RELRO scheme may leave a 1-page gap before the RELRO segment
>> > > and align the end of the RELRO segment to the page size:
>> > >
>> > > [18] .eh_frame PROGBITS 408fa0 008fa0 005e80 00 A 0 0 8
>> > > [19] .init_array INIT_ARRAY 410de0 00fde0 000008 08 WA 0 0 8
>> > > [20] .fini_array FINI_ARRAY 410de8 00fde8 000008 08 WA 0 0 8
>> > > [21] .dynamic DYNAMIC 410df0 00fdf0 000200 10 WA 7 0 8
>> > > [22] .got PROGBITS 410ff0 00fff0 000010 08 WA 0 0 8
>> > > [23] .got.plt PROGBITS 411000 010000 000048 08 WA 0 0 8
>> >
>> > Do you know what is going wrong with the relro section layout for this
>> > to occur?
>> >
>> > In this particular case, the end of the read-only segment is at
>> > 0x408fa0 + 0x5e80 = 0x40ee20. My guess is that layout of the
>> > following rw sections starts on the next page plus current offset
>> > within page, the standard choice to minimise disk pages. ie. We start
>> > at 0x40fe20. Then discover that this puts .got.plt at 0x40fe20 + 8 +
>> > 8 + 0x200 + 0x10 = 0x40f040. However, we want this to be on a page
>> > boundary so that the relro segment ends on a page boundary. So we
>> > bump 0x40f040 up to 0x411000 and calculate backwards from there to
>> > arrive at .init_array with a vma of 0x410de0. Resulting in the
>> > 0x40f000 page being unused.
>> >
>> > If instead we start relro layout on the next page, we'd start laying
>> > out at 0x40f000 rather than 0x40fe20. I think that would be the
>>
>> But if the prior ro section size is greater than one page, we want
>> to subtract 1 page:
>>
>> + /* If the preceding section size is greater than the maximum
>> + page size, subtract the maximum page size. Otherwise,
>> + align the RELRO segment to the maximum page size. */
>> + if (prev_sec->size > seg->maxpagesize)
>> + {
>> + desired_end -= seg->maxpagesize;
>> + relro_end -= seg->maxpagesize;
>> + }
>> + else
>> + {
>> + desired_end &= ~(seg->maxpagesize - 1);
>> + relro_end &= ~(seg->maxpagesize - 1);
>> + }
>> + }
>
>The above code is the major reason why I took a dislike to your
>patch. I fully expected you would have rev 2 posted. Why does
>anything depend on the size of a previous section?? That just doesn't
>make sense. And please don't write comments that just say what the
>code is doing. Any half competent programmer can see what the code is
>doing. Write comments that say *why*.
>
>> > correct thing to do rather than fixing up afterwards as your patch
>> > does.
>> >
>>
>> I am checking in my patch as Nick has approved it. There is a
>> possibility that one 1-page gap may be needed to maintain
>> the section alignment. My patch checks it and includes many
>> testcase adjustments to remove one 1-page gap. Can you
>> update the RELRO segment algorithm to make my fixup
>> unnecessary?
>
>Yes, I do think that is possible and desirable. My thinking last
>night was that it ought to be easy too. A one-liner even. Silly me,
>a little experimentation soon showed up a fail of the pr18176
>testcase, demonstrating that there are cases we can save disk space
>without causing gaps.
How about the 2-RW design? |RW(RELRO) |RW(non-RELRO)
The bar | indicates an alignment. Only alignment the start of a
segment, never the end of a segment.
It may avoid the unusual "otherwise DATA_SEGMENT_ALIGN is padded so that
exp + offset is aligned to the commonpagesize argument given to
DATA_SEGMENT_ALIGN" semantics on
https://sourceware.org/binutils/docs/ld/Builtin-Functions.html
I thought about the layout in the past and did not find cases where it
used more pages than the single-RW layout, but I did not think very
hard. (I haven't thought about max-page-size > common-page-size cases.)
next prev parent reply other threads:[~2022-01-14 9:37 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-11 2:12 H.J. Lu
2022-01-11 5:26 ` Fangrui Song
2022-01-13 12:44 ` Nick Clifton
2022-01-13 12:52 ` Alan Modra
2022-01-13 13:19 ` H.J. Lu
2022-01-14 8:12 ` Alan Modra
2022-01-14 9:37 ` Fangrui Song [this message]
2022-01-14 14:58 ` H.J. Lu
2022-01-14 21:55 ` [PATCH] ld: Rewrite lang_size_relro_segment_1 H.J. Lu
2022-01-17 4:08 ` Alan Modra
2022-01-18 4:16 ` [PATCH v2] " H.J. Lu
[not found] ` <CAMe9rOpdkYZDigz8r_oPbweLnaCJUjx3-L-v-vp-70c0MGOHQw@mail.gmail.com>
2022-01-24 16:24 ` Fwd: " Nick Clifton
2022-01-24 21:17 ` [PATCH v3] " H.J. Lu
2022-01-25 15:05 ` [PATCH v4] " H.J. Lu
2022-01-26 10:55 ` Nick Clifton
2022-01-27 0:48 ` Alan Modra
2022-01-27 2:10 ` H.J. Lu
2022-01-29 1:01 ` Alan Modra
2022-01-29 9:06 ` Fangrui Song
2022-01-29 16:45 ` H.J. Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220114093745.rjjquetgtqtdlu3w@gmail.com \
--to=i@maskray.me \
--cc=amodra@gmail.com \
--cc=binutils@sourceware.org \
--cc=hjl.tools@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).