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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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