mountpoint-util: some tweaks for fd_is_mount_point()

- Drop fstat() fallback path now that we assume fdinfo
  is available
- Use at_flags_normalize_nofollow()
- Accept empty path the same way as NULL
- Accept fd being AT_FDCWD and filename being "."

diff --git a/src/basic/mountpoint-util.c b/src/basic/mountpoint-util.c
index b596d32680c4be553ade552aa48838c5b3a5aa0a..750bcfa71be83a2a64dedc77c594279d9e28c620 100644 (file)
--- a/src/basic/mountpoint-util.c
+++ b/src/basic/mountpoint-util.c
@@ -213,27 +213,35 @@ bool file_handle_equal(const struct file_handle *a, const struct file_handle *b)
 }
 
 int fd_is_mount_point(int fd, const char *filename, int flags) {
-        _cleanup_free_ struct file_handle *h = NULL, *h_parent = NULL;
-        int mount_id = -1, mount_id_parent = -1;
-        bool nosupp = false, check_st_dev = true;
-        STRUCT_STATX_DEFINE(sx);
-        struct stat a, b;
+        bool fd_is_self;
         int r;
 
-        assert(fd >= 0);
+        assert(fd >= 0 || fd == AT_FDCWD);
         assert((flags & ~AT_SYMLINK_FOLLOW) == 0);
 
-        if (!filename) {
-                /* If the file name is specified as NULL we'll see if the specified 'fd' is a mount
-                 * point. That's only supported if the kernel supports statx(), or if the inode specified via
-                 * 'fd' refers to a directory. Otherwise, we'll have to fail (ENOTDIR), because we have no
-                 * kernel API to query the information we need. */
-                flags |= AT_EMPTY_PATH;
-                filename = "";
-        } else if (!filename_possibly_with_slash_suffix(filename))
-                /* Insist that the specified filename is actually a filename, and not a path, i.e. some inode further
-                 * up or down the tree then immediately below the specified directory fd. */
-                return -EINVAL;
+        if (isempty(filename)) {
+                if (fd == AT_FDCWD)
+                        filename = ".";
+                else {
+                        /* If the file name is empty we'll see if the specified 'fd' is a mount point.
+                         * That's only supported if the kernel supports statx(), or if the inode specified
+                         * via 'fd' refers to a directory. Otherwise, we'll have to fail (ENOTDIR), because
+                         * we have no kernel API to query the information we need. */
+                        flags |= AT_EMPTY_PATH;
+                        filename = "";
+                }
+
+                fd_is_self = true;
+        } else if (STR_IN_SET(filename, ".", "./"))
+                fd_is_self = true;
+        else {
+                /* Insist that the specified filename is actually a filename, and not a path, i.e. some inode
+                 * further up or down the tree then immediately below the specified directory fd. */
+                if (!filename_possibly_with_slash_suffix(filename))
+                        return -EINVAL;
+
+                fd_is_self = false;
+        }
 
         /* First we will try statx()' STATX_ATTR_MOUNT_ROOT attribute, which is our ideal API, available
          * since kernel 5.8.
@@ -246,18 +254,17 @@ int fd_is_mount_point(int fd, const char *filename, int flags) {
          * If that didn't work we will try to read the mount id from /proc/self/fdinfo/<fd>. This is almost
          * as good as name_to_handle_at(), however, does not return the opaque file handle. The opaque file
          * handle is pretty useful to detect the root directory, which we should always consider a mount
-         * point. Hence we use this only as fallback. Exporting the mnt_id in fdinfo is a pretty recent
-         * kernel addition.
+         * point. Hence we use this only as fallback.
          *
-         * As last fallback we do traditional fstat() based st_dev comparisons. This is how things were
-         * traditionally done, but unionfs breaks this since it exposes file systems with a variety of st_dev
-         * reported. Also, btrfs subvolumes have different st_dev, even though they aren't real mounts of
-         * their own. */
-
-        if (statx(fd,
-                  filename,
-                  (FLAGS_SET(flags, AT_SYMLINK_FOLLOW) ? 0 : AT_SYMLINK_NOFOLLOW) |
-                  (flags & AT_EMPTY_PATH) |
+         * Note that traditionally the check is done via fstat()-based st_dev comparisons. However, various
+         * file systems don't guarantee same st_dev across single fs anymore, e.g. unionfs exposes file systems
+         * with a variety of st_dev reported. Also, btrfs subvolumes have different st_dev, even though
+         * they aren't real mounts of their own. */
+
+        STRUCT_STATX_DEFINE(sx);
+
+        if (statx(fd, filename,
+                  at_flags_normalize_nofollow(flags) |
                   AT_NO_AUTOMOUNT |            /* don't trigger automounts – mounts are a local concept, hence no need to trigger automounts to determine STATX_ATTR_MOUNT_ROOT */
                   AT_STATX_DONT_SYNC,          /* don't go to the network for this – for similar reasons */
                   STATX_TYPE,
@@ -271,6 +278,10 @@ int fd_is_mount_point(int fd, const char *filename, int flags) {
         } else if (FLAGS_SET(sx.stx_attributes_mask, STATX_ATTR_MOUNT_ROOT)) /* yay! */
                 return FLAGS_SET(sx.stx_attributes, STATX_ATTR_MOUNT_ROOT);
 
+        _cleanup_free_ struct file_handle *h = NULL, *h_parent = NULL;
+        int mount_id = -1, mount_id_parent = -1;
+        bool nosupp = false;
+
         r = name_to_handle_at_try_fid(fd, filename, &h, &mount_id, flags);
         if (r < 0) {
                 if (is_name_to_handle_at_fatal_error(r))
@@ -278,13 +289,12 @@ int fd_is_mount_point(int fd, const char *filename, int flags) {
                 if (!ERRNO_IS_NOT_SUPPORTED(r))
                         goto fallback_fdinfo;
 
-                /* This kernel or file system does not support name_to_handle_at(), hence let's see
-                 * if the upper fs supports it (in which case it is a mount point), otherwise fall
-                 * back to the traditional stat() logic */
+                /* This file system does not support name_to_handle_at(), hence let's see if the upper fs
+                 * supports it (in which case it is a mount point), otherwise fall back to the fdinfo logic. */
                 nosupp = true;
         }
 
-        if (isempty(filename))
+        if (fd_is_self)
                 r = name_to_handle_at_try_fid(fd, "..", &h_parent, &mount_id_parent, 0); /* can't work for non-directories 😢 */
         else
                 r = name_to_handle_at_try_fid(fd, "", &h_parent, &mount_id_parent, AT_EMPTY_PATH);
@@ -316,12 +326,10 @@ int fd_is_mount_point(int fd, const char *filename, int flags) {
 
 fallback_fdinfo:
         r = fd_fdinfo_mnt_id(fd, filename, flags, &mount_id);
-        if (ERRNO_IS_NEG_NOT_SUPPORTED(r) || ERRNO_IS_NEG_PRIVILEGE(r))
-                goto fallback_fstat;
         if (r < 0)
                 return r;
 
-        if (isempty(filename))
+        if (fd_is_self)
                 r = fd_fdinfo_mnt_id(fd, "..", 0, &mount_id_parent); /* can't work for non-directories 😢 */
         else
                 r = fd_fdinfo_mnt_id(fd, "", AT_EMPTY_PATH, &mount_id_parent);
@@ -333,31 +341,27 @@ fallback_fdinfo:
 
         /* Hmm, so, the mount ids are the same. This leaves one special case though for the root file
          * system. For that, let's see if the parent directory has the same inode as we are interested
-         * in. Hence, let's also do fstat() checks now, too, but avoid the st_dev comparisons, since they
-         * aren't that useful on unionfs mounts. */
-        check_st_dev = false;
+         * in. */
+
+        struct stat a, b;
 
-fallback_fstat:
         /* yay for fstatat() taking a different set of flags than the other _at() above */
-        if (flags & AT_SYMLINK_FOLLOW)
-                flags &= ~AT_SYMLINK_FOLLOW;
-        else
-                flags |= AT_SYMLINK_NOFOLLOW;
-        if (fstatat(fd, filename, &a, flags) < 0)
+        if (fstatat(fd, filename, &a, at_flags_normalize_nofollow(flags)) < 0)
                 return -errno;
 
-        if (isempty(filename))
+        if (fd_is_self)
                 r = fstatat(fd, "..", &b, 0);
         else
                 r = fstatat(fd, "", &b, AT_EMPTY_PATH);
         if (r < 0)
                 return -errno;
 
-        /* A directory with same device and inode as its parent? Must be the root directory */
-        if (stat_inode_same(&a, &b))
-                return 1;
-
-        return check_st_dev && (a.st_dev != b.st_dev);
+        /* A directory with same device and inode as its parent must be the root directory. Otherwise
+         * not a mount point.
+         *
+         * NB: we avoid inode_same_at() here because it internally attempts name_to_handle_at_try_fid() first,
+         * which is redundant. */
+        return stat_inode_same(&a, &b);
 }
 
 /* flags can be AT_SYMLINK_FOLLOW or 0 */

diff --git a/src/test/test-mountpoint-util.c b/src/test/test-mountpoint-util.c
index 89093d021244461f92b26f9cba00cc2f021eac43..dc202cbda6defba3ffb7ad89b05012cde4b9a4be 100644 (file)
--- a/src/test/test-mountpoint-util.c
+++ b/src/test/test-mountpoint-util.c
@@ -273,6 +273,7 @@ TEST(path_is_mount_point) {
 
 TEST(fd_is_mount_point) {
         _cleanup_(rm_rf_physical_and_freep) char *tmpdir = NULL;
+        _cleanup_free_ char *pwd = NULL;
         _cleanup_close_ int fd = -EBADF;
         int r;
 
@@ -281,11 +282,8 @@ TEST(fd_is_mount_point) {
 
         /* Not allowed, since "/" is a path, not a plain filename */
         assert_se(fd_is_mount_point(fd, "/", 0) == -EINVAL);
-        assert_se(fd_is_mount_point(fd, ".", 0) == -EINVAL);
-        assert_se(fd_is_mount_point(fd, "./", 0) == -EINVAL);
         assert_se(fd_is_mount_point(fd, "..", 0) == -EINVAL);
         assert_se(fd_is_mount_point(fd, "../", 0) == -EINVAL);
-        assert_se(fd_is_mount_point(fd, "", 0) == -EINVAL);
         assert_se(fd_is_mount_point(fd, "/proc", 0) == -EINVAL);
         assert_se(fd_is_mount_point(fd, "/proc/", 0) == -EINVAL);
         assert_se(fd_is_mount_point(fd, "proc/sys", 0) == -EINVAL);
@@ -307,9 +305,20 @@ TEST(fd_is_mount_point) {
         fd = open("/proc", O_RDONLY|O_CLOEXEC|O_DIRECTORY|O_NOCTTY);
         assert_se(fd >= 0);
 
-        assert_se(fd_is_mount_point(fd, NULL, 0) > 0);
-        assert_se(fd_is_mount_point(fd, "", 0) == -EINVAL);
-        assert_se(fd_is_mount_point(fd, "version", 0) == 0);
+        ASSERT_OK_POSITIVE(fd_is_mount_point(fd, NULL, 0));
+        ASSERT_OK_POSITIVE(fd_is_mount_point(fd, "", 0));
+        ASSERT_OK_POSITIVE(fd_is_mount_point(fd, ".", 0));
+        ASSERT_OK_POSITIVE(fd_is_mount_point(fd, "./", 0));
+        ASSERT_OK_ZERO(fd_is_mount_point(fd, "version", 0));
+
+        ASSERT_OK(safe_getcwd(&pwd));
+        ASSERT_OK_ERRNO(fchdir(fd));
+
+        ASSERT_OK_POSITIVE(fd_is_mount_point(AT_FDCWD, NULL, 0));
+        ASSERT_OK_POSITIVE(fd_is_mount_point(AT_FDCWD, "", 0));
+        ASSERT_OK_POSITIVE(fd_is_mount_point(AT_FDCWD, "./", 0));
+
+        ASSERT_OK_ERRNO(chdir(pwd));
 
         safe_close(fd);
         fd = open("/proc/version", O_RDONLY|O_CLOEXEC|O_NOCTTY);
@@ -317,7 +326,6 @@ TEST(fd_is_mount_point) {
 
         r = fd_is_mount_point(fd, NULL, 0);
         assert_se(IN_SET(r, 0, -ENOTDIR)); /* on old kernels we can't determine if regular files are mount points if we have no directory fd */
-        assert_se(fd_is_mount_point(fd, "", 0) == -EINVAL);
 
         if (!mount_new_api_supported())
                 return;