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;
}