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

[Alan Modra <amodra@gmail.com>]

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.

-- 
Alan Modra
Australia Development Lab, IBM