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

["H.J. Lu" <hjl.tools@gmail.com>]

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



-- 
H.J.