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

public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed

* [PATCH] elf: Remove the 1-page gap before the RELRO segment
@ 2022-01-11  2:12 H.J. Lu
  2022-01-11  5:26 ` Fangrui Song
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: H.J. Lu @ 2022-01-11  2:12 UTC (permalink / raw)
  To: binutils

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

Instead, we can remove the 1-page gap if the maximum page size >= the
maximum section alignment:

  [18] .eh_frame    PROGBITS    408fa0 008fa0 005e80 00   A  0   0  8
  [19] .init_array  INIT_ARRAY  40fde0 00fde0 000008 08  WA  0   0  8
  [20] .fini_array  FINI_ARRAY  40fde8 00fde8 000008 08  WA  0   0  8
  [21] .dynamic     DYNAMIC     40fdf0 00fdf0 000200 10  WA  7   0  8
  [22] .got         PROGBITS    40fff0 00fff0 000010 08  WA  0   0  8
  [23] .got.plt     PROGBITS    410000 010000 000048 08  WA  0   0  8

Because the end of the RELRO segment is always aligned to the page size
and may not be moved, the RELRO segment size may be increased:

  [ 3] .dynstr      STRTAB      000148 000148 000001 00   A  0   0  1
  [ 4] .eh_frame    PROGBITS    000150 000150 000000 00   A  0   0  8
  [ 5] .init_array  INIT_ARRAY  200150 000150 000010 08  WA  0   0  1
  [ 6] .fini_array  FINI_ARRAY  200160 000160 000010 08  WA  0   0  1
  [ 7] .jcr         PROGBITS    200170 000170 000008 00  WA  0   0  1
  [ 8] .data.rel.ro PROGBITS    200180 000180 000020 00  WA  0   0 16
  [ 9] .dynamic     DYNAMIC     2001a0 0001a0 0001c0 10  WA  3   0  8
  [10] .got         PROGBITS    200360 000360 0002a8 00  WA  0   0  8
  [11] .bss         NOBITS      201000 000608 000840 00  WA  0   0  1

vs the old section layout:

  [ 3] .dynstr      STRTAB      000148 000148 000001 00   A  0   0  1
  [ 4] .eh_frame    PROGBITS    000150 000150 000000 00   A  0   0  8
  [ 5] .init_array  INIT_ARRAY  200b48 000b48 000010 08  WA  0   0  1
  [ 6] .fini_array  FINI_ARRAY  200b58 000b58 000010 08  WA  0   0  1
  [ 7] .jcr         PROGBITS    200b68 000b68 000008 00  WA  0   0  1
  [ 8] .data.rel.ro PROGBITS    200b70 000b70 000020 00  WA  0   0 16
  [ 9] .dynamic     DYNAMIC     200b90 000b90 0001c0 10  WA  3   0  8
  [10] .got         PROGBITS    200d50 000d50 0002a8 00  WA  0   0  8
  [11] .bss         NOBITS      201000 000ff8 000840 00  WA  0   0  1

But there is no 1-page gap.

	PR ld/28743
	* ldlang.c (lang_size_relro_segment_1): Remove the 1-page gap
	before the RELRO segment if the maximum page size >= the maximum
	section alignment.
	* testsuite/ld-i386/pr20830.d: Adjusted.
	* testsuite/ld-s390/gotreloc_64-relro-1.dd: Likewise.
	* testsuite/ld-x86-64/pr14207.d: Likewise.
	* testsuite/ld-x86-64/pr18176.d: Likewise.
	* testsuite/ld-x86-64/pr20830a-now.d: Likewise.
	* testsuite/ld-x86-64/pr20830a.d: Likewise.
	* testsuite/ld-x86-64/pr20830b-now.d: Likewise.
	* testsuite/ld-x86-64/pr20830b.d: Likewise.
	* testsuite/ld-x86-64/pr21038a-now.d: Likewise.
	* testsuite/ld-x86-64/pr21038a.d: Likewise.
	* testsuite/ld-x86-64/pr21038b-now.d: Likewise.
	* testsuite/ld-x86-64/pr21038c-now.d: Likewise.
	* testsuite/ld-x86-64/pr21038c.d: Likewise.
---
 ld/ldlang.c                                 | 89 ++++++++++++++++-----
 ld/testsuite/ld-i386/pr20830.d              |  4 +-
 ld/testsuite/ld-s390/gotreloc_64-relro-1.dd |  6 +-
 ld/testsuite/ld-x86-64/pr14207.d            |  6 +-
 ld/testsuite/ld-x86-64/pr18176.d            |  2 +-
 ld/testsuite/ld-x86-64/pr20830a-now.d       |  8 +-
 ld/testsuite/ld-x86-64/pr20830a.d           |  4 +-
 ld/testsuite/ld-x86-64/pr20830b-now.d       | 10 +--
 ld/testsuite/ld-x86-64/pr20830b.d           |  6 +-
 ld/testsuite/ld-x86-64/pr21038a-now.d       |  8 +-
 ld/testsuite/ld-x86-64/pr21038a.d           |  4 +-
 ld/testsuite/ld-x86-64/pr21038b-now.d       |  6 +-
 ld/testsuite/ld-x86-64/pr21038c-now.d       | 10 +--
 ld/testsuite/ld-x86-64/pr21038c.d           |  4 +-
 14 files changed, 110 insertions(+), 57 deletions(-)

diff --git a/ld/ldlang.c b/ld/ldlang.c
index 9dbc8752f87..4a1dc19e100 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -6361,7 +6361,9 @@ static bfd_vma
 lang_size_relro_segment_1 (seg_align_type *seg)
 {
   bfd_vma relro_end, desired_end;
-  asection *sec;
+  asection *sec, *prev_sec = NULL;
+  bool remove_page_gap = false;
+  unsigned int max_alignment_power = 0;
 
   /* Compute the expected PT_GNU_RELRO/PT_LOAD segment end.  */
   relro_end = ((seg->relro_end + seg->pagesize - 1)
@@ -6372,28 +6374,79 @@ lang_size_relro_segment_1 (seg_align_type *seg)
 
   /* For sections in the relro segment..  */
   for (sec = link_info.output_bfd->section_last; sec; sec = sec->prev)
-    if ((sec->flags & SEC_ALLOC) != 0
-	&& sec->vma >= seg->base
-	&& sec->vma < seg->relro_end - seg->relro_offset)
+    if ((sec->flags & SEC_ALLOC) != 0)
       {
-	/* 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 (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 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;
+	    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)
+    {
+      /* 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;
+
+      /* Remove the 1-page gap before the RELRO segment only if the
+	 maximum page size >= the maximum section alignment.  */
+      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)
+	    {
+	      desired_end -= seg->maxpagesize;
+	      relro_end -= seg->maxpagesize;
+	    }
+	  else
+	    {
+	      desired_end &= ~(seg->maxpagesize - 1);
+	      relro_end &= ~(seg->maxpagesize - 1);
+	    }
+	  }
+      }
+
   seg->base = desired_end;
   return relro_end;
 }
diff --git a/ld/testsuite/ld-i386/pr20830.d b/ld/testsuite/ld-i386/pr20830.d
index 8a14a6087a1..f1e37336733 100644
--- a/ld/testsuite/ld-i386/pr20830.d
+++ b/ld/testsuite/ld-i386/pr20830.d
@@ -49,12 +49,12 @@ Disassembly of section .plt:
 Disassembly of section .plt.got:
 
 0+120 <func@plt>:
- +[a-f0-9]+:	ff a3 fc ff ff ff    	jmp    \*-0x4\(%ebx\)
+ +[a-f0-9]+:	ff a3 78 f0 ff ff    	jmp    \*-0xf88\(%ebx\)
  +[a-f0-9]+:	66 90                	xchg   %ax,%ax
 
 Disassembly of section .text:
 
 0+128 <foo>:
  +[a-f0-9]+:	e8 f3 ff ff ff       	call   120 <func@plt>
- +[a-f0-9]+:	8b 83 fc ff ff ff    	mov    -0x4\(%ebx\),%eax
+ +[a-f0-9]+:	8b 83 78 f0 ff ff    	mov    -0xf88\(%ebx\),%eax
 #pass
diff --git a/ld/testsuite/ld-s390/gotreloc_64-relro-1.dd b/ld/testsuite/ld-s390/gotreloc_64-relro-1.dd
index 64151d10a7c..5a107465be2 100644
--- a/ld/testsuite/ld-s390/gotreloc_64-relro-1.dd
+++ b/ld/testsuite/ld-s390/gotreloc_64-relro-1.dd
@@ -5,8 +5,8 @@ Disassembly of section .text:
 .* <foo>:
 .*:	c0 10 00 00 0f 0c [	 ]*larl	%r1,2000 <bar>
 .*:	c0 10 00 00 0f 09 [	 ]*larl	%r1,2000 <bar>
-.*:	c4 1d 00 00 0f 02 [	 ]*lrl	%r1,1ff8 <_GLOBAL_OFFSET_TABLE_\+0x28>
+.*:	c4 1d 00 00 07 8a [	 ]*lrl	%r1,1108 <_GLOBAL_OFFSET_TABLE_\+0x28>
 .*:	58 10 c0 28 [	 ]*l	%r1,40\(%r12\)
 .*:	e3 10 c0 28 00 58 [	 ]*ly	%r1,40\(%r12\)
-.*:	c4 18 00 00 0e f6 [	 ]*lgrl	%r1,1ff0 <_GLOBAL_OFFSET_TABLE_\+0x20>
-.*:	c4 18 00 00 0e ef [	 ]*lgrl	%r1,1fe8 <_GLOBAL_OFFSET_TABLE_\+0x18>
+.*:	c4 18 00 00 07 7e [	 ]*lgrl	%r1,1100 <_GLOBAL_OFFSET_TABLE_\+0x20>
+.*:	c4 18 00 00 07 77 [	 ]*lgrl	%r1,10f8 <_GLOBAL_OFFSET_TABLE_\+0x18>
diff --git a/ld/testsuite/ld-x86-64/pr14207.d b/ld/testsuite/ld-x86-64/pr14207.d
index f330600b916..c06755f96d9 100644
--- a/ld/testsuite/ld-x86-64/pr14207.d
+++ b/ld/testsuite/ld-x86-64/pr14207.d
@@ -11,9 +11,9 @@ There are 4 program headers, starting at offset 64
 Program Headers:
   Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
   LOAD           0x000000 0x0000000000000000 0x0000000000000000 0x000150 0x000150 R   0x200000
-  LOAD           0x000b.8 0x0000000000200b.8 0x0000000000200b.8 0x0004.0 0x000c.8 RW  0x200000
-  DYNAMIC        0x000b.0 0x0000000000200b.0 0x0000000000200b.0 0x0001.0 0x0001.0 RW  0x8
-  GNU_RELRO      0x000b.8 0x0000000000200b.8 0x0000000000200b.8 0x0004.0 0x0004.8 R   0x1
+  LOAD           0x000150 0x0000000000200150 0x0000000000200150 0x0004b8 0x0016f0 RW  0x200000
+  DYNAMIC        0x0001a0 0x00000000002001a0 0x00000000002001a0 0x0001c0 0x0001c0 RW  0x8
+  GNU_RELRO      0x000150 0x0000000000200150 0x0000000000200150 0x0004b8 0x000eb0 R   0x1
 
  Section to Segment mapping:
   Segment Sections...
diff --git a/ld/testsuite/ld-x86-64/pr18176.d b/ld/testsuite/ld-x86-64/pr18176.d
index 4e3ad9ff08d..3ff34fad6cb 100644
--- a/ld/testsuite/ld-x86-64/pr18176.d
+++ b/ld/testsuite/ld-x86-64/pr18176.d
@@ -5,5 +5,5 @@
 #target: x86_64-*-linux*
 
 #...
-  GNU_RELRO      0x04bd17 0x000000000024bd17 0x000000000024bd17 0x0022e9 0x0022e9 R   0x1
+  GNU_RELRO      0x04bcc7 0x000000000024bcc7 0x000000000024bcc7 0x002339 0x002339 R   0x1
 #pass
diff --git a/ld/testsuite/ld-x86-64/pr20830a-now.d b/ld/testsuite/ld-x86-64/pr20830a-now.d
index 4f284c44a40..b8fa6acd1d4 100644
--- a/ld/testsuite/ld-x86-64/pr20830a-now.d
+++ b/ld/testsuite/ld-x86-64/pr20830a-now.d
@@ -50,19 +50,19 @@ Contents of the .eh_frame section:
 Disassembly of section .plt:
 
 0+1b0 <.plt>:
- +[a-f0-9]+:	ff 35 32 0e 20 00    	push   0x200e32\(%rip\)        # 200fe8 <_GLOBAL_OFFSET_TABLE_\+0x8>
- +[a-f0-9]+:	ff 25 34 0e 20 00    	jmp    \*0x200e34\(%rip\)        # 200ff0 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ +[a-f0-9]+:	ff 35 aa 01 20 00    	push   0x2001aa\(%rip\)        # 200360 <_GLOBAL_OFFSET_TABLE_\+0x8>
+ +[a-f0-9]+:	ff 25 ac 01 20 00    	jmp    \*0x2001ac\(%rip\)        # 200368 <_GLOBAL_OFFSET_TABLE_\+0x10>
  +[a-f0-9]+:	0f 1f 40 00          	nopl   0x0\(%rax\)
 
 Disassembly of section .plt.got:
 
 0+1c0 <func@plt>:
- +[a-f0-9]+:	ff 25 32 0e 20 00    	jmp    \*0x200e32\(%rip\)        # 200ff8 <func>
+ +[a-f0-9]+:	ff 25 aa 01 20 00    	jmp    \*0x2001aa\(%rip\)        # 200370 <func>
  +[a-f0-9]+:	66 90                	xchg   %ax,%ax
 
 Disassembly of section .text:
 
 0+1c8 <foo>:
  +[a-f0-9]+:	e8 f3 ff ff ff       	call   1c0 <func@plt>
- +[a-f0-9]+:	48 8b 05 24 0e 20 00 	mov    0x200e24\(%rip\),%rax        # 200ff8 <func>
+ +[a-f0-9]+:	48 8b 05 9c 01 20 00 	mov    0x20019c\(%rip\),%rax        # 200370 <func>
 #pass
diff --git a/ld/testsuite/ld-x86-64/pr20830a.d b/ld/testsuite/ld-x86-64/pr20830a.d
index 615b59fd5de..5c16652c4a8 100644
--- a/ld/testsuite/ld-x86-64/pr20830a.d
+++ b/ld/testsuite/ld-x86-64/pr20830a.d
@@ -57,12 +57,12 @@ Disassembly of section .plt:
 Disassembly of section .plt.got:
 
 0+1c0 <func@plt>:
- +[a-f0-9]+:	ff 25 32 0e 20 00    	jmp    \*0x200e32\(%rip\)        # 200ff8 <func>
+ +[a-f0-9]+:	ff 25 72 01 20 00    	jmp    \*0x200172\(%rip\)        # 200338 <func>
  +[a-f0-9]+:	66 90                	xchg   %ax,%ax
 
 Disassembly of section .text:
 
 0+1c8 <foo>:
  +[a-f0-9]+:	e8 f3 ff ff ff       	call   1c0 <func@plt>
- +[a-f0-9]+:	48 8b 05 24 0e 20 00 	mov    0x200e24\(%rip\),%rax        # 200ff8 <func>
+ +[a-f0-9]+:	48 8b 05 64 01 20 00 	mov    0x200164\(%rip\),%rax        # 200338 <func>
 #pass
diff --git a/ld/testsuite/ld-x86-64/pr20830b-now.d b/ld/testsuite/ld-x86-64/pr20830b-now.d
index 7c7f2f928ac..0c4f2ada3dd 100644
--- a/ld/testsuite/ld-x86-64/pr20830b-now.d
+++ b/ld/testsuite/ld-x86-64/pr20830b-now.d
@@ -1,4 +1,4 @@
-#name: PR ld/20830 (.plt.got, -z now)
+#name: PR ld/20830 x32 (.plt.got, -z now)
 #source: pr20830.s
 #as: --x32
 #ld: -z now -melf32_x86_64 -shared -z relro --ld-generated-unwind-info --hash-style=sysv -z max-page-size=0x200000 -z noseparate-code
@@ -42,19 +42,19 @@ Contents of the .eh_frame section:
 Disassembly of section .plt:
 
 0+120 <.plt>:
- +[a-f0-9]+:	ff 35 c2 0e 20 00    	push   0x200ec2\(%rip\)        # 200fe8 <_GLOBAL_OFFSET_TABLE_\+0x8>
- +[a-f0-9]+:	ff 25 c4 0e 20 00    	jmp    \*0x200ec4\(%rip\)        # 200ff0 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ +[a-f0-9]+:	ff 35 12 01 20 00    	push   0x200112\(%rip\)        # 200238 <_GLOBAL_OFFSET_TABLE_\+0x8>
+ +[a-f0-9]+:	ff 25 14 01 20 00    	jmp    \*0x200114\(%rip\)        # 200240 <_GLOBAL_OFFSET_TABLE_\+0x10>
  +[a-f0-9]+:	0f 1f 40 00          	nopl   0x0\(%rax\)
 
 Disassembly of section .plt.got:
 
 0+130 <func@plt>:
- +[a-f0-9]+:	ff 25 c2 0e 20 00    	jmp    \*0x200ec2\(%rip\)        # 200ff8 <func>
+ +[a-f0-9]+:	ff 25 12 01 20 00    	jmp    \*0x200112\(%rip\)        # 200248 <func>
  +[a-f0-9]+:	66 90                	xchg   %ax,%ax
 
 Disassembly of section .text:
 
 0+138 <foo>:
  +[a-f0-9]+:	e8 f3 ff ff ff       	call   130 <func@plt>
- +[a-f0-9]+:	48 8b 05 b4 0e 20 00 	mov    0x200eb4\(%rip\),%rax        # 200ff8 <func>
+ +[a-f0-9]+:	48 8b 05 04 01 20 00 	mov    0x200104\(%rip\),%rax        # 200248 <func>
 #pass
diff --git a/ld/testsuite/ld-x86-64/pr20830b.d b/ld/testsuite/ld-x86-64/pr20830b.d
index 3c5d42b9c43..6f4f22ca960 100644
--- a/ld/testsuite/ld-x86-64/pr20830b.d
+++ b/ld/testsuite/ld-x86-64/pr20830b.d
@@ -1,4 +1,4 @@
-#name: PR ld/20830 (.plt.got)
+#name: PR ld/20830 x32 (.plt.got)
 #source: pr20830.s
 #as: --x32
 #ld: -melf32_x86_64 -shared -z relro --ld-generated-unwind-info --hash-style=sysv -z max-page-size=0x200000 -z noseparate-code
@@ -49,12 +49,12 @@ Disassembly of section .plt:
 Disassembly of section .plt.got:
 
 0+130 <func@plt>:
- +[a-f0-9]+:	ff 25 c2 0e 20 00    	jmp    \*0x200ec2\(%rip\)        # 200ff8 <func>
+ +[a-f0-9]+:	ff 25 ea 00 20 00    	jmp    \*0x2000ea\(%rip\)        # 200220 <func>
  +[a-f0-9]+:	66 90                	xchg   %ax,%ax
 
 Disassembly of section .text:
 
 0+138 <foo>:
  +[a-f0-9]+:	e8 f3 ff ff ff       	call   130 <func@plt>
- +[a-f0-9]+:	48 8b 05 b4 0e 20 00 	mov    0x200eb4\(%rip\),%rax        # 200ff8 <func>
+ +[a-f0-9]+:	48 8b 05 dc 00 20 00 	mov    0x2000dc\(%rip\),%rax        # 200220 <func>
 #pass
diff --git a/ld/testsuite/ld-x86-64/pr21038a-now.d b/ld/testsuite/ld-x86-64/pr21038a-now.d
index 1653a68ff8a..c05800962be 100644
--- a/ld/testsuite/ld-x86-64/pr21038a-now.d
+++ b/ld/testsuite/ld-x86-64/pr21038a-now.d
@@ -50,19 +50,19 @@ Contents of the .eh_frame section:
 Disassembly of section .plt:
 
 0+1b0 <.plt>:
- +[a-f0-9]+:	ff 35 32 0e 20 00    	push   0x200e32\(%rip\)        # 200fe8 <_GLOBAL_OFFSET_TABLE_\+0x8>
- +[a-f0-9]+:	f2 ff 25 33 0e 20 00 	bnd jmp \*0x200e33\(%rip\)        # 200ff0 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ +[a-f0-9]+:	ff 35 aa 01 20 00    	push   0x2001aa\(%rip\)        # 200360 <_GLOBAL_OFFSET_TABLE_\+0x8>
+ +[a-f0-9]+:	f2 ff 25 ab 01 20 00 	bnd jmp \*0x2001ab\(%rip\)        # 200368 <_GLOBAL_OFFSET_TABLE_\+0x10>
  +[a-f0-9]+:	0f 1f 00             	nopl   \(%rax\)
 
 Disassembly of section .plt.got:
 
 0+1c0 <func@plt>:
- +[a-f0-9]+:	f2 ff 25 31 0e 20 00 	bnd jmp \*0x200e31\(%rip\)        # 200ff8 <func>
+ +[a-f0-9]+:	f2 ff 25 a9 01 20 00 	bnd jmp \*0x2001a9\(%rip\)        # 200370 <func>
  +[a-f0-9]+:	90                   	nop
 
 Disassembly of section .text:
 
 0+1c8 <foo>:
  +[a-f0-9]+:	e8 f3 ff ff ff       	call   1c0 <func@plt>
- +[a-f0-9]+:	48 8b 05 24 0e 20 00 	mov    0x200e24\(%rip\),%rax        # 200ff8 <func>
+ +[a-f0-9]+:	48 8b 05 9c 01 20 00 	mov    0x20019c\(%rip\),%rax        # 200370 <func>
 #pass
diff --git a/ld/testsuite/ld-x86-64/pr21038a.d b/ld/testsuite/ld-x86-64/pr21038a.d
index 6ef8db254e9..e0c3c7cf63f 100644
--- a/ld/testsuite/ld-x86-64/pr21038a.d
+++ b/ld/testsuite/ld-x86-64/pr21038a.d
@@ -56,12 +56,12 @@ Disassembly of section .plt:
 Disassembly of section .plt.got:
 
 0+1c0 <func@plt>:
- +[a-f0-9]+:	f2 ff 25 31 0e 20 00 	bnd jmp \*0x200e31\(%rip\)        # 200ff8 <func>
+ +[a-f0-9]+:	f2 ff 25 71 01 20 00 	bnd jmp \*0x200171\(%rip\)        # 200338 <func>
  +[a-f0-9]+:	90                   	nop
 
 Disassembly of section .text:
 
 0+1c8 <foo>:
  +[a-f0-9]+:	e8 f3 ff ff ff       	call   1c0 <func@plt>
- +[a-f0-9]+:	48 8b 05 24 0e 20 00 	mov    0x200e24\(%rip\),%rax        # 200ff8 <func>
+ +[a-f0-9]+:	48 8b 05 64 01 20 00 	mov    0x200164\(%rip\),%rax        # 200338 <func>
 #pass
diff --git a/ld/testsuite/ld-x86-64/pr21038b-now.d b/ld/testsuite/ld-x86-64/pr21038b-now.d
index c042b6cf702..519c5a3e957 100644
--- a/ld/testsuite/ld-x86-64/pr21038b-now.d
+++ b/ld/testsuite/ld-x86-64/pr21038b-now.d
@@ -50,8 +50,8 @@ Contents of the .eh_frame section:
 Disassembly of section .plt:
 
 0+1b0 <.plt>:
- +[a-f0-9]+:	ff 35 32 0e 20 00    	push   0x200e32\(%rip\)        # 200fe8 <_GLOBAL_OFFSET_TABLE_\+0x8>
- +[a-f0-9]+:	f2 ff 25 33 0e 20 00 	bnd jmp \*0x200e33\(%rip\)        # 200ff0 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ +[a-f0-9]+:	ff 35 b2 01 20 00    	push   0x2001b2\(%rip\)        # 200368 <_GLOBAL_OFFSET_TABLE_\+0x8>
+ +[a-f0-9]+:	f2 ff 25 b3 01 20 00 	bnd jmp \*0x2001b3\(%rip\)        # 200370 <_GLOBAL_OFFSET_TABLE_\+0x10>
  +[a-f0-9]+:	0f 1f 00             	nopl   \(%rax\)
  +[a-f0-9]+:	68 00 00 00 00       	push   \$0x0
  +[a-f0-9]+:	f2 e9 e5 ff ff ff    	bnd jmp 1b0 <func@plt-0x20>
@@ -60,7 +60,7 @@ Disassembly of section .plt:
 Disassembly of section .plt.sec:
 
 0+1d0 <func@plt>:
- +[a-f0-9]+:	f2 ff 25 21 0e 20 00 	bnd jmp \*0x200e21\(%rip\)        # 200ff8 <func>
+ +[a-f0-9]+:	f2 ff 25 a1 01 20 00 	bnd jmp \*0x2001a1\(%rip\)        # 200378 <func>
  +[a-f0-9]+:	90                   	nop
 
 Disassembly of section .text:
diff --git a/ld/testsuite/ld-x86-64/pr21038c-now.d b/ld/testsuite/ld-x86-64/pr21038c-now.d
index 2058512b74e..6c947ea4c13 100644
--- a/ld/testsuite/ld-x86-64/pr21038c-now.d
+++ b/ld/testsuite/ld-x86-64/pr21038c-now.d
@@ -59,8 +59,8 @@ Contents of the .eh_frame section:
 Disassembly of section .plt:
 
 0+1f0 <.plt>:
- +[a-f0-9]+:	ff 35 ea 0d 20 00    	push   0x200dea\(%rip\)        # 200fe0 <_GLOBAL_OFFSET_TABLE_\+0x8>
- +[a-f0-9]+:	f2 ff 25 eb 0d 20 00 	bnd jmp \*0x200deb\(%rip\)        # 200fe8 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ +[a-f0-9]+:	ff 35 12 02 20 00    	push   0x200212\(%rip\)        # 200408 <_GLOBAL_OFFSET_TABLE_\+0x8>
+ +[a-f0-9]+:	f2 ff 25 13 02 20 00 	bnd jmp \*0x200213\(%rip\)        # 200410 <_GLOBAL_OFFSET_TABLE_\+0x10>
  +[a-f0-9]+:	0f 1f 00             	nopl   \(%rax\)
  +[a-f0-9]+:	68 00 00 00 00       	push   \$0x0
  +[a-f0-9]+:	f2 e9 e5 ff ff ff    	bnd jmp 1f0 <func1@plt-0x20>
@@ -69,13 +69,13 @@ Disassembly of section .plt:
 Disassembly of section .plt.got:
 
 0+210 <func1@plt>:
- +[a-f0-9]+:	f2 ff 25 e1 0d 20 00 	bnd jmp \*0x200de1\(%rip\)        # 200ff8 <func1>
+ +[a-f0-9]+:	f2 ff 25 09 02 20 00 	bnd jmp \*0x200209\(%rip\)        # 200420 <func1>
  +[a-f0-9]+:	90                   	nop
 
 Disassembly of section .plt.sec:
 
 0+218 <func2@plt>:
- +[a-f0-9]+:	f2 ff 25 d1 0d 20 00 	bnd jmp \*0x200dd1\(%rip\)        # 200ff0 <func2>
+ +[a-f0-9]+:	f2 ff 25 f9 01 20 00 	bnd jmp \*0x2001f9\(%rip\)        # 200418 <func2>
  +[a-f0-9]+:	90                   	nop
 
 Disassembly of section .text:
@@ -83,5 +83,5 @@ Disassembly of section .text:
 0+220 <foo>:
  +[a-f0-9]+:	e8 eb ff ff ff       	call   210 <func1@plt>
  +[a-f0-9]+:	e8 ee ff ff ff       	call   218 <func2@plt>
- +[a-f0-9]+:	48 8b 05 c7 0d 20 00 	mov    0x200dc7\(%rip\),%rax        # 200ff8 <func1>
+ +[a-f0-9]+:	48 8b 05 ef 01 20 00 	mov    0x2001ef\(%rip\),%rax        # 200420 <func1>
 #pass
diff --git a/ld/testsuite/ld-x86-64/pr21038c.d b/ld/testsuite/ld-x86-64/pr21038c.d
index a62d43a7bc0..40ecc97e517 100644
--- a/ld/testsuite/ld-x86-64/pr21038c.d
+++ b/ld/testsuite/ld-x86-64/pr21038c.d
@@ -68,7 +68,7 @@ Disassembly of section .plt:
 Disassembly of section .plt.got:
 
 0+210 <func1@plt>:
- +[a-f0-9]+:	f2 ff 25 e1 0d 20 00 	bnd jmp \*0x200de1\(%rip\)        # 200ff8 <func1>
+ +[a-f0-9]+:	f2 ff 25 c9 01 20 00 	bnd jmp \*0x2001c9\(%rip\)        # 2003e0 <func1>
  +[a-f0-9]+:	90                   	nop
 
 Disassembly of section .plt.sec:
@@ -82,5 +82,5 @@ Disassembly of section .text:
 0+220 <foo>:
  +[a-f0-9]+:	e8 eb ff ff ff       	call   210 <func1@plt>
  +[a-f0-9]+:	e8 ee ff ff ff       	call   218 <func2@plt>
- +[a-f0-9]+:	48 8b 05 c7 0d 20 00 	mov    0x200dc7\(%rip\),%rax        # 200ff8 <func1>
+ +[a-f0-9]+:	48 8b 05 af 01 20 00 	mov    0x2001af\(%rip\),%rax        # 2003e0 <func1>
 #pass
-- 
2.34.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] elf: Remove the 1-page gap before the RELRO segment
  2022-01-11  2:12 [PATCH] elf: Remove the 1-page gap before the RELRO segment H.J. Lu
@ 2022-01-11  5:26 ` Fangrui Song
  2022-01-13 12:44 ` Nick Clifton
  2022-01-13 12:52 ` Alan Modra
  2 siblings, 0 replies; 20+ messages in thread
