From f3d485e1034cdea60cab3c257e340dd9b3e48fc1 Mon Sep 17 00:00:00 2001

From: Lukas Nykryn <lnykryn@redhat.com>

Date: Mon, 18 Sep 2017 10:13:05 +0200

Subject: [PATCH] Revert "path-util: make use of "mnt_id" field exported in

 /proc/self/fdinfo/<fd>"

This reverts commit f63b66b6347a8d8e5e6930a939d1997bfd8e2e7c.

This implementation was not working becuase of misbehaving

canonicalize_file_name.

Related: #1472439

---

 src/core/automount.c                        |   2 +-

 src/core/machine-id-setup.c                 |   2 +-

 src/core/mount-setup.c                      |   2 +-

 src/efi-boot-generator/efi-boot-generator.c |   2 +-

 src/gpt-auto-generator/gpt-auto-generator.c |   2 +-

 src/login/logind-user.c                     |   2 +-

 src/nspawn/nspawn.c                         |  10 +-

 src/shared/cgroup-util.c                    |   2 +-

 src/shared/condition.c                      |   2 +-

 src/shared/path-util.c                      | 209 +++++---------------

 src/shared/path-util.h                      |   3 +-

 src/test/test-path-util.c                   |  66 +------

 12 files changed, 62 insertions(+), 242 deletions(-)

diff --git a/src/core/automount.c b/src/core/automount.c

index eedd9b824..4e066613d 100644

--- a/src/core/automount.c

+++ b/src/core/automount.c

@@ -749,7 +749,7 @@ static int automount_start(Unit *u) {

         assert(a);

         assert(a->state == AUTOMOUNT_DEAD || a->state == AUTOMOUNT_FAILED);

-        if (path_is_mount_point(a->where, 0)) {

+        if (path_is_mount_point(a->where, false)) {

                 log_unit_error(u->id,

                                "Path %s is already a mount point, refusing start for %s",

                                a->where, u->id);

diff --git a/src/core/machine-id-setup.c b/src/core/machine-id-setup.c

index 1121d373f..d00a53246 100644

--- a/src/core/machine-id-setup.c

+++ b/src/core/machine-id-setup.c

@@ -203,7 +203,7 @@ int machine_id_commit(const char *root) {

                 etc_machine_id = path_kill_slashes(x);

         }

-        r = path_is_mount_point(etc_machine_id, 0);

+        r = path_is_mount_point(etc_machine_id, false);

         if (r < 0)

                 return log_error_errno(r, "Failed to determine whether %s is a mount point: %m", etc_machine_id);

         if (r == 0) {

diff --git a/src/core/mount-setup.c b/src/core/mount-setup.c

index 2b8fbab1a..521545e5c 100644

--- a/src/core/mount-setup.c

+++ b/src/core/mount-setup.c

@@ -160,7 +160,7 @@ static int mount_one(const MountPoint *p, bool relabel) {

         if (relabel)

                 label_fix(p->where, true, true);

-        r = path_is_mount_point(p->where, AT_SYMLINK_FOLLOW);

+        r = path_is_mount_point(p->where, true);

         if (r < 0)

                 return r;

diff --git a/src/efi-boot-generator/efi-boot-generator.c b/src/efi-boot-generator/efi-boot-generator.c

index 5492b1994..b3ff3a8b7 100644

--- a/src/efi-boot-generator/efi-boot-generator.c

+++ b/src/efi-boot-generator/efi-boot-generator.c

@@ -69,7 +69,7 @@ int main(int argc, char *argv[]) {

                 return EXIT_SUCCESS;

         }

-        if (path_is_mount_point("/boot", AT_SYMLINK_FOLLOW) <= 0 &&

+        if (path_is_mount_point("/boot", true) <= 0 &&

             dir_is_empty("/boot") <= 0) {

                 log_debug("/boot already populated, exiting.");

                 return EXIT_SUCCESS;

diff --git a/src/gpt-auto-generator/gpt-auto-generator.c b/src/gpt-auto-generator/gpt-auto-generator.c

index d7b047118..00a2141a5 100644

--- a/src/gpt-auto-generator/gpt-auto-generator.c

+++ b/src/gpt-auto-generator/gpt-auto-generator.c

@@ -299,7 +299,7 @@ static int probe_and_add_mount(

         assert(where);

         assert(description);

-        if (path_is_mount_point(where, AT_SYMLINK_FOLLOW) <= 0 &&

+        if (path_is_mount_point(where, true) <= 0 &&

             dir_is_empty(where) <= 0) {

                 log_debug("%s already populated, ignoring.", where);

                 return 0;

diff --git a/src/login/logind-user.c b/src/login/logind-user.c

index 912c50ebd..4298704ce 100644

--- a/src/login/logind-user.c

+++ b/src/login/logind-user.c

@@ -320,7 +320,7 @@ static int user_mkdir_runtime_path(User *u) {

         } else

                 p = u->runtime_path;

-        if (path_is_mount_point(p, 0) <= 0) {

+        if (path_is_mount_point(p, false) <= 0) {

                 _cleanup_free_ char *t = NULL;

                 (void) mkdir(p, 0700);

diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c

index a90a3a5d7..ea365b3f9 100644

--- a/src/nspawn/nspawn.c

+++ b/src/nspawn/nspawn.c

@@ -863,7 +863,7 @@ static int mount_all(const char *dest) {

                 if (!where)

                         return log_oom();

-                t = path_is_mount_point(where, AT_SYMLINK_FOLLOW);

+                t = path_is_mount_point(where, true);

                 if (t < 0) {

                         log_error_errno(t, "Failed to detect whether %s is a mount point: %m", where);

@@ -989,7 +989,7 @@ static int mount_cgroup_hierarchy(const char *dest, const char *controller, cons

         to = strjoina(dest, "/sys/fs/cgroup/", hierarchy);

-        r = path_is_mount_point(to, 0);

+        r = path_is_mount_point(to, false);

         if (r < 0)

                 return log_error_errno(r, "Failed to determine if %s is mounted already: %m", to);

         if (r > 0)

@@ -1787,7 +1787,7 @@ static int setup_journal(const char *directory) {

         if (!p || !q)

                 return log_oom();

-        if (path_is_mount_point(p, 0) > 0) {

+        if (path_is_mount_point(p, false) > 0) {

                 if (arg_link_journal != LINK_AUTO) {

                         log_error("%s: already a mount point, refusing to use for journal", p);

                         return -EEXIST;

@@ -1796,7 +1796,7 @@ static int setup_journal(const char *directory) {

                 return 0;

         }

-        if (path_is_mount_point(q, 0) > 0) {

+        if (path_is_mount_point(q, false) > 0) {

                 if (arg_link_journal != LINK_AUTO) {

                         log_error("%s: already a mount point, refusing to use for journal", q);

                         return -EEXIST;

@@ -3665,7 +3665,7 @@ int main(int argc, char *argv[]) {

                          * the specified is not a mount point we

                          * create the new snapshot in the parent

                          * directory, just next to it. */

-                        r = path_is_mount_point(arg_directory, 0);

+                        r = path_is_mount_point(arg_directory, false);

                         if (r < 0) {

                                 log_error_errno(r, "Failed to determine whether directory %s is mount point: %m", arg_directory);

                                 goto finish;

diff --git a/src/shared/cgroup-util.c b/src/shared/cgroup-util.c

index cf085cb5f..c5d9e4bb5 100644

--- a/src/shared/cgroup-util.c

+++ b/src/shared/cgroup-util.c

@@ -488,7 +488,7 @@ int cg_get_path(const char *controller, const char *path, const char *suffix, ch

         if (_unlikely_(!good)) {

                 int r;

-                r = path_is_mount_point("/sys/fs/cgroup", 0);

+                r = path_is_mount_point("/sys/fs/cgroup", false);

                 if (r <= 0)

                         return r < 0 ? r : -ENOENT;

diff --git a/src/shared/condition.c b/src/shared/condition.c

index 0d2cd2bc3..796cc520d 100644

--- a/src/shared/condition.c

+++ b/src/shared/condition.c

@@ -350,7 +350,7 @@ static int condition_test_path_is_mount_point(Condition *c) {

         assert(c->parameter);

         assert(c->type == CONDITION_PATH_IS_MOUNT_POINT);

-        return path_is_mount_point(c->parameter, AT_SYMLINK_FOLLOW) > 0;

+        return path_is_mount_point(c->parameter, true) > 0;

 }

 static int condition_test_path_is_read_write(Condition *c) {

diff --git a/src/shared/path-util.c b/src/shared/path-util.c

index 0f252ec26..1181ffb9d 100644

--- a/src/shared/path-util.c

+++ b/src/shared/path-util.c

@@ -36,7 +36,6 @@

 #include "strv.h"

 #include "path-util.h"

 #include "missing.h"

-#include "fileio.h"

 bool path_is_absolute(const char *p) {

         return p[0] == '/';

@@ -474,203 +473,87 @@ char* path_join(const char *root, const char *path, const char *rest) {

                                NULL);

 }

-static int fd_fdinfo_mnt_id(int fd, const char *filename, int flags, int *mnt_id) {

-        char path[strlen("/proc/self/fdinfo/") + DECIMAL_STR_MAX(int)];

-        _cleanup_free_ char *fdinfo = NULL;

-        _cleanup_close_ int subfd = -1;

-        char *p;

-        int r;

-

-        if ((flags & AT_EMPTY_PATH) && isempty(filename))

-                xsprintf(path, "/proc/self/fdinfo/%i", fd);

-        else {

-                subfd = openat(fd, filename, O_RDONLY|O_CLOEXEC|O_NOCTTY|O_PATH);

-                if (subfd < 0)

-                        return -errno;

-

-                xsprintf(path, "/proc/self/fdinfo/%i", subfd);

-        }

-

-        r = read_full_file(path, &fdinfo, NULL);

-        if (r == -ENOENT) /* The fdinfo directory is a relatively new addition */

-                return -EOPNOTSUPP;

-        if (r < 0)

-                return -errno;

-

-        p = startswith(fdinfo, "mnt_id:");

-        if (!p) {

-                p = strstr(fdinfo, "\nmnt_id:");

-                if (!p) /* The mnt_id field is a relatively new addition */

-                        return -EOPNOTSUPP;

-

-                p += 8;

-        }

+int path_is_mount_point(const char *t, bool allow_symlink) {

-        p += strspn(p, WHITESPACE);

-        p[strcspn(p, WHITESPACE)] = 0;

-

-        return safe_atoi(p, mnt_id);

-}

-

-int fd_is_mount_point(int fd, const char *filename, int flags) {

-        union file_handle_union h = FILE_HANDLE_INIT, h_parent = FILE_HANDLE_INIT;

+        union file_handle_union h = FILE_HANDLE_INIT;

         int mount_id = -1, mount_id_parent = -1;

-        bool nosupp = false, check_st_dev = true;

+        _cleanup_free_ char *parent = NULL;

         struct stat a, b;

         int r;

+        bool nosupp = false;

-        assert(fd >= 0);

-        assert(filename);

+        /* We are not actually interested in the file handles, but

+         * name_to_handle_at() also passes us the mount ID, hence use

+         * it but throw the handle away */

-        /* First we will try the name_to_handle_at() syscall, which

-         * tells us the mount id and an opaque file "handle". It is

-         * not supported everywhere though (kernel compile-time

-         * option, not all file systems are hooked up). If it works

-         * the mount id is usually good enough to tell us whether

-         * something is a mount point.

-         *

-         * 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.

-         *

-         * As last fallback we do traditional fstat() based st_dev

-         * comparisons. This is how things were traditionally done,

-         * but unionfs breaks 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. */

-

-        r = name_to_handle_at(fd, filename, &h.handle, &mount_id, flags);

+        if (path_equal(t, "/"))

+                return 1;

+

+        r = name_to_handle_at(AT_FDCWD, t, &h.handle, &mount_id, allow_symlink ? AT_SYMLINK_FOLLOW : 0);

         if (r < 0) {

                 if (errno == ENOSYS)

                         /* This kernel does not support name_to_handle_at()

-                         * fall back to simpler logic. */

-                        goto fallback_fdinfo;

+                         * fall back to the traditional stat() logic. */

+                        goto fallback;

                 else if (errno == EOPNOTSUPP)

                         /* 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 fallback to the

+                         * name_to_handle_at(), hence fallback to the

                          * traditional stat() logic */

                         nosupp = true;

+                else if (errno == ENOENT)

+                        return 0;

                 else

                         return -errno;

         }

-        r = name_to_handle_at(fd, "", &h_parent.handle, &mount_id_parent, AT_EMPTY_PATH);

-        if (r < 0) {

-                if (errno == EOPNOTSUPP) {

+        r = path_get_parent(t, &parent);

+        if (r < 0)

+                return r;

+

+        h.handle.handle_bytes = MAX_HANDLE_SZ;

+        r = name_to_handle_at(AT_FDCWD, parent, &h.handle, &mount_id_parent, AT_SYMLINK_FOLLOW);

+        if (r < 0)

+                if (errno == EOPNOTSUPP)

                         if (nosupp)

                                 /* Neither parent nor child do name_to_handle_at()?

                                    We have no choice but to fall back. */

-                                goto fallback_fdinfo;

+                                goto fallback;

                         else

-                                /* The parent can't do name_to_handle_at() but the

-                                 * directory we are interested in can?

+                                /* The parent can't do name_to_handle_at() but

+                                 * the directory we are interested in can?

+                                 * Or the other way around?

                                  * If so, it must be a mount point. */

                                 return 1;

-                } else

+                else

                         return -errno;

-        }

+        else

+                return mount_id != mount_id_parent;

-        /* The parent can do name_to_handle_at() but the

-         * directory we are interested in can't? If so, it

-         * must be a mount point. */

-        if (nosupp)

-                return 1;

+fallback:

+        if (allow_symlink)

+                r = stat(t, &a);

+        else

+                r = lstat(t, &a);

-        /* If the file handle for the directory we are

-         * interested in and its parent are identical, we

-         * assume this is the root directory, which is a mount

-         * point. */

+        if (r < 0) {

+                if (errno == ENOENT)

+                        return 0;

-        if (h.handle.handle_bytes == h_parent.handle.handle_bytes &&

-            h.handle.handle_type == h_parent.handle.handle_type &&

-            memcmp(h.handle.f_handle, h_parent.handle.f_handle, h.handle.handle_bytes) == 0)

-                return 1;

+                return -errno;

+        }

-        return mount_id != mount_id_parent;

+        free(parent);

+        parent = NULL;

-fallback_fdinfo:

-        r = fd_fdinfo_mnt_id(fd, filename, flags, &mount_id);

-        if (r == -EOPNOTSUPP)

-                goto fallback_fstat;

+        r = path_get_parent(t, &parent);

         if (r < 0)

                 return r;

-        r = fd_fdinfo_mnt_id(fd, "", AT_EMPTY_PATH, &mount_id_parent);

+        r = stat(parent, &b);

         if (r < 0)

-                return r;

-

-        if (mount_id != mount_id_parent)

-                return 1;

-

-        /* 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;

-

-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)

-                return -errno;

-

-        if (fstatat(fd, "", &b, AT_EMPTY_PATH) < 0)

-                return -errno;

-

-        /* A directory with same device and inode as its parent? Must

-         * be the root directory */

-        if (a.st_dev == b.st_dev &&

-            a.st_ino == b.st_ino)

-                return 1;

-

-        return check_st_dev && (a.st_dev != b.st_dev);

-}

-

-/* flags can be AT_SYMLINK_FOLLOW or 0 */

-int path_is_mount_point(const char *t, int flags) {

-        _cleanup_close_ int fd = -1;

-        _cleanup_free_ char *canonical = NULL, *parent = NULL;

-

-        assert(t);

-

-        if (path_equal(t, "/"))

-                return 1;

-

-        /* we need to resolve symlinks manually, we can't just rely on

-         * fd_is_mount_point() to do that for us; if we have a structure like

-         * /bin -> /usr/bin/ and /usr is a mount point, then the parent that we

-         * look at needs to be /usr, not /. */

-        if (flags & AT_SYMLINK_FOLLOW) {

-                canonical = canonicalize_file_name(t);

-                if (!canonical)

-                        return -errno;

-

-                t = canonical;

-        }

-

-        parent = dirname_malloc(t);

-        if (!parent)

-                return -ENOMEM;

-

-        fd = openat(AT_FDCWD, parent, O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC|O_PATH);

-        if (fd < 0)

                 return -errno;

-        return fd_is_mount_point(fd, basename(t), flags);

+        return a.st_dev != b.st_dev;

 }

 int path_is_read_only_fs(const char *path) {

diff --git a/src/shared/path-util.h b/src/shared/path-util.h

index e16484087..71bb740e9 100644

--- a/src/shared/path-util.h

+++ b/src/shared/path-util.h

@@ -53,8 +53,7 @@ char** path_strv_make_absolute_cwd(char **l);

 char** path_strv_resolve(char **l, const char *prefix);

 char** path_strv_resolve_uniq(char **l, const char *prefix);

-int fd_is_mount_point(int fd, const char *filename, int flags);

-int path_is_mount_point(const char *path, int flags);

+int path_is_mount_point(const char *path, bool allow_symlink);

 int path_is_read_only_fs(const char *path);

 int path_is_os_tree(const char *path);

diff --git a/src/test/test-path-util.c b/src/test/test-path-util.c

index 8870f178a..6396fcb39 100644

--- a/src/test/test-path-util.c

+++ b/src/test/test-path-util.c

@@ -21,7 +21,6 @@

 #include <stdio.h>

 #include <unistd.h>

-#include <sys/mount.h>

 #include "path-util.h"

 #include "util.h"

@@ -86,8 +85,8 @@ static void test_path(void) {

         test_parent("/aa///file...", "/aa///");

         test_parent("file.../", NULL);

-        assert_se(path_is_mount_point("/", AT_SYMLINK_FOLLOW));

-        assert_se(path_is_mount_point("/", 0));

+        assert_se(path_is_mount_point("/", true));

+        assert_se(path_is_mount_point("/", false));

         {

                 char p1[] = "aaa/bbb////ccc";

@@ -100,66 +99,6 @@ static void test_path(void) {

         }

 }

-static void test_path_is_mount_point(void) {

-        int fd, rt, rf, rlt, rlf;

-        char tmp_dir[] = "/tmp/test-path-is-mount-point-XXXXXX";

-        _cleanup_free_ char *file1 = NULL, *file2 = NULL, *link1 = NULL, *link2 = NULL;

-

-        assert_se(path_is_mount_point("/", AT_SYMLINK_FOLLOW) > 0);

-        assert_se(path_is_mount_point("/", 0) > 0);

-

-        assert_se(path_is_mount_point("/proc", AT_SYMLINK_FOLLOW) > 0);

-        assert_se(path_is_mount_point("/proc", 0) > 0);

-

-        assert_se(path_is_mount_point("/proc/1", AT_SYMLINK_FOLLOW) == 0);

-        assert_se(path_is_mount_point("/proc/1", 0) == 0);

-

-        assert_se(path_is_mount_point("/sys", AT_SYMLINK_FOLLOW) > 0);

-        assert_se(path_is_mount_point("/sys", 0) > 0);

-

-        /* file mountpoints */

-        assert_se(mkdtemp(tmp_dir) != NULL);

-        file1 = path_join(NULL, tmp_dir, "file1");

-        assert_se(file1);

-        file2 = path_join(NULL, tmp_dir, "file2");

-        assert_se(file2);

-        fd = open(file1, O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0664);

-        assert_se(fd > 0);

-        close(fd);

-        fd = open(file2, O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0664);

-        assert_se(fd > 0);

-        close(fd);

-        link1 = path_join(NULL, tmp_dir, "link1");

-        assert_se(link1);

-        assert_se(symlink("file1", link1) == 0);

-        link2 = path_join(NULL, tmp_dir, "link2");

-        assert_se(link1);

-        assert_se(symlink("file2", link2) == 0);

-

-        assert_se(path_is_mount_point(file1, AT_SYMLINK_FOLLOW) == 0);

-        assert_se(path_is_mount_point(file1, 0) == 0);

-        assert_se(path_is_mount_point(link1, AT_SYMLINK_FOLLOW) == 0);

-        assert_se(path_is_mount_point(link1, 0) == 0);

-

-        /* this test will only work as root */

-        if (mount(file1, file2, NULL, MS_BIND, NULL) >= 0) {

-                rf = path_is_mount_point(file2, 0);

-                rt = path_is_mount_point(file2, AT_SYMLINK_FOLLOW);

-                rlf = path_is_mount_point(link2, 0);

-                rlt = path_is_mount_point(link2, AT_SYMLINK_FOLLOW);

-

-                assert_se(umount(file2) == 0);

-

-                assert_se(rf == 1);

-                assert_se(rt == 1);

-                assert_se(rlf == 0);

-                assert_se(rlt == 1);

-        } else

-                printf("Skipping bind mount file test: %m\n");

-

-        assert_se(rm_rf(tmp_dir, false, true, false) == 0);

-}

-

 static void test_find_binary(const char *self, bool local) {

         char *p;

@@ -349,7 +288,6 @@ int main(int argc, char **argv) {

         test_make_relative();

         test_strv_resolve();

         test_path_startswith();

-        test_path_is_mount_point();

         return 0;

 }