[PATCH RFC 0/4] xfs_repair: do not trash valid root dirs

[PATCH RFC 0/4] xfs_repair: do not trash valid root dirs

[Darrick J. Wong]

Hi all,

Alex Lyakas observed that xfs_repair can accidentally trash the root
directory on a filesystem.  Specifically, if one formats a V4 filesystem
without sparse inodes but with sunit/swidth set and then mounts that
filesystem with a different sunit/swidth, the kernel will update the
values in the superblock.  This causes xfs_repair's sb_rootino
estimation to differ from the actual root directory, and it discards the
actual root directory even though there's nothing wrong with it.

Therefore, hoist the logic that computes the root inode location into
libxfs so that the kernel will avoid the sb update if the proposed
sunit/swidth changes would alter the sb_rootino estimation; and then
teach xfs_repair to retain the root directory even if the estimation
doesn't add up, as long as sb_rootino points to a directory whose '..'
entry points to itself.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

[PATCH 1/4] xfs: don't commit sunit/swidth updates to disk if that would cause repair failures

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Alex Lyakas reported[1] that mounting an xfs filesystem with new sunit
and swidth values could cause xfs_repair to fail loudly.  The problem
here is that repair calculates the where mkfs should have allocated the
root inode, based on the superblock geometry.  The allocation decisions
depend on sunit, which means that we really can't go updating sunit if
it would lead to a subsequent repair failure on an otherwise correct
filesystem.

Port the computation code from xfs_repair and teach mount to avoid the
ondisk update if it would cause problems for repair.  We allow the mount
to proceed (and new allocations will reflect this new geometry) because
we've never screened this kind of thing before.

[1] https://lore.kernel.org/linux-xfs/20191125130744.GA44777@bfoster/T/#m00f9594b511e076e2fcdd489d78bc30216d72a7d

Reported-by: Alex Lyakas <alex@zadara.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/xfs_ialloc.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 libxfs/xfs_ialloc.h |    2 +
 2 files changed, 90 insertions(+)


diff --git a/libxfs/xfs_ialloc.c b/libxfs/xfs_ialloc.c
index f039b287..fe6553d3 100644
--- a/libxfs/xfs_ialloc.c
+++ b/libxfs/xfs_ialloc.c
@@ -2848,3 +2848,91 @@ xfs_ialloc_setup_geometry(
 	else
 		igeo->ialloc_align = 0;
 }
