[03/13] elf: dont pass fd to _dl_process_pt_xx
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
It is not used in these functions.
rtld.c:rtld_setup_main_map() does the same.
The test-suite was run on x86_64/64 and showed no regressions.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
---
elf/dl-load.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Comments
On 18/03/23 13:51, Stas Sergeev via Libc-alpha wrote:
> It is not used in these functions.
> rtld.c:rtld_setup_main_map() does the same.
>
> The test-suite was run on x86_64/64 and showed no regressions.
>
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> ---
> elf/dl-load.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index fcb39a78d4..ab8b648687 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1379,10 +1379,10 @@ cannot enable executable stack as shared object requires");
> switch (ph[-1].p_type)
> {
> case PT_NOTE:
> - _dl_process_pt_note (l, fd, &ph[-1]);
> + _dl_process_pt_note (l, -1, &ph[-1]);
> break;
> case PT_GNU_PROPERTY:
> - _dl_process_pt_gnu_property (l, fd, &ph[-1]);
> + _dl_process_pt_gnu_property (l, -1, &ph[-1]);
> break;
> }
>
It allows both _dl_process_pt_note and _dl_process_pt_gnu_property to know
if the called where rtld code during statup code or dlopen. But you are
right that it is not used.
However this does not accomplish anything, a better refactor would to just
remove the argument altogether. It at least would simplify the interface
and allow slight better code generation.
29.03.2023 22:10, Adhemerval Zanella Netto пишет:
>
> On 18/03/23 13:51, Stas Sergeev via Libc-alpha wrote:
>> It is not used in these functions.
>> rtld.c:rtld_setup_main_map() does the same.
>>
>> The test-suite was run on x86_64/64 and showed no regressions.
>>
>> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
>> ---
>> elf/dl-load.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index fcb39a78d4..ab8b648687 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -1379,10 +1379,10 @@ cannot enable executable stack as shared object requires");
>> switch (ph[-1].p_type)
>> {
>> case PT_NOTE:
>> - _dl_process_pt_note (l, fd, &ph[-1]);
>> + _dl_process_pt_note (l, -1, &ph[-1]);
>> break;
>> case PT_GNU_PROPERTY:
>> - _dl_process_pt_gnu_property (l, fd, &ph[-1]);
>> + _dl_process_pt_gnu_property (l, -1, &ph[-1]);
>> break;
>> }
>>
>
> It allows both _dl_process_pt_note and _dl_process_pt_gnu_property to know
> if the called where rtld code during statup code or dlopen. But you are
> right that it is not used.
>
> However this does not accomplish anything, a better refactor would to just
> remove the argument altogether. It at least would simplify the interface
> and allow slight better code generation.
I tried to do that, but there is also that
_dl_process_gnu_property() called from
_dl_process_pt_gnu_property().
For aarch64 it has this:
unsigned int feature_1 = *(unsigned int *) data;
if (feature_1 & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
_dl_bti_protect (l, fd);
What should I do here to remove that fd?
On 30/03/23 13:08, stsp wrote:
>
> 29.03.2023 22:10, Adhemerval Zanella Netto пишет:
>>
>> On 18/03/23 13:51, Stas Sergeev via Libc-alpha wrote:
>>> It is not used in these functions.
>>> rtld.c:rtld_setup_main_map() does the same.
>>>
>>> The test-suite was run on x86_64/64 and showed no regressions.
>>>
>>> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
>>> ---
>>> elf/dl-load.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>>> index fcb39a78d4..ab8b648687 100644
>>> --- a/elf/dl-load.c
>>> +++ b/elf/dl-load.c
>>> @@ -1379,10 +1379,10 @@ cannot enable executable stack as shared object requires");
>>> switch (ph[-1].p_type)
>>> {
>>> case PT_NOTE:
>>> - _dl_process_pt_note (l, fd, &ph[-1]);
>>> + _dl_process_pt_note (l, -1, &ph[-1]);
>>> break;
>>> case PT_GNU_PROPERTY:
>>> - _dl_process_pt_gnu_property (l, fd, &ph[-1]);
>>> + _dl_process_pt_gnu_property (l, -1, &ph[-1]);
>>> break;
>>> }
>>>
>>
>> It allows both _dl_process_pt_note and _dl_process_pt_gnu_property to know
>> if the called where rtld code during statup code or dlopen. But you are
>> right that it is not used.
>>
>> However this does not accomplish anything, a better refactor would to just
>> remove the argument altogether. It at least would simplify the interface
>> and allow slight better code generation.
>
> I tried to do that, but there is also that
> _dl_process_gnu_property() called from
> _dl_process_pt_gnu_property().
> For aarch64 it has this:
> unsigned int feature_1 = *(unsigned int *) data;
> if (feature_1 & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
> _dl_bti_protect (l, fd);
>
> What should I do here to remove that fd?
>
In fact aarch64 _dl_bti_protect requires to know whether the map was done by
the kernel or not.
Szabolcs, shouldn't the code:
1257 for (const ElfW(Phdr) *ph = &phdr[phnum]; ph != phdr; --ph)
1258 switch (ph[-1].p_type)
1259 {
1260 case PT_NOTE:
1261 _dl_process_pt_note (main_map, -1, &ph[-1]);
1262 break;
1263 case PT_GNU_PROPERTY:
1264 _dl_process_pt_gnu_property (main_map, -1, &ph[-1]);
1265 break;
1266 }
Take in consideration whether the main_map was allocated by the kernel or
by loader to pass the correct value on 'fd'?
The 03/30/2023 17:46, Adhemerval Zanella Netto wrote:
> On 30/03/23 13:08, stsp wrote:
> >
> > 29.03.2023 22:10, Adhemerval Zanella Netto пишет:
> >>
> >> On 18/03/23 13:51, Stas Sergeev via Libc-alpha wrote:
> >>> It is not used in these functions.
> >>> rtld.c:rtld_setup_main_map() does the same.
> >>>
> >>> The test-suite was run on x86_64/64 and showed no regressions.
> >>>
> >>> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> >>> ---
> >>> elf/dl-load.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/elf/dl-load.c b/elf/dl-load.c
> >>> index fcb39a78d4..ab8b648687 100644
> >>> --- a/elf/dl-load.c
> >>> +++ b/elf/dl-load.c
> >>> @@ -1379,10 +1379,10 @@ cannot enable executable stack as shared object requires");
> >>> switch (ph[-1].p_type)
> >>> {
> >>> case PT_NOTE:
> >>> - _dl_process_pt_note (l, fd, &ph[-1]);
> >>> + _dl_process_pt_note (l, -1, &ph[-1]);
> >>> break;
> >>> case PT_GNU_PROPERTY:
> >>> - _dl_process_pt_gnu_property (l, fd, &ph[-1]);
> >>> + _dl_process_pt_gnu_property (l, -1, &ph[-1]);
> >>> break;
> >>> }
> >>>
> >>
> >> It allows both _dl_process_pt_note and _dl_process_pt_gnu_property to know
> >> if the called where rtld code during statup code or dlopen. But you are
> >> right that it is not used.
> >>
> >> However this does not accomplish anything, a better refactor would to just
> >> remove the argument altogether. It at least would simplify the interface
> >> and allow slight better code generation.
> >
> > I tried to do that, but there is also that
> > _dl_process_gnu_property() called from
> > _dl_process_pt_gnu_property().
> > For aarch64 it has this:
> > unsigned int feature_1 = *(unsigned int *) data;
> > if (feature_1 & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
> > _dl_bti_protect (l, fd);
> >
> > What should I do here to remove that fd?
> >
>
> In fact aarch64 _dl_bti_protect requires to know whether the map was done by
> the kernel or not.
>
> Szabolcs, shouldn't the code:
>
> 1257 for (const ElfW(Phdr) *ph = &phdr[phnum]; ph != phdr; --ph)
> 1258 switch (ph[-1].p_type)
> 1259 {
> 1260 case PT_NOTE:
> 1261 _dl_process_pt_note (main_map, -1, &ph[-1]);
> 1262 break;
> 1263 case PT_GNU_PROPERTY:
> 1264 _dl_process_pt_gnu_property (main_map, -1, &ph[-1]);
> 1265 break;
> 1266 }
>
> Take in consideration whether the main_map was allocated by the kernel or
> by loader to pass the correct value on 'fd'?
if the exe is loaded by ld.so then we already took care of bti
(and other gnu properties) at load time.
here we could skip processing properties again in that case but
it does not hurt on aarch64 (it will try to mprotect with
PROT_BTI again and ignore any failures).
however the fd must be passed down in elf/dl-load.c otherwise
bti protection is silently dropped (when systemd filters out
mprotect with PROT_EXEC, this is being fixed, but there are old
systems configured like that).
On 31/03/23 09:02, Szabolcs Nagy wrote:
> The 03/30/2023 17:46, Adhemerval Zanella Netto wrote:
>> On 30/03/23 13:08, stsp wrote:
>>>
>>> 29.03.2023 22:10, Adhemerval Zanella Netto пишет:
>>>>
>>>> On 18/03/23 13:51, Stas Sergeev via Libc-alpha wrote:
>>>>> It is not used in these functions.
>>>>> rtld.c:rtld_setup_main_map() does the same.
>>>>>
>>>>> The test-suite was run on x86_64/64 and showed no regressions.
>>>>>
>>>>> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
>>>>> ---
>>>>> elf/dl-load.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>>>>> index fcb39a78d4..ab8b648687 100644
>>>>> --- a/elf/dl-load.c
>>>>> +++ b/elf/dl-load.c
>>>>> @@ -1379,10 +1379,10 @@ cannot enable executable stack as shared object requires");
>>>>> switch (ph[-1].p_type)
>>>>> {
>>>>> case PT_NOTE:
>>>>> - _dl_process_pt_note (l, fd, &ph[-1]);
>>>>> + _dl_process_pt_note (l, -1, &ph[-1]);
>>>>> break;
>>>>> case PT_GNU_PROPERTY:
>>>>> - _dl_process_pt_gnu_property (l, fd, &ph[-1]);
>>>>> + _dl_process_pt_gnu_property (l, -1, &ph[-1]);
>>>>> break;
>>>>> }
>>>>>
>>>>
>>>> It allows both _dl_process_pt_note and _dl_process_pt_gnu_property to know
>>>> if the called where rtld code during statup code or dlopen. But you are
>>>> right that it is not used.
>>>>
>>>> However this does not accomplish anything, a better refactor would to just
>>>> remove the argument altogether. It at least would simplify the interface
>>>> and allow slight better code generation.
>>>
>>> I tried to do that, but there is also that
>>> _dl_process_gnu_property() called from
>>> _dl_process_pt_gnu_property().
>>> For aarch64 it has this:
>>> unsigned int feature_1 = *(unsigned int *) data;
>>> if (feature_1 & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
>>> _dl_bti_protect (l, fd);
>>>
>>> What should I do here to remove that fd?
>>>
>>
>> In fact aarch64 _dl_bti_protect requires to know whether the map was done by
>> the kernel or not.
>>
>> Szabolcs, shouldn't the code:
>>
>> 1257 for (const ElfW(Phdr) *ph = &phdr[phnum]; ph != phdr; --ph)
>> 1258 switch (ph[-1].p_type)
>> 1259 {
>> 1260 case PT_NOTE:
>> 1261 _dl_process_pt_note (main_map, -1, &ph[-1]);
>> 1262 break;
>> 1263 case PT_GNU_PROPERTY:
>> 1264 _dl_process_pt_gnu_property (main_map, -1, &ph[-1]);
>> 1265 break;
>> 1266 }
>>
>> Take in consideration whether the main_map was allocated by the kernel or
>> by loader to pass the correct value on 'fd'?
>
> if the exe is loaded by ld.so then we already took care of bti
> (and other gnu properties) at load time.
>
> here we could skip processing properties again in that case but
> it does not hurt on aarch64 (it will try to mprotect with
> PROT_BTI again and ignore any failures).
Right, I was asking if this part is really necessary. Because if kernel
already supports BTI this will be covered, otherwise this seems unnecessary.
>
> however the fd must be passed down in elf/dl-load.c otherwise
> bti protection is silently dropped (when systemd filters out
> mprotect with PROT_EXEC, this is being fixed, but there are old
> systems configured like that).
>
This patch I figured out much.
Hi Adhemerval,
31.03.2023 17:54, Adhemerval Zanella Netto пишет:
>> however the fd must be passed down in elf/dl-load.c otherwise
>> bti protection is silently dropped (when systemd filters out
>> mprotect with PROT_EXEC, this is being fixed, but there are old
>> systems configured like that).
>>
> This patch I figured out much.
So if I understand correctly, these fds
should stay, right?
If so - may I bypass the _dl_process_pt_XX
calls completely in case of dlmem, or
are they needed for dlmem?
Sorry I have no idea what they do...
@@ -1379,10 +1379,10 @@ cannot enable executable stack as shared object requires");
switch (ph[-1].p_type)
{
case PT_NOTE:
- _dl_process_pt_note (l, fd, &ph[-1]);
+ _dl_process_pt_note (l, -1, &ph[-1]);
break;
case PT_GNU_PROPERTY:
- _dl_process_pt_gnu_property (l, fd, &ph[-1]);
+ _dl_process_pt_gnu_property (l, -1, &ph[-1]);
break;
}