* [PATCH] Initialise the machine specific part of rtld bootstrap map
@ 2016-10-26 19:54 Matthew Fortune
2016-10-26 20:12 ` Joseph Myers
0 siblings, 1 reply; 7+ messages in thread
From: Matthew Fortune @ 2016-10-26 19:54 UTC (permalink / raw)
To: libc-alpha
Cc: Sandra Loosemore (sandra@codesourcery.com),
Joseph Myers (joseph@codesourcery.com),
Petar Jovanovic
Hi,
This is a long delayed follow up to a patch by Sandra:
https://sourceware.org/ml/libc-alpha/2015-03/msg00704.html
I've reimplemented it to be MIPS specific and lightweight to just
initialise the necessary fields. Tested using mips64el-linux-gnu (n64)
with some manual verification to make sure I saw the loader failures if I
initialised the fpabi to an illegal value instead of zero.
MIPS requires two fields in the machine specific part of the link map to
be zero initialised. This is natually achieved except when the map is
allocated on the stack. The only map allocated on the stack is the
bootstrap map which is often the first use of the stack space following
kernel allocation and is therefore zero. However, if rtld is invoked
such that the stack has already been used then there may be non-zero
data and ABI checks which use the affected fields will spuriously fail.
* elf/rtld.c (_dl_start) [ifndef DONT_USE_BOOTSTRAP_MAP]: Call
ELF_MACHINE_INIT_MAP.
* sysdeps/mips/dl-machine.h (ELF_MACHINE_INIT_MAP): Define macro.
Thanks,
Matthew
---
elf/rtld.c | 3 +++
sysdeps/mips/dl-machine.h | 7 +++++++
2 files changed, 10 insertions(+)
diff --git a/elf/rtld.c b/elf/rtld.c
index 647661c..31539a4 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -364,6 +364,9 @@ _dl_start (void *arg)
do not have to use the temporary bootstrap_map. Global variables
are initialized to zero by default. */
#ifndef DONT_USE_BOOTSTRAP_MAP
+# ifdef ELF_MACHINE_INIT_MAP
+ ELF_MACHINE_INIT_MAP (bootstrap_map);
+# endif
# ifdef HAVE_BUILTIN_MEMSET
__builtin_memset (bootstrap_map.l_info, '\0', sizeof (bootstrap_map.l_info));
# else
diff --git a/sysdeps/mips/dl-machine.h b/sysdeps/mips/dl-machine.h
index 8c0b40e..d929477 100644
--- a/sysdeps/mips/dl-machine.h
+++ b/sysdeps/mips/dl-machine.h
@@ -93,6 +93,13 @@ do { if ((l)->l_info[DT_MIPS (RLD_MAP_REL)]) \
# define ELF_MACHINE_NAN2008 0
#endif
+/* Initialise the machine dependent parts of a map. This is not normally
+ required unless the map is allocated on the stack. */
+#define ELF_MACHINE_INIT_MAP(MAP) \
+do { (MAP)->l_mach.fpabi = 0; \
+ (MAP)->l_mach.odd_spreg = 0; \
+ } while (0)
+
/* Return nonzero iff ELF header is compatible with the running host. */
static inline int __attribute_used__
elf_machine_matches_host (const ElfW(Ehdr) *ehdr)
--
2.2.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Initialise the machine specific part of rtld bootstrap map
2016-10-26 19:54 [PATCH] Initialise the machine specific part of rtld bootstrap map Matthew Fortune
@ 2016-10-26 20:12 ` Joseph Myers
2016-10-26 21:01 ` Matthew Fortune
0 siblings, 1 reply; 7+ messages in thread
From: Joseph Myers @ 2016-10-26 20:12 UTC (permalink / raw)
To: Matthew Fortune
Cc: libc-alpha, Sandra Loosemore (sandra@codesourcery.com), Petar Jovanovic
On Wed, 26 Oct 2016, Matthew Fortune wrote:
> +# ifdef ELF_MACHINE_INIT_MAP
> + ELF_MACHINE_INIT_MAP (bootstrap_map);
> +# endif
We don't encourage use of #ifdef like that. It's better to have an inline
function defined everywhere and used unconditionally, for which most
systems have a dummy definition (see dl-machine-reject-phdr.h and
elf_machine_reject_phdr_p for an example - if you have a header for a
single function, you don't need to update lots of dl-machine.h headers,
just add a generic version - which has the comments detailing the
semantics of the function and when it's needed - and a MIPS version).
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] Initialise the machine specific part of rtld bootstrap map
2016-10-26 20:12 ` Joseph Myers
@ 2016-10-26 21:01 ` Matthew Fortune
2016-10-26 21:07 ` Sandra Loosemore
2016-10-26 21:37 ` Joseph Myers
0 siblings, 2 replies; 7+ messages in thread
From: Matthew Fortune @ 2016-10-26 21:01 UTC (permalink / raw)
To: Joseph Myers
Cc: libc-alpha, Sandra Loosemore (sandra@codesourcery.com), Petar Jovanovic
Joseph Myers <joseph@codesourcery.com> writes:
> On Wed, 26 Oct 2016, Matthew Fortune wrote:
>
> > +# ifdef ELF_MACHINE_INIT_MAP
> > + ELF_MACHINE_INIT_MAP (bootstrap_map); # endif
>
> We don't encourage use of #ifdef like that. It's better to have an
> inline function defined everywhere and used unconditionally, for which
> most systems have a dummy definition (see dl-machine-reject-phdr.h and
> elf_machine_reject_phdr_p for an example - if you have a header for a
> single function, you don't need to update lots of dl-machine.h headers,
> just add a generic version - which has the comments detailing the
> semantics of the function and when it's needed - and a MIPS version).
Thanks Joseph. It's been a while since I did a glibc patch and couldn't
remember the recommended approach.
Do you think I should add a whole new header for this? Or, since this
is directly related to the reject_phdr feature for MIPS and only MIPS
is affected then I could just add it to dl-machine-reject-phdr.h?
Matthew
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Initialise the machine specific part of rtld bootstrap map
2016-10-26 21:01 ` Matthew Fortune
@ 2016-10-26 21:07 ` Sandra Loosemore
2016-10-26 21:22 ` Matthew Fortune
2016-10-28 12:46 ` Matthew Fortune
2016-10-26 21:37 ` Joseph Myers
1 sibling, 2 replies; 7+ messages in thread
From: Sandra Loosemore @ 2016-10-26 21:07 UTC (permalink / raw)
To: Matthew Fortune, Joseph Myers; +Cc: libc-alpha, Petar Jovanovic
On 10/26/2016 03:01 PM, Matthew Fortune wrote:
> Joseph Myers <joseph@codesourcery.com> writes:
>> On Wed, 26 Oct 2016, Matthew Fortune wrote:
>>
>>> +# ifdef ELF_MACHINE_INIT_MAP
>>> + ELF_MACHINE_INIT_MAP (bootstrap_map); # endif
>>
>> We don't encourage use of #ifdef like that. It's better to have an
>> inline function defined everywhere and used unconditionally, for which
>> most systems have a dummy definition (see dl-machine-reject-phdr.h and
>> elf_machine_reject_phdr_p for an example - if you have a header for a
>> single function, you don't need to update lots of dl-machine.h headers,
>> just add a generic version - which has the comments detailing the
>> semantics of the function and when it's needed - and a MIPS version).
>
> Thanks Joseph. It's been a while since I did a glibc patch and couldn't
> remember the recommended approach.
>
> Do you think I should add a whole new header for this? Or, since this
> is directly related to the reject_phdr feature for MIPS and only MIPS
> is affected then I could just add it to dl-machine-reject-phdr.h?
Wouldn't it be easier and more maintainable just to unconditionally
zero-initialize the structure, as I did in the original patch?
-Sandra
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] Initialise the machine specific part of rtld bootstrap map
2016-10-26 21:07 ` Sandra Loosemore
@ 2016-10-26 21:22 ` Matthew Fortune
2016-10-28 12:46 ` Matthew Fortune
1 sibling, 0 replies; 7+ messages in thread
From: Matthew Fortune @ 2016-10-26 21:22 UTC (permalink / raw)
To: Sandra Loosemore, Joseph Myers; +Cc: libc-alpha, Petar Jovanovic
Sandra Loosemore <sandra@codesourcery.com> writes:
> On 10/26/2016 03:01 PM, Matthew Fortune wrote:
> > Joseph Myers <joseph@codesourcery.com> writes:
> >> On Wed, 26 Oct 2016, Matthew Fortune wrote:
> >>
> >>> +# ifdef ELF_MACHINE_INIT_MAP
> >>> + ELF_MACHINE_INIT_MAP (bootstrap_map); # endif
> >>
> >> We don't encourage use of #ifdef like that. It's better to have an
> >> inline function defined everywhere and used unconditionally, for
> >> which most systems have a dummy definition (see
> >> dl-machine-reject-phdr.h and elf_machine_reject_phdr_p for an example
> >> - if you have a header for a single function, you don't need to
> >> update lots of dl-machine.h headers, just add a generic version -
> >> which has the comments detailing the semantics of the function and
> when it's needed - and a MIPS version).
> >
> > Thanks Joseph. It's been a while since I did a glibc patch and
> > couldn't remember the recommended approach.
> >
> > Do you think I should add a whole new header for this? Or, since this
> > is directly related to the reject_phdr feature for MIPS and only MIPS
> > is affected then I could just add it to dl-machine-reject-phdr.h?
>
> Wouldn't it be easier and more maintainable just to unconditionally
> zero-initialize the structure, as I did in the original patch?
I was just trying to factor in the comments from Roland on the original
patch. I don't know if the initialisation cost outweighs the drawback
of not simply initialising the whole structure. I'm happy to be led
by other opinions though.
Matthew
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] Initialise the machine specific part of rtld bootstrap map
2016-10-26 21:07 ` Sandra Loosemore
2016-10-26 21:22 ` Matthew Fortune
@ 2016-10-28 12:46 ` Matthew Fortune
1 sibling, 0 replies; 7+ messages in thread
From: Matthew Fortune @ 2016-10-28 12:46 UTC (permalink / raw)
To: Sandra Loosemore, Joseph Myers, roland; +Cc: libc-alpha, Petar Jovanovic
Sandra Loosemore <sandra@codesourcery.com> writes:
> On 10/26/2016 03:01 PM, Matthew Fortune wrote:
> > Joseph Myers <joseph@codesourcery.com> writes:
> >> On Wed, 26 Oct 2016, Matthew Fortune wrote:
> >>
> >>> +# ifdef ELF_MACHINE_INIT_MAP
> >>> + ELF_MACHINE_INIT_MAP (bootstrap_map); # endif
> >>
> >> We don't encourage use of #ifdef like that. It's better to have an
> >> inline function defined everywhere and used unconditionally, for
> >> which most systems have a dummy definition (see
> >> dl-machine-reject-phdr.h and elf_machine_reject_phdr_p for an example
> >> - if you have a header for a single function, you don't need to
> >> update lots of dl-machine.h headers, just add a generic version -
> >> which has the comments detailing the semantics of the function and
> when it's needed - and a MIPS version).
> >
> > Thanks Joseph. It's been a while since I did a glibc patch and
> > couldn't remember the recommended approach.
> >
> > Do you think I should add a whole new header for this? Or, since this
> > is directly related to the reject_phdr feature for MIPS and only MIPS
> > is affected then I could just add it to dl-machine-reject-phdr.h?
>
> Wouldn't it be easier and more maintainable just to unconditionally
> zero-initialize the structure, as I did in the original patch?
I'd like to get some consensus on the best solution here before I do
another implementation.
We either go for:
1) The bare minimum to initialise just the fields that must be zero for
successful execution on a per architecture basis (and live with the
associated risk of missing some)
or
2) Unconditionally zero the whole l_mach link_map_machine structure
Or
3) Unconditionally zero the entire link_map
Given these I'd actually go for (2) as a good balance.
Thanks,
Matthew
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] Initialise the machine specific part of rtld bootstrap map
2016-10-26 21:01 ` Matthew Fortune
2016-10-26 21:07 ` Sandra Loosemore
@ 2016-10-26 21:37 ` Joseph Myers
1 sibling, 0 replies; 7+ messages in thread
From: Joseph Myers @ 2016-10-26 21:37 UTC (permalink / raw)
To: Matthew Fortune
Cc: libc-alpha, Sandra Loosemore (sandra@codesourcery.com), Petar Jovanovic
On Wed, 26 Oct 2016, Matthew Fortune wrote:
> Do you think I should add a whole new header for this? Or, since this
> is directly related to the reject_phdr feature for MIPS and only MIPS
> is affected then I could just add it to dl-machine-reject-phdr.h?
If you can argue that there is no possible need for this new function
other than with the reject_phdr feature and include that explanation in
the comments in the header, sharing the header seems reasonable.
Otherwise it should be a separate header.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-10-28 12:46 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-26 19:54 [PATCH] Initialise the machine specific part of rtld bootstrap map Matthew Fortune
2016-10-26 20:12 ` Joseph Myers
2016-10-26 21:01 ` Matthew Fortune
2016-10-26 21:07 ` Sandra Loosemore
2016-10-26 21:22 ` Matthew Fortune
2016-10-28 12:46 ` Matthew Fortune
2016-10-26 21:37 ` Joseph Myers
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).