Bug 3782 - LKFT: arm32: kselftest proc selftests: proc-self-map-files-002 failed
Summary: LKFT: arm32: kselftest proc selftests: proc-self-map-files-002 failed
Status: VERIFIED FIXED
Alias: None
Product: Kernel Functional Testing
Classification: Unclassified
Component: kselftest (show other bugs)
Version: linux-next
Hardware: Other Linux
: --- major
Target Milestone: ---
Assignee: Rafael David Tinoco
URL:
Keywords: LKFT
Depends on:
Blocks:
 
Reported: 2018-05-02 11:41 UTC by Naresh Kamboju
Modified: 2018-12-14 12:33 UTC (History)
5 users (show)

See Also:
Linux kernel version:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Naresh Kamboju 2018-05-02 11:41:56 UTC
Kselftest proc test case proc-self-map-files-002 failed on arm32 running mainline kernel.

selftests: proc-self-map-files-002 [FAIL]

history showing test failed on arm32 x15 and qemu_arm32,
https://qa-reports.linaro.org/lkft/linux-mainline-oe/tests/kselftest/proc-self-map-files-002

metadata:

  
  git branch: master
  git repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
  git commit: 2d618bdf71635463a4aa4ad0fe46ec852292bc0c
  git describe: v4.17-rc3-13-g2d618bdf7163
  make_kernelversion: 4.17.0-rc3
  kernel-config: http://snapshots.linaro.org/openembedded/lkft/morty/am57xx-evm/rpb/linux-mainline/836/config
  kernel-defconfig: http://snapshots.linaro.org/openembedded/lkft/morty/am57xx-evm/rpb/linux-mainline/836/defconfig
  build-url: https://ci.linaro.org/job/openembedded-lkft-linux-mainline/DISTRO=rpb,MACHINE=am57xx-evm,label=docker-stretch-amd64/836/
  build-location: http://snapshots.linaro.org/openembedded/lkft/morty/am57xx-evm/rpb/linux-mainline/836
  toolchain: arm-linaro-linux-gnueabi linaro-6.2
  series: lkft

  kselftest__url: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
  kselftest__version: 4.16+gitAUTOINC+2d618bdf71
  kselftest__revision: 2d618bdf71635463a4aa4ad0fe46ec852292bc0c
Comment 1 Naresh Kamboju 2018-05-02 11:46:30 UTC
LAVA job id:
https://lkft.validation.linaro.org/scheduler/job/203026#L3413
Comment 3 Naresh Kamboju 2018-06-18 17:11:12 UTC
FAIL on arm32 for 4.17 and above.
Fails on All devices for older kernel.
FAIL on 4.16 to 4.4
https://qa-reports.linaro.org/lkft/linux-stable-rc-4.16-oe/tests/kselftest/proc-self-map-files-002
<>
https://qa-reports.linaro.org/lkft/linux-stable-rc-4.4-oe/tests/kselftest/proc-self-map-files-002
Comment 4 Dan Rue 2018-10-09 15:22:30 UTC
Related to bug 3783
Comment 5 Rafael David Tinoco 2018-10-10 20:18:18 UTC
proc-self-map-files-002 is failing in the following line:

p = mmap(NULL, PAGE_SIZE, PROT_NONE, MAP_PRIVATE|MAP_FILE|MAP_FIXED, fd, 0);

with EINVAL.

Since:

void *mmap(void *addr, size_t length, int prot, int flags, int fd, off_t offset);

where:

DESCRIPTION
       mmap()  creates  a  new  mapping in the virtual address space of the calling process.
       The starting address for the new mapping is specified in addr.  The  length  argument
       specifies the length of the mapping (which must be greater than 0).

       If  addr is NULL, then the kernel chooses the (page-aligned) address at which to cre‐
       ate the mapping; this is the most portable method of creating a new mapping.  If addr
       is  not NULL, then the kernel takes it as a hint about where to place the mapping; on
       Linux, the mapping will be created at a nearby page boundary.  The address of the new
       mapping is returned as the result of the call.

