Preventing overlap between arch mman.h and asm-generic/mman.h
Subsystems (6)
RFC 1/1 Preventing overlap between arch mman.h and asm-generic/mman.h
David,
Thanks for the great UAPI work. While working on glibc mman.h sync I had the chance to look more closely at some of the UAPI headers.
Currently (3.7.0-rc6) sparc, powerpc, and tile are including mman-common.h, but nothing in mman-common.h helps prevent the accidental definition of an overlapping new constant value.
For example in mman.h powerpc defines:
#define MAP_NORESERVE 0x40 /* don't reserve swap pages */
#define MAP_LOCKED 0x80
If someone were to add a new MAP_* value in mman-common.h at 0x40 it would conflict with the existing powerpc value. The powerpc maintainers might not notice, and that's a bug.
I notice that someone is trying to avoid this kind of bug, because for example the new MADV_HWPOISON was chosen as 100, which is higher than any other MADV_* value for any architecture.
Rather than avoid the problem by spot-checking, would it be acceptable to merge in some private macros that block out exactly which constants are already in use as existing values for arches using UAPI headers?
Doing it this way would allow for the merged arches to more safely include the asm-generic version of mman.h and start benefiting from new constants without the usual maintenance cost (not to mention everyone would start in lock-step going forward). Not to mention it simplifies some of the glibc maintenance.
Rather than just talk about it here's a back-of-the-napkin sketch and a patch:
* Merges asm-generic/mman-common.h back into asm-generic/mman.h
* Include asm-generic/mman.h instead of asm-generic/mman-common.h for powerpc, sparc, tile and parisc.
* Delete mman-common.h.
* Use internal placeholder macros for all existing uses of the various constants in mman.h to avoid future overlap.
Is it only worth doing if *all* arches are included in the cleanup?
Is it worth loosing some bits for the benefit in maintenance (note that bits can be got back through #undef reallocation in the arch header)?
Is this too ugly for the benefit?
Comments?
Notes:
* Partly verified using a set of scripts to automate checking for macro defines matching regexp's and constant values.
* Build tested on parisc.
arch/parisc/include/uapi/asm/mman.h | 52 ++++++----------- arch/powerpc/include/uapi/asm/mman.h | 16 +---- arch/sparc/include/uapi/asm/mman.h | 18 ++--- arch/tile/include/uapi/asm/mman.h | 22 ++----- include/uapi/asm-generic/mman-common.h | 58 ------------------- include/uapi/asm-generic/mman.h | 76 ++++++++++++++++++++++++- 6 files changed, 118 insertions(+), 124 deletions(-)
arch/parisc/include/uapi/asm/mman.h +20 -32
@@ -1,24 +1,30 @@#ifndef __PARISC_MMAN_H__#define __PARISC_MMAN_H__ -#define PROT_READ 0x1/* page can be read */ -#define PROT_WRITE 0x2/* page can be written */ -#define PROT_EXEC 0x4/* page can be executed */ -#define PROT_SEM 0x8/* page may be used for atomic ops */ -#define PROT_NONE 0x0/* page can not be accessed */ -#define PROT_GROWSDOWN 0x01000000/* mprotect flag: extend change to start of growsdown vma */ -#define PROT_GROWSUP 0x02000000/* mprotect flag: extend change to end of growsup vma */ +#include <asm-generic/mman.h> + +/* The following constants don't match mman.h. */ +#undef MAP_TYPE +#undef MAP_FIXED +#undef MAP_ANONYMOUS +#undef MAP_GROWSDOWN +#undef MAP_POPULATE +#undef MAP_NONBLOCK +#undef MAP_STACK +#undef MAP_HUGETLB +#undef MS_ASYNC +#undef MS_INVALIDATE +#undef MS_SYNC +#undef MADV_MERGEABLE +#undef MADV_UNMERGEABLE +#undef MADV_HUGEPAGE +#undef MADV_NOHUGEPAGE +#undef MADV_DONTDUMP +#undef MADV_DODUMP -#define MAP_SHARED 0x01/* Share changes */ -#define MAP_PRIVATE 0x02/* Changes are private */ #define MAP_TYPE 0x03/* Mask for type of mapping */ #define MAP_FIXED 0x04/* Interpret addr exactly */ #define MAP_ANONYMOUS 0x10/* don't use a file */ - -#define MAP_DENYWRITE 0x0800/* ETXTBSY */ -#define MAP_EXECUTABLE 0x1000/* mark it as an executable */ -#define MAP_LOCKED 0x2000/* pages are locked */ -#define MAP_NORESERVE 0x4000/* don't check for reservations */ #define MAP_GROWSDOWN 0x8000/* stack-like segment */ #define MAP_POPULATE 0x10000/* populate (prefault) pagetables */ #define MAP_NONBLOCK 0x20000/* do not block on IO */
@@ -29,23 +35,9 @@#define MS_ASYNC 2/* sync memory asynchronously */ #define MS_INVALIDATE 4/* invalidate the caches */ -#define MCL_CURRENT 1/* lock all current mappings */ -#define MCL_FUTURE 2/* lock all future mappings */ - -#define MADV_NORMAL 0/* no further special treatment */ -#define MADV_RANDOM 1/* expect random page references */ -#define MADV_SEQUENTIAL 2/* expect sequential page references */ -#define MADV_WILLNEED 3/* will need these pages */ -#define MADV_DONTNEED 4/* don't need these pages */ #define MADV_SPACEAVAIL 5/* insure that resources are reserved */ #define MADV_VPS_PURGE 6/* Purge pages from VM page cache */ #define MADV_VPS_INHERIT 7/* Inherit parents page size */ - -/* common/generic parameters */ -#define MADV_REMOVE 9/* remove these pages & resources */ -#define MADV_DONTFORK 10/* don't inherit across fork */ -#define MADV_DOFORK 11/* do inherit across fork */ -/* The range 12-64 is reserved for page size specification. */ #define MADV_4K_PAGES 12/* Use 4K pages */ #define MADV_16K_PAGES 14/* Use 16K pages */
@@ -55,19 +47,15 @@#define MADV_4M_PAGES 22/* Use 4 Megabyte pages */ #define MADV_16M_PAGES 24/* Use 16 Megabyte pages */ #define MADV_64M_PAGES 26/* Use 64 Megabyte pages */ -#define MADV_MERGEABLE 65/* KSM may merge identical pages */ #define MADV_UNMERGEABLE 66/* KSM may not merge identical pages */ -#define MADV_HUGEPAGE 67/* Worth backing with hugepages */ #define MADV_NOHUGEPAGE 68/* Not worth backing with hugepages */ -#define MADV_DONTDUMP 69/* Explicity exclude from the core dump, overrides the coredump filter bits */ #define MADV_DODUMP 70/* Clear the MADV_NODUMP flag */ /* compatibility flags */ -#define MAP_FILE 0#define MAP_VARIABLE 0#endif /* __PARISC_MMAN_H__ */
arch/powerpc/include/uapi/asm/mman.h +6 -10
@@ -7,8 +7,13 @@#ifndef _UAPI_ASM_POWERPC_MMAN_H#define _UAPI_ASM_POWERPC_MMAN_H -#include <asm-generic/mman-common.h> +#include <asm-generic/mman.h> +/* The following constants don't match mman.h. */ +#undef MAP_LOCKED +#undef MAP_NORESERVE +#undef MCL_CURRENT +#undef MCL_FUTURE#define PROT_SAO 0x10/* Strong Access Ordering */
@@ -16,16 +21,7 @@#define MAP_NORESERVE 0x40/* don't reserve swap pages */ #define MAP_LOCKED 0x80 -#define MAP_GROWSDOWN 0x0100/* stack-like segment */ -#define MAP_DENYWRITE 0x0800/* ETXTBSY */ -#define MAP_EXECUTABLE 0x1000/* mark it as an executable */ -#define MCL_CURRENT 0x2000/* lock all currently mapped pages */ #define MCL_FUTURE 0x4000/* lock all additions to address space */ -#define MAP_POPULATE 0x8000/* populate (prefault) pagetables */ -#define MAP_NONBLOCK 0x10000/* do not block on IO */ -#define MAP_STACK 0x20000/* give out an address that is best suited for process/thread stacks */ -#define MAP_HUGETLB 0x40000/* create a huge page mapping */ -#endif /* _UAPI_ASM_POWERPC_MMAN_H */
arch/sparc/include/uapi/asm/mman.h +8 -10
@@ -1,10 +1,16 @@#ifndef _UAPI__SPARC_MMAN_H__#define _UAPI__SPARC_MMAN_H__ -#include <asm-generic/mman-common.h> +#include <asm-generic/mman.h> -/* SunOS'ified... */ +/* The following constants don't match mman.h. */ +#undef MAP_NORESERVE +#undef MAP_LOCKED +#undef MAP_GROWSDOWN +#undef MCL_CURRENT +#undef MCL_FUTURE +/* SunOS'ified... */ #define MAP_RENAME MAP_ANONYMOUS/* In SunOS terminology */ #define MAP_NORESERVE 0x40/* don't reserve swap pages */ #define MAP_INHERIT 0x80/* SunOS doesn't do this, but... */
@@ -12,16 +18,8 @@#define _MAP_NEW 0x80000000/* Binary compatibility is fun... */ #define MAP_GROWSDOWN 0x0200/* stack-like segment */ -#define MAP_DENYWRITE 0x0800/* ETXTBSY */ -#define MAP_EXECUTABLE 0x1000/* mark it as an executable */ #define MCL_CURRENT 0x2000/* lock all currently mapped pages */ #define MCL_FUTURE 0x4000/* lock all additions to address space */ -#define MAP_POPULATE 0x8000/* populate (prefault) pagetables */ -#define MAP_NONBLOCK 0x10000/* do not block on IO */ -#define MAP_STACK 0x20000/* give out an address that is best suited for process/thread stacks */ -#define MAP_HUGETLB 0x40000/* create a huge page mapping */ - -#endif /* _UAPI__SPARC_MMAN_H__ */
arch/tile/include/uapi/asm/mman.h +9 -13
@@ -15,27 +15,23 @@#ifndef _ASM_TILE_MMAN_H#define _ASM_TILE_MMAN_H -#include <asm-generic/mman-common.h> +#include <asm-generic/mman.h> #include <arch/chip.h> -/* Standard Linux flags */ +/* The following constants don't match mman.h. */ +#undef MAP_POPULATE +#undef MAP_NONBLOCK +#undef MAP_STACK +#undef MAP_LOCKED +#undef MAP_NORESERVE +#undef MAP_HUGETLB +/* Standard Linux flags */ #define MAP_POPULATE 0x0040/* populate (prefault) pagetables */ #define MAP_NONBLOCK 0x0080/* do not block on IO */ -#define MAP_GROWSDOWN 0x0100/* stack-like segment */ #define MAP_STACK MAP_GROWSDOWN/* provide convenience alias */ #define MAP_LOCKED 0x0200/* pages are locked */ #define MAP_NORESERVE 0x0400/* don't check for reservations */ -#define MAP_DENYWRITE 0x0800/* ETXTBSY */ -#define MAP_EXECUTABLE 0x1000/* mark it as an executable */ #define MAP_HUGETLB 0x4000/* create a huge page mapping */ - -/* -* Flags for mlockall -*/ -#define MCL_CURRENT 1/* lock all current mappings */ -#define MCL_FUTURE 2/* lock all future mappings */ - -#endif /* _ASM_TILE_MMAN_H */
include/uapi/asm-generic/mman-common.h deleted +0 -58
58 lines not on this page · raw message
include/uapi/asm-generic/mman.h +75 -1
92 lines not on this page · raw message
Quoted 25 lines · Carlos O'Donell
David,
Thanks for the great UAPI work. While working on glibc
mman.hsync I had the chance to look more closely at some of the UAPI headers.Currently (3.7.0-rc6) sparc, powerpc, and tile are including
mman-common.h, but nothing inmman-common.hhelps prevent the accidental definition of an overlapping new constant value.For example in
mman.hpowerpc defines:
#defineMAP_NORESERVE0x40/* don't reserve swap pages */
#defineMAP_LOCKED0x80If someone were to add a new MAP_* value in
mman-common.hat0x40it would conflict with the existing powerpc value. The powerpc maintainers might not notice, and that's a bug.I notice that someone is trying to avoid this kind of bug, because for example the new
MADV_HWPOISONwas chosen as 100, which is higher than any other MADV_* value for any architecture.Rather than avoid the problem by spot-checking, would it be acceptable to merge in some private macros that block out exactly which constants are already in use as existing values for arches using UAPI headers?
Quoted 5 lines
Doing it this way would allow for the merged arches to more safely include the asm-generic version of
mman.hand start benefiting from new constants without the usual maintenance cost (not to mention everyone would start in lock-step going forward). Not to mention it simplifies some of the glibc maintenance.
This proposal looks reasonable to me and the proposed code seems OK for tile, at least. Thanks!
Signature · 2 lines
Chris Metcalf, Tilera Corp. http://www.tilera.com