2023-07-07 15:09:06

by Zhangjin Wu

[permalink] [raw]
Subject: [PATCH v6 13/15] selftests/nolibc: add mmap_bad test case

The length argument of mmap() must be greater than 0, passing a zero
length argument expects failure with -EINVAL.

Signed-off-by: Zhangjin Wu <[email protected]>
---
tools/testing/selftests/nolibc/nolibc-test.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index fde1b3c51a4a..4026772ed4c5 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -669,6 +669,7 @@ int run_syscall(int min, int max)
CASE_TEST(lseek_m1); EXPECT_SYSER(1, lseek(-1, 0, SEEK_SET), -1, EBADF); break;
CASE_TEST(lseek_0); EXPECT_SYSER(1, lseek(0, 0, SEEK_SET), -1, ESPIPE); break;
CASE_TEST(mkdir_root); EXPECT_SYSER(1, mkdir("/", 0755), -1, EEXIST); break;
+ CASE_TEST(mmap_bad); EXPECT_PTRER(1, mmap(NULL, 0, PROT_READ, MAP_PRIVATE, 0, 0), MAP_FAILED, EINVAL); break;
CASE_TEST(open_tty); EXPECT_SYSNE(1, tmp = open("/dev/null", 0), -1); if (tmp != -1) close(tmp); break;
CASE_TEST(open_blah); EXPECT_SYSER(1, tmp = open("/proc/self/blah", 0), -1, ENOENT); if (tmp != -1) close(tmp); break;
CASE_TEST(poll_null); EXPECT_SYSZR(1, poll(NULL, 0, 0)); break;
--
2.25.1



2023-07-09 10:17:13

by Willy Tarreau

[permalink] [raw]
Subject: Re: [PATCH v6 13/15] selftests/nolibc: add mmap_bad test case

On Fri, Jul 07, 2023 at 11:05:49PM +0800, Zhangjin Wu wrote:
> The length argument of mmap() must be greater than 0, passing a zero
> length argument expects failure with -EINVAL.

This one doesn't work for me on x86_64 kernel 5.15.112, qemu userland:

46 mmap_bad = <0x0> EEXIST != (<0xffffffffffffffff> EINVAL) [FAIL]

This EEXIST actually is the errno from the previous test. If I run
the test natively it's OK:

$ ./nolibc-test syscall:46
Running test 'syscall'
46 mmap_bad = <0xffffffffffffffff> EINVAL [OK]
Errors during this test: 0

I'll queue it anyway for now but it would be nice that we figure what's
happening (even if we need to adjust or drop the test if it's a false
positive) so that we don't get used to "ah this is a normal error".

Willy

2023-07-09 18:53:41

by Zhangjin Wu

[permalink] [raw]
Subject: Re: [PATCH v6 13/15] selftests/nolibc: add mmap_bad test case

Hi, Willy

> On Fri, Jul 07, 2023 at 11:05:49PM +0800, Zhangjin Wu wrote:
> > The length argument of mmap() must be greater than 0, passing a zero
> > length argument expects failure with -EINVAL.
>
> This one doesn't work for me on x86_64 kernel 5.15.112, qemu userland:
>
> 46 mmap_bad = <0x0> EEXIST != (<0xffffffffffffffff> EINVAL) [FAIL]
>

Just rerun 'run-user' on x86_64 with kernel 5.11.0-41-generic, it is ok.

The failure is very interesting, and also rechecked the kernel mmap code:

unsigned long do_mmap(struct file *file, unsigned long addr,
unsigned long len, unsigned long prot,
unsigned long flags, unsigned long pgoff,
unsigned long *populate, struct list_head *uf)
{
...
if (!len)
return -EINVAL;
...
}

$ git blame -L 1202,1202 mm/mmap.c
e37609bb36f70 (Piotr Kwapulinski 2015-06-24 16:58:39 -0700 1202) if (!len)

$ git show e37609bb36f706c954e82e580e2e790e9d5caef8:Makefile
VERSION = 4
PATCHLEVEL = 1
SUBLEVEL = 0
EXTRAVERSION =

So, the kernel side should be ok from v4.1?

For qemu-user, I have rechecked the following version:

$ qemu-x86_64 --version
qemu-x86_64 version 4.2.1 (Debian 1:4.2-3ubuntu6.18)

$ qemu-x86_64 --version
qemu-x86_64 version 7.0.0 (Debian 1:7.0+dfsg-7ubuntu2.6~backport20.04.202306190332~ubuntu20.04.1)

$ build/x86_64/pc/qemu/v8.0.2/qemu-x86_64 --version
qemu-x86_64 version 8.0.2 (v8.0.2-dirty)

all of them work well, as a comparison, what's your qemu-user version?

> This EEXIST actually is the errno from the previous test. If I run
> the test natively it's OK:
>
> $ ./nolibc-test syscall:46
> Running test 'syscall'
> 46 mmap_bad = <0xffffffffffffffff> EINVAL [OK]
> Errors during this test: 0
>
> I'll queue it anyway for now but it would be nice that we figure what's
> happening (even if we need to adjust or drop the test if it's a false
> positive) so that we don't get used to "ah this is a normal error".
>

Yes, if there is a failure, we should figure out why. It is ok for me to remove
this one or let's find another errno condition before we find the root cause of
the reported failure.

Thanks,
Zhangjin

> Willy

2023-07-10 07:27:39

by Willy Tarreau

[permalink] [raw]
Subject: Re: [PATCH v6 13/15] selftests/nolibc: add mmap_bad test case

Hi Zhangjin,

On Mon, Jul 10, 2023 at 02:33:22AM +0800, Zhangjin Wu wrote:
> For qemu-user, I have rechecked the following version:
>
> $ qemu-x86_64 --version
> qemu-x86_64 version 4.2.1 (Debian 1:4.2-3ubuntu6.18)
>
> $ qemu-x86_64 --version
> qemu-x86_64 version 7.0.0 (Debian 1:7.0+dfsg-7ubuntu2.6~backport20.04.202306190332~ubuntu20.04.1)
>
> $ build/x86_64/pc/qemu/v8.0.2/qemu-x86_64 --version
> qemu-x86_64 version 8.0.2 (v8.0.2-dirty)
>
> all of them work well, as a comparison, what's your qemu-user version?

Spot on! I ran it remotely and had a different qemu in my default path:

$ qemu-x86_64 --version
qemu-x86_64 version 2.7.0, Copyright (c) 2003-2016 Fabrice Bellard and the QEMU Project developers

I didn't notice but it yells at me during the test:

qemu: Unsupported syscall: 332

Locally with this version it's much cleaner:

$ qemu-x86_64 --version
qemu-x86_64 version 6.2.0
Copyright (c) 2003-2021 Fabrice Bellard and the QEMU Project developers
$ make run-user | grep status:
143 test(s): 141 passed, 2 skipped, 0 failed => status: warning

So we can keep it and blame my environment for that failure.

Thanks for checking,
Willy