xfsprogs: ensure growfs rejects non-existent mount point

xfs_growfs manpage clearly states that the filesystem must be mounted to be grown. Current behavior allows xfs_growfs to proceed if the filesystem /containing/ the path of the desired target is mounted. This is not the specified behavior. Instead, also check the targeted fs argument against the entry found in the fstable lookup. Unless the targeted fs is actually mounted, reject the command.

Signed-off-by Bill O'Donnell <[email protected]>

---
 growfs/xfs_growfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

growfs/xfs_growfs.c +0 -0

index a294e14..05630b8 100644

growfs/xfs_growfs.c +0 -0

 		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
 			progname, argv[optind]);
 		return 1;

Signature · 6 lines

2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

xfs_growfs manpage clearly states that the filesystem must be mounted to be grown. Current behavior allows xfs_growfs to proceed if the filesystem /containing/ the path of the desired target is mounted. This is not the specified behavior. Instead, also check the targeted fs argument against the entry found in the fstable lookup. Unless the targeted fs is actually mounted, reject the command.

Signed-off-by Bill O'Donnell <[email protected]>

---
 growfs/xfs_growfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

growfs/xfs_growfs.c +0 -0

index a294e14..05630b8 100644

Quoted 19 lines · Bill O'Donnell

xfs_growfs manpage clearly states that the filesystem must be mounted to be grown. Current behavior allows xfs_growfs to proceed if the filesystem /containing/ the path of the desired target is mounted. This is not the specified behavior. Instead, also check the targeted fs argument against the entry found in the fstable lookup. Unless the targeted fs is actually mounted, reject the command.

Signed-off-by Bill O'Donnell <[email protected]>

---
 growfs/xfs_growfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

growfs/xfs_growfs.c +0 -0

index a294e14..05630b8 100644

growfs/xfs_growfs.c main(int argc, char **argv) · line 209

Quoted 8 lines

growfs/xfs_growfs.c +1 -1

@@ -203,7 +203,7 @@ main(int argc, char **argv)
 
 	fs_table_initialise(0, NULL, 0, NULL);
 	fs = fs_table_lookup(argv[optind], FS_MOUNT_POINT);
