Re: [PATCH v4] ld: Rewrite lang_size_relro_segment_1

[Fangrui Song <i@maskray.me>]

On 2022-01-29, Alan Modra via Binutils wrote:
>On Wed, Jan 26, 2022 at 06:10:28PM -0800, H.J. Lu wrote:
>> On Wed, Jan 26, 2022 at 4:48 PM Alan Modra <amodra@gmail.com> wrote:
>> >
>> > On Wed, Jan 26, 2022 at 10:55:35AM +0000, Nick Clifton via Binutils wrote:
>> > > Hi H.J.
>> > >
>> > > > Here is the v4 patch to align the PT_GNU_RELRO segment first and subtract
>> > > > the maximum page size if therer is still a 1-page gap.  This fixes:
>> > > >
>> > > > https://sourceware.org/bugzilla/show_bug.cgi?id=28819
>> > >
>> > > Looks good to me.  Please apply.  Branch and mainline.
>> >
>> > How was this patch tested?  If it has not been tested on targets other
>> > than x86 then it is not appropriate for the branch.  Instead, I
>>
>> What kinds of tests are you looking for?  Do you have a testcase to show
>> there is still an issue?
>
>Can you answer my question?
>
>> > believe commit 2f83249c13d ought to be reverted on the branch.  A
>> > wasted page in memory layout is not an important problem.  We have
>> > more serious relro issues on targets other than x86.  See pr28824.
>> >
>> > I'm speaking up because I raised concerns over the design of the
>> > previous patch, not just the implementation, and had those concerns
>> > overridden by Nick.  Or possibly Nick didn't see my email before the
>> > previous patch was approved.  Regardless of how it happened, I fear
>> > we might have even worse relro support in 2.38.
>> >
>> > FWIW, I also think that any fix I might develop for pr28824 will
>> > likely not be release branch quality until is has been tested for some
>> > time on mainline.
>
>This is the patch I'm playing with.  It reverts your changes to
>lang_size_relro_segment_1 and fixes PR2844.
>
>The end of the PT_GNU_RELRO segment must be aligned to maxpagesize in
>order for the last page in the segment to be write protected after
>relocation.  Unfortunately we currently only align to commonpagesize,
>which works for x86 but no other architecture that actually uses
>maxpagesize memory pages.  I've kept the commonpage alignment for x86,
>other targets get maxpagesize.

Wouldn't aligning to maxpagesize punish the architectures with 64KiB
maxpagesize? These binaries will incur size bloat due the PT_GNU_RELRO
end alignment.

I think my idea of two PF_R|PF_W PT_LOAD will be worth implementing at
some point :) There is no alignment.

The glibc strict check
https://sourceware.org/bugzilla/show_bug.cgi?id=28688 affects glibc<2.35
so I don't think ld making p_align smaller than maxpagesize is
reasonable in the near term, at least before objcopy/strip supporting
the small p_align becomes widespread.
(I need to confess I am not closely subscribed to the sudden p_align
movement in glibc)

