diff mbox series

proc: fix and merge proc-self-map-file tests

Message ID 20181109113036.21512-1-rafael.tinoco@linaro.org (mailing list archive)
State New
Headers show
Series proc: fix and merge proc-self-map-file tests | expand

Commit Message

Rafael David Tinoco Nov. 9, 2018, 11:30 a.m. UTC
Merge proc-self-map-files tests into one since this test should focus in
testing readlink in /proc/self/map_files/* only, and not trying to test
mapping virtual address 0.

Lowest virtual address for user space mapping in other architectures,
like arm, is *at least* *(PAGE_SIZE * 2) and NULL hint does not
guarantee that when MAP_FIXED flag, important to this test, is given.
This patch also fixes this issue in remaining test.

Link: https://bugs.linaro.org/show_bug.cgi?id=3782
Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
---
 tools/testing/selftests/proc/.gitignore       |  1 -
 tools/testing/selftests/proc/Makefile         |  1 -
 .../selftests/proc/proc-self-map-files-001.c  |  5 +-
 .../selftests/proc/proc-self-map-files-002.c  | 85 -------------------
 4 files changed, 3 insertions(+), 89 deletions(-)
 delete mode 100644 tools/testing/selftests/proc/proc-self-map-files-002.c

Comments

Cyrill Gorcunov Nov. 9, 2018, 11:41 a.m. UTC | #1
On Fri, Nov 09, 2018 at 09:30:36AM -0200, Rafael David Tinoco wrote:
> Merge proc-self-map-files tests into one since this test should focus in
> testing readlink in /proc/self/map_files/* only, and not trying to test
> mapping virtual address 0.
> 
> Lowest virtual address for user space mapping in other architectures,
> like arm, is *at least* *(PAGE_SIZE * 2) and NULL hint does not
> guarantee that when MAP_FIXED flag, important to this test, is given.
> This patch also fixes this issue in remaining test.
> 
> Link: https://bugs.linaro.org/show_bug.cgi?id=3782
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>

Wait, Rafael. But we will loose the test of mapping virtual address 0
then. I though you would move testing of virtual address 0 into
a separate testcase. I mean testing of first page was a positive
side effect of the former Alexey's patch, so we definitely should
keep it on x86 at least. Gimme some time I'll try to address it
today evening or tomorrow. I think this way everybody will be
happy: procfs get passed on arm32 and x86 will still have first
page testing.
Rafael David Tinoco Nov. 9, 2018, 11:45 a.m. UTC | #2
On Fri, Nov 9, 2018, at 9:41 AM, Cyrill Gorcunov wrote:
> On Fri, Nov 09, 2018 at 09:30:36AM -0200, Rafael David Tinoco wrote:
> > Merge proc-self-map-files tests into one since this test should focus in
> > testing readlink in /proc/self/map_files/* only, and not trying to test
> > mapping virtual address 0.
> > 
> > Lowest virtual address for user space mapping in other architectures,
> > like arm, is *at least* *(PAGE_SIZE * 2) and NULL hint does not
> > guarantee that when MAP_FIXED flag, important to this test, is given.
> > This patch also fixes this issue in remaining test.
> > 
> > Link: https://bugs.linaro.org/show_bug.cgi?id=3782
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
> 
> Wait, Rafael. But we will loose the test of mapping virtual address 0
> then. I though you would move testing of virtual address 0 into
> a separate testcase. I mean testing of first page was a positive
> side effect of the former Alexey's patch, so we definitely should
> keep it on x86 at least. Gimme some time I'll try to address it
> today evening or tomorrow. I think this way everybody will be
> happy: procfs get passed on arm32 and x86 will still have first
> page testing.

Ohh, my understanding was that this was going to be addressed in some other test, like what you said.. I did not understand you wanted me to create a test for it altogether, my bad. I can do it if you want, let me know, pls.

Thanks!
Cyrill Gorcunov Nov. 9, 2018, 11:48 a.m. UTC | #3
On Fri, Nov 09, 2018 at 09:45:36AM -0200, Rafael David Tinoco wrote:
...
> > today evening or tomorrow. I think this way everybody will be
> > happy: procfs get passed on arm32 and x86 will still have first
> > page testing.
> 
> Ohh, my understanding was that this was going to be addressed in some other test,
> like what you said.. I did not understand you wanted me to create a test for
> it altogether, my bad. I can do it if you want, let me know, pls.

:-) I'll be able to create patch and test in about 10 hours, so
feel free to beat me if you have time meanwhile.
Rafael David Tinoco Nov. 9, 2018, 12:01 p.m. UTC | #4
On Fri, Nov 9, 2018, at 9:48 AM, Cyrill Gorcunov wrote:
> On Fri, Nov 09, 2018 at 09:45:36AM -0200, Rafael David Tinoco wrote:
> ...
> > > today evening or tomorrow. I think this way everybody will be
> > > happy: procfs get passed on arm32 and x86 will still have first
> > > page testing.
> > 
> > Ohh, my understanding was that this was going to be addressed in some other test,
> > like what you said.. I did not understand you wanted me to create a test for
> > it altogether, my bad. I can do it if you want, let me know, pls.
> 
> :-) I'll be able to create patch and test in about 10 hours, so
> feel free to beat me if you have time meanwhile.

Alright, I'm fixing membarrier_test before, so.. I guess we have a competition.. =o)
Cyrill Gorcunov Nov. 9, 2018, 6:04 p.m. UTC | #5
On Fri, Nov 09, 2018 at 10:01:13AM -0200, Rafael David Tinoco wrote:
> 
> Alright, I'm fixing membarrier_test before, so.. I guess we have a competition.. =o)

Rafael, Alexey, what about simply wrap the test code with x86 and extend later
with all archs which support zero address mapping?
---
 tools/testing/selftests/proc/proc-self-map-files-002.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

Index: linux-ml.git/tools/testing/selftests/proc/proc-self-map-files-002.c
===================================================================
--- linux-ml.git.orig/tools/testing/selftests/proc/proc-self-map-files-002.c
+++ linux-ml.git/tools/testing/selftests/proc/proc-self-map-files-002.c
@@ -23,6 +23,11 @@
 #include <sys/mman.h>
 #include <stdlib.h>
 
+/*
+ * Should run on archs which support zero address mapping.
+ */
+#if defined(__i386) || defined(__x86_64)
+
 static void pass(const char *fmt, unsigned long a, unsigned long b)
 {
 	char name[64];
@@ -83,3 +88,12 @@ int main(void)
 
 	return 0;
 }
