[LTP] [PATCH] mmap04.c: Avoid vma merging

[LTP] [PATCH] mmap04.c: Avoid vma merging

[Avinesh Kumar]

We hit a scenario where new mapping was merged with existing mapping of
same permission and the return address from the mmap was hidden in the
merged mapping in /proc/self/maps, causing the test to fail.
To avoid this, we first  create a 2-page mapping with the different
permissions, and then remap the 2nd page with the perms being tested.

Signed-off-by: Avinesh Kumar <akumar@suse.de>
Reported-by: Martin Doucha <mdoucha@suse.cz>
---
 testcases/kernel/syscalls/mmap/mmap04.c | 49 +++++++++++++++----------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/testcases/kernel/syscalls/mmap/mmap04.c b/testcases/kernel/syscalls/mmap/mmap04.c
index f6f4f7c98..f0f87b7f5 100644
--- a/testcases/kernel/syscalls/mmap/mmap04.c
+++ b/testcases/kernel/syscalls/mmap/mmap04.c
@@ -17,28 +17,28 @@
 #include "tst_test.h"
 #include <stdio.h>
 
-#define MMAPSIZE 1024
-static char *addr;
+static char *addr1;
+static char *addr2;
 
 static struct tcase {
 	int prot;
 	int flags;
 	char *exp_perms;
 } tcases[] = {
-	{PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, "---p"},
-	{PROT_NONE, MAP_ANONYMOUS | MAP_SHARED, "---s"},
-	{PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, "r--p"},
-	{PROT_READ, MAP_ANONYMOUS | MAP_SHARED, "r--s"},
-	{PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, "-w-p"},
-	{PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED, "-w-s"},
-	{PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, "rw-p"},
-	{PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED, "rw-s"},
-	{PROT_READ | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE, "r-xp"},
-	{PROT_READ | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED, "r-xs"},
-	{PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE, "-wxp"},
-	{PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED, "-wxs"},
-	{PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE, "rwxp"},
-	{PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED, "rwxs"}
+	{PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "---p"},
+	{PROT_NONE, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "---s"},
+	{PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "r--p"},
+	{PROT_READ, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "r--s"},
+	{PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "-w-p"},
+	{PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "-w-s"},
+	{PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "rw-p"},
+	{PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "rw-s"},
+	{PROT_READ | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "r-xp"},
+	{PROT_READ | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "r-xs"},
+	{PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "-wxp"},
+	{PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "-wxs"},
+	{PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "rwxp"},
+	{PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "rwxs"}
 };
 
 static void run(unsigned int i)
@@ -47,10 +47,21 @@ static void run(unsigned int i)
 	char addr_str[20];
 	char perms[8];
 	char fmt[1024];
+	unsigned int pagesize;
 
-	addr = SAFE_MMAP(NULL, MMAPSIZE, tc->prot, tc->flags, -1, 0);
+	pagesize = SAFE_SYSCONF(_SC_PAGESIZE);
 
-	sprintf(addr_str, "%" PRIxPTR, (uintptr_t)addr);
+	/* To avoid new mapping getting merged with existing mappings, we first
+	   create a 2-page mapping with the different permissions, and then remap
+	   the 2nd page with the perms being tested. */
+	if ((tc->prot == PROT_NONE) && (tc->flags == (MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED)))
+		addr1 = SAFE_MMAP(NULL, pagesize * 2, PROT_READ, MAP_ANONYMOUS | MAP_SHARED, -1, 0);
+	else
+		addr1 = SAFE_MMAP(NULL, pagesize * 2, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+
+	addr2 = SAFE_MMAP(addr1 + pagesize, pagesize, tc->prot, tc->flags, -1, 0);
+
+	sprintf(addr_str, "%" PRIxPTR, (uintptr_t)addr2);
 	sprintf(fmt, "%s-%%*x %%s", addr_str);
 	SAFE_FILE_LINES_SCANF("/proc/self/maps", fmt, perms);
 
@@ -61,7 +72,7 @@ static void run(unsigned int i)
 						tc->exp_perms, perms);
 	}
 
