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

[Brian Foster <bfoster@redhat.com>]

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
>  	 */
>