>There are of course testsuite changes that are needed, multiple x86
>and one s390x test.  The only fail of any significance is pr18176, but
>there is a fundamental tension between pr18176 and pr28743.  You
>either create holes or you waste disk space, both conditions can't be
>met.  Worse IMO, the pr18176 testcase not only has a hole but also a
>gap in memory layout after the last section in the relro segment.
>That's not ideal for powerpc where sections after .got (the last relro
>section) are in some cases accessed via the got/toc pointer reg.  On
>x86 you'd see a gap between .got and .got.plt when SEPARATE_GOTPLT.
>
>diff --git a/bfd/elfxx-x86.c b/bfd/elfxx-x86.c
>index da8a488db36..c9b3e7c4d9f 100644
>--- a/bfd/elfxx-x86.c
>+++ b/bfd/elfxx-x86.c
>@@ -4205,4 +4205,5 @@ _bfd_elf_linker_x86_set_options (struct bfd_link_info * info,
>     = elf_x86_hash_table (info, bed->target_id);
>   if (htab != NULL)
>     htab->params = params;
>+  info->relro_use_commonpagesize = true;
> }
>diff --git a/include/bfdlink.h b/include/bfdlink.h
>index 69fc9d33ff4..8d5561b0b28 100644
>--- a/include/bfdlink.h
>+++ b/include/bfdlink.h
>@@ -413,6 +413,10 @@ struct bfd_link_info
>   /* TRUE if PT_GNU_RELRO segment should be created.  */
>   unsigned int relro: 1;
>
>+  /* TRUE if the relro segment should be aligned to commonpagesize
>+     rather than maxpagesize.  */
>+  unsigned int relro_use_commonpagesize : 1;
>+
>   /* TRUE if DT_RELR should be enabled for compact relative
>      relocations.  */
>   unsigned int enable_dt_relr: 1;
>diff --git a/ld/ldexp.c b/ld/ldexp.c
>index 5f904aaf8ae..65618912043 100644
>--- a/ld/ldexp.c
>+++ b/ld/ldexp.c
>@@ -469,7 +469,8 @@ fold_segment_align (seg_align_type *seg, etree_value_type *lhs)
> 	}
>       else
> 	{
>-	  expld.result.value += expld.dot & (maxpage - 1);
>+	  if (!link_info.relro)
>+	    expld.result.value += expld.dot & (maxpage - 1);
> 	  if (seg->phase == exp_seg_done)
> 	    {
> 	      /* OK.  */
>@@ -480,6 +481,10 @@ fold_segment_align (seg_align_type *seg, etree_value_type *lhs)
> 	      seg->base = expld.result.value;
> 	      seg->pagesize = commonpage;
> 	      seg->maxpagesize = maxpage;
>+	      if (link_info.relro_use_commonpagesize)
>+		seg->relropagesize = commonpage;
>+	      else
>+		seg->relropagesize = maxpage;
> 	      seg->relro_end = 0;
> 	    }
> 	  else
>@@ -508,10 +513,10 @@ fold_segment_relro_end (seg_align_type *seg, etree_value_type *lhs)
> 	seg->relro_end = lhs->value + expld.result.value;
>
>       if (seg->phase == exp_seg_relro_adjust
>-	  && (seg->relro_end & (seg->pagesize - 1)))
>+	  && (seg->relro_end & (seg->relropagesize - 1)))
> 	{
>-	  seg->relro_end += seg->pagesize - 1;
>-	  seg->relro_end &= ~(seg->pagesize - 1);
>+	  seg->relro_end += seg->relropagesize - 1;
>+	  seg->relro_end &= ~(seg->relropagesize - 1);
> 	  expld.result.value = seg->relro_end - expld.result.value;
> 	}
>       else
>diff --git a/ld/ldexp.h b/ld/ldexp.h
>index ac4fa7e82b0..f1701b66330 100644
>--- a/ld/ldexp.h
>+++ b/ld/ldexp.h
>@@ -136,7 +136,8 @@ enum relro_enum {
> typedef struct {
>   enum phase_enum phase;
>
>-  bfd_vma base, relro_offset, relro_end, end, pagesize, maxpagesize;
>+  bfd_vma base, relro_offset, relro_end, end;
>+  bfd_vma pagesize, maxpagesize, relropagesize;
>
>   enum relro_enum relro;
>
>diff --git a/ld/ldlang.c b/ld/ldlang.c
>index 5dd3df12a0f..1a09b988fbe 100644
>--- a/ld/ldlang.c
>+++ b/ld/ldlang.c
>@@ -6370,101 +6370,40 @@ lang_size_segment (seg_align_type *seg)
> static bfd_vma
> lang_size_relro_segment_1 (seg_align_type *seg)
> {
>-  bfd_vma relro_end, desired_relro_base;
>-  asection *sec, *relro_sec = NULL;
>-  unsigned int max_alignment_power = 0;
>-  bool seen_reloc_section = false;
>-  bool desired_relro_base_reduced = false;
>+  bfd_vma relro_end, desired_end;
>+  asection *sec;
>
>   /* Compute the expected PT_GNU_RELRO/PT_LOAD segment end.  */
>-  relro_end = ((seg->relro_end + seg->pagesize - 1)
>-	       & ~(seg->pagesize - 1));
>+  relro_end = (seg->relro_end + seg->relropagesize - 1) & -seg->relropagesize;
>
>   /* Adjust by the offset arg of XXX_SEGMENT_RELRO_END.  */
>-  desired_relro_base = relro_end - seg->relro_offset;
>+  desired_end = relro_end - seg->relro_offset;
>
>-  /* For sections in the relro segment.  */
>+  /* For sections in the relro segment..  */
>   for (sec = link_info.output_bfd->section_last; sec; sec = sec->prev)
>-    if ((sec->flags & SEC_ALLOC) != 0)
>+    if ((sec->flags & SEC_ALLOC) != 0
>+	&& sec->vma >= seg->base
>+	&& sec->vma < seg->relro_end - seg->relro_offset)
>       {
>-	/* Record the maximum alignment for all sections starting from
>-	   the relro segment.  */
>-	if (sec->alignment_power > max_alignment_power)
>-	  max_alignment_power = sec->alignment_power;
>-
>-	if (sec->vma >= seg->base
>-	    && sec->vma < seg->relro_end - seg->relro_offset)
>-	  {
>-	    /* Where do we want to put the relro section so that the
>-	       relro segment ends on the page bounary?  */
>-	    bfd_vma start, end, bump;
>-
>-	    end = start = sec->vma;
>-	    if (!IS_TBSS (sec))
>-	      end += TO_ADDR (sec->size);
>-	    bump = desired_relro_base - 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_relro_base = start;
>-	    relro_sec = sec;
>-	    seen_reloc_section = true;
>-	  }
>-	else if (seen_reloc_section)
>-	  {
>-	    /* Stop searching if we see a non-relro section after seeing
>-	       relro sections.  */
>-	    break;
>-	  }
>+	/* 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;
>       }
>
>-  if (relro_sec != NULL
>-      && seg->maxpagesize >= (1U << max_alignment_power))
>-    {
>-      asection *prev_sec;
>-      bfd_vma prev_sec_end_plus_1_page;
>-
>-       /* Find the first preceding load section.  */
>-      for (prev_sec = relro_sec->prev;
>-	   prev_sec != NULL;
>-	   prev_sec = prev_sec->prev)
>-	if ((prev_sec->flags & SEC_ALLOC) != 0)
>-	  break;
>-
>-      prev_sec_end_plus_1_page = (prev_sec->vma + prev_sec->size
>-				  + seg->maxpagesize);
>-      if (prev_sec_end_plus_1_page < desired_relro_base)
>-	{
>-	  bfd_vma aligned_relro_base;
>-
>-	  desired_relro_base_reduced = true;
>-
>-	  /* Don't add the 1-page gap before the relro segment.  Align
>-	     the relro segment first.  */
>-	  aligned_relro_base = (desired_relro_base
>-				 & ~(seg->maxpagesize - 1));
>-	  if (prev_sec_end_plus_1_page < aligned_relro_base)
>-	    {
>-	      /* Subtract the maximum page size if therer is still a
>-		 1-page gap.  */
>-	      desired_relro_base -= seg->maxpagesize;
>-	      relro_end -= seg->maxpagesize;
>-	    }
>-	  else
>-	    {
>-	      /* Align the relro segment.  */
>-	      desired_relro_base = aligned_relro_base;
>-	      relro_end &= ~(seg->maxpagesize - 1);
>-	    }
>-	}
>-    }
>-
>   seg->phase = exp_seg_relro_adjust;
>-  ASSERT (desired_relro_base_reduced
>-	  || desired_relro_base >= seg->base);
>-  seg->base = desired_relro_base;
>+  ASSERT (desired_end >= seg->base);
>+  seg->base = desired_end;
>   return relro_end;
> }
>
>
>-- 
>Alan Modra
>Australia Development Lab, IBM