All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xfsprogs: ensure growfs rejects non-existent mount point
@ 2017-04-07 17:58 Bill O'Donnell
  2017-04-07 18:33 ` Darrick J. Wong
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Bill O'Donnell @ 2017-04-07 17:58 UTC (permalink / raw)
  To: linux-xfs

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 <billodo@redhat.com>
---
 growfs/xfs_growfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
index a294e14..05630b8 100644
--- a/growfs/xfs_growfs.c
+++ b/growfs/xfs_growfs.c
@@ -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;
-- 
2.9.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfsprogs: ensure growfs rejects non-existent mount point
  2017-04-07 17:58 [PATCH] xfsprogs: ensure growfs rejects non-existent mount point Bill O'Donnell
@ 2017-04-07 18:33 ` Darrick J. Wong
  2017-04-07 18:51 ` Eric Sandeen
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2017-04-07 18:33 UTC (permalink / raw)
  To: Bill O'Donnell; +Cc: linux-xfs

On Fri, Apr 07, 2017 at 12:58:09PM -0500, Bill O'Donnell wrote:
> 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 <billodo@redhat.com>
> ---
>  growfs/xfs_growfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
> index a294e14..05630b8 100644
> --- a/growfs/xfs_growfs.c
> +++ b/growfs/xfs_growfs.c
> @@ -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)) {

The parentheses around the strcmp comparison aren't necessary.

There's a find_mountpoint() function in xfs_fsr that wants to do
something similar to this.  I wonder if it'd make sense to change it
too?  (Separate patch.)

--D

>  		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
>  			progname, argv[optind]);
>  		return 1;
> -- 
> 2.9.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfsprogs: ensure growfs rejects non-existent mount point
  2017-04-07 17:58 [PATCH] xfsprogs: ensure growfs rejects non-existent mount point Bill O'Donnell
  2017-04-07 18:33 ` Darrick J. Wong
@ 2017-04-07 18:51 ` Eric Sandeen
  2017-04-11 23:20   ` Bill O'Donnell
  2017-04-11 17:22 ` [PATCH v2] " Bill O'Donnell
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Eric Sandeen @ 2017-04-07 18:51 UTC (permalink / raw)
  To: Bill O'Donnell, linux-xfs

On 4/7/17 12:58 PM, Bill O'Donnell wrote:
> 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 <billodo@redhat.com>
> ---
>  growfs/xfs_growfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
> index a294e14..05630b8 100644
> --- a/growfs/xfs_growfs.c
> +++ b/growfs/xfs_growfs.c
> @@ -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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2] xfsprogs: ensure growfs rejects non-existent mount point
  2017-04-07 17:58 [PATCH] xfsprogs: ensure growfs rejects non-existent mount point Bill O'Donnell
  2017-04-07 18:33 ` Darrick J. Wong
  2017-04-07 18:51 ` Eric Sandeen
@ 2017-04-11 17:22 ` Bill O'Donnell
  2017-04-19 22:05   ` Eric Sandeen
  2017-04-20 22:22   ` Eric Sandeen
  2017-04-24 14:29 ` [PATCH v3] " Bill O'Donnell
  2017-04-27 18:23 ` [PATCH v4] xfs_growfs: ensure target path is an active xfs mountpoint Bill O'Donnell
  4 siblings, 2 replies; 14+ messages in thread
From: Bill O'Donnell @ 2017-04-11 17:22 UTC (permalink / raw)
  To: linux-xfs

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 <billodo@redhat.com>
---

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

diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
index a294e14..a61e2f1 100644
--- a/growfs/xfs_growfs.c
+++ b/growfs/xfs_growfs.c
@@ -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: %s is not a mounted XFS filesystem\n"),
+			progname, argv[optind]);
+		return 1;
+	}
+
+	fs = fs_table_lookup_mount(rpath);
 	if (!fs) {
 		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
 			progname, argv[optind]);
diff --git a/include/path.h b/include/path.h
index d077bac..1d3a902 100644
--- a/include/path.h
+++ b/include/path.h
@@ -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	*/
diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index 816acc2..10927c6 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -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,
-- 
2.9.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] xfsprogs: ensure growfs rejects non-existent mount point
  2017-04-07 18:51 ` Eric Sandeen
@ 2017-04-11 23:20   ` Bill O'Donnell
  0 siblings, 0 replies; 14+ messages in thread
From: Bill O'Donnell @ 2017-04-11 23:20 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: linux-xfs

On Fri, Apr 07, 2017 at 01:51:47PM -0500, Eric Sandeen wrote:
> On 4/7/17 12:58 PM, Bill O'Donnell wrote:
> > 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 <billodo@redhat.com>
> > ---
> >  growfs/xfs_growfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
> > index a294e14..05630b8 100644
> > --- a/growfs/xfs_growfs.c
> > +++ b/growfs/xfs_growfs.c
> > @@ -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.

