[LTP] [PATCH] openposix: mmap/21-1: adjust the test to work with MAP_SHARED_VALIDATE

[LTP] [PATCH] openposix: mmap/21-1: adjust the test to work with MAP_SHARED_VALIDATE

[Stanislav Kholmanskikh]

Linux commit 50a8e840d050 ("mm: introduce MAP_SHARED_VALIDATE, a mechanism to safely define new mmap flags")
introduced the MAP_SHARED_VALIDATE flag, which is similar to MAP_SHARED but
provides additional semantics. So on we need to make sure that the invalid
value of 'flags' in the test is generated taking into account this new flag.

Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
---
 .../conformance/interfaces/mmap/21-1.c             |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/testcases/open_posix_testsuite/conformance/interfaces/mmap/21-1.c b/testcases/open_posix_testsuite/conformance/interfaces/mmap/21-1.c
index dc0cc13..81114b7 100644
--- a/testcases/open_posix_testsuite/conformance/interfaces/mmap/21-1.c
+++ b/testcases/open_posix_testsuite/conformance/interfaces/mmap/21-1.c
@@ -27,6 +27,24 @@
 #include <errno.h>
 #include "posixtest.h"
 
+int is_valid(int flag)
+{
+	if (flag == MAP_SHARED || flag == MAP_PRIVATE || flag == MAP_FIXED)
+		return 1;
+
+#ifdef __linux__
+
+#ifndef MAP_SHARED_VALIDATE
+#define MAP_SHARED_VALIDATE 0x03
+#endif
+
+	if (flag == MAP_SHARED_VALIDATE)
+		return 1;
+#endif
+
+	return 0;
+}
+
 int main(void)
 {
 	char tmpfname[256];
@@ -51,7 +69,7 @@ int main(void)
 	}
 
 	flag = MAP_SHARED;
-	while (flag == MAP_SHARED || flag == MAP_PRIVATE || flag == MAP_FIXED)
+	while (is_valid(flag))
 		flag++;
 
 	pa = mmap(NULL, size, PROT_READ | PROT_WRITE, flag, fd, 0);
-- 
1.7.1

[LTP] [PATCH] openposix: mmap/21-1: adjust the test to work with MAP_SHARED_VALIDATE

[Cyril Hrubis]

Hi!
> +int is_valid(int flag)
> +{
> +	if (flag == MAP_SHARED || flag == MAP_PRIVATE || flag == MAP_FIXED)
> +		return 1;
> +
> +#ifdef __linux__
> +
> +#ifndef MAP_SHARED_VALIDATE
> +#define MAP_SHARED_VALIDATE 0x03
> +#endif
> +
> +	if (flag == MAP_SHARED_VALIDATE)
> +		return 1;
> +#endif
> +
> +	return 0;
> +}

Hmm, this probably does not scale up, what about setting the flag to
have all its bits set (i.e. passing ~0 to the mmap()) from the start
instead?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] openposix: mmap/21-1: adjust the test to work with MAP_SHARED_VALIDATE

[Stanislav Kholmanskikh]

Hi.

On 02/08/2018 07:34 PM, Cyril Hrubis wrote:
> Hi!
>> +int is_valid(int flag)
>> +{
>> +	if (flag == MAP_SHARED || flag == MAP_PRIVATE || flag == MAP_FIXED)
>> +		return 1;
>> +
>> +#ifdef __linux__
>> +
>> +#ifndef MAP_SHARED_VALIDATE
>> +#define MAP_SHARED_VALIDATE 0x03
>> +#endif
>> +
>> +	if (flag == MAP_SHARED_VALIDATE)
>> +		return 1;
>> +#endif
>> +
>> +	return 0;
>> +}
> 
> Hmm, this probably does not scale up, what about setting the flag to
> have all its bits set (i.e. passing ~0 to the mmap()) from the start
> instead?
> 

In main() we search for the first value of 'flag' for which the
following statement is not true:

flag == MAP_SHARED || flag == MAP_PRIVATE || flag == MAP_FIXED || flag
== MAP_SHARED_VALIDATE

where the last part is Linux-specific and likely needs an #ifdef.

We are not interested if flag has MAP_SHARED or other bits set, but we
are interested when flag _equals_ MAP_SHARED or other values.

In order to make the code more readable (IMHO) I decided to move this
check into a separate function.

I don't fully understand your proposal. Could you, please, elaborate a
bit more?

Thanks.

[LTP] [PATCH] openposix: mmap/21-1: adjust the test to work with MAP_SHARED_VALIDATE

[Cyril Hrubis]

