diff mbox series

[RFC,03/23] Allow glibc to be compiled without EXEC_PAGESIZE

Message ID 20240103171502.1358371-4-bugaevc@gmail.com
State New
Headers show
Series aarch64-gnu port | expand

Commit Message

Sergey Bugaev Jan. 3, 2024, 5:14 p.m. UTC
We would like to avoid statically defining any specific page size on
aarch64-gnu, and instead make sure that everything uses the dynamic
page size, available via vm_page_size and GLRO(dl_pagesize).

There are currently a few places in glibc that require EXEC_PAGESIZE
to be defined. Per Roland's suggestion [0], drop the static
GLRO(dl_pagesize) initializers (for now, only if EXEC_PAGESIZE is not
defined), and don't require EXEC_PAGESIZE definition for libio to
enable mmap usage.

[0]: https://mail.gnu.org/archive/html/bug-hurd/2011-10/msg00035.html

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---

I opted to play it safe and only disable the static GLRO(dl_pagesize)
initializers when EXEC_PAGESIZE is not defined (i.e. on aarch64-gnu).
I can change this to be unconditional if that's considered OK.

 elf/dl-support.c | 6 +++++-
 elf/rtld.c       | 2 ++
 libio/libioP.h   | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

Comments

Florian Weimer March 11, 2024, 4:13 p.m. UTC | #1
* Sergey Bugaev:

> diff --git a/elf/dl-support.c b/elf/dl-support.c
> index 2f502c8b..6375dc95 100644
> --- a/elf/dl-support.c
> +++ b/elf/dl-support.c
> @@ -135,7 +135,11 @@ void *_dl_random;
>  #include <dl-procruntime.c>
>  #include <dl-procinfo.c>
>  
> -size_t _dl_pagesize = EXEC_PAGESIZE;
> +size_t _dl_pagesize
> +#ifdef EXEC_PAGESIZE
> +  = EXEC_PAGESIZE
> +#endif
> +;

I think the intent here is to initialize _dl_pagesize with a
conservative default, to avoid initialization ordering issues.
EXEC_PAGESIZE is supposed to be largest supported page size.

Thanks,
Florian
Florian Weimer March 25, 2024, 11:58 a.m. UTC | #2
* Florian Weimer:

> * Sergey Bugaev:
>
>> diff --git a/elf/dl-support.c b/elf/dl-support.c
>> index 2f502c8b..6375dc95 100644
>> --- a/elf/dl-support.c
>> +++ b/elf/dl-support.c
>> @@ -135,7 +135,11 @@ void *_dl_random;
>>  #include <dl-procruntime.c>
>>  #include <dl-procinfo.c>
>>  
>> -size_t _dl_pagesize = EXEC_PAGESIZE;
>> +size_t _dl_pagesize
>> +#ifdef EXEC_PAGESIZE
>> +  = EXEC_PAGESIZE
>> +#endif
>> +;
>
> I think the intent here is to initialize _dl_pagesize with a
> conservative default, to avoid initialization ordering issues.
> EXEC_PAGESIZE is supposed to be largest supported page size.

This was committed without addressing the comment above.

Thanks,
Florian
Sergey Bugaev March 25, 2024, 12:24 p.m. UTC | #3
Hello,

On Mon, Mar 25, 2024 at 2:58 PM Florian Weimer <fweimer@redhat.com> wrote:
> > I think the intent here is to initialize _dl_pagesize with a
> > conservative default, to avoid initialization ordering issues.
> > EXEC_PAGESIZE is supposed to be largest supported page size.
>
> This was committed without addressing the comment above.

yes, I also didn't expect this to get pushed until we come to an agreement here.

On topic: I understand that this must have been done this way because
of potential initialization order issues (and the mail I linked also
makes this guess). The question is, is that (working around
initialization order issues) actually required, or is that a leftover
from something that's no longer relevant (or perhaps never was)?
Things seem to still work with this patch on aarch64-gnu for me, both
in SHARED and !SHARED, though I obviously didn't test every potential
codepath.

I was hoping that some broader testing, such as running the testsuite
on CI on existing ports, could answer that question comprehensively --
though now I realize that since this patch leaves the initialization
as-is when EXEC_PAGESIZE is defined (i.e. everywhere but on
aarch64-gnu), CI wouldn't catch any issues that removing it would
cause.

We could define EXEC_PAGESIZE to some conservative value on
aarch64-gnu too, if it turns out that this little workaround is really
required. But it seems cleaner to make sure we don't need to, as
Roland's email suggests, and introducing a new port that doesn't have
a fixed page size (and doesn't come with an EXEC_PAGESIZE value
already defined in kernel headers) seems to be a good opportunity to
do that. That's my reasoning here.

Sergey
Florian Weimer April 10, 2024, 11:57 a.m. UTC | #4
* Sergey Bugaev:

