* [PATCH] elf: Set p_align to the common page size if possible
@ 2021-12-15 16:32 H.J. Lu
2021-12-16 8:52 ` Fangrui Song
2021-12-20 15:50 ` H.J. Lu
0 siblings, 2 replies; 19+ messages in thread
From: H.J. Lu @ 2021-12-15 16:32 UTC (permalink / raw)
To: binutils; +Cc: Florian Weimer
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 +-
| 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" \
--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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] elf: Set p_align to the common page size if possible
2021-12-15 16:32 [PATCH] elf: Set p_align to the common page size if possible H.J. Lu
@ 2021-12-16 8:52 ` Fangrui Song
2021-12-16 14:33 ` H.J. Lu
2021-12-20 16:36 ` Florian Weimer
2021-12-20 15:50 ` H.J. Lu
1 sibling, 2 replies; 19+ messages in thread
From: Fangrui Song @ 2021-12-16 8:52 UTC (permalink / raw)
To: H.J. Lu; +Cc: binutils, Florian Weimer
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] elf: Set p_align to the common page size if possible
2021-12-16 8:52 ` Fangrui Song
@ 2021-12-16 14:33 ` H.J. Lu
2021-12-20 16:36 ` Florian Weimer
1 sibling, 0 replies; 19+ messages in thread
From: H.J. Lu @ 2021-12-16 14:33 UTC (permalink / raw)
To: Fangrui Song; +Cc: Binutils, Florian Weimer
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] elf: Set p_align to the common page size if possible
2021-12-15 16:32 [PATCH] elf: Set p_align to the common page size if possible H.J. Lu
2021-12-16 8:52 ` Fangrui Song
@ 2021-12-20 15:50 ` H.J. Lu
2021-12-22 8:57 ` Alan Modra
[not found] ` <f64b9fb3-7d2d-7ff3-a8f6-795292af6744@redhat.com>
1 sibling, 2 replies; 19+ messages in thread
From: H.J. Lu @ 2021-12-20 15:50 UTC (permalink / raw)
To: Binutils, Alan Modra, Nick Clifton; +Cc: Florian Weimer
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] elf: Set p_align to the common page size if possible
2021-12-16 8:52 ` Fangrui Song
2021-12-16 14:33 ` H.J. Lu
@ 2021-12-20 16:36 ` Florian Weimer
2021-12-20 18:39 ` H.J. Lu
1 sibling, 1 reply; 19+ messages in thread
From: Florian Weimer @ 2021-12-20 16:36 UTC (permalink / raw)
To: Fangrui Song; +Cc: H.J. Lu, binutils
* 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] elf: Set p_align to the common page size if possible
2021-12-20 16:36 ` Florian Weimer
@ 2021-12-20 18:39 ` H.J. Lu
2021-12-20 18:51 ` Florian Weimer
0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-12-20 18:39 UTC (permalink / raw)
To: Florian Weimer; +Cc: Fangrui Song, Binutils
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] elf: Set p_align to the common page size if possible
2021-12-20 18:39 ` H.J. Lu
@ 2021-12-20 18:51 ` Florian Weimer
2021-12-20 18:59 ` H.J. Lu
0 siblings, 1 reply; 19+ messages in thread
From: Florian Weimer @ 2021-12-20 18:51 UTC (permalink / raw)
To: H.J. Lu; +Cc: Fangrui Song, Binutils
* 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] elf: Set p_align to the common page size if possible
2021-12-20 18:51 ` Florian Weimer
@ 2021-12-20 18:59 ` H.J. Lu
2021-12-20 19:25 ` Florian Weimer
0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-12-20 18:59 UTC (permalink / raw)
To: Florian Weimer; +Cc: Fangrui Song, Binutils
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] elf: Set p_align to the common page size if possible
2021-12-20 18:59 ` H.J. Lu
@ 2021-12-20 19:25 ` Florian Weimer
2021-12-20 19:31 ` H.J. Lu
0 siblings, 1 reply; 19+ messages in thread
From: Florian Weimer @ 2021-12-20 19:25 UTC (permalink / raw)
To: H.J. Lu; +Cc: Fangrui Song, Binutils
* 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] elf: Set p_align to the common page size if possible
2021-12-20 19:25 ` Florian Weimer
@ 2021-12-20 19:31 ` H.J. Lu
2021-12-20 19:51 ` Florian Weimer
0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-12-20 19:31 UTC (permalink / raw)
To: Florian Weimer; +Cc: Fangrui Song, Binutils
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] elf: Set p_align to the common page size if possible
2021-12-20 19:31 ` H.J. Lu
@ 2021-12-20 19:51 ` Florian Weimer
0 siblings, 0 replies; 19+ messages in thread
From: Florian Weimer @ 2021-12-20 19:51 UTC (permalink / raw)
To: H.J. Lu; +Cc: Fangrui Song, Binutils
* 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] elf: Set p_align to the common page size if possible
2021-12-20 15:50 ` H.J. Lu
@ 2021-12-22 8:57 ` Alan Modra
2021-12-22 14:38 ` [PATCH v2] elf: Set p_align to the minimum " H.J. Lu
[not found] ` <f64b9fb3-7d2d-7ff3-a8f6-795292af6744@redhat.com>
1 sibling, 1 reply; 19+ messages in thread
From: Alan Modra @ 2021-12-22 8:57 UTC (permalink / raw)
To: H.J. Lu; +Cc: Binutils, Nick Clifton, Florian Weimer
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] elf: Set p_align to the minimum page size if possible
2021-12-22 8:57 ` Alan Modra
@ 2021-12-22 14:38 ` H.J. Lu
2021-12-25 1:03 ` Alan Modra
0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-12-22 14:38 UTC (permalink / raw)
To: Alan Modra; +Cc: Binutils, Nick Clifton, Florian Weimer
[-- 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 --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] elf: Set p_align to the minimum page size if possible
2021-12-22 14:38 ` [PATCH v2] elf: Set p_align to the minimum " H.J. Lu
@ 2021-12-25 1:03 ` Alan Modra
2021-12-25 2:50 ` H.J. Lu
0 siblings, 1 reply; 19+ messages in thread
From: Alan Modra @ 2021-12-25 1:03 UTC (permalink / raw)
To: H.J. Lu; +Cc: Binutils, Nick Clifton, Florian Weimer
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] elf: Set p_align to the minimum page size if possible
2021-12-25 1:03 ` Alan Modra
@ 2021-12-25 2:50 ` H.J. Lu
2021-12-25 3:04 ` Alan Modra
0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-12-25 2:50 UTC (permalink / raw)
To: Alan Modra; +Cc: Binutils, Nick Clifton, Florian Weimer
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] elf: Set p_align to the minimum page size if possible
2021-12-25 2:50 ` H.J. Lu
@ 2021-12-25 3:04 ` Alan Modra
2021-12-25 3:08 ` H.J. Lu
0 siblings, 1 reply; 19+ messages in thread
From: Alan Modra @ 2021-12-25 3:04 UTC (permalink / raw)
To: H.J. Lu; +Cc: Binutils, Nick Clifton, Florian Weimer
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] elf: Set p_align to the minimum page size if possible
2021-12-25 3:04 ` Alan Modra
@ 2021-12-25 3:08 ` H.J. Lu
2021-12-25 3:58 ` Alan Modra
0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-12-25 3:08 UTC (permalink / raw)
To: Alan Modra; +Cc: Binutils, Nick Clifton, Florian Weimer
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] elf: Set p_align to the minimum page size if possible
2021-12-25 3:08 ` H.J. Lu
@ 2021-12-25 3:58 ` Alan Modra
0 siblings, 0 replies; 19+ messages in thread
From: Alan Modra @ 2021-12-25 3:58 UTC (permalink / raw)
To: H.J. Lu; +Cc: Binutils, Nick Clifton, Florian Weimer
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] elf: Set p_align to the common page size if possible
[not found] ` <CAMe9rOrvGXXEJAESEgTbhgQJee__Ak9+OvFwzWa0Nxv+m+OjLg@mail.gmail.com>
@ 2022-01-05 10:17 ` Nick Clifton
0 siblings, 0 replies; 19+ messages in thread
From: Nick Clifton @ 2022-01-05 10:17 UTC (permalink / raw)
To: H.J. Lu; +Cc: Binutils
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
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-01-05 10:17 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-15 16:32 [PATCH] elf: Set p_align to the common page size if possible H.J. Lu
2021-12-16 8:52 ` Fangrui Song
2021-12-16 14:33 ` H.J. Lu
2021-12-20 16:36 ` Florian Weimer
2021-12-20 18:39 ` H.J. Lu
2021-12-20 18:51 ` Florian Weimer
2021-12-20 18:59 ` H.J. Lu
2021-12-20 19:25 ` Florian Weimer
2021-12-20 19:31 ` H.J. Lu
2021-12-20 19:51 ` Florian Weimer
2021-12-20 15:50 ` H.J. Lu
2021-12-22 8:57 ` Alan Modra
2021-12-22 14:38 ` [PATCH v2] elf: Set p_align to the minimum " H.J. Lu
2021-12-25 1:03 ` Alan Modra
2021-12-25 2:50 ` H.J. Lu
2021-12-25 3:04 ` Alan Modra
2021-12-25 3:08 ` H.J. Lu
2021-12-25 3:58 ` Alan Modra
[not found] ` <f64b9fb3-7d2d-7ff3-a8f6-795292af6744@redhat.com>
[not found] ` <CAMe9rOrvGXXEJAESEgTbhgQJee__Ak9+OvFwzWa0Nxv+m+OjLg@mail.gmail.com>
2022-01-05 10:17 ` [PATCH] elf: Set p_align to the common " Nick Clifton
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).