From: Fangrui Song @ 2022-01-11  5:26 UTC (permalink / raw)
  To: H.J. Lu; +Cc: binutils

On 2022-01-10, 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
>
>Instead, we can remove the 1-page gap if the maximum page size >= the
>maximum section alignment:
>
>  [18] .eh_frame    PROGBITS    408fa0 008fa0 005e80 00   A  0   0  8
>  [19] .init_array  INIT_ARRAY  40fde0 00fde0 000008 08  WA  0   0  8
>  [20] .fini_array  FINI_ARRAY  40fde8 00fde8 000008 08  WA  0   0  8
>  [21] .dynamic     DYNAMIC     40fdf0 00fdf0 000200 10  WA  7   0  8
>  [22] .got         PROGBITS    40fff0 00fff0 000010 08  WA  0   0  8
>  [23] .got.plt     PROGBITS    410000 010000 000048 08  WA  0   0  8
>
>Because the end of the RELRO segment is always aligned to the page size
>and may not be moved, the RELRO segment size may be increased:
>
>  [ 3] .dynstr      STRTAB      000148 000148 000001 00   A  0   0  1
>  [ 4] .eh_frame    PROGBITS    000150 000150 000000 00   A  0   0  8
>  [ 5] .init_array  INIT_ARRAY  200150 000150 000010 08  WA  0   0  1
>  [ 6] .fini_array  FINI_ARRAY  200160 000160 000010 08  WA  0   0  1
>  [ 7] .jcr         PROGBITS    200170 000170 000008 00  WA  0   0  1
>  [ 8] .data.rel.ro PROGBITS    200180 000180 000020 00  WA  0   0 16
>  [ 9] .dynamic     DYNAMIC     2001a0 0001a0 0001c0 10  WA  3   0  8
>  [10] .got         PROGBITS    200360 000360 0002a8 00  WA  0   0  8
>  [11] .bss         NOBITS      201000 000608 000840 00  WA  0   0  1
>
>vs the old section layout:
>
>  [ 3] .dynstr      STRTAB      000148 000148 000001 00   A  0   0  1
>  [ 4] .eh_frame    PROGBITS    000150 000150 000000 00   A  0   0  8
>  [ 5] .init_array  INIT_ARRAY  200b48 000b48 000010 08  WA  0   0  1
>  [ 6] .fini_array  FINI_ARRAY  200b58 000b58 000010 08  WA  0   0  1
>  [ 7] .jcr         PROGBITS    200b68 000b68 000008 00  WA  0   0  1
>  [ 8] .data.rel.ro PROGBITS    200b70 000b70 000020 00  WA  0   0 16
>  [ 9] .dynamic     DYNAMIC     200b90 000b90 0001c0 10  WA  3   0  8
>  [10] .got         PROGBITS    200d50 000d50 0002a8 00  WA  0   0  8
>  [11] .bss         NOBITS      201000 000ff8 000840 00  WA  0   0  1
>
>But there is no 1-page gap.
> [...}