-	SAFE_MUNMAP(addr, MMAPSIZE);
+	SAFE_MUNMAP(addr1, pagesize * 2);
 }
 
 static struct tst_test test = {
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mmap04.c: Avoid vma merging

[Martin Doucha]

Hi,
some comments below.

On 23. 01. 24 17:55, Avinesh Kumar wrote:
> We hit a scenario where new mapping was merged with existing mapping of
> same permission and the return address from the mmap was hidden in the
> merged mapping in /proc/self/maps, causing the test to fail.
> To avoid this, we first  create a 2-page mapping with the different
> permissions, and then remap the 2nd page with the perms being tested.
> 
> Signed-off-by: Avinesh Kumar <akumar@suse.de>
> Reported-by: Martin Doucha <mdoucha@suse.cz>
> ---
>   testcases/kernel/syscalls/mmap/mmap04.c | 49 +++++++++++++++----------
>   1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/mmap/mmap04.c b/testcases/kernel/syscalls/mmap/mmap04.c
> index f6f4f7c98..f0f87b7f5 100644
> --- a/testcases/kernel/syscalls/mmap/mmap04.c
> +++ b/testcases/kernel/syscalls/mmap/mmap04.c
> @@ -17,28 +17,28 @@
>   #include "tst_test.h"
>   #include <stdio.h>
>   
> -#define MMAPSIZE 1024
> -static char *addr;
> +static char *addr1;
> +static char *addr2;
>   
>   static struct tcase {
>   	int prot;
>   	int flags;
>   	char *exp_perms;
>   } tcases[] = {
> -	{PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, "---p"},
> -	{PROT_NONE, MAP_ANONYMOUS | MAP_SHARED, "---s"},
> -	{PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, "r--p"},
> -	{PROT_READ, MAP_ANONYMOUS | MAP_SHARED, "r--s"},
> -	{PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, "-w-p"},
> -	{PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED, "-w-s"},
> -	{PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, "rw-p"},
> -	{PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED, "rw-s"},
> -	{PROT_READ | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE, "r-xp"},
> -	{PROT_READ | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED, "r-xs"},
> -	{PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE, "-wxp"},
> -	{PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED, "-wxs"},
> -	{PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE, "rwxp"},
> -	{PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED, "rwxs"}
> +	{PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "---p"},
> +	{PROT_NONE, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "---s"},
> +	{PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "r--p"},
> +	{PROT_READ, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "r--s"},
> +	{PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "-w-p"},
> +	{PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "-w-s"},
> +	{PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "rw-p"},
> +	{PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "rw-s"},
> +	{PROT_READ | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "r-xp"},
> +	{PROT_READ | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "r-xs"},
> +	{PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "-wxp"},
> +	{PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "-wxs"},
> +	{PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "rwxp"},
> +	{PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "rwxs"}

The MAP_FIXED flag doesn't belong in the testcases, it should be added 
in the mmap() call instead: SAFE_MMAP(..., tc->flags | MAP_FIXED, ...);
It's an implementation detail not related to the testcases themselves. 
You don't want to rewrite all the test cases again if we decide to not 
use MAP_FIXED for whatever reason in the future.

>   };
>   
>   static void run(unsigned int i)
> @@ -47,10 +47,21 @@ static void run(unsigned int i)
>   	char addr_str[20];
>   	char perms[8];
>   	char fmt[1024];
> +	unsigned int pagesize;
>   
> -	addr = SAFE_MMAP(NULL, MMAPSIZE, tc->prot, tc->flags, -1, 0);
> +	pagesize = SAFE_SYSCONF(_SC_PAGESIZE);
>   
> -	sprintf(addr_str, "%" PRIxPTR, (uintptr_t)addr);
> +	/* To avoid new mapping getting merged with existing mappings, we first
> +	   create a 2-page mapping with the different permissions, and then remap
> +	   the 2nd page with the perms being tested. */
> +	if ((tc->prot == PROT_NONE) && (tc->flags == (MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED)))
> +		addr1 = SAFE_MMAP(NULL, pagesize * 2, PROT_READ, MAP_ANONYMOUS | MAP_SHARED, -1, 0);
> +	else
> +		addr1 = SAFE_MMAP(NULL, pagesize * 2, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);

This would be cleaner (just invert the shared/private flag):
int flags = (tc->flags & MAP_PRIVATE) ? MAP_SHARED : MAP_PRIVATE;
addr1 = SAFE_MMAP(NULL, pagesize * 2, PROT_NONE, MAP_ANONYMOUS | flags, 
-1, 0);

> +
> +	addr2 = SAFE_MMAP(addr1 + pagesize, pagesize, tc->prot, tc->flags, -1, 0);
> +
> +	sprintf(addr_str, "%" PRIxPTR, (uintptr_t)addr2);

Why not merge the two sprintf()s into one?
sprintf(fmt, "%" PRIxPTR "-%%*x %%s", (uintptr_t)addr2);

>   	sprintf(fmt, "%s-%%*x %%s", addr_str);
>   	SAFE_FILE_LINES_SCANF("/proc/self/maps", fmt, perms);
>   
> @@ -61,7 +72,7 @@ static void run(unsigned int i)
>   						tc->exp_perms, perms);
>   	}
>   
> -	SAFE_MUNMAP(addr, MMAPSIZE);
> +	SAFE_MUNMAP(addr1, pagesize * 2);
>   }
>   
>   static struct tst_test test = {

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2] mmap04.c: Avoid vma merging