> We could define EXEC_PAGESIZE to some conservative value on
> aarch64-gnu too, if it turns out that this little workaround is really
> required. But it seems cleaner to make sure we don't need to, as
> Roland's email suggests, and introducing a new port that doesn't have
> a fixed page size (and doesn't come with an EXEC_PAGESIZE value
> already defined in kernel headers) seems to be a good opportunity to
> do that. That's my reasoning here.

But the ELF image must be laid out with certain expectations regarding
the maximum support page size.  Otherwise, something (kernel or dynamic
linker) needs to perform copies or upgrade conflicting permissions
within one page to a superset of all permissions.  I don't think we have
code for that today, and we wouldn't necessarily want to implement that,
I think.

Thanks,
Florian
Sergey Bugaev April 15, 2024, 12:53 p.m. UTC | #5
Hello,

On Wed, Apr 10, 2024 at 2:57 PM Florian Weimer <fweimer@redhat.com> wrote:
> * Sergey Bugaev:
>
> > We could define EXEC_PAGESIZE to some conservative value on
> > aarch64-gnu too, if it turns out that this little workaround is really
> > required. But it seems cleaner to make sure we don't need to, as
> > Roland's email suggests, and introducing a new port that doesn't have
> > a fixed page size (and doesn't come with an EXEC_PAGESIZE value
> > already defined in kernel headers) seems to be a good opportunity to
> > do that. That's my reasoning here.
>
> But the ELF image must be laid out with certain expectations regarding
> the maximum support page size.  Otherwise, something (kernel or dynamic
> linker) needs to perform copies or upgrade conflicting permissions
> within one page to a superset of all permissions.  I don't think we have
> code for that today, and we wouldn't necessarily want to implement that,
> I think.

Certainly -- and I wouldn't expect the kernel or the dynamic linker to
do anything about it other than reporting an error.

I think what you're saying is you consider EXEC_PAGESIZE to indeed
describe/define this required alignment of ELF segments (as the name
suggests). If I adopt that view, then yes, having EXEC_PAGESIZE makes
sense, and it makes some sense to use it as a conservative page size
value to use while the real value is not yet available (assuming there
is a real need for that).

The way I've been viewing it -- based on the fact that neither Linux's
nor glibc's (dynamic) nor BFD's nor LLVM's (static) linkers use it for
that purpose -- is that it's just some PAGE_SIZE-like definition
that's unrelated to binary loading (despite its name -- perhaps it has
been historically related to segment alignment in some old versions of
Linux?) that has been co-opted by glibc for pre-initializing
dl_pagesize, for dubious reasons. It also seems to be a Linux- (and
x86 Hurd) specific thing; I cannot find it in the BSDs.

Which one is it?

Sergey
Samuel Thibault April 22, 2024, 9:01 p.m. UTC | #6
Hello,

Sergey Bugaev, le lun. 25 mars 2024 15:24:14 +0300, a ecrit:
> On Mon, Mar 25, 2024 at 2:58 PM Florian Weimer <fweimer@redhat.com> wrote:
> > > I think the intent here is to initialize _dl_pagesize with a
> > > conservative default, to avoid initialization ordering issues.
> > > EXEC_PAGESIZE is supposed to be largest supported page size.
> >
> > This was committed without addressing the comment above.
> 
> yes, I also didn't expect this to get pushed until we come to an agreement here.

Sorry for this, I have reverted the change for now.

Samuel
diff mbox series

Patch

diff --git a/elf/dl-support.c b/elf/dl-support.c
index 2f502c8b..6375dc95 100644
--- a/elf/dl-support.c
+++ b/elf/dl-support.c
@@ -135,7 +135,11 @@  void *_dl_random;
 #include <dl-procruntime.c>
 #include <dl-procinfo.c>
 
-size_t _dl_pagesize = EXEC_PAGESIZE;
+size_t _dl_pagesize
+#ifdef EXEC_PAGESIZE
+  = EXEC_PAGESIZE
+#endif
+;
 
 size_t _dl_minsigstacksize = CONSTANT_MINSIGSTKSZ;
 
diff --git a/elf/rtld.c b/elf/rtld.c
index 4f494b79..98b042c0 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -358,7 +358,9 @@  struct rtld_global_ro _rtld_global_ro attribute_relro =
     ._dl_debug_fd = STDERR_FILENO,
     ._dl_lazy = 1,
     ._dl_fpu_control = _FPU_DEFAULT,
+#ifdef EXEC_PAGESIZE
     ._dl_pagesize = EXEC_PAGESIZE,
+#endif
     ._dl_inhibit_cache = 0,
     ._dl_profile_output = "/var/tmp",
 
diff --git a/libio/libioP.h b/libio/libioP.h
index 1af287b1..1a7f547e 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -852,7 +852,7 @@  extern off64_t _IO_seekpos_unlocked (FILE *, off64_t, int)
 #  define MAP_ANONYMOUS MAP_ANON
 # endif
 
-# if !defined(MAP_ANONYMOUS) || !defined(EXEC_PAGESIZE)
+# if !defined(MAP_ANONYMOUS)
 #  undef _G_HAVE_MMAP
 #  define _G_HAVE_MMAP 0
 # endif