If you want to avoid a max-page-size alignment (at the end of PT_GNU_RELRO), you may adopt ld.lld's design
I implemented:

   LOAD           0x000000 0x0000000000200000 0x0000000000200000 0xcaac4c 0xcaac4c R   0x1000
   LOAD           0xcaac50 0x0000000000eabc50 0x0000000000eabc50 0x208a0c0 0x208a0c0 R E 0x1000
   LOAD           0x2d34d10 0x0000000002f36d10 0x0000000002f36d10 0x1777e8 0x1777e8 RW  0x1000     match
   LOAD           0x2eac500 0x00000000030af500 0x00000000030af500 0x008038 0x064664 RW  0x1000
   TLS            0x2d34d10 0x0000000002f35d10 0x0000000002f35d10 0x000000 0x000018 R   0x8
   DYNAMIC        0x2ea5570 0x00000000030a7570 0x00000000030a7570 0x000240 0x000240 RW  0x8
   GNU_RELRO      0x2d34d10 0x0000000002f36d10 0x0000000002f36d10 0x1777e8 0x1782f0 R   0x1        match
   GNU_EH_FRAME   0x89ce34 0x0000000000a9ce34 0x0000000000a9ce34 0x08586c 0x08586c R   0x4
   GNU_STACK      0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x000000 RW  0
   NOTE           0x0002fc 0x00000000002002fc 0x00000000002002fc 0x000020 0x000020 R   0x4