Hi!
> >> +int is_valid(int flag)
> >> +{
> >> +	if (flag == MAP_SHARED || flag == MAP_PRIVATE || flag == MAP_FIXED)
> >> +		return 1;
> >> +
> >> +#ifdef __linux__
> >> +
> >> +#ifndef MAP_SHARED_VALIDATE
> >> +#define MAP_SHARED_VALIDATE 0x03
> >> +#endif
> >> +
> >> +	if (flag == MAP_SHARED_VALIDATE)
> >> +		return 1;
> >> +#endif
> >> +
> >> +	return 0;
> >> +}
> > 
> > Hmm, this probably does not scale up, what about setting the flag to
> > have all its bits set (i.e. passing ~0 to the mmap()) from the start
> > instead?
> > 
> 
> In main() we search for the first value of 'flag' for which the
> following statement is not true:
> 
> flag == MAP_SHARED || flag == MAP_PRIVATE || flag == MAP_FIXED || flag
> == MAP_SHARED_VALIDATE
> 
> where the last part is Linux-specific and likely needs an #ifdef.
> 
> We are not interested if flag has MAP_SHARED or other bits set, but we
> are interested when flag _equals_ MAP_SHARED or other values.
> 
> In order to make the code more readable (IMHO) I decided to move this
> check into a separate function.
> 
> I don't fully understand your proposal. Could you, please, elaborate a
> bit more?

We are trying to trigger EINVAL by passing an invalid flags value to
mmap. And given that the flags are likely to be allocated in increasing
manner starting with known flag and increasing it while it's equal to
other known flags will break again sooner or later and we will end up
adding more and more special cases for different systems.

Hence I suggest to use a value that is unlikely to be valid flag (all
bits set) and use that for the testing.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] openposix: mmap/21-1: use all bits set as the invalid flag

[Stanislav Kholmanskikh]

The test is trying to trigger EINVAL by passing an invalid flags
value to mmap(). Linux commit 50a8e840d050 ("mm: introduce MAP_SHARED_VALIDATE, a mechanism to safely define new mmap flags")
introduced a new flag (MAP_SHARED_VALIDATE) and that flag may not be the last one.

So instead of adding an 'if' for each new flag we use a value with all bits
set (which is unlikely to be a valid flag).

Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
---
 .../conformance/interfaces/mmap/21-1.c             |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/testcases/open_posix_testsuite/conformance/interfaces/mmap/21-1.c b/testcases/open_posix_testsuite/conformance/interfaces/mmap/21-1.c
index dc0cc13..ce636cf 100644
--- a/testcases/open_posix_testsuite/conformance/interfaces/mmap/21-1.c
+++ b/testcases/open_posix_testsuite/conformance/interfaces/mmap/21-1.c
@@ -33,7 +33,7 @@ int main(void)
 
 	void *pa;
 	size_t size = 1024;
-	int flag;
+	int flag = ~0;
 	int fd;
 
 	snprintf(tmpfname, sizeof(tmpfname), "/tmp/pts_mmap_21_1_%d", getpid());
@@ -50,10 +50,6 @@ int main(void)
 		return PTS_UNRESOLVED;
 	}
 
-	flag = MAP_SHARED;
-	while (flag == MAP_SHARED || flag == MAP_PRIVATE || flag == MAP_FIXED)
-		flag++;
-
 	pa = mmap(NULL, size, PROT_READ | PROT_WRITE, flag, fd, 0);
 	if (pa == MAP_FAILED && errno == EINVAL) {
 		printf("Test PASSED\n");
-- 
1.7.1

[LTP] [PATCH] openposix: mmap/21-1: use all bits set as the invalid flag

[Cyril Hrubis]

Hi!
> The test is trying to trigger EINVAL by passing an invalid flags
> value to mmap(). Linux commit 50a8e840d050 ("mm: introduce MAP_SHARED_VALIDATE, a mechanism to safely define new mmap flags")
> introduced a new flag (MAP_SHARED_VALIDATE) and that flag may not be the last one.
> 
> So instead of adding an 'if' for each new flag we use a value with all bits
> set (which is unlikely to be a valid flag).

Acked, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] openposix: mmap/21-1: use all bits set as the invalid flag

[Stanislav Kholmanskikh]

On 03/01/2018 12:50 PM, Cyril Hrubis wrote:
> Hi!
>> The test is trying to trigger EINVAL by passing an invalid flags
>> value to mmap(). Linux commit 50a8e840d050 ("mm: introduce MAP_SHARED_VALIDATE, a mechanism to safely define new mmap flags")
>> introduced a new flag (MAP_SHARED_VALIDATE) and that flag may not be the last one.
>>
>> So instead of adding an 'if' for each new flag we use a value with all bits
>> set (which is unlikely to be a valid flag).
> 
> Acked, thanks.
> 
Thank you. Committed.