[PATCH] elf: Remove the 1-page gap before the RELRO segment

[Fangrui Song]

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