[v6,0/2] fix p_align on PT_LOAD segment in DSO isn't honored

  Hi

This patch mainly to fix a reported bug:

"p_align on PT_LOAD segment in DSO isn't honored"
https://sourceware.org/bugzilla/show_bug.cgi?id=28676

Patch 1/1 is a simple testcase which modified from H.J.Lu.

Thanks.

Changelog:
v5 -> v6
- Patch "Add a testcase to check alignment of PT_LOAD segment"
add some comments
- Patch "elf: Properly align PT_LOAD segments"
update copyright

v4 -> v5
- Patch "Add a testcase to check alignment of PT_LOAD segment"
add new testcase for PT_LOAD segment
- Patch "elf: Properly align PT_LOAD segments"
fix map_start to use map_start_aligned when second mmap failed

v3 -> v4
- Patch "elf: Properly align PT_LOAD segments"
Call unmap when the second mmap fails.

v2 -> v3
- Patch "elf: Properly align PT_LOAD segments"
move mapalign into 'struct loadcmd'
fix some coding style

RFC/v1 -> v2

- Patch "elf: align the mapping address of LOAD segments with p_align"
fix coding format and add testcase in commit.

RFC link:
https://patchwork.sourceware.org/project/glibc/patch/20211204045848.71105-2-rongwei.wang@linux.alibaba.com/

H.J. Lu (1):
  Add a testcase to check alignment of PT_LOAD segment

Rongwei Wang (1):
  elf: Properly align PT_LOAD segments

 elf/Makefile          | 14 +++++++++++--
 elf/dl-load.c         |  1 +
 elf/dl-load.h         |  2 +-
 elf/dl-map-segments.h | 49 +++++++++++++++++++++++++++++++++++++++----
 elf/tst-align3.c      | 37 ++++++++++++++++++++++++++++++++
 elf/tst-alignmod3.c   | 31 +++++++++++++++++++++++++++
 6 files changed, 127 insertions(+), 7 deletions(-)
 create mode 100644 elf/tst-align3.c
 create mode 100644 elf/tst-alignmod3.c
  

On 2021-12-13, Rongwei Wang via Libc-alpha wrote:
>Hi
>
>This patch mainly to fix a reported bug:
>
>"p_align on PT_LOAD segment in DSO isn't honored"
>https://sourceware.org/bugzilla/show_bug.cgi?id=28676

(From linekr perspective) I am unsure this is a bug.

The generic-abi just says:

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

The requirement is p_offset = p_vaddr (mod p_align).
It does not necessarily imply that the system has to make p_vaddr =
real_vaddr (mod p_align).

Linkers (GNU ld, gold, ld.lld) set p_align(PT_LOAD) to the
CONSTANT(MAXPAGESIZE) (set by -z max-page-size=) value. This is just
the largest page size the linked object supports.
(The current behavior (including many many ld.so implementations) is `p_vaddr = real_vaddr (mod page_size)`).

I guess this reasoning may be related to why the linker option is called
max-page-size, not just page-size.
My linker oriented stance may be strengthened by the existence of
CONSTANT(COMMONPAGESIZE), which is used by PT_GNU_RELRO and is allowed
to be smaller than max-page-size: if ld.so always overaligns to p_align,
there would be no need to have COMMONPAGESIZE/MAXPAGESIZE distinction.

---

I understand that letting ld.so use a large p_align value may make
transparent hugepage easy, and may have performance boost for some large
executables by some corporate users, but have you considered the
downside of always using p_align? How can an user opt out the changed
behavior?  I think there are many tunable knobs and userspace remapping
the pages may have some benefits over ld.so doing it automatically.

* At the very least, I can think that people may want to treat RX and RW
   memory mappings differently, or call mlock() in some circumstances.
* If I set max-page-size to 1GB, am I disallowed to use 2M hugepagesize?
* Can a user express intention like mlock?
* What if a user doesn't want to place some cold code in hugepages?

OK, I don't know hugepages well.  CC Chris Kennelly as an expert in this
area.

>Patch 1/1 is a simple testcase which modified from H.J.Lu.
>
>Thanks.
>
>Changelog:
>v5 -> v6
>- Patch "Add a testcase to check alignment of PT_LOAD segment"
>add some comments
>- Patch "elf: Properly align PT_LOAD segments"
>update copyright
>
>v4 -> v5
>- Patch "Add a testcase to check alignment of PT_LOAD segment"
>add new testcase for PT_LOAD segment
>- Patch "elf: Properly align PT_LOAD segments"
>fix map_start to use map_start_aligned when second mmap failed
>
>v3 -> v4
>- Patch "elf: Properly align PT_LOAD segments"
>Call unmap when the second mmap fails.
>
>v2 -> v3
>- Patch "elf: Properly align PT_LOAD segments"
>move mapalign into 'struct loadcmd'
>fix some coding style
>
>RFC/v1 -> v2
>
>- Patch "elf: align the mapping address of LOAD segments with p_align"
>fix coding format and add testcase in commit.
>
>RFC link:
>https://patchwork.sourceware.org/project/glibc/patch/20211204045848.71105-2-rongwei.wang@linux.alibaba.com/
>
>H.J. Lu (1):
>  Add a testcase to check alignment of PT_LOAD segment
>
>Rongwei Wang (1):
>  elf: Properly align PT_LOAD segments
>
> elf/Makefile          | 14 +++++++++++--
> elf/dl-load.c         |  1 +
> elf/dl-load.h         |  2 +-
> elf/dl-map-segments.h | 49 +++++++++++++++++++++++++++++++++++++++----
> elf/tst-align3.c      | 37 ++++++++++++++++++++++++++++++++
> elf/tst-alignmod3.c   | 31 +++++++++++++++++++++++++++
> 6 files changed, 127 insertions(+), 7 deletions(-)
> create mode 100644 elf/tst-align3.c
> create mode 100644 elf/tst-alignmod3.c
>
>-- 
>2.27.0
>
  