The idea is to have 2 RW PT_LOAD with one exactly matching PT_GNU_RELRO.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] elf: Remove the 1-page gap before the RELRO segment
  2022-01-11  2:12 [PATCH] elf: Remove the 1-page gap before the RELRO segment H.J. Lu
  2022-01-11  5:26 ` Fangrui Song
@ 2022-01-13 12:44 ` Nick Clifton
  2022-01-13 12:52 ` Alan Modra
  2 siblings, 0 replies; 20+ messages in thread
From: Nick Clifton @ 2022-01-13 12:44 UTC (permalink / raw)
  To: H.J. Lu, binutils

Hi H.J.

> 	PR ld/28743
> 	* ldlang.c (lang_size_relro_segment_1): Remove the 1-page gap
> 	before the RELRO segment if the maximum page size >= the maximum
> 	section alignment.
> 	* testsuite/ld-i386/pr20830.d: Adjusted.
> 	* testsuite/ld-s390/gotreloc_64-relro-1.dd: Likewise.
> 	* testsuite/ld-x86-64/pr14207.d: Likewise.
> 	* testsuite/ld-x86-64/pr18176.d: Likewise.
> 	* testsuite/ld-x86-64/pr20830a-now.d: Likewise.
> 	* testsuite/ld-x86-64/pr20830a.d: Likewise.
> 	* testsuite/ld-x86-64/pr20830b-now.d: Likewise.
> 	* testsuite/ld-x86-64/pr20830b.d: Likewise.
> 	* testsuite/ld-x86-64/pr21038a-now.d: Likewise.
> 	* testsuite/ld-x86-64/pr21038a.d: Likewise.
> 	* testsuite/ld-x86-64/pr21038b-now.d: Likewise.
> 	* testsuite/ld-x86-64/pr21038c-now.d: Likewise.
> 	* testsuite/ld-x86-64/pr21038c.d: Likewise.

Approved - please apply.

Cheers
   Nick


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] elf: Remove the 1-page gap before the RELRO segment
  2022-01-11  2:12 [PATCH] elf: Remove the 1-page gap before the RELRO segment H.J. Lu
  2022-01-11  5:26 ` Fangrui Song
  2022-01-13 12:44 ` Nick Clifton
@ 2022-01-13 12:52 ` Alan Modra
  2022-01-13 13:19   ` H.J. Lu
  2 siblings, 1 reply; 20+ messages in thread
From: Alan Modra @ 2022-01-13 12:52 UTC (permalink / raw)
  To: H.J. Lu; +Cc: binutils

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
correct thing to do rather than fixing up afterwards as your patch
does.

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] elf: Remove the 1-page gap before the RELRO segment
  2022-01-13 12:52 ` Alan Modra
@ 2022-01-13 13:19   ` H.J. Lu
  2022-01-14  8:12     ` Alan Modra
  0 siblings, 1 reply; 20+ messages in thread
From: H.J. Lu @ 2022-01-13 13:19 UTC (permalink / raw)
  To: Alan Modra; +Cc: Binutils

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);
+     }
+   }

> 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?

Thanks.

-- 
H.J.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] elf: Remove the 1-page gap before the RELRO segment
  2022-01-13 13:19   ` H.J. Lu
@ 2022-01-14  8:12     ` Alan Modra
  2022-01-14  9:37       ` Fangrui Song
                         ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Alan Modra @ 2022-01-14  8:12 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Binutils

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] elf: Remove the 1-page gap before the RELRO segment
  2022-01-14  8:12     ` Alan Modra
@ 2022-01-14  9:37       ` Fangrui Song
  2022-01-14 14:58       ` H.J. Lu
  2022-01-14 21:55       ` [PATCH] ld: Rewrite lang_size_relro_segment_1 H.J. Lu
  2 siblings, 0 replies; 20+ messages in thread
From: Fangrui Song @ 2022-01-14  9:37 UTC (permalink / raw)
  To: Alan Modra; +Cc: H.J. Lu, Binutils

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] elf: Remove the 1-page gap before the RELRO segment
  2022-01-14  8:12     ` Alan Modra
  2022-01-14  9:37       ` Fangrui Song
@ 2022-01-14 14:58       ` H.J. Lu
  2022-01-14 21:55       ` [PATCH] ld: Rewrite lang_size_relro_segment_1 H.J. Lu
  2 siblings, 0 replies; 20+ messages in thread
From: H.J. Lu @ 2022-01-14 14:58 UTC (permalink / raw)
  To: Alan Modra; +Cc: Binutils

On Fri, Jan 14, 2022 at 12:12 AM Alan Modra <amodra@gmail.com> 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

If the size of the preceding section is less than one page, we need
to align the RELRO segment.  If we subtract one page, we still
have a 1-page gap.   I ran out of time before I could dig into the
complex logic.

> 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



-- 
H.J.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] ld: Rewrite lang_size_relro_segment_1
  2022-01-14  8:12     ` Alan Modra
  2022-01-14  9:37       ` Fangrui Song
  2022-01-14 14:58       ` H.J. Lu
@ 2022-01-14 21:55       ` H.J. Lu
  2022-01-17  4:08         ` Alan Modra
  2 siblings, 1 reply; 20+ messages in thread
From: H.J. Lu @ 2022-01-14 21:55 UTC (permalink / raw)
  To: Alan Modra; +Cc: Binutils

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] ld: Rewrite lang_size_relro_segment_1
  2022-01-14 21:55       ` [PATCH] ld: Rewrite lang_size_relro_segment_1 H.J. Lu
@ 2022-01-17  4:08         ` Alan Modra
  2022-01-18  4:16           ` [PATCH v2] " H.J. Lu
  0 siblings, 1 reply; 20+ messages in thread
From: Alan Modra @ 2022-01-17  4:08 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Binutils

On Fri, Jan 14, 2022 at 01:55:27PM -0800, H.J. Lu wrote:
> Now I have time to rewrite lang_size_relro_segment_1.  Is it OK for
> master?

No, I don't believe you can calculate the adjustment for the first
relro section without paintakingly working backwards from the last
section taking into account each section's alignment.  See commit
0e5fabeb2c4b and b39910205f54.

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2] ld: Rewrite lang_size_relro_segment_1
  2022-01-17  4:08         ` Alan Modra
