This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] aarch64: fix errno address calculation in SYSCALL_ERROR_HANDLER()
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.