diff mbox series

Avoid padding in _init and _fini. [BZ #31042]

Message ID 20231121123303.3405218-1-stli@linux.ibm.com
State Committed
Commit 807849965bbdeaa9b0a8f675d098efc520eeaaa8
Headers
Series Avoid padding in _init and _fini. [BZ #31042] |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed

Commit Message

Stefan Liebler Nov. 21, 2023, 12:33 p.m. UTC 
  The linker just concatenates the .init and .fini sections which
results in the complete _init and _fini functions. If needed the
linker adds padding bytes due to an alignment. GNU ld is adding
NOPs, which is fine.  But e.g. mold is adding traps which results
in broken _init and _fini functions.

Thus this patch removes the alignment in .init and .fini sections
in crtn.S files.

We keep the 4 byte function alignment in crti.S files. As the
assembler now also outputs the start of _init and _fini functions
as multiples of 4 byte, it perhaps has to fill it. Although GNU as
is using NOPs here, to be sure, we just keep the alignment with
0x07 (=NOPs) at the end of crti.S.

In order to avoid an obvious NOP slide in _fini, this patch also
uses an lg instead of lgr instruction. Then the emitted instructions
needs a multiple of 4 bytes.
---
 sysdeps/s390/s390-64/crti.S | 2 +-
 sysdeps/s390/s390-64/crtn.S | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

Comments

Adhemerval Zanella Netto Nov. 21, 2023, 1:10 p.m. UTC | #1 
On 21/11/23 09:33, Stefan Liebler wrote:
> The linker just concatenates the .init and .fini sections which
> results in the complete _init and _fini functions. If needed the
> linker adds padding bytes due to an alignment. GNU ld is adding
> NOPs, which is fine.  But e.g. mold is adding traps which results
> in broken _init and _fini functions.

Shouldn't mold follow GNU ld and fill with NOPs? Otherwise, mold will
only be able to link glibc installations with this fix.

> 
> Thus this patch removes the alignment in .init and .fini sections
> in crtn.S files.
> 
> We keep the 4 byte function alignment in crti.S files. As the
> assembler now also outputs the start of _init and _fini functions
> as multiples of 4 byte, it perhaps has to fill it. Although GNU as
> is using NOPs here, to be sure, we just keep the alignment with
> 0x07 (=NOPs) at the end of crti.S.
> 
> In order to avoid an obvious NOP slide in _fini, this patch also
> uses an lg instead of lgr instruction. Then the emitted instructions
> needs a multiple of 4 bytes.
> ---
>  sysdeps/s390/s390-64/crti.S | 2 +-
>  sysdeps/s390/s390-64/crtn.S | 2 --
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/sysdeps/s390/s390-64/crti.S b/sysdeps/s390/s390-64/crti.S
> index 11ab75e8d9..4c8246da26 100644
> --- a/sysdeps/s390/s390-64/crti.S
> +++ b/sysdeps/s390/s390-64/crti.S
> @@ -85,7 +85,7 @@ _init:
>  	.type	_fini,@function
>  _fini:
>  	stmg	%r6,%r15,48(%r15)
> -	lgr	%r1,%r15
> +	lg	%r1,120(%r15)
>  	aghi	%r15,-160
>  	stg	%r1,0(%r15)
>  	larl	%r12,_GLOBAL_OFFSET_TABLE_
> diff --git a/sysdeps/s390/s390-64/crtn.S b/sysdeps/s390/s390-64/crtn.S
> index 0eabcb346c..6bb1bc9dcf 100644
> --- a/sysdeps/s390/s390-64/crtn.S
> +++ b/sysdeps/s390/s390-64/crtn.S
> @@ -37,13 +37,11 @@
>     corresponding to the prologues in crti.S. */
>  
>  	.section .init
> -	.align	4
>  	lg	%r4,272(%r15)
>  	lmg	%r6,%r15,208(%r15)
>  	br	%r4
>  
>  	.section .fini
> -	.align	4
>  	lg	%r4,272(%r15)
>  	lmg	%r6,%r15,208(%r15)
>  	br	%r4
Rui Ueyama Nov. 21, 2023, 1:20 p.m. UTC | #2 
On Tue, Nov 21, 2023 at 10:10 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 21/11/23 09:33, Stefan Liebler wrote:
> > The linker just concatenates the .init and .fini sections which
> > results in the complete _init and _fini functions. If needed the
> > linker adds padding bytes due to an alignment. GNU ld is adding
> > NOPs, which is fine.  But e.g. mold is adding traps which results
> > in broken _init and _fini functions.
>
> Shouldn't mold follow GNU ld and fill with NOPs? Otherwise, mold will
> only be able to link glibc installations with this fix.

Well, we do follow GNU ld because otherwise we can't link working
executables. That being said, eliminating the dependency to this
particular GNU ld's behavior is I think generally a good idea,
especially given that only .init and .fini depend on it and it is easy
to eliminate it from them.

> >
> > Thus this patch removes the alignment in .init and .fini sections
> > in crtn.S files.
> >
> > We keep the 4 byte function alignment in crti.S files. As the
> > assembler now also outputs the start of _init and _fini functions
> > as multiples of 4 byte, it perhaps has to fill it. Although GNU as
> > is using NOPs here, to be sure, we just keep the alignment with
> > 0x07 (=NOPs) at the end of crti.S.
> >
> > In order to avoid an obvious NOP slide in _fini, this patch also
> > uses an lg instead of lgr instruction. Then the emitted instructions
> > needs a multiple of 4 bytes.
> > ---
> >  sysdeps/s390/s390-64/crti.S | 2 +-
> >  sysdeps/s390/s390-64/crtn.S | 2 --
> >  2 files changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/sysdeps/s390/s390-64/crti.S b/sysdeps/s390/s390-64/crti.S
> > index 11ab75e8d9..4c8246da26 100644
> > --- a/sysdeps/s390/s390-64/crti.S
> > +++ b/sysdeps/s390/s390-64/crti.S
> > @@ -85,7 +85,7 @@ _init:
> >       .type   _fini,@function
> >  _fini:
> >       stmg    %r6,%r15,48(%r15)
> > -     lgr     %r1,%r15
> > +     lg      %r1,120(%r15)
> >       aghi    %r15,-160
> >       stg     %r1,0(%r15)
> >       larl    %r12,_GLOBAL_OFFSET_TABLE_
> > diff --git a/sysdeps/s390/s390-64/crtn.S b/sysdeps/s390/s390-64/crtn.S
> > index 0eabcb346c..6bb1bc9dcf 100644
> > --- a/sysdeps/s390/s390-64/crtn.S
> > +++ b/sysdeps/s390/s390-64/crtn.S
> > @@ -37,13 +37,11 @@
> >     corresponding to the prologues in crti.S. */
> >
> >       .section .init
> > -     .align  4
> >       lg      %r4,272(%r15)
> >       lmg     %r6,%r15,208(%r15)
> >       br      %r4
> >
> >       .section .fini
> > -     .align  4
> >       lg      %r4,272(%r15)
> >       lmg     %r6,%r15,208(%r15)
> >       br      %r4
Stefan Liebler Nov. 21, 2023, 3:47 p.m. UTC | #3 
On 21.11.23 14:20, Rui Ueyama wrote:
> On Tue, Nov 21, 2023 at 10:10 PM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 21/11/23 09:33, Stefan Liebler wrote:
>>> The linker just concatenates the .init and .fini sections which
>>> results in the complete _init and _fini functions. If needed the
>>> linker adds padding bytes due to an alignment. GNU ld is adding
>>> NOPs, which is fine.  But e.g. mold is adding traps which results
>>> in broken _init and _fini functions.
>>
>> Shouldn't mold follow GNU ld and fill with NOPs? Otherwise, mold will
>> only be able to link glibc installations with this fix.
> 
I also think that mold should follow GNU ld as a user-object files with
a .init/.fini-section might also have an alignment which would result in
a broken _init/_fini function - of course not only on s390x.

> Well, we do follow GNU ld because otherwise we can't link working
> executables. That being said, eliminating the dependency to this
> particular GNU ld's behavior is I think generally a good idea,
> especially given that only .init and .fini depend on it and it is easy
> to eliminate it from them.
> 
But we don't need the alignment in crtn.S. s390x (64bit) is the only
crtn.S file with an alignment. As said, it is needed and also not
present in s390 (31bit).

If a linker e.g. wants to add a hardening-flag or switch defaults in
future, s390x will behave the same here as all other architectures.

>>>
>>> Thus this patch removes the alignment in .init and .fini sections
>>> in crtn.S files.
>>>
>>> We keep the 4 byte function alignment in crti.S files. As the
>>> assembler now also outputs the start of _init and _fini functions
>>> as multiples of 4 byte, it perhaps has to fill it. Although GNU as
>>> is using NOPs here, to be sure, we just keep the alignment with
>>> 0x07 (=NOPs) at the end of crti.S.
>>>
>>> In order to avoid an obvious NOP slide in _fini, this patch also
>>> uses an lg instead of lgr instruction. Then the emitted instructions
>>> needs a multiple of 4 bytes.
>>> ---
>>>  sysdeps/s390/s390-64/crti.S | 2 +-
>>>  sysdeps/s390/s390-64/crtn.S | 2 --
>>>  2 files changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/sysdeps/s390/s390-64/crti.S b/sysdeps/s390/s390-64/crti.S
>>> index 11ab75e8d9..4c8246da26 100644
>>> --- a/sysdeps/s390/s390-64/crti.S
>>> +++ b/sysdeps/s390/s390-64/crti.S
>>> @@ -85,7 +85,7 @@ _init:
>>>       .type   _fini,@function
>>>  _fini:
>>>       stmg    %r6,%r15,48(%r15)
>>> -     lgr     %r1,%r15
>>> +     lg      %r1,120(%r15)
>>>       aghi    %r15,-160
>>>       stg     %r1,0(%r15)
>>>       larl    %r12,_GLOBAL_OFFSET_TABLE_
>>> diff --git a/sysdeps/s390/s390-64/crtn.S b/sysdeps/s390/s390-64/crtn.S
>>> index 0eabcb346c..6bb1bc9dcf 100644
>>> --- a/sysdeps/s390/s390-64/crtn.S
>>> +++ b/sysdeps/s390/s390-64/crtn.S
>>> @@ -37,13 +37,11 @@
>>>     corresponding to the prologues in crti.S. */
>>>
>>>       .section .init
>>> -     .align  4
>>>       lg      %r4,272(%r15)
>>>       lmg     %r6,%r15,208(%r15)
>>>       br      %r4
>>>
>>>       .section .fini
>>> -     .align  4
>>>       lg      %r4,272(%r15)
>>>       lmg     %r6,%r15,208(%r15)
>>>       br      %r4
Adhemerval Zanella Netto Nov. 21, 2023, 3:57 p.m. UTC | #4 
On 21/11/23 12:47, Stefan Liebler wrote:
> On 21.11.23 14:20, Rui Ueyama wrote:
>> On Tue, Nov 21, 2023 at 10:10 PM Adhemerval Zanella Netto
>> <adhemerval.zanella@linaro.org> wrote:
>>>
>>>
>>>
>>> On 21/11/23 09:33, Stefan Liebler wrote:
>>>> The linker just concatenates the .init and .fini sections which
>>>> results in the complete _init and _fini functions. If needed the
>>>> linker adds padding bytes due to an alignment. GNU ld is adding
>>>> NOPs, which is fine.  But e.g. mold is adding traps which results
>>>> in broken _init and _fini functions.
>>>
>>> Shouldn't mold follow GNU ld and fill with NOPs? Otherwise, mold will
>>> only be able to link glibc installations with this fix.
>>
> I also think that mold should follow GNU ld as a user-object files with
> a .init/.fini-section might also have an alignment which would result in
> a broken _init/_fini function - of course not only on s390x.

Fair enough, from the commit message I had the impression that this change
was motivated to enable mold support.

> 
>> Well, we do follow GNU ld because otherwise we can't link working
>> executables. That being said, eliminating the dependency to this
>> particular GNU ld's behavior is I think generally a good idea,
>> especially given that only .init and .fini depend on it and it is easy
>> to eliminate it from them.
>>
> But we don't need the alignment in crtn.S. s390x (64bit) is the only
> crtn.S file with an alignment. As said, it is needed and also not
> present in s390 (31bit).
> 
> If a linker e.g. wants to add a hardening-flag or switch defaults in
> future, s390x will behave the same here as all other architectures.
> 
>>>>
>>>> Thus this patch removes the alignment in .init and .fini sections
>>>> in crtn.S files.
>>>>
>>>> We keep the 4 byte function alignment in crti.S files. As the
>>>> assembler now also outputs the start of _init and _fini functions
>>>> as multiples of 4 byte, it perhaps has to fill it. Although GNU as
>>>> is using NOPs here, to be sure, we just keep the alignment with
>>>> 0x07 (=NOPs) at the end of crti.S.
>>>>
>>>> In order to avoid an obvious NOP slide in _fini, this patch also
>>>> uses an lg instead of lgr instruction. Then the emitted instructions
>>>> needs a multiple of 4 bytes.
>>>> ---
>>>>  sysdeps/s390/s390-64/crti.S | 2 +-
>>>>  sysdeps/s390/s390-64/crtn.S | 2 --
>>>>  2 files changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/sysdeps/s390/s390-64/crti.S b/sysdeps/s390/s390-64/crti.S
>>>> index 11ab75e8d9..4c8246da26 100644
>>>> --- a/sysdeps/s390/s390-64/crti.S
>>>> +++ b/sysdeps/s390/s390-64/crti.S
>>>> @@ -85,7 +85,7 @@ _init:
>>>>       .type   _fini,@function
>>>>  _fini:
>>>>       stmg    %r6,%r15,48(%r15)
>>>> -     lgr     %r1,%r15
>>>> +     lg      %r1,120(%r15)
>>>>       aghi    %r15,-160
>>>>       stg     %r1,0(%r15)
>>>>       larl    %r12,_GLOBAL_OFFSET_TABLE_
>>>> diff --git a/sysdeps/s390/s390-64/crtn.S b/sysdeps/s390/s390-64/crtn.S
>>>> index 0eabcb346c..6bb1bc9dcf 100644
>>>> --- a/sysdeps/s390/s390-64/crtn.S
>>>> +++ b/sysdeps/s390/s390-64/crtn.S
>>>> @@ -37,13 +37,11 @@
>>>>     corresponding to the prologues in crti.S. */
>>>>
>>>>       .section .init
>>>> -     .align  4
>>>>       lg      %r4,272(%r15)
>>>>       lmg     %r6,%r15,208(%r15)
>>>>       br      %r4
>>>>
>>>>       .section .fini
>>>> -     .align  4
>>>>       lg      %r4,272(%r15)
>>>>       lmg     %r6,%r15,208(%r15)
>>>>       br      %r4
>
Stefan Liebler Nov. 30, 2023, 12:36 p.m. UTC | #5 
I've just committed the patch and closed the bugzilla

On 21.11.23 16:57, Adhemerval Zanella Netto wrote:
> 
> 
> On 21/11/23 12:47, Stefan Liebler wrote:
>> On 21.11.23 14:20, Rui Ueyama wrote:
>>> On Tue, Nov 21, 2023 at 10:10 PM Adhemerval Zanella Netto
>>> <adhemerval.zanella@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 21/11/23 09:33, Stefan Liebler wrote:
>>>>> The linker just concatenates the .init and .fini sections which
>>>>> results in the complete _init and _fini functions. If needed the
>>>>> linker adds padding bytes due to an alignment. GNU ld is adding
>>>>> NOPs, which is fine.  But e.g. mold is adding traps which results
>>>>> in broken _init and _fini functions.
>>>>
>>>> Shouldn't mold follow GNU ld and fill with NOPs? Otherwise, mold will
>>>> only be able to link glibc installations with this fix.
>>>
>> I also think that mold should follow GNU ld as a user-object files with
>> a .init/.fini-section might also have an alignment which would result in
>> a broken _init/_fini function - of course not only on s390x.
> 
> Fair enough, from the commit message I had the impression that this change
> was motivated to enable mold support.
> 
>>
>>> Well, we do follow GNU ld because otherwise we can't link working
>>> executables. That being said, eliminating the dependency to this
>>> particular GNU ld's behavior is I think generally a good idea,
>>> especially given that only .init and .fini depend on it and it is easy
>>> to eliminate it from them.
>>>
>> But we don't need the alignment in crtn.S. s390x (64bit) is the only
>> crtn.S file with an alignment. As said, it is needed and also not
>> present in s390 (31bit).
>>
>> If a linker e.g. wants to add a hardening-flag or switch defaults in
>> future, s390x will behave the same here as all other architectures.
>>
>>>>>
>>>>> Thus this patch removes the alignment in .init and .fini sections
>>>>> in crtn.S files.
>>>>>
>>>>> We keep the 4 byte function alignment in crti.S files. As the
>>>>> assembler now also outputs the start of _init and _fini functions
>>>>> as multiples of 4 byte, it perhaps has to fill it. Although GNU as
>>>>> is using NOPs here, to be sure, we just keep the alignment with
>>>>> 0x07 (=NOPs) at the end of crti.S.
>>>>>
>>>>> In order to avoid an obvious NOP slide in _fini, this patch also
>>>>> uses an lg instead of lgr instruction. Then the emitted instructions
>>>>> needs a multiple of 4 bytes.
>>>>> ---
>>>>>  sysdeps/s390/s390-64/crti.S | 2 +-
>>>>>  sysdeps/s390/s390-64/crtn.S | 2 --
>>>>>  2 files changed, 1 insertion(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/sysdeps/s390/s390-64/crti.S b/sysdeps/s390/s390-64/crti.S
>>>>> index 11ab75e8d9..4c8246da26 100644
>>>>> --- a/sysdeps/s390/s390-64/crti.S
>>>>> +++ b/sysdeps/s390/s390-64/crti.S
>>>>> @@ -85,7 +85,7 @@ _init:
>>>>>       .type   _fini,@function
>>>>>  _fini:
>>>>>       stmg    %r6,%r15,48(%r15)
>>>>> -     lgr     %r1,%r15
>>>>> +     lg      %r1,120(%r15)
>>>>>       aghi    %r15,-160
>>>>>       stg     %r1,0(%r15)
>>>>>       larl    %r12,_GLOBAL_OFFSET_TABLE_
>>>>> diff --git a/sysdeps/s390/s390-64/crtn.S b/sysdeps/s390/s390-64/crtn.S
>>>>> index 0eabcb346c..6bb1bc9dcf 100644
>>>>> --- a/sysdeps/s390/s390-64/crtn.S
>>>>> +++ b/sysdeps/s390/s390-64/crtn.S
>>>>> @@ -37,13 +37,11 @@
>>>>>     corresponding to the prologues in crti.S. */
>>>>>
>>>>>       .section .init
>>>>> -     .align  4
>>>>>       lg      %r4,272(%r15)
>>>>>       lmg     %r6,%r15,208(%r15)
>>>>>       br      %r4
>>>>>
>>>>>       .section .fini
>>>>> -     .align  4
>>>>>       lg      %r4,272(%r15)
>>>>>       lmg     %r6,%r15,208(%r15)
>>>>>       br      %r4
>>
diff mbox series

Patch

diff --git a/sysdeps/s390/s390-64/crti.S b/sysdeps/s390/s390-64/crti.S
index 11ab75e8d9..4c8246da26 100644
--- a/sysdeps/s390/s390-64/crti.S
+++ b/sysdeps/s390/s390-64/crti.S
@@ -85,7 +85,7 @@  _init:
 	.type	_fini,@function
 _fini:
 	stmg	%r6,%r15,48(%r15)
-	lgr	%r1,%r15
+	lg	%r1,120(%r15)
 	aghi	%r15,-160
 	stg	%r1,0(%r15)
 	larl	%r12,_GLOBAL_OFFSET_TABLE_
diff --git a/sysdeps/s390/s390-64/crtn.S b/sysdeps/s390/s390-64/crtn.S
index 0eabcb346c..6bb1bc9dcf 100644
--- a/sysdeps/s390/s390-64/crtn.S
+++ b/sysdeps/s390/s390-64/crtn.S
@@ -37,13 +37,11 @@ 
    corresponding to the prologues in crti.S. */
 
 	.section .init
-	.align	4
 	lg	%r4,272(%r15)
 	lmg	%r6,%r15,208(%r15)
 	br	%r4
 
 	.section .fini
-	.align	4
 	lg	%r4,272(%r15)
 	lmg	%r6,%r15,208(%r15)
 	br	%r4