@ 2022-01-18  4:16           ` H.J. Lu
       [not found]             ` <CAMe9rOpdkYZDigz8r_oPbweLnaCJUjx3-L-v-vp-70c0MGOHQw@mail.gmail.com>
  0 siblings, 1 reply; 20+ messages in thread
From: H.J. Lu @ 2022-01-18  4:16 UTC (permalink / raw)
  To: Alan Modra; +Cc: Binutils

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

On Sun, Jan 16, 2022 at 8:08 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Fri, Jan 14, 2022 at 01:55:27PM -0800, H.J. Lu wrote:
> > Now I have time to rewrite lang_size_relro_segment_1.  Is it OK for
> > master?
>
> No, I don't believe you can calculate the adjustment for the first
> relro section without paintakingly working backwards from the last
> section taking into account each section's alignment.  See commit
> 0e5fabeb2c4b and b39910205f54.
>

Here is the v2 patch to track the maximum section alignment from
the RELRO segment.  If the maximum page size >= the maximum
section alignment, we can align the RELRO segment to the maximum
page size.


-- 
H.J.

[-- Attachment #2: v2-0001-ld-Rewrite-lang_size_relro_segment_1.patch --]
[-- Type: text/x-patch, Size: 11043 bytes --]

From 6cef5bac1669defc772927d0dd93a66b0a32d027 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 14 Jan 2022 13:48:36 -0800
Subject: [PATCH v2] ld: Rewrite lang_size_relro_segment_1

Rewrite lang_size_relro_segment_1:

1. Find the first section in the relro segment.
2. Find the first non-empty preceding section.
3. Find the maximum section alignment in sections starting from the
PT_GNU_RELRO segment.
4. Don't add the 1-page gap before the relro segment if the maximum page
size >= the maximum section alignment.  When the maximum page size >= the
maximum section alignment, align the PT_GNU_RELRO segment to the maximum
page size won't impact the maximum section alignment in sections starting
from the PT_GNU_RELRO segment.
5. 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.
	* testsuite/ld-x86-64/pr28743-1.d: New file.
	* testsuite/ld-x86-64/pr28743-1.s: Likewise.
	* testsuite/ld-x86-64/x86-64.exp: Run pr28743-1.
---
 ld/ldlang.c                        | 115 +++++++++++++++--------------
 ld/testsuite/ld-x86-64/pr28743-1.d |  52 +++++++++++++
 ld/testsuite/ld-x86-64/pr28743-1.s |  16 ++++
 ld/testsuite/ld-x86-64/x86-64.exp  |   1 +
 4 files changed, 128 insertions(+), 56 deletions(-)
 create mode 100644 ld/testsuite/ld-x86-64/pr28743-1.d
 create mode 100644 ld/testsuite/ld-x86-64/pr28743-1.s

diff --git a/ld/ldlang.c b/ld/ldlang.c
index 93fcfc4cbc7..5403bfe237f 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -6370,10 +6370,15 @@ 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;
+  enum
+    {
+      relro_sec_after,		/* After the relro segment.  */
+      relro_sec_in,		/* In the relro segment.  */
+      relro_sec_before		/* Before the relro segment.  */
+    } last_sec_status = relro_sec_after;
 
   /* Compute the expected PT_GNU_RELRO/PT_LOAD segment end.  */
   relro_end = ((seg->relro_end + seg->pagesize - 1)
@@ -6382,81 +6387,79 @@ 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)
       {
-	if (sec->alignment_power > max_alignment_power)
+	if (last_sec_status != relro_sec_before
+	    && 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 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;
+	    last_sec_status = relro_sec_in;
 	  }
+	else
+	  last_sec_status = relro_sec_before;
       }
 
-  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;
 }
diff --git a/ld/testsuite/ld-x86-64/pr28743-1.d b/ld/testsuite/ld-x86-64/pr28743-1.d
new file mode 100644
index 00000000000..36297d284e8
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/pr28743-1.d
@@ -0,0 +1,52 @@
+#as: --64
+#ld: -melf_x86_64 -shared -z relro -z now --hash-style=sysv -z max-page-size=0x1000 -z noseparate-code $NO_DT_RELR_LDFLAGS
+#readelf: -S -l --wide
+#target: x86_64-*-linux*
+
+There are 17 section headers, starting at offset 0x101228:
+
+Section Headers:
+  \[Nr\] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
+  \[ 0\]                   NULL            0000000000000000 000000 000000 00      0   0  0
+  \[ 1\] .hash             HASH            0000000000000158 000158 000018 04   A  2   0  8
+  \[ 2\] .dynsym           DYNSYM          0000000000000170 000170 000048 18   A  3   1  8
+  \[ 3\] .dynstr           STRTAB          00000000000001b8 0001b8 00000a 00   A  0   0  1
+  \[ 4\] .rela.dyn         RELA            00000000000001c8 0001c8 000018 18   A  2   0  8
+  \[ 5\] .plt              PROGBITS        00000000000001e0 0001e0 000010 10  AX  0   0 16
+  \[ 6\] .plt.got          PROGBITS        00000000000001f0 0001f0 000008 08  AX  0   0  8
+  \[ 7\] .text             PROGBITS        00000000000001f8 0001f8 00000c 00  AX  0   0  1
+  \[ 8\] .eh_frame         PROGBITS        0000000000000208 000208 00006c 00   A  0   0  8
+  \[ 9\] .init_array       INIT_ARRAY      0000000000001278 000278 000004 08  WA  0   0  8
+  \[10\] .fini_array       FINI_ARRAY      0000000000100000 100000 000100 08  WA  0   0 1048576
+  \[11\] .dynamic          DYNAMIC         0000000000100100 100100 000150 10  WA  3   0  8
+  \[12\] .got              PROGBITS        0000000000100250 100250 000020 08  WA  0   0  8
+  \[13\] .data             PROGBITS        0000000000101000 101000 000100 00  WA  0   0  1
+  \[14\] .symtab           SYMTAB          0000000000000000 101100 000078 18     15   3  8
+  \[15\] .strtab           STRTAB          0000000000000000 101178 000029 00      0   0  1
+  \[16\] .shstrtab         STRTAB          0000000000000000 1011a1 000080 00      0   0  1
+Key to Flags:
+  W \(write\), A \(alloc\), X \(execute\), M \(merge\), S \(strings\), I \(info\),
+  L \(link order\), O \(extra OS processing required\), G \(group\), T \(TLS\),
+  C \(compressed\), x \(unknown\), o \(OS specific\), E \(exclude\),
+  D \(mbind\), l \(large\), p \(processor specific\)
+
+Elf file type is DYN \(Shared object file\)
+Entry point 0x0
+There are 5 program headers, starting at offset 64
+
+Program Headers:
+  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
+  LOAD           0x000000 0x0000000000000000 0x0000000000000000 0x000274 0x000274 R E 0x1000
+  LOAD           0x000278 0x0000000000001278 0x0000000000001278 0x000004 0x000004 RW  0x1000
+  LOAD           0x100000 0x0000000000100000 0x0000000000100000 0x001100 0x001100 RW  0x100000
+  DYNAMIC        0x100100 0x0000000000100100 0x0000000000100100 0x000150 0x000150 RW  0x8
+  GNU_RELRO      0x000278 0x0000000000001278 0x0000000000001278 0x000004 0x0ffd88 R   0x1
+
+ Section to Segment mapping:
+  Segment Sections...
+   00     .hash .dynsym .dynstr .rela.dyn .plt .plt.got .text .eh_frame 
+   01     .init_array 
+   02     .fini_array .dynamic .got .data 
+   03     .dynamic 
+   04     .init_array 
+#pass
diff --git a/ld/testsuite/ld-x86-64/pr28743-1.s b/ld/testsuite/ld-x86-64/pr28743-1.s
new file mode 100644
index 00000000000..2e9a1503741
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/pr28743-1.s
@@ -0,0 +1,16 @@
+	.text
+	.globl foo
+	.type foo, @function
+foo:
+	.cfi_startproc
+	call	func@plt
+	movq	func@GOTPCREL(%rip), %rax
+	.cfi_endproc
+	.section .init_array,"aw",@init_array
+	.p2align 3
+	.zero 0x4
+	.section .fini_array,"aw",@fini_array
+	.p2align 20
+	.zero 0x100
+	.data
+	.zero 0x100
diff --git a/ld/testsuite/ld-x86-64/x86-64.exp b/ld/testsuite/ld-x86-64/x86-64.exp
index 3bfc5e6e467..5ef66a237ef 100644
--- a/ld/testsuite/ld-x86-64/x86-64.exp
+++ b/ld/testsuite/ld-x86-64/x86-64.exp
@@ -511,6 +511,7 @@ run_dump_test "dt-relr-1a"
 run_dump_test "dt-relr-1a-x32"
 run_dump_test "dt-relr-1b"
 run_dump_test "dt-relr-1b-x32"
