proc: fix proc-self-map-files selftest for arm
diff mbox series

Message ID 20181011184359.15627-1-rafael.tinoco@linaro.org
State New
Headers show
Series
  • proc: fix proc-self-map-files selftest for arm
Related show

Commit Message

Rafael David Tinoco Oct. 11, 2018, 6:43 p.m. UTC
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(-)

Comments

Cyrill Gorcunov Oct. 11, 2018, 7:42 p.m. UTC | #1
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>
Alexey Dobriyan Oct. 11, 2018, 8:56 p.m. UTC | #2
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.
Cyrill Gorcunov Oct. 11, 2018, 9:02 p.m. UTC | #3
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?
Alexey Dobriyan Oct. 11, 2018, 9:30 p.m. UTC | #4
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.
Cyrill Gorcunov Oct. 11, 2018, 10 p.m. UTC | #5
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.
Rafael David Tinoco Oct. 15, 2018, 4:55 p.m. UTC | #6
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.
>
Cyrill Gorcunov Oct. 15, 2018, 5:21 p.m. UTC | #7
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.
Rafael David Tinoco Nov. 8, 2018, 10:41 a.m. UTC | #8
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 ?
Cyrill Gorcunov Nov. 8, 2018, 11:11 a.m. UTC | #9
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

Patch
diff mbox series

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;