[PATCH] elf: Set p_align to the common page size if possible

[PATCH] elf: Set p_align to the common page size if possible

[H.J. Lu]

Currently, on 32-bit and 64-bit ARM, it seems that ld generates p_align
values of 0x10000 even if no section alignment is greater than 0x1000.
The issue is more general and probably affects other targets with
multiple common page sizes.

While file layout absolutely must take 64K page size into account, that
does not have to be reflected in the p_align value.  If running on a 64K
kernel, the file will be loaded at a 64K page boundary by necessity. On
a 4K kernel, 64K alignment is not needed.

The glibc loader has been fixed to honor p_align:

https://sourceware.org/bugzilla/show_bug.cgi?id=28676

similar to kernel:

commit ce81bb256a224259ab686742a6284930cbe4f1fa
Author: Chris Kennelly <ckennelly@google.com>
Date:   Thu Oct 15 20:12:32 2020 -0700

    fs/binfmt_elf: use PT_LOAD p_align values for suitable start address

This means that on 4K kernels, we will start to do extra work for 64K
p_align, but this pointless for pretty much all binaries (whose section
alignment rarely exceeds 16).

1. Set p_align to common page size while laying out segments aligning to
maximum page size or section alignment.  The run-time loader can align
segments to common page size or maximum page size, depending on system
page size.
2. If -z max-page-size=NNN is used, p_align will be set to maximum page
size or the largest section alignment.
3. If a section requires alignment higher than the common page size,
don't set p_align to the common page size.
4. If a section requires alignment higher than the maximum page size,
set p_align to the section alignment.
5. For objcopy, when the commone page size != the maximum page size,
p_align may be set to the commone page size while segments are aligned
to the maximum page size.  In this case, the input p_align will be
ignored and the maximum page size will be used to align the ouput
segments.
6. Update linker to disallow the commone page size > the maximum page
size.

bfd/

	PR ld/28689
	PR ld/28695
	* elf.c (assign_file_positions_for_load_sections): Set p_align
	to common page size while laying out segments aligning to
	maximum page size or section alignment.
	(elf_is_p_align_valid): New function.
	(copy_elf_program_header): Call elf_is_p_align_valid to determine
	if p_align is valid.

include/

	PR ld/28689
	PR ld/28695
	* bfdlink.h (bfd_link_info): Add maxpagesize_is_set.

ld/

	PR ld/28689
	PR ld/28695
	* emultempl/elf.em (gld${EMULATION_NAME}_handle_option): Set
	link_info.maxpagesize_is_set for -z max-page-size=NNN.
	* ldelf.c (ldelf_after_parse): Disallow link_info.commonpagesize
	> link_info.maxpagesize.
	* testsuite/ld-elf/elf.exp: Pass -z max-page-size=0x4000 to
	linker to build mbind2a and mbind2b.
	* testsuite/ld-elf/header.d: Add -z common-page-size=0x100.
	* testsuite/ld-elf/linux-x86.exp: Add PR ld/28689 tests.
	* testsuite/ld-elf/p_align-1.c: New file.
	* testsuite/ld-elf/page-size-1.d: New test.
---
 bfd/elf.c                         | 78 +++++++++++++++++++++++++++++--
 include/bfdlink.h                 |  3 ++
 ld/emultempl/elf.em               |  1 +
 ld/ldelf.c                        |  3 ++
 ld/testsuite/ld-elf/elf.exp       |  4 +-
 ld/testsuite/ld-elf/header.d      |  2 +-
 ld/testsuite/ld-elf/linux-x86.exp | 36 ++++++++++++++
 ld/testsuite/ld-elf/p_align-1.c   | 25 ++++++++++
 ld/testsuite/ld-elf/page-size-1.d |  4 ++
 9 files changed, 149 insertions(+), 7 deletions(-)
 create mode 100644 ld/testsuite/ld-elf/p_align-1.c
 create mode 100644 ld/testsuite/ld-elf/page-size-1.d

diff --git a/bfd/elf.c b/bfd/elf.c
index e6c6a8a6c05..431084760ab 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -5406,6 +5406,8 @@ assign_file_positions_for_load_sections (bfd *abfd,
   Elf_Internal_Phdr *p;
   file_ptr off;  /* Octets.  */
   bfd_size_type maxpagesize;
+  bfd_size_type commonpagesize;
+  bool p_align_commonpagesize_p = false;
   unsigned int alloc, actual;
   unsigned int i, j;
   struct elf_segment_map **sorted_seg_map;
@@ -5491,12 +5493,19 @@ assign_file_positions_for_load_sections (bfd *abfd,
 	   elf_sort_segments);
 
   maxpagesize = 1;
+  commonpagesize = 1;
   if ((abfd->flags & D_PAGED) != 0)
     {
       if (link_info != NULL)
-	maxpagesize = link_info->maxpagesize;
+	{
+	  maxpagesize = link_info->maxpagesize;
+	  commonpagesize = link_info->commonpagesize;
+	}
       else
-	maxpagesize = bed->maxpagesize;
+	{
+	  maxpagesize = bed->maxpagesize;
+	  commonpagesize = bed->commonpagesize;
+	}
     }
 
   /* Sections must map to file offsets past the ELF file header.  */
@@ -5560,6 +5569,13 @@ assign_file_positions_for_load_sections (bfd *abfd,
 	     segment.  */
 	  if (m->p_align_valid)
 	    maxpagesize = m->p_align;
+	  else if (link_info == NULL || !link_info->maxpagesize_is_set)
+	    /* Set p_align to common page size while laying out segments
+	       aligning to the maximum page size or the largest section
+	       alignment.  The run-time loader can align segments to the
+	       common page size or the maximum page size, depending on
+	       system page size.  */
+	    p_align_commonpagesize_p = true;
 
 	  p->p_align = maxpagesize;
 	}
@@ -5597,7 +5613,22 @@ assign_file_positions_for_load_sections (bfd *abfd,
 		}
 	      align = (bfd_size_type) 1 << align_power;
 	      if (align < maxpagesize)
-		align = maxpagesize;
+		{
+		  /* If a section requires alignment higher than the
+		     common page size, don't set p_align to the common
+		     page size.  */
+		  if (align > commonpagesize)
+		    p_align_commonpagesize_p = false;
+		  align = maxpagesize;
+		}
+	      else
+		{
+		  /* If a section requires alignment higher than the
+		     maximum page size, set p_align to the section
+		     alignment.  */
+		  p_align_commonpagesize_p = true;
+		  commonpagesize = align;
+		}
 	    }
 
 	  for (i = 0; i < m->count; i++)
@@ -5976,6 +6007,9 @@ assign_file_positions_for_load_sections (bfd *abfd,
 		  print_segment_map (m);
 		}
 	    }
+
+	  if (p_align_commonpagesize_p)
+	    p->p_align = commonpagesize;
 	}
     }
 
@@ -7485,6 +7519,39 @@ rewrite_elf_program_header (bfd *ibfd, bfd *obfd, bfd_vma maxpagesize)
   return true;
 }
 
+/* Return true if p_align in the ELF program header in ABFD is valid.  */
+
+static bool
+elf_is_p_align_valid (bfd *abfd)
+{
+  unsigned int i;
+  Elf_Internal_Phdr *segment;
+  unsigned int num_segments;
+  const struct elf_backend_data *bed = get_elf_backend_data (abfd);
+  bfd_size_type maxpagesize = bed->maxpagesize;
+  bfd_size_type commonpagesize = bed->commonpagesize;
+
+  if (commonpagesize == maxpagesize)
+    return true;
+
+  /* When the commone page size != the maximum page size, p_align may
+     be set to the commone page size while segments are aligned to
+     the maximum page size.  In this case, the input p_align will be
+     ignored and the maximum page size will be used to align the ouput
+     segments.  */
+  segment = elf_tdata (abfd)->phdr;
+  num_segments = elf_elfheader (abfd)->e_phnum;
+  for (i = 0; i < num_segments; i++, segment++)
+    if (segment->p_type == PT_LOAD
+	&& (segment->p_align != commonpagesize
+	    || vma_page_aligned_bias (segment->p_vaddr,
+				      segment->p_offset,
+				      maxpagesize) != 0))
+      return true;
+
+  return false;
+}
+
 /* Copy ELF program header information.  */
 
 static bool
@@ -7499,6 +7566,7 @@ copy_elf_program_header (bfd *ibfd, bfd *obfd)
   unsigned int num_segments;
   bool phdr_included = false;
   bool p_paddr_valid;
+  bool p_palign_valid;
   unsigned int opb = bfd_octets_per_byte (ibfd, NULL);
 
   iehdr = elf_elfheader (ibfd);
@@ -7519,6 +7587,8 @@ copy_elf_program_header (bfd *ibfd, bfd *obfd)
 	break;
       }
 
+  p_palign_valid = elf_is_p_align_valid (ibfd);
+
   for (i = 0, segment = elf_tdata (ibfd)->phdr;
        i < num_segments;
        i++, segment++)
@@ -7561,7 +7631,7 @@ copy_elf_program_header (bfd *ibfd, bfd *obfd)
       map->p_paddr = segment->p_paddr;
       map->p_paddr_valid = p_paddr_valid;
       map->p_align = segment->p_align;
-      map->p_align_valid = 1;
+      map->p_align_valid = p_palign_valid;
       map->p_vaddr_offset = 0;
 
       if (map->p_type == PT_GNU_RELRO
diff --git a/include/bfdlink.h b/include/bfdlink.h
index 566529ee644..8cf34b05c1a 100644
--- a/include/bfdlink.h
+++ b/include/bfdlink.h
@@ -525,6 +525,9 @@ struct bfd_link_info
   /* TRUE if all symbol names should be unique.  */
   unsigned int unique_symbol : 1;
 
+  /* TRUE if maxpagesize is set on command-line.  */
+  unsigned int maxpagesize_is_set : 1;
+
   /* Char that may appear as the first char of a symbol, but should be
      skipped (like symbol_leading_char) when looking up symbols in
      wrap_hash.  Used by PowerPC Linux for 'dot' symbols.  */
diff --git a/ld/emultempl/elf.em b/ld/emultempl/elf.em
index bfaf8130a3e..5eadab9f4b9 100644
--- a/ld/emultempl/elf.em
+++ b/ld/emultempl/elf.em
@@ -721,6 +721,7 @@ fragment <<EOF
 	      || (link_info.maxpagesize & (link_info.maxpagesize - 1)) != 0)
 	    einfo (_("%F%P: invalid maximum page size \`%s'\n"),
 		   optarg + 14);
+	  link_info.maxpagesize_is_set = true;
 	}
       else if (startswith (optarg, "common-page-size="))
 	{
diff --git a/ld/ldelf.c b/ld/ldelf.c
index 529992b02ae..ee09df5f35c 100644
--- a/ld/ldelf.c
+++ b/ld/ldelf.c
@@ -72,6 +72,9 @@ ldelf_after_parse (void)
       link_info.dynamic_undefined_weak = 0;
     }
   after_parse_default ();
+  if (link_info.commonpagesize > link_info.maxpagesize)
+    einfo (_("%F%P: common page size (0x%v) > maximum page size (0x%v)\n"),
+	   link_info.commonpagesize, link_info.maxpagesize);
 }
 
 /* Handle the generation of DT_NEEDED tags.  */