+run_dump_test "pr28743-1"
 
 if ![istarget "x86_64-*-linux*"] {
     return
-- 
2.34.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

[parent not found: <CAMe9rOpdkYZDigz8r_oPbweLnaCJUjx3-L-v-vp-70c0MGOHQw@mail.gmail.com>]

* Re: Fwd: [PATCH v2] ld: Rewrite lang_size_relro_segment_1
       [not found]             ` <CAMe9rOpdkYZDigz8r_oPbweLnaCJUjx3-L-v-vp-70c0MGOHQw@mail.gmail.com>
@ 2022-01-24 16:24               ` Nick Clifton
  2022-01-24 21:17                 ` [PATCH v3] " H.J. Lu
  0 siblings, 1 reply; 20+ messages in thread
From: Nick Clifton @ 2022-01-24 16:24 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Binutils

Hi H.J.

> Here is the v2 patch to track the maximum section alignment from
> the RELRO segment.  If the maximum page size >= the maximum
> section alignment, we can align the RELRO segment to the maximum
> page size.

I am having some trouble getting my head around this patch, so please
bare with me if I ask some silly questions:

    /* 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)
        {
  	if (last_sec_status != relro_sec_before
  	    && 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)
  	  {
  	    relro_sec = sec;
  	    prev_sec = sec->prev;
  	    last_sec_status = relro_sec_in;
  	  }
  	else
  	  last_sec_status = relro_sec_before;
        }

Questions:
   Should a zero-sized section (with SHF_ALLOC) be treated as a
   viable candidate ?

   Once last_sec_status has been set to relro_sec_before, is there
   any point in continuing the scan ?

   If the section list contains a section which starts beyond the
   end of the segment (ie sec->vma >= seg->relro_end - seg->relro_offset)
   then a) is this an error ?  and b) should its alignment power be
   considered against max_alignment_power ?

    if (relro_sec)
      {
        /* Where do we want to put this section so that it ends as
  	 desired?  */

   By "desired" I assume you mean "with the desired alignment, but
   without any unnecessary padding".  Is that correct ?

       /* 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;

   Given that prev_sec is only needed later on, inside an if-statement,
   you could move this loop - potentially saving a bit of time in some
   cases.

   Overall I think that the code could use some more comments, explaining
   what is going on with some examples of how each part of the algorithm
   operates.

Cheers
   Nick

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3] ld: Rewrite lang_size_relro_segment_1
  2022-01-24 16:24               ` Fwd: " Nick Clifton
@ 2022-01-24 21:17                 ` H.J. Lu
  2022-01-25 15:05                   ` [PATCH v4] " H.J. Lu
  0 siblings, 1 reply; 20+ messages in thread
From: H.J. Lu @ 2022-01-24 21:17 UTC (permalink / raw)
  To: Nick Clifton; +Cc: Binutils

On Mon, Jan 24, 2022 at 04:24:33PM +0000, Nick Clifton wrote:
> Hi H.J.
> 
> > Here is the v2 patch to track the maximum section alignment from
> > the RELRO segment.  If the maximum page size >= the maximum
> > section alignment, we can align the RELRO segment to the maximum
> > page size.
> 
> I am having some trouble getting my head around this patch, so please
> bare with me if I ask some silly questions:
> 
>    /* 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)
>        {
>  	if (last_sec_status != relro_sec_before
>  	    && 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)
>  	  {
>  	    relro_sec = sec;
>  	    prev_sec = sec->prev;
>  	    last_sec_status = relro_sec_in;
>  	  }
>  	else
>  	  last_sec_status = relro_sec_before;
>        }
> 
> Questions:
>   Should a zero-sized section (with SHF_ALLOC) be treated as a
>   viable candidate ?

Fixed.

> 
>   Once last_sec_status has been set to relro_sec_before, is there
>   any point in continuing the scan ?

Fixed.

> 
>   If the section list contains a section which starts beyond the
>   end of the segment (ie sec->vma >= seg->relro_end - seg->relro_offset)
>   then a) is this an error ?  and b) should its alignment power be
>   considered against max_alignment_power ?

Since sections are scanned backwards from the highest address and
sections whose vma >= seg->relro_end - seg->relro_offset are placed
before the PT_GNU_RELRO segment, these sections are scanned for
max_alignment_power.

> 
> 
>    if (relro_sec)
>      {
>        /* Where do we want to put this section so that it ends as
>  	 desired?  */
> 
>   By "desired" I assume you mean "with the desired alignment, but
>   without any unnecessary padding".  Is that correct ?

I changed it to

            /* Where do we want to put the relro section so that the
               relro segment ends on the page bounary?  */
            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;
> 
>   Given that prev_sec is only needed later on, inside an if-statement,
>   you could move this loop - potentially saving a bit of time in some
>   cases.
> 

Fixed.

> 
>   Overall I think that the code could use some more comments, explaining
>   what is going on with some examples of how each part of the algorithm
>   operates.
> 

I added more comments.  Here is the v3 patch.

Thanks.


H.J.
 --
1. Compute the desired PT_GNU_RELRO segment base and find the maximum
section alignment of sections starting from the PT_GNU_RELRO segment.
2. Find the first preceding load section.
3. Don't add the 1-page gap between the first preceding load section and
the relro segment if the maximum page size >= the maximum section
alignment.  Subtract the maximum page size from the desired PT_GNU_RELRO
segment base only if the preceding load section size is > the maximum
page size to avoid moving the PT_GNU_RELRO segment base before the
preceding load section.  Otherwise, simply align the desired PT_GNU_RELRO
segment base.

	PR ld/28743
	* ldlang.c (lang_size_relro_segment_1): Rewrite.
	* testsuite/ld-x86-64/pr28743-1.d: New file.
	* testsuite/ld-x86-64/pr28743-1.s: Likewise.
	* testsuite/ld-x86-64/x86-64.exp: Run pr28743-1.
---
 ld/ldlang.c                        | 90 ++++++++++++++++--------------
 ld/testsuite/ld-x86-64/pr28743-1.d | 50 +++++++++++++++++
 ld/testsuite/ld-x86-64/pr28743-1.s | 16 ++++++
 ld/testsuite/ld-x86-64/x86-64.exp  |  1 +
 4 files changed, 114 insertions(+), 43 deletions(-)
 create mode 100644 ld/testsuite/ld-x86-64/pr28743-1.d
 create mode 100644 ld/testsuite/ld-x86-64/pr28743-1.s

diff --git a/ld/ldlang.c b/ld/ldlang.c
index 93fcfc4cbc7..25eac62a63a 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -6370,94 +6370,98 @@ 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_relro_base;
+  asection *sec, *relro_sec = NULL;
   unsigned int max_alignment_power = 0;
+  bool seen_reloc_section = false;
+  bool desired_relro_base_reduced = false;
 
   /* Compute the expected PT_GNU_RELRO/PT_LOAD segment end.  */
   relro_end = ((seg->relro_end + seg->pagesize - 1)
 	       & ~(seg->pagesize - 1));
 
   /* Adjust by the offset arg of XXX_SEGMENT_RELRO_END.  */
-  desired_end = relro_end - seg->relro_offset;
+  desired_relro_base = 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)
       {
+	/* 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 this section so that it ends as
-	       desired?  */
+	    /* 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_end - end;
+	    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_end = start;
-	    prev_sec = sec->prev;
+	    desired_relro_base = start;
+	    relro_sec = sec;
+	    seen_reloc_section = true;
 	  }
-      }
-
-  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)
+	else if (seen_reloc_section)
 	  {
-	    /* The 1-page gap before the RELRO segment may be removed.  */
-	    remove_page_gap = ((prev_sec->vma + prev_sec->size
-				+ seg->maxpagesize) < desired_end);
-
+	    /* Stop searching if we see a non-relro section after seeing
+	       relro sections.  */
 	    break;
 	  }
       }
 
-  if (remove_page_gap)
+  if (relro_sec != NULL
+      && seg->maxpagesize >= (1U << max_alignment_power))
     {
-      /* 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;
+      asection *prev_sec;
 
-      /* Remove the 1-page gap before the RELRO segment only if the
-	 maximum page size >= the maximum section alignment.  */
-      if (seg->maxpagesize >= (1U << max_alignment_power))
+       /* 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;
+
+      if ((prev_sec->vma + prev_sec->size + seg->maxpagesize)
+	  < desired_relro_base)
 	{
-	  /* 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.  */
+	  desired_relro_base_reduced = true;
+
+	  /* Don't add the 1-page gap before the relro segment if the
+	     maximum page size >= the maximum section alignment.  */
 	  if (prev_sec->size > seg->maxpagesize)
 	    {
-	      desired_end -= seg->maxpagesize;
+	      /* Subtract the maximum page size only if the preceding
+	         load section size is > the maximum page size to avoid
+		 moving the relro segment base before the preceding
+		 load section.  */
+	      desired_relro_base -= seg->maxpagesize;
 	      relro_end -= seg->maxpagesize;
 	    }
 	  else
 	    {
-	      desired_end &= ~(seg->maxpagesize - 1);
+	      /* Align the relro segment if there is a 1-page gap
+		 before the relro segment.  */
+	      desired_relro_base &= ~(seg->maxpagesize - 1);
 	      relro_end &= ~(seg->maxpagesize - 1);
 	    }
-	  }
-      }
+	}
+    }
 
-  seg->base = desired_end;
+  seg->phase = exp_seg_relro_adjust;
+  ASSERT (desired_relro_base_reduced
+	  || desired_relro_base >= seg->base);
+  seg->base = desired_relro_base;
   return relro_end;
 }
 