I submitted a v2. I'll also work on an xfstest.

Thanks-
Bill

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] xfsprogs: ensure growfs rejects non-existent mount point
  2017-04-11 17:22 ` [PATCH v2] " Bill O'Donnell
@ 2017-04-19 22:05   ` Eric Sandeen
  2017-04-19 22:08     ` Bill O'Donnell
  2017-04-20 22:22   ` Eric Sandeen
  1 sibling, 1 reply; 14+ messages in thread
From: Eric Sandeen @ 2017-04-19 22:05 UTC (permalink / raw)
  To: Bill O'Donnell, linux-xfs

On 4/11/17 12:22 PM, Bill O'Donnell wrote:
> 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 <billodo@redhat.com>
> ---
> 
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] xfsprogs: ensure growfs rejects non-existent mount point
  2017-04-19 22:05   ` Eric Sandeen
@ 2017-04-19 22:08     ` Bill O'Donnell
  0 siblings, 0 replies; 14+ messages in thread
From: Bill O'Donnell @ 2017-04-19 22:08 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: linux-xfs

On Wed, Apr 19, 2017 at 05:05:36PM -0500, Eric Sandeen wrote:
> On 4/11/17 12:22 PM, Bill O'Donnell wrote:
> > 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 <billodo@redhat.com>
> > ---
> > 
> > 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?

xfstest is still a work in progress. Hopefully will have one
by end of this week.
Thanks-
Bill


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] xfsprogs: ensure growfs rejects non-existent mount point
  2017-04-11 17:22 ` [PATCH v2] " Bill O'Donnell
  2017-04-19 22:05   ` Eric Sandeen
@ 2017-04-20 22:22   ` Eric Sandeen
  2017-04-20 22:43     ` Eric Sandeen
  1 sibling, 1 reply; 14+ messages in thread
From: Eric Sandeen @ 2017-04-20 22:22 UTC (permalink / raw)
  To: Bill O'Donnell, linux-xfs

On 4/11/17 12:22 PM, Bill O'Donnell wrote:
> 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 <billodo@redhat.com>
> ---
> 
> 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(-)
> 
> diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
> index a294e14..a61e2f1 100644
> --- a/growfs/xfs_growfs.c
> +++ b/growfs/xfs_growfs.c
> @@ -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: %s is not a mounted XFS filesystem\n"),
> +			progname, argv[optind]);
> +		return 1;

Probably not the clearest error message; if realpath failed, we
really don't know much at all about the path yet.

(_("Path resolution failed for %s: %s\n"), path, strerror(errno))
or something like that would be more informative if it fails..

Otherwise, I think this looks ok.  I was a little unsure about
keeping the old fs_table_lookup behavior around, but given that
xfs_growfs is the only utility (I think) which /explicitly/ says
it requires a mount point (and can have irreversible consequences
if it's pointed at the wrong thing) that's probably ok.

One other suggestion would be to document the difference between
the 2 lookup functions a bit more, making it more clear that one
will find the fs table entry for the fs hosting the file at *path,
and the other will find it only if it /is/ the mount point.

I had a concern that while you're passing a realpath'd path to
the new function, the mount points in the table aren't realpath'd.
But I /think/ that still works; if I mount onto a symlinked dir,
it's the realpath (symlink target) that ends up in /proc/mounts.

Still, it might be safest to realpath the path found in the
fs table as well prior to the comparison - if it's iterating
over /etc/mtab instead of /proc/mounts on an older machine, I'm
not sure what the paths end up looking like in that file. 

-Eric

> +	}
> +
> +	fs = fs_table_lookup_mount(rpath);
>  	if (!fs) {
>  		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
>  			progname, argv[optind]);
> diff --git a/include/path.h b/include/path.h
> index d077bac..1d3a902 100644
> --- a/include/path.h
> +++ b/include/path.h
> @@ -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	*/
> diff --git a/libxcmd/paths.c b/libxcmd/paths.c
> index 816acc2..10927c6 100644
> --- a/libxcmd/paths.c
> +++ b/libxcmd/paths.c
> @@ -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,
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] xfsprogs: ensure growfs rejects non-existent mount point
  2017-04-20 22:22   ` Eric Sandeen
@ 2017-04-20 22:43     ` Eric Sandeen
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Sandeen @ 2017-04-20 22:43 UTC (permalink / raw)
  To: Bill O'Donnell, linux-xfs


On 4/20/17 5:22 PM, Eric Sandeen wrote:
> On 4/11/17 12:22 PM, Bill O'Donnell wrote:


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

Oh, and I think we need to test the flags on the
entry to be sure that it is a FS_MOUNT_POINT as well,
and not a project path.

-Eric

>> +	}
>> +	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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3] xfsprogs: ensure growfs rejects non-existent mount point
  2017-04-07 17:58 [PATCH] xfsprogs: ensure growfs rejects non-existent mount point Bill O'Donnell
                   ` (2 preceding siblings ...)
  2017-04-11 17:22 ` [PATCH v2] " Bill O'Donnell
@ 2017-04-24 14:29 ` Bill O'Donnell
  2017-04-26 22:23   ` Eric Sandeen
  2017-04-27 18:23 ` [PATCH v4] xfs_growfs: ensure target path is an active xfs mountpoint Bill O'Donnell
  4 siblings, 1 reply; 14+ messages in thread
From: Bill O'Donnell @ 2017-04-24 14:29 UTC (permalink / raw)
  To: linux-xfs

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 <billodo@redhat.com>
---

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

diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
index a294e14..c3344d0 100644
--- a/growfs/xfs_growfs.c
+++ b/growfs/xfs_growfs.c
@@ -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, _("Path resolution failed for %s: %s\n"),
+			progname, argv[optind]);
+		return 1;
+	}
+
+	fs = fs_table_lookup_mount(rpath);
 	if (!fs) {
 		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
 			progname, argv[optind]);
diff --git a/include/path.h b/include/path.h
index d077bac..1d3a902 100644
--- a/include/path.h
+++ b/include/path.h
@@ -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	*/
diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index 816acc2..b767e9d 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -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,
-- 
2.9.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v3] xfsprogs: ensure growfs rejects non-existent mount point
  2017-04-24 14:29 ` [PATCH v3] " Bill O'Donnell
@ 2017-04-26 22:23   ` Eric Sandeen
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Sandeen @ 2017-04-26 22:23 UTC (permalink / raw)
  To: Bill O'Donnell, linux-xfs

On 4/24/17 9:29 AM, Bill O'Donnell wrote:

I probably should have mentioned this earlier but the subject
and commit log are a little weird; we want this stuff to be
as accurate as possible for those who come after us in 2050
when XFS is still in use.  :P

"xfsprogs: ensure growfs rejects non-existent mount point"

What is a "non-existent mount point?"  If it doesn't exist,
how can it be a mount point?  :)  Maybe:

xfs_growfs: ensure target path is an active xfs mountpoint

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

..that the target path must be an active xfs mountpoint.

(the filesystem /does/ have to be mounted to be grown, it's
just that growfs found it via paths which are not the mountpoint,
which is not what the manpage says it requires)

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

... to proceed if the target path resides anywhere on a mounted
xfs filesystem.  This could lead to unexpected results.

(of course it was mounted, the problem was that it was
not the mountpoint itself)

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

Unless the target path is an active xfs mountpoint, reject it.

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

(we needed this with or without bind mounts so that's not
really inaccurate...)

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 <billodo@redhat.com>
> ---
> 
> 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(-)
> 
> diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
> index a294e14..c3344d0 100644
> --- a/growfs/xfs_growfs.c
> +++ b/growfs/xfs_growfs.c
> @@ -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, _("Path resolution failed for %s: %s\n"),
> +			progname, argv[optind]);
> +		return 1;
> +	}

so we get:

# growfs/xfs_growfs -n foobar
Path resolution failed for xfs_growfs: foobar

That's a little odd...

What I had suggested was to use strerror to say /why/ it failed:

+	if (!realpath(argv[optind], rpath)) {
+		fprintf(stderr, _("%s: path resolution failed for %s: %s\n"),
+			progname, argv[optind], strerror(errno));
+		return 1;
+	}

so:

# xfs_growfs -n foobar
xfs_growfs: path resolution failed for foobar: No such file or directory

Sorry for being the BMFH.

-Eric

> +
> +	fs = fs_table_lookup_mount(rpath);
>  	if (!fs) {
>  		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
>  			progname, argv[optind]);
> diff --git a/include/path.h b/include/path.h
> index d077bac..1d3a902 100644
> --- a/include/path.h
> +++ b/include/path.h
> @@ -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	*/
> diff --git a/libxcmd/paths.c b/libxcmd/paths.c
> index 816acc2..b767e9d 100644
> --- a/libxcmd/paths.c
> +++ b/libxcmd/paths.c
> @@ -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,
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v4] xfs_growfs: ensure target path is an active xfs mountpoint
  2017-04-07 17:58 [PATCH] xfsprogs: ensure growfs rejects non-existent mount point Bill O'Donnell
                   ` (3 preceding siblings ...)
  2017-04-24 14:29 ` [PATCH v3] " Bill O'Donnell
@ 2017-04-27 18:23 ` Bill O'Donnell
  2017-04-27 19:15   ` Eric Sandeen
  4 siblings, 1 reply; 14+ messages in thread
From: Bill O'Donnell @ 2017-04-27 18:23 UTC (permalink / raw)
  To: linux-xfs

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 <billodo@redhat.com>
---

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

diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
index a294e14..366176b 100644
--- a/growfs/xfs_growfs.c
+++ b/growfs/xfs_growfs.c
@@ -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]);
diff --git a/include/path.h b/include/path.h
index d077bac..1d3a902 100644
--- a/include/path.h
+++ b/include/path.h
@@ -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	*/
diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index 816acc2..b767e9d 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -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,
-- 
2.9.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v4] xfs_growfs: ensure target path is an active xfs mountpoint
  2017-04-27 18:23 ` [PATCH v4] xfs_growfs: ensure target path is an active xfs mountpoint Bill O'Donnell
