aarch64: fix errno address calculation in SYSCALL_ERROR_HANDLER()

  This patch fixes the last regression in LTP lite scenario (mmap16) comparing
to lp64 in my source trees [1, 2]. The fix has been suggested back in 2015 [3]
but was never applied, so I reinvented the weel while debugging mmap16.

[1] https://github.com/norov/glibc/tree/dev9
[2] https://github.com/norov/linux/tree/ilp32-20170203
[3] https://sourceware.org/ml/libc-alpha/2015-03/msg00587.html

	* sysdeps/unix/sysv/linux/aarch64/sysdep.h: use PTR_REG() for offset
	calculation in SYSCALL_ERROR_HANDLER().

---
 sysdeps/unix/sysv/linux/aarch64/sysdep.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

If it is an user visible issue it requires a bugzilla report (although I am not
sure in this situation since I think this issue should arise only for ilp32).

Also, errno for mmap in particular should be covered on 'posix/tst-mmap.c'.
When/how exactly this issues is triggered? It could be a good thing to have
at least a regression check or increase coverage in an existing test.

On 07/02/2017 09:48, Yury Norov wrote:
> This patch fixes the last regression in LTP lite scenario (mmap16) comparing
> to lp64 in my source trees [1, 2]. The fix has been suggested back in 2015 [3]
> but was never applied, so I reinvented the weel while debugging mmap16.
> 
> [1] https://github.com/norov/glibc/tree/dev9
> [2] https://github.com/norov/linux/tree/ilp32-20170203
> [3] https://sourceware.org/ml/libc-alpha/2015-03/msg00587.html
> 
> 	* sysdeps/unix/sysv/linux/aarch64/sysdep.h: use PTR_REG() for offset
> 	calculation in SYSCALL_ERROR_HANDLER().
> 
> ---
>  sysdeps/unix/sysv/linux/aarch64/sysdep.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> index 1ffabc2..d926e19 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> @@ -108,7 +108,7 @@
>  .Lsyscall_error:						\
>  	adrp	x1, :gottprel:errno;				\
>  	neg	w2, w0;						\
> -	ldr	x1, [x1, :gottprel_lo12:errno];			\
> +	ldr	PTR_REG (1), [x1, :gottprel_lo12:errno];	\
>  	mrs	x3, tpidr_el0;					\
>  	mov	x0, -1;						\
>  	str	w2, [x1, x3];					\
>
  

On Tue, Feb 07, 2017 at 10:10:42AM -0200, Adhemerval Zanella wrote:
> If it is an user visible issue it requires a bugzilla report (although I am not
> sure in this situation since I think this issue should arise only for ilp32).

This is not a user-visible bug because arm64/ilp32 is not a user
visible feature at now. If you think that I should create a bug, I can do it.

> Also, errno for mmap in particular should be covered on 'posix/tst-mmap.c'.
> When/how exactly this issues is triggered? It could be a good thing to have
> at least a regression check or increase coverage in an existing test.

This is not mmap bug. This is SYSCALL_ERROR_HANDLER() bug that triggered in
the mmap16 test. The macro is used in generation of wrappers for pseudo
syscalls.  Bug is triggered for syscall sys_write(). Kernel returns ENOSPC
for it, as intended, and triggers the invocation of SYSCALL_ERROR_HANDLER().

113                 while (1) {
114                         ret = write(parentfd, buf, FS_BLOCKSIZE);
115                         if (ret < 0) {
116                                 if (errno == ENOSPC) {
117                                         break;
118                                 } else {
119                                         tst_brkm(TBROK | TERRNO, cleanup,
120                                                  "write failed unexpectedly");
121                                 }
122                         }
123                 }


The macro stores error code at the errno address:
#   define SYSCALL_ERROR_HANDLER                                \ 
.Lsyscall_error:                                                \ 
        adrp    x1, :gottprel:errno;                            \ 
        neg     w2, w0;                                         \ 
        ldr     x1, [x1, :gottprel_lo12:errno];                 \ 
        mrs     x3, tpidr_el0;                                  \ 
        mov     x0, -1;                                         \ 
        str     w2, [x1, x3];                                   \ 
        RET;