+
+/*
+ * Compute the first and last inodes numbers of the inode chunk that was
+ * preallocated for the root directory.
+ */
+void
+xfs_ialloc_find_prealloc(
+	struct xfs_mount	*mp,
+	xfs_agino_t		*first_agino,
+	xfs_agino_t		*last_agino)
+{
+	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
+	xfs_agblock_t		first_bno;
+
+	/*
+	 * Pre-calculate the geometry of ag 0. We know what it looks like
+	 * because we know what mkfs does: 2 allocation btree roots (by block
+	 * and by size), the inode allocation btree root, the free inode
+	 * allocation btree root (if enabled) and some number of blocks to
+	 * prefill the agfl.
+	 *
+	 * Because the current shape of the btrees may differ from the current
+	 * shape, we open code the mkfs freelist block count here. mkfs creates
+	 * single level trees, so the calculation is pertty straight forward for
+	 * the trees that use the AGFL.
+	 */
+
+	/* free space by block btree root comes after the ag headers */
+	first_bno = howmany(4 * mp->m_sb.sb_sectsize, mp->m_sb.sb_blocksize);
+
+	/* free space by length btree root */
+	first_bno += 1;
+
+	/* inode btree root */
+	first_bno += 1;
+
+	/* agfl */
+	first_bno += (2 * min(2U, mp->m_ag_maxlevels)) + 1;
+
+	if (xfs_sb_version_hasfinobt(&mp->m_sb))
+		first_bno++;
+
+	if (xfs_sb_version_hasrmapbt(&mp->m_sb)) {
+		first_bno += min(2U, mp->m_rmap_maxlevels); /* agfl blocks */
+		first_bno++;
+	}
+
+	if (xfs_sb_version_hasreflink(&mp->m_sb))
+		first_bno++;
+
+	/*
+	 * If the log is allocated in the first allocation group we need to
+	 * add the number of blocks used by the log to the above calculation.
+	 *
+	 * This can happens with filesystems that only have a single
+	 * allocation group, or very odd geometries created by old mkfs
+	 * versions on very small filesystems.
+	 */
+	if (mp->m_sb.sb_logstart &&
+	    XFS_FSB_TO_AGNO(mp, mp->m_sb.sb_logstart) == 0) {
+
+		/*
+		 * XXX(hch): verify that sb_logstart makes sense?
+		 */
+		 first_bno += mp->m_sb.sb_logblocks;
+	}
+
+	/*
+	 * ditto the location of the first inode chunks in the fs ('/')
+	 */
+	if (xfs_sb_version_hasdalign(&mp->m_sb) && igeo->ialloc_align > 0) {
+		*first_agino = XFS_AGB_TO_AGINO(mp,
+				roundup(first_bno, mp->m_sb.sb_unit));
+	} else if (xfs_sb_version_hasalign(&mp->m_sb) &&
+		   mp->m_sb.sb_inoalignmt > 1)  {
+		*first_agino = XFS_AGB_TO_AGINO(mp,
+				roundup(first_bno, mp->m_sb.sb_inoalignmt));
+	} else  {
+		*first_agino = XFS_AGB_TO_AGINO(mp, first_bno);
+	}
+
+	ASSERT(igeo->ialloc_blks > 0);
+
+	if (igeo->ialloc_blks > 1)
+		*last_agino = *first_agino + XFS_INODES_PER_CHUNK;
+	else
+		*last_agino = XFS_AGB_TO_AGINO(mp, first_bno + 1);
+}
diff --git a/libxfs/xfs_ialloc.h b/libxfs/xfs_ialloc.h
index 323592d5..9d9fe7b4 100644
--- a/libxfs/xfs_ialloc.h
+++ b/libxfs/xfs_ialloc.h
@@ -152,5 +152,7 @@ int xfs_inobt_insert_rec(struct xfs_btree_cur *cur, uint16_t holemask,
 
 int xfs_ialloc_cluster_alignment(struct xfs_mount *mp);
 void xfs_ialloc_setup_geometry(struct xfs_mount *mp);
+void xfs_ialloc_find_prealloc(struct xfs_mount *mp, xfs_agino_t *first_agino,
+		xfs_agino_t *last_agino);
 
 #endif	/* __XFS_IALLOC_H__ */

[PATCH 2/4] mkfs: check root inode location

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Make sure the root inode gets created where repair thinks it should be
created.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_api_defs.h |    1 +
 mkfs/xfs_mkfs.c          |   29 +++++++++++++++++++++++------
 2 files changed, 24 insertions(+), 6 deletions(-)


diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 645c9b1b..8f6b9fc2 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -156,5 +156,6 @@
 
 #define xfs_ag_init_headers		libxfs_ag_init_headers
 #define xfs_buf_delwri_submit		libxfs_buf_delwri_submit
+#define xfs_ialloc_find_prealloc	libxfs_ialloc_find_prealloc
 
 #endif /* __LIBXFS_API_DEFS_H__ */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 18338a61..5143d9b4 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3521,6 +3521,28 @@ rewrite_secondary_superblocks(
 	libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
 }
 