Looks like the NULL argument for addr pointer (addr = task virtual memory to contain the mapping for the asked mapped area) will make addr to be at a page boundary (begin). On arm32 systems I'm getting: 0xb6f95000 for NULL without MAP_FIXED, and... 0xffffffff with MAP_FIXED flag and NULL as addr hinter. Since 0xffffffff is 4GB limit, I get EINVAL from this call.

Replacing the same call for:

p = mmap((void *) (0+(1*PAGE_SIZE)), PAGE_SIZE, PROT_NONE, MAP_SHARED|MAP_FILE|MAP_FIXED, fd, 0);

I still get 0xffffffff as pointer and EINVAL as errno.

If I try to map something something after the (2*PAGESIZE) range, mmap works:

p = mmap((void *) (0+(2*PAGE_SIZE)), PAGE_SIZE, PROT_NONE, MAP_SHARED|MAP_FILE|MAP_FIXED, fd, 0);

I have to figure out if this is expected (likely not) or a BUG and report to ARM kernel mailing list. If it is expected, by changing the test I'll make it to work for arm32, but, if not, than we have a BUG in mmap() for arm 32bits when using NULL as addr and MAP_FIXED as a flag.
Comment 6 Rafael David Tinoco 2018-10-11 01:24:25 UTC
Okay, so pointer was just uninitialized pointer. 

The logic causing the error is this:

do_mmap() -> addr = get_unmmaped_area()

get_unmapped_area() might be:
 - if mmap is legacy = arch_get_unmapped_area() -> works
 - else = arch_get_unmapped_area_topdown() -> does not work:

arch_get_unmapped_area_topdown() from arch/arm/mm/mmap.c:

...
	if (flags & MAP_FIXED) {
		if (aliasing && flags & MAP_SHARED &&
		    (addr - (pgoff << PAGE_SHIFT)) & (SHMLBA - 1))
			return -EINVAL;
		return addr;
	}
...

Looks like the get_unmapped_area_topdown() disallow us to get a virtual address using such flags.

TODO:

Is this mandatory for the arch arm 32 bits ? If it is, then I'll have to change "addr" pointer for the test only, and that will fix this issue.
Comment 7 Rafael David Tinoco 2018-10-11 17:34:41 UTC
By creating:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

int
main(int argc, char **argv)
{
    const unsigned int PAGE_SIZE = sysconf(_SC_PAGESIZE);
    void *ptr;
    unsigned int i;
    int fd;

    fd = open("/dev/zero", O_RDONLY);
    if (fd == -1)
        perror("open"), exit(1);

    for (i=4096; i < (0xffffffff - 4096); i+=PAGE_SIZE) {

    ptr = mmap((void *) i, 1024, PROT_NONE, MAP_FILE|MAP_SHARED|MAP_FIXED, fd, 0);

        if (ptr == MAP_FAILED) {

            perror("mmap");
            printf("i=%u, page=%u, addr=%x (map failed), round=%d\n",
                                    i, i/PAGE_SIZE, ptr, (int) ptr%PAGE_SIZE);

        } else {

            printf("i=%u, page=%u, addr=%x (map okay), round=%d\n",
                                    i, i/PAGE_SIZE, ptr, (int) ptr%PAGE_SIZE);
            munmap(ptr, 1024);

        }

        return 0;

    }

    return 0;
}

I was able to see, indeed, that we can't map anything to first 2 * PAGE_SIZE addresses of a userland task. I have confirmed that by checking:

/*
 * This is the lowest virtual address we can permit any user space
 * mapping to be mapped at.  This is particularly important for
 * non-high vector CPUs.
 */
#define FIRST_USER_ADDRESS	(PAGE_SIZE * 2)

in file: arch/arm/include/asm/pgtable.h

