Misaligned stack on 32-bit s390?

Misaligned stack on 32-bit s390?

[Carlos O'Donell]

Andreas,

In sysdeps/s390/s390-32/dl-machine.h:

174         # Adjust the stack pointer to skip _dl_skip_args words.\n\
175         sll   %r1,2\n\
176         ar    %r15,%r1\n\

This misalign the stack.

The 32-bit ABI requires an 8-byte alignment.

If we skip over 4 bytes the stack is no longer aligned.

We're seeing crashes when running the dynamic loader manually
as the unaligned stack is passed to the IFUNC resolver which
uses an instruction that expects an aligned stack
e.g. "stfle   96(%r15)"

Any clever ideas on how to fix this without copying up a large
portion of the stack?

Cheers,
Carlos.

Re: Misaligned stack on 32-bit s390?

[Richard Henderson]

On 11/11/2014 06:31 AM, Carlos O'Donell wrote:
> Any clever ideas on how to fix this without copying up a large
> portion of the stack?

Nope, because other targets do in fact have to do just that.

I'm actually surprised that almost all of them don't.  I suppose
that just depends on how the ABI is set up to pass parameters to
the user _start...

Fortunately, s390 has a block copy instruction, so the move
should be trivial to implement.


r~

Re: Misaligned stack on 32-bit s390?

[Carlos O'Donell]

On 11/11/2014 04:28 AM, Richard Henderson wrote:
> On 11/11/2014 06:31 AM, Carlos O'Donell wrote:
>> Any clever ideas on how to fix this without copying up a large
>> portion of the stack?
> 
> Nope, because other targets do in fact have to do just that.

Right.
 
> I'm actually surprised that almost all of them don't.  I suppose
> that just depends on how the ABI is set up to pass parameters to
> the user _start...

Yes, on hppa the ABI was such that I didn't depend on the layout,
but s390 does, thus I think it is non-trivial.

The argv might be trivial to copy, but the size of envp is unknown,
and _start from the application side in start.S expects argv and
envp to be right up against eachother AFAICT. Worse it wants to
scan past them in order to find auxv. The saving grace is that
the auxv scan parses an arbitrary number of zeros between envp
and auxv. So I can move the gap to be between envp and auxv.
 
> Fortunately, s390 has a block copy instruction, so the move
> should be trivial to implement.

For argv only. What instruction is the block copy? Are you
talking about lm/stm?

The most naive fix I have working is as follows (I've handed
off to Siddhesh to have a look since I'm out of time this
evening/morning).

diff --git a/sysdeps/s390/s390-32/dl-machine.h b/sysdeps/s390/s390-32/dl-machine.h
index c56185c..b189552 100644
--- a/sysdeps/s390/s390-32/dl-machine.h
+++ b/sysdeps/s390/s390-32/dl-machine.h
@@ -166,18 +166,47 @@ _dl_start_user:\n\
 	# See if we were run as a command with the executable file\n\
 	# name as an extra leading argument.\n\
 	l     %r1,_dl_skip_args@GOT12(0,%r12)\n\
-	l     %r1,0(%r1)          # load _dl_skip_args\n\
+	l     %r1,0(%r1)	# load _dl_skip_args\n\
+	ltr   %r1,%r1\n\
+	je    .L4		# Skip the arg adjustment if there were none.\n\
 	# Get the original argument count.\n\
 	l     %r0,96(%r15)\n\
 	# Subtract _dl_skip_args from it.\n\
 	sr    %r0,%r1\n\
-	# Adjust the stack pointer to skip _dl_skip_args words.\n\
-	sll   %r1,2\n\
-	ar    %r15,%r1\n\
-	# Set the back chain to zero again\n\
-	xc    0(4,%r15),0(%r15)\n\
 	# Store back the modified argument count.\n\
 	st    %r0,96(%r15)\n\
+	# Copy argv and envp forward to account for skipped argv entries.\n\
+	# We skipped at least one argument or we would not get here.\n\
+	la    %r6,100(%r15)	# Destination pointer i.e. &argv[0]\n\
+	lr    %r5,%r6\n\
+	lr    %r0,%r1\n\
+	sll   %r0,2\n\		# Number of skipped bytes.\n\
+	ar    %r5,%r0		# Source pointer = Dest + Skipped args.\n\
+	# argv copy loop:\n\
+.L1:	l     %r7,0(%r5)	# Load a word from the source.\n\
+	st    %r7,0(%r6)	# Store the word in the destination.\n\
+	ahi   %r5,4\n\
+	ahi   %r6,4\n\
+	ltr   %r7,%r7\n\
+	jne   .L1		# Stop after copying the NULL.\n\
+	# envp copy loop:\n\
+.L2:	l     %r7,0(%r5)	# Load a word from the source.\n\
+	st    %r7,0(%r6)	# Store the word in the destination.\n\
+	ahi   %r5,4\n\
+	ahi   %r6,4\n\
+	ltr   %r7,%r7\n\
+	jne   .L2		# Stop after copying the NULL.\n\
+	# Now we have to zero out the envp entries after NULL to allow\n\
+	# start.S to properly find auxv by skipping zeroes.\n\
+	# zero out loop:\n\
+	lhi   %r7,0\n\
+.L3:	st    %r7,0(%r6)	# Store zero.\n\
+	ahi   %r6,4		# Advance dest pointer.\n\
+	ahi   %r1,-1		# Subtract one from the word count.\n\
+	ltr   %r1,%r1\n\
+	jne    .L3		# Keep copying if the word count is non-zero.\n\
+	# Set the back chain to zero again\n\
+.L4:	xc    0(4,%r15),0(%r15)\n\
 	# The special initializer gets called with the stack just\n\
 	# as the application's entry point will see it; it can\n\
 	# switch stacks if it moves these contents over.\n\
---

Cheers,
Carlos.

Re: Misaligned stack on 32-bit s390?

[Richard Henderson]

On 11/11/2014 10:49 AM, Carlos O'Donell wrote:
> For argv only. What instruction is the block copy? Are you
> talking about lm/stm?

I was thinking of EX combined with MVC.

But of course you're right about envp and argp, where you'd
have to count the number of entries before forming the count.

> +	# Now we have to zero out the envp entries after NULL to allow\n\
> +	# start.S to properly find auxv by skipping zeroes.\n\
> +	# zero out loop:\n\
> +	lhi   %r7,0\n\
> +.L3:	st    %r7,0(%r6)	# Store zero.\n\
> +	ahi   %r6,4		# Advance dest pointer.\n\
> +	ahi   %r1,-1		# Subtract one from the word count.\n\
> +	ltr   %r1,%r1\n\
> +	jne    .L3		# Keep copying if the word count is non-zero.\n\

In the alpha port, I just copy auxv down as well.


r~

Re: Misaligned stack on 32-bit s390?

[Siddhesh Poyarekar]

[-- Attachment #1: Type: text/plain, Size: 240 bytes --]

On Tue, Nov 11, 2014 at 04:49:56AM -0500, Carlos O'Donell wrote:
> +	# Set the back chain to zero again\n\
> +.L4:	xc    0(4,%r15),0(%r15)\n\

I don't think this is needed now, since you're not shifting the stack
pointer anymore.

Siddhesh

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: Misaligned stack on 32-bit s390?

[Andreas Krebbel]

On 11/11/2014 10:49 AM, Carlos O'Donell wrote:
> On 11/11/2014 04:28 AM, Richard Henderson wrote:
>> On 11/11/2014 06:31 AM, Carlos O'Donell wrote:
>>> Any clever ideas on how to fix this without copying up a large
>>> portion of the stack?
>>
>> Nope, because other targets do in fact have to do just that.
> 
> Right.
> 
>> I'm actually surprised that almost all of them don't.  I suppose
>> that just depends on how the ABI is set up to pass parameters to
>> the user _start...
> 
> Yes, on hppa the ABI was such that I didn't depend on the layout,
> but s390 does, thus I think it is non-trivial.
> 
> The argv might be trivial to copy, but the size of envp is unknown,
> and _start from the application side in start.S expects argv and
> envp to be right up against eachother AFAICT. Worse it wants to
> scan past them in order to find auxv. The saving grace is that
> the auxv scan parses an arbitrary number of zeros between envp
> and auxv. So I can move the gap to be between envp and auxv.
> 
>> Fortunately, s390 has a block copy instruction, so the move
>> should be trivial to implement.
> 
> For argv only. What instruction is the block copy? Are you
> talking about lm/stm?
> 
> The most naive fix I have working is as follows (I've handed
> off to Siddhesh to have a look since I'm out of time this
> evening/morning).

Thanks for working on this!  Really surprising that this stayed unnoticed for that long. I thought
the testsuite invokes the testcases as ld.so parameter as well?

As I understand it an alignment fix is only required if _dl_skip_args is odd.  So what about
checking the least significant bit (tmll) and copying argv/envv down by 4 bytes? That way we would
not need to copy anything in 50% of the cases?

Would you like to continue working on it or should we try to take over? (Stefan or myself)

Bye,

-Andreas-

> 
> diff --git a/sysdeps/s390/s390-32/dl-machine.h b/sysdeps/s390/s390-32/dl-machine.h
> index c56185c..b189552 100644
> --- a/sysdeps/s390/s390-32/dl-machine.h
> +++ b/sysdeps/s390/s390-32/dl-machine.h
> @@ -166,18 +166,47 @@ _dl_start_user:\n\
>  	# See if we were run as a command with the executable file\n\
>  	# name as an extra leading argument.\n\
>  	l     %r1,_dl_skip_args@GOT12(0,%r12)\n\
> -	l     %r1,0(%r1)          # load _dl_skip_args\n\
> +	l     %r1,0(%r1)	# load _dl_skip_args\n\
> +	ltr   %r1,%r1\n\
> +	je    .L4		# Skip the arg adjustment if there were none.\n\
>  	# Get the original argument count.\n\
>  	l     %r0,96(%r15)\n\
>  	# Subtract _dl_skip_args from it.\n\
>  	sr    %r0,%r1\n\
> -	# Adjust the stack pointer to skip _dl_skip_args words.\n\
> -	sll   %r1,2\n\
> -	ar    %r15,%r1\n\
> -	# Set the back chain to zero again\n\
> -	xc    0(4,%r15),0(%r15)\n\
>  	# Store back the modified argument count.\n\
>  	st    %r0,96(%r15)\n\
> +	# Copy argv and envp forward to account for skipped argv entries.\n\
> +	# We skipped at least one argument or we would not get here.\n\
> +	la    %r6,100(%r15)	# Destination pointer i.e. &argv[0]\n\
> +	lr    %r5,%r6\n\
> +	lr    %r0,%r1\n\
> +	sll   %r0,2\n\		# Number of skipped bytes.\n\
> +	ar    %r5,%r0		# Source pointer = Dest + Skipped args.\n\
> +	# argv copy loop:\n\
> +.L1:	l     %r7,0(%r5)	# Load a word from the source.\n\
> +	st    %r7,0(%r6)	# Store the word in the destination.\n\
> +	ahi   %r5,4\n\
> +	ahi   %r6,4\n\
> +	ltr   %r7,%r7\n\
> +	jne   .L1		# Stop after copying the NULL.\n\
> +	# envp copy loop:\n\
> +.L2:	l     %r7,0(%r5)	# Load a word from the source.\n\
> +	st    %r7,0(%r6)	# Store the word in the destination.\n\
> +	ahi   %r5,4\n\
> +	ahi   %r6,4\n\
> +	ltr   %r7,%r7\n\
> +	jne   .L2		# Stop after copying the NULL.\n\
> +	# Now we have to zero out the envp entries after NULL to allow\n\
> +	# start.S to properly find auxv by skipping zeroes.\n\
> +	# zero out loop:\n\
> +	lhi   %r7,0\n\
> +.L3:	st    %r7,0(%r6)	# Store zero.\n\
> +	ahi   %r6,4		# Advance dest pointer.\n\
> +	ahi   %r1,-1		# Subtract one from the word count.\n\
> +	ltr   %r1,%r1\n\
> +	jne    .L3		# Keep copying if the word count is non-zero.\n\
> +	# Set the back chain to zero again\n\
> +.L4:	xc    0(4,%r15),0(%r15)\n\
>  	# The special initializer gets called with the stack just\n\
>  	# as the application's entry point will see it; it can\n\
>  	# switch stacks if it moves these contents over.\n\
> ---
> 
> Cheers,
> Carlos.
> 

Re: Misaligned stack on 32-bit s390?

[Andreas Schwab]

Andreas Krebbel <krebbel@linux.vnet.ibm.com> writes:

> As I understand it an alignment fix is only required if _dl_skip_args is
> odd.  So what about
> checking the least significant bit (tmll) and copying argv/envv down by 4
> bytes? That way we would
> not need to copy anything in 50% of the cases?

I don't think it makes sense optimizing for this case, which is rare
outside of runing the glibc testsuite.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Misaligned stack on 32-bit s390?

[Andreas Krebbel]

On 11/11/2014 10:49 AM, Carlos O'Donell wrote:
> On 11/11/2014 04:28 AM, Richard Henderson wrote:
>> On 11/11/2014 06:31 AM, Carlos O'Donell wrote:
>>> Any clever ideas on how to fix this without copying up a large
>>> portion of the stack?
>>
>> Nope, because other targets do in fact have to do just that.
> 
> Right.
> 
>> I'm actually surprised that almost all of them don't.  I suppose
>> that just depends on how the ABI is set up to pass parameters to
>> the user _start...
> 
> Yes, on hppa the ABI was such that I didn't depend on the layout,
> but s390 does, thus I think it is non-trivial.
> 
> The argv might be trivial to copy, but the size of envp is unknown,
> and _start from the application side in start.S expects argv and
> envp to be right up against eachother AFAICT. Worse it wants to
> scan past them in order to find auxv. The saving grace is that
> the auxv scan parses an arbitrary number of zeros between envp
> and auxv. So I can move the gap to be between envp and auxv.
> 
>> Fortunately, s390 has a block copy instruction, so the move
>> should be trivial to implement.
> 
> For argv only. What instruction is the block copy? Are you
> talking about lm/stm?
> 
> The most naive fix I have working is as follows (I've handed
> off to Siddhesh to have a look since I'm out of time this
> evening/morning).
> 
> diff --git a/sysdeps/s390/s390-32/dl-machine.h b/sysdeps/s390/s390-32/dl-machine.h
> index c56185c..b189552 100644
> --- a/sysdeps/s390/s390-32/dl-machine.h
> +++ b/sysdeps/s390/s390-32/dl-machine.h
> @@ -166,18 +166,47 @@ _dl_start_user:\n\
>  	# See if we were run as a command with the executable file\n\
>  	# name as an extra leading argument.\n\
>  	l     %r1,_dl_skip_args@GOT12(0,%r12)\n\
> -	l     %r1,0(%r1)          # load _dl_skip_args\n\
> +	l     %r1,0(%r1)	# load _dl_skip_args\n\
> +	ltr   %r1,%r1\n\
> +	je    .L4		# Skip the arg adjustment if there were none.\n\
>  	# Get the original argument count.\n\
>  	l     %r0,96(%r15)\n\
>  	# Subtract _dl_skip_args from it.\n\
>  	sr    %r0,%r1\n\
> -	# Adjust the stack pointer to skip _dl_skip_args words.\n\
> -	sll   %r1,2\n\
> -	ar    %r15,%r1\n\
> -	# Set the back chain to zero again\n\
> -	xc    0(4,%r15),0(%r15)\n\
>  	# Store back the modified argument count.\n\
>  	st    %r0,96(%r15)\n\
> +	# Copy argv and envp forward to account for skipped argv entries.\n\
> +	# We skipped at least one argument or we would not get here.\n\
> +	la    %r6,100(%r15)	# Destination pointer i.e. &argv[0]\n\
> +	lr    %r5,%r6\n\
> +	lr    %r0,%r1\n\
> +	sll   %r0,2\n\		# Number of skipped bytes.\n\
> +	ar    %r5,%r0		# Source pointer = Dest + Skipped args.\n\
> +	# argv copy loop:\n\
> +.L1:	l     %r7,0(%r5)	# Load a word from the source.\n\
> +	st    %r7,0(%r6)	# Store the word in the destination.\n\
> +	ahi   %r5,4\n\
> +	ahi   %r6,4\n\
> +	ltr   %r7,%r7\n\
> +	jne   .L1		# Stop after copying the NULL.\n\
> +	# envp copy loop:\n\
> +.L2:	l     %r7,0(%r5)	# Load a word from the source.\n\
> +	st    %r7,0(%r6)	# Store the word in the destination.\n\
> +	ahi   %r5,4\n\
> +	ahi   %r6,4\n\
> +	ltr   %r7,%r7\n\
> +	jne   .L2		# Stop after copying the NULL.\n\
> +	# Now we have to zero out the envp entries after NULL to allow\n\
> +	# start.S to properly find auxv by skipping zeroes.\n\
> +	# zero out loop:\n\
> +	lhi   %r7,0\n\
> +.L3:	st    %r7,0(%r6)	# Store zero.\n\
> +	ahi   %r6,4		# Advance dest pointer.\n\
> +	ahi   %r1,-1		# Subtract one from the word count.\n\
> +	ltr   %r1,%r1\n\
> +	jne    .L3		# Keep copying if the word count is non-zero.\n\
> +	# Set the back chain to zero again\n\
> +.L4:	xc    0(4,%r15),0(%r15)\n\
>  	# The special initializer gets called with the stack just\n\
>  	# as the application's entry point will see it; it can\n\
>  	# switch stacks if it moves these contents over.\n\

The patch is ok to apply with the xc removed. Thanks!

A minor improvement might be to make use of an index reg in the loops. Perhaps something like this
(untested):

 +	# argv copy loop:\n\
 +	lhi   %r8,0
 +.L1:	l     %r7,0(%r8,%r5)	# Load a word from the source.\n\
 +	st    %r7,0(%r8,%r6)	# Store the word in the destination.\n\
 +	ahi   %r8,4\n\
 +	ltr   %r7,%r7\n\
 +	jne   .L1		# Stop after copying the NULL.\n\
 +	# envp copy loop:\n\
 +.L2:	l     %r7,0(%r8,%r5)	# Load a word from the source.\n\
 +	st    %r7,0(%r8,%r6)	# Store the word in the destination.\n\
 +	ahi   %r8,4\n\
 +	ltr   %r7,%r7\n\
 +	jne   .L2		# Stop after copying the NULL.\n\
 +	# Now we have to zero out the envp entries after NULL to allow\n\
 +	# start.S to properly find auxv by skipping zeroes.\n\
 +	# zero out loop:\n\
 +	lhi   %r7,0\n\
 +.L3:	st    %r7,0(%r8,%r6)	# Store zero.\n\
 +	ahi   %r8,4		# Advance dest pointer.\n\
 +	ahi   %r1,-1		# Subtract one from the word count.\n\
 +	ltr   %r1,%r1\n\
 +	jne    .L3		# Keep copying if the word count is non-zero.\n\
 +	# Set the back chain to zero again\n\
 +.L4:

Bye,

-Andreas-

Re: Misaligned stack on 32-bit s390?

[Carlos O'Donell]

On 11/11/2014 09:54 AM, Andreas Krebbel wrote:
> Would you like to continue working on it or should we try to take
> over? (Stefan or myself)

We can get it done. It would be great to have you review the final
patch.

Please note that the patch I posted is incomplete, it fails to
readjust _dl_argv which is cached by the loader and needs to be
changed if argv is moved. Simple fix though.

I also don't know how long s390 lasted without this breaking
something. I guess aligned stacks don't really matter all that
much ;-)

Cheers,
Carlos.

Re: Misaligned stack on 32-bit s390?

[Siddhesh Poyarekar]

[-- Attachment #1: Type: text/plain, Size: 5473 bytes --]

On Tue, Nov 11, 2014 at 11:00:43PM -0500, Carlos O'Donell wrote:
> Please note that the patch I posted is incomplete, it fails to
> readjust _dl_argv which is cached by the loader and needs to be
> changed if argv is moved. Simple fix though.

Here's the updated patch that adjusts _dl_argv.  As a result now,
_dl_argv is no longer relro on s390.  Tested to verify that there are
no new failures on s390.

OK to commit?

Siddhesh

	* sysdeps/s390/s390-32/dl-machine.h (_dl_start_user):
	Move argv and envp down instead of moving argc up.
	* sysdeps/s390/s390-32/dl-sysdep.h: New file.

commit b4b885c804fa494a7346794a2d8f54186a8af828
Author: Siddhesh Poyarekar <siddhesh@redhat.com>
Date:   Wed Nov 12 12:37:51 2014 +0530

    Fix stack alignment when loader is invoked directly
    
    The s390 ABI requires the stack pointer to be aligned at 8-bytes.
    When a program is invoked as an argument to the dynamic linker,
    _dl_start_user adjusts the stack to remove the dynamic linker
    arguments so that the program sees only its name and arguments.  This
    may result in the stack being misaligned since each argument shift is
    only a word and not a double-word.
    
    This is now fixed shifting argv and envp down instead of shifting argc
    up and reclaiming the stack.  This requires _dl_argv to be adjusted
    and hence, is no longer relro.

diff --git a/sysdeps/s390/s390-32/dl-machine.h b/sysdeps/s390/s390-32/dl-machine.h
index c56185c..fba8d60 100644
--- a/sysdeps/s390/s390-32/dl-machine.h
+++ b/sysdeps/s390/s390-32/dl-machine.h
@@ -166,18 +166,49 @@ _dl_start_user:\n\
 	# See if we were run as a command with the executable file\n\
 	# name as an extra leading argument.\n\
 	l     %r1,_dl_skip_args@GOT12(0,%r12)\n\
-	l     %r1,0(%r1)          # load _dl_skip_args\n\
+	l     %r1,0(%r1)	# load _dl_skip_args\n\
+	ltr   %r1,%r1\n\
+	je    .L4		# Skip the arg adjustment if there were none.\n\
 	# Get the original argument count.\n\
 	l     %r0,96(%r15)\n\
 	# Subtract _dl_skip_args from it.\n\
 	sr    %r0,%r1\n\
-	# Adjust the stack pointer to skip _dl_skip_args words.\n\
-	sll   %r1,2\n\
-	ar    %r15,%r1\n\
-	# Set the back chain to zero again\n\
-	xc    0(4,%r15),0(%r15)\n\
 	# Store back the modified argument count.\n\
 	st    %r0,96(%r15)\n\
+	# Copy argv and envp forward to account for skipped argv entries.\n\
+	# We skipped at least one argument or we would not get here.\n\
+	la    %r6,100(%r15)	# Destination pointer i.e. &argv[0]\n\
+	lr    %r5,%r6\n\
+	lr    %r0,%r1\n\
+	sll   %r0,2\n		# Number of skipped bytes.\n\
+	ar    %r5,%r0		# Source pointer = Dest + Skipped args.\n\
+	# argv copy loop:\n\
+.L1:	l     %r7,0(%r5)	# Load a word from the source.\n\
+	st    %r7,0(%r6)	# Store the word in the destination.\n\
+	ahi   %r5,4\n\
+	ahi   %r6,4\n\
+	ltr   %r7,%r7\n\
+	jne   .L1		# Stop after copying the NULL.\n\
+	# envp copy loop:\n\
+.L2:	l     %r7,0(%r5)	# Load a word from the source.\n\
+	st    %r7,0(%r6)	# Store the word in the destination.\n\
+	ahi   %r5,4\n\
+	ahi   %r6,4\n\
+	ltr   %r7,%r7\n\
+	jne   .L2		# Stop after copying the NULL.\n\
+	# Now we have to zero out the envp entries after NULL to allow\n\
+	# start.S to properly find auxv by skipping zeroes.\n\
+	# zero out loop:\n\
+	lhi   %r7,0\n\
+.L3:	st    %r7,0(%r6)	# Store zero.\n\
+	ahi   %r6,4		# Advance dest pointer.\n\
+	ahi   %r1,-1		# Subtract one from the word count.\n\
+	ltr   %r1,%r1\n\
+	jne    .L3		# Keep copying if the word count is non-zero.\n\
+	# Adjust _dl_argv\n\
+	la    %r6,100(%r15)\n\
+	l     %r1,_dl_argv@GOT12(0,%r12)\n\
+	st    %r6,0(%r1)\n\
 	# The special initializer gets called with the stack just\n\
 	# as the application's entry point will see it; it can\n\
 	# switch stacks if it moves these contents over.\n\
@@ -185,7 +216,7 @@ _dl_start_user:\n\
 	# Call the function to run the initializers.\n\
 	# Load the parameters:\n\
 	# (%r2, %r3, %r4, %r5) = (_dl_loaded, argc, argv, envp)\n\
-	l     %r2,_rtld_local@GOT(%r12)\n\
+.L4:	l     %r2,_rtld_local@GOT(%r12)\n\
 	l     %r2,0(%r2)\n\
 	l     %r3,96(%r15)\n\
 	la    %r4,100(%r15)\n\
diff --git a/sysdeps/s390/s390-32/dl-sysdep.h b/sysdeps/s390/s390-32/dl-sysdep.h
new file mode 100644
index 0000000..b992778
--- /dev/null
+++ b/sysdeps/s390/s390-32/dl-sysdep.h
@@ -0,0 +1,23 @@
+/* System-specific settings for dynamic linker code.  S/390 version.
+   Copyright (C) 2014 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include_next <dl-sysdep.h>
+
+/* _dl_argv cannot be attribute_relro, because _dl_start_user
+   might write into it after _dl_start returns.  */
+#define DL_ARGV_NOT_RELRO 1

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: Misaligned stack on 32-bit s390?

[Richard Henderson]

On 11/12/2014 08:09 AM, Siddhesh Poyarekar wrote:
> +	l     %r1,0(%r1)	# load _dl_skip_args\n\
> +	ltr   %r1,%r1\n\

	lt	%r1, 0(%r1)

> +	l     %r1,_dl_argv@GOT12(0,%r12)\n\

Why are you using got12?  Drop the indexed form and just use got.
Hmm.  I do see there's a mix in this file, but I don't know why...


r~

Re: Misaligned stack on 32-bit s390?

[Siddhesh Poyarekar]

[-- Attachment #1: Type: text/plain, Size: 5615 bytes --]

On Wed, Nov 12, 2014 at 08:18:48AM +0100, Richard Henderson wrote:
> On 11/12/2014 08:09 AM, Siddhesh Poyarekar wrote:
> > +	l     %r1,0(%r1)	# load _dl_skip_args\n\
> > +	ltr   %r1,%r1\n\
> 
> 	lt	%r1, 0(%r1)
> 
> > +	l     %r1,_dl_argv@GOT12(0,%r12)\n\
> 
> Why are you using got12?  Drop the indexed form and just use got.
> Hmm.  I do see there's a mix in this file, but I don't know why...

I got an invalid reference when I used GOT and my first thought was
that the binutils manual was lying.  I think I later actually fixed it
by changing something else and I just forgot to verify the manual's
honesty again.  I'll change the remaining GOT12 in a separate patch.

Updated patch, generated code is identical.

Siddhesh

commit 5767d20b332eda3501a2b559bfdbdac943c524fd
Author: Siddhesh Poyarekar <siddhesh@redhat.com>
Date:   Wed Nov 12 13:17:15 2014 +0530

    Fix stack alignment when loader is invoked directly
    
    The s390 ABI requires the stack pointer to be aligned at 8-bytes.
    When a program is invoked as an argument to the dynamic linker,
    _dl_start_user adjusts the stack to remove the dynamic linker
    arguments so that the program sees only its name and arguments.  This
    may result in the stack being misaligned since each argument shift is
    only a word and not a double-word.
    
    This is now fixed shifting argv and envp down instead of shifting argc
    up and reclaiming the stack.  This requires _dl_argv to be adjusted
    and hence, is no longer relro.

diff --git a/sysdeps/s390/s390-32/dl-machine.h b/sysdeps/s390/s390-32/dl-machine.h
index c56185c..0fd5a4f 100644
--- a/sysdeps/s390/s390-32/dl-machine.h
+++ b/sysdeps/s390/s390-32/dl-machine.h
@@ -166,18 +166,49 @@ _dl_start_user:\n\
 	# See if we were run as a command with the executable file\n\
 	# name as an extra leading argument.\n\
 	l     %r1,_dl_skip_args@GOT12(0,%r12)\n\
-	l     %r1,0(%r1)          # load _dl_skip_args\n\
+	l     %r1,0(%r1)	# load _dl_skip_args\n\
+	ltr   %r1,%r1\n\
+	je    .L4		# Skip the arg adjustment if there were none.\n\
 	# Get the original argument count.\n\
 	l     %r0,96(%r15)\n\
 	# Subtract _dl_skip_args from it.\n\
 	sr    %r0,%r1\n\
-	# Adjust the stack pointer to skip _dl_skip_args words.\n\
-	sll   %r1,2\n\
-	ar    %r15,%r1\n\
-	# Set the back chain to zero again\n\
-	xc    0(4,%r15),0(%r15)\n\
 	# Store back the modified argument count.\n\
 	st    %r0,96(%r15)\n\
+	# Copy argv and envp forward to account for skipped argv entries.\n\
+	# We skipped at least one argument or we would not get here.\n\
+	la    %r6,100(%r15)	# Destination pointer i.e. &argv[0]\n\
+	lr    %r5,%r6\n\
+	lr    %r0,%r1\n\
+	sll   %r0,2\n		# Number of skipped bytes.\n\
+	ar    %r5,%r0		# Source pointer = Dest + Skipped args.\n\
+	# argv copy loop:\n\
+.L1:	l     %r7,0(%r5)	# Load a word from the source.\n\
+	st    %r7,0(%r6)	# Store the word in the destination.\n\
+	ahi   %r5,4\n\
+	ahi   %r6,4\n\
+	ltr   %r7,%r7\n\
+	jne   .L1		# Stop after copying the NULL.\n\
+	# envp copy loop:\n\
+.L2:	l     %r7,0(%r5)	# Load a word from the source.\n\
+	st    %r7,0(%r6)	# Store the word in the destination.\n\
+	ahi   %r5,4\n\
+	ahi   %r6,4\n\
+	ltr   %r7,%r7\n\
+	jne   .L2		# Stop after copying the NULL.\n\
+	# Now we have to zero out the envp entries after NULL to allow\n\
+	# start.S to properly find auxv by skipping zeroes.\n\
+	# zero out loop:\n\
+	lhi   %r7,0\n\
+.L3:	st    %r7,0(%r6)	# Store zero.\n\
+	ahi   %r6,4		# Advance dest pointer.\n\
+	ahi   %r1,-1		# Subtract one from the word count.\n\
+	ltr   %r1,%r1\n\
+	jne    .L3		# Keep copying if the word count is non-zero.\n\
+	# Adjust _dl_argv\n\
+	la    %r6,100(%r15)\n\
+	l     %r1,_dl_argv@GOT(%r12)\n\
+	st    %r6,0(%r1)\n\
 	# The special initializer gets called with the stack just\n\
 	# as the application's entry point will see it; it can\n\
 	# switch stacks if it moves these contents over.\n\
@@ -185,7 +216,7 @@ _dl_start_user:\n\
 	# Call the function to run the initializers.\n\
 	# Load the parameters:\n\
 	# (%r2, %r3, %r4, %r5) = (_dl_loaded, argc, argv, envp)\n\
-	l     %r2,_rtld_local@GOT(%r12)\n\
+.L4:	l     %r2,_rtld_local@GOT(%r12)\n\
 	l     %r2,0(%r2)\n\
 	l     %r3,96(%r15)\n\
 	la    %r4,100(%r15)\n\
diff --git a/sysdeps/s390/s390-32/dl-sysdep.h b/sysdeps/s390/s390-32/dl-sysdep.h
new file mode 100644
index 0000000..b992778
--- /dev/null
+++ b/sysdeps/s390/s390-32/dl-sysdep.h
@@ -0,0 +1,23 @@
+/* System-specific settings for dynamic linker code.  S/390 version.
+   Copyright (C) 2014 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include_next <dl-sysdep.h>
+
+/* _dl_argv cannot be attribute_relro, because _dl_start_user
+   might write into it after _dl_start returns.  */
+#define DL_ARGV_NOT_RELRO 1

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: Misaligned stack on 32-bit s390?

[Andreas Krebbel]

On 11/12/2014 05:00 AM, Carlos O'Donell wrote:
> On 11/11/2014 09:54 AM, Andreas Krebbel wrote:
>> Would you like to continue working on it or should we try to take
>> over? (Stefan or myself)
> 
> We can get it done. It would be great to have you review the final
> patch.
> 
> Please note that the patch I posted is incomplete, it fails to
> readjust _dl_argv which is cached by the loader and needs to be
> changed if argv is moved. Simple fix though.
> 
> I also don't know how long s390 lasted without this breaking
> something. I guess aligned stacks don't really matter all that
> much ;-)

The reason probably is that the broken alignment never reaches the executable. In _start.S we
correct the alignment again. So it really is only a problem for code executed between the argv
adjustments done by ld.so and the entry point of the executable.

-Andreas-

> 
> Cheers,
> Carlos.
> 

Re: Misaligned stack on 32-bit s390?

[Carlos O'Donell]

On 11/12/2014 02:09 AM, Siddhesh Poyarekar wrote:
> On Tue, Nov 11, 2014 at 11:00:43PM -0500, Carlos O'Donell wrote:
>> Please note that the patch I posted is incomplete, it fails to
>> readjust _dl_argv which is cached by the loader and needs to be
>> changed if argv is moved. Simple fix though.
> 
> Here's the updated patch that adjusts _dl_argv.  As a result now,
> _dl_argv is no longer relro on s390.  Tested to verify that there are
> no new failures on s390.
> 
> OK to commit?

Yes.

The dlfcn/default test case is good enough to catch future
failures with _dl_argv so I'm happy there. I don't think it's all
that useful to write tests to check for stack alignment at each
point along the startup.

Andreas Krebbel ACK'd the original patch so I think this is good
to go in. The only change in this v2 patch is the adjustment of
_dl_argv which looks correct to me.

Please check this in.

Cheers,
Carlos.

Re: Misaligned stack on 32-bit s390?

[Stefan Liebler]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

Hi,

i have built glibc from scratch with the patch and get the following 
error while compiling rtld.c:
/tmp/cc3Ata9V.s: Assembler messages:
/tmp/cc3Ata9V.s:111: Error: symbol `.L3' is already defined
/tmp/cc3Ata9V.s:124: Error: symbol `.L4' is already defined

The compiler generated these labels in function 
_dl_initial_error_catch_tsd in order to get the address of the variable 
data via literal pool and got. A compiler with --with-arch=z9-109 uses 
larl-instruction to get the address and does not generate these labels.

Using numbered labels in the inline-assembly in macro RTLD_START (see 
patch) avoids the label collision and the glibc build succeeds
There is no test-suite regression and the stack is adjusted.
The inline-assembly in s390-64/dl-machine.h does not use any label,
thus there we need no change.

Thanks.
Stefan


---
2014-11-12  Stefan Liebler  <stli@linux.vnet.ibm.com>

	* sysdeps/s390/s390-32/dl-machine.h (RTLD_START):
	Use numbered labels in inline assembly.

[-- Attachment #2: 20141112_s390_32_dlmachine --]
[-- Type: text/plain, Size: 2557 bytes --]

diff --git a/sysdeps/s390/s390-32/dl-machine.h b/sysdeps/s390/s390-32/dl-machine.h
index 47f5874..6780405 100644
--- a/sysdeps/s390/s390-32/dl-machine.h
+++ b/sysdeps/s390/s390-32/dl-machine.h
@@ -148,7 +148,7 @@ elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
 .globl _dl_start_user\n\
 _start:\n\
 	basr  %r13,0\n\
-.L0:    ahi   %r13,.Llit-.L0\n\
+0:      ahi   %r13,.Llit-0b\n\
 	lr    %r2,%r15\n\
 	# Alloc stack frame\n\
 	ahi   %r15,-96\n\
@@ -168,7 +168,7 @@ _dl_start_user:\n\
 	l     %r1,_dl_skip_args@GOT(%r12)\n\
 	l     %r1,0(%r1)	# load _dl_skip_args\n\
 	ltr   %r1,%r1\n\
-	je    .L4		# Skip the arg adjustment if there were none.\n\
+	je    4f		# Skip the arg adjustment if there were none.\n\
 	# Get the original argument count.\n\
 	l     %r0,96(%r15)\n\
 	# Subtract _dl_skip_args from it.\n\
@@ -183,28 +183,28 @@ _dl_start_user:\n\
 	sll   %r0,2\n		# Number of skipped bytes.\n\
 	ar    %r5,%r0		# Source pointer = Dest + Skipped args.\n\
 	# argv copy loop:\n\
-.L1:	l     %r7,0(%r5)	# Load a word from the source.\n\
+1:	l     %r7,0(%r5)	# Load a word from the source.\n\
 	st    %r7,0(%r6)	# Store the word in the destination.\n\
 	ahi   %r5,4\n\
 	ahi   %r6,4\n\
 	ltr   %r7,%r7\n\
-	jne   .L1		# Stop after copying the NULL.\n\
+	jne   1b		# Stop after copying the NULL.\n\
 	# envp copy loop:\n\
-.L2:	l     %r7,0(%r5)	# Load a word from the source.\n\
+2:	l     %r7,0(%r5)	# Load a word from the source.\n\
 	st    %r7,0(%r6)	# Store the word in the destination.\n\
 	ahi   %r5,4\n\
 	ahi   %r6,4\n\
 	ltr   %r7,%r7\n\
-	jne   .L2		# Stop after copying the NULL.\n\
+	jne   2b		# Stop after copying the NULL.\n\
 	# Now we have to zero out the envp entries after NULL to allow\n\
 	# start.S to properly find auxv by skipping zeroes.\n\
 	# zero out loop:\n\
 	lhi   %r7,0\n\
-.L3:	st    %r7,0(%r6)	# Store zero.\n\
+3:	st    %r7,0(%r6)	# Store zero.\n\
 	ahi   %r6,4		# Advance dest pointer.\n\
 	ahi   %r1,-1		# Subtract one from the word count.\n\
 	ltr   %r1,%r1\n\
-	jne    .L3		# Keep copying if the word count is non-zero.\n\
+	jne    3b		# Keep copying if the word count is non-zero.\n\
 	# Adjust _dl_argv\n\
 	la    %r6,100(%r15)\n\
 	l     %r1,_dl_argv@GOT(%r12)\n\
@@ -216,7 +216,7 @@ _dl_start_user:\n\
 	# Call the function to run the initializers.\n\
 	# Load the parameters:\n\
 	# (%r2, %r3, %r4, %r5) = (_dl_loaded, argc, argv, envp)\n\
-.L4:	l     %r2,_rtld_local@GOT(%r12)\n\
+4:	l     %r2,_rtld_local@GOT(%r12)\n\
 	l     %r2,0(%r2)\n\
 	l     %r3,96(%r15)\n\
 	la    %r4,100(%r15)\n\

Re: Misaligned stack on 32-bit s390?

[Rich Felker]

On Wed, Nov 12, 2014 at 10:05:45AM +0100, Andreas Krebbel wrote:
> On 11/12/2014 05:00 AM, Carlos O'Donell wrote:
> > On 11/11/2014 09:54 AM, Andreas Krebbel wrote:
> >> Would you like to continue working on it or should we try to take
> >> over? (Stefan or myself)
> > 
> > We can get it done. It would be great to have you review the final
> > patch.
> > 
> > Please note that the patch I posted is incomplete, it fails to
> > readjust _dl_argv which is cached by the loader and needs to be
> > changed if argv is moved. Simple fix though.
> > 
> > I also don't know how long s390 lasted without this breaking
> > something. I guess aligned stacks don't really matter all that
> > much ;-)
> 
> The reason probably is that the broken alignment never reaches the executable. In _start.S we
> correct the alignment again. So it really is only a problem for code executed between the argv
> adjustments done by ld.so and the entry point of the executable.

How/why does any code get executed in that interval anyway?

Rich

Re: Misaligned stack on 32-bit s390?

[Andreas Schwab]

Rich Felker <dalias@libc.org> writes:

> On Wed, Nov 12, 2014 at 10:05:45AM +0100, Andreas Krebbel wrote:
>> The reason probably is that the broken alignment never reaches the executable. In _start.S we
>> correct the alignment again. So it really is only a problem for code executed between the argv
>> adjustments done by ld.so and the entry point of the executable.
>
> How/why does any code get executed in that interval anyway?

It's the dynamic linker itself and the init code.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Misaligned stack on 32-bit s390?

[Siddhesh Poyarekar]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

On Wed, Nov 12, 2014 at 04:41:58PM +0100, Stefan Liebler wrote:
> Hi,
> 
> i have built glibc from scratch with the patch and get the following error
> while compiling rtld.c:
> /tmp/cc3Ata9V.s: Assembler messages:
> /tmp/cc3Ata9V.s:111: Error: symbol `.L3' is already defined
> /tmp/cc3Ata9V.s:124: Error: symbol `.L4' is already defined
> 
> The compiler generated these labels in function _dl_initial_error_catch_tsd
> in order to get the address of the variable data via literal pool and got. A
> compiler with --with-arch=z9-109 uses larl-instruction to get the address
> and does not generate these labels.
> 
> Using numbered labels in the inline-assembly in macro RTLD_START (see patch)
> avoids the label collision and the glibc build succeeds
> There is no test-suite regression and the stack is adjusted.
> The inline-assembly in s390-64/dl-machine.h does not use any label,
> thus there we need no change.
> 
> Thanks.
> Stefan
> 
> 
> ---
> 2014-11-12  Stefan Liebler  <stli@linux.vnet.ibm.com>
> 
> 	* sysdeps/s390/s390-32/dl-machine.h (RTLD_START):
> 	Use numbered labels in inline assembly.

Thanks, this looks good to me.  Technically this should wait for
machine maintainer approval, but IMO it would result in identical code
and if you can somehow prove that, you'd be good to go.

Siddhesh

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: Misaligned stack on 32-bit s390?

[Andreas Krebbel]

On 11/12/2014 04:41 PM, Stefan Liebler wrote:
> 2014-11-12  Stefan Liebler  <stli@linux.vnet.ibm.com>
> 
> 	* sysdeps/s390/s390-32/dl-machine.h (RTLD_START):
> 	Use numbered labels in inline assembly.
> 

Applied. Thanks!