Commit Message
On 09/03/2017 16:00, Stefan Liebler wrote:
> On 03/08/2017 05:20 PM, Zack Weinberg wrote:
>> On 03/06/2017 09:42 PM, Yury Norov wrote:
>>> On Mon, Mar 06, 2017 at 05:49:19PM -0300, Wainer dos Santos Moschetta wrote:
>>>> LGTM.
>>> Thanks. I don't have the write access to the glibc repo. Could you
>>> (someone else) apply the patch?
>>
>> I have committed the patch.
>>
>> zw
>>
>>
> Hi,
>
> on s390 (31bit), I get the following fails:
> FAIL: misc/test-errno:
> FAIL: mlock: errno is: 12 (Cannot allocate memory) expected: 22 (Invalid argument)
>
> FAIL: posix/test-errno:
> FAIL: mlock: errno is: 12 (Cannot allocate memory) expected: 22 (Invalid argument)
>
> Is it intended, that the same test is run twice?
It is since although they have the same name they are different test
in fact: posix/test-errno.c only uses POSIX defined interfaces while
sysdeps/unix/sysv/linux/test-errno.c check for Linux specific ones.
> Both are compiled with sysdeps/unix/sysv/linux/test-errno.c.
But this indeed the issue and it need to be fixed. I am not sure if
glibc Makefile system can handle test with same name in multiple
paths (and I personally not compiling to actually debug if it is
the case), so I would recommend to just rename Linux specific one
to test-errno-linux.c.
> Or should there two different tests, one compiled
> with posix/test-errno.c
> and the other with sysdeps/unix/sysv/linux/test-errno.c?
>
> Why is the test-errno added to tests in sysdeps/unix/sysv/linux/Makefile with:
> ifeq ($(subdir),misc)
> tests += test-errno
> endif
Afaik it is basically to put the resulting objects/binaries on misc folder
(where usually Linux-only tests are placed).
>
>
>
> 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.
For glibc side, I think we should do something like:
[1] http://pubs.opengroup.org/onlinepubs/9699919799/
> Bye
> Stefan
>
Comments
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.
@@ -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).
@@ -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);