+
+#else
+
+int main(void)
+{
+	return 0;
+}
+
+#endif
Rafael David Tinoco Nov. 9, 2018, 6:48 p.m. UTC | #6
On Fri, Nov 9, 2018, at 4:04 PM, Cyrill Gorcunov wrote:
> On Fri, Nov 09, 2018 at 10:01:13AM -0200, Rafael David Tinoco wrote:
> > 
> > Alright, I'm fixing membarrier_test before, so.. I guess we have a competition.. =o)
> 
> Rafael, Alexey, what about simply wrap the test code with x86 and extend later
> with all archs which support zero address mapping?
> ---
>  tools/testing/selftests/proc/proc-self-map-files-002.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> Index: linux-ml.git/tools/testing/selftests/proc/proc-self-map-files-002.c
> ===================================================================
> --- linux-ml.git.orig/tools/testing/selftests/proc/proc-self-map-files-002.c
> +++ linux-ml.git/tools/testing/selftests/proc/proc-self-map-files-002.c
> @@ -23,6 +23,11 @@
>  #include <sys/mman.h>
>  #include <stdlib.h>
>  
> +/*
> + * Should run on archs which support zero address mapping.
> + */
> +#if defined(__i386) || defined(__x86_64)
> +
>  static void pass(const char *fmt, unsigned long a, unsigned long b)
>  {
>  	char name[64];
> @@ -83,3 +88,12 @@ int main(void)
>  
>  	return 0;
>  }
> +
> +#else
> +
> +int main(void)
> +{
> +	return 0;
> +}
> +
> +#endif

let me see if I got this right.. the premise for this test is to have *at least*
2 vmas, so we can check if the symlink for the mem range, describing the mapped
area, is correct in procfs files, correct ? if yes, then why to have a totally
duplicated test... just to check if mmap(0, ... MAP_FIXED ...) would work ?

Wouldn't exist a better place to have such test ? like in
tools/testing/selftests/vm/mmap-null.c or something like it ?  genuine
curiosity.. thinking i'm missing something about this test...
Cyrill Gorcunov Nov. 9, 2018, 7:39 p.m. UTC | #7
On Fri, Nov 09, 2018 at 04:48:49PM -0200, Rafael David Tinoco wrote:
> 
> let me see if I got this right.. the premise for this test is to have *at least*
> 2 vmas, so we can check if the symlink for the mem range, describing the mapped
> area, is correct in procfs files, correct ? if yes, then why to have a totally
> duplicated test... just to check if mmap(0, ... MAP_FIXED ...) would work ?
> 
> Wouldn't exist a better place to have such test ? like in
> tools/testing/selftests/vm/mmap-null.c or something like it ?  genuine
> curiosity.. thinking i'm missing something about this test...

Ah, I happen to miss that they are identical except nil address. Then
true, vm/ looks like more suitable place for that. Do you happen to
know which exactly archs reserve first page (together with x86)?
Alexey Dobriyan Nov. 10, 2018, 5:47 p.m. UTC | #8
On Fri, Nov 09, 2018 at 09:30:36AM -0200, Rafael David Tinoco wrote:
> Merge proc-self-map-files tests into one since this test should focus in
> testing readlink in /proc/self/map_files/* only, and not trying to test
> mapping virtual address 0.
> 
> Lowest virtual address for user space mapping in other architectures,
> like arm, is *at least* *(PAGE_SIZE * 2) and NULL hint does not
> guarantee that when MAP_FIXED flag, important to this test, is given.
> This patch also fixes this issue in remaining test.

> -	p = mmap(NULL, PAGE_SIZE, PROT_NONE, MAP_PRIVATE|MAP_FILE, fd, 0);
> +	p = mmap((void *) (2 * PAGE_SIZE), PAGE_SIZE, PROT_NONE,

I don't know ARM. Is this 2 page limitation a limitation of hardware or
kernel's?
Rafael David Tinoco Nov. 10, 2018, 5:56 p.m. UTC | #9
On Sat, Nov 10, 2018, at 3:47 PM, Alexey Dobriyan wrote:
> On Fri, Nov 09, 2018 at 09:30:36AM -0200, Rafael David Tinoco wrote:
> > Merge proc-self-map-files tests into one since this test should focus in
> > testing readlink in /proc/self/map_files/* only, and not trying to test
> > mapping virtual address 0.
> > 
> > Lowest virtual address for user space mapping in other architectures,
> > like arm, is *at least* *(PAGE_SIZE * 2) and NULL hint does not
> > guarantee that when MAP_FIXED flag, important to this test, is given.
> > This patch also fixes this issue in remaining test.
> 
> > -	p = mmap(NULL, PAGE_SIZE, PROT_NONE, MAP_PRIVATE|MAP_FILE, fd, 0);
> > +	p = mmap((void *) (2 * PAGE_SIZE), PAGE_SIZE, PROT_NONE,
> 
> I don't know ARM. Is this 2 page limitation a limitation of hardware or
> kernel's?

Kernel:
https://bugs.linaro.org/show_bug.cgi?id=3782#c7
Alexey Dobriyan Nov. 10, 2018, 6:49 p.m. UTC | #10
On Sat, Nov 10, 2018 at 03:56:03PM -0200, Rafael David Tinoco wrote:
> On Sat, Nov 10, 2018, at 3:47 PM, Alexey Dobriyan wrote:
> > On Fri, Nov 09, 2018 at 09:30:36AM -0200, Rafael David Tinoco wrote:
> > > Merge proc-self-map-files tests into one since this test should focus in
> > > testing readlink in /proc/self/map_files/* only, and not trying to test
> > > mapping virtual address 0.
> > > 
> > > Lowest virtual address for user space mapping in other architectures,
> > > like arm, is *at least* *(PAGE_SIZE * 2) and NULL hint does not
> > > guarantee that when MAP_FIXED flag, important to this test, is given.
> > > This patch also fixes this issue in remaining test.
> > 
> > > -	p = mmap(NULL, PAGE_SIZE, PROT_NONE, MAP_PRIVATE|MAP_FILE, fd, 0);
> > > +	p = mmap((void *) (2 * PAGE_SIZE), PAGE_SIZE, PROT_NONE,
> > 
> > I don't know ARM. Is this 2 page limitation a limitation of hardware or
> > kernel's?
> 
> Kernel:
> https://bugs.linaro.org/show_bug.cgi?id=3782#c7

Ahh. please test the path I've sent, I don't have arm install readily
available.
Rafael David Tinoco Nov. 11, 2018, 2:50 a.m. UTC | #11
On Sat, Nov 10, 2018, at 4:49 PM, Alexey Dobriyan wrote:
> On Sat, Nov 10, 2018 at 03:56:03PM -0200, Rafael David Tinoco wrote:
> > On Sat, Nov 10, 2018, at 3:47 PM, Alexey Dobriyan wrote:
> > > On Fri, Nov 09, 2018 at 09:30:36AM -0200, Rafael David Tinoco wrote:
> > > > Merge proc-self-map-files tests into one since this test should focus in
> > > > testing readlink in /proc/self/map_files/* only, and not trying to test
> > > > mapping virtual address 0.
> > > > 
> > > > Lowest virtual address for user space mapping in other architectures,
> > > > like arm, is *at least* *(PAGE_SIZE * 2) and NULL hint does not
> > > > guarantee that when MAP_FIXED flag, important to this test, is given.
> > > > This patch also fixes this issue in remaining test.
> > > 
> > > > -	p = mmap(NULL, PAGE_SIZE, PROT_NONE, MAP_PRIVATE|MAP_FILE, fd, 0);
> > > > +	p = mmap((void *) (2 * PAGE_SIZE), PAGE_SIZE, PROT_NONE,
> > > 
> > > I don't know ARM. Is this 2 page limitation a limitation of hardware or
> > > kernel's?
> > 
> > Kernel:
> > https://bugs.linaro.org/show_bug.cgi?id=3782#c7
> 
> Ahh. please test the path I've sent, I don't have arm install readily
> available.

I replied to your patch based on some of the discussion we had in this thread.

Thanks

Rafael
-
Rafael D. Tinoco
Linaro Kernel Validation Team
diff mbox series

Patch

diff --git a/tools/testing/selftests/proc/.gitignore b/tools/testing/selftests/proc/.gitignore
index 82121a81681f..d44ec8755879 100644
--- a/tools/testing/selftests/proc/.gitignore
+++ b/tools/testing/selftests/proc/.gitignore
@@ -3,7 +3,6 @@ 
 /fd-003-kthread
 /proc-loadavg-001
 /proc-self-map-files-001
-/proc-self-map-files-002
 /proc-self-syscall
 /proc-self-wchan
 /proc-uptime-001
diff --git a/tools/testing/selftests/proc/Makefile b/tools/testing/selftests/proc/Makefile
index 1c12c34cf85d..6c17557c2f9a 100644
--- a/tools/testing/selftests/proc/Makefile
+++ b/tools/testing/selftests/proc/Makefile
@@ -7,7 +7,6 @@  TEST_GEN_PROGS += fd-002-posix-eq
 TEST_GEN_PROGS += fd-003-kthread
 TEST_GEN_PROGS += proc-loadavg-001
 TEST_GEN_PROGS += proc-self-map-files-001
-TEST_GEN_PROGS += proc-self-map-files-002
 TEST_GEN_PROGS += proc-self-syscall
 TEST_GEN_PROGS += proc-self-wchan
 TEST_GEN_PROGS += proc-uptime-001
diff --git a/tools/testing/selftests/proc/proc-self-map-files-001.c b/tools/testing/selftests/proc/proc-self-map-files-001.c
index 4209c64283d6..646d8d3fba3a 100644
--- a/tools/testing/selftests/proc/proc-self-map-files-001.c
+++ b/tools/testing/selftests/proc/proc-self-map-files-001.c
@@ -46,16 +46,17 @@  static void fail(const char *fmt, unsigned long a, unsigned long b)
 
 int main(void)
 {
-	const unsigned int PAGE_SIZE = sysconf(_SC_PAGESIZE);
 	void *p;
 	int fd;
 	unsigned long a, b;
+	const long PAGE_SIZE = sysconf(_SC_PAGESIZE);
 
 	fd = open("/dev/zero", O_RDONLY);
 	if (fd == -1)
 		return 1;
 
-	p = mmap(NULL, PAGE_SIZE, PROT_NONE, MAP_PRIVATE|MAP_FILE, fd, 0);
+	p = mmap((void *) (2 * PAGE_SIZE), PAGE_SIZE, PROT_NONE,
+			MAP_PRIVATE|MAP_FILE, fd, 0);
 	if (p == MAP_FAILED)
 		return 1;
 
diff --git a/tools/testing/selftests/proc/proc-self-map-files-002.c b/tools/testing/selftests/proc/proc-self-map-files-002.c
deleted file mode 100644
index 6f1f4a6e1ecb..000000000000
--- a/tools/testing/selftests/proc/proc-self-map-files-002.c
+++ /dev/null
@@ -1,85 +0,0 @@ 
-/*
- * Copyright © 2018 Alexey Dobriyan <adobriyan@gmail.com>
- *
- * Permission to use, copy, modify, and distribute this software for any
- * purpose with or without fee is hereby granted, provided that the above
- * copyright notice and this permission notice appear in all copies.
- *
- * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
- * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
- * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
- * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
- * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
- * 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. */
-#include <errno.h>
-#include <sys/types.h>
-#include <sys/stat.h>
-#include <fcntl.h>
-#include <stdio.h>
-#include <unistd.h>
-#include <sys/mman.h>
-#include <stdlib.h>
-
-static void pass(const char *fmt, unsigned long a, unsigned long b)
-{
-	char name[64];
-	char buf[64];
-
-	snprintf(name, sizeof(name), fmt, a, b);
-	if (readlink(name, buf, sizeof(buf)) == -1)
-		exit(1);
-}
-
-static void fail(const char *fmt, unsigned long a, unsigned long b)
-{
-	char name[64];
-	char buf[64];
-
-	snprintf(name, sizeof(name), fmt, a, b);
-	if (readlink(name, buf, sizeof(buf)) == -1 && errno == ENOENT)
-		return;
-	exit(1);
-}
-
-int main(void)
-{
-	const unsigned int PAGE_SIZE = sysconf(_SC_PAGESIZE);
-	void *p;
-	int fd;
-	unsigned long a, b;
-
-	fd = open("/dev/zero", O_RDONLY);
-	if (fd == -1)
-		return 1;
-
-	p = mmap(NULL, PAGE_SIZE, PROT_NONE, MAP_PRIVATE|MAP_FILE|MAP_FIXED, fd, 0);
-	if (p == MAP_FAILED) {
-		if (errno == EPERM)
-			return 2;
-		return 1;
-	}
-
-	a = (unsigned long)p;
-	b = (unsigned long)p + PAGE_SIZE;
-
-	pass("/proc/self/map_files/%lx-%lx", a, b);
-	fail("/proc/self/map_files/ %lx-%lx", a, b);
-	fail("/proc/self/map_files/%lx -%lx", a, b);
-	fail("/proc/self/map_files/%lx- %lx", a, b);
-	fail("/proc/self/map_files/%lx-%lx ", a, b);
-	fail("/proc/self/map_files/0%lx-%lx", a, b);
-	fail("/proc/self/map_files/%lx-0%lx", a, b);
-	if (sizeof(long) == 4) {
-		fail("/proc/self/map_files/100000000%lx-%lx", a, b);
-		fail("/proc/self/map_files/%lx-100000000%lx", a, b);
-	} else if (sizeof(long) == 8) {
-		fail("/proc/self/map_files/10000000000000000%lx-%lx", a, b);
-		fail("/proc/self/map_files/%lx-10000000000000000%lx", a, b);
-	} else
-		return 1;
-
-	return 0;
-}