+static void
+check_root_ino(
+	struct xfs_mount	*mp)
+{
+	xfs_agino_t		first, last;
+
+	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
+		fprintf(stderr,
+			_("%s: root inode created in AG %u, not AG 0\n"),
+			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
+		exit(1);
+	}
+
+	libxfs_ialloc_find_prealloc(mp, &first, &last);
+	if (mp->m_sb.sb_rootino != XFS_AGINO_TO_INO(mp, 0, first)) {
+		fprintf(stderr,
+			_("%s: root inode (%llu) not created in first chunk\n"),
+			progname, (unsigned long long)mp->m_sb.sb_rootino);
+		exit(1);
+	}
+}
+
 int
 main(
 	int			argc,
@@ -3807,12 +3829,7 @@ main(
 	/*
 	 * Protect ourselves against possible stupidity
 	 */
-	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
-		fprintf(stderr,
-			_("%s: root inode created in AG %u, not AG 0\n"),
-			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
-		exit(1);
-	}
+	check_root_ino(mp);
 
 	/*
 	 * Re-write multiple secondary superblocks with rootinode field set

[PATCH 3/4] xfs_repair: use xfs_ialloc_find_prealloc

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Replace the open-coded first_prealloc_ino computation with a call to
xfs_ialloc_find_prealloc.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/globals.c    |    3 --
 repair/globals.h    |    3 --
 repair/xfs_repair.c |   70 +--------------------------------------------------
 3 files changed, 2 insertions(+), 74 deletions(-)


diff --git a/repair/globals.c b/repair/globals.c
index dcd79ea4..91cc49c6 100644
--- a/repair/globals.c
+++ b/repair/globals.c
@@ -74,9 +74,6 @@ int	lost_pquotino;
 
 xfs_agino_t	first_prealloc_ino;
 xfs_agino_t	last_prealloc_ino;
-xfs_agblock_t	bnobt_root;
-xfs_agblock_t	bcntbt_root;
-xfs_agblock_t	inobt_root;
 
 /* configuration vars -- fs geometry dependent */
 
diff --git a/repair/globals.h b/repair/globals.h
index 008bdd90..31dd760c 100644
--- a/repair/globals.h
+++ b/repair/globals.h
@@ -115,9 +115,6 @@ extern int		lost_pquotino;
 
 extern xfs_agino_t	first_prealloc_ino;
 extern xfs_agino_t	last_prealloc_ino;
-extern xfs_agblock_t	bnobt_root;
-extern xfs_agblock_t	bcntbt_root;
-extern xfs_agblock_t	inobt_root;
 
 /* configuration vars -- fs geometry dependent */
 
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 9295673d..6798b88c 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -398,74 +398,8 @@ do_log(char const *msg, ...)
 static void
 calc_mkfs(xfs_mount_t *mp)
 {
-	xfs_agblock_t	fino_bno;
-	int		do_inoalign;
-
-	do_inoalign = M_IGEO(mp)->ialloc_align;
-
-	/*
-	 * Pre-calculate the geometry of ag 0. We know what it looks like
-	 * because we know what mkfs does: 2 allocation btree roots (by block
-	 * and by size), the inode allocation btree root, the free inode
-	 * allocation btree root (if enabled) and some number of blocks to
-	 * prefill the agfl.
-	 *
-	 * Because the current shape of the btrees may differ from the current
-	 * shape, we open code the mkfs freelist block count here. mkfs creates
-	 * single level trees, so the calculation is pertty straight forward for
-	 * the trees that use the AGFL.
-	 */
-	bnobt_root = howmany(4 * mp->m_sb.sb_sectsize, mp->m_sb.sb_blocksize);
-	bcntbt_root = bnobt_root + 1;
-	inobt_root = bnobt_root + 2;
-	fino_bno = inobt_root + (2 * min(2, mp->m_ag_maxlevels)) + 1;
-	if (xfs_sb_version_hasfinobt(&mp->m_sb))
-		fino_bno++;
-	if (xfs_sb_version_hasrmapbt(&mp->m_sb)) {
-		fino_bno += min(2, mp->m_rmap_maxlevels); /* agfl blocks */
-		fino_bno++;
-	}
-	if (xfs_sb_version_hasreflink(&mp->m_sb))
-		fino_bno++;
-
-	/*
-	 * If the log is allocated in the first allocation group we need to
-	 * add the number of blocks used by the log to the above calculation.
-	 *
-	 * This can happens with filesystems that only have a single
-	 * allocation group, or very odd geometries created by old mkfs
-	 * versions on very small filesystems.
-	 */
-	if (mp->m_sb.sb_logstart &&
-	    XFS_FSB_TO_AGNO(mp, mp->m_sb.sb_logstart) == 0) {
-
-		/*
-		 * XXX(hch): verify that sb_logstart makes sense?
-		 */
-		 fino_bno += mp->m_sb.sb_logblocks;
-	}
-
-	/*
-	 * ditto the location of the first inode chunks in the fs ('/')
-	 */
-	if (xfs_sb_version_hasdalign(&mp->m_sb) && do_inoalign)  {
-		first_prealloc_ino = XFS_AGB_TO_AGINO(mp, roundup(fino_bno,
-					mp->m_sb.sb_unit));
-	} else if (xfs_sb_version_hasalign(&mp->m_sb) &&
-					mp->m_sb.sb_inoalignmt > 1)  {
-		first_prealloc_ino = XFS_AGB_TO_AGINO(mp,
-					roundup(fino_bno,
-						mp->m_sb.sb_inoalignmt));
-	} else  {
-		first_prealloc_ino = XFS_AGB_TO_AGINO(mp, fino_bno);
-	}
-
-	ASSERT(M_IGEO(mp)->ialloc_blks > 0);
-
-	if (M_IGEO(mp)->ialloc_blks > 1)
-		last_prealloc_ino = first_prealloc_ino + XFS_INODES_PER_CHUNK;
-	else
-		last_prealloc_ino = XFS_AGB_TO_AGINO(mp, fino_bno + 1);
+	libxfs_ialloc_find_prealloc(mp, &first_prealloc_ino,
+			&last_prealloc_ino);
 
 	/*
 	 * now the first 3 inodes in the system

[PATCH 4/4] xfs_repair: check plausiblitiy of root dir pointer

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If sb_rootino doesn't point to where we think mkfs was supposed to have
preallocated an inode chunk, check to see if the alleged root directory
actually looks like a root directory.  If so, we'll let it go because
someone could have changed sunit since formatting time.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/xfs_repair.c |   50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)


diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 6798b88c..f6134cca 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -395,12 +395,60 @@ do_log(char const *msg, ...)
 	va_end(args);
 }
 
+/*
+ * If sb_rootino points to a different inode than we were expecting, try
+ * loading the alleged root inode to see if it's a plausibly a root directory.
+ * If so, we'll readjust the computations.
+ */
+static void
+check_misaligned_root(
+	struct xfs_mount	*mp)
+{
+	struct xfs_inode	*ip;
+	xfs_ino_t		ino;
+	int			error;
+
+	error = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &ip,
+			&xfs_default_ifork_ops);
+	if (error)
+		return;
+	if (!S_ISDIR(VFS_I(ip)->i_mode))
+		goto out_rele;
+
+	error = -libxfs_dir_lookup(NULL, ip, &xfs_name_dotdot, &ino, NULL);
+	if (error)
+		goto out_rele;
+
+	if (ino == mp->m_sb.sb_rootino) {
+		do_warn(
+_("sb root inode value %" PRIu64 " inconsistent with calculated value %u but looks like a root directory\n"),
+			mp->m_sb.sb_rootino, first_prealloc_ino);
+		last_prealloc_ino += (int)ino - first_prealloc_ino;
+		first_prealloc_ino = ino;
+	}
+
+out_rele:
+	libxfs_irele(ip);
+}
+
 static void
-calc_mkfs(xfs_mount_t *mp)
+calc_mkfs(
+	struct xfs_mount	*mp)
 {
 	libxfs_ialloc_find_prealloc(mp, &first_prealloc_ino,
 			&last_prealloc_ino);
 
+	/*
+	 * If the root inode isn't where we think it is, check its plausibility
+	 * as a root directory.  It's possible that somebody changed sunit since
+	 * the filesystem was created, which can change the value of the above
+	 * computation.  Try to avoid blowing up the filesystem if this is the
+	 * case.
+	 */
+	if (mp->m_sb.sb_rootino != NULLFSINO &&
+	    mp->m_sb.sb_rootino != first_prealloc_ino)
+		check_misaligned_root(mp);
+
 	/*
 	 * now the first 3 inodes in the system
 	 */

Re: [PATCH 2/4] mkfs: check root inode location

[Brian Foster]

On Mon, Dec 02, 2019 at 09:36:11AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make sure the root inode gets created where repair thinks it should be
> created.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  libxfs/libxfs_api_defs.h |    1 +
>  mkfs/xfs_mkfs.c          |   29 +++++++++++++++++++++++------
>  2 files changed, 24 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index 645c9b1b..8f6b9fc2 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -156,5 +156,6 @@
>  
>  #define xfs_ag_init_headers		libxfs_ag_init_headers
>  #define xfs_buf_delwri_submit		libxfs_buf_delwri_submit
> +#define xfs_ialloc_find_prealloc	libxfs_ialloc_find_prealloc
>  

Perhaps this should be in the previous patch..?


>  #endif /* __LIBXFS_API_DEFS_H__ */
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 18338a61..5143d9b4 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3521,6 +3521,28 @@ rewrite_secondary_superblocks(
>  	libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
>  }
>  
> +static void
> +check_root_ino(
> +	struct xfs_mount	*mp)
> +{
> +	xfs_agino_t		first, last;
> +
> +	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> +		fprintf(stderr,
> +			_("%s: root inode created in AG %u, not AG 0\n"),
> +			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> +		exit(1);
> +	}
> +
> +	libxfs_ialloc_find_prealloc(mp, &first, &last);
> +	if (mp->m_sb.sb_rootino != XFS_AGINO_TO_INO(mp, 0, first)) {
> +		fprintf(stderr,
> +			_("%s: root inode (%llu) not created in first chunk\n"),
> +			progname, (unsigned long long)mp->m_sb.sb_rootino);

If the root inode ended up somewhere in the middle of the first chunk,
we'd fail (rightly), but with a misleading error message. Perhaps
something like "root inode (..) not allocated in expected location"
would be better? I'd also like to see a comment somewhere in here to
explain why we have this check. For example:

"The superblock refers directly to the root inode, but repair makes
hardcoded assumptions about its location based on filesystem geometry
for an extra level of verification. If this assumption ever breaks, we
should flag it immediately and fail the mkfs. Otherwise repair may
consider the filesystem corrupt and toss the root inode."

Feel free to reword that however appropriate (given the behavior change
in subsequent patches), of course..

Brian

> +		exit(1);
> +	}
> +}
> +
>  int
>  main(
>  	int			argc,
> @@ -3807,12 +3829,7 @@ main(
>  	/*
>  	 * Protect ourselves against possible stupidity
>  	 */
> -	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> -		fprintf(stderr,
> -			_("%s: root inode created in AG %u, not AG 0\n"),
> -			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> -		exit(1);
> -	}
> +	check_root_ino(mp);
>  
>  	/*
>  	 * Re-write multiple secondary superblocks with rootinode field set
> 

Re: [PATCH 4/4] xfs_repair: check plausiblitiy of root dir pointer

[Brian Foster]

On Mon, Dec 02, 2019 at 09:36:25AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If sb_rootino doesn't point to where we think mkfs was supposed to have
> preallocated an inode chunk, check to see if the alleged root directory
> actually looks like a root directory.  If so, we'll let it go because
> someone could have changed sunit since formatting time.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/xfs_repair.c |   50 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 6798b88c..f6134cca 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -395,12 +395,60 @@ do_log(char const *msg, ...)
>  	va_end(args);
>  }
>  
> +/*
> + * If sb_rootino points to a different inode than we were expecting, try
> + * loading the alleged root inode to see if it's a plausibly a root directory.
> + * If so, we'll readjust the computations.

"... readjust the calculated inode chunk range such that the root inode
is the first inode in the chunk."

> + */
> +static void
> +check_misaligned_root(
> +	struct xfs_mount	*mp)
> +{
> +	struct xfs_inode	*ip;
> +	xfs_ino_t		ino;
> +	int			error;
> +
> +	error = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &ip,
> +			&xfs_default_ifork_ops);
> +	if (error)
> +		return;
> +	if (!S_ISDIR(VFS_I(ip)->i_mode))
> +		goto out_rele;
> +
> +	error = -libxfs_dir_lookup(NULL, ip, &xfs_name_dotdot, &ino, NULL);
> +	if (error)
> +		goto out_rele;
> +
> +	if (ino == mp->m_sb.sb_rootino) {
> +		do_warn(
> +_("sb root inode value %" PRIu64 " inconsistent with calculated value %u but looks like a root directory\n"),

Just a nit, but I think the error would be more informative if it just
said something like:

"sb root inode %" PRIu64 " inconsistent with alignment (expected rootino %u)."

> +			mp->m_sb.sb_rootino, first_prealloc_ino);
> +		last_prealloc_ino += (int)ino - first_prealloc_ino;
> +		first_prealloc_ino = ino;

Why assume ino > first_prealloc_ino? How about we just assign
last_prealloc_ino as done in _find_prealloc()?

Brian

> +	}
> +
> +out_rele:
> +	libxfs_irele(ip);
> +}
> +
>  static void
> -calc_mkfs(xfs_mount_t *mp)
> +calc_mkfs(
> +	struct xfs_mount	*mp)
>  {
>  	libxfs_ialloc_find_prealloc(mp, &first_prealloc_ino,
>  			&last_prealloc_ino);
>  
> +	/*
> +	 * If the root inode isn't where we think it is, check its plausibility
> +	 * as a root directory.  It's possible that somebody changed sunit since
> +	 * the filesystem was created, which can change the value of the above
> +	 * computation.  Try to avoid blowing up the filesystem if this is the
> +	 * case.
> +	 */
> +	if (mp->m_sb.sb_rootino != NULLFSINO &&
> +	    mp->m_sb.sb_rootino != first_prealloc_ino)
> +		check_misaligned_root(mp);
> +
>  	/*
>  	 * now the first 3 inodes in the system
>  	 */
> 

Re: [PATCH 2/4] mkfs: check root inode location

[Darrick J. Wong]

On Tue, Dec 03, 2019 at 08:02:53AM -0500, Brian Foster wrote:
> On Mon, Dec 02, 2019 at 09:36:11AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Make sure the root inode gets created where repair thinks it should be
> > created.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  libxfs/libxfs_api_defs.h |    1 +
> >  mkfs/xfs_mkfs.c          |   29 +++++++++++++++++++++++------
> >  2 files changed, 24 insertions(+), 6 deletions(-)
> > 
> > 
> > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> > index 645c9b1b..8f6b9fc2 100644
> > --- a/libxfs/libxfs_api_defs.h
> > +++ b/libxfs/libxfs_api_defs.h
> > @@ -156,5 +156,6 @@
> >  
> >  #define xfs_ag_init_headers		libxfs_ag_init_headers
> >  #define xfs_buf_delwri_submit		libxfs_buf_delwri_submit
> > +#define xfs_ialloc_find_prealloc	libxfs_ialloc_find_prealloc
> >  
> 
> Perhaps this should be in the previous patch..?

<shrug> I think the libxfs wrapper macro things shouldn't be introduced
until there's a caller outside of libxfs.

> 
> >  #endif /* __LIBXFS_API_DEFS_H__ */
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 18338a61..5143d9b4 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -3521,6 +3521,28 @@ rewrite_secondary_superblocks(
> >  	libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
> >  }
> >  
> > +static void
> > +check_root_ino(
> > +	struct xfs_mount	*mp)
> > +{
> > +	xfs_agino_t		first, last;
> > +
> > +	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> > +		fprintf(stderr,
> > +			_("%s: root inode created in AG %u, not AG 0\n"),
> > +			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> > +		exit(1);
> > +	}
> > +
> > +	libxfs_ialloc_find_prealloc(mp, &first, &last);
> > +	if (mp->m_sb.sb_rootino != XFS_AGINO_TO_INO(mp, 0, first)) {
> > +		fprintf(stderr,
> > +			_("%s: root inode (%llu) not created in first chunk\n"),
> > +			progname, (unsigned long long)mp->m_sb.sb_rootino);
> 
> If the root inode ended up somewhere in the middle of the first chunk,
> we'd fail (rightly), but with a misleading error message. Perhaps
> something like "root inode (..) not allocated in expected location"

Ok, fixed.

> would be better? I'd also like to see a comment somewhere in here to
> explain why we have this check. For example:
> 
> "The superblock refers directly to the root inode, but repair makes
> hardcoded assumptions about its location based on filesystem geometry
> for an extra level of verification. If this assumption ever breaks, we
> should flag it immediately and fail the mkfs. Otherwise repair may
> consider the filesystem corrupt and toss the root inode."

How about:

/*
 * The superblock points to the root directory inode, but xfs_repair
 * expects to find the root inode in a very specific location computed
 * from the filesystem geometry for an extra level of verification.
 *
 * Fail the format immediately if those assumptions ever break, because
 * repair will toss the root directory.
 */

> Feel free to reword that however appropriate (given the behavior change
> in subsequent patches), of course..

Ok.

--D

> Brian
> 
> > +		exit(1);
> > +	}
> > +}
> > +
> >  int
> >  main(
> >  	int			argc,
> > @@ -3807,12 +3829,7 @@ main(
> >  	/*
> >  	 * Protect ourselves against possible stupidity
> >  	 */
> > -	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> > -		fprintf(stderr,
> > -			_("%s: root inode created in AG %u, not AG 0\n"),
> > -			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> > -		exit(1);
> > -	}
> > +	check_root_ino(mp);
> >  
> >  	/*
> >  	 * Re-write multiple secondary superblocks with rootinode field set
> > 
> 

Re: [PATCH 4/4] xfs_repair: check plausiblitiy of root dir pointer

[Darrick J. Wong]

On Tue, Dec 03, 2019 at 08:03:06AM -0500, Brian Foster wrote:
> On Mon, Dec 02, 2019 at 09:36:25AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > If sb_rootino doesn't point to where we think mkfs was supposed to have
> > preallocated an inode chunk, check to see if the alleged root directory
> > actually looks like a root directory.  If so, we'll let it go because
> > someone could have changed sunit since formatting time.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  repair/xfs_repair.c |   50 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 49 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index 6798b88c..f6134cca 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -395,12 +395,60 @@ do_log(char const *msg, ...)
> >  	va_end(args);
> >  }
> >  
> > +/*
> > + * If sb_rootino points to a different inode than we were expecting, try
> > + * loading the alleged root inode to see if it's a plausibly a root directory.
> > + * If so, we'll readjust the computations.
> 
> "... readjust the calculated inode chunk range such that the root inode
> is the first inode in the chunk."
> 
> > + */
> > +static void
> > +check_misaligned_root(
> > +	struct xfs_mount	*mp)
> > +{
> > +	struct xfs_inode	*ip;
> > +	xfs_ino_t		ino;
> > +	int			error;
> > +
> > +	error = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &ip,
> > +			&xfs_default_ifork_ops);
> > +	if (error)
> > +		return;
> > +	if (!S_ISDIR(VFS_I(ip)->i_mode))
> > +		goto out_rele;
> > +
> > +	error = -libxfs_dir_lookup(NULL, ip, &xfs_name_dotdot, &ino, NULL);
> > +	if (error)
> > +		goto out_rele;
> > +
> > +	if (ino == mp->m_sb.sb_rootino) {
> > +		do_warn(
> > +_("sb root inode value %" PRIu64 " inconsistent with calculated value %u but looks like a root directory\n"),
> 
> Just a nit, but I think the error would be more informative if it just
> said something like:
> 
> "sb root inode %" PRIu64 " inconsistent with alignment (expected rootino %u)."

Fixed.  Thanks for reviewing all this!

> > +			mp->m_sb.sb_rootino, first_prealloc_ino);
> > +		last_prealloc_ino += (int)ino - first_prealloc_ino;
> > +		first_prealloc_ino = ino;
> 
> Why assume ino > first_prealloc_ino? How about we just assign
> last_prealloc_ino as done in _find_prealloc()?

I think I'll just blow all that away since the {last,first}_alloc_ino
stuff seems incorrect anyway.

--D

> Brian
> 
> > +	}
> > +
> > +out_rele:
> > +	libxfs_irele(ip);
> > +}
> > +
> >  static void
> > -calc_mkfs(xfs_mount_t *mp)
> > +calc_mkfs(
> > +	struct xfs_mount	*mp)
> >  {
> >  	libxfs_ialloc_find_prealloc(mp, &first_prealloc_ino,
> >  			&last_prealloc_ino);
> >  
> > +	/*
> > +	 * If the root inode isn't where we think it is, check its plausibility
> > +	 * as a root directory.  It's possible that somebody changed sunit since
> > +	 * the filesystem was created, which can change the value of the above
> > +	 * computation.  Try to avoid blowing up the filesystem if this is the
> > +	 * case.
> > +	 */
> > +	if (mp->m_sb.sb_rootino != NULLFSINO &&
> > +	    mp->m_sb.sb_rootino != first_prealloc_ino)
> > +		check_misaligned_root(mp);
> > +
> >  	/*
> >  	 * now the first 3 inodes in the system
> >  	 */
> > 
> 

Re: [PATCH 2/4] mkfs: check root inode location

[Brian Foster]

On Tue, Dec 03, 2019 at 03:40:07PM -0800, Darrick J. Wong wrote:
> On Tue, Dec 03, 2019 at 08:02:53AM -0500, Brian Foster wrote:
> > On Mon, Dec 02, 2019 at 09:36:11AM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Make sure the root inode gets created where repair thinks it should be
> > > created.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  libxfs/libxfs_api_defs.h |    1 +
> > >  mkfs/xfs_mkfs.c          |   29 +++++++++++++++++++++++------
> > >  2 files changed, 24 insertions(+), 6 deletions(-)
> > > 
> > > 
> > > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> > > index 645c9b1b..8f6b9fc2 100644
> > > --- a/libxfs/libxfs_api_defs.h
> > > +++ b/libxfs/libxfs_api_defs.h
> > > @@ -156,5 +156,6 @@
> > >  
> > >  #define xfs_ag_init_headers		libxfs_ag_init_headers
> > >  #define xfs_buf_delwri_submit		libxfs_buf_delwri_submit
> > > +#define xfs_ialloc_find_prealloc	libxfs_ialloc_find_prealloc
> > >  
> > 
> > Perhaps this should be in the previous patch..?
> 
> <shrug> I think the libxfs wrapper macro things shouldn't be introduced
> until there's a caller outside of libxfs.
> 

Ok, fair enough..

> > 
> > >  #endif /* __LIBXFS_API_DEFS_H__ */
> > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > > index 18338a61..5143d9b4 100644
> > > --- a/mkfs/xfs_mkfs.c
> > > +++ b/mkfs/xfs_mkfs.c
> > > @@ -3521,6 +3521,28 @@ rewrite_secondary_superblocks(
> > >  	libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
> > >  }
> > >  
> > > +static void
> > > +check_root_ino(
> > > +	struct xfs_mount	*mp)
> > > +{
> > > +	xfs_agino_t		first, last;
> > > +
> > > +	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> > > +		fprintf(stderr,
> > > +			_("%s: root inode created in AG %u, not AG 0\n"),
> > > +			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> > > +		exit(1);
> > > +	}
> > > +
> > > +	libxfs_ialloc_find_prealloc(mp, &first, &last);
> > > +	if (mp->m_sb.sb_rootino != XFS_AGINO_TO_INO(mp, 0, first)) {
> > > +		fprintf(stderr,
> > > +			_("%s: root inode (%llu) not created in first chunk\n"),
> > > +			progname, (unsigned long long)mp->m_sb.sb_rootino);
> > 
> > If the root inode ended up somewhere in the middle of the first chunk,
> > we'd fail (rightly), but with a misleading error message. Perhaps
> > something like "root inode (..) not allocated in expected location"
> 
> Ok, fixed.
> 
> > would be better? I'd also like to see a comment somewhere in here to
> > explain why we have this check. For example:
> > 
> > "The superblock refers directly to the root inode, but repair makes
> > hardcoded assumptions about its location based on filesystem geometry
> > for an extra level of verification. If this assumption ever breaks, we
> > should flag it immediately and fail the mkfs. Otherwise repair may
> > consider the filesystem corrupt and toss the root inode."
> 
> How about:
> 
> /*
>  * The superblock points to the root directory inode, but xfs_repair
>  * expects to find the root inode in a very specific location computed
>  * from the filesystem geometry for an extra level of verification.
>  *
>  * Fail the format immediately if those assumptions ever break, because
>  * repair will toss the root directory.
>  */
> 

Sounds good, thanks!

Brian

> > Feel free to reword that however appropriate (given the behavior change
> > in subsequent patches), of course..
> 
> Ok.
> 
> --D
> 
> > Brian
> > 
> > > +		exit(1);
> > > +	}
> > > +}
> > > +
> > >  int
> > >  main(
> > >  	int			argc,
> > > @@ -3807,12 +3829,7 @@ main(
> > >  	/*
> > >  	 * Protect ourselves against possible stupidity
> > >  	 */
> > > -	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> > > -		fprintf(stderr,
> > > -			_("%s: root inode created in AG %u, not AG 0\n"),
> > > -			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> > > -		exit(1);
> > > -	}
> > > +	check_root_ino(mp);
> > >  
> > >  	/*
> > >  	 * Re-write multiple secondary superblocks with rootinode field set
> > > 
> > 
>