diff --git a/ld/testsuite/ld-x86-64/pr28743-1.d b/ld/testsuite/ld-x86-64/pr28743-1.d
new file mode 100644
index 00000000000..4a20f8e3900
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/pr28743-1.d
@@ -0,0 +1,50 @@
+#as: --64
+#ld: -melf_x86_64 -shared -z relro -z now --hash-style=sysv -z max-page-size=0x1000 -z noseparate-code $NO_DT_RELR_LDFLAGS
+#readelf: -S -l --wide
+#target: x86_64-*-linux*
+
+There are 17 section headers, starting at offset 0x101228:
+
+Section Headers:
+  \[Nr\] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
+  \[ 0\]                   NULL            0000000000000000 000000 000000 00      0   0  0
+  \[ 1\] .hash             HASH            0000000000000120 000120 000018 04   A  2   0  8
+  \[ 2\] .dynsym           DYNSYM          0000000000000138 000138 000048 18   A  3   1  8
+  \[ 3\] .dynstr           STRTAB          0000000000000180 000180 00000a 00   A  0   0  1
+  \[ 4\] .rela.dyn         RELA            0000000000000190 000190 000018 18   A  2   0  8
+  \[ 5\] .plt              PROGBITS        00000000000001b0 0001b0 000010 10  AX  0   0 16
+  \[ 6\] .plt.got          PROGBITS        00000000000001c0 0001c0 000008 08  AX  0   0  8
+  \[ 7\] .text             PROGBITS        00000000000001c8 0001c8 00000c 00  AX  0   0  1
+  \[ 8\] .eh_frame         PROGBITS        00000000000001d8 0001d8 00006c 00   A  0   0  8
+  \[ 9\] .init_array       INIT_ARRAY      00000000000ffff8 0ffff8 000004 08  WA  0   0  8
+  \[10\] .fini_array       FINI_ARRAY      0000000000100000 100000 000100 08  WA  0   0 1048576
+  \[11\] .dynamic          DYNAMIC         0000000000100100 100100 000150 10  WA  3   0  8
+  \[12\] .got              PROGBITS        0000000000100250 100250 000020 08  WA  0   0  8
+  \[13\] .data             PROGBITS        0000000000101000 101000 000100 00  WA  0   0  1
+  \[14\] .symtab           SYMTAB          0000000000000000 101100 000078 18     15   3  8
+  \[15\] .strtab           STRTAB          0000000000000000 101178 000029 00      0   0  1
+  \[16\] .shstrtab         STRTAB          0000000000000000 1011a1 000080 00      0   0  1
+Key to Flags:
+  W \(write\), A \(alloc\), X \(execute\), M \(merge\), S \(strings\), I \(info\),
+  L \(link order\), O \(extra OS processing required\), G \(group\), T \(TLS\),
+  C \(compressed\), x \(unknown\), o \(OS specific\), E \(exclude\),
+  D \(mbind\), l \(large\), p \(processor specific\)
+
+Elf file type is DYN \(Shared object file\)
+Entry point 0x0
+There are 4 program headers, starting at offset 64
+
+Program Headers:
+  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
+  LOAD           0x000000 0x0000000000000000 0x0000000000000000 0x000244 0x000244 R E 0x1000
+  LOAD           0x0ffff8 0x00000000000ffff8 0x00000000000ffff8 0x001108 0x001108 RW  0x100000
+  DYNAMIC        0x100100 0x0000000000100100 0x0000000000100100 0x000150 0x000150 RW  0x8
+  GNU_RELRO      0x0ffff8 0x00000000000ffff8 0x00000000000ffff8 0x001008 0x001008 R   0x1
+
+ Section to Segment mapping:
+  Segment Sections...
+   00     .hash .dynsym .dynstr .rela.dyn .plt .plt.got .text .eh_frame 
+   01     .init_array .fini_array .dynamic .got .data 
+   02     .dynamic 
+   03     .init_array .fini_array .dynamic .got 
+#pass
diff --git a/ld/testsuite/ld-x86-64/pr28743-1.s b/ld/testsuite/ld-x86-64/pr28743-1.s
new file mode 100644
index 00000000000..2e9a1503741
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/pr28743-1.s
@@ -0,0 +1,16 @@
+	.text
+	.globl foo
+	.type foo, @function
+foo:
+	.cfi_startproc
+	call	func@plt
+	movq	func@GOTPCREL(%rip), %rax
+	.cfi_endproc
+	.section .init_array,"aw",@init_array
+	.p2align 3
+	.zero 0x4
+	.section .fini_array,"aw",@fini_array
+	.p2align 20
+	.zero 0x100
+	.data
+	.zero 0x100
diff --git a/ld/testsuite/ld-x86-64/x86-64.exp b/ld/testsuite/ld-x86-64/x86-64.exp
index 3bfc5e6e467..5ef66a237ef 100644
--- a/ld/testsuite/ld-x86-64/x86-64.exp
+++ b/ld/testsuite/ld-x86-64/x86-64.exp
@@ -511,6 +511,7 @@ run_dump_test "dt-relr-1a"
 run_dump_test "dt-relr-1a-x32"
 run_dump_test "dt-relr-1b"
 run_dump_test "dt-relr-1b-x32"
+run_dump_test "pr28743-1"
 
 if ![istarget "x86_64-*-linux*"] {
     return
-- 
2.34.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v4] ld: Rewrite lang_size_relro_segment_1
  2022-01-24 21:17                 ` [PATCH v3] " H.J. Lu
@ 2022-01-25 15:05                   ` H.J. Lu
  2022-01-26 10:55                     ` Nick Clifton
  0 siblings, 1 reply; 20+ messages in thread
From: H.J. Lu @ 2022-01-25 15:05 UTC (permalink / raw)
  To: Nick Clifton, Binutils

On Mon, Jan 24, 2022 at 01:17:23PM -0800, H.J. Lu wrote:
> On Mon, Jan 24, 2022 at 04:24:33PM +0000, Nick Clifton wrote:
> > Hi H.J.
> > 
> > > Here is the v2 patch to track the maximum section alignment from
> > > the RELRO segment.  If the maximum page size >= the maximum
> > > section alignment, we can align the RELRO segment to the maximum
> > > page size.
> > 
> > I am having some trouble getting my head around this patch, so please
> > bare with me if I ask some silly questions:
> > 
> >    /* 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)
> >        {
> >  	if (last_sec_status != relro_sec_before
> >  	    && 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)
> >  	  {
> >  	    relro_sec = sec;
> >  	    prev_sec = sec->prev;
> >  	    last_sec_status = relro_sec_in;
> >  	  }
> >  	else
> >  	  last_sec_status = relro_sec_before;
> >        }
> > 
> > Questions:
> >   Should a zero-sized section (with SHF_ALLOC) be treated as a
> >   viable candidate ?
> 
> Fixed.
> 
> > 
> >   Once last_sec_status has been set to relro_sec_before, is there
> >   any point in continuing the scan ?
> 
> Fixed.
> 
> > 
> >   If the section list contains a section which starts beyond the
> >   end of the segment (ie sec->vma >= seg->relro_end - seg->relro_offset)
> >   then a) is this an error ?  and b) should its alignment power be
> >   considered against max_alignment_power ?
> 
> Since sections are scanned backwards from the highest address and
> sections whose vma >= seg->relro_end - seg->relro_offset are placed
> before the PT_GNU_RELRO segment, these sections are scanned for
> max_alignment_power.
> 
> > 
> > 
> >    if (relro_sec)
> >      {
> >        /* Where do we want to put this section so that it ends as
> >  	 desired?  */
> > 
> >   By "desired" I assume you mean "with the desired alignment, but
> >   without any unnecessary padding".  Is that correct ?
> 
> I changed it to
> 
>             /* Where do we want to put the relro section so that the
>                relro segment ends on the page bounary?  */
>             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;
> > 
> >   Given that prev_sec is only needed later on, inside an if-statement,
> >   you could move this loop - potentially saving a bit of time in some
> >   cases.
> > 
> 
> Fixed.
> 
> > 
> >   Overall I think that the code could use some more comments, explaining
> >   what is going on with some examples of how each part of the algorithm
> >   operates.
> > 
> 
> I added more comments.  Here is the v3 patch.
> 
> Thanks.
> 

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


H.J.
---
1. Compute the desired PT_GNU_RELRO segment base and find the maximum
section alignment of sections starting from the PT_GNU_RELRO segment.
2. Find the first preceding load section.
3. Don't add the 1-page gap between the first preceding load section and
the relro segment if the maximum page size >= the maximum section
alignment.  Align the PT_GNU_RELRO segment first.  Subtract the maximum
page size if therer is still a 1-page gap.

	PR ld/28743
	PR ld/28819
	* ldlang.c (lang_size_relro_segment_1): Rewrite.
	* testsuite/ld-x86-64/pr28743-1.d: New file.
	* testsuite/ld-x86-64/pr28743-1.s: Likewise.
	* testsuite/ld-x86-64/x86-64.exp: Run pr28743-1.
---
 ld/ldlang.c                        | 95 ++++++++++++++++--------------
 ld/testsuite/ld-x86-64/pr28743-1.d | 50 ++++++++++++++++
 ld/testsuite/ld-x86-64/pr28743-1.s | 16 +++++
 ld/testsuite/ld-x86-64/x86-64.exp  |  1 +
 4 files changed, 118 insertions(+), 44 deletions(-)
 create mode 100644 ld/testsuite/ld-x86-64/pr28743-1.d
 create mode 100644 ld/testsuite/ld-x86-64/pr28743-1.s

diff --git a/ld/ldlang.c b/ld/ldlang.c
index 93fcfc4cbc7..4e6511ba4ff 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -6370,94 +6370,101 @@ 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_relro_base;
+  asection *sec, *relro_sec = NULL;
   unsigned int max_alignment_power = 0;
+  bool seen_reloc_section = false;
+  bool desired_relro_base_reduced = false;
 
   /* Compute the expected PT_GNU_RELRO/PT_LOAD segment end.  */
   relro_end = ((seg->relro_end + seg->pagesize - 1)
 	       & ~(seg->pagesize - 1));
 
   /* Adjust by the offset arg of XXX_SEGMENT_RELRO_END.  */
-  desired_end = relro_end - seg->relro_offset;
+  desired_relro_base = 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)
       {
+	/* 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 this section so that it ends as
-	       desired?  */
+	    /* 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_end - end;
+	    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_end = start;
-	    prev_sec = sec->prev;
+	    desired_relro_base = start;
+	    relro_sec = sec;
+	    seen_reloc_section = true;
 	  }
-      }
-
-  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)
+	else if (seen_reloc_section)
 	  {
-	    /* The 1-page gap before the RELRO segment may be removed.  */
-	    remove_page_gap = ((prev_sec->vma + prev_sec->size
-				+ seg->maxpagesize) < desired_end);
-
+	    /* Stop searching if we see a non-relro section after seeing
+	       relro sections.  */
 	    break;
 	  }
       }
 
-  if (remove_page_gap)
+  if (relro_sec != NULL
+      && seg->maxpagesize >= (1U << max_alignment_power))
     {
-      /* 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;
+      asection *prev_sec;
+      bfd_vma prev_sec_end_plus_1_page;
 
-      /* Remove the 1-page gap before the RELRO segment only if the
-	 maximum page size >= the maximum section alignment.  */
-      if (seg->maxpagesize >= (1U << max_alignment_power))
+       /* 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)
 	{
-	  /* 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)
+	  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)
 	    {
-	      desired_end -= seg->maxpagesize;
+	      /* Subtract the maximum page size if therer is still a
+		 1-page gap.  */
+	      desired_relro_base -= seg->maxpagesize;
 	      relro_end -= seg->maxpagesize;
 	    }
 	  else
 	    {
-	      desired_end &= ~(seg->maxpagesize - 1);
+	      /* Align the relro segment.  */
+	      desired_relro_base = aligned_relro_base;
 	      relro_end &= ~(seg->maxpagesize - 1);
 	    }
-	  }
-      }
+	}
+    }
 
-  seg->base = desired_end;
+  seg->phase = exp_seg_relro_adjust;
+  ASSERT (desired_relro_base_reduced
+	  || desired_relro_base >= seg->base);
+  seg->base = desired_relro_base;
   return relro_end;
 }
 
diff --git a/ld/testsuite/ld-x86-64/pr28743-1.d b/ld/testsuite/ld-x86-64/pr28743-1.d
new file mode 100644
index 00000000000..4a20f8e3900
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/pr28743-1.d
@@ -0,0 +1,50 @@
+#as: --64
+#ld: -melf_x86_64 -shared -z relro -z now --hash-style=sysv -z max-page-size=0x1000 -z noseparate-code $NO_DT_RELR_LDFLAGS
+#readelf: -S -l --wide
+#target: x86_64-*-linux*
+
+There are 17 section headers, starting at offset 0x101228:
+
+Section Headers:
+  \[Nr\] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
+  \[ 0\]                   NULL            0000000000000000 000000 000000 00      0   0  0
+  \[ 1\] .hash             HASH            0000000000000120 000120 000018 04   A  2   0  8
+  \[ 2\] .dynsym           DYNSYM          0000000000000138 000138 000048 18   A  3   1  8
+  \[ 3\] .dynstr           STRTAB          0000000000000180 000180 00000a 00   A  0   0  1
+  \[ 4\] .rela.dyn         RELA            0000000000000190 000190 000018 18   A  2   0  8
+  \[ 5\] .plt              PROGBITS        00000000000001b0 0001b0 000010 10  AX  0   0 16
+  \[ 6\] .plt.got          PROGBITS        00000000000001c0 0001c0 000008 08  AX  0   0  8
+  \[ 7\] .text             PROGBITS        00000000000001c8 0001c8 00000c 00  AX  0   0  1
+  \[ 8\] .eh_frame         PROGBITS        00000000000001d8 0001d8 00006c 00   A  0   0  8
+  \[ 9\] .init_array       INIT_ARRAY      00000000000ffff8 0ffff8 000004 08  WA  0   0  8
+  \[10\] .fini_array       FINI_ARRAY      0000000000100000 100000 000100 08  WA  0   0 1048576
+  \[11\] .dynamic          DYNAMIC         0000000000100100 100100 000150 10  WA  3   0  8
+  \[12\] .got              PROGBITS        0000000000100250 100250 000020 08  WA  0   0  8
+  \[13\] .data             PROGBITS        0000000000101000 101000 000100 00  WA  0   0  1
+  \[14\] .symtab           SYMTAB          0000000000000000 101100 000078 18     15   3  8
+  \[15\] .strtab           STRTAB          0000000000000000 101178 000029 00      0   0  1
+  \[16\] .shstrtab         STRTAB          0000000000000000 1011a1 000080 00      0   0  1
+Key to Flags:
+  W \(write\), A \(alloc\), X \(execute\), M \(merge\), S \(strings\), I \(info\),
+  L \(link order\), O \(extra OS processing required\), G \(group\), T \(TLS\),
+  C \(compressed\), x \(unknown\), o \(OS specific\), E \(exclude\),
+  D \(mbind\), l \(large\), p \(processor specific\)
+
+Elf file type is DYN \(Shared object file\)
+Entry point 0x0
+There are 4 program headers, starting at offset 64
+
+Program Headers:
+  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
+  LOAD           0x000000 0x0000000000000000 0x0000000000000000 0x000244 0x000244 R E 0x1000
+  LOAD           0x0ffff8 0x00000000000ffff8 0x00000000000ffff8 0x001108 0x001108 RW  0x100000
+  DYNAMIC        0x100100 0x0000000000100100 0x0000000000100100 0x000150 0x000150 RW  0x8
+  GNU_RELRO      0x0ffff8 0x00000000000ffff8 0x00000000000ffff8 0x001008 0x001008 R   0x1
+
+ Section to Segment mapping:
+  Segment Sections...
+   00     .hash .dynsym .dynstr .rela.dyn .plt .plt.got .text .eh_frame 
+   01     .init_array .fini_array .dynamic .got .data 
+   02     .dynamic 
+   03     .init_array .fini_array .dynamic .got 
+#pass
diff --git a/ld/testsuite/ld-x86-64/pr28743-1.s b/ld/testsuite/ld-x86-64/pr28743-1.s
new file mode 100644
index 00000000000..2e9a1503741
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/pr28743-1.s
@@ -0,0 +1,16 @@
+	.text
+	.globl foo
+	.type foo, @function
+foo:
+	.cfi_startproc
+	call	func@plt
+	movq	func@GOTPCREL(%rip), %rax
+	.cfi_endproc
+	.section .init_array,"aw",@init_array
+	.p2align 3
+	.zero 0x4
+	.section .fini_array,"aw",@fini_array
+	.p2align 20
+	.zero 0x100
+	.data
+	.zero 0x100
diff --git a/ld/testsuite/ld-x86-64/x86-64.exp b/ld/testsuite/ld-x86-64/x86-64.exp
index 3bfc5e6e467..5ef66a237ef 100644
--- a/ld/testsuite/ld-x86-64/x86-64.exp
+++ b/ld/testsuite/ld-x86-64/x86-64.exp
@@ -511,6 +511,7 @@ run_dump_test "dt-relr-1a"
 run_dump_test "dt-relr-1a-x32"
 run_dump_test "dt-relr-1b"
 run_dump_test "dt-relr-1b-x32"
+run_dump_test "pr28743-1"
 
 if ![istarget "x86_64-*-linux*"] {
     return
-- 
2.34.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4] ld: Rewrite lang_size_relro_segment_1
  2022-01-25 15:05                   ` [PATCH v4] " H.J. Lu
