Message ID | b3124860-b279-df32-20b0-3cbc2516a132@linaro.org |
---|---|
State | New |
Headers | show |
On Thu, Mar 09, 2017 at 05:30:03PM +0100, Adhemerval Zanella wrote: > > Regarding mlock-syscall: > > If the compat mlock syscall is used, it returns 12 (ENOMEM). > > This is also observable if you compile and run the testcase with -m32 on a x86_64 system. > > > > > > I've compiled and run posix/test-errno.c on my s390x system and > > get the following error: > > FAIL: setsockopt: errno is: 22 (Invalid argument) expected: 9 (Bad file descriptor) > > > > sl=0xfdfa9170 before setsockopt syscall. > > The test succeeds if I sl is initialized to zero. > > > > POSIX [1] states that mlock should may fail with EINVAL only if > the addr argument is not a multiple of {PAGESIZE}. Linux does > not return this issue, since it aligns the resulting address > to pagesize: > > * mm/mlock.c: > > 666 static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t flags) > 667 { > [...] > 676 > 677 len = PAGE_ALIGN(len + (offset_in_page(start))); > 678 start &= PAGE_MASK; > > EINVAL is only returned on 'apply_vma_lock_flags': > > 578 static int apply_vma_lock_flags(unsigned long start, size_t len, > 579 vm_flags_t flags) > 580 { > [...] > 587 end = start + len; > 588 if (end < start) > 589 return -EINVAL; > > But if you runs 32 binaries on 64 bits kernel overflow won't happen. > It is documented in man pages, so I think from kernel side it should > be consistent for 32 bit binaries on 64 bit kernel. I also observed this fail in aarch64/ilp32, but I decided that this is the ilp32 problem while it's compat one, as you discovered here. The test is not intended to check each and every syscall, as it is mentioned in comment. For example, all syscalls that may fail with EFAULT are not tested because they may both return the EFAULT or actually terminate the application with segfault. This is the similar case for me. I think that it's better to keep the test simple, and just drop the mlock test, and not introduce another macro. Though, the patch that handles the issue is already there, and I don't object if you submit it. Yury
On 09/03/2017 19:29, Yury Norov wrote: > On Thu, Mar 09, 2017 at 05:30:03PM +0100, Adhemerval Zanella wrote: >>> Regarding mlock-syscall: >>> If the compat mlock syscall is used, it returns 12 (ENOMEM). >>> This is also observable if you compile and run the testcase with -m32 on a x86_64 system. >>> >>> >>> I've compiled and run posix/test-errno.c on my s390x system and >>> get the following error: >>> FAIL: setsockopt: errno is: 22 (Invalid argument) expected: 9 (Bad file descriptor) >>> >>> sl=0xfdfa9170 before setsockopt syscall. >>> The test succeeds if I sl is initialized to zero. >>> >> >> POSIX [1] states that mlock should may fail with EINVAL only if >> the addr argument is not a multiple of {PAGESIZE}. Linux does >> not return this issue, since it aligns the resulting address >> to pagesize: >> >> * mm/mlock.c: >> >> 666 static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t flags) >> 667 { >> [...] >> 676 >> 677 len = PAGE_ALIGN(len + (offset_in_page(start))); >> 678 start &= PAGE_MASK; >> >> EINVAL is only returned on 'apply_vma_lock_flags': >> >> 578 static int apply_vma_lock_flags(unsigned long start, size_t len, >> 579 vm_flags_t flags) >> 580 { >> [...] >> 587 end = start + len; >> 588 if (end < start) >> 589 return -EINVAL; >> >> But if you runs 32 binaries on 64 bits kernel overflow won't happen. >> It is documented in man pages, so I think from kernel side it should >> be consistent for 32 bit binaries on 64 bit kernel. > > I also observed this fail in aarch64/ilp32, but I decided that this > is the ilp32 problem while it's compat one, as you discovered here. > > The test is not intended to check each and every syscall, as it is > mentioned in comment. For example, all syscalls that may fail with > EFAULT are not tested because they may both return the EFAULT or > actually terminate the application with segfault. This is the > similar case for me. I think that it's better to keep the test simple, > and just drop the mlock test, and not introduce another macro. > > Though, the patch that handles the issue is already there, and I don't > object if you submit it. I do not have a strong opinion here, but since adjusting to check for both results is pretty much straightforward I think it is worth to just add it and comment why it requires to check for both results. I will send a patch upstream shortly.
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index 6b7aa3f..1872cdb 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -43,7 +43,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \ bits/mman-linux.h tests += tst-clone tst-clone2 tst-fanotify tst-personality tst-quota \ - tst-sync_file_range test-errno + tst-sync_file_range test-errno-linux # Generate the list of SYS_* macros for the system calls (__NR_* macros). diff --git a/sysdeps/unix/sysv/linux/test-errno-linux.c b/sysdeps/unix/sysv/linux/test-errno-linux.c index ab3735f..c3facd5 100644 --- a/sysdeps/unix/sysv/linux/test-errno-linux.c +++ b/sysdeps/unix/sysv/linux/test-errno-linux.c @@ -1,4 +1,5 @@ /* Test that failing system calls do set errno to the correct value. + Linux sycalls version. Copyright (C) 2017 Free Software Foundation, Inc. This file is part of the GNU C Library. @@ -90,9 +91,37 @@ fail; \ })) +#define test_wrp_rv2(rtype, prtype, experr1, experr2, syscall, ...) \ + (__extension__ ({ \ + errno = 0xdead; \ + rtype ret = syscall (__VA_ARGS__); \ + int err = errno; \ + int fail; \ + if (ret == (rtype) -1 && (err == experr1 || err == experr2)) \ + fail = 0; \ + else \ + { \ + fail = 1; \ + if (ret != (rtype) -1) \ + printf ("FAIL: " #syscall ": didn't fail as expected" \ + " (return "prtype")\n", ret); \ + else if (err == 0xdead) \ + puts("FAIL: " #syscall ": didn't update errno\n"); \ + else if (err != experr1 && err != experr2) \ + printf ("FAIL: " #syscall \ + ": errno is: %d (%s) expected: %d (%s) or %d (%s)\n", \ + err, strerror (err), experr1, strerror (experr1), \ + experr2, strerror (experr2)); \ + } \ + fail; \ + })) + #define test_wrp(experr, syscall, ...) \ test_wrp_rv(int, "%d", experr, syscall, __VA_ARGS__) +#define test_wrp2(experr1, experr2, syscall, ...) \ + test_wrp_rv2(int, "%d", experr1, experr2, syscall, __VA_ARGS__) + static int do_test (void) { @@ -120,7 +149,12 @@ do_test (void) fails |= test_wrp (ESRCH, getpgid, -1); fails |= test_wrp (EINVAL, inotify_add_watch, -1, "/", 0); fails |= test_wrp (EINVAL, mincore, (void *) -1, 0, vec); - fails |= test_wrp (EINVAL, mlock, (void *) -1, 1); // different errors + /* mlock fails if the result of the addition addr+len was less than addr + which indicates final address overflow), however on 32 bits binaries + running on 64 bits kernel, internal syscall address check won't result + in an invalid address and thus syscalls fails later in vma + allocation. */ + fails |= test_wrp2 (EINVAL, ENOMEM, mlock, (void *) -1, 1); fails |= test_wrp (EINVAL, nanosleep, &ts, &ts); fails |= test_wrp (EINVAL, poll, &pollfd, -1, 0); fails |= test_wrp (ENODEV, quotactl, Q_GETINFO, NULL, -1, (caddr_t) &dqblk);