xfsdump: reject bind mounted subdir targets
diff mbox series

Message ID 1a18d094-0fe0-0bee-ea1f-861ccb60a671@sandeen.net
State New
Headers show
Series
  • xfsdump: reject bind mounted subdir targets
Related show

Commit Message

Eric Sandeen Aug. 22, 2019, 7:39 p.m. UTC
Once upon a time, xfsdump pointed at a bind-mounted subdirectory
caused problems because we assumed the inode of the bind-mounted
dir was the root inode of the filesystem.  That got written into the
dump header, and a subsequent restore would fail on the mismatch.

An effort was made to determine the true root inode using bulkstat,
to retrieve the first (lowest-numbered) inode in the filesystem.

That approach is now known to fail in the (rare) case where inodes
are allocated with a lower number than the root.  As a result, thewrong root inode number still goes into the dump header, and restore
will fail with:

xfsrestore: tree.c:757: tree_begindir: Assertion `ino != persp->p_rootino || hardh == persp->p_rooth' failed.

in this case as well.

Rather than trying to work out the true root inode for the filesystem
in question if it's bind mounted, just go the simple route and reject
bind mounts of subdirs altogether.  This probably leads to less surprise
in any case, as dumping a bind-mounted subdir actually will dump the
entire filesystem AFAICT.

Fixes: 25195ebf107 ("xfsdump: handle bind mount targets")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Comments

Eric Sandeen Aug. 22, 2019, 8:54 p.m. UTC | #1
On 8/22/19 2:39 PM, Eric Sandeen wrote:
> Once upon a time, xfsdump pointed at a bind-mounted subdirectory
> caused problems because we assumed the inode of the bind-mounted
> dir was the root inode of the filesystem.  That got written into the
> dump header, and a subsequent restore would fail on the mismatch.
> 
> An effort was made to determine the true root inode using bulkstat,
> to retrieve the first (lowest-numbered) inode in the filesystem.
> 
> That approach is now known to fail in the (rare) case where inodes
> are allocated with a lower number than the root.  As a result, thewrong root inode number still goes into the dump header, and restore
> will fail with:
> 
> xfsrestore: tree.c:757: tree_begindir: Assertion `ino != persp->p_rootino || hardh == persp->p_rooth' failed.
> 
> in this case as well.
> 
> Rather than trying to work out the true root inode for the filesystem
> in question if it's bind mounted, just go the simple route and reject
> bind mounts of subdirs altogether.  This probably leads to less surprise
> in any case, as dumping a bind-mounted subdir actually will dump the
> entire filesystem AFAICT.
> 
> Fixes: 25195ebf107 ("xfsdump: handle bind mount targets")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/dump/content.c b/dump/content.c
> index 30232d4..b9f4929 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -1383,12 +1383,12 @@ baseuuidbypass:
>  
>  	/* figure out the ino for the root directory of the fs
>  	 * and get its struct xfs_bstat for inomap_build().  This could
> -	 * be a bind mount; don't ask for the mount point inode,
> -	 * find the actual lowest inode number in the filesystem.
> +	 * be a bind mount; disallow this case because we don't know the
> +	 * root inode.
>  	 */
>  	{
>  		stat64_t rootstat;
> -		xfs_ino_t lastino = 0;
> +		xfs_ino_t lastino;
>  		int ocount = 0;
>  		struct xfs_fsop_bulkreq bulkreq;
>  
> @@ -1404,7 +1404,8 @@ baseuuidbypass:
>  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>  		assert(sc_rootxfsstatp);
>  
> -		/* Get the first valid (i.e. root) inode in this fs */
> +		/* Bulkstat this inode to get generation number */
> +		lastino = rootstat.st_ino - 1;
>  		bulkreq.lastip = (__u64 *)&lastino;
>  		bulkreq.icount = 1;
>  		bulkreq.ubuffer = sc_rootxfsstatp;

actually, I forgot we have bigstat_one() which wraps this up, I'll use that
instead.

> @@ -1415,10 +1416,13 @@ baseuuidbypass:
>  			return BOOL_FALSE;
>  		}
>  
> -		if (sc_rootxfsstatp->bs_ino != rootstat.st_ino)
> -			mlog (MLOG_NORMAL | MLOG_NOTE,
> -			       _("root ino %lld differs from mount dir ino %lld, bind mount?\n"),
> -			         sc_rootxfsstatp->bs_ino, rootstat.st_ino);
> +		/* The real root inode will have a generation of zero */
> +		if (sc_rootxfsstatp->bs_gen != 0) {
> +			mlog (MLOG_ERROR,
> +_("Dir inode %lld at %s has non-zero generation, not a root inode, bind mount?\n"),
> +				rootstat.st_ino, mntpnt);
> +			return BOOL_FALSE;
> +		}
>  	}
>  
>  	/* alloc a file system handle, to be used with the jdm_open()
>

Patch
diff mbox series

diff --git a/dump/content.c b/dump/content.c
index 30232d4..b9f4929 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -1383,12 +1383,12 @@  baseuuidbypass:
 
 	/* figure out the ino for the root directory of the fs
 	 * and get its struct xfs_bstat for inomap_build().  This could
-	 * be a bind mount; don't ask for the mount point inode,
-	 * find the actual lowest inode number in the filesystem.
+	 * be a bind mount; disallow this case because we don't know the
+	 * root inode.
 	 */
 	{
 		stat64_t rootstat;
-		xfs_ino_t lastino = 0;
+		xfs_ino_t lastino;
 		int ocount = 0;
 		struct xfs_fsop_bulkreq bulkreq;
 
@@ -1404,7 +1404,8 @@  baseuuidbypass:
 			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
 		assert(sc_rootxfsstatp);
 
-		/* Get the first valid (i.e. root) inode in this fs */
+		/* Bulkstat this inode to get generation number */
+		lastino = rootstat.st_ino - 1;
 		bulkreq.lastip = (__u64 *)&lastino;
 		bulkreq.icount = 1;
 		bulkreq.ubuffer = sc_rootxfsstatp;
@@ -1415,10 +1416,13 @@  baseuuidbypass:
 			return BOOL_FALSE;
 		}
 
-		if (sc_rootxfsstatp->bs_ino != rootstat.st_ino)
-			mlog (MLOG_NORMAL | MLOG_NOTE,
-			       _("root ino %lld differs from mount dir ino %lld, bind mount?\n"),
-			         sc_rootxfsstatp->bs_ino, rootstat.st_ino);
+		/* The real root inode will have a generation of zero */
+		if (sc_rootxfsstatp->bs_gen != 0) {
+			mlog (MLOG_ERROR,
+_("Dir inode %lld at %s has non-zero generation, not a root inode, bind mount?\n"),
+				rootstat.st_ino, mntpnt);
+			return BOOL_FALSE;
+		}
 	}
 
 	/* alloc a file system handle, to be used with the jdm_open()