@ 2022-01-26 10:55                     ` Nick Clifton
  2022-01-27  0:48                       ` Alan Modra
  0 siblings, 1 reply; 20+ messages in thread
From: Nick Clifton @ 2022-01-26 10:55 UTC (permalink / raw)
  To: H.J. Lu, Binutils

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.

Cheers
   Nick


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4] ld: Rewrite lang_size_relro_segment_1
  2022-01-26 10:55                     ` Nick Clifton
@ 2022-01-27  0:48                       ` Alan Modra
  2022-01-27  2:10                         ` H.J. Lu
  0 siblings, 1 reply; 20+ messages in thread
From: Alan Modra @ 2022-01-27  0:48 UTC (permalink / raw)
  To: Nick Clifton; +Cc: H.J. Lu, Binutils

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

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4] ld: Rewrite lang_size_relro_segment_1
  2022-01-27  0:48                       ` Alan Modra
@ 2022-01-27  2:10                         ` H.J. Lu
  2022-01-29  1:01                           ` Alan Modra
  0 siblings, 1 reply; 20+ messages in thread
From: H.J. Lu @ 2022-01-27  2:10 UTC (permalink / raw)
  To: Alan Modra; +Cc: Nick Clifton, Binutils

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?

> 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.
>
> --
> Alan Modra
> Australia Development Lab, IBM



-- 
H.J.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4] ld: Rewrite lang_size_relro_segment_1
  2022-01-27  2:10                         ` H.J. Lu
@ 2022-01-29  1:01                           ` Alan Modra
  2022-01-29  9:06                             ` Fangrui Song
  2022-01-29 16:45                             ` H.J. Lu
  0 siblings, 2 replies; 20+ messages in thread
From: Alan Modra @ 2022-01-29  1:01 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Nick Clifton, Binutils

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.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4] ld: Rewrite lang_size_relro_segment_1
  2022-01-29  1:01                           ` Alan Modra
@ 2022-01-29  9:06                             ` Fangrui Song
  2022-01-29 16:45                             ` H.J. Lu
  1 sibling, 0 replies; 20+ messages in thread
From: Fangrui Song @ 2022-01-29  9:06 UTC (permalink / raw)
  To: Alan Modra; +Cc: H.J. Lu, Binutils

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4] ld: Rewrite lang_size_relro_segment_1
  2022-01-29  1:01                           ` Alan Modra
  2022-01-29  9:06                             ` Fangrui Song
@ 2022-01-29 16:45                             ` H.J. Lu
  1 sibling, 0 replies; 20+ messages in thread
From: H.J. Lu @ 2022-01-29 16:45 UTC (permalink / raw)
  To: Alan Modra; +Cc: Nick Clifton, Binutils

On Fri, Jan 28, 2022 at 5:01 PM Alan Modra <amodra@gmail.com> 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?

My initial patch passes all tests in binutils, including cross binutils.   But
tests didn't catch all the cases.   For the second patch, I built
glibc for Linux
targets.  Only ia64 glibc doesn't have GNU_RELRO since it doesn't support
GNU_RELRO.

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

x86 should also get maxpagesize.   commonpagesize should be only
used to align the beginning of the PT_GNU_RELRO segment, not the
end of it.  Since the PT_LOAD segment is aligned to maxpagesize, it
should be OK to use commonpagesize to align the PT_GNU_RELRO
segment which is inside of the  PT_LOAD segment.  But there is no
reason to have a 1-page gap before the PT_GNU_RELRO segment.

> 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



-- 
H.J.

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2022-01-29 16:46 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-11  2:12 [PATCH] elf: Remove the 1-page gap before the RELRO segment H.J. Lu
2022-01-11  5:26 ` Fangrui Song
2022-01-13 12:44 ` Nick Clifton
2022-01-13 12:52 ` Alan Modra
2022-01-13 13:19   ` H.J. Lu
2022-01-14  8:12     ` Alan Modra
2022-01-14  9:37       ` Fangrui Song
2022-01-14 14:58       ` H.J. Lu
2022-01-14 21:55       ` [PATCH] ld: Rewrite lang_size_relro_segment_1 H.J. Lu
2022-01-17  4:08         ` Alan Modra
2022-01-18  4:16           ` [PATCH v2] " H.J. Lu
     [not found]             ` <CAMe9rOpdkYZDigz8r_oPbweLnaCJUjx3-L-v-vp-70c0MGOHQw@mail.gmail.com>
2022-01-24 16:24               ` Fwd: " Nick Clifton
2022-01-24 21:17                 ` [PATCH v3] " H.J. Lu
2022-01-25 15:05                   ` [PATCH v4] " H.J. Lu
2022-01-26 10:55                     ` Nick Clifton
2022-01-27  0:48                       ` Alan Modra
2022-01-27  2:10                         ` H.J. Lu
2022-01-29  1:01                           ` Alan Modra
2022-01-29  9:06                             ` Fangrui Song
2022-01-29 16:45                             ` H.J. Lu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).