[Avinesh Kumar]

We hit a scenario where new mapping was merged with existing mapping of
same permission and the return address from the mmap was hidden in the
merged mapping in /proc/self/maps, causing the test to fail.
To avoid this, we first  create a 2-page mapping with the different
permissions, and then remap the 2nd page with the perms being tested.

Signed-off-by: Avinesh Kumar <akumar@suse.de>
Reported-by: Martin Doucha <mdoucha@suse.cz>
Signed-off-by: Avinesh Kumar <akumar@suse.de>
---
 testcases/kernel/syscalls/mmap/mmap04.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/testcases/kernel/syscalls/mmap/mmap04.c b/testcases/kernel/syscalls/mmap/mmap04.c
index f6f4f7c98..fa85deed1 100644
--- a/testcases/kernel/syscalls/mmap/mmap04.c
+++ b/testcases/kernel/syscalls/mmap/mmap04.c
@@ -17,8 +17,8 @@
 #include "tst_test.h"
 #include <stdio.h>
 
-#define MMAPSIZE 1024
-static char *addr;
+static char *addr1;
+static char *addr2;
 
 static struct tcase {
 	int prot;
@@ -44,14 +44,23 @@ static struct tcase {
 static void run(unsigned int i)
 {
 	struct tcase *tc = &tcases[i];
-	char addr_str[20];
 	char perms[8];
 	char fmt[1024];
+	unsigned int pagesize;
+	int flag;
 
-	addr = SAFE_MMAP(NULL, MMAPSIZE, tc->prot, tc->flags, -1, 0);
+	pagesize = SAFE_SYSCONF(_SC_PAGESIZE);
 
-	sprintf(addr_str, "%" PRIxPTR, (uintptr_t)addr);
-	sprintf(fmt, "%s-%%*x %%s", addr_str);
+	/* To avoid new mapping getting merged with existing mappings, we first
+	 * create a 2-page mapping with the different permissions, and then remap
+	 * the 2nd page with the perms being tested.
+	 */
+	flag = (tc->flags & MAP_PRIVATE) ? MAP_SHARED : MAP_PRIVATE;
+	addr1 = SAFE_MMAP(NULL, pagesize * 2, PROT_NONE, MAP_ANONYMOUS | flag, -1, 0);
+
+	addr2 = SAFE_MMAP(addr1 + pagesize, pagesize, tc->prot, tc->flags | MAP_FIXED, -1, 0);
+
+	sprintf(fmt, "%" PRIxPTR "-%%*x %%s", (uintptr_t)addr2);
 	SAFE_FILE_LINES_SCANF("/proc/self/maps", fmt, perms);
 
 	if (!strcmp(perms, tc->exp_perms)) {
@@ -61,7 +70,7 @@ static void run(unsigned int i)
 						tc->exp_perms, perms);
 	}
 
-	SAFE_MUNMAP(addr, MMAPSIZE);
+	SAFE_MUNMAP(addr1, pagesize * 2);
 }
 
 static struct tst_test test = {
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mmap04.c: Avoid vma merging

[Avinesh Kumar]

Hi Martin,

On Wednesday, January 24, 2024 12:56:58 PM CET Martin Doucha wrote:
> Hi,
> some comments below.
> 
> On 23. 01. 24 17:55, Avinesh Kumar wrote:
> > We hit a scenario where new mapping was merged with existing mapping of
> > same permission and the return address from the mmap was hidden in the
> > merged mapping in /proc/self/maps, causing the test to fail.
> > To avoid this, we first  create a 2-page mapping with the different
> > permissions, and then remap the 2nd page with the perms being tested.
> > 
> > Signed-off-by: Avinesh Kumar <akumar@suse.de>
> > Reported-by: Martin Doucha <mdoucha@suse.cz>
> > ---
> > 
> >   testcases/kernel/syscalls/mmap/mmap04.c | 49 +++++++++++++++----------
> >   1 file changed, 30 insertions(+), 19 deletions(-)
> > 
> > diff --git a/testcases/kernel/syscalls/mmap/mmap04.c
> > b/testcases/kernel/syscalls/mmap/mmap04.c index f6f4f7c98..f0f87b7f5
> > 100644
> > --- a/testcases/kernel/syscalls/mmap/mmap04.c
> > +++ b/testcases/kernel/syscalls/mmap/mmap04.c
> > @@ -17,28 +17,28 @@
> > 
> >   #include "tst_test.h"
> >   #include <stdio.h>
> > 
> > -#define MMAPSIZE 1024
> > -static char *addr;
> > +static char *addr1;
> > +static char *addr2;
> > 
> >   static struct tcase {
> >   
> >   	int prot;
> >   	int flags;
> >   	char *exp_perms;
> >   
> >   } tcases[] = {
> > 
> > -	{PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, "---p"},
> > -	{PROT_NONE, MAP_ANONYMOUS | MAP_SHARED, "---s"},
> > -	{PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, "r--p"},
> > -	{PROT_READ, MAP_ANONYMOUS | MAP_SHARED, "r--s"},
> > -	{PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, "-w-p"},
> > -	{PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED, "-w-s"},
> > -	{PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, "rw-p"},
> > -	{PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED, "rw-s"},
> > -	{PROT_READ | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE, "r-xp"},
> > -	{PROT_READ | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED, "r-xs"},
> > -	{PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE, "-wxp"},
> > -	{PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED, "-wxs"},
> > -	{PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE,
> > "rwxp"}, -	{PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS |
> > MAP_SHARED, "rwxs"} +	{PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE |
> > MAP_FIXED, "---p"},
> > +	{PROT_NONE, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "---s"},
> > +	{PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "r--p"},
> > +	{PROT_READ, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "r--s"},
> > +	{PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "-w-p"},
> > +	{PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "-w-s"},
> > +	{PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED,
> > "rw-p"}, +	{PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED |
> > MAP_FIXED, "rw-s"}, +	{PROT_READ | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE
> > | MAP_FIXED, "r-xp"}, +	{PROT_READ | PROT_EXEC, MAP_ANONYMOUS |
> > MAP_SHARED | MAP_FIXED, "r-xs"}, +	{PROT_WRITE | PROT_EXEC, 
MAP_ANONYMOUS
> > | MAP_PRIVATE | MAP_FIXED, "-wxp"}, +	{PROT_WRITE | PROT_EXEC,
> > MAP_ANONYMOUS | MAP_SHARED | MAP_FIXED, "-wxs"}, +	{PROT_READ |
> > PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, "rwxp"},
> > +	{PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANONYMOUS | MAP_SHARED |
> > MAP_FIXED, "rwxs"}
> The MAP_FIXED flag doesn't belong in the testcases, it should be added
> in the mmap() call instead: SAFE_MMAP(..., tc->flags | MAP_FIXED, ...);
> It's an implementation detail not related to the testcases themselves.
> You don't want to rewrite all the test cases again if we decide to not
> use MAP_FIXED for whatever reason in the future.
> 
Thank you for review and all the corrections/suggestions. I have send the 
updated patch.

> >   };
> >   
> >   static void run(unsigned int i)
> > 
> > @@ -47,10 +47,21 @@ static void run(unsigned int i)
> > 
> >   	char addr_str[20];
> >   	char perms[8];
> >   	char fmt[1024];
> > 
> > +	unsigned int pagesize;
> > 
> > -	addr = SAFE_MMAP(NULL, MMAPSIZE, tc->prot, tc->flags, -1, 0);
> > +	pagesize = SAFE_SYSCONF(_SC_PAGESIZE);
> > 
> > -	sprintf(addr_str, "%" PRIxPTR, (uintptr_t)addr);
> > +	/* To avoid new mapping getting merged with existing mappings, we 
first
> > +	   create a 2-page mapping with the different permissions, and then
> > remap
> > +	   the 2nd page with the perms being tested. */
> > +	if ((tc->prot == PROT_NONE) && (tc->flags == (MAP_ANONYMOUS |
> > MAP_PRIVATE | MAP_FIXED))) +		addr1 = SAFE_MMAP(NULL, 
pagesize * 2,
> > PROT_READ, MAP_ANONYMOUS | MAP_SHARED, -1, 0); +	else
> > +		addr1 = SAFE_MMAP(NULL, pagesize * 2, PROT_NONE, 
MAP_ANONYMOUS |
> > MAP_PRIVATE, -1, 0);
> This would be cleaner (just invert the shared/private flag):
> int flags = (tc->flags & MAP_PRIVATE) ? MAP_SHARED : MAP_PRIVATE;
> addr1 = SAFE_MMAP(NULL, pagesize * 2, PROT_NONE, MAP_ANONYMOUS | flags,
> -1, 0);
> 
> > +
> > +	addr2 = SAFE_MMAP(addr1 + pagesize, pagesize, tc->prot, tc->flags, 
-1,
> > 0); +
> > +	sprintf(addr_str, "%" PRIxPTR, (uintptr_t)addr2);
> 
> Why not merge the two sprintf()s into one?
> sprintf(fmt, "%" PRIxPTR "-%%*x %%s", (uintptr_t)addr2);
> 
> >   	sprintf(fmt, "%s-%%*x %%s", addr_str);
> >   	SAFE_FILE_LINES_SCANF("/proc/self/maps", fmt, perms);
> > 
> > @@ -61,7 +72,7 @@ static void run(unsigned int i)
> > 
> >   						tc-
>exp_perms, perms);
> >   	
> >   	}
> > 
> > -	SAFE_MUNMAP(addr, MMAPSIZE);
> > +	SAFE_MUNMAP(addr1, pagesize * 2);
> > 
> >   }
> >   
> >   static struct tst_test test = {




-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] mmap04.c: Avoid vma merging

