Date: Fri, 14 Apr 2017 17:23:52 +0000 (UTC) From: Andriy Gapon <avg@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-vendor@freebsd.org Subject: svn commit: r316875 - vendor/illumos/dist/lib/libzfs/common Message-ID: <201704141723.v3EHNqu3043958@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: avg Date: Fri Apr 14 17:23:52 2017 New Revision: 316875 URL: https://svnweb.freebsd.org/changeset/base/316875 Log: 7336 vfork and O_CLOEXEC causes zfs_mount EBUSY illumos/illumos-gate@873c4903a52d089cd8234b79d24f5a3fc3bccc82 https://github.com/illumos/illumos-gate/commit/873c4903a52d089cd8234b79d24f5a3fc3bccc82 https://www.illumos.org/issues/7336 We can run into a problem where we call into zfs_mount, which in turn calls is_dir_empty, which opens the directory to try and make sure it's empty. The issue with the current approach is that it holds the directory open while it traverses it with readdir, which, due to subtle interaction with the Java JVM, vfork, and exec can cause a tricky race condition resulting in zfs_mount failures. The approach to resolving the issue in this patch is to drop the usage of readdir altogether, and instead rely on the fact that ZFS stores the number of entries contained in a directory using the st_size field of the stat structure. Thus, if the directory in question is a ZFS directory, we can check to see if it's empty by calling stat() and inspecting the st_size field of structure returned. =============================================================================== The root cause appears to be an interesting race between vfork, exec, and zfs_mount's usage of O_CLOEXEC when calling openat. Here's what is going on: 1. We call zfs_mount, and this in turn calls openat to check if the directory is empty, which results in opening the directory we're trying to mount onto, and increment v_count. 2. As we're in the middle of reading the directory, vfork is called by the JVM and proceeds to exec the jspawnhelper utility. As a result of the vfork, we take an additional hold on the directory, which increments v_count a second time. The semantics of vfork mean the parent process will wait for the child process to exit or exec before the parent can continue; at this point the parent is in the middle of zfs_mount, reading the directory to determine if it's empty or not. 3. The child process exec-ing jspawnhelper gets to the relvm call within exec_args (which is called by exec_common). relvm is the function that releases the parent process, allowing the parent to proceed. The problem is, at this point of calling relvm, the child hasn't yet called close_exec which is responsible for closing the file descriptors inherited from the parent process Reviewed by: Matt Ahrens <mahrens@delphix.com> Reviewed by: Paul Dagnelie <pcd@delphix.com> Reviewed by: Robert Mustacchi <rm@joyent.com> Approved by: Dan McDonald <danmcd@omniti.com> Author: Prakash Surya <prakash.surya@delphix.com> Modified: vendor/illumos/dist/lib/libzfs/common/libzfs_mount.c Modified: vendor/illumos/dist/lib/libzfs/common/libzfs_mount.c ============================================================================== --- vendor/illumos/dist/lib/libzfs/common/libzfs_mount.c Fri Apr 14 17:23:28 2017 (r316874) +++ vendor/illumos/dist/lib/libzfs/common/libzfs_mount.c Fri Apr 14 17:23:52 2017 (r316875) @@ -75,6 +75,7 @@ #include <sys/mntent.h> #include <sys/mount.h> #include <sys/stat.h> +#include <sys/statvfs.h> #include <libzfs.h> @@ -170,13 +171,32 @@ is_shared(libzfs_handle_t *hdl, const ch return (SHARED_NOT_SHARED); } -/* - * Returns true if the specified directory is empty. If we can't open the - * directory at all, return true so that the mount can fail with a more - * informative error message. - */ static boolean_t -dir_is_empty(const char *dirname) +dir_is_empty_stat(const char *dirname) +{ + struct stat st; + + /* + * We only want to return false if the given path is a non empty + * directory, all other errors are handled elsewhere. + */ + if (stat(dirname, &st) < 0 || !S_ISDIR(st.st_mode)) { + return (B_TRUE); + } + + /* + * An empty directory will still have two entries in it, one + * entry for each of "." and "..". + */ + if (st.st_size > 2) { + return (B_FALSE); + } + + return (B_TRUE); +} + +static boolean_t +dir_is_empty_readdir(const char *dirname) { DIR *dirp; struct dirent64 *dp; @@ -206,6 +226,42 @@ dir_is_empty(const char *dirname) } /* + * Returns true if the specified directory is empty. If we can't open the + * directory at all, return true so that the mount can fail with a more + * informative error message. + */ +static boolean_t +dir_is_empty(const char *dirname) +{ + struct statvfs64 st; + + /* + * If the statvfs call fails or the filesystem is not a ZFS + * filesystem, fall back to the slow path which uses readdir. + */ + if ((statvfs64(dirname, &st) != 0) || + (strcmp(st.f_basetype, "zfs") != 0)) { + return (dir_is_empty_readdir(dirname)); + } + + /* + * At this point, we know the provided path is on a ZFS + * filesystem, so we can use stat instead of readdir to + * determine if the directory is empty or not. We try to avoid + * using readdir because that requires opening "dirname"; this + * open file descriptor can potentially end up in a child + * process if there's a concurrent fork, thus preventing the + * zfs_mount() from otherwise succeeding (the open file + * descriptor inherited by the child process will cause the + * parent's mount to fail with EBUSY). The performance + * implications of replacing the open, read, and close with a + * single stat is nice; but is not the main motivation for the + * added complexity. + */ + return (dir_is_empty_stat(dirname)); +} + +/* * Checks to see if the mount is active. If the filesystem is mounted, we fill * in 'where' with the current mountpoint, and return 1. Otherwise, we return * 0.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201704141723.v3EHNqu3043958>