@ 2017-04-27 19:15   ` Eric Sandeen
  2017-05-03 16:08     ` Darrick J. Wong
  0 siblings, 1 reply; 14+ messages in thread
From: Eric Sandeen @ 2017-04-27 19:15 UTC (permalink / raw)
  To: Bill O'Donnell, linux-xfs

On 4/27/17 1:23 PM, Bill O'Donnell wrote:
> 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 <billodo@redhat.com>

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 <sandeen@redhat.com>

> ---
> 
> 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(-)
> 
> diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
> index a294e14..366176b 100644
> --- a/growfs/xfs_growfs.c
> +++ b/growfs/xfs_growfs.c
> @@ -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]);
> diff --git a/include/path.h b/include/path.h
> index d077bac..1d3a902 100644
> --- a/include/path.h
> +++ b/include/path.h
> @@ -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	*/
> diff --git a/libxcmd/paths.c b/libxcmd/paths.c
> index 816acc2..b767e9d 100644
> --- a/libxcmd/paths.c
> +++ b/libxcmd/paths.c
> @@ -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,
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4] xfs_growfs: ensure target path is an active xfs mountpoint
  2017-04-27 19:15   ` Eric Sandeen
@ 2017-05-03 16:08     ` Darrick J. Wong
  0 siblings, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2017-05-03 16:08 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Bill O'Donnell, linux-xfs

On Thu, Apr 27, 2017 at 02:15:50PM -0500, Eric Sandeen wrote:
> On 4/27/17 1:23 PM, Bill O'Donnell wrote:
> > 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 <billodo@redhat.com>
> 
> 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).

Ofc now that Eryu has accepted the xfstests for this, we'll fail
xfs/289 for a whole release cycle... :)

(Not that I particularly mind, just leaving a breadcrumb in case people
start wondering why we have a test for something that isn't upstream.)

((Which you know will be me in about 3 weeks when I forget that any of
this ever happened. :P))

--D

> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> > ---
> > 
> > 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(-)
> > 
> > diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
> > index a294e14..366176b 100644
> > --- a/growfs/xfs_growfs.c
> > +++ b/growfs/xfs_growfs.c
> > @@ -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]);
> > diff --git a/include/path.h b/include/path.h
> > index d077bac..1d3a902 100644
> > --- a/include/path.h
> > +++ b/include/path.h
> > @@ -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	*/
> > diff --git a/libxcmd/paths.c b/libxcmd/paths.c
> > index 816acc2..b767e9d 100644
> > --- a/libxcmd/paths.c
> > +++ b/libxcmd/paths.c
> > @@ -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,
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-05-03 16:09 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-07 17:58 [PATCH] xfsprogs: ensure growfs rejects non-existent mount point Bill O'Donnell
2017-04-07 18:33 ` Darrick J. Wong
2017-04-07 18:51 ` Eric Sandeen
2017-04-11 23:20   ` Bill O'Donnell
2017-04-11 17:22 ` [PATCH v2] " Bill O'Donnell
2017-04-19 22:05   ` Eric Sandeen
2017-04-19 22:08     ` Bill O'Donnell
2017-04-20 22:22   ` Eric Sandeen
2017-04-20 22:43     ` Eric Sandeen
2017-04-24 14:29 ` [PATCH v3] " Bill O'Donnell
2017-04-26 22:23   ` Eric Sandeen
2017-04-27 18:23 ` [PATCH v4] xfs_growfs: ensure target path is an active xfs mountpoint Bill O'Donnell
2017-04-27 19:15   ` Eric Sandeen
2017-05-03 16:08     ` Darrick J. Wong

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.