diff mbox series

[2/1] libxfs: stop overriding MAP_SYNC in publicly exported header files

Message ID YtmB005kkkErb5uw@magnolia (mailing list archive)
State Accepted
Headers show
Series xfsprogs: random fixes for 5.19 | expand

Commit Message

Darrick J. Wong July 21, 2022, 4:41 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Florian Fainelli most recently reported that xfsprogs doesn't build with
musl on mips:

"MIPS platforms building with recent kernel headers and the musl-libc
toolchain will expose the following build failure:

mmap.c: In function 'mmap_f':
mmap.c:196:12: error: 'MAP_SYNC' undeclared (first use in this function); did you mean 'MS_SYNC'?
  196 |    flags = MAP_SYNC | MAP_SHARED_VALIDATE;
      |            ^~~~~~~~
      |            MS_SYNC
mmap.c:196:12: note: each undeclared identifier is reported only once for each function it appears in
make[4]: *** [../include/buildrules:81: mmap.o] Error 1"

At first glance, the build failure here is caused by the fact that:

1. The configure script doesn't detect MAP_SYNC support
2. The build system doesn't set HAVE_MAP_SYNC
2. io/mmap.c includes input.h -> projects.h -> xfs.h and later sys/mman.h
3. include/linux.h #define's MAP_SYNC to 0 if HAVE_MAP_SYNC is not set
4. musl's sys/mman.h #undef MAP_SYNC on platforms that don't support it
5. io/mmap.c tries to use MAP_SYNC, not realizing that libc undefined it

Normally, xfs_io only exports functionality that is defined by the libc
and/or kernel headers on the build system.  We often make exceptions for
new functionality so that we have a way to test them before the header
file packages catch up, hence this '#ifndef HAVE_FOO #define FOO'
paradigm.

MAP_SYNC is a gross and horribly broken example of this.  These support
crutches are supposed to be *private* to xfsprogs for benefit of early
testing, but they were instead added to include/linux.h, which we
provide to user programs in the xfslibs-dev package.  IOWs, we've been
#defining MAP_SYNC to zero for unsuspecting programs.

Worst yet, gcc 11.3 doesn't even warn about overriding a #define to 0:

#include <stdio.h>
#include <sys/mman.h>
#ifdef STUPID
# include <xfs/xfs.h>
#endif

int main(int argc, char *argv[]) {
	printf("MAP_SYNC 0x%x\n", MAP_SYNC);
}

$ gcc -o a a.c -Wall
$ ./a
MAP_SYNC 0x80000
$ gcc -DSTUPID -o a a.c -Wall
$ ./a
MAP_SYNC 0x0

Four years have gone by since the introduction of MAP_SYNC, so let's get
rid of the override code entirely -- any platform that supports MAP_SYNC
has had plenty of chances to ensure their header files have the right
bits.  While we're at it, fix AC_HAVE_MAP_SYNC to look for MAP_SYNC in
the same header file that the one user (io/mmap.c) uses -- sys/mman.h.

Annoyingly, I had to test this by hand because the sole fstest that
exercises MAP_SYNC (generic/470) requires dm-logwrites and dm-thinp,
neither of which support fsdax on current kernels.

Reported-by: info@mobile-stream.com
Reported-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
Reported-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 include/linux.h       |    8 --------
 io/io.h               |    2 +-
 io/mmap.c             |   25 +++++++++++++------------
 m4/package_libcdev.m4 |    3 +--
 4 files changed, 15 insertions(+), 23 deletions(-)
diff mbox series

Patch