[Martin Doucha]

Hi,
Reviewed-by: Martin Doucha <mdoucha@suse.cz>

On 24. 01. 24 14:26, Avinesh Kumar wrote:
> We hit a scenario where new mapping was merged with existing mapping of
> same permission and the return address from the mmap was hidden in the
> merged mapping in /proc/self/maps, causing the test to fail.
> To avoid this, we first  create a 2-page mapping with the different
> permissions, and then remap the 2nd page with the perms being tested.
> 
> Signed-off-by: Avinesh Kumar <akumar@suse.de>
> Reported-by: Martin Doucha <mdoucha@suse.cz>
> Signed-off-by: Avinesh Kumar <akumar@suse.de>
> ---
>   testcases/kernel/syscalls/mmap/mmap04.c | 23 ++++++++++++++++-------
>   1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/mmap/mmap04.c b/testcases/kernel/syscalls/mmap/mmap04.c
> index f6f4f7c98..fa85deed1 100644
> --- a/testcases/kernel/syscalls/mmap/mmap04.c
> +++ b/testcases/kernel/syscalls/mmap/mmap04.c
> @@ -17,8 +17,8 @@
>   #include "tst_test.h"
>   #include <stdio.h>
>   
> -#define MMAPSIZE 1024
> -static char *addr;
> +static char *addr1;
> +static char *addr2;
>   
>   static struct tcase {
>   	int prot;
> @@ -44,14 +44,23 @@ static struct tcase {
>   static void run(unsigned int i)
>   {
>   	struct tcase *tc = &tcases[i];
> -	char addr_str[20];
>   	char perms[8];
>   	char fmt[1024];
> +	unsigned int pagesize;
> +	int flag;
>   
> -	addr = SAFE_MMAP(NULL, MMAPSIZE, tc->prot, tc->flags, -1, 0);
> +	pagesize = SAFE_SYSCONF(_SC_PAGESIZE);
>   
> -	sprintf(addr_str, "%" PRIxPTR, (uintptr_t)addr);
> -	sprintf(fmt, "%s-%%*x %%s", addr_str);
> +	/* To avoid new mapping getting merged with existing mappings, we first
> +	 * create a 2-page mapping with the different permissions, and then remap
> +	 * the 2nd page with the perms being tested.
> +	 */
> +	flag = (tc->flags & MAP_PRIVATE) ? MAP_SHARED : MAP_PRIVATE;
> +	addr1 = SAFE_MMAP(NULL, pagesize * 2, PROT_NONE, MAP_ANONYMOUS | flag, -1, 0);
> +
> +	addr2 = SAFE_MMAP(addr1 + pagesize, pagesize, tc->prot, tc->flags | MAP_FIXED, -1, 0);
> +
> +	sprintf(fmt, "%" PRIxPTR "-%%*x %%s", (uintptr_t)addr2);
>   	SAFE_FILE_LINES_SCANF("/proc/self/maps", fmt, perms);
>   
>   	if (!strcmp(perms, tc->exp_perms)) {
> @@ -61,7 +70,7 @@ static void run(unsigned int i)
>   						tc->exp_perms, perms);
>   	}
>   
> -	SAFE_MUNMAP(addr, MMAPSIZE);
> +	SAFE_MUNMAP(addr1, pagesize * 2);
>   }
>   
>   static struct tst_test test = {

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] mmap04.c: Avoid vma merging

[Petr Vorel]

Hi Avinesh, Martin,

> Hi,
> Reviewed-by: Martin Doucha <mdoucha@suse.cz>

> On 24. 01. 24 14:26, Avinesh Kumar wrote:
> > We hit a scenario where new mapping was merged with existing mapping of
> > same permission and the return address from the mmap was hidden in the
> > merged mapping in /proc/self/maps, causing the test to fail.
> > To avoid this, we first  create a 2-page mapping with the different
> > permissions, and then remap the 2nd page with the perms being tested.

> > Signed-off-by: Avinesh Kumar <akumar@suse.de>
> > Reported-by: Martin Doucha <mdoucha@suse.cz>
> > Signed-off-by: Avinesh Kumar <akumar@suse.de>
> > ---
> >   testcases/kernel/syscalls/mmap/mmap04.c | 23 ++++++++++++++++-------
> >   1 file changed, 16 insertions(+), 7 deletions(-)

> > diff --git a/testcases/kernel/syscalls/mmap/mmap04.c b/testcases/kernel/syscalls/mmap/mmap04.c
> > index f6f4f7c98..fa85deed1 100644
> > --- a/testcases/kernel/syscalls/mmap/mmap04.c
> > +++ b/testcases/kernel/syscalls/mmap/mmap04.c
> > @@ -17,8 +17,8 @@
> >   #include "tst_test.h"
> >   #include <stdio.h>
> > -#define MMAPSIZE 1024
> > -static char *addr;
> > +static char *addr1;
> > +static char *addr2;
> >   static struct tcase {
> >   	int prot;
> > @@ -44,14 +44,23 @@ static struct tcase {
> >   static void run(unsigned int i)
> >   {
> >   	struct tcase *tc = &tcases[i];
> > -	char addr_str[20];
> >   	char perms[8];
> >   	char fmt[1024];
> > +	unsigned int pagesize;
> > +	int flag;
> > -	addr = SAFE_MMAP(NULL, MMAPSIZE, tc->prot, tc->flags, -1, 0);
> > +	pagesize = SAFE_SYSCONF(_SC_PAGESIZE);
> > -	sprintf(addr_str, "%" PRIxPTR, (uintptr_t)addr);
> > -	sprintf(fmt, "%s-%%*x %%s", addr_str);
> > +	/* To avoid new mapping getting merged with existing mappings, we first
> > +	 * create a 2-page mapping with the different permissions, and then remap
> > +	 * the 2nd page with the perms being tested.
> > +	 */
> > +	flag = (tc->flags & MAP_PRIVATE) ? MAP_SHARED : MAP_PRIVATE;
> > +	addr1 = SAFE_MMAP(NULL, pagesize * 2, PROT_NONE, MAP_ANONYMOUS | flag, -1, 0);
> > +
> > +	addr2 = SAFE_MMAP(addr1 + pagesize, pagesize, tc->prot, tc->flags | MAP_FIXED, -1, 0);
> > +
> > +	sprintf(fmt, "%" PRIxPTR "-%%*x %%s", (uintptr_t)addr2);
> >   	SAFE_FILE_LINES_SCANF("/proc/self/maps", fmt, perms);
> >   	if (!strcmp(perms, tc->exp_perms)) {
> > @@ -61,7 +70,7 @@ static void run(unsigned int i)
> >   						tc->exp_perms, perms);
> >   	}
> > -	SAFE_MUNMAP(addr, MMAPSIZE);
> > +	SAFE_MUNMAP(addr1, pagesize * 2);

Shouldn't there be also second munmap()?
SAFE_MUNMAP(addr2, pagesize);

Kind regards,
Petr

> >   }
> >   static struct tst_test test = {

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] mmap04.c: Avoid vma merging

[Avinesh Kumar]

Hi Petr,

On Wednesday, January 24, 2024 6:05:47 PM CET Petr Vorel wrote:
> Hi Avinesh, Martin,
> 
> > Hi,
> > Reviewed-by: Martin Doucha <mdoucha@suse.cz>
> > 
> > On 24. 01. 24 14:26, Avinesh Kumar wrote:
> > > We hit a scenario where new mapping was merged with existing mapping of
> > > same permission and the return address from the mmap was hidden in the
> > > merged mapping in /proc/self/maps, causing the test to fail.
> > > To avoid this, we first  create a 2-page mapping with the different
> > > permissions, and then remap the 2nd page with the perms being tested.
> > > 
> > > Signed-off-by: Avinesh Kumar <akumar@suse.de>
> > > Reported-by: Martin Doucha <mdoucha@suse.cz>
> > > Signed-off-by: Avinesh Kumar <akumar@suse.de>
> > > ---
> > > 
> > >   testcases/kernel/syscalls/mmap/mmap04.c | 23 ++++++++++++++++-------
> > >   1 file changed, 16 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/testcases/kernel/syscalls/mmap/mmap04.c
> > > b/testcases/kernel/syscalls/mmap/mmap04.c index f6f4f7c98..fa85deed1
> > > 100644
> > > --- a/testcases/kernel/syscalls/mmap/mmap04.c
> > > +++ b/testcases/kernel/syscalls/mmap/mmap04.c
> > > @@ -17,8 +17,8 @@
> > > 
> > >   #include "tst_test.h"
> > >   #include <stdio.h>
> > > 
> > > -#define MMAPSIZE 1024
> > > -static char *addr;
> > > +static char *addr1;
> > > +static char *addr2;
> > > 
> > >   static struct tcase {
> > >   
> > >   	int prot;
> > > 
> > > @@ -44,14 +44,23 @@ static struct tcase {
> > > 
> > >   static void run(unsigned int i)
> > >   {
> > >   
> > >   	struct tcase *tc = &tcases[i];
> > > 
> > > -	char addr_str[20];
> > > 
> > >   	char perms[8];
> > >   	char fmt[1024];
> > > 
> > > +	unsigned int pagesize;
> > > +	int flag;
> > > -	addr = SAFE_MMAP(NULL, MMAPSIZE, tc->prot, tc->flags, -1, 0);
> > > +	pagesize = SAFE_SYSCONF(_SC_PAGESIZE);
> > > -	sprintf(addr_str, "%" PRIxPTR, (uintptr_t)addr);
> > > -	sprintf(fmt, "%s-%%*x %%s", addr_str);
> > > +	/* To avoid new mapping getting merged with existing mappings, we
> > > first
> > > +	 * create a 2-page mapping with the different permissions, and then
> > > remap
> > > +	 * the 2nd page with the perms being tested.
> > > +	 */
> > > +	flag = (tc->flags & MAP_PRIVATE) ? MAP_SHARED : MAP_PRIVATE;
> > > +	addr1 = SAFE_MMAP(NULL, pagesize * 2, PROT_NONE, MAP_ANONYMOUS | 
flag,
> > > -1, 0); +
> > > +	addr2 = SAFE_MMAP(addr1 + pagesize, pagesize, tc->prot, tc->flags |
> > > MAP_FIXED, -1, 0); +
> > > +	sprintf(fmt, "%" PRIxPTR "-%%*x %%s", (uintptr_t)addr2);
> > > 
> > >   	SAFE_FILE_LINES_SCANF("/proc/self/maps", fmt, perms);
> > >   	if (!strcmp(perms, tc->exp_perms)) {
> > > 
> > > @@ -61,7 +70,7 @@ static void run(unsigned int i)
> > > 
> > >   						tc-
>exp_perms, perms);
> > >   	
> > >   	}
> > > 
> > > -	SAFE_MUNMAP(addr, MMAPSIZE);
> > > +	SAFE_MUNMAP(addr1, pagesize * 2);
> 
> Shouldn't there be also second munmap()?
> SAFE_MUNMAP(addr2, pagesize);
No, we are unmapping both the mappings ( 2 pages ) together.

Regards,
Avinesh

> 
> Kind regards,
> Petr
> 
> > >   }
> > >   static struct tst_test test = {





-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] mmap04.c: Avoid vma merging

[Petr Vorel]

Hi Avinesh, Martin,

> > Shouldn't there be also second munmap()?
> > SAFE_MUNMAP(addr2, pagesize);
> No, we are unmapping both the mappings ( 2 pages ) together.

Ah, thanks!
Merged.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp