From f3d485e1034cdea60cab3c257e340dd9b3e48fc1 Mon Sep 17 00:00:00 2001 From: Lukas Nykryn 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/" 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/. 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 #include -#include #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; }