[4/4] xfs_repair: check plausiblitiy of root dir pointer
diff mbox series

Message ID 157530818573.126767.13434243816626977089.stgit@magnolia
State New
Headers show
Series
  • xfs_repair: do not trash valid root dirs
Related show

Commit Message

Darrick J. Wong Dec. 2, 2019, 5:36 p.m. UTC
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(-)

Comments

Brian Foster Dec. 3, 2019, 1:03 p.m. UTC | #1
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
>  	 */
>
Darrick J. Wong Dec. 4, 2019, 12:11 a.m. UTC | #2
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
> >  	 */
> > 
>

Patch
diff mbox series

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