From bb9cecbf67341e03e53dd36b8f69520f3a26f834 Mon Sep 17 00:00:00 2001 From: Matthias Goergens Date: Tue, 5 Mar 2024 15:10:21 +0100 Subject: Use posix_spawn instead of fork+exec Client code might allocate a lot of memory before starting the mount. Fork is slow for processes that are using a lot of memory. But posix_spawn fixes that. Another issue with fork is if the process is also doing RDMA - this might lead to data corruption, as least if memory used for RDMA is not marked with MADV_DONTFORK. At least with linux kernels before 5.12. Also see https://blog.nelhage.com/post/a-cursed-bug/ for more details Change by Bernd: This also prepares the new fusermount option "--comm-fd", but keeps the previous way to pass the parameter as env variable. In a future release (exact data to be determined) we are going to remove usage of the env variable and will switch to the new parameter. --- lib/mount.c | 182 ++++++++++++++++++++++++++---------------------------- util/fusermount.c | 15 +++-- 2 files changed, 99 insertions(+), 98 deletions(-) diff --git a/lib/mount.c b/lib/mount.c index f98a8bb..2ffb05c 100644 --- a/lib/mount.c +++ b/lib/mount.c @@ -8,6 +8,9 @@ See the file COPYING.LIB. */ +/* For environ */ +#define _GNU_SOURCE + #include "fuse_config.h" #include "fuse_i.h" #include "fuse_misc.h" @@ -22,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -44,10 +48,6 @@ #define FUSERMOUNT_PROG "fusermount3" #define FUSE_COMMFD_ENV "_FUSE_COMMFD" -#ifndef HAVE_FORK -#define fork() vfork() -#endif - #ifndef MS_DIRSYNC #define MS_DIRSYNC 128 #endif @@ -117,20 +117,17 @@ static const struct fuse_opt fuse_mount_opts[] = { FUSE_OPT_END }; -static void exec_fusermount(const char *argv[]) -{ - execv(FUSERMOUNT_DIR "/" FUSERMOUNT_PROG, (char **) argv); - execvp(FUSERMOUNT_PROG, (char **) argv); -} - void fuse_mount_version(void) { - int pid = fork(); - if (!pid) { - const char *argv[] = { FUSERMOUNT_PROG, "--version", NULL }; - exec_fusermount(argv); - _exit(1); - } else if (pid != -1) + char const *const argv[] = {FUSERMOUNT_PROG, "--version", NULL}; + pid_t pid; + int status = posix_spawn(&pid, FUSERMOUNT_DIR "/" FUSERMOUNT_PROG, NULL, + NULL, (char * const *) argv, environ) + && posix_spawnp(&pid, FUSERMOUNT_PROG, NULL, + NULL, (char * const *) argv, environ); + if(status != 0) + perror("posix_spawn"); + else waitpid(pid, NULL, 0); } @@ -269,7 +266,7 @@ static int receive_fd(int fd) void fuse_kern_unmount(const char *mountpoint, int fd) { int res; - int pid; + pid_t pid; if (fd != -1) { struct pollfd pfd; @@ -301,23 +298,24 @@ void fuse_kern_unmount(const char *mountpoint, int fd) if (res == 0) return; - pid = fork(); - if(pid == -1) + char const * const argv[] = + { FUSERMOUNT_PROG, "--unmount", "--quiet", "--lazy", + "--", mountpoint, NULL }; + int status = posix_spawn( + &pid, FUSERMOUNT_DIR "/" FUSERMOUNT_PROG, NULL, NULL, (char * const *) argv, environ) + && posix_spawnp( + &pid, FUSERMOUNT_PROG, NULL, NULL, (char * const *) argv, environ); + if(status != 0) { + perror("posix_spawn"); return; - - if(pid == 0) { - const char *argv[] = { FUSERMOUNT_PROG, "-u", "-q", "-z", - "--", mountpoint, NULL }; - - exec_fusermount(argv); - _exit(1); } waitpid(pid, NULL, 0); } static int setup_auto_unmount(const char *mountpoint, int quiet) { - int fds[2], pid; + int fds[2]; + pid_t pid; int res; if (!mountpoint) { @@ -331,55 +329,55 @@ static int setup_auto_unmount(const char *mountpoint, int quiet) return -1; } - pid = fork(); - if(pid == -1) { - perror("fuse: fork() failed"); - close(fds[0]); - close(fds[1]); - return -1; + char arg_fd_entry[30]; + snprintf(arg_fd_entry, sizeof(arg_fd_entry), "%i", fds[0]); + setenv(FUSE_COMMFD_ENV, arg_fd_entry, 1); + + char const *const argv[] = { + FUSERMOUNT_PROG, + "--auto-unmount", + "--", + mountpoint, + NULL, + }; + + // TODO: add error handling for all manipulations of action. + posix_spawn_file_actions_t action; + posix_spawn_file_actions_init(&action); + + if (quiet) { + posix_spawn_file_actions_addclose(&action, 1); + posix_spawn_file_actions_addclose(&action, 2); } + posix_spawn_file_actions_addclose(&action, fds[1]); - if(pid == 0) { - char env[10]; - const char *argv[32]; - int a = 0; - - if (quiet) { - int fd = open("/dev/null", O_RDONLY); - if (fd != -1) { - dup2(fd, 1); - dup2(fd, 2); - } - } - - argv[a++] = FUSERMOUNT_PROG; - argv[a++] = "--auto-unmount"; - argv[a++] = "--"; - argv[a++] = mountpoint; - argv[a++] = NULL; + int status = posix_spawn(&pid, FUSERMOUNT_DIR "/" FUSERMOUNT_PROG, &action, + NULL, (char *const *) argv, environ) + && posix_spawnp(&pid, FUSERMOUNT_PROG, &action, + NULL, (char *const *) argv, environ); + posix_spawn_file_actions_destroy(&action); + if(status != 0) { + close(fds[0]); close(fds[1]); - fcntl(fds[0], F_SETFD, 0); - snprintf(env, sizeof(env), "%i", fds[0]); - setenv(FUSE_COMMFD_ENV, env, 1); - exec_fusermount(argv); - perror("fuse: failed to exec fusermount3"); - _exit(1); + perror("fuse: posix_spawnp() of fusermount3 failed"); + return -1; } - + // passed to child now, so can close here. close(fds[0]); // Now fusermount3 will only exit when fds[1] closes automatically when our // process exits. return 0; + // Note: fds[1] is leakend and doesn't get FD_CLOEXEC } static int fuse_mount_fusermount(const char *mountpoint, struct mount_opts *mo, const char *opts, int quiet) { - int fds[2], pid; + int fds[2]; + pid_t pid; int res; - int rv; if (!mountpoint) { fuse_log(FUSE_LOG_ERR, "fuse: missing mountpoint parameter\n"); @@ -392,47 +390,45 @@ static int fuse_mount_fusermount(const char *mountpoint, struct mount_opts *mo, return -1; } - pid = fork(); - if(pid == -1) { - perror("fuse: fork() failed"); - close(fds[0]); - close(fds[1]); - return -1; - } + char arg_fd_entry[30]; + snprintf(arg_fd_entry, sizeof(arg_fd_entry), "%i", fds[0]); + setenv(FUSE_COMMFD_ENV, arg_fd_entry, 1); - if(pid == 0) { - char env[10]; - const char *argv[32]; - int a = 0; + char const *const argv[] = { + FUSERMOUNT_PROG, + "-o", opts ? opts : "", + "--", + mountpoint, + NULL, + }; - if (quiet) { - int fd = open("/dev/null", O_RDONLY); - if (fd != -1) { - dup2(fd, 1); - dup2(fd, 2); - } - } - argv[a++] = FUSERMOUNT_PROG; - if (opts) { - argv[a++] = "-o"; - argv[a++] = opts; - } - argv[a++] = "--"; - argv[a++] = mountpoint; - argv[a++] = NULL; + posix_spawn_file_actions_t action; + posix_spawn_file_actions_init(&action); + + if (quiet) { + posix_spawn_file_actions_addclose(&action, 1); + posix_spawn_file_actions_addclose(&action, 2); + } + posix_spawn_file_actions_addclose(&action, fds[1]); + int status = posix_spawn(&pid, FUSERMOUNT_DIR "/" FUSERMOUNT_PROG, &action, + NULL, (char *const *) argv, environ) + && posix_spawnp(&pid, FUSERMOUNT_PROG, &action, + NULL, (char *const *) argv, environ); + posix_spawn_file_actions_destroy(&action); + + if(status != 0) { + close(fds[0]); close(fds[1]); - fcntl(fds[0], F_SETFD, 0); - snprintf(env, sizeof(env), "%i", fds[0]); - setenv(FUSE_COMMFD_ENV, env, 1); - exec_fusermount(argv); - perror("fuse: failed to exec fusermount3"); - _exit(1); + perror("fuse: posix_spawnp() of fusermount3 failed"); + return -1; } + // passed to child now, so can close here. close(fds[0]); - rv = receive_fd(fds[1]); + + int rv = receive_fd(fds[1]); if (!mo->auto_unmount) { /* with auto_unmount option fusermount3 will not exit until diff --git a/util/fusermount.c b/util/fusermount.c index 06f5f56..5716c76 100644 --- a/util/fusermount.c +++ b/util/fusermount.c @@ -1454,7 +1454,7 @@ int main(int argc, char *argv[]) static int unmount = 0; static int lazy = 0; static int quiet = 0; - char *commfd; + char *commfd = NULL; int cfd; const char *opts = ""; const char *type = NULL; @@ -1462,14 +1462,15 @@ int main(int argc, char *argv[]) static const struct option long_opts[] = { {"unmount", no_argument, NULL, 'u'}, - // Note: auto-unmount deliberately does not have a short version. - // It's meant for internal use by mount.c's setup_auto_unmount. - {"auto-unmount", no_argument, NULL, 'U'}, {"lazy", no_argument, NULL, 'z'}, {"quiet", no_argument, NULL, 'q'}, {"help", no_argument, NULL, 'h'}, {"version", no_argument, NULL, 'V'}, {"options", required_argument, NULL, 'o'}, + // Note: auto-unmount and comm-fd don't have short versions. + // They'ne meant for internal use by mount.c + {"auto-unmount", no_argument, NULL, 'U'}, + {"comm-fd", required_argument, NULL, 'c'}, {0, 0, 0, 0}}; progname = strdup(argc > 0 ? argv[0] : "fusermount"); @@ -1501,6 +1502,9 @@ int main(int argc, char *argv[]) auto_unmount = 1; setup_auto_unmount_only = 1; break; + case 'c': + commfd = optarg; + break; case 'z': lazy = 1; break; @@ -1547,7 +1551,8 @@ int main(int argc, char *argv[]) if (!setup_auto_unmount_only && unmount) goto do_unmount; - commfd = getenv(FUSE_COMMFD_ENV); + if(commfd == NULL) + commfd = getenv(FUSE_COMMFD_ENV); if (commfd == NULL) { fprintf(stderr, "%s: old style mounting not supported\n", progname); -- cgit v1.2.3 From 290c65b1ad4b8a42a9af507b411df480400d9aff Mon Sep 17 00:00:00 2001 From: Bernd Schubert Date: Tue, 5 Mar 2024 15:47:50 +0100 Subject: posix_spawn style updates - This adds a wrapper function for the call sequence of posix_spawn and posix_spawnp. - Replaces perror() with fuse_log - the latter can be redirected through the file system log function and gives better end user friendly output - other minor changes, like variable renames - no functional change --- lib/mount.c | 96 ++++++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 64 insertions(+), 32 deletions(-) diff --git a/lib/mount.c b/lib/mount.c index 2ffb05c..3fbbe72 100644 --- a/lib/mount.c +++ b/lib/mount.c @@ -117,18 +117,51 @@ static const struct fuse_opt fuse_mount_opts[] = { FUSE_OPT_END }; -void fuse_mount_version(void) +/* + * Running fusermount by calling 'posix_spawn' + * + * @param out_pid might be NULL + */ +static int fusermount_posix_spawn(posix_spawn_file_actions_t *action, + char const * const argv[], pid_t *out_pid) { - char const *const argv[] = {FUSERMOUNT_PROG, "--version", NULL}; + const char *full_path = FUSERMOUNT_DIR "/" FUSERMOUNT_PROG; pid_t pid; - int status = posix_spawn(&pid, FUSERMOUNT_DIR "/" FUSERMOUNT_PROG, NULL, - NULL, (char * const *) argv, environ) - && posix_spawnp(&pid, FUSERMOUNT_PROG, NULL, - NULL, (char * const *) argv, environ); - if(status != 0) - perror("posix_spawn"); + + /* See man 7 environ for the global environ pointer */ + + /* first try the install path */ + int status = posix_spawn(&pid, full_path, action, NULL, + (char * const *) argv, environ); + if (status != 0) { + /* if that fails, try a system install */ + status = posix_spawnp(&pid, FUSERMOUNT_PROG, action, NULL, + (char * const *) argv, environ); + } + + if (status != 0) { + fuse_log(FUSE_LOG_ERR, + "On calling fusermount posix_spawn failed: %s\n", + strerror(status)); + return -status; + } + + if (out_pid) + *out_pid = pid; else waitpid(pid, NULL, 0); + + return 0; +} + +void fuse_mount_version(void) +{ + char const *const argv[] = {FUSERMOUNT_PROG, "--version", NULL}; + int status = fusermount_posix_spawn(NULL, argv, NULL); + + if(status != 0) + fuse_log(FUSE_LOG_ERR, "Running '%s --version' failed", + FUSERMOUNT_PROG); } struct mount_flags { @@ -246,7 +279,7 @@ static int receive_fd(int fd) while(((rv = recvmsg(fd, &msg, 0)) == -1) && errno == EINTR); if (rv == -1) { - perror("recvmsg"); + fuse_log(FUSE_LOG_ERR, "recvmsg failed: %s", strerror(errno)); return -1; } if(!rv) { @@ -266,7 +299,6 @@ static int receive_fd(int fd) void fuse_kern_unmount(const char *mountpoint, int fd) { int res; - pid_t pid; if (fd != -1) { struct pollfd pfd; @@ -301,15 +333,12 @@ void fuse_kern_unmount(const char *mountpoint, int fd) char const * const argv[] = { FUSERMOUNT_PROG, "--unmount", "--quiet", "--lazy", "--", mountpoint, NULL }; - int status = posix_spawn( - &pid, FUSERMOUNT_DIR "/" FUSERMOUNT_PROG, NULL, NULL, (char * const *) argv, environ) - && posix_spawnp( - &pid, FUSERMOUNT_PROG, NULL, NULL, (char * const *) argv, environ); + int status = fusermount_posix_spawn(NULL, argv, NULL); if(status != 0) { - perror("posix_spawn"); + fuse_log(FUSE_LOG_ERR, "Spawaning %s to unumount failed", + FUSERMOUNT_PROG); return; } - waitpid(pid, NULL, 0); } static int setup_auto_unmount(const char *mountpoint, int quiet) @@ -325,7 +354,8 @@ static int setup_auto_unmount(const char *mountpoint, int quiet) res = socketpair(PF_UNIX, SOCK_STREAM, 0, fds); if(res == -1) { - perror("fuse: socketpair() failed"); + fuse_log(FUSE_LOG_ERR, "Setting up auto-unmountsocketpair() failed", + strerror(errno)); return -1; } @@ -351,16 +381,18 @@ static int setup_auto_unmount(const char *mountpoint, int quiet) } posix_spawn_file_actions_addclose(&action, fds[1]); - int status = posix_spawn(&pid, FUSERMOUNT_DIR "/" FUSERMOUNT_PROG, &action, - NULL, (char *const *) argv, environ) - && posix_spawnp(&pid, FUSERMOUNT_PROG, &action, - NULL, (char *const *) argv, environ); + /* + * auto-umount runs in the background - it is not waiting for the + * process + */ + int status = fusermount_posix_spawn(&action, argv, &pid); + posix_spawn_file_actions_destroy(&action); if(status != 0) { close(fds[0]); close(fds[1]); - perror("fuse: posix_spawnp() of fusermount3 failed"); + fuse_log(FUSE_LOG_ERR, "fuse: Setting up auto-unmount failed"); return -1; } // passed to child now, so can close here. @@ -386,7 +418,8 @@ static int fuse_mount_fusermount(const char *mountpoint, struct mount_opts *mo, res = socketpair(PF_UNIX, SOCK_STREAM, 0, fds); if(res == -1) { - perror("fuse: socketpair() failed"); + fuse_log(FUSE_LOG_ERR, "Running %s: socketpair() failed: %s\n", + FUSERMOUNT_PROG, strerror(errno)); return -1; } @@ -412,23 +445,22 @@ static int fuse_mount_fusermount(const char *mountpoint, struct mount_opts *mo, } posix_spawn_file_actions_addclose(&action, fds[1]); - int status = posix_spawn(&pid, FUSERMOUNT_DIR "/" FUSERMOUNT_PROG, &action, - NULL, (char *const *) argv, environ) - && posix_spawnp(&pid, FUSERMOUNT_PROG, &action, - NULL, (char *const *) argv, environ); + int status = fusermount_posix_spawn(&action, argv, &pid); + posix_spawn_file_actions_destroy(&action); if(status != 0) { close(fds[0]); close(fds[1]); - perror("fuse: posix_spawnp() of fusermount3 failed"); + fuse_log(FUSE_LOG_ERR, "posix_spawnp() for %s failed", + FUSERMOUNT_PROG, strerror(errno)); return -1; } // passed to child now, so can close here. close(fds[0]); - int rv = receive_fd(fds[1]); + int fd = receive_fd(fds[1]); if (!mo->auto_unmount) { /* with auto_unmount option fusermount3 will not exit until @@ -437,10 +469,10 @@ static int fuse_mount_fusermount(const char *mountpoint, struct mount_opts *mo, waitpid(pid, NULL, 0); /* bury zombie */ } - if (rv >= 0) - fcntl(rv, F_SETFD, FD_CLOEXEC); + if (fd >= 0) + fcntl(fd, F_SETFD, FD_CLOEXEC); - return rv; + return fd; } #ifndef O_CLOEXEC -- cgit v1.2.3 From b1cdc497b8c858b2f292a94e28ed39a4960f5274 Mon Sep 17 00:00:00 2001 From: Bernd Schubert Date: Wed, 6 Mar 2024 00:23:22 +0100 Subject: ci-build.sh: Run ASAN and UBSAN at the same time Also set halt_on_error=1 to make UBSAN to fail if it would find something. --- test/ci-build.sh | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/test/ci-build.sh b/test/ci-build.sh index 28ff0d8..c266ea2 100755 --- a/test/ci-build.sh +++ b/test/ci-build.sh @@ -3,6 +3,10 @@ set -e TEST_CMD="python3 -m pytest --maxfail=99 test/" +SAN="-Db_sanitize=address,undefined" + +# not default +export UBSAN_OPTIONS=halt_on_error=1 # Make sure binaries can be accessed when invoked by root. umask 0022 @@ -26,7 +30,7 @@ export CC # Standard build for CC in gcc gcc-9 gcc-10 clang; do echo "=== Building with ${CC} ===" - mkdir build-${CC}; cd build-${CC} + mkdir build-${CC}; pushd build-${CC} if [ "${CC}" == "clang" ]; then export CXX="clang++" export TEST_WITH_VALGRIND=false @@ -49,25 +53,25 @@ for CC in gcc gcc-9 gcc-10 clang; do sudo chown root:root util/fusermount3 sudo chmod 4755 util/fusermount3 ${TEST_CMD} - cd .. + popd done -(cd build-$CC; sudo ninja install) sanitized_build() -{ - san=$1 - additonal_option=$2 +( + echo "=== Building with clang and sanitizers" - echo "=== Building with clang and ${san} sanitizer ===" - [ -n ${additonal_option} ] || echo "Additional option: ${additonal_option}" + mkdir build-san; pushd build-san - mkdir build-${san}; pushd build-${san} + meson setup -D werror=true\ + "${SOURCE_DIR}" \ + || (ct meson-logs/meson-log.txt; false) + meson configure $SAN # b_lundef=false is required to work around clang # bug, cf. https://groups.google.com/forum/#!topic/mesonbuild/tgEdAXIIdC4 - meson setup -D b_sanitize=${san} -D b_lundef=false -D werror=true\ - ${additonal_option} "${SOURCE_DIR}" \ - || (cat meson-logs/meson-log.txt; false) + meson configure -D b_lundef=false + + meson configure ninja # Test as root and regular user @@ -80,23 +84,19 @@ sanitized_build() ${TEST_CMD} popd - rm -fr build-${san} -} + rm -fr build-san +) # Sanitized build CC=clang CXX=clang++ TEST_WITH_VALGRIND=false -for san in undefined address; do - sanitized_build ${san} -done +sanitized_build $SAN # Sanitized build without libc versioned symbols CC=clang CXX=clang++ -for san in undefined address; do - sanitized_build ${san} "-Ddisable-libc-symbol-version=true" -done +sanitized_build # Documentation. (cd "${SOURCE_DIR}"; doxygen doc/Doxyfile) -- cgit v1.2.3 From aab146eea8877ee744a1b5a0da8bbbf31d14bad1 Mon Sep 17 00:00:00 2001 From: Bernd Schubert Date: Wed, 6 Mar 2024 00:37:53 +0100 Subject: ci-build.sh: Always install and add s-bit for fusermount3 As per pull #898, fusermount3 had a severe issue that should have been detected by ASAN. I guess tests used the system default and not the sanitized binary. Order of execution of fusermount3 is to try 1) full install path if that fails 2) just fusermount3 So tests should be fixed by installing libfuse, setting the s-bit on fusermount3 and then to run the tests. --- test/ci-build.sh | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/test/ci-build.sh b/test/ci-build.sh index c266ea2..a023c15 100755 --- a/test/ci-build.sh +++ b/test/ci-build.sh @@ -18,6 +18,9 @@ umask 0022 # readable/executable. SOURCE_DIR="$(readlink -f .)" TEST_DIR="$(mktemp -dt libfuse-build-XXXXXX)" + +PREFIX_DIR="$(mktemp -dt libfuse-install-XXXXXXX)" + chmod 0755 "${TEST_DIR}" cd "${TEST_DIR}" echo "Running in ${TEST_DIR}" @@ -47,13 +50,17 @@ for CC in gcc gcc-9 gcc-10 clang; do else build_opts='' fi - meson setup -D werror=true ${build_opts} "${SOURCE_DIR}" || (cat meson-logs/meson-log.txt; false) + meson setup -Dprefix=${PREFIX_DIR} -D werror=true ${build_opts} "${SOURCE_DIR}" || (cat meson-logs/meson-log.txt; false) ninja + sudo ninja install + + # libfuse will first try the install path and then system defaults + sudo chmod 4755 ${PREFIX_DIR}/bin/fusermount3 - sudo chown root:root util/fusermount3 - sudo chmod 4755 util/fusermount3 ${TEST_CMD} popd + rm -fr build-${CC} + sudo rm -fr ${PREFIX_DIR} done sanitized_build() @@ -62,7 +69,7 @@ sanitized_build() mkdir build-san; pushd build-san - meson setup -D werror=true\ + meson setup -Dprefix=${PREFIX_DIR} -D werror=true\ "${SOURCE_DIR}" \ || (ct meson-logs/meson-log.txt; false) meson configure $SAN @@ -73,11 +80,11 @@ sanitized_build() meson configure ninja + sudo ninja install + sudo chmod 4755 ${PREFIX_DIR}/bin/fusermount3 # Test as root and regular user sudo ${TEST_CMD} - sudo chown root:root util/fusermount3 - sudo chmod 4755 util/fusermount3 # Cleanup temporary files (since they are now owned by root) sudo rm -rf test/.pytest_cache/ test/__pycache__ @@ -85,6 +92,7 @@ sanitized_build() popd rm -fr build-san + sudo rm -fr ${PREFIX_DIR} ) # Sanitized build -- cgit v1.2.3