Re: [PATCH] PR ld/16322: ld fails to generate GNU_RELRO segment

[Alan Modra <amodra at gmail dot com>]

On Thu, Jan 09, 2014 at 04:08:17AM -0800, H.J. Lu wrote:
> On Wed, Jan 8, 2014 at 8:13 PM, Alan Modra <amodra@gmail.com> wrote:
> > On Wed, Jan 08, 2014 at 06:01:49AM -0800, H.J. Lu wrote:
> >> >> +         /* If PT_LOAD segment doesn't fit PT_GNU_RELRO segment,
> >> >> +            adjust its p_filesz and p_memsz.  */
> >
> > This is broken.  What if there is another LOAD segment following
> > the one you're adjusting?
> 
> It will only happen with a costumer linker script using
> DATA_SEGMENT_ALIGN,  DATA_SEGMENT_RELRO_END,
> DATA_SEGMENT_END.

Right.  Such a script is possible.

>  Do you have a testcase?

No.

> We can
> either ignore relro or issue an error.

Please fix it one way or the other.


> >> >>               if (expld.dataseg.base - (1 << max_alignment_power) < old_base)
> >> >>                 expld.dataseg.base += expld.dataseg.pagesize;
> >> >> -             expld.dataseg.base -= (1 << max_alignment_power);
> >> >> +             /* Properly align base to max_alignment_power.  */
> >> >> +             expld.dataseg.base &= ~((1 << max_alignment_power) - 1);
> >> >>               lang_reset_memory_regions ();
> >> >>               one_lang_size_sections_pass (relax, check_regions);
> >
> > This also doesn't look correct to me.  Please explain why you think
> > this is a good idea.
> 
> There are
> 
>       if (expld.dataseg.relro_end > relro_end)
>         {
>           /* The alignment of sections between DATA_SEGMENT_ALIGN
>              and DATA_SEGMENT_RELRO_END caused huge padding to be
>              inserted at DATA_SEGMENT_RELRO_END.  Try to start a bit lower so
>              that the section alignments will fit in.  */
>           asection *sec;
>           unsigned int max_alignment_power = 0;
> 
>           /* Find maximum alignment power of sections between
>              DATA_SEGMENT_ALIGN and DATA_SEGMENT_RELRO_END.  */
>           for (sec = link_info.output_bfd->sections; sec; sec = sec->next)
>             if (sec->vma >= expld.dataseg.base
>                 && sec->vma < expld.dataseg.relro_end
>                 && sec->alignment_power > max_alignment_power)
>               max_alignment_power = sec->alignment_power;
> 
>           if (((bfd_vma) 1 << max_alignment_power) < expld.dataseg.pagesize)
>             {
>               if (expld.dataseg.base - (1 << max_alignment_power) < old_base)
>                 expld.dataseg.base += expld.dataseg.pagesize;
>               /* Properly align base to max_alignment_power.  */
>               expld.dataseg.base &= ~((1 << max_alignment_power) - 1);
>               lang_reset_memory_regions ();
>               one_lang_size_sections_pass (relax, check_regions);
>             }
>         }
>       link_info.relro_start = expld.dataseg.base;
>       link_info.relro_end = expld.dataseg.relro_end;
> 
> max_alignment_power is the maximum alignment of sections
> between DATA_SEGMENT_ALIGN and DATA_SEGMENT_RELRO_END.
> If expld.dataseg.base (link_info.relro_start) isn't aligned to
> max_alignment_power, some sections won't fit into RELRO
> segment.  If you have a testcase to show it doesn't some
> cases correctly, I will fix it.

I understand well enough what the code is trying to do, but this code
is not obvious and it seems to me that you have not spent the time to
properly understand it before committing unreviewed patches.  That's
why I wanted you to explain your patch.

What caught my eye first is that this adjustment

	      if (expld.dataseg.base - (1 << max_alignment_power) < old_base)
		expld.dataseg.base += expld.dataseg.pagesize;

no longer makes sense after you changed the following lines to

	      /* Properly align base to max_alignment_power.  */
	      expld.dataseg.base &= ~((1 << max_alignment_power) - 1);

That makes your patch wrong, or at least incomplete.  I also think
your comment is worse than useless.

What you need to understand is that the whole point of the relro
adjustment to dataseg.base is to make the *end* of the relro area
page aligned.  That's what the following is trying to do
      expld.dataseg.base += (-expld.dataseg.relro_end
			     & (expld.dataseg.pagesize - 1));

"Properly aligning base" is not what we're doing at all!

We need at least the following fixes..


diff --git a/ld/ldlang.c b/ld/ldlang.c
index 7851615..79b3c4b 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -5362,18 +5362,14 @@ lang_size_sections (bfd_boolean *relax, bfd_boolean check_regions)
       && link_info.relro && expld.dataseg.relro_end)
     {
       /* If DATA_SEGMENT_ALIGN DATA_SEGMENT_RELRO_END pair was seen, try
-	 to put expld.dataseg.relro on a (common) page boundary.  */
-      bfd_vma min_base, old_base, relro_end, maxpage;
+	 to put expld.dataseg.relro_end on a (common) page boundary.  */
+      bfd_vma min_base, relro_end, maxpage;
 
       expld.dataseg.phase = exp_dataseg_relro_adjust;
       maxpage = expld.dataseg.maxpagesize;
       /* MIN_BASE is the absolute minimum address we are allowed to start the
 	 read-write segment (byte before will be mapped read-only).  */
       min_base = (expld.dataseg.min_base + maxpage - 1) & ~(maxpage - 1);
-      /* OLD_BASE is the address for a feasible minimum address which will
-	 still not cause a data overlap inside MAXPAGE causing file offset skip
-	 by MAXPAGE.  */
-      old_base = expld.dataseg.base;
       expld.dataseg.base += (-expld.dataseg.relro_end
 			     & (expld.dataseg.pagesize - 1));
       /* Compute the expected PT_GNU_RELRO segment end.  */
@@ -5405,9 +5401,9 @@ lang_size_sections (bfd_boolean *relax, bfd_boolean check_regions)
 
 	  if (((bfd_vma) 1 << max_alignment_power) < expld.dataseg.pagesize)
 	    {
-	      if (expld.dataseg.base - (1 << max_alignment_power) < old_base)
-		expld.dataseg.base += expld.dataseg.pagesize;
-	      /* Properly align base to max_alignment_power.  */
+	      /* Aligning the adjusted base guarantees the padding between
+		 sections won't change.  FIXME: Doesn't this mean relro end
+		 might no longer be page aligned?  */
 	      expld.dataseg.base &= ~((1 << max_alignment_power) - 1);
 	      lang_reset_memory_regions ();
 	      one_lang_size_sections_pass (relax, check_regions);

-- 
Alan Modra
Australia Development Lab, IBM