From fc010b01e7a2d02130269add7c0a0ebbc7171d30 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2017 12:42:24 +0100 Subject: [PATCH 1/5] mount-util: drop exponential buffer growing in name_to_handle_at_loop() So, it appears name_to_handle_at() always returns the right buffer size on EOVERFLOW, when it's returned due to a too small buffer. Let's rely on that exclusively for sizing the buffer, and let's drop the exponential buffer growing. The new logic is now: if we see EOVERFLOW and the returned size has increased, resize our buffer and try again. But if it didn't increase, then propagate the EOVERFLOW as it likely has other causes. --- src/basic/mount-util.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/basic/mount-util.c b/src/basic/mount-util.c index 5f22bd68ce..fc7b0b599e 100644 --- a/src/basic/mount-util.c +++ b/src/basic/mount-util.c @@ -65,7 +65,6 @@ int name_to_handle_at_loop( for (;;) { int mnt_id = -1; - size_t m; if (name_to_handle_at(fd, path, h, &mnt_id, flags) >= 0) { @@ -84,22 +83,24 @@ int name_to_handle_at_loop( if (!ret_handle && ret_mnt_id && mnt_id >= 0) { - /* As it appears, name_to_handle_at() fills in mnt_id even when it returns EOVERFLOW, but - * that's undocumented. Hence, let's make use of this if it appears to be filled in, and the - * caller was interested in only the mount ID an nothing else. */ + /* As it appears, name_to_handle_at() fills in mnt_id even when it returns EOVERFLOW when the + * buffer is too small, but that's undocumented. Hence, let's make use of this if it appears to + * be filled in, and the caller was interested in only the mount ID an nothing else. */ *ret_mnt_id = mnt_id; return 0; } - /* The buffer was too small. Size the new buffer by what name_to_handle_at() returned, but make sure it - * is always larger than what we passed in before */ - m = h->handle_bytes > n ? h->handle_bytes : n * 2; - if (m < n) /* Check for multiplication overflow */ + /* If name_to_handle_at() didn't increase the byte size, then this EOVERFLOW is caused by something + * else (apparently EOVERFLOW is returned for untriggered nfs4 mounts sometimes), not by the too small + * buffer. In that case propagate EOVERFLOW */ + if (h->handle_bytes <= n) return -EOVERFLOW; - if (offsetof(struct file_handle, f_handle) + m < m) /* check for addition overflow */ + + /* The buffer was too small. Size the new buffer by what name_to_handle_at() returned. */ + n = h->handle_bytes; + if (offsetof(struct file_handle, f_handle) + n < n) /* check for addition overflow */ return -EOVERFLOW; - n = m; free(h); h = malloc0(offsetof(struct file_handle, f_handle) + n); From 0d9bcb7c372e4823e83f4366d1d5239b2044d038 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2017 12:44:17 +0100 Subject: [PATCH 2/5] mount-util: fix error propagation in fd_fdinfo_mnt_id() --- src/basic/mount-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/mount-util.c b/src/basic/mount-util.c index fc7b0b599e..ac072fa0a5 100644 --- a/src/basic/mount-util.c +++ b/src/basic/mount-util.c @@ -132,7 +132,7 @@ static int fd_fdinfo_mnt_id(int fd, const char *filename, int flags, int *mnt_id if (r == -ENOENT) /* The fdinfo directory is a relatively new addition */ return -EOPNOTSUPP; if (r < 0) - return -errno; + return r; p = startswith(fdinfo, "mnt_id:"); if (!p) { From c83b20d73bc839cbb9d08eec682d4970992a78b5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2017 12:44:49 +0100 Subject: [PATCH 3/5] mount-util: EOVERFLOW might have other causes than buffer size issues When we get EOVERFLOW this might be caused by untriggered nfs4 mounts (see discussion at https://github.com/systemd/systemd/pull/7395#issuecomment-346164481 and further down). Handle this nicely by falling back to fdinfo-based mntid determination. Fixes: #7082 --- src/basic/mount-util.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/basic/mount-util.c b/src/basic/mount-util.c index ac072fa0a5..d20da3407b 100644 --- a/src/basic/mount-util.c +++ b/src/basic/mount-util.c @@ -183,9 +183,10 @@ int fd_is_mount_point(int fd, const char *filename, int flags) { * real mounts of their own. */ r = name_to_handle_at_loop(fd, filename, &h, &mount_id, flags); - if (IN_SET(r, -ENOSYS, -EACCES, -EPERM)) - /* This kernel does not support name_to_handle_at() at all, or the syscall was blocked (maybe through - * seccomp, because we are running inside of a container?): fall back to simpler logic. */ + if (IN_SET(r, -ENOSYS, -EACCES, -EPERM, -EOVERFLOW)) + /* This kernel does not support name_to_handle_at() at all (ENOSYS), or the syscall was blocked + * (EACCES/EPERM; maybe through seccomp, because we are running inside of a container?), or the mount + * point is not triggered yet (EOVERFLOW, thinkg nfs4): fall back to simpler logic. */ goto fallback_fdinfo; else if (r == -EOPNOTSUPP) /* This kernel or file system does not support name_to_handle_at(), hence let's see if the upper fs @@ -307,7 +308,7 @@ int path_get_mnt_id(const char *path, int *ret) { int r; r = name_to_handle_at_loop(AT_FDCWD, path, NULL, ret, 0); - if (IN_SET(r, -EOPNOTSUPP, -ENOSYS, -EACCES, -EPERM)) /* kernel/fs don't support this, or seccomp blocks access */ + if (IN_SET(r, -EOPNOTSUPP, -ENOSYS, -EACCES, -EPERM, -EOVERFLOW)) /* kernel/fs don't support this, or seccomp blocks access, or untriggered mount */ return fd_fdinfo_mnt_id(AT_FDCWD, path, 0, ret); return r; From 4739fc554dd90bb4ec3b3fa09a5e660bdac5a463 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2017 12:48:12 +0100 Subject: [PATCH 4/5] mount-util: fix bad indenting --- src/basic/mount-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/mount-util.c b/src/basic/mount-util.c index d20da3407b..e32502308a 100644 --- a/src/basic/mount-util.c +++ b/src/basic/mount-util.c @@ -206,7 +206,7 @@ int fd_is_mount_point(int fd, const char *filename, int flags) { * it must be a mount point. */ return 1; } else if (r < 0) - return r; + return r; /* The parent can do name_to_handle_at() but the * directory we are interested in can't? If so, it From 9b1573ef76b54f72619adf12a1307eb39664952f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2017 12:48:42 +0100 Subject: [PATCH 5/5] test: fix test-mount-util when handling duplicate mounts on the same location The test was written so far under the assumption that if two mounts are placed onto the same location the "upper" mount is listed later in /proc/self/mountinfo. This appears not to be guaranteed however, as running the tests in a normal nspawn shows. This patch fixes that: it reverses the hashmap of mounts we build: instead of keying by path, we key by mnt_id, and if we notice that path_get_mnt_id() doesn't match what a line in /proc/self/mountinfo says, we use the returned ID to check if maybe another line agrees. Fixes: #7431 --- src/test/test-mount-util.c | 39 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/src/test/test-mount-util.c b/src/test/test-mount-util.c index 5a93da1c0d..9582d6ceec 100644 --- a/src/test/test-mount-util.c +++ b/src/test/test-mount-util.c @@ -27,6 +27,7 @@ #include "hashmap.h" #include "log.h" #include "mount-util.h" +#include "path-util.h" #include "string-util.h" static void test_mount_propagation_flags(const char *name, int ret, unsigned long expected) { @@ -51,16 +52,15 @@ static void test_mnt_id(void) { _cleanup_fclose_ FILE *f = NULL; Hashmap *h; Iterator i; - char *k; - void *p; + char *p; + void *k; int r; assert_se(f = fopen("/proc/self/mountinfo", "re")); - assert_se(h = hashmap_new(&string_hash_ops)); + assert_se(h = hashmap_new(&trivial_hash_ops)); for (;;) { _cleanup_free_ char *line = NULL, *path = NULL; - void *old_key; int mnt_id; r = read_line(f, LONG_LINE_MAX, &line); @@ -70,38 +70,35 @@ static void test_mnt_id(void) { assert_se(sscanf(line, "%i %*s %*s %*s %ms", &mnt_id, &path) == 2); - /* Add all mount points and their ids to a hashtable, so that we filter out mount points that are - * overmounted. For those we only care for the "upper" mount, since that's the only one - * path_get_mnt_id() can determine. */ - - if (hashmap_remove2(h, path, &old_key)) - free(old_key); - - assert_se(hashmap_put(h, path, INT_TO_PTR(mnt_id)) >= 0); + assert_se(hashmap_put(h, INT_TO_PTR(mnt_id), path) >= 0); path = NULL; } HASHMAP_FOREACH_KEY(p, k, h, i) { - int mnt_id = PTR_TO_INT(p), mnt_id2; + int mnt_id = PTR_TO_INT(k), mnt_id2; - r = path_get_mnt_id(k, &mnt_id2); + r = path_get_mnt_id(p, &mnt_id2); if (r == -EOPNOTSUPP) { /* kernel or file system too old? */ - log_debug("%s doesn't support mount IDs\n", k); + log_debug("%s doesn't support mount IDs\n", p); continue; } if (IN_SET(r, -EACCES, -EPERM)) { - log_debug("Can't access %s\n", k); + log_debug("Can't access %s\n", p); continue; } - log_debug("mnt id of %s is %i\n", k, mnt_id2); + log_debug("mnt id of %s is %i\n", p, mnt_id2); - assert_se(r >= 0); - assert_se(mnt_id == mnt_id2); + if (mnt_id == mnt_id2) + continue; + + /* The ids don't match? If so, then there are two mounts on the same path, let's check if that's really + * the case */ + assert_se(path_equal_ptr(hashmap_get(h, INT_TO_PTR(mnt_id2)), p)); } - while ((k = hashmap_steal_first_key(h))) - free(k); + while ((p = hashmap_steal_first(h))) + free(p); hashmap_free(h); }