Message ID | 1486468092-20986-1-git-send-email-ynorov@caviumnetworks.com |
---|---|
State | New |
Headers | show |
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.
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]; \