With that, the solution here is to change selftest to map an anonymous buffer, with MAP_FIXED, to an address above 8192. Will suggest a patch upstream.
Comment 8 Rafael David Tinoco 2018-10-11 18:46:13 UTC
I have just submitted the patch to upstream:

https://lkml.org/lkml/2018/10/11/1106

PATCH:

From f2ba529b20e7775d3ea30ec2047e829fa249d0e4 Mon Sep 17 00:00:00 2001
From: Rafael David Tinoco <rafael.tinoco@linaro.org>
Date: Thu, 11 Oct 2018 15:33:14 -0300
Subject: [PATCH] proc: fix proc-self-map-files selftest for arm

MAP_FIXED is important for this test but, unfortunately, lowest virtual
address for user space mapping on arm is (PAGE_SIZE * 2) and NULL hint
does not seem to guarantee that when MAP_FIXED is given. This patch sets
the virtual address that will hold the mapping for the test, fixing the
issue.

Link: https://bugs.linaro.org/show_bug.cgi?id=3782
Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
---
 tools/testing/selftests/proc/proc-self-map-files-002.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/proc/proc-self-map-files-002.c b/tools/testing/selftests/proc/proc-self-map-files-002.c
index 6f1f4a6e1ecb..0a47eaca732a 100644
--- a/tools/testing/selftests/proc/proc-self-map-files-002.c
+++ b/tools/testing/selftests/proc/proc-self-map-files-002.c
@@ -55,7 +55,9 @@ int main(void)
 	if (fd == -1)
 		return 1;
 
