* [LTP] [PATCH] mmap04.c: Avoid vma merging @ 2024-01-23 16:55 Avinesh Kumar 2024-01-24 11:56 ` Martin Doucha 0 siblings, 1 reply; 8+ messages in thread From: Avinesh Kumar @ 2024-01-23 16:55 UTC (permalink / raw) To: mdoucha, ltp 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [LTP] [PATCH] mmap04.c: Avoid vma merging 2024-01-23 16:55 [LTP] [PATCH] mmap04.c: Avoid vma merging Avinesh Kumar @ 2024-01-24 11:56 ` Martin Doucha 2024-01-24 13:26 ` [LTP] [PATCH v2] " Avinesh Kumar 2024-01-24 14:36 ` [LTP] [PATCH] " Avinesh Kumar 0 siblings, 2 replies; 8+ messages in thread From: Martin Doucha @ 2024-01-24 11:56 UTC (permalink / raw) To: Avinesh Kumar, ltp 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [LTP] [PATCH v2] mmap04.c: Avoid vma merging 2024-01-24 11:56 ` Martin Doucha @ 2024-01-24 13:26 ` Avinesh Kumar 2024-01-24 16:23 ` Martin Doucha 2024-01-24 14:36 ` [LTP] [PATCH] " Avinesh Kumar 1 sibling, 1 reply; 8+ messages in thread From: Avinesh Kumar @ 2024-01-24 13:26 UTC (permalink / raw) To: ltp 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [LTP] [PATCH v2] mmap04.c: Avoid vma merging 2024-01-24 13:26 ` [LTP] [PATCH v2] " Avinesh Kumar @ 2024-01-24 16:23 ` Martin Doucha 2024-01-24 17:05 ` Petr Vorel 0 siblings, 1 reply; 8+ messages in thread From: Martin Doucha @ 2024-01-24 16:23 UTC (permalink / raw) To: Avinesh Kumar, ltp 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [LTP] [PATCH v2] mmap04.c: Avoid vma merging 2024-01-24 16:23 ` Martin Doucha @ 2024-01-24 17:05 ` Petr Vorel 2024-01-25 8:14 ` Avinesh Kumar 0 siblings, 1 reply; 8+ messages in thread From: Petr Vorel @ 2024-01-24 17:05 UTC (permalink / raw) To: Martin Doucha; +Cc: ltp 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [LTP] [PATCH v2] mmap04.c: Avoid vma merging 2024-01-24 17:05 ` Petr Vorel @ 2024-01-25 8:14 ` Avinesh Kumar 2024-01-25 8:25 ` Petr Vorel 0 siblings, 1 reply; 8+ messages in thread From: Avinesh Kumar @ 2024-01-25 8:14 UTC (permalink / raw) To: Petr Vorel; +Cc: ltp 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [LTP] [PATCH v2] mmap04.c: Avoid vma merging 2024-01-25 8:14 ` Avinesh Kumar @ 2024-01-25 8:25 ` Petr Vorel 0 siblings, 0 replies; 8+ messages in thread From: Petr Vorel @ 2024-01-25 8:25 UTC (permalink / raw) To: Avinesh Kumar; +Cc: ltp 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [LTP] [PATCH] mmap04.c: Avoid vma merging 2024-01-24 11:56 ` Martin Doucha 2024-01-24 13:26 ` [LTP] [PATCH v2] " Avinesh Kumar @ 2024-01-24 14:36 ` Avinesh Kumar 1 sibling, 0 replies; 8+ messages in thread From: Avinesh Kumar @ 2024-01-24 14:36 UTC (permalink / raw) To: Martin Doucha; +Cc: ltp 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-01-25 8:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-23 16:55 [LTP] [PATCH] mmap04.c: Avoid vma merging Avinesh Kumar 2024-01-24 11:56 ` Martin Doucha 2024-01-24 13:26 ` [LTP] [PATCH v2] " Avinesh Kumar 2024-01-24 16:23 ` Martin Doucha 2024-01-24 17:05 ` Petr Vorel 2024-01-25 8:14 ` Avinesh Kumar 2024-01-25 8:25 ` Petr Vorel 2024-01-24 14:36 ` [LTP] [PATCH] " Avinesh Kumar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox