| Message ID | 20181011184359.15627-1-rafael.tinoco@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | proc: fix proc-self-map-files selftest for arm | expand |
On Thu, Oct 11, 2018 at 03:43:59PM -0300, Rafael David Tinoco wrote: > 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; As far as I remember nil virtual address has been there only to be sure the vma allocated won't be merged with another vmas. Fore sure most of x86 standart application won't be using 8K address as well, so should do the trick I think. (Strictlly speaking the test should be rather parsing own maps first and find unused address instead but whatever :) Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
On Thu, Oct 11, 2018 at 03:43:59PM -0300, Rafael David Tinoco wrote: > 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); As the comment in the beginning says this test is specifically for addresss 0. Maybe it should be ifdeffed with __arm__ then.
On Thu, Oct 11, 2018 at 11:56:01PM +0300, Alexey Dobriyan wrote: > > As the comment in the beginning says this test is specifically for addresss 0. > Maybe it should be ifdeffed with __arm__ then. Is there some other reason than allocating non-mergable VMA?
On Fri, Oct 12, 2018 at 12:02:56AM +0300, Cyrill Gorcunov wrote: > On Thu, Oct 11, 2018 at 11:56:01PM +0300, Alexey Dobriyan wrote: > > > > As the comment in the beginning says this test is specifically for addresss 0. > > Maybe it should be ifdeffed with __arm__ then. > > Is there some other reason than allocating non-mergable VMA? IIRC the reason is to test address 0 as it is effectively banned for userspace so if it will be broken, it will be broken silently for a long time. As for "unmergeable" libc here doesn't map /dev/zero. I know how to avoid even theoretical breakage by creating binaries by hand but it will be probably too much.
On Fri, Oct 12, 2018 at 12:30:06AM +0300, Alexey Dobriyan wrote: > On Fri, Oct 12, 2018 at 12:02:56AM +0300, Cyrill Gorcunov wrote: > > On Thu, Oct 11, 2018 at 11:56:01PM +0300, Alexey Dobriyan wrote: > > > > > > As the comment in the beginning says this test is specifically for addresss 0. > > > Maybe it should be ifdeffed with __arm__ then. > > > > Is there some other reason than allocating non-mergable VMA? > > IIRC the reason is to test address 0 as it is effectively banned > for userspace so if it will be broken, it will be broken silently > for a long time. This is rather a side effect of the test because the primary reason was to check procfs numbers conversion, right? Don't get me wrong, I don't mind about __arm__ define or similar, this is fine for one architecture, but if there comes more we will get a number of #ifdefs which is unrelated to procfs numeric routines at all. > As for "unmergeable" libc here doesn't map /dev/zero. I know how to > avoid even theoretical breakage by creating binaries by hand but it > will be probably too much. Sure.
On 10/11/18 7:00 PM, Cyrill Gorcunov wrote: > On Fri, Oct 12, 2018 at 12:30:06AM +0300, Alexey Dobriyan wrote: >> On Fri, Oct 12, 2018 at 12:02:56AM +0300, Cyrill Gorcunov wrote: >>> On Thu, Oct 11, 2018 at 11:56:01PM +0300, Alexey Dobriyan wrote: >>>> >>>> As the comment in the beginning says this test is specifically for addresss 0. >>>> Maybe it should be ifdeffed with __arm__ then. >>> >>> Is there some other reason than allocating non-mergable VMA? >> >> IIRC the reason is to test address 0 as it is effectively banned >> for userspace so if it will be broken, it will be broken silently >> for a long time. > > This is rather a side effect of the test because the primary reason > was to check procfs numbers conversion, right? Don't get me wrong, > I don't mind about __arm__ define or similar, this is fine for > one architecture, but if there comes more we will get a number > of #ifdefs which is unrelated to procfs numeric routines at all. That is what I also had in mind, thus the patch. I just realized we had another issue on LKFT (our functional tests tool) for proc-self-map-files-001.c. Test 001 does pretty much the same as 002, but without the MAP_FIXED mmap flag. Is it okay to consolidate both tests into just 1, and focus in checking procfs numbers conversion only, rather than if mapping 0 is allowed or not ? Can I send a v2 with that in mind ? > >> As for "unmergeable" libc here doesn't map /dev/zero. I know how to >> avoid even theoretical breakage by creating binaries by hand but it >> will be probably too much. > > Sure. >
On Mon, Oct 15, 2018 at 01:55:14PM -0300, Rafael David Tinoco wrote: > That is what I also had in mind, thus the patch. I just realized we had > another issue on LKFT (our functional tests tool) for > proc-self-map-files-001.c. Test 001 does pretty much the same as 002, but > without the MAP_FIXED mmap flag. > > Is it okay to consolidate both tests into just 1, and focus in checking > procfs numbers conversion only, rather than if mapping 0 is allowed or not ? > Can I send a v2 with that in mind ? As to me -- yes, I would move zero page testing to a separate memory testcase. But since Alexey is the former author of the tests better wait for his opinion.
On Mon, Oct 15, 2018 at 2:21 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote: > On Mon, Oct 15, 2018 at 01:55:14PM -0300, Rafael David Tinoco wrote: >> That is what I also had in mind, thus the patch. I just realized we had >> another issue on LKFT (our functional tests tool) for >> proc-self-map-files-001.c. Test 001 does pretty much the same as 002, but >> without the MAP_FIXED mmap flag. >> >> Is it okay to consolidate both tests into just 1, and focus in checking >> procfs numbers conversion only, rather than if mapping 0 is allowed or not ? >> Can I send a v2 with that in mind ? > > As to me -- yes, I would move zero page testing to a separate memory testcase. > But since Alexey is the former author of the tests better wait for his opinion. Alexey, would you care if we turn those 2 tests into 1, taking care of the zero page testing elsewhere ? Would you mind if I send out a patch for that ?
On Thu, Nov 08, 2018 at 08:41:46AM -0200, Rafael David Tinoco wrote: > > > > As to me -- yes, I would move zero page testing to a separate memory testcase. > > But since Alexey is the former author of the tests better wait for his opinion. > > Alexey, > > would you care if we turn those 2 tests into 1, taking care of the > zero page testing elsewhere ? Would you mind if I send out a patch for > that ? Rafael, just make it so. While we preserve old functionality I think it is fine. Cyrill
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;
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(-) -- 2.19.1