-	p = mmap(NULL, PAGE_SIZE, PROT_NONE, MAP_PRIVATE|MAP_FILE|MAP_FIXED, fd, 0);
+	p = mmap((void *) (2 * PAGE_SIZE), PAGE_SIZE, PROT_NONE,
+			MAP_PRIVATE|MAP_FILE|MAP_FIXED, fd, 0);
+
 	if (p == MAP_FAILED) {
 		if (errno == EPERM)
 			return 2;
-- 
2.19.1

Waiting acceptance to VERIFY this bug.
Comment 9 Rafael David Tinoco 2018-11-09 11:34:18 UTC
Submitted the following patch:

https://lkml.org/lkml/2018/11/9/1120

Fixing this issue.
Comment 10 Rafael David Tinoco 2018-11-09 11:35:17 UTC
Flagging as resolved. Will flag it as verified as soon as patch is accepted.
Comment 11 Rafael David Tinoco 2018-11-27 10:00:19 UTC
The patch titled
     Subject: proc: fixup map_files test on arm
has been added to the -mm tree.  Its filename is
     proc-fixup-map_files-test-on-arm.patch

This patch should soon appear at
    http://ozlabs.org/~akpm/mmots/broken-out/proc-fixup-map_files-test-on-arm.patch
and later at
    http://ozlabs.org/~akpm/mmotm/broken-out/proc-fixup-map_files-test-on-arm.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***

The -mm tree is included into linux-next and is updated
there every 3-4 working days

------------------------------------------------------
From: Alexey Dobriyan <adobriyan@gmail.com>
Subject: proc: fixup map_files test on arm

https://bugs.linaro.org/show_bug.cgi?id=3782

Turns out arm doesn't allow to map address 0, so try minimum virtual
address instead.

Link: http://lkml.kernel.org/r/20181113165446.GA28157@avx2
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
Reported-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
Tested-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: Shuah Khan <shuah@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---


=======================================

--- a/tools/testing/selftests/proc/proc-self-map-files-002.c~proc-fixup-map_files-test-on-arm

+++ a/tools/testing/selftests/proc/proc-self-map-files-002.c

@@ -13,7 +13,7 @@

 ·*·ACTION·OF·CONTRACT,·NEGLIGENCE·OR·OTHER·TORTIOUS·ACTION,·ARISING·OUT·OF 
 ·*·OR·IN·CONNECTION·WITH·THE·USE·OR·PERFORMANCE·OF·THIS·SOFTWARE. 
 ·*/ 
-/*·Test·readlink·/proc/self/map_files/...·with·address·0.·*/ 
+/*·Test·readlink·/proc/self/map_files/...·with·minimum·address.·*/ 
 #include·<errno.h> 
 #include·<sys/types.h> 
 #include·<sys/stat.h> 

@@ -47,6 +47,11 @@

 static void fail(const char *fmt, unsign

 int·main(void) 
 { 
 »       const·unsigned·int·PAGE_SIZE·=·sysconf(_SC_PAGESIZE); 
+#ifdef·__arm__ 
+»       unsigned·long·va·=·2·*·PAGE_SIZE; 
+#else 
+»       unsigned·long·va·=·0; 
+#endif 
 »       void·*p; 
 »       int·fd; 
 »       unsigned·long·a,·b; 

@@ -55,7 +60,7 @@

 int main(void)

 »       if·(fd·==·-1) 
 »       »       return·1; 
  
-»       p·=·mmap(NULL,·PAGE_SIZE,·PROT_NONE,·MAP_PRIVATE|MAP_FILE|MAP_FIXED,·fd,·0); 
+»       p·=·mmap((void·*)va,·PAGE_SIZE,·PROT_NONE,·MAP_PRIVATE|MAP_FILE|MAP_FIXED,·fd,·0); 
 »       if·(p·==·MAP_FAILED)·{ 
 »       »       if·(errno·==·EPERM) 
 »       »       »       return·2; 

_
Patches currently in -mm which might be from adobriyan@gmail.com are

proc-update-maintainers-with-proctxt.patch
proc-fixup-map_files-test-on-arm.patch
mm-make-migratetype_names-const-char.patch
mm-make-migrate_reason_names-const-char.patch
mm-make-free_reserved_area-return-const-char.patch
coding-style-dont-use-extern-with-function-prototypes.patch
drop-silly-static-inline-asmlinkage-from-dump_stack.patch
make-initcall_level_names-const-char.patch
Comment 12 Rafael David Tinoco 2018-12-10 13:54:00 UTC
commit dbd4af54745fc0c805217693c807a3928b2d408b
Author: Alexey Dobriyan <adobriyan@gmail.com>
Date:   Fri Nov 30 20:09:53 2018

    proc: fixup map_files test on arm
    
    https://bugs.linaro.org/show_bug.cgi?id=3782
    
    Turns out arm doesn't permit mapping address 0, so try minimum virtual
    address instead.
    
    Link: http://lkml.kernel.org/r/20181113165446.GA28157@avx2
    Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
    Reported-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
    Tested-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
    Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
    Cc: Shuah Khan <shuah@kernel.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

is in mainline now. 

I'm not sure its worth asking Greg to backport this fix, so, instead, I think we should wait until we start using kselftest from 4.20. This way I'll flag both tests "proc-self-map-files-001 and proc-self-map-files-002" as known issue.

"proc-self-map-files-001" (bug 3908) will remain as known issue forever since it hasn't been fixed for arm and it tests exactly the same thing as "proc-self-map-files-002".
Comment 13 Rafael David Tinoco 2018-12-10 14:11:36 UTC
As soon as we start using 4.20 kselftest, we can remove proc-self-map-files-002 from the known issues.
Comment 14 Rafael David Tinoco 2018-12-14 12:33:49 UTC
4.19-stable review patch.  If anyone has any objections, please let me know.

------------------

[ Upstream commit dbd4af54745fc0c805217693c807a3928b2d408b ]

https://bugs.linaro.org/show_bug.cgi?id=3782

Turns out arm doesn't permit mapping address 0, so try minimum virtual
address instead.

Link: http://lkml.kernel.org/r/20181113165446.GA28157@avx2
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
Reported-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
Tested-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: Shuah Khan <shuah@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 tools/testing/selftests/proc/proc-self-map-files-002.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

It got backported to 4.19. We shall now see proc-self-map-files-002 as fixed after this 4.19 version.