On Mon, Dec 13, 2021 at 6:03 PM Fangrui Song <maskray@google.com> wrote:
>
> On 2021-12-13, Rongwei Wang via Libc-alpha wrote:
> >Hi
> >
> >This patch mainly to fix a reported bug:
> >
> >"p_align on PT_LOAD segment in DSO isn't honored"
> >https://sourceware.org/bugzilla/show_bug.cgi?id=28676
>
> (From linekr perspective) I am unsure this is a bug.
>
> The generic-abi just says:
>
> > 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.
>
> The requirement is p_offset = p_vaddr (mod p_align).
> It does not necessarily imply that the system has to make p_vaddr =
> real_vaddr (mod p_align).
>
> Linkers (GNU ld, gold, ld.lld) set p_align(PT_LOAD) to the
> CONSTANT(MAXPAGESIZE) (set by -z max-page-size=) value. This is just
> the largest page size the linked object supports.
> (The current behavior (including many many ld.so implementations) is `p_vaddr = real_vaddr (mod page_size)`).
>
> I guess this reasoning may be related to why the linker option is called
> max-page-size, not just page-size.
> My linker oriented stance may be strengthened by the existence of
> CONSTANT(COMMONPAGESIZE), which is used by PT_GNU_RELRO and is allowed
> to be smaller than max-page-size: if ld.so always overaligns to p_align,
> there would be no need to have COMMONPAGESIZE/MAXPAGESIZE distinction.
>
> ---
>
> I understand that letting ld.so use a large p_align value may make
> transparent hugepage easy, and may have performance boost for some large
> executables by some corporate users, but have you considered the
> downside of always using p_align? How can an user opt out the changed
> behavior?  I think there are many tunable knobs and userspace remapping
> the pages may have some benefits over ld.so doing it automatically.

Kernel has been doing this since:

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

Here is the linker proposal how to opt it out:

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

by setting p_align to common page size by default.

> * At the very least, I can think that people may want to treat RX and RW
>    memory mappings differently, or call mlock() in some circumstances.
> * If I set max-page-size to 1GB, am I disallowed to use 2M hugepagesize?
> * Can a user express intention like mlock?
> * What if a user doesn't want to place some cold code in hugepages?
>
> OK, I don't know hugepages well.  CC Chris Kennelly as an expert in this
> area.
>
> >Patch 1/1 is a simple testcase which modified from H.J.Lu.
> >
> >Thanks.
> >
> >Changelog:
> >v5 -> v6
> >- Patch "Add a testcase to check alignment of PT_LOAD segment"
> >add some comments
> >- Patch "elf: Properly align PT_LOAD segments"
> >update copyright
> >
> >v4 -> v5
> >- Patch "Add a testcase to check alignment of PT_LOAD segment"
> >add new testcase for PT_LOAD segment
> >- Patch "elf: Properly align PT_LOAD segments"
> >fix map_start to use map_start_aligned when second mmap failed
> >
> >v3 -> v4
> >- Patch "elf: Properly align PT_LOAD segments"
> >Call unmap when the second mmap fails.
> >
> >v2 -> v3
> >- Patch "elf: Properly align PT_LOAD segments"
> >move mapalign into 'struct loadcmd'
> >fix some coding style
> >
> >RFC/v1 -> v2
> >
> >- Patch "elf: align the mapping address of LOAD segments with p_align"
> >fix coding format and add testcase in commit.
> >
> >RFC link:
> >https://patchwork.sourceware.org/project/glibc/patch/20211204045848.71105-2-rongwei.wang@linux.alibaba.com/
> >
> >H.J. Lu (1):
> >  Add a testcase to check alignment of PT_LOAD segment
> >
> >Rongwei Wang (1):
> >  elf: Properly align PT_LOAD segments
> >
> > elf/Makefile          | 14 +++++++++++--
> > elf/dl-load.c         |  1 +
> > elf/dl-load.h         |  2 +-
> > elf/dl-map-segments.h | 49 +++++++++++++++++++++++++++++++++++++++----
> > elf/tst-align3.c      | 37 ++++++++++++++++++++++++++++++++
> > elf/tst-alignmod3.c   | 31 +++++++++++++++++++++++++++
> > 6 files changed, 127 insertions(+), 7 deletions(-)
> > create mode 100644 elf/tst-align3.c
> > create mode 100644 elf/tst-alignmod3.c
> >
> >--
> >2.27.0
> >