Re: [PATCH 6/6] xfs_repair: check plausibility of root dir pointer before trashing it

[Brian Foster <bfoster@redhat.com>]

On Wed, Dec 04, 2019 at 09:05:02AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If sb_rootino doesn't point to where we think mkfs should have allocated
> the root directory, check to see if the alleged root directory actually
> looks like a root directory.  If so, we'll let it live because someone
> could have changed sunit since formatting time, and that changes the
> root directory inode estimate.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/xfs_repair.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> 
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index abd568c9..b0407f4b 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -426,6 +426,37 @@ _("would reset superblock %s inode pointer to %"PRIu64"\n"),
>  	*ino = expected_ino;
>  }
>  
> +/* Does the root directory inode look like a plausible root directory? */
> +static bool
> +has_plausible_rootdir(
> +	struct xfs_mount	*mp)
> +{
> +	struct xfs_inode	*ip;
> +	xfs_ino_t		ino;
> +	int			error;
> +	bool			ret = false;
> +
> +	error = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &ip,
> +			&xfs_default_ifork_ops);
> +	if (error)
> +		goto out;
> +	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;
> +
> +	/* The root directory '..' entry points to the directory. */
> +	if (ino == mp->m_sb.sb_rootino)
> +		ret = true;
> +
> +out_rele:
> +	libxfs_irele(ip);
> +out:
> +	return ret;
> +}
> +
>  /*
>   * Make sure that the first 3 inodes in the filesystem are the root directory,
>   * the realtime bitmap, and the realtime summary, in that order.
> @@ -436,6 +467,20 @@ calc_mkfs(
>  {
>  	xfs_ino_t		rootino = libxfs_ialloc_calc_rootino(mp, -1);
>  
> +	/*
> +	 * 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.  Don't blow up the root directory if this is the
> +	 * case.
> +	 */
> +	if (mp->m_sb.sb_rootino != rootino && has_plausible_rootdir(mp)) {
> +		do_warn(
> +_("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"),
> +			mp->m_sb.sb_rootino, rootino);
> +		rootino = mp->m_sb.sb_rootino;
> +	}
> +

A slightly unfortunate side effect of this is that there's seemingly no
straightforward way for a user to "clear" this state/warning. We've
solved the major problem by allowing repair to handle this condition,
but AFAICT this warning will persist unless the stripe unit is changed
back to its original value.

IOW, what if this problem exists simply because a user made a mistake
and wants to undo it? It's probably easy enough for us to say "use
whatever you did at mkfs time," but what if that's unknown or was set
automatically? I feel like that is the type of thing that in practice
could result in unnecessary bugs or error reports unless the tool can
make a better suggestion to the end user. For example, could we check
the geometry on secondary supers (if they exist) against the current
rootino and use that as a secondary form of verification and/or suggest
the user reset to that geometry (if desired)? OTOH, I guess we'd have to
consider what happens if the filesystem was grown in that scenario too..
:/

(Actually on a quick test, it looks like growfs updates every super,
even preexisting..).

Brian

>  	ensure_fixed_ino(&mp->m_sb.sb_rootino, rootino,
>  			_("root"));
>  	ensure_fixed_ino(&mp->m_sb.sb_rbmino, rootino + 1,
>