diff --git a/include/linux.h b/include/linux.h
index 3d9f4e3d..eddc4ad9 100644
--- a/include/linux.h
+++ b/include/linux.h
@@ -251,14 +251,6 @@  struct fsxattr {
 #define FS_XFLAG_COWEXTSIZE	0x00010000	/* CoW extent size allocator hint */
 #endif
 
-#ifndef HAVE_MAP_SYNC
-#define MAP_SYNC 0
-#define MAP_SHARED_VALIDATE 0
-#else
-#include <asm-generic/mman.h>
-#include <asm-generic/mman-common.h>
-#endif /* HAVE_MAP_SYNC */
-
 /*
  * Reminder: anything added to this file will be compiled into downstream
  * userspace projects!
diff --git a/io/io.h b/io/io.h
index ada0a149..de4ef607 100644
--- a/io/io.h
+++ b/io/io.h
@@ -58,7 +58,7 @@  typedef struct mmap_region {
 	size_t		length;		/* length of mapping */
 	off64_t		offset;		/* start offset into backing file */
 	int		prot;		/* protection mode of the mapping */
-	bool		map_sync;	/* is this a MAP_SYNC mapping? */
+	int		flags;		/* MAP_* flags passed to mmap() */
 	char		*name;		/* name of backing file */
 } mmap_region_t;
 
diff --git a/io/mmap.c b/io/mmap.c
index 8c048a0a..425957d4 100644
--- a/io/mmap.c
+++ b/io/mmap.c
@@ -46,8 +46,11 @@  print_mapping(
 	for (i = 0, p = pflags; p->prot != PROT_NONE; i++, p++)
 		buffer[i] = (map->prot & p->prot) ? p->mode : '-';
 
-	if (map->map_sync)
+#ifdef HAVE_MAP_SYNC
+	if ((map->flags & (MAP_SYNC | MAP_SHARED_VALIDATE)) ==
+			  (MAP_SYNC | MAP_SHARED_VALIDATE))
 		sprintf(&buffer[i], " S");
+#endif
 
 	printf("%c%03d%c 0x%lx - 0x%lx %s  %14s (%lld : %ld)\n",
 		braces? '[' : ' ', index, braces? ']' : ' ',
@@ -139,7 +142,9 @@  mmap_help(void)
 " -r -- map with PROT_READ protection\n"
 " -w -- map with PROT_WRITE protection\n"
 " -x -- map with PROT_EXEC protection\n"
+#ifdef HAVE_MAP_SYNC
 " -S -- map with MAP_SYNC and MAP_SHARED_VALIDATE flags\n"
+#endif
 " -s <size> -- first do mmap(size)/munmap(size), try to reserve some free space\n"
 " If no protection mode is specified, all are used by default.\n"
 "\n"));
@@ -193,18 +198,14 @@  mmap_f(
 			prot |= PROT_EXEC;
 			break;
 		case 'S':
+#ifdef HAVE_MAP_SYNC
 			flags = MAP_SYNC | MAP_SHARED_VALIDATE;
-
-			/*
-			 * If MAP_SYNC and MAP_SHARED_VALIDATE aren't defined
-			 * in the system headers we will have defined them
-			 * both as 0.
-			 */
-			if (!flags) {
-				printf("MAP_SYNC not supported\n");
-				return 0;
-			}
 			break;
+#else
+			printf("MAP_SYNC not supported\n");
+			exitcode = 1;
+			return command_usage(&mmap_cmd);
+#endif
 		case 's':
 			length2 = cvtnum(blocksize, sectsize, optarg);
 			break;
@@ -281,7 +282,7 @@  mmap_f(
 	mapping->offset = offset;
 	mapping->name = filename;
 	mapping->prot = prot;
-	mapping->map_sync = (flags == (MAP_SYNC | MAP_SHARED_VALIDATE));
+	mapping->flags = flags;
 	return 0;
 }
 
diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4
index df44174d..5293dd1a 100644
--- a/m4/package_libcdev.m4
+++ b/m4/package_libcdev.m4
@@ -387,8 +387,7 @@  AC_DEFUN([AC_HAVE_MAP_SYNC],
   [ AC_MSG_CHECKING([for MAP_SYNC])
     AC_COMPILE_IFELSE(
     [	AC_LANG_PROGRAM([[
-#include <asm-generic/mman.h>
-#include <asm-generic/mman-common.h>
+#include <sys/mman.h>
 	]], [[
 int flags = MAP_SYNC | MAP_SHARED_VALIDATE;
 	]])