[PATCH] ld: Rewrite lang_size_relro_segment_1

["H.J. Lu" <hjl.tools@gmail.com>]

On Fri, Jan 14, 2022 at 06:42:16PM +1030, Alan Modra 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.
> 

Now I have time to rewrite lang_size_relro_segment_1.  Is it OK for
master?

Thanks.


H.J.
---
1. Find the first section in the relro segment.
2. Find the first non-empty preceding section.
3. Don't add the 1-page gap before the relro segment if the maximum page
size >= the maximum section alignment.
4. Align the relro segment if there is a 1-page gap before the relro
segment.

	PR ld/28743
	* ldlang.c (lang_size_relro_segment_1): Rewrite.
---
 ld/ldlang.c | 103 ++++++++++++++++++++++++----------------------------
 1 file changed, 48 insertions(+), 55 deletions(-)

diff --git a/ld/ldlang.c b/ld/ldlang.c
index 48e40828634..90b4e868a89 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -6370,9 +6370,8 @@ lang_size_segment (seg_align_type *seg)
 static bfd_vma
 lang_size_relro_segment_1 (seg_align_type *seg)
 {
-  bfd_vma relro_end, desired_end;
-  asection *sec, *prev_sec = NULL;
-  bool remove_page_gap = false;
+  bfd_vma relro_end, desired_end, aligned_start = 0;
+  asection *sec, *prev_sec = NULL, *relro_sec = NULL;
   unsigned int max_alignment_power = 0;
 
   /* Compute the expected PT_GNU_RELRO/PT_LOAD segment end.  */
@@ -6382,7 +6381,7 @@ lang_size_relro_segment_1 (seg_align_type *seg)
   /* Adjust by the offset arg of XXX_SEGMENT_RELRO_END.  */
   desired_end = relro_end - seg->relro_offset;
 
-  /* For sections in the relro segment..  */
+  /* Find the first section in the relro segment. */
   for (sec = link_info.output_bfd->section_last; sec; sec = sec->prev)
     if ((sec->flags & SEC_ALLOC) != 0)
       {
@@ -6392,71 +6391,65 @@ lang_size_relro_segment_1 (seg_align_type *seg)
 	if (sec->vma >= seg->base
 	    && sec->vma < seg->relro_end - seg->relro_offset)
 	  {
-	    /* Where do we want to put this section so that it ends as
-	       desired?  */
-	    bfd_vma start, end, bump;
-
-	    end = start = sec->vma;
-	    if (!IS_TBSS (sec))
-	      end += TO_ADDR (sec->size);
-	    bump = desired_end - end;
-	    /* We'd like to increase START by BUMP, but we must heed
-	       alignment so the increase might be less than optimum.  */
-	    start += bump;
-	    start &= ~(((bfd_vma) 1 << sec->alignment_power) - 1);
-	    /* This is now the desired end for the previous section.  */
-	    desired_end = start;
+	    relro_sec = sec;
 	    prev_sec = sec->prev;
 	  }
       }
 
-  seg->phase = exp_seg_relro_adjust;
-  ASSERT (desired_end >= seg->base);
-
-  for (; prev_sec; prev_sec = prev_sec->prev)
-    if ((prev_sec->flags & SEC_ALLOC) != 0)
-      {
-	if (prev_sec->alignment_power > max_alignment_power)
-	  max_alignment_power = prev_sec->alignment_power;
-
-	if (prev_sec->size != 0)
-	  {
-	    /* The 1-page gap before the RELRO segment may be removed.  */
-	    remove_page_gap = ((prev_sec->vma + prev_sec->size
-				+ seg->maxpagesize) < desired_end);
-
-	    break;
-	  }
-      }
-
-  if (remove_page_gap)
+  if (relro_sec)
     {
-      /* Find the maximum section alignment.  */
-      for (sec = prev_sec; sec; sec = sec->prev)
-	if ((sec->flags & SEC_ALLOC) != 0
-	    && sec->alignment_power > max_alignment_power)
-	  max_alignment_power = sec->alignment_power;
+      /* Where do we want to put this section so that it ends as
+	 desired?  */
+      bfd_vma start, end, bump;
+
+      /* Find the first non-empty preceding section.  */
+      for (; prev_sec; prev_sec = prev_sec->prev)
+	if (prev_sec->size != 0 && (prev_sec->flags & SEC_ALLOC) != 0)
+	  break;
 
-      /* Remove the 1-page gap before the RELRO segment only if the
-	 maximum page size >= the maximum section alignment.  */
+      end = start = relro_sec->vma;
+      if (!IS_TBSS (relro_sec))
+	end += TO_ADDR (relro_sec->size);
+      bump = desired_end - end;
       if (seg->maxpagesize >= (1U << max_alignment_power))
 	{
-	  /* 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)
+	  /* Don't add the 1-page gap before the relro segment if the
+	     maximum page size >= the maximum section alignment.  */
+	  if (bump > seg->maxpagesize)
 	    {
-	      desired_end -= seg->maxpagesize;
+	      bump -= seg->maxpagesize;
 	      relro_end -= seg->maxpagesize;
 	    }
-	  else
+	  else if (prev_sec != NULL)
 	    {
-	      desired_end &= ~(seg->maxpagesize - 1);
-	      relro_end &= ~(seg->maxpagesize - 1);
+	      /* Align the relro segment if there is a 1-page gap
+		 before the relro segment.  */
+	      aligned_start = start + bump;
+	      aligned_start &= ~(((bfd_vma) 1 << relro_sec->alignment_power)
+			     - 1);
+	      if ((prev_sec->vma + prev_sec->size + seg->maxpagesize)
+		  < aligned_start)
+		{
+		  aligned_start &= ~(seg->maxpagesize - 1);
+		  relro_end &= ~(seg->maxpagesize - 1);
+		}
 	    }
-	  }
-      }
+	}
+      /* We'd like to increase START by BUMP, but we must heed
+	 alignment so the increase might be less than optimum.  */
+      if (aligned_start != 0)
+	start = aligned_start;
+      else
+	{
+	  start += bump;
+	  start &= ~(((bfd_vma) 1 << relro_sec->alignment_power) - 1);
+	}
+      /* This is now the desired end for the previous section.  */
+      desired_end = start;
+    }
 
+  seg->phase = exp_seg_relro_adjust;
+  ASSERT (aligned_start != 0 || desired_end >= seg->base);
   seg->base = desired_end;
   return relro_end;
 }
-- 
2.34.1