Assembler translates it to next commands in test mmap16 (with my comments):
   │0xf77a9ba4      adrp   x1, 0xf77bf000
   │0xf77a9ba8      neg    w2, w0                         // 28
   │0xf77a9bac      ldr    x1, [x1,#8048]                 // $x1==0
   │0xf77a9bb0      mrs    x3, tpidr_el0                  // 0xf77ec350
   │0xf77a9bb4      mov    x0, #0xffffffffffffffff        // #-1 
   │0xf77a9bb8      str    w2, [x1,x3]                    // [0xf77ec350] == 28
   │0xf77a9bbc      ret    

The code that retrieves errno is written in C, and can be found in 
errno_location(). For ilp32 it looks like this:
   │0xf77aca10      adrp   x0, 0xf77bf000
   │0xf77aca14      ldr    w0, [x0,#4024]                 // $w0==0x10
   │0xf77aca18      mrs    x7, tpidr_el0                  // $x7==0xf77ec350
   │0xf77aca1c      add    w0, w0, w7                     // $w0==0xf77ec360
   │0xf77aca20      ret

The problem is in the 'ldr    x1, [x1,#8048]' instruction. It
should be 'ldr    w1, [x1,#4024]' for ilp32. The address $x1+8048
becomes valid occasionally, and contains '0'. Error code is therefore
stored at 0xf77ec350 while the proper location for it is 0xf77ec360.
After it errno still contains the code of previous error which is
ENOENT, and test fails.

There's the patch 389d1f1b (Partial ILP32 support for aarch64) in
glibc tree that contains the bunch of fixes of this sort for arm64,
but this one is missed.

If you think there should be the specific test for this case, I can
write it. But I think it should be the test for pseudo syscalls
wrapper, not the part of mmap() or write() test.

Yury
 
> On 07/02/2017 09:48, Yury Norov wrote:
> > This patch fixes the last regression in LTP lite scenario (mmap16) comparing
> > to lp64 in my source trees [1, 2]. The fix has been suggested back in 2015 [3]
> > but was never applied, so I reinvented the weel while debugging mmap16.
> > 
> > [1] https://github.com/norov/glibc/tree/dev9
> > [2] https://github.com/norov/linux/tree/ilp32-20170203
> > [3] https://sourceware.org/ml/libc-alpha/2015-03/msg00587.html
> > 
> > 	* sysdeps/unix/sysv/linux/aarch64/sysdep.h: use PTR_REG() for offset
> > 	calculation in SYSCALL_ERROR_HANDLER().
> > 
> > ---
> >  sysdeps/unix/sysv/linux/aarch64/sysdep.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> > index 1ffabc2..d926e19 100644
> > --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> > +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> > @@ -108,7 +108,7 @@
> >  .Lsyscall_error:						\
> >  	adrp	x1, :gottprel:errno;				\
> >  	neg	w2, w0;						\
> > -	ldr	x1, [x1, :gottprel_lo12:errno];			\
> > +	ldr	PTR_REG (1), [x1, :gottprel_lo12:errno];	\
> >  	mrs	x3, tpidr_el0;					\
> >  	mov	x0, -1;						\
> >  	str	w2, [x1, x3];					\
> >
  

On 07/02/2017 12:54, Yury Norov wrote:
> On Tue, Feb 07, 2017 at 10:10:42AM -0200, Adhemerval Zanella wrote:
>> If it is an user visible issue it requires a bugzilla report (although I am not
>> sure in this situation since I think this issue should arise only for ilp32).
> 
> This is not a user-visible bug because arm64/ilp32 is not a user
> visible feature at now. If you think that I should create a bug, I can do it.

Right, aarch64 lp64 does not trigger so I think we may proceed without
opening a bug report.

> 
>> Also, errno for mmap in particular should be covered on 'posix/tst-mmap.c'.
>> When/how exactly this issues is triggered? It could be a good thing to have
>> at least a regression check or increase coverage in an existing test.
> 
> This is not mmap bug. This is SYSCALL_ERROR_HANDLER() bug that triggered in
> the mmap16 test. The macro is used in generation of wrappers for pseudo
> syscalls.  Bug is triggered for syscall sys_write(). Kernel returns ENOSPC
> for it, as intended, and triggers the invocation of SYSCALL_ERROR_HANDLER().
> 
> 113                 while (1) {
> 114                         ret = write(parentfd, buf, FS_BLOCKSIZE);
> 115                         if (ret < 0) {
> 116                                 if (errno == ENOSPC) {
> 117                                         break;
> 118                                 } else {
> 119                                         tst_brkm(TBROK | TERRNO, cleanup,
> 120                                                  "write failed unexpectedly");
> 121                                 }
> 122                         }
> 123                 }
> 
> 
> The macro stores error code at the errno address:
> #   define SYSCALL_ERROR_HANDLER                                \ 
> .Lsyscall_error:                                                \ 
>         adrp    x1, :gottprel:errno;                            \ 
>         neg     w2, w0;                                         \ 
>         ldr     x1, [x1, :gottprel_lo12:errno];                 \ 
>         mrs     x3, tpidr_el0;                                  \ 
>         mov     x0, -1;                                         \ 
>         str     w2, [x1, x3];                                   \ 
>         RET;
> 
> Assembler translates it to next commands in test mmap16 (with my comments):
>    │0xf77a9ba4      adrp   x1, 0xf77bf000
>    │0xf77a9ba8      neg    w2, w0                         // 28
>    │0xf77a9bac      ldr    x1, [x1,#8048]                 // $x1==0
>    │0xf77a9bb0      mrs    x3, tpidr_el0                  // 0xf77ec350
>    │0xf77a9bb4      mov    x0, #0xffffffffffffffff        // #-1 
>    │0xf77a9bb8      str    w2, [x1,x3]                    // [0xf77ec350] == 28
>    │0xf77a9bbc      ret    
> 
> The code that retrieves errno is written in C, and can be found in 
> errno_location(). For ilp32 it looks like this:
>    │0xf77aca10      adrp   x0, 0xf77bf000
>    │0xf77aca14      ldr    w0, [x0,#4024]                 // $w0==0x10
>    │0xf77aca18      mrs    x7, tpidr_el0                  // $x7==0xf77ec350
>    │0xf77aca1c      add    w0, w0, w7                     // $w0==0xf77ec360
>    │0xf77aca20      ret
> 
> The problem is in the 'ldr    x1, [x1,#8048]' instruction. It
> should be 'ldr    w1, [x1,#4024]' for ilp32. The address $x1+8048
> becomes valid occasionally, and contains '0'. Error code is therefore
> stored at 0xf77ec350 while the proper location for it is 0xf77ec360.
> After it errno still contains the code of previous error which is
> ENOENT, and test fails.
> 
> There's the patch 389d1f1b (Partial ILP32 support for aarch64) in
> glibc tree that contains the bunch of fixes of this sort for arm64,
> but this one is missed.
> 
> If you think there should be the specific test for this case, I can
> write it. But I think it should be the test for pseudo syscalls
> wrapper, not the part of mmap() or write() test.

My only concern is if and why glibc own testsuite did not trigger this
kind of issue in any tests and in this case if it was by chance or lack
of coverage. If glibc own testsuite does not trigger this in any case,
I think a regression should be added.
  

On Tue, Feb 07, 2017 at 02:42:25PM -0200, Adhemerval Zanella wrote:
> 
> 
> On 07/02/2017 12:54, Yury Norov wrote:
> > On Tue, Feb 07, 2017 at 10:10:42AM -0200, Adhemerval Zanella wrote:
> >> If it is an user visible issue it requires a bugzilla report (although I am not
> >> sure in this situation since I think this issue should arise only for ilp32).
> > 
> > This is not a user-visible bug because arm64/ilp32 is not a user
> > visible feature at now. If you think that I should create a bug, I can do it.
> 
> Right, aarch64 lp64 does not trigger so I think we may proceed without
> opening a bug report.

 [...]
 
> > If you think there should be the specific test for this case, I can
> > write it. But I think it should be the test for pseudo syscalls
> > wrapper, not the part of mmap() or write() test.
> 
> My only concern is if and why glibc own testsuite did not trigger this
> kind of issue in any tests and in this case if it was by chance or lack
> of coverage. If glibc own testsuite does not trigger this in any case,
> I think a regression should be added.

OK, I'll write some test tomorrow. (Too late in my place)

Is the patch fine by itself? Can you (someone) push it?

Yury
  

On 07/02/2017 15:06, Yury Norov wrote:
> On Tue, Feb 07, 2017 at 02:42:25PM -0200, Adhemerval Zanella wrote:
>>
>>
>> On 07/02/2017 12:54, Yury Norov wrote:
>>> On Tue, Feb 07, 2017 at 10:10:42AM -0200, Adhemerval Zanella wrote:
>>>> If it is an user visible issue it requires a bugzilla report (although I am not
>>>> sure in this situation since I think this issue should arise only for ilp32).
>>>
>>> This is not a user-visible bug because arm64/ilp32 is not a user
>>> visible feature at now. If you think that I should create a bug, I can do it.
>>
>> Right, aarch64 lp64 does not trigger so I think we may proceed without
>> opening a bug report.
> 
>  [...]
>  
>>> If you think there should be the specific test for this case, I can
>>> write it. But I think it should be the test for pseudo syscalls
>>> wrapper, not the part of mmap() or write() test.
>>
>> My only concern is if and why glibc own testsuite did not trigger this
>> kind of issue in any tests and in this case if it was by chance or lack
>> of coverage. If glibc own testsuite does not trigger this in any case,
>> I think a regression should be added.
> 
> OK, I'll write some test tomorrow. (Too late in my place)
> 
> Is the patch fine by itself? Can you (someone) push it?
> 
> Yury
> 

I will handle it.