-	if (!fs) {
+	if (!fs || (strcmp(argv[optind], fs->fs_dir) != 0)) {
 		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
 			progname, argv[optind]);
 		return 1;

Looks like the right start, but I see a few problems with this.

First, fs_dir contains a full path, so if we point at a relative path, this will fail now (with a little extra error annotation):

# growfs/xfs_growfs -n mnt
xfs_growfs: mnt is not a mounted XFS filesystem (failed compare to /mnt/test/mnt)

but will work if given a full path:

# growfs/xfs_growfs -n `pwd`/mnt
meta-data=/dev/loop0             isize=512    agcount=4, agsize=65536 blks
...

The path stuff in libxcmd tries to boil everything down to a realpath() - see:

050a7f1 xfsprogs: handle symlinks etc in fs_table_initialise_mounts() and
ed350fc libxcmd: make all comparisons using realpath'd paths

so if you want to compare paths to what's in the fstable, you'll need to realpath() them first to get the the same canonical pathname that it has stored.

Also, if we have the same filesystem mounted
in multiple ways, say: via loop, via a secondary mount, and via a bind mount (and maybe via a symlinked mount point?):

/mnt/test/fsfile on /mnt/test/mnt type xfs (rw,loop=/dev/loop0) /dev/loop0 on /mnt/test/mnt2 type xfs (rw) /mnt/test/mnt2 on /mnt/test/mnt3 type none (rw,bind)

it will fail on all but the first one it finds in the internal table:

# growfs/xfs_growfs -n `pwd`/mnt2
xfs_growfs: /mnt/test/mnt2 is not a mounted XFS filesystem (failed compare to /mnt/test/mnt)
# growfs/xfs_growfs -n `pwd`/mnt3
xfs_growfs: /mnt/test/mnt3 is not a mounted XFS filesystem (failed compare to /mnt/test/mnt)

because it the lookup stats the path, and picks the /first/ fstable entry which has a matching device, ignoring the mountpoint found in the fstable entry.

So I think all that indicates an xfstest is needed to validate the change, and a slightly different approach to finding a match.

Thanks,
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

xfs_growfs manpage clearly states that the filesystem must be mounted to be grown. Current behavior allows xfs_growfs to proceed if the filesystem /containing/ the path of the desired target is mounted. This is not the specified behavior. Instead, also check the targeted fs argument against the entry found in the fstable lookup. Unless the targeted fs is actually mounted, reject the command.

In order to cover bind-mounts, create a new lookup function based on the mountpoints instead of just the device name.

Signed-off-by Bill O'Donnell <[email protected]>

---

v2: in order to properly handle relative pathnames, symlinks, and bind-mounts, use realpath to establish canonical path name. This also requires the introduction of a lookup function based on the target mountpoint.

Quoted 14 lines · Bill O'Donnell

xfs_growfs manpage clearly states that the filesystem must be mounted to be grown. Current behavior allows xfs_growfs to proceed if the filesystem /containing/ the path of the desired target is mounted. This is not the specified behavior. Instead, also check the targeted fs argument against the entry found in the fstable lookup. Unless the targeted fs is actually mounted, reject the command.

In order to cover bind-mounts, create a new lookup function based on the mountpoints instead of just the device name.

Signed-off-by Bill O'Donnell <[email protected]>

---

Quoted 4 lines

v2: in order to properly handle relative pathnames, symlinks, and bind-mounts, use realpath to establish canonical path name. This also requires the introduction of a lookup function based on the target mountpoint.

I still need to review this one carefully, but have you made any progress towards an xfstest for this behavior?

Thanks,
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

xfs_growfs manpage clearly states that the filesystem must be mounted to be grown. Current behavior allows xfs_growfs to proceed if the filesystem /containing/ the path of the desired target is mounted. This is not the specified behavior. Instead, also check the targeted fs argument against the entry found in the fstable lookup. Unless the targeted fs is actually mounted, reject the command.

In order to cover bind-mounts, create a new lookup function based on the mountpoints instead of just the device name.

Signed-off-by Bill O'Donnell <[email protected]>

---

v2: in order to properly handle relative pathnames, symlinks, and bind-mounts, use realpath to establish canonical path name. This also requires the introduction of a lookup function based on the target mountpoint.

 growfs/xfs_growfs.c | 10 +++++++++-
 include/path.h      |  1 +
 libxcmd/paths.c     | 21 +++++++++++++++++++++
 3 files changed, 31 insertions(+), 1 deletion(-)

growfs/xfs_growfs.c +1 -0

index a294e14..a61e2f1 100644
@@ -133,6 +133,7 @@ main(int argc, char **argv)
 	int			spinodes;
 	int			rmapbt_enabled;
 	int			reflink_enabled;
+	char			rpath[PATH_MAX];
 
 	progname = basename(argv[0]);
 	setlocale(LC_ALL, "");

growfs/xfs_growfs.c +3 -0

+	}
+
+	fs = fs_table_lookup_mount(rpath);
 	if (!fs) {
 		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
 			progname, argv[optind]);

include/path.h +1 -0

index d077bac..1d3a902 100644
@@ -56,6 +56,7 @@ extern void fs_table_insert_project_path(char *__dir, uint __projid);
 
 
 extern fs_path_t *fs_table_lookup(const char *__dir, uint __flags);
+extern fs_path_t *fs_table_lookup_mount(const char *__dir);
 
 typedef struct fs_cursor {
 	uint		count;		/* total count of mount entries	*/

libxcmd/paths.c +21 -0

index 816acc2..10927c6 100644
@@ -86,6 +86,27 @@ fs_table_lookup(
 	return NULL;
 }
 
+/*
+ * Find the FS table entry for the given path. Compare
+ * the path to mountpoint entries in the table.
+ */
+struct fs_path *
+fs_table_lookup_mount(
+	const char *dir)
+{
+	uint		i;
+	dev_t		dev = 0;
+
+	if (fs_device_number(dir, &dev))
+		return NULL;
+
+	for (i = 0; i < fs_count; i++) {
+		if (strcmp(fs_table[i].fs_dir, dir) == 0)
+			return &fs_table[i];
+	}
+	return NULL;
+}
+
 static int
 fs_table_insert(
 	char		*dir,

Quoted 9 lines · Bill O'Donnell

libxcmd/paths.c +9 -0

+/*
+ * Find the FS table entry for the given path. Compare
+ * the path to mountpoint entries in the table.
+ */
+struct fs_path *
+fs_table_lookup_mount(
+	const char *dir)
+{
+	uint		i;

libxcmd/paths.c fs_table_lookup( · line 105

Quoted 8 lines

libxcmd/paths.c +8 -0

+	dev_t		dev = 0;
+
+	if (fs_device_number(dir, &dev))
+		return NULL;
+
+	for (i = 0; i < fs_count; i++) {
+		if (strcmp(fs_table[i].fs_dir, dir) == 0)
+			return &fs_table[i];

libxcmd/paths.c fs_table_lookup( · line 112

Quoted 8 lines

libxcmd/paths.c +4 -0

+	}
+	return NULL;
+}
+
 static int
 fs_table_insert(
 	char		*dir,

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

xfs_growfs manpage clearly states that the filesystem must be mounted to be grown.

Current behavior allows xfs_growfs
to proceed if the filesystem /containing/ the path of the desired target is mounted.

This is not the specified
behavior. Instead, also check the targeted fs argument against the entry found in the fstable lookup. Unless the targeted fs is actually mounted, reject the command.

In order to cover bind-mounts, create a new lookup function based on the mountpoints instead of just the device name.

Signed-off-by Bill O'Donnell <[email protected]>

---

v3: improve error message for realpath failure. Add comments to better document the differences between fs_table_lookup() and fs_table_lookup_mount(). Use realpath() in fs_table_lookup_mount() prior to directory comparison.

v2: in order to properly handle relative pathnames, symlinks, and bind-mounts, use realpath to establish canonical path name. This also requires the introduction of a lookup function based on the target mountpoint.

 growfs/xfs_growfs.c | 10 +++++++++-
 include/path.h      |  1 +
 libxcmd/paths.c     | 31 +++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)

growfs/xfs_growfs.c +1 -0

index a294e14..c3344d0 100644
@@ -133,6 +133,7 @@ main(int argc, char **argv)
 	int			spinodes;
 	int			rmapbt_enabled;
 	int			reflink_enabled;
+	char			rpath[PATH_MAX];
 
 	progname = basename(argv[0]);
 	setlocale(LC_ALL, "");

growfs/xfs_growfs.c +2 -0

+
+	fs = fs_table_lookup_mount(rpath);
 	if (!fs) {
 		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
 			progname, argv[optind]);

include/path.h +1 -0

index d077bac..1d3a902 100644
@@ -56,6 +56,7 @@ extern void fs_table_insert_project_path(char *__dir, uint __projid);
 
 
 extern fs_path_t *fs_table_lookup(const char *__dir, uint __flags);
+extern fs_path_t *fs_table_lookup_mount(const char *__dir);
 
 typedef struct fs_cursor {
 	uint		count;		/* total count of mount entries	*/

libxcmd/paths.c +31 -0

index 816acc2..b767e9d 100644
@@ -65,6 +65,9 @@ fs_device_number(
  * Find the FS table entry for the given path.  The "flags" argument
  * is a mask containing FS_MOUNT_POINT or FS_PROJECT_PATH (or both)
  * to indicate the type of table entry sought.
+ * fs_table_lookup() finds the fs table entry for the filesystem hosting
+ * the file represented in the "dir" argument. To compare against actual
+ * mount point entries, use fs_table_lookup_mount() instead.
  */
 struct fs_path *
 fs_table_lookup(

@@ -86,6 +89,34 @@ fs_table_lookup(
 	return NULL;
 }
 
+/*
+ * Find the FS table entry describing an actual mount for the given path.
+ * Unlike fs_table_lookup(), fs_table_lookup_mount() compares the "dir"
+ * argument to actual mount point entries in the table. Accordingly, it
+ * will find matches only if the "dir" argument is indeed mounted.
+ */
+struct fs_path *
+fs_table_lookup_mount(
+	const char	*dir)
+{
+	uint		i;
+	dev_t		dev = 0;
+	char		rpath[PATH_MAX];
+
+	if (fs_device_number(dir, &dev))
+		return NULL;
+
+	for (i = 0; i < fs_count; i++) {
+		if (fs_table[i].fs_flags != FS_MOUNT_POINT)
+			continue;
+		if (!realpath(fs_table[i].fs_dir, rpath))
+			continue;
+		if (strcmp(rpath, dir) == 0)
+			return &fs_table[i];
+	}
+	return NULL;
+}
+
 static int
 fs_table_insert(
 	char		*dir,

xfs_growfs manpage clearly states that the target path must be an active xfs mountpoint.

Current behavior allows xfs_growfs to proceed if the target path resides anywhere on a mounted xfs filesystem. This could lead to unexpected results. Unless the target path is an active xfs mountpoint, reject it. Create a new fs table lookup function which matches only active xfs mount points, not any file residing within those mountpoints.

Signed-off-by Bill O'Donnell <[email protected]>

---

v4: improve commit message and subject line. Further improvement of error message for realpath() prior to table lookup in xfs_growfs.

v3: improve error message for realpath failure. Add comments to better document the differences between fs_table_lookup() and fs_table_lookup_mount(). Use realpath() in fs_table_lookup_mount() prior to directory comparison.

v2: in order to properly handle relative pathnames, symlinks, and bind-mounts, use realpath to establish canonical path name. This also requires the introduction of a lookup function based on the target mountpoint.

 growfs/xfs_growfs.c | 10 +++++++++-
 include/path.h      |  1 +
 libxcmd/paths.c     | 31 +++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)

growfs/xfs_growfs.c +9 -1

index a294e14..366176b 100644
@@ -133,6 +133,7 @@ main(int argc, char **argv)
 	int			spinodes;
 	int			rmapbt_enabled;
 	int			reflink_enabled;
+	char			rpath[PATH_MAX];
 
 	progname = basename(argv[0]);
 	setlocale(LC_ALL, "");

@@ -202,7 +203,14 @@ main(int argc, char **argv)
 		aflag = 1;
 
 	fs_table_initialise(0, NULL, 0, NULL);
-	fs = fs_table_lookup(argv[optind], FS_MOUNT_POINT);
+
+	if (!realpath(argv[optind], rpath)) {
+		fprintf(stderr, _("%s: path resolution failed for %s: %s\n"),
+			progname, argv[optind], strerror(errno));
+		return 1;
+	}
+
+	fs = fs_table_lookup_mount(rpath);
 	if (!fs) {
 		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
 			progname, argv[optind]);

include/path.h +1 -0

index d077bac..1d3a902 100644
@@ -56,6 +56,7 @@ extern void fs_table_insert_project_path(char *__dir, uint __projid);
 
 
 extern fs_path_t *fs_table_lookup(const char *__dir, uint __flags);
+extern fs_path_t *fs_table_lookup_mount(const char *__dir);
 
 typedef struct fs_cursor {
 	uint		count;		/* total count of mount entries	*/

libxcmd/paths.c +31 -0

index 816acc2..b767e9d 100644
@@ -65,6 +65,9 @@ fs_device_number(
  * Find the FS table entry for the given path.  The "flags" argument
  * is a mask containing FS_MOUNT_POINT or FS_PROJECT_PATH (or both)
  * to indicate the type of table entry sought.
+ * fs_table_lookup() finds the fs table entry for the filesystem hosting
+ * the file represented in the "dir" argument. To compare against actual
+ * mount point entries, use fs_table_lookup_mount() instead.
  */
 struct fs_path *
 fs_table_lookup(

@@ -86,6 +89,34 @@ fs_table_lookup(
 	return NULL;
 }
 
+/*
+ * Find the FS table entry describing an actual mount for the given path.
+ * Unlike fs_table_lookup(), fs_table_lookup_mount() compares the "dir"
+ * argument to actual mount point entries in the table. Accordingly, it
+ * will find matches only if the "dir" argument is indeed mounted.
+ */
+struct fs_path *
+fs_table_lookup_mount(
+	const char	*dir)
+{
+	uint		i;
+	dev_t		dev = 0;
+	char		rpath[PATH_MAX];
+
+	if (fs_device_number(dir, &dev))
+		return NULL;
+
+	for (i = 0; i < fs_count; i++) {
+		if (fs_table[i].fs_flags != FS_MOUNT_POINT)
+			continue;
+		if (!realpath(fs_table[i].fs_dir, rpath))
+			continue;
+		if (strcmp(rpath, dir) == 0)
+			return &fs_table[i];
+	}
+	return NULL;
+}
+
 static int
 fs_table_insert(
 	char		*dir,

Quoted 10 lines · Bill O'Donnell

xfs_growfs manpage clearly states that the target path must be an active xfs mountpoint.

Current behavior allows xfs_growfs to proceed if the target path resides anywhere on a mounted xfs filesystem. This could lead to unexpected results. Unless the target path is an active xfs mountpoint, reject it. Create a new fs table lookup function which matches only active xfs mount points, not any file residing within those mountpoints.

Quoted 1 line

Signed-off-by Bill O'Donnell <[email protected]>

Looks good, thanks. And thanks for putting up with my pedantry.

I will probably queue this for the 4.12 release since it's fairly user-visible and 4.11 is close to release (he says, hopefully).

Reviewed-by Eric Sandeen <[email protected]>

Quoted 100 lines

---

v4: improve commit message and subject line. Further improvement of error message for realpath() prior to table lookup in xfs_growfs.

v3: improve error message for realpath failure. Add comments to better document the differences between fs_table_lookup() and fs_table_lookup_mount(). Use realpath() in fs_table_lookup_mount() prior to directory comparison.

v2: in order to properly handle relative pathnames, symlinks, and bind-mounts, use realpath to establish canonical path name. This also requires the introduction of a lookup function based on the target mountpoint.

 growfs/xfs_growfs.c | 10 +++++++++-
 include/path.h      |  1 +
 libxcmd/paths.c     | 31 +++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)

growfs/xfs_growfs.c +9 -1

index a294e14..366176b 100644
@@ -133,6 +133,7 @@ main(int argc, char **argv)
 	int			spinodes;
 	int			rmapbt_enabled;
 	int			reflink_enabled;
+	char			rpath[PATH_MAX];
 
 	progname = basename(argv[0]);
 	setlocale(LC_ALL, "");

@@ -202,7 +203,14 @@ main(int argc, char **argv)
 		aflag = 1;
 
 	fs_table_initialise(0, NULL, 0, NULL);
-	fs = fs_table_lookup(argv[optind], FS_MOUNT_POINT);
+
+	if (!realpath(argv[optind], rpath)) {
+		fprintf(stderr, _("%s: path resolution failed for %s: %s\n"),
+			progname, argv[optind], strerror(errno));
+		return 1;
+	}
+
+	fs = fs_table_lookup_mount(rpath);
 	if (!fs) {
 		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
 			progname, argv[optind]);

include/path.h +1 -0

index d077bac..1d3a902 100644
@@ -56,6 +56,7 @@ extern void fs_table_insert_project_path(char *__dir, uint __projid);
 
 
 extern fs_path_t *fs_table_lookup(const char *__dir, uint __flags);
+extern fs_path_t *fs_table_lookup_mount(const char *__dir);
 
 typedef struct fs_cursor {
 	uint		count;		/* total count of mount entries	*/

libxcmd/paths.c +24 -0

index 816acc2..b767e9d 100644
@@ -65,6 +65,9 @@ fs_device_number(
  * Find the FS table entry for the given path.  The "flags" argument
  * is a mask containing FS_MOUNT_POINT or FS_PROJECT_PATH (or both)
  * to indicate the type of table entry sought.
+ * fs_table_lookup() finds the fs table entry for the filesystem hosting
+ * the file represented in the "dir" argument. To compare against actual
+ * mount point entries, use fs_table_lookup_mount() instead.
  */
 struct fs_path *
 fs_table_lookup(

@@ -86,6 +89,34 @@ fs_table_lookup(
 	return NULL;
 }
 
+/*
+ * Find the FS table entry describing an actual mount for the given path.
+ * Unlike fs_table_lookup(), fs_table_lookup_mount() compares the "dir"
+ * argument to actual mount point entries in the table. Accordingly, it
+ * will find matches only if the "dir" argument is indeed mounted.
+ */
+struct fs_path *
+fs_table_lookup_mount(
+	const char	*dir)
+{
+	uint		i;
+	dev_t		dev = 0;
+	char		rpath[PATH_MAX];
+
+	if (fs_device_number(dir, &dev))
+		return NULL;
+
+	for (i = 0; i < fs_count; i++) {
+		if (fs_table[i].fs_flags != FS_MOUNT_POINT)
+			continue;
+		if (!realpath(fs_table[i].fs_dir, rpath))

libxcmd/paths.c fs_table_lookup( · line 122

Quoted 10 lines

libxcmd/paths.c +7 -0

+			continue;
+		if (strcmp(rpath, dir) == 0)
+			return &fs_table[i];
+	}
+	return NULL;
+}
+
 static int
 fs_table_insert(
 	char		*dir,

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html