diff --git a/ld/testsuite/ld-elf/elf.exp b/ld/testsuite/ld-elf/elf.exp
index 01d22faad9a..ae8f76db1c7 100644
--- a/ld/testsuite/ld-elf/elf.exp
+++ b/ld/testsuite/ld-elf/elf.exp
@@ -365,7 +365,7 @@ if { [istarget *-*-linux*]
     run_ld_link_exec_tests [list \
 	[list \
 	    "Run mbind2a" \
-	    "$NOPIE_LDFLAGS -Wl,-z,common-page-size=0x4000" \
+	    "$NOPIE_LDFLAGS -Wl,-z,common-page-size=0x4000,-z,max-page-size=0x4000" \
 	    "" \
 	    { mbind2a.s mbind2b.c } \
 	    "mbind2a" \
@@ -374,7 +374,7 @@ if { [istarget *-*-linux*]
 	] \
 	[list \
 	    "Run mbind2b" \
-	    "-static -Wl,-z,common-page-size=0x4000" \
+	    "-static -Wl,-z,common-page-size=0x4000,-z,max-page-size=0x4000" \
 	    "" \
 	    { mbind2a.s mbind2b.c } \
 	    "mbind2b" \
diff --git a/ld/testsuite/ld-elf/header.d b/ld/testsuite/ld-elf/header.d
index c4d174a98da..67f0c981920 100644
--- a/ld/testsuite/ld-elf/header.d
+++ b/ld/testsuite/ld-elf/header.d
@@ -1,5 +1,5 @@
 # target: *-*-linux* *-*-gnu* *-*-vxworks arm*-*-uclinuxfdpiceabi
-# ld: -T header.t -z max-page-size=0x100
+# ld: -T header.t -z max-page-size=0x100 -z common-page-size=0x100
 # objdump: -hpw
 
 #...
diff --git a/ld/testsuite/ld-elf/linux-x86.exp b/ld/testsuite/ld-elf/linux-x86.exp
index ee03b565faf..25a8d0411d9 100644
--- a/ld/testsuite/ld-elf/linux-x86.exp
+++ b/ld/testsuite/ld-elf/linux-x86.exp
@@ -185,6 +185,42 @@ run_ld_link_exec_tests [list \
 	"" \
 	"tmpdir/indirect-extern-access-2.so" \
     ] \
+    [list \
+	"Run p_align-1a without PIE" \
+	"$NOPIE_LDFLAGS" \
+	"" \
+	{ p_align-1.c } \
+	"p_align-1a" \
+	"pass.out" \
+	"$NOPIE_CFLAGS" \
+    ] \
+    [list \
+	"Run p_align-1b with PIE" \
+	"-pie" \
+	"" \
+	{ p_align-1.c } \
+	"p_align-1b" \
+	"pass.out" \
+	"-fpie" \
+    ] \
+    [list \
+	"Run p_align-1c with -Wl,-z,max-page-size=0x1000 without PIE" \
+	"$NOPIE_LDFLAGS -Wl,-z,max-page-size=0x1000" \
+	"" \
+	{ p_align-1.c } \
+	"p_align-1c" \
+	"pass.out" \
+	"$NOPIE_CFLAGS" \
+    ] \
+    [list \
+	"Run p_align-1d with -Wl,-z,max-page-size=0x1000 with PIE" \
+	"-pie -Wl,-z,max-page-size=0x1000" \
+	"" \
+	{ p_align-1.c } \
+	"p_align-1d" \
+	"pass.out" \
+	"-fpie" \
+    ] \
 ]
 
 proc elfedit_test { options test output } {
diff --git a/ld/testsuite/ld-elf/p_align-1.c b/ld/testsuite/ld-elf/p_align-1.c
new file mode 100644
index 00000000000..6579cd74e52
--- /dev/null
+++ b/ld/testsuite/ld-elf/p_align-1.c
@@ -0,0 +1,25 @@
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+
+#ifndef ALIGN
+# define ALIGN 0x800000
+#endif
+
+int
+__attribute__ ((weak))
+is_aligned (void *p, int align)
+{
+  return (((uintptr_t) p) & (align - 1)) == 0;
+}
+
+int foo __attribute__ ((aligned (ALIGN))) = 1;
+
+int
+main (void)
+{
+  if (!is_aligned (&foo, ALIGN))
+    abort ();
+  printf ("PASS\n");
+  return 0;
+}
diff --git a/ld/testsuite/ld-elf/page-size-1.d b/ld/testsuite/ld-elf/page-size-1.d
new file mode 100644
index 00000000000..04d2153b2f9
--- /dev/null
+++ b/ld/testsuite/ld-elf/page-size-1.d
@@ -0,0 +1,4 @@
+#source: dummy.s
+#ld: -z common-page-size=0x4000 -z max-page-size=0x1000
+#error: common page size \(0x4000\) > maximum page size \(0x1000\)
+#target: *-*-linux-gnu *-*-gnu* arm*-*-uclinuxfdpiceabi
-- 
2.33.1

Re: [PATCH] elf: Set p_align to the common page size if possible

[Fangrui Song]

On 2021-12-15, H.J. Lu via Binutils wrote:
>Currently, on 32-bit and 64-bit ARM, it seems that ld generates p_align
>values of 0x10000 even if no section alignment is greater than 0x1000.
>The issue is more general and probably affects other targets with
>multiple common page sizes.
>
>While file layout absolutely must take 64K page size into account, that
>does not have to be reflected in the p_align value.  If running on a 64K
>kernel, the file will be loaded at a 64K page boundary by necessity. On
>a 4K kernel, 64K alignment is not needed.
>
>The glibc loader has been fixed to honor p_align:

Maybe it's just me who is very careful on the words: aligning to p_align
is a new feature, not a bug, as no ABI requires it. No ld.so I know
(FreeBSD, musl, bionic) does this.

>https://sourceware.org/bugzilla/show_bug.cgi?id=28676
>
>similar to kernel:
>
>commit ce81bb256a224259ab686742a6284930cbe4f1fa
>Author: Chris Kennelly <ckennelly@google.com>
>Date:   Thu Oct 15 20:12:32 2020 -0700
>
>    fs/binfmt_elf: use PT_LOAD p_align values for suitable start address

This kernel patch has no cost. It just picks a load bias, while the
glibc's .so loading patch has some costs because there is no alignment
parameter to mmap... So now, every

* (Linux x86-64) -z noseparate-code (default max-page-size=2MiB) .so incurs some munmap overhead
* arm/aarch64/powerpc (default max-page-size=65536) .so incurs some munmap overhead...

If I were to do this, I would fix objcopy first, then adjust ld's
p_align, finally tune glibc's .so loading.

>This means that on 4K kernels, we will start to do extra work for 64K
>p_align, but this pointless for pretty much all binaries (whose section
>alignment rarely exceeds 16).
>
>1. Set p_align to common page size while laying out segments aligning to
>maximum page size or section alignment.  The run-time loader can align
>segments to common page size or maximum page size, depending on system
>page size.
>2. If -z max-page-size=NNN is used, p_align will be set to maximum page
>size or the largest section alignment.
>3. If a section requires alignment higher than the common page size,
>don't set p_align to the common page size.
>4. If a section requires alignment higher than the maximum page size,
>set p_align to the section alignment.
>5. For objcopy, when the commone page size != the maximum page size,
>p_align may be set to the commone page size while segments are aligned
>to the maximum page size.  In this case, the input p_align will be
>ignored and the maximum page size will be used to align the ouput
>segments.

Does this patch do anything with selecting max-page-size for objcopy?
https://sourceware.org/bugzilla/show_bug.cgi?id=28689#c4
Something like gcd({p_vaddr - p_offset}) can be used, but I don't see
anything related to gcd.

>6. Update linker to disallow the commone page size > the maximum page
>size.
>
>bfd/
>
>	PR ld/28689
>	PR ld/28695
>	* elf.c (assign_file_positions_for_load_sections): Set p_align
>	to common page size while laying out segments aligning to
>	maximum page size or section alignment.
>	(elf_is_p_align_valid): New function.
>	(copy_elf_program_header): Call elf_is_p_align_valid to determine
>	if p_align is valid.
>
>include/
>
>	PR ld/28689
>	PR ld/28695
>	* bfdlink.h (bfd_link_info): Add maxpagesize_is_set.
>
>ld/
>
>	PR ld/28689
>	PR ld/28695
>	* emultempl/elf.em (gld${EMULATION_NAME}_handle_option): Set
>	link_info.maxpagesize_is_set for -z max-page-size=NNN.
>	* ldelf.c (ldelf_after_parse): Disallow link_info.commonpagesize
>	> link_info.maxpagesize.
>	* testsuite/ld-elf/elf.exp: Pass -z max-page-size=0x4000 to
>	linker to build mbind2a and mbind2b.
>	* testsuite/ld-elf/header.d: Add -z common-page-size=0x100.
>	* testsuite/ld-elf/linux-x86.exp: Add PR ld/28689 tests.
>	* testsuite/ld-elf/p_align-1.c: New file.
>	* testsuite/ld-elf/page-size-1.d: New test.
>---
> bfd/elf.c                         | 78 +++++++++++++++++++++++++++++--
> include/bfdlink.h                 |  3 ++
> ld/emultempl/elf.em               |  1 +
> ld/ldelf.c                        |  3 ++
> ld/testsuite/ld-elf/elf.exp       |  4 +-
> ld/testsuite/ld-elf/header.d      |  2 +-
> ld/testsuite/ld-elf/linux-x86.exp | 36 ++++++++++++++
> ld/testsuite/ld-elf/p_align-1.c   | 25 ++++++++++
> ld/testsuite/ld-elf/page-size-1.d |  4 ++
> 9 files changed, 149 insertions(+), 7 deletions(-)
> create mode 100644 ld/testsuite/ld-elf/p_align-1.c
> create mode 100644 ld/testsuite/ld-elf/page-size-1.d
>
>diff --git a/bfd/elf.c b/bfd/elf.c
>index e6c6a8a6c05..431084760ab 100644
>--- a/bfd/elf.c
>+++ b/bfd/elf.c
>@@ -5406,6 +5406,8 @@ assign_file_positions_for_load_sections (bfd *abfd,
>   Elf_Internal_Phdr *p;
>   file_ptr off;  /* Octets.  */
>   bfd_size_type maxpagesize;
>+  bfd_size_type commonpagesize;
>+  bool p_align_commonpagesize_p = false;
>   unsigned int alloc, actual;
>   unsigned int i, j;
>   struct elf_segment_map **sorted_seg_map;
>@@ -5491,12 +5493,19 @@ assign_file_positions_for_load_sections (bfd *abfd,
> 	   elf_sort_segments);
>
>   maxpagesize = 1;
>+  commonpagesize = 1;
>   if ((abfd->flags & D_PAGED) != 0)
>     {
>       if (link_info != NULL)
>-	maxpagesize = link_info->maxpagesize;
>+	{
>+	  maxpagesize = link_info->maxpagesize;
>+	  commonpagesize = link_info->commonpagesize;
>+	}
>       else
>-	maxpagesize = bed->maxpagesize;
>+	{
>+	  maxpagesize = bed->maxpagesize;
>+	  commonpagesize = bed->commonpagesize;
>+	}
>     }
>
>   /* Sections must map to file offsets past the ELF file header.  */
>@@ -5560,6 +5569,13 @@ assign_file_positions_for_load_sections (bfd *abfd,
> 	     segment.  */
> 	  if (m->p_align_valid)
> 	    maxpagesize = m->p_align;
>+	  else if (link_info == NULL || !link_info->maxpagesize_is_set)
>+	    /* Set p_align to common page size while laying out segments
>+	       aligning to the maximum page size or the largest section
>+	       alignment.  The run-time loader can align segments to the
>+	       common page size or the maximum page size, depending on
>+	       system page size.  */
>+	    p_align_commonpagesize_p = true;
>
> 	  p->p_align = maxpagesize;
> 	}
>@@ -5597,7 +5613,22 @@ assign_file_positions_for_load_sections (bfd *abfd,
> 		}
> 	      align = (bfd_size_type) 1 << align_power;
> 	      if (align < maxpagesize)
>-		align = maxpagesize;
>+		{
>+		  /* If a section requires alignment higher than the
>+		     common page size, don't set p_align to the common
>+		     page size.  */
>+		  if (align > commonpagesize)
>+		    p_align_commonpagesize_p = false;
>+		  align = maxpagesize;
>+		}
>+	      else
>+		{
>+		  /* If a section requires alignment higher than the
>+		     maximum page size, set p_align to the section
>+		     alignment.  */
>+		  p_align_commonpagesize_p = true;
>+		  commonpagesize = align;
>+		}
> 	    }
>
> 	  for (i = 0; i < m->count; i++)
>@@ -5976,6 +6007,9 @@ assign_file_positions_for_load_sections (bfd *abfd,
> 		  print_segment_map (m);
> 		}
> 	    }
>+
>+	  if (p_align_commonpagesize_p)
>+	    p->p_align = commonpagesize;
> 	}
>     }
>
>@@ -7485,6 +7519,39 @@ rewrite_elf_program_header (bfd *ibfd, bfd *obfd, bfd_vma maxpagesize)
>   return true;
> }
>
>+/* Return true if p_align in the ELF program header in ABFD is valid.  */
>+
>+static bool
>+elf_is_p_align_valid (bfd *abfd)
>+{
>+  unsigned int i;
>+  Elf_Internal_Phdr *segment;
>+  unsigned int num_segments;
>+  const struct elf_backend_data *bed = get_elf_backend_data (abfd);
>+  bfd_size_type maxpagesize = bed->maxpagesize;
>+  bfd_size_type commonpagesize = bed->commonpagesize;
>+
>+  if (commonpagesize == maxpagesize)
>+    return true;
>+
>+  /* When the commone page size != the maximum page size, p_align may
>+     be set to the commone page size while segments are aligned to
>+     the maximum page size.  In this case, the input p_align will be
>+     ignored and the maximum page size will be used to align the ouput
>+     segments.  */
>+  segment = elf_tdata (abfd)->phdr;
>+  num_segments = elf_elfheader (abfd)->e_phnum;
>+  for (i = 0; i < num_segments; i++, segment++)
>+    if (segment->p_type == PT_LOAD
>+	&& (segment->p_align != commonpagesize
>+	    || vma_page_aligned_bias (segment->p_vaddr,
>+				      segment->p_offset,
>+				      maxpagesize) != 0))
>+      return true;
>+
>+  return false;
>+}
>+
> /* Copy ELF program header information.  */
>
> static bool
>@@ -7499,6 +7566,7 @@ copy_elf_program_header (bfd *ibfd, bfd *obfd)
>   unsigned int num_segments;
>   bool phdr_included = false;
>   bool p_paddr_valid;
>+  bool p_palign_valid;
>   unsigned int opb = bfd_octets_per_byte (ibfd, NULL);
>
>   iehdr = elf_elfheader (ibfd);
>@@ -7519,6 +7587,8 @@ copy_elf_program_header (bfd *ibfd, bfd *obfd)
> 	break;
>       }
>
>+  p_palign_valid = elf_is_p_align_valid (ibfd);
>+
>   for (i = 0, segment = elf_tdata (ibfd)->phdr;
>        i < num_segments;
>        i++, segment++)
>@@ -7561,7 +7631,7 @@ copy_elf_program_header (bfd *ibfd, bfd *obfd)
>       map->p_paddr = segment->p_paddr;
>       map->p_paddr_valid = p_paddr_valid;
>       map->p_align = segment->p_align;
>-      map->p_align_valid = 1;
>+      map->p_align_valid = p_palign_valid;
>       map->p_vaddr_offset = 0;
>
>       if (map->p_type == PT_GNU_RELRO
>diff --git a/include/bfdlink.h b/include/bfdlink.h
>index 566529ee644..8cf34b05c1a 100644
>--- a/include/bfdlink.h
>+++ b/include/bfdlink.h
>@@ -525,6 +525,9 @@ struct bfd_link_info
>   /* TRUE if all symbol names should be unique.  */
>   unsigned int unique_symbol : 1;
>
>+  /* TRUE if maxpagesize is set on command-line.  */
>+  unsigned int maxpagesize_is_set : 1;
>+
>   /* Char that may appear as the first char of a symbol, but should be
>      skipped (like symbol_leading_char) when looking up symbols in
>      wrap_hash.  Used by PowerPC Linux for 'dot' symbols.  */
>diff --git a/ld/emultempl/elf.em b/ld/emultempl/elf.em
>index bfaf8130a3e..5eadab9f4b9 100644
>--- a/ld/emultempl/elf.em
>+++ b/ld/emultempl/elf.em
>@@ -721,6 +721,7 @@ fragment <<EOF
> 	      || (link_info.maxpagesize & (link_info.maxpagesize - 1)) != 0)
> 	    einfo (_("%F%P: invalid maximum page size \`%s'\n"),
> 		   optarg + 14);
>+	  link_info.maxpagesize_is_set = true;
> 	}
>       else if (startswith (optarg, "common-page-size="))
> 	{
>diff --git a/ld/ldelf.c b/ld/ldelf.c
>index 529992b02ae..ee09df5f35c 100644
>--- a/ld/ldelf.c
>+++ b/ld/ldelf.c
>@@ -72,6 +72,9 @@ ldelf_after_parse (void)
>       link_info.dynamic_undefined_weak = 0;
>     }
>   after_parse_default ();
>+  if (link_info.commonpagesize > link_info.maxpagesize)
>+    einfo (_("%F%P: common page size (0x%v) > maximum page size (0x%v)\n"),
>+	   link_info.commonpagesize, link_info.maxpagesize);
> }
>
> /* Handle the generation of DT_NEEDED tags.  */
>diff --git a/ld/testsuite/ld-elf/elf.exp b/ld/testsuite/ld-elf/elf.exp
>index 01d22faad9a..ae8f76db1c7 100644
>--- a/ld/testsuite/ld-elf/elf.exp
>+++ b/ld/testsuite/ld-elf/elf.exp
>@@ -365,7 +365,7 @@ if { [istarget *-*-linux*]
>     run_ld_link_exec_tests [list \
> 	[list \
> 	    "Run mbind2a" \
>-	    "$NOPIE_LDFLAGS -Wl,-z,common-page-size=0x4000" \
>+	    "$NOPIE_LDFLAGS -Wl,-z,common-page-size=0x4000,-z,max-page-size=0x4000" \
> 	    "" \
> 	    { mbind2a.s mbind2b.c } \
> 	    "mbind2a" \
>@@ -374,7 +374,7 @@ if { [istarget *-*-linux*]
> 	] \
> 	[list \
> 	    "Run mbind2b" \
>-	    "-static -Wl,-z,common-page-size=0x4000" \
>+	    "-static -Wl,-z,common-page-size=0x4000,-z,max-page-size=0x4000" \
> 	    "" \
> 	    { mbind2a.s mbind2b.c } \
> 	    "mbind2b" \
>diff --git a/ld/testsuite/ld-elf/header.d b/ld/testsuite/ld-elf/header.d
>index c4d174a98da..67f0c981920 100644
>--- a/ld/testsuite/ld-elf/header.d
>+++ b/ld/testsuite/ld-elf/header.d
>@@ -1,5 +1,5 @@
> # target: *-*-linux* *-*-gnu* *-*-vxworks arm*-*-uclinuxfdpiceabi
>-# ld: -T header.t -z max-page-size=0x100
>+# ld: -T header.t -z max-page-size=0x100 -z common-page-size=0x100
> # objdump: -hpw
>
> #...
>diff --git a/ld/testsuite/ld-elf/linux-x86.exp b/ld/testsuite/ld-elf/linux-x86.exp
>index ee03b565faf..25a8d0411d9 100644
>--- a/ld/testsuite/ld-elf/linux-x86.exp
>+++ b/ld/testsuite/ld-elf/linux-x86.exp
>@@ -185,6 +185,42 @@ run_ld_link_exec_tests [list \
> 	"" \
> 	"tmpdir/indirect-extern-access-2.so" \
>     ] \
>+    [list \
>+	"Run p_align-1a without PIE" \
>+	"$NOPIE_LDFLAGS" \
>+	"" \
>+	{ p_align-1.c } \
>+	"p_align-1a" \
>+	"pass.out" \
>+	"$NOPIE_CFLAGS" \
>+    ] \
>+    [list \
>+	"Run p_align-1b with PIE" \
>+	"-pie" \
>+	"" \
>+	{ p_align-1.c } \
>+	"p_align-1b" \
>+	"pass.out" \
>+	"-fpie" \
>+    ] \
>+    [list \
>+	"Run p_align-1c with -Wl,-z,max-page-size=0x1000 without PIE" \
>+	"$NOPIE_LDFLAGS -Wl,-z,max-page-size=0x1000" \
>+	"" \
>+	{ p_align-1.c } \
>+	"p_align-1c" \
>+	"pass.out" \
>+	"$NOPIE_CFLAGS" \
>+    ] \
>+    [list \
>+	"Run p_align-1d with -Wl,-z,max-page-size=0x1000 with PIE" \
>+	"-pie -Wl,-z,max-page-size=0x1000" \
>+	"" \
>+	{ p_align-1.c } \
>+	"p_align-1d" \
>+	"pass.out" \
>+	"-fpie" \
>+    ] \
> ]
>
> proc elfedit_test { options test output } {
>diff --git a/ld/testsuite/ld-elf/p_align-1.c b/ld/testsuite/ld-elf/p_align-1.c
>new file mode 100644
>index 00000000000..6579cd74e52
>--- /dev/null
>+++ b/ld/testsuite/ld-elf/p_align-1.c
>@@ -0,0 +1,25 @@
>+#include <stdio.h>
>+#include <stdint.h>
>+#include <stdlib.h>
>+
>+#ifndef ALIGN
>+# define ALIGN 0x800000
>+#endif
>+
>+int
>+__attribute__ ((weak))
>+is_aligned (void *p, int align)
>+{
>+  return (((uintptr_t) p) & (align - 1)) == 0;
>+}
>+
>+int foo __attribute__ ((aligned (ALIGN))) = 1;
>+
>+int
>+main (void)
>+{
>+  if (!is_aligned (&foo, ALIGN))
>+    abort ();
>+  printf ("PASS\n");
>+  return 0;
>+}
>diff --git a/ld/testsuite/ld-elf/page-size-1.d b/ld/testsuite/ld-elf/page-size-1.d
>new file mode 100644
>index 00000000000..04d2153b2f9
>--- /dev/null
>+++ b/ld/testsuite/ld-elf/page-size-1.d
>@@ -0,0 +1,4 @@
>+#source: dummy.s
>+#ld: -z common-page-size=0x4000 -z max-page-size=0x1000
>+#error: common page size \(0x4000\) > maximum page size \(0x1000\)
>+#target: *-*-linux-gnu *-*-gnu* arm*-*-uclinuxfdpiceabi
>-- 
>2.33.1

There is no test about max-page-size > common-page-size.

Re: [PATCH] elf: Set p_align to the common page size if possible

[H.J. Lu]

On Thu, Dec 16, 2021 at 12:52 AM Fangrui Song <i@maskray.me> wrote:
>
> On 2021-12-15, H.J. Lu via Binutils wrote:
> >Currently, on 32-bit and 64-bit ARM, it seems that ld generates p_align
> >values of 0x10000 even if no section alignment is greater than 0x1000.
> >The issue is more general and probably affects other targets with
> >multiple common page sizes.
> >
> >While file layout absolutely must take 64K page size into account, that
> >does not have to be reflected in the p_align value.  If running on a 64K
> >kernel, the file will be loaded at a 64K page boundary by necessity. On
> >a 4K kernel, 64K alignment is not needed.
> >
> >The glibc loader has been fixed to honor p_align:
>
> Maybe it's just me who is very careful on the words: aligning to p_align
> is a new feature, not a bug, as no ABI requires it. No ld.so I know
> (FreeBSD, musl, bionic) does this.
>
> >https://sourceware.org/bugzilla/show_bug.cgi?id=28676
> >
> >similar to kernel:
> >
> >commit ce81bb256a224259ab686742a6284930cbe4f1fa
> >Author: Chris Kennelly <ckennelly@google.com>
> >Date:   Thu Oct 15 20:12:32 2020 -0700
> >
> >    fs/binfmt_elf: use PT_LOAD p_align values for suitable start address
>
> This kernel patch has no cost. It just picks a load bias, while the
> glibc's .so loading patch has some costs because there is no alignment
> parameter to mmap... So now, every
>
> * (Linux x86-64) -z noseparate-code (default max-page-size=2MiB) .so incurs some munmap overhead
> * arm/aarch64/powerpc (default max-page-size=65536) .so incurs some munmap overhead...
>
> If I were to do this, I would fix objcopy first, then adjust ld's
> p_align, finally tune glibc's .so loading.
>
> >This means that on 4K kernels, we will start to do extra work for 64K
> >p_align, but this pointless for pretty much all binaries (whose section
> >alignment rarely exceeds 16).
> >
> >1. Set p_align to common page size while laying out segments aligning to
> >maximum page size or section alignment.  The run-time loader can align
> >segments to common page size or maximum page size, depending on system
> >page size.
> >2. If -z max-page-size=NNN is used, p_align will be set to maximum page
> >size or the largest section alignment.
> >3. If a section requires alignment higher than the common page size,
> >don't set p_align to the common page size.
> >4. If a section requires alignment higher than the maximum page size,
> >set p_align to the section alignment.
> >5. For objcopy, when the commone page size != the maximum page size,
> >p_align may be set to the commone page size while segments are aligned
> >to the maximum page size.  In this case, the input p_align will be
> >ignored and the maximum page size will be used to align the ouput
> >segments.
>
> Does this patch do anything with selecting max-page-size for objcopy?
> https://sourceware.org/bugzilla/show_bug.cgi?id=28689#c4
> Something like gcd({p_vaddr - p_offset}) can be used, but I don't see
> anything related to gcd.

For objcopy, my patch does

static bool
elf_is_p_align_valid (bfd *abfd)
{
  ...
  if (commonpagesize == maxpagesize)
    return true;

  /* When the common page size != the maximum page size, p_align may
     be set to the common page size while segments are aligned to
     the maximum page size.  In this case, the input p_align will be
     ignored and the maximum page size will be used to align the output
     segments.  */
  segment = elf_tdata (abfd)->phdr;
  num_segments = elf_elfheader (abfd)->e_phnum;
  for (i = 0; i < num_segments; i++, segment++)
    if (segment->p_type == PT_LOAD
        && (segment->p_align != commonpagesize
            || vma_page_aligned_bias (segment->p_vaddr,
                                      segment->p_offset,
                                      maxpagesize) != 0))
      return true;

  return false;
}

1. If p_align isn't set to the common page size, use it as the maximum
page size.
2. Otherwise, use the default maximum page size as the maximum
page size if the segment layout supports it.

objcopy shouldn't change p_align.  But it must keep the same segment
layout.

> >6. Update linker to disallow the commone page size > the maximum page
> >size.
> >
> >bfd/
> >
> >       PR ld/28689
> >       PR ld/28695
> >       * elf.c (assign_file_positions_for_load_sections): Set p_align
> >       to common page size while laying out segments aligning to
> >       maximum page size or section alignment.
> >       (elf_is_p_align_valid): New function.
> >       (copy_elf_program_header): Call elf_is_p_align_valid to determine
> >       if p_align is valid.
> >
> >include/
> >
> >       PR ld/28689
> >       PR ld/28695
> >       * bfdlink.h (bfd_link_info): Add maxpagesize_is_set.
> >
> >ld/
> >
> >       PR ld/28689
> >       PR ld/28695
> >       * emultempl/elf.em (gld${EMULATION_NAME}_handle_option): Set
> >       link_info.maxpagesize_is_set for -z max-page-size=NNN.
> >       * ldelf.c (ldelf_after_parse): Disallow link_info.commonpagesize
> >       > link_info.maxpagesize.
> >       * testsuite/ld-elf/elf.exp: Pass -z max-page-size=0x4000 to
> >       linker to build mbind2a and mbind2b.
> >       * testsuite/ld-elf/header.d: Add -z common-page-size=0x100.
> >       * testsuite/ld-elf/linux-x86.exp: Add PR ld/28689 tests.
> >       * testsuite/ld-elf/p_align-1.c: New file.
> >       * testsuite/ld-elf/page-size-1.d: New test.
> >---
> > bfd/elf.c                         | 78 +++++++++++++++++++++++++++++--
> > include/bfdlink.h                 |  3 ++
> > ld/emultempl/elf.em               |  1 +
> > ld/ldelf.c                        |  3 ++
> > ld/testsuite/ld-elf/elf.exp       |  4 +-
> > ld/testsuite/ld-elf/header.d      |  2 +-
> > ld/testsuite/ld-elf/linux-x86.exp | 36 ++++++++++++++
> > ld/testsuite/ld-elf/p_align-1.c   | 25 ++++++++++
> > ld/testsuite/ld-elf/page-size-1.d |  4 ++
> > 9 files changed, 149 insertions(+), 7 deletions(-)
> > create mode 100644 ld/testsuite/ld-elf/p_align-1.c
> > create mode 100644 ld/testsuite/ld-elf/page-size-1.d
> >
> >diff --git a/bfd/elf.c b/bfd/elf.c
> >index e6c6a8a6c05..431084760ab 100644
> >--- a/bfd/elf.c
> >+++ b/bfd/elf.c
> >@@ -5406,6 +5406,8 @@ assign_file_positions_for_load_sections (bfd *abfd,
> >   Elf_Internal_Phdr *p;
> >   file_ptr off;  /* Octets.  */
> >   bfd_size_type maxpagesize;
> >+  bfd_size_type commonpagesize;
> >+  bool p_align_commonpagesize_p = false;
> >   unsigned int alloc, actual;
> >   unsigned int i, j;
> >   struct elf_segment_map **sorted_seg_map;
> >@@ -5491,12 +5493,19 @@ assign_file_positions_for_load_sections (bfd *abfd,
> >          elf_sort_segments);
> >
> >   maxpagesize = 1;
> >+  commonpagesize = 1;
> >   if ((abfd->flags & D_PAGED) != 0)
> >     {
> >       if (link_info != NULL)
> >-      maxpagesize = link_info->maxpagesize;
> >+      {
> >+        maxpagesize = link_info->maxpagesize;
> >+        commonpagesize = link_info->commonpagesize;
> >+      }
> >       else
> >-      maxpagesize = bed->maxpagesize;
> >+      {
> >+        maxpagesize = bed->maxpagesize;
> >+        commonpagesize = bed->commonpagesize;
> >+      }
> >     }
> >
> >   /* Sections must map to file offsets past the ELF file header.  */
> >@@ -5560,6 +5569,13 @@ assign_file_positions_for_load_sections (bfd *abfd,
> >            segment.  */
> >         if (m->p_align_valid)
> >           maxpagesize = m->p_align;
> >+        else if (link_info == NULL || !link_info->maxpagesize_is_set)
> >+          /* Set p_align to common page size while laying out segments
> >+             aligning to the maximum page size or the largest section
> >+             alignment.  The run-time loader can align segments to the
> >+             common page size or the maximum page size, depending on
> >+             system page size.  */
> >+          p_align_commonpagesize_p = true;
> >
> >         p->p_align = maxpagesize;
> >       }
> >@@ -5597,7 +5613,22 @@ assign_file_positions_for_load_sections (bfd *abfd,
> >               }
> >             align = (bfd_size_type) 1 << align_power;
> >             if (align < maxpagesize)
> >-              align = maxpagesize;
> >+              {
> >+                /* If a section requires alignment higher than the
> >+                   common page size, don't set p_align to the common
> >+                   page size.  */
> >+                if (align > commonpagesize)
> >+                  p_align_commonpagesize_p = false;
> >+                align = maxpagesize;
> >+              }
> >+            else
> >+              {
> >+                /* If a section requires alignment higher than the
> >+                   maximum page size, set p_align to the section
> >+                   alignment.  */
> >+                p_align_commonpagesize_p = true;
> >+                commonpagesize = align;
> >+              }
> >           }
> >
> >         for (i = 0; i < m->count; i++)
> >@@ -5976,6 +6007,9 @@ assign_file_positions_for_load_sections (bfd *abfd,
> >                 print_segment_map (m);
> >               }
> >           }
> >+
> >+        if (p_align_commonpagesize_p)
> >+          p->p_align = commonpagesize;
> >       }
> >     }
> >
> >@@ -7485,6 +7519,39 @@ rewrite_elf_program_header (bfd *ibfd, bfd *obfd, bfd_vma maxpagesize)
> >   return true;
> > }
> >
> >+/* Return true if p_align in the ELF program header in ABFD is valid.  */
> >+
> >+static bool
> >+elf_is_p_align_valid (bfd *abfd)
> >+{
> >+  unsigned int i;
> >+  Elf_Internal_Phdr *segment;
> >+  unsigned int num_segments;
> >+  const struct elf_backend_data *bed = get_elf_backend_data (abfd);
> >+  bfd_size_type maxpagesize = bed->maxpagesize;
> >+  bfd_size_type commonpagesize = bed->commonpagesize;
> >+
> >+  if (commonpagesize == maxpagesize)
> >+    return true;
> >+
> >+  /* When the commone page size != the maximum page size, p_align may
> >+     be set to the commone page size while segments are aligned to
> >+     the maximum page size.  In this case, the input p_align will be
> >+     ignored and the maximum page size will be used to align the ouput
> >+     segments.  */
> >+  segment = elf_tdata (abfd)->phdr;
> >+  num_segments = elf_elfheader (abfd)->e_phnum;
> >+  for (i = 0; i < num_segments; i++, segment++)
> >+    if (segment->p_type == PT_LOAD
> >+      && (segment->p_align != commonpagesize
> >+          || vma_page_aligned_bias (segment->p_vaddr,
> >+                                    segment->p_offset,
> >+                                    maxpagesize) != 0))
> >+      return true;
> >+
> >+  return false;
> >+}
> >+
> > /* Copy ELF program header information.  */
> >
> > static bool
> >@@ -7499,6 +7566,7 @@ copy_elf_program_header (bfd *ibfd, bfd *obfd)
> >   unsigned int num_segments;
> >   bool phdr_included = false;
> >   bool p_paddr_valid;
> >+  bool p_palign_valid;
> >   unsigned int opb = bfd_octets_per_byte (ibfd, NULL);
> >
> >   iehdr = elf_elfheader (ibfd);
> >@@ -7519,6 +7587,8 @@ copy_elf_program_header (bfd *ibfd, bfd *obfd)
> >       break;
> >       }
> >
> >+  p_palign_valid = elf_is_p_align_valid (ibfd);
> >+
> >   for (i = 0, segment = elf_tdata (ibfd)->phdr;
> >        i < num_segments;
> >        i++, segment++)
> >@@ -7561,7 +7631,7 @@ copy_elf_program_header (bfd *ibfd, bfd *obfd)
> >       map->p_paddr = segment->p_paddr;
> >       map->p_paddr_valid = p_paddr_valid;
> >       map->p_align = segment->p_align;
> >-      map->p_align_valid = 1;
> >+      map->p_align_valid = p_palign_valid;
> >       map->p_vaddr_offset = 0;
> >
> >       if (map->p_type == PT_GNU_RELRO
> >diff --git a/include/bfdlink.h b/include/bfdlink.h
> >index 566529ee644..8cf34b05c1a 100644
> >--- a/include/bfdlink.h
> >+++ b/include/bfdlink.h
> >@@ -525,6 +525,9 @@ struct bfd_link_info
> >   /* TRUE if all symbol names should be unique.  */
> >   unsigned int unique_symbol : 1;
> >
> >+  /* TRUE if maxpagesize is set on command-line.  */
> >+  unsigned int maxpagesize_is_set : 1;
> >+
> >   /* Char that may appear as the first char of a symbol, but should be
> >      skipped (like symbol_leading_char) when looking up symbols in
> >      wrap_hash.  Used by PowerPC Linux for 'dot' symbols.  */
> >diff --git a/ld/emultempl/elf.em b/ld/emultempl/elf.em
> >index bfaf8130a3e..5eadab9f4b9 100644
> >--- a/ld/emultempl/elf.em
> >+++ b/ld/emultempl/elf.em
> >@@ -721,6 +721,7 @@ fragment <<EOF
> >             || (link_info.maxpagesize & (link_info.maxpagesize - 1)) != 0)
> >           einfo (_("%F%P: invalid maximum page size \`%s'\n"),
> >                  optarg + 14);
> >+        link_info.maxpagesize_is_set = true;
> >       }
> >       else if (startswith (optarg, "common-page-size="))
> >       {
> >diff --git a/ld/ldelf.c b/ld/ldelf.c
> >index 529992b02ae..ee09df5f35c 100644
> >--- a/ld/ldelf.c
> >+++ b/ld/ldelf.c
> >@@ -72,6 +72,9 @@ ldelf_after_parse (void)
> >       link_info.dynamic_undefined_weak = 0;
> >     }
> >   after_parse_default ();
> >+  if (link_info.commonpagesize > link_info.maxpagesize)
> >+    einfo (_("%F%P: common page size (0x%v) > maximum page size (0x%v)\n"),
> >+         link_info.commonpagesize, link_info.maxpagesize);
> > }
> >
> > /* Handle the generation of DT_NEEDED tags.  */
> >diff --git a/ld/testsuite/ld-elf/elf.exp b/ld/testsuite/ld-elf/elf.exp
> >index 01d22faad9a..ae8f76db1c7 100644
> >--- a/ld/testsuite/ld-elf/elf.exp
> >+++ b/ld/testsuite/ld-elf/elf.exp
> >@@ -365,7 +365,7 @@ if { [istarget *-*-linux*]
> >     run_ld_link_exec_tests [list \
> >       [list \
> >           "Run mbind2a" \
> >-          "$NOPIE_LDFLAGS -Wl,-z,common-page-size=0x4000" \
> >+          "$NOPIE_LDFLAGS -Wl,-z,common-page-size=0x4000,-z,max-page-size=0x4000" \
> >           "" \
> >           { mbind2a.s mbind2b.c } \
> >           "mbind2a" \
> >@@ -374,7 +374,7 @@ if { [istarget *-*-linux*]
> >       ] \
> >       [list \
> >           "Run mbind2b" \
> >-          "-static -Wl,-z,common-page-size=0x4000" \
> >+          "-static -Wl,-z,common-page-size=0x4000,-z,max-page-size=0x4000" \
> >           "" \
> >           { mbind2a.s mbind2b.c } \
> >           "mbind2b" \
> >diff --git a/ld/testsuite/ld-elf/header.d b/ld/testsuite/ld-elf/header.d
> >index c4d174a98da..67f0c981920 100644
> >--- a/ld/testsuite/ld-elf/header.d
> >+++ b/ld/testsuite/ld-elf/header.d
> >@@ -1,5 +1,5 @@
> > # target: *-*-linux* *-*-gnu* *-*-vxworks arm*-*-uclinuxfdpiceabi
> >-# ld: -T header.t -z max-page-size=0x100
> >+# ld: -T header.t -z max-page-size=0x100 -z common-page-size=0x100
> > # objdump: -hpw
> >
> > #...
> >diff --git a/ld/testsuite/ld-elf/linux-x86.exp b/ld/testsuite/ld-elf/linux-x86.exp
> >index ee03b565faf..25a8d0411d9 100644
> >--- a/ld/testsuite/ld-elf/linux-x86.exp
> >+++ b/ld/testsuite/ld-elf/linux-x86.exp
> >@@ -185,6 +185,42 @@ run_ld_link_exec_tests [list \
> >       "" \
> >       "tmpdir/indirect-extern-access-2.so" \
> >     ] \
> >+    [list \
> >+      "Run p_align-1a without PIE" \
> >+      "$NOPIE_LDFLAGS" \
> >+      "" \
> >+      { p_align-1.c } \
> >+      "p_align-1a" \
> >+      "pass.out" \
> >+      "$NOPIE_CFLAGS" \
> >+    ] \
> >+    [list \
> >+      "Run p_align-1b with PIE" \
> >+      "-pie" \
> >+      "" \
> >+      { p_align-1.c } \
> >+      "p_align-1b" \
> >+      "pass.out" \
> >+      "-fpie" \
> >+    ] \
> >+    [list \
> >+      "Run p_align-1c with -Wl,-z,max-page-size=0x1000 without PIE" \
> >+      "$NOPIE_LDFLAGS -Wl,-z,max-page-size=0x1000" \
> >+      "" \
> >+      { p_align-1.c } \
> >+      "p_align-1c" \
> >+      "pass.out" \
> >+      "$NOPIE_CFLAGS" \
> >+    ] \
> >+    [list \
> >+      "Run p_align-1d with -Wl,-z,max-page-size=0x1000 with PIE" \
> >+      "-pie -Wl,-z,max-page-size=0x1000" \
> >+      "" \
> >+      { p_align-1.c } \
> >+      "p_align-1d" \
> >+      "pass.out" \
> >+      "-fpie" \
> >+    ] \
> > ]
> >
> > proc elfedit_test { options test output } {
> >diff --git a/ld/testsuite/ld-elf/p_align-1.c b/ld/testsuite/ld-elf/p_align-1.c
> >new file mode 100644
> >index 00000000000..6579cd74e52
> >--- /dev/null
> >+++ b/ld/testsuite/ld-elf/p_align-1.c
> >@@ -0,0 +1,25 @@
> >+#include <stdio.h>
> >+#include <stdint.h>
> >+#include <stdlib.h>
> >+
> >+#ifndef ALIGN
> >+# define ALIGN 0x800000
> >+#endif
> >+
> >+int
> >+__attribute__ ((weak))
> >+is_aligned (void *p, int align)
> >+{
> >+  return (((uintptr_t) p) & (align - 1)) == 0;
> >+}
> >+
> >+int foo __attribute__ ((aligned (ALIGN))) = 1;
> >+
> >+int
> >+main (void)
> >+{
> >+  if (!is_aligned (&foo, ALIGN))
> >+    abort ();
> >+  printf ("PASS\n");
> >+  return 0;
> >+}
> >diff --git a/ld/testsuite/ld-elf/page-size-1.d b/ld/testsuite/ld-elf/page-size-1.d
> >new file mode 100644
> >index 00000000000..04d2153b2f9
> >--- /dev/null
> >+++ b/ld/testsuite/ld-elf/page-size-1.d
> >@@ -0,0 +1,4 @@
> >+#source: dummy.s
> >+#ld: -z common-page-size=0x4000 -z max-page-size=0x1000
> >+#error: common page size \(0x4000\) > maximum page size \(0x1000\)
> >+#target: *-*-linux-gnu *-*-gnu* arm*-*-uclinuxfdpiceabi
> >--
> >2.33.1
>
> There is no test about max-page-size > common-page-size.



-- 
H.J.

Re: [PATCH] elf: Set p_align to the common page size if possible

[Florian Weimer]

* Fangrui Song:

> On 2021-12-15, H.J. Lu via Binutils wrote:
>>Currently, on 32-bit and 64-bit ARM, it seems that ld generates p_align
>>values of 0x10000 even if no section alignment is greater than 0x1000.
>>The issue is more general and probably affects other targets with
>>multiple common page sizes.
>>
>>While file layout absolutely must take 64K page size into account, that
>>does not have to be reflected in the p_align value.  If running on a 64K
>>kernel, the file will be loaded at a 64K page boundary by necessity. On
>>a 4K kernel, 64K alignment is not needed.
>>
>>The glibc loader has been fixed to honor p_align:
>
> Maybe it's just me who is very careful on the words: aligning to p_align
> is a new feature, not a bug, as no ABI requires it. No ld.so I know
> (FreeBSD, musl, bionic) does this.

The expectation seems to be fairly clear that p_align should reflect
segment alignment.  It's true that the ELF specification does not
explicitly say that segment alignment of virtual addresses also carries
over to the process image, but that part seems so obvious that it
perhaps wasn't stated explicitly.

Current linkers probably should have used the reserved values 0 and 1 if
they want to convey that alignment does not matter.

>>https://sourceware.org/bugzilla/show_bug.cgi?id=28676
>>
>>similar to kernel:
>>
>>commit ce81bb256a224259ab686742a6284930cbe4f1fa
>>Author: Chris Kennelly <ckennelly@google.com>
>>Date:   Thu Oct 15 20:12:32 2020 -0700
>>
>>    fs/binfmt_elf: use PT_LOAD p_align values for suitable start address
>
> This kernel patch has no cost. It just picks a load bias, while the
> glibc's .so loading patch has some costs because there is no alignment
> parameter to mmap... So now, every
>
> * (Linux x86-64) -z noseparate-code (default max-page-size=2MiB) .so incurs some munmap overhead
> * arm/aarch64/powerpc (default max-page-size=65536) .so incurs some munmap overhead...
>
> If I were to do this, I would fix objcopy first, then adjust ld's
> p_align, finally tune glibc's .so loading.

Maybe we need to add some markup that the p_align value is actually
real.

Thanks,
Florian

Re: [PATCH] elf: Set p_align to the common page size if possible

[H.J. Lu]

On Mon, Dec 20, 2021 at 8:36 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Fangrui Song:
>
> > On 2021-12-15, H.J. Lu via Binutils wrote:
> >>Currently, on 32-bit and 64-bit ARM, it seems that ld generates p_align
> >>values of 0x10000 even if no section alignment is greater than 0x1000.
> >>The issue is more general and probably affects other targets with
> >>multiple common page sizes.
> >>
> >>While file layout absolutely must take 64K page size into account, that
> >>does not have to be reflected in the p_align value.  If running on a 64K
> >>kernel, the file will be loaded at a 64K page boundary by necessity. On
> >>a 4K kernel, 64K alignment is not needed.
> >>
> >>The glibc loader has been fixed to honor p_align:
> >
> > Maybe it's just me who is very careful on the words: aligning to p_align
> > is a new feature, not a bug, as no ABI requires it. No ld.so I know
> > (FreeBSD, musl, bionic) does this.
>
> The expectation seems to be fairly clear that p_align should reflect
> segment alignment.  It's true that the ELF specification does not
> explicitly say that segment alignment of virtual addresses also carries
> over to the process image, but that part seems so obvious that it
> perhaps wasn't stated explicitly.
>
> Current linkers probably should have used the reserved values 0 and 1 if
> they want to convey that alignment does not matter.
>
> >>https://sourceware.org/bugzilla/show_bug.cgi?id=28676
> >>
> >>similar to kernel:
> >>
> >>commit ce81bb256a224259ab686742a6284930cbe4f1fa
> >>Author: Chris Kennelly <ckennelly@google.com>
> >>Date:   Thu Oct 15 20:12:32 2020 -0700
> >>
> >>    fs/binfmt_elf: use PT_LOAD p_align values for suitable start address
> >
> > This kernel patch has no cost. It just picks a load bias, while the
> > glibc's .so loading patch has some costs because there is no alignment
> > parameter to mmap... So now, every
> >
> > * (Linux x86-64) -z noseparate-code (default max-page-size=2MiB) .so incurs some munmap overhead
> > * arm/aarch64/powerpc (default max-page-size=65536) .so incurs some munmap overhead...
> >
> > If I were to do this, I would fix objcopy first, then adjust ld's
> > p_align, finally tune glibc's .so loading.
>
> Maybe we need to add some markup that the p_align value is actually
> real.
>

We need to set larger p_align > sh_addralign for huge page executables.
My current algorithm to decide if p_align should be used as the maximum
page size for objcopy is

static bool
elf_is_p_align_valid (bfd *abfd)
{
  unsigned int i;
  Elf_Internal_Phdr *segment;
  unsigned int num_segments;
  const struct elf_backend_data *bed = get_elf_backend_data (abfd);
  bfd_size_type maxpagesize = bed->maxpagesize;
  bfd_size_type commonpagesize = bed->commonpagesize;

  if (commonpagesize == maxpagesize)
    return true;

  /* When the common page size != the maximum page size, p_align may
     be set to the common page size while segments are aligned to
     the maximum page size.  In this case, the input p_align will be
     ignored and the maximum page size will be used to align the output
     segments.  */
  segment = elf_tdata (abfd)->phdr;
  num_segments = elf_elfheader (abfd)->e_phnum;
  for (i = 0; i < num_segments; i++, segment++)
    if (segment->p_type == PT_LOAD
        && (segment->p_align != commonpagesize
            || vma_page_aligned_bias (segment->p_vaddr,
                                      segment->p_offset,
                                      maxpagesize) != 0))
      return true;

  return false;
}

It should cover all cases.

-- 
H.J.

Re: [PATCH] elf: Set p_align to the common page size if possible

[Florian Weimer]

* H. J. Lu:

> We need to set larger p_align > sh_addralign for huge page executables.
> My current algorithm to decide if p_align should be used as the maximum
> page size for objcopy is
>
> static bool
> elf_is_p_align_valid (bfd *abfd)
> {
>   unsigned int i;
>   Elf_Internal_Phdr *segment;
>   unsigned int num_segments;
>   const struct elf_backend_data *bed = get_elf_backend_data (abfd);
>   bfd_size_type maxpagesize = bed->maxpagesize;
>   bfd_size_type commonpagesize = bed->commonpagesize;
>
>   if (commonpagesize == maxpagesize)
>     return true;
>
>   /* When the common page size != the maximum page size, p_align may
>      be set to the common page size while segments are aligned to
>      the maximum page size.  In this case, the input p_align will be
>      ignored and the maximum page size will be used to align the output
>      segments.  */
>   segment = elf_tdata (abfd)->phdr;
>   num_segments = elf_elfheader (abfd)->e_phnum;
>   for (i = 0; i < num_segments; i++, segment++)
>     if (segment->p_type == PT_LOAD
>         && (segment->p_align != commonpagesize
>             || vma_page_aligned_bias (segment->p_vaddr,
>                                       segment->p_offset,
>                                       maxpagesize) != 0))
>       return true;
>
>   return false;
> }
>
> It should cover all cases.

Will this switch from 64K to 4K on POWER and AArch64?  I think this will
then produce binaries which cannot be loaded by current glibc in all
cases:

  PT_LOAD p_align check is too strict
  <https://sourceware.org/bugzilla/show_bug.cgi?id=28688>

Thanks,
Florian

Re: [PATCH] elf: Set p_align to the common page size if possible

[H.J. Lu]

On Mon, Dec 20, 2021 at 10:51 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > We need to set larger p_align > sh_addralign for huge page executables.
> > My current algorithm to decide if p_align should be used as the maximum
> > page size for objcopy is
> >
> > static bool
> > elf_is_p_align_valid (bfd *abfd)
> > {
> >   unsigned int i;
> >   Elf_Internal_Phdr *segment;
> >   unsigned int num_segments;
> >   const struct elf_backend_data *bed = get_elf_backend_data (abfd);
> >   bfd_size_type maxpagesize = bed->maxpagesize;
> >   bfd_size_type commonpagesize = bed->commonpagesize;
> >
> >   if (commonpagesize == maxpagesize)
> >     return true;
> >
> >   /* When the common page size != the maximum page size, p_align may
> >      be set to the common page size while segments are aligned to
> >      the maximum page size.  In this case, the input p_align will be
> >      ignored and the maximum page size will be used to align the output
> >      segments.  */
> >   segment = elf_tdata (abfd)->phdr;
> >   num_segments = elf_elfheader (abfd)->e_phnum;
> >   for (i = 0; i < num_segments; i++, segment++)
> >     if (segment->p_type == PT_LOAD
> >         && (segment->p_align != commonpagesize
> >             || vma_page_aligned_bias (segment->p_vaddr,
> >                                       segment->p_offset,
> >                                       maxpagesize) != 0))
> >       return true;
> >
> >   return false;
> > }
> >
> > It should cover all cases.
>
> Will this switch from 64K to 4K on POWER and AArch64?  I think this will

It will set p_align to ELF_COMMONPAGESIZE.  It glibc doesn't support
p_align == ELF_COMMONPAGESIZE, the ELF_COMMONPAGESIZE
is wrong.

> then produce binaries which cannot be loaded by current glibc in all
> cases:
>
>   PT_LOAD p_align check is too strict
>   <https://sourceware.org/bugzilla/show_bug.cgi?id=28688>
>
> Thanks,
> Florian
>


-- 
H.J.

Re: [PATCH] elf: Set p_align to the common page size if possible

[Florian Weimer]

* H. J. Lu:

> On Mon, Dec 20, 2021 at 10:51 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > We need to set larger p_align > sh_addralign for huge page executables.
>> > My current algorithm to decide if p_align should be used as the maximum
>> > page size for objcopy is
>> >
>> > static bool
>> > elf_is_p_align_valid (bfd *abfd)
>> > {
>> >   unsigned int i;
>> >   Elf_Internal_Phdr *segment;
>> >   unsigned int num_segments;
>> >   const struct elf_backend_data *bed = get_elf_backend_data (abfd);
>> >   bfd_size_type maxpagesize = bed->maxpagesize;
>> >   bfd_size_type commonpagesize = bed->commonpagesize;
>> >
>> >   if (commonpagesize == maxpagesize)
>> >     return true;
>> >
>> >   /* When the common page size != the maximum page size, p_align may
>> >      be set to the common page size while segments are aligned to
>> >      the maximum page size.  In this case, the input p_align will be
>> >      ignored and the maximum page size will be used to align the output
>> >      segments.  */
>> >   segment = elf_tdata (abfd)->phdr;
>> >   num_segments = elf_elfheader (abfd)->e_phnum;
>> >   for (i = 0; i < num_segments; i++, segment++)
>> >     if (segment->p_type == PT_LOAD
>> >         && (segment->p_align != commonpagesize
>> >             || vma_page_aligned_bias (segment->p_vaddr,
>> >                                       segment->p_offset,
>> >                                       maxpagesize) != 0))
>> >       return true;
>> >
>> >   return false;
>> > }
>> >
>> > It should cover all cases.
>>
>> Will this switch from 64K to 4K on POWER and AArch64?  I think this will
>
> It will set p_align to ELF_COMMONPAGESIZE.  It glibc doesn't support
> p_align == ELF_COMMONPAGESIZE, the ELF_COMMONPAGESIZE
> is wrong.

Hmm.

bfd/elf32-arm.c:#define ELF_COMMONPAGESIZE              0x1000
bfd/elf32-arm.c:#undef  ELF_COMMONPAGESIZE
bfd/elf32-arm.c:#undef  ELF_COMMONPAGESIZE
bfd/elf32-arm.c:#define ELF_COMMONPAGESIZE              0x1000
bfd/elf32-ppc.c:#define ELF_COMMONPAGESIZE      0x1000
bfd/elf64-ppc.c:#define ELF_COMMONPAGESIZE      0x1000
bfd/elfnn-aarch64.c:#define ELF_COMMONPAGESIZE          0x1000

This looks problematic.

We have a sed patch in binutils.spec to change ELF_COMMONPAGESIZE:

# On ppc64 and aarch64, we might use 64KiB pages
sed -i -e '/#define.*ELF_COMMONPAGESIZE/s/0x1000$/0x10000/' bfd/elf*ppc.c
sed -i -e '/#define.*ELF_COMMONPAGESIZE/s/0x1000$/0x10000/' bfd/elf*aarch64.c
sed -i -e '/common_pagesize/s/4 /64 /' gold/powerpc.cc
sed -i -e '/pagesize/s/0x1000,/0x10000,/' gold/aarch64.cc

So I guess we paper over the problem.  But we reject to run-time load
binaries without this patch due to bug 28688.

I think we need to fix glibc bug 28688 and drop our downstream
workaround from our binutils builds.  We tried to fix it in the wrong
place, it seems.

This lack of cross-distro interoperability is embarrassing. 8-(

Thanks,
Florian

Re: [PATCH] elf: Set p_align to the common page size if possible

[H.J. Lu]

On Mon, Dec 20, 2021 at 11:25 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Mon, Dec 20, 2021 at 10:51 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * H. J. Lu:
> >>
> >> > We need to set larger p_align > sh_addralign for huge page executables.
> >> > My current algorithm to decide if p_align should be used as the maximum
> >> > page size for objcopy is
> >> >
> >> > static bool
> >> > elf_is_p_align_valid (bfd *abfd)
> >> > {
> >> >   unsigned int i;
> >> >   Elf_Internal_Phdr *segment;
> >> >   unsigned int num_segments;
> >> >   const struct elf_backend_data *bed = get_elf_backend_data (abfd);
> >> >   bfd_size_type maxpagesize = bed->maxpagesize;
> >> >   bfd_size_type commonpagesize = bed->commonpagesize;
> >> >
> >> >   if (commonpagesize == maxpagesize)
> >> >     return true;
> >> >
> >> >   /* When the common page size != the maximum page size, p_align may
> >> >      be set to the common page size while segments are aligned to
> >> >      the maximum page size.  In this case, the input p_align will be
> >> >      ignored and the maximum page size will be used to align the output
> >> >      segments.  */
> >> >   segment = elf_tdata (abfd)->phdr;
> >> >   num_segments = elf_elfheader (abfd)->e_phnum;
> >> >   for (i = 0; i < num_segments; i++, segment++)
> >> >     if (segment->p_type == PT_LOAD
> >> >         && (segment->p_align != commonpagesize
> >> >             || vma_page_aligned_bias (segment->p_vaddr,
> >> >                                       segment->p_offset,
> >> >                                       maxpagesize) != 0))
> >> >       return true;
> >> >
> >> >   return false;
> >> > }
> >> >
> >> > It should cover all cases.
> >>
> >> Will this switch from 64K to 4K on POWER and AArch64?  I think this will
> >
> > It will set p_align to ELF_COMMONPAGESIZE.  It glibc doesn't support
> > p_align == ELF_COMMONPAGESIZE, the ELF_COMMONPAGESIZE
> > is wrong.
>
> Hmm.
>
> bfd/elf32-arm.c:#define ELF_COMMONPAGESIZE              0x1000
> bfd/elf32-arm.c:#undef  ELF_COMMONPAGESIZE
> bfd/elf32-arm.c:#undef  ELF_COMMONPAGESIZE
> bfd/elf32-arm.c:#define ELF_COMMONPAGESIZE              0x1000
> bfd/elf32-ppc.c:#define ELF_COMMONPAGESIZE      0x1000
> bfd/elf64-ppc.c:#define ELF_COMMONPAGESIZE      0x1000
> bfd/elfnn-aarch64.c:#define ELF_COMMONPAGESIZE          0x1000
>
> This looks problematic.
>
> We have a sed patch in binutils.spec to change ELF_COMMONPAGESIZE:
>
> # On ppc64 and aarch64, we might use 64KiB pages
> sed -i -e '/#define.*ELF_COMMONPAGESIZE/s/0x1000$/0x10000/' bfd/elf*ppc.c
> sed -i -e '/#define.*ELF_COMMONPAGESIZE/s/0x1000$/0x10000/' bfd/elf*aarch64.c
> sed -i -e '/common_pagesize/s/4 /64 /' gold/powerpc.cc
> sed -i -e '/pagesize/s/0x1000,/0x10000,/' gold/aarch64.cc

This should be fixed in binutils upstream.

> So I guess we paper over the problem.  But we reject to run-time load
> binaries without this patch due to bug 28688.
>
> I think we need to fix glibc bug 28688 and drop our downstream
> workaround from our binutils builds.  We tried to fix it in the wrong
> place, it seems.
>
> This lack of cross-distro interoperability is embarrassing. 8-(
>
> Thanks,
> Florian
>


-- 
H.J.

Re: [PATCH] elf: Set p_align to the common page size if possible

[Florian Weimer]

* H. J. Lu:

> On Mon, Dec 20, 2021 at 11:25 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > On Mon, Dec 20, 2021 at 10:51 AM Florian Weimer <fweimer@redhat.com> wrote:
>> >>
>> >> * H. J. Lu:
>> >>
>> >> > We need to set larger p_align > sh_addralign for huge page executables.
>> >> > My current algorithm to decide if p_align should be used as the maximum
>> >> > page size for objcopy is
>> >> >
>> >> > static bool
>> >> > elf_is_p_align_valid (bfd *abfd)
>> >> > {
>> >> >   unsigned int i;
>> >> >   Elf_Internal_Phdr *segment;
>> >> >   unsigned int num_segments;
>> >> >   const struct elf_backend_data *bed = get_elf_backend_data (abfd);
>> >> >   bfd_size_type maxpagesize = bed->maxpagesize;
>> >> >   bfd_size_type commonpagesize = bed->commonpagesize;
>> >> >
>> >> >   if (commonpagesize == maxpagesize)
>> >> >     return true;
>> >> >
>> >> >   /* When the common page size != the maximum page size, p_align may
>> >> >      be set to the common page size while segments are aligned to
>> >> >      the maximum page size.  In this case, the input p_align will be
>> >> >      ignored and the maximum page size will be used to align the output
>> >> >      segments.  */
>> >> >   segment = elf_tdata (abfd)->phdr;
>> >> >   num_segments = elf_elfheader (abfd)->e_phnum;
>> >> >   for (i = 0; i < num_segments; i++, segment++)
>> >> >     if (segment->p_type == PT_LOAD
>> >> >         && (segment->p_align != commonpagesize
>> >> >             || vma_page_aligned_bias (segment->p_vaddr,
>> >> >                                       segment->p_offset,
>> >> >                                       maxpagesize) != 0))
>> >> >       return true;
>> >> >
>> >> >   return false;
>> >> > }
>> >> >
>> >> > It should cover all cases.
>> >>
>> >> Will this switch from 64K to 4K on POWER and AArch64?  I think this will
>> >
>> > It will set p_align to ELF_COMMONPAGESIZE.  It glibc doesn't support
>> > p_align == ELF_COMMONPAGESIZE, the ELF_COMMONPAGESIZE
>> > is wrong.
>>
>> Hmm.
>>
>> bfd/elf32-arm.c:#define ELF_COMMONPAGESIZE              0x1000
>> bfd/elf32-arm.c:#undef  ELF_COMMONPAGESIZE
>> bfd/elf32-arm.c:#undef  ELF_COMMONPAGESIZE
>> bfd/elf32-arm.c:#define ELF_COMMONPAGESIZE              0x1000
>> bfd/elf32-ppc.c:#define ELF_COMMONPAGESIZE      0x1000
>> bfd/elf64-ppc.c:#define ELF_COMMONPAGESIZE      0x1000
>> bfd/elfnn-aarch64.c:#define ELF_COMMONPAGESIZE          0x1000
>>
>> This looks problematic.
>>
>> We have a sed patch in binutils.spec to change ELF_COMMONPAGESIZE:
>>
>> # On ppc64 and aarch64, we might use 64KiB pages
>> sed -i -e '/#define.*ELF_COMMONPAGESIZE/s/0x1000$/0x10000/' bfd/elf*ppc.c
>> sed -i -e '/#define.*ELF_COMMONPAGESIZE/s/0x1000$/0x10000/' bfd/elf*aarch64.c
>> sed -i -e '/common_pagesize/s/4 /64 /' gold/powerpc.cc
>> sed -i -e '/pagesize/s/0x1000,/0x10000,/' gold/aarch64.cc
>
> This should be fixed in binutils upstream.

I've brought this up internally.  To me it looks like binutils is fine,
we tried to workaround the strict p_align stricting in glibc (bug 28688)
for some reason.  This was fifteen years ago, so we'll see if we can
still figure out why we did it this way.

Thanks,
Florian

Re: [PATCH] elf: Set p_align to the common page size if possible

[H.J. Lu]

On Wed, Dec 15, 2021 at 8:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> Currently, on 32-bit and 64-bit ARM, it seems that ld generates p_align
> values of 0x10000 even if no section alignment is greater than 0x1000.
> The issue is more general and probably affects other targets with
> multiple common page sizes.
>
> While file layout absolutely must take 64K page size into account, that
> does not have to be reflected in the p_align value.  If running on a 64K
> kernel, the file will be loaded at a 64K page boundary by necessity. On
> a 4K kernel, 64K alignment is not needed.
>
> The glibc loader has been fixed to honor p_align:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=28676
>
> similar to kernel:
>
> commit ce81bb256a224259ab686742a6284930cbe4f1fa
> Author: Chris Kennelly <ckennelly@google.com>
> Date:   Thu Oct 15 20:12:32 2020 -0700
>
>     fs/binfmt_elf: use PT_LOAD p_align values for suitable start address
>
> This means that on 4K kernels, we will start to do extra work for 64K
> p_align, but this pointless for pretty much all binaries (whose section
> alignment rarely exceeds 16).
>
> 1. Set p_align to common page size while laying out segments aligning to
> maximum page size or section alignment.  The run-time loader can align
> segments to common page size or maximum page size, depending on system
> page size.
> 2. If -z max-page-size=NNN is used, p_align will be set to maximum page
> size or the largest section alignment.
> 3. If a section requires alignment higher than the common page size,
> don't set p_align to the common page size.
> 4. If a section requires alignment higher than the maximum page size,
> set p_align to the section alignment.
> 5. For objcopy, when the commone page size != the maximum page size,
> p_align may be set to the commone page size while segments are aligned
> to the maximum page size.  In this case, the input p_align will be
> ignored and the maximum page size will be used to align the ouput
> segments.
> 6. Update linker to disallow the commone page size > the maximum page
> size.
>
> bfd/
>
>         PR ld/28689
>         PR ld/28695
>         * elf.c (assign_file_positions_for_load_sections): Set p_align
>         to common page size while laying out segments aligning to
>         maximum page size or section alignment.
>         (elf_is_p_align_valid): New function.
>         (copy_elf_program_header): Call elf_is_p_align_valid to determine
>         if p_align is valid.
>
> include/
>
>         PR ld/28689
>         PR ld/28695
>         * bfdlink.h (bfd_link_info): Add maxpagesize_is_set.
>
> ld/
>
>         PR ld/28689
>         PR ld/28695
>         * emultempl/elf.em (gld${EMULATION_NAME}_handle_option): Set
>         link_info.maxpagesize_is_set for -z max-page-size=NNN.
>         * ldelf.c (ldelf_after_parse): Disallow link_info.commonpagesize
>         > link_info.maxpagesize.
>         * testsuite/ld-elf/elf.exp: Pass -z max-page-size=0x4000 to
>         linker to build mbind2a and mbind2b.
>         * testsuite/ld-elf/header.d: Add -z common-page-size=0x100.
>         * testsuite/ld-elf/linux-x86.exp: Add PR ld/28689 tests.
>         * testsuite/ld-elf/p_align-1.c: New file.
>         * testsuite/ld-elf/page-size-1.d: New test.
> ---
>  bfd/elf.c                         | 78 +++++++++++++++++++++++++++++--
>  include/bfdlink.h                 |  3 ++
>  ld/emultempl/elf.em               |  1 +
>  ld/ldelf.c                        |  3 ++
>  ld/testsuite/ld-elf/elf.exp       |  4 +-
>  ld/testsuite/ld-elf/header.d      |  2 +-
>  ld/testsuite/ld-elf/linux-x86.exp | 36 ++++++++++++++
>  ld/testsuite/ld-elf/p_align-1.c   | 25 ++++++++++
>  ld/testsuite/ld-elf/page-size-1.d |  4 ++
>  9 files changed, 149 insertions(+), 7 deletions(-)
>  create mode 100644 ld/testsuite/ld-elf/p_align-1.c
>  create mode 100644 ld/testsuite/ld-elf/page-size-1.d
>
> diff --git a/bfd/elf.c b/bfd/elf.c
> index e6c6a8a6c05..431084760ab 100644
> --- a/bfd/elf.c
> +++ b/bfd/elf.c
> @@ -5406,6 +5406,8 @@ assign_file_positions_for_load_sections (bfd *abfd,
>    Elf_Internal_Phdr *p;
>    file_ptr off;  /* Octets.  */
>    bfd_size_type maxpagesize;
> +  bfd_size_type commonpagesize;
> +  bool p_align_commonpagesize_p = false;
>    unsigned int alloc, actual;
>    unsigned int i, j;
>    struct elf_segment_map **sorted_seg_map;
> @@ -5491,12 +5493,19 @@ assign_file_positions_for_load_sections (bfd *abfd,
>            elf_sort_segments);
>
>    maxpagesize = 1;
> +  commonpagesize = 1;
>    if ((abfd->flags & D_PAGED) != 0)
>      {
>        if (link_info != NULL)
> -       maxpagesize = link_info->maxpagesize;
> +       {
> +         maxpagesize = link_info->maxpagesize;
> +         commonpagesize = link_info->commonpagesize;
> +       }
>        else
> -       maxpagesize = bed->maxpagesize;
> +       {
> +         maxpagesize = bed->maxpagesize;
> +         commonpagesize = bed->commonpagesize;
> +       }
>      }
>
>    /* Sections must map to file offsets past the ELF file header.  */
> @@ -5560,6 +5569,13 @@ assign_file_positions_for_load_sections (bfd *abfd,
>              segment.  */
>           if (m->p_align_valid)
>             maxpagesize = m->p_align;
> +         else if (link_info == NULL || !link_info->maxpagesize_is_set)
> +           /* Set p_align to common page size while laying out segments
> +              aligning to the maximum page size or the largest section
> +              alignment.  The run-time loader can align segments to the
> +              common page size or the maximum page size, depending on
> +              system page size.  */
> +           p_align_commonpagesize_p = true;
>
>           p->p_align = maxpagesize;
>         }
> @@ -5597,7 +5613,22 @@ assign_file_positions_for_load_sections (bfd *abfd,
>                 }
>               align = (bfd_size_type) 1 << align_power;
>               if (align < maxpagesize)
> -               align = maxpagesize;
> +               {
> +                 /* If a section requires alignment higher than the
> +                    common page size, don't set p_align to the common
> +                    page size.  */
> +                 if (align > commonpagesize)
> +                   p_align_commonpagesize_p = false;
> +                 align = maxpagesize;
> +               }
> +             else
> +               {
> +                 /* If a section requires alignment higher than the
> +                    maximum page size, set p_align to the section
> +                    alignment.  */
> +                 p_align_commonpagesize_p = true;
> +                 commonpagesize = align;
> +               }
>             }
>
>           for (i = 0; i < m->count; i++)
> @@ -5976,6 +6007,9 @@ assign_file_positions_for_load_sections (bfd *abfd,
>                   print_segment_map (m);
>                 }
>             }
> +
> +         if (p_align_commonpagesize_p)
> +           p->p_align = commonpagesize;
>         }
>      }
>
> @@ -7485,6 +7519,39 @@ rewrite_elf_program_header (bfd *ibfd, bfd *obfd, bfd_vma maxpagesize)
>    return true;
>  }
>
> +/* Return true if p_align in the ELF program header in ABFD is valid.  */
> +
> +static bool
> +elf_is_p_align_valid (bfd *abfd)
> +{
> +  unsigned int i;
> +  Elf_Internal_Phdr *segment;
> +  unsigned int num_segments;
> +  const struct elf_backend_data *bed = get_elf_backend_data (abfd);
> +  bfd_size_type maxpagesize = bed->maxpagesize;
> +  bfd_size_type commonpagesize = bed->commonpagesize;
> +
> +  if (commonpagesize == maxpagesize)
> +    return true;
> +
> +  /* When the commone page size != the maximum page size, p_align may
> +     be set to the commone page size while segments are aligned to
> +     the maximum page size.  In this case, the input p_align will be
> +     ignored and the maximum page size will be used to align the ouput
> +     segments.  */
> +  segment = elf_tdata (abfd)->phdr;
> +  num_segments = elf_elfheader (abfd)->e_phnum;
> +  for (i = 0; i < num_segments; i++, segment++)
> +    if (segment->p_type == PT_LOAD
> +       && (segment->p_align != commonpagesize
> +           || vma_page_aligned_bias (segment->p_vaddr,
> +                                     segment->p_offset,
> +                                     maxpagesize) != 0))
> +      return true;
> +
> +  return false;
> +}
> +
>  /* Copy ELF program header information.  */
>
>  static bool
> @@ -7499,6 +7566,7 @@ copy_elf_program_header (bfd *ibfd, bfd *obfd)
>    unsigned int num_segments;
>    bool phdr_included = false;
>    bool p_paddr_valid;
> +  bool p_palign_valid;
>    unsigned int opb = bfd_octets_per_byte (ibfd, NULL);
>
>    iehdr = elf_elfheader (ibfd);
> @@ -7519,6 +7587,8 @@ copy_elf_program_header (bfd *ibfd, bfd *obfd)
>         break;
>        }
>
> +  p_palign_valid = elf_is_p_align_valid (ibfd);
> +
>    for (i = 0, segment = elf_tdata (ibfd)->phdr;
>         i < num_segments;
>         i++, segment++)
> @@ -7561,7 +7631,7 @@ copy_elf_program_header (bfd *ibfd, bfd *obfd)
>        map->p_paddr = segment->p_paddr;
>        map->p_paddr_valid = p_paddr_valid;
>        map->p_align = segment->p_align;
> -      map->p_align_valid = 1;
> +      map->p_align_valid = p_palign_valid;
>        map->p_vaddr_offset = 0;
>
>        if (map->p_type == PT_GNU_RELRO
> diff --git a/include/bfdlink.h b/include/bfdlink.h
> index 566529ee644..8cf34b05c1a 100644
> --- a/include/bfdlink.h
> +++ b/include/bfdlink.h
> @@ -525,6 +525,9 @@ struct bfd_link_info
>    /* TRUE if all symbol names should be unique.  */
>    unsigned int unique_symbol : 1;
>
> +  /* TRUE if maxpagesize is set on command-line.  */
> +  unsigned int maxpagesize_is_set : 1;
> +
>    /* Char that may appear as the first char of a symbol, but should be
>       skipped (like symbol_leading_char) when looking up symbols in
>       wrap_hash.  Used by PowerPC Linux for 'dot' symbols.  */
> diff --git a/ld/emultempl/elf.em b/ld/emultempl/elf.em
> index bfaf8130a3e..5eadab9f4b9 100644
> --- a/ld/emultempl/elf.em
> +++ b/ld/emultempl/elf.em
> @@ -721,6 +721,7 @@ fragment <<EOF
>               || (link_info.maxpagesize & (link_info.maxpagesize - 1)) != 0)
>             einfo (_("%F%P: invalid maximum page size \`%s'\n"),
>                    optarg + 14);
> +         link_info.maxpagesize_is_set = true;
>         }
>        else if (startswith (optarg, "common-page-size="))
>         {
> diff --git a/ld/ldelf.c b/ld/ldelf.c
> index 529992b02ae..ee09df5f35c 100644
> --- a/ld/ldelf.c
> +++ b/ld/ldelf.c
> @@ -72,6 +72,9 @@ ldelf_after_parse (void)
>        link_info.dynamic_undefined_weak = 0;
>      }
>    after_parse_default ();
> +  if (link_info.commonpagesize > link_info.maxpagesize)
> +    einfo (_("%F%P: common page size (0x%v) > maximum page size (0x%v)\n"),
> +          link_info.commonpagesize, link_info.maxpagesize);
>  }
>
>  /* Handle the generation of DT_NEEDED tags.  */
> diff --git a/ld/testsuite/ld-elf/elf.exp b/ld/testsuite/ld-elf/elf.exp
> index 01d22faad9a..ae8f76db1c7 100644
> --- a/ld/testsuite/ld-elf/elf.exp
> +++ b/ld/testsuite/ld-elf/elf.exp
> @@ -365,7 +365,7 @@ if { [istarget *-*-linux*]
>      run_ld_link_exec_tests [list \
>         [list \
>             "Run mbind2a" \
> -           "$NOPIE_LDFLAGS -Wl,-z,common-page-size=0x4000" \
> +           "$NOPIE_LDFLAGS -Wl,-z,common-page-size=0x4000,-z,max-page-size=0x4000" \
>             "" \
>             { mbind2a.s mbind2b.c } \
>             "mbind2a" \
> @@ -374,7 +374,7 @@ if { [istarget *-*-linux*]
>         ] \
>         [list \
>             "Run mbind2b" \
> -           "-static -Wl,-z,common-page-size=0x4000" \
> +           "-static -Wl,-z,common-page-size=0x4000,-z,max-page-size=0x4000" \
>             "" \
>             { mbind2a.s mbind2b.c } \
>             "mbind2b" \
> diff --git a/ld/testsuite/ld-elf/header.d b/ld/testsuite/ld-elf/header.d
> index c4d174a98da..67f0c981920 100644
> --- a/ld/testsuite/ld-elf/header.d
> +++ b/ld/testsuite/ld-elf/header.d
> @@ -1,5 +1,5 @@
>  # target: *-*-linux* *-*-gnu* *-*-vxworks arm*-*-uclinuxfdpiceabi
> -# ld: -T header.t -z max-page-size=0x100
> +# ld: -T header.t -z max-page-size=0x100 -z common-page-size=0x100
>  # objdump: -hpw
>
>  #...
> diff --git a/ld/testsuite/ld-elf/linux-x86.exp b/ld/testsuite/ld-elf/linux-x86.exp
> index ee03b565faf..25a8d0411d9 100644
> --- a/ld/testsuite/ld-elf/linux-x86.exp
> +++ b/ld/testsuite/ld-elf/linux-x86.exp
> @@ -185,6 +185,42 @@ run_ld_link_exec_tests [list \
>         "" \
>         "tmpdir/indirect-extern-access-2.so" \
>      ] \
> +    [list \
> +       "Run p_align-1a without PIE" \
> +       "$NOPIE_LDFLAGS" \
> +       "" \
> +       { p_align-1.c } \
> +       "p_align-1a" \
> +       "pass.out" \
> +       "$NOPIE_CFLAGS" \
> +    ] \
> +    [list \
> +       "Run p_align-1b with PIE" \
> +       "-pie" \
> +       "" \
> +       { p_align-1.c } \
> +       "p_align-1b" \
> +       "pass.out" \
> +       "-fpie" \
> +    ] \
> +    [list \
> +       "Run p_align-1c with -Wl,-z,max-page-size=0x1000 without PIE" \
> +       "$NOPIE_LDFLAGS -Wl,-z,max-page-size=0x1000" \
> +       "" \
> +       { p_align-1.c } \
> +       "p_align-1c" \
> +       "pass.out" \
> +       "$NOPIE_CFLAGS" \
> +    ] \
> +    [list \
> +       "Run p_align-1d with -Wl,-z,max-page-size=0x1000 with PIE" \
> +       "-pie -Wl,-z,max-page-size=0x1000" \
> +       "" \
> +       { p_align-1.c } \
> +       "p_align-1d" \
> +       "pass.out" \
> +       "-fpie" \
> +    ] \
>  ]
>
>  proc elfedit_test { options test output } {
> diff --git a/ld/testsuite/ld-elf/p_align-1.c b/ld/testsuite/ld-elf/p_align-1.c
> new file mode 100644
> index 00000000000..6579cd74e52
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/p_align-1.c
> @@ -0,0 +1,25 @@
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +
> +#ifndef ALIGN
> +# define ALIGN 0x800000
> +#endif
> +
> +int
> +__attribute__ ((weak))
> +is_aligned (void *p, int align)
> +{
> +  return (((uintptr_t) p) & (align - 1)) == 0;
> +}
> +
> +int foo __attribute__ ((aligned (ALIGN))) = 1;
> +
> +int
> +main (void)
> +{
> +  if (!is_aligned (&foo, ALIGN))
> +    abort ();
> +  printf ("PASS\n");
> +  return 0;
> +}
> diff --git a/ld/testsuite/ld-elf/page-size-1.d b/ld/testsuite/ld-elf/page-size-1.d
> new file mode 100644
> index 00000000000..04d2153b2f9
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/page-size-1.d
> @@ -0,0 +1,4 @@
> +#source: dummy.s
> +#ld: -z common-page-size=0x4000 -z max-page-size=0x1000
> +#error: common page size \(0x4000\) > maximum page size \(0x1000\)
> +#target: *-*-linux-gnu *-*-gnu* arm*-*-uclinuxfdpiceabi
> --
> 2.33.1
>

Hi Nick, Alan,

Can you review this patch?

Thanks.

-- 
H.J.

Re: [PATCH] elf: Set p_align to the common page size if possible

[Alan Modra]

On Mon, Dec 20, 2021 at 07:50:30AM -0800, H.J. Lu wrote:
> On Wed, Dec 15, 2021 at 8:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > Currently, on 32-bit and 64-bit ARM, it seems that ld generates p_align
> > values of 0x10000 even if no section alignment is greater than 0x1000.
> > The issue is more general and probably affects other targets with
> > multiple common page sizes.
> >
> > While file layout absolutely must take 64K page size into account, that
> > does not have to be reflected in the p_align value.  If running on a 64K
> > kernel, the file will be loaded at a 64K page boundary by necessity. On
> > a 4K kernel, 64K alignment is not needed.
> >
> > The glibc loader has been fixed to honor p_align:
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=28676
> >
> > similar to kernel:
> >
> > commit ce81bb256a224259ab686742a6284930cbe4f1fa
> > Author: Chris Kennelly <ckennelly@google.com>
> > Date:   Thu Oct 15 20:12:32 2020 -0700
> >
> >     fs/binfmt_elf: use PT_LOAD p_align values for suitable start address

The ELF gABI says of p_align:
p_align
    As ``Program Loading'' describes in this chapter of the processor
    supplement, loadable process segments must have congruent values
    for p_vaddr and p_offset, modulo the page size. This member gives
    the value to which the segments are aligned in memory and in the
    file. Values 0 and 1 mean no alignment is required. Otherwise,
    p_align should be a positive, integral power of 2, and p_vaddr
    should equal p_offset, modulo p_align.

Notice the parallel between page size and p_align.  I believe it was
reasonable for ld to set p_align to the maximum page size supported by
the segment layout, if you treat p_align as a hint.  Now that p_align
is treated as required alignment, I agree that we should not set
p_align to maxpagesize.  However, why choose commonpagesize?  Is
anything more than the maximum section alignment in any given segment
needed?

Imagine a binary created with maxpagesize of 64k, commonpagesize of
16k, and running with actual page size of 4k.  If section alignment
requirement is less than 4k, why should it be aligned to 16k?

-- 
Alan Modra
Australia Development Lab, IBM

[PATCH v2] elf: Set p_align to the minimum page size if possible

[H.J. Lu]

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

On Wed, Dec 22, 2021 at 12:57 AM Alan Modra <amodra@gmail.com> wrote:
>
> On Mon, Dec 20, 2021 at 07:50:30AM -0800, H.J. Lu wrote:
> > On Wed, Dec 15, 2021 at 8:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > Currently, on 32-bit and 64-bit ARM, it seems that ld generates p_align
> > > values of 0x10000 even if no section alignment is greater than 0x1000.
> > > The issue is more general and probably affects other targets with
> > > multiple common page sizes.
> > >
> > > While file layout absolutely must take 64K page size into account, that
> > > does not have to be reflected in the p_align value.  If running on a 64K
> > > kernel, the file will be loaded at a 64K page boundary by necessity. On
> > > a 4K kernel, 64K alignment is not needed.
> > >
> > > The glibc loader has been fixed to honor p_align:
> > >
> > > https://sourceware.org/bugzilla/show_bug.cgi?id=28676
> > >
> > > similar to kernel:
> > >
> > > commit ce81bb256a224259ab686742a6284930cbe4f1fa
> > > Author: Chris Kennelly <ckennelly@google.com>
> > > Date:   Thu Oct 15 20:12:32 2020 -0700
> > >
> > >     fs/binfmt_elf: use PT_LOAD p_align values for suitable start address
>
> The ELF gABI says of p_align:
> p_align
>     As ``Program Loading'' describes in this chapter of the processor
>     supplement, loadable process segments must have congruent values
>     for p_vaddr and p_offset, modulo the page size. This member gives
>     the value to which the segments are aligned in memory and in the
>     file. Values 0 and 1 mean no alignment is required. Otherwise,
>     p_align should be a positive, integral power of 2, and p_vaddr
>     should equal p_offset, modulo p_align.
>
> Notice the parallel between page size and p_align.  I believe it was
> reasonable for ld to set p_align to the maximum page size supported by
> the segment layout, if you treat p_align as a hint.  Now that p_align
> is treated as required alignment, I agree that we should not set
> p_align to maxpagesize.  However, why choose commonpagesize?  Is
> anything more than the maximum section alignment in any given segment
> needed?

I choose commonpagesize due to this glibc bug:

https://sourceware.org/bugzilla/show_bug.cgi?id=28688

It has been fixed in glibc 2.35.  But linker output must work on
existing glibc binaries.

> Imagine a binary created with maxpagesize of 64k, commonpagesize of
> 16k, and running with actual page size of 4k.  If section alignment
> requirement is less than 4k, why should it be aligned to 16k?
>

Here is the v2 patch to set p_align to the minimum page size, instead of
the common page size.

OK for master?

Thanks.

--
H.J.

[-- Attachment #2: v2-0001-elf-Set-p_align-to-the-minimum-page-size-if-possi.patch --]
[-- Type: application/x-patch, Size: 13030 bytes --]

Re: [PATCH v2] elf: Set p_align to the minimum page size if possible

[Alan Modra]

On Wed, Dec 22, 2021 at 06:38:54AM -0800, H.J. Lu wrote:
> I choose commonpagesize due to this glibc bug:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=28688

This means we are stuck.  Choosing commonpagesize or minpagesize will
generate binaries that won't run with older glibc if the actual page
size is maxpagesize (assuming maxpagesize != commonpagesize).

I don't see a way to change DT_LOAD p_align that is backwards
compatible, except perhaps using a p_align of zero if the required
segment alignment (as calculated from section alignment) is less than
maxpagesize.  That horrible hack just happens to work for older glibc,
but may break other loaders.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH v2] elf: Set p_align to the minimum page size if possible

[H.J. Lu]

On Fri, Dec 24, 2021 at 5:03 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Wed, Dec 22, 2021 at 06:38:54AM -0800, H.J. Lu wrote:
> > I choose commonpagesize due to this glibc bug:
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=28688
>
> This means we are stuck.  Choosing commonpagesize or minpagesize will
> generate binaries that won't run with older glibc if the actual page
> size is maxpagesize (assuming maxpagesize != commonpagesize).

We can make this new behavior as opt-in since this is an optimization
for ld.so.

> I don't see a way to change DT_LOAD p_align that is backwards
> compatible, except perhaps using a p_align of zero if the required
> segment alignment (as calculated from section alignment) is less than
> maxpagesize.  That horrible hack just happens to work for older glibc,
> but may break other loaders.

Neither 0 nor 1 work for older glibc:

          if (__glibc_unlikely ((ph->p_align & (GLRO(dl_pagesize) - 1)) != 0))
            {
              errstring = N_("ELF load command alignment not page-aligned");
              goto lose;
            }
          if (__glibc_unlikely (((ph->p_vaddr - ph->p_offset)
                                 & (ph->p_align - 1)) != 0))
            {
              errstring
                = N_("ELF load command address/offset not properly aligned");
              goto lose;
            }

--
H.J.

Re: [PATCH v2] elf: Set p_align to the minimum page size if possible

[Alan Modra]

On Fri, Dec 24, 2021 at 06:50:30PM -0800, H.J. Lu wrote:
> On Fri, Dec 24, 2021 at 5:03 PM Alan Modra <amodra@gmail.com> wrote:
> > I don't see a way to change DT_LOAD p_align that is backwards
> > compatible, except perhaps using a p_align of zero if the required
> > segment alignment (as calculated from section alignment) is less than
> > maxpagesize.  That horrible hack just happens to work for older glibc,
> > but may break other loaders.
> 
> Neither 0 nor 1 work for older glibc:
> 
>           if (__glibc_unlikely ((ph->p_align & (GLRO(dl_pagesize) - 1)) != 0))
>             {
>               errstring = N_("ELF load command alignment not page-aligned");
>               goto lose;
>             }
>           if (__glibc_unlikely (((ph->p_vaddr - ph->p_offset)
>                                  & (ph->p_align - 1)) != 0))
>             {
>               errstring
>                 = N_("ELF load command address/offset not properly aligned");
>               goto lose;
>             }

Zero in p_align triggers neither of these conditions.  Binaries with
zero p_align run.  I checked.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH v2] elf: Set p_align to the minimum page size if possible

[H.J. Lu]

On Fri, Dec 24, 2021 at 7:04 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Fri, Dec 24, 2021 at 06:50:30PM -0800, H.J. Lu wrote:
> > On Fri, Dec 24, 2021 at 5:03 PM Alan Modra <amodra@gmail.com> wrote:
> > > I don't see a way to change DT_LOAD p_align that is backwards
> > > compatible, except perhaps using a p_align of zero if the required
> > > segment alignment (as calculated from section alignment) is less than
> > > maxpagesize.  That horrible hack just happens to work for older glibc,
> > > but may break other loaders.
> >
> > Neither 0 nor 1 work for older glibc:
> >
> >           if (__glibc_unlikely ((ph->p_align & (GLRO(dl_pagesize) - 1)) != 0))
> >             {
> >               errstring = N_("ELF load command alignment not page-aligned");
> >               goto lose;
> >             }
> >           if (__glibc_unlikely (((ph->p_vaddr - ph->p_offset)
> >                                  & (ph->p_align - 1)) != 0))
> >             {
> >               errstring
> >                 = N_("ELF load command address/offset not properly aligned");
> >               goto lose;
> >             }
>
> Zero in p_align triggers neither of these conditions.  Binaries with
> zero p_align run.  I checked.
>

I tried setting p_align to 0 and got

ELF load command address/offset not properly aligned

For

 if (__glibc_unlikely (((ph->p_vaddr - ph->p_offset)
                                 & (ph->p_align - 1)) != 0))


If ph->p_vaddr - ph->p_offset == 0, it will trigger this
error:

  Type           Offset   VirtAddr           PhysAddr
FileSiz  MemSiz   Flg Align
  LOAD           0x000000 0x0000000000000000 0x0000000000000000
0x000578 0x000578 R   0x1000
  LOAD           0x001000 0x0000000000001000 0x0000000000001000
0x00012d 0x00012d R E 0x1000
  LOAD           0x002000 0x0000000000002000 0x0000000000002000
0x0000a4 0x0000a4 R   0x1000
  LOAD           0x002df8 0x0000000000003df8 0x0000000000003df8
0x00022c 0x000230 RW  0x1000

-- 
H.J.

Re: [PATCH v2] elf: Set p_align to the minimum page size if possible

[Alan Modra]

On Fri, Dec 24, 2021 at 07:08:57PM -0800, H.J. Lu wrote:
> On Fri, Dec 24, 2021 at 7:04 PM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Fri, Dec 24, 2021 at 06:50:30PM -0800, H.J. Lu wrote:
> > > On Fri, Dec 24, 2021 at 5:03 PM Alan Modra <amodra@gmail.com> wrote:
> > > > I don't see a way to change DT_LOAD p_align that is backwards
> > > > compatible, except perhaps using a p_align of zero if the required
> > > > segment alignment (as calculated from section alignment) is less than
> > > > maxpagesize.  That horrible hack just happens to work for older glibc,
> > > > but may break other loaders.
> > >
> > > Neither 0 nor 1 work for older glibc:
> > >
> > >           if (__glibc_unlikely ((ph->p_align & (GLRO(dl_pagesize) - 1)) != 0))
> > >             {
> > >               errstring = N_("ELF load command alignment not page-aligned");
> > >               goto lose;
> > >             }
> > >           if (__glibc_unlikely (((ph->p_vaddr - ph->p_offset)
> > >                                  & (ph->p_align - 1)) != 0))
> > >             {
> > >               errstring
> > >                 = N_("ELF load command address/offset not properly aligned");
> > >               goto lose;
> > >             }
> >
> > Zero in p_align triggers neither of these conditions.  Binaries with
> > zero p_align run.  I checked.
> >
> 
> I tried setting p_align to 0 and got
> 
> ELF load command address/offset not properly aligned

Yeah, silly me.  The binary I happened to test had p_offset equal to
p_vaddr.  So zero doesn't work.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] elf: Set p_align to the common page size if possible

[Nick Clifton]

Hi H.J.

> Please use the v3 patch:
> 
> https://sourceware.org/pipermail/binutils/2021-December/119047.html

Thanks - that fixed the problems I was encountering with the v2 patch.

Patch approved - please apply.

Cheers
   Nick