| Message ID | 20230404112852.1271969-1-pvorel@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/1] mmap20: Fix invalid address | expand |
On Tue, Apr 4, 2023 at 1:29 PM Petr Vorel <pvorel@suse.cz> wrote: > > 1 << 7 (0x80) is defined as MAP_LOCKED on ppc64le and mips. > Use 1 << 9 (0x200), which looks to be unused on all archs. > > Fixes: 2764e9e6a ("mmap20: New test for mmap() with MAP_SHARED_VALIDATE") > > Signed-off-by: Petr Vorel <pvorel@suse.cz> Should work. Alternative is we don't map a file, and then add MAP_SYNC to flags, which looking at LEGACY_MAP_MASK should also give us -EOPNOTSUPP. I'd go with the patch, we can tweak it later if someone has better idea: Acked-by: Jan Stancek <jstancek@redhat.com> > --- > NOTE: (1 << 10) would work as well. > > testcases/kernel/syscalls/mmap/mmap20.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/testcases/kernel/syscalls/mmap/mmap20.c b/testcases/kernel/syscalls/mmap/mmap20.c > index c346df5d1..5269179e5 100644 > --- a/testcases/kernel/syscalls/mmap/mmap20.c > +++ b/testcases/kernel/syscalls/mmap/mmap20.c > @@ -20,7 +20,7 @@ > > #define TEST_FILE "file_to_mmap" > #define TEST_FILE_SIZE 1024 > -#define INVALID_FLAG (1 << 7) > +#define INVALID_FLAG (1 << 9) > > static int fd = -1; > static void *addr; > -- > 2.40.0 >
Hi Petr, (1<<9) is also used by another flag on alpha, but it said not linux-specific, so I guess it won't be a problem (at least for now). alpha/include/uapi/asm/mman.h:#define _MAP_HASSEMAPHORE 0x0200 Reviewed-by: Li Wang <liwang@redhat.com> On Tue, Apr 4, 2023 at 7:29 PM Petr Vorel <pvorel@suse.cz> wrote: > 1 << 7 (0x80) is defined as MAP_LOCKED on ppc64le and mips. > Use 1 << 9 (0x200), which looks to be unused on all archs. > > Fixes: 2764e9e6a ("mmap20: New test for mmap() with MAP_SHARED_VALIDATE") > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > NOTE: (1 << 10) would work as well. > > testcases/kernel/syscalls/mmap/mmap20.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/testcases/kernel/syscalls/mmap/mmap20.c > b/testcases/kernel/syscalls/mmap/mmap20.c > index c346df5d1..5269179e5 100644 > --- a/testcases/kernel/syscalls/mmap/mmap20.c > +++ b/testcases/kernel/syscalls/mmap/mmap20.c > @@ -20,7 +20,7 @@ > > #define TEST_FILE "file_to_mmap" > #define TEST_FILE_SIZE 1024 > -#define INVALID_FLAG (1 << 7) > +#define INVALID_FLAG (1 << 9) > > static int fd = -1; > static void *addr; > -- > 2.40.0 > >
Hi all, > Hi Petr, > (1<<9) is also used by another flag on alpha, but it said not > linux-specific, > so I guess it won't be a problem (at least for now). > alpha/include/uapi/asm/mman.h:#define _MAP_HASSEMAPHORE 0x0200 Good point! I checked only glibc/musl headers, obviously checking kernel sources is better as it contains old archs. It affects alpha, ia64 and sparc (well, they might not even compile current LTP, we were dropping ia64 fixes anyway). $ git grep -w 0x0200 arch/*/include/uapi/asm/mman.h arch/alpha/include/uapi/asm/mman.h:#define _MAP_HASSEMAPHORE 0x0200 arch/ia64/include/uapi/asm/mman.h:#define MAP_GROWSUP 0x0200 /* register stack-like segment */ arch/sparc/include/uapi/asm/mman.h:#define MAP_GROWSDOWN 0x0200 /* stack-like segment */ But as 0x400 (1 << 10) is not used anywhere, I merged it with this value. It'd be good to follow Jan's solution (don't map a file, and then add MAP_SYNC to flags), but I'll postpone it to later/when needed/never (there are other tests which needs to be fixed. Thanks a lot both! Kind regards, Petr
On Thu, Apr 6, 2023 at 5:49 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi all, > > > Hi Petr, > > > (1<<9) is also used by another flag on alpha, but it said not > > linux-specific, > > so I guess it won't be a problem (at least for now). > > alpha/include/uapi/asm/mman.h:#define _MAP_HASSEMAPHORE 0x0200 > > Good point! I checked only glibc/musl headers, obviously checking kernel > sources > is better as it contains old archs. It affects alpha, ia64 and sparc > (well, they might not even compile current LTP, we were dropping ia64 fixes > anyway). > Okay, I only noticed that alpha is not defined 0x0200 for Linux specifically. > > $ git grep -w 0x0200 arch/*/include/uapi/asm/mman.h > arch/alpha/include/uapi/asm/mman.h:#define _MAP_HASSEMAPHORE 0x0200 > arch/ia64/include/uapi/asm/mman.h:#define MAP_GROWSUP 0x0200 /* > register stack-like segment */ > arch/sparc/include/uapi/asm/mman.h:#define MAP_GROWSDOWN 0x0200 > /* stack-like segment */ > > But as 0x400 (1 << 10) is not used anywhere, I merged it with this value. > Hmm, I didn't understand here why merged with 0x0400, aren't there still duplicated flags? $ git grep -w 0x0400 arch/*/include/uapi/asm/mman.h arch/alpha/include/uapi/asm/mman.h:#define _MAP_INHERIT 0x0400 arch/mips/include/uapi/asm/mman.h:#define MAP_NORESERVE 0x0400 /* don't check for reservations */ arch/xtensa/include/uapi/asm/mman.h:#define MAP_NORESERVE 0x0400 /* don't check for reservations */ > > It'd be good to follow Jan's solution (don't map a file, and then add > MAP_SYNC > to flags), but I'll postpone it to later/when needed/never (there are other > tests which needs to be fixed. > > Thanks a lot both! > > Kind regards, > Petr > >
Hi all, > On Thu, Apr 6, 2023 at 5:49 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi all, > > > Hi Petr, > > > (1<<9) is also used by another flag on alpha, but it said not > > > linux-specific, > > > so I guess it won't be a problem (at least for now). > > > alpha/include/uapi/asm/mman.h:#define _MAP_HASSEMAPHORE 0x0200 > > Good point! I checked only glibc/musl headers, obviously checking kernel > > sources > > is better as it contains old archs. It affects alpha, ia64 and sparc > > (well, they might not even compile current LTP, we were dropping ia64 fixes > > anyway). > Okay, I only noticed that alpha is not defined 0x0200 for Linux > specifically. > > $ git grep -w 0x0200 arch/*/include/uapi/asm/mman.h > > arch/alpha/include/uapi/asm/mman.h:#define _MAP_HASSEMAPHORE 0x0200 > > arch/ia64/include/uapi/asm/mman.h:#define MAP_GROWSUP 0x0200 /* > > register stack-like segment */ > > arch/sparc/include/uapi/asm/mman.h:#define MAP_GROWSDOWN 0x0200 > > /* stack-like segment */ > > But as 0x400 (1 << 10) is not used anywhere, I merged it with this value. > Hmm, I didn't understand here why merged with 0x0400, > aren't there still duplicated flags? > $ git grep -w 0x0400 arch/*/include/uapi/asm/mman.h > arch/alpha/include/uapi/asm/mman.h:#define _MAP_INHERIT 0x0400 > arch/mips/include/uapi/asm/mman.h:#define MAP_NORESERVE 0x0400 /* > don't check for reservations */ > arch/xtensa/include/uapi/asm/mman.h:#define MAP_NORESERVE 0x0400 > /* don't check for reservations */ I'm sorry I now don't know what I greped, because there are results for 0x0400 :(. Obviously last minute change without ack is a bad practise. You're right, linux specific 0x0400 is in mips and xtensa (it's non-linux specific on alpha). OTOH ia64 and sparc define: arch/ia64/include/uapi/asm/mman.h #define MAP_GROWSUP 0x0200 /* register stack-like segment */ arch/sparc/include/uapi/asm/mman.h #define MAP_GROWSDOWN 0x0200 /* stack-like segment */ Are these used as flags? If yes none of the values is optimal as it breaks some arch. If you agree, I'd keep 0x0400 and follow Jan's suggestion (don't map a file, and then add MAP_SYNC to flags). Kind regards, Petr
On Thu, Apr 6, 2023 at 7:13 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi all, > > > On Thu, Apr 6, 2023 at 5:49 PM Petr Vorel <pvorel@suse.cz> wrote: > > > > Hi all, > > > > > Hi Petr, > > > > > (1<<9) is also used by another flag on alpha, but it said not > > > > linux-specific, > > > > so I guess it won't be a problem (at least for now). > > > > alpha/include/uapi/asm/mman.h:#define _MAP_HASSEMAPHORE 0x0200 > > > > Good point! I checked only glibc/musl headers, obviously checking > kernel > > > sources > > > is better as it contains old archs. It affects alpha, ia64 and sparc > > > (well, they might not even compile current LTP, we were dropping ia64 > fixes > > > anyway). > > > > Okay, I only noticed that alpha is not defined 0x0200 for Linux > > specifically. > > > > $ git grep -w 0x0200 arch/*/include/uapi/asm/mman.h > > > arch/alpha/include/uapi/asm/mman.h:#define _MAP_HASSEMAPHORE 0x0200 > > > arch/ia64/include/uapi/asm/mman.h:#define MAP_GROWSUP 0x0200 > /* > > > register stack-like segment */ > > > arch/sparc/include/uapi/asm/mman.h:#define MAP_GROWSDOWN 0x0200 > > > /* stack-like segment */ > > > > But as 0x400 (1 << 10) is not used anywhere, I merged it with this > value. > > > > > Hmm, I didn't understand here why merged with 0x0400, > > aren't there still duplicated flags? > > > $ git grep -w 0x0400 arch/*/include/uapi/asm/mman.h > > arch/alpha/include/uapi/asm/mman.h:#define _MAP_INHERIT 0x0400 > > arch/mips/include/uapi/asm/mman.h:#define MAP_NORESERVE 0x0400 > /* > > don't check for reservations */ > > arch/xtensa/include/uapi/asm/mman.h:#define MAP_NORESERVE 0x0400 > > /* don't check for reservations */ > > I'm sorry I now don't know what I greped, because there are results for > 0x0400 :(. > Obviously last minute change without ack is a bad practise. > > You're right, linux specific 0x0400 is in mips and xtensa (it's non-linux > specific on alpha). OTOH ia64 and sparc define: > > arch/ia64/include/uapi/asm/mman.h > #define MAP_GROWSUP 0x0200 /* register stack-like segment */ > > arch/sparc/include/uapi/asm/mman.h > #define MAP_GROWSDOWN 0x0200 /* stack-like segment */ > > Are these used as flags? If yes none of the values is optimal as it breaks > some > The answer is yes, MAP_GROWSDOWN/GROWSUP is widely used for stack growth direction. It is hard to choose an unused value for INVALID_FLAG, as it there possibly be duplicated in the future we don't know. > arch. If you agree, I'd keep 0x0400 and follow Jan's suggestion (don't map > a > file, and then add MAP_SYNC to flags). > Add MAP_SYNC sounds like a good practice, we can have a try.
diff --git a/testcases/kernel/syscalls/mmap/mmap20.c b/testcases/kernel/syscalls/mmap/mmap20.c index c346df5d1..5269179e5 100644 --- a/testcases/kernel/syscalls/mmap/mmap20.c +++ b/testcases/kernel/syscalls/mmap/mmap20.c @@ -20,7 +20,7 @@ #define TEST_FILE "file_to_mmap" #define TEST_FILE_SIZE 1024 -#define INVALID_FLAG (1 << 7) +#define INVALID_FLAG (1 << 9) static int fd = -1; static void *addr;
1 << 7 (0x80) is defined as MAP_LOCKED on ppc64le and mips. Use 1 << 9 (0x200), which looks to be unused on all archs. Fixes: 2764e9e6a ("mmap20: New test for mmap() with MAP_SHARED_VALIDATE") Signed-off-by: Petr Vorel <pvorel@suse.cz> --- NOTE: (1 << 10) would work as well. testcases/kernel/syscalls/mmap/mmap20.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)