[5/6] xfs_repair: check plausibility of root dir pointer before trashing it
diff mbox series

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

Commit Message

Darrick J. Wong Jan. 24, 2020, 12:17 a.m. UTC
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(+)

Comments

Eric Sandeen Jan. 30, 2020, 8:18 p.m. UTC | #1
On 1/23/20 6:17 PM, 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.

I forget, is there an fstest for this?

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

...

> @@ -438,6 +469,20 @@ calc_mkfs(
>  
>  	rootino = libxfs_ialloc_calc_rootino(mp, mp->m_sb.sb_unit);
>  
> +	/*
> +	 * 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);

what would a user do with this warning?  Is there any value in emitting it?

Otherwise this looks good.


> +		rootino = mp->m_sb.sb_rootino;
> +	}
> +
>  	ensure_fixed_ino(&mp->m_sb.sb_rootino, rootino,
>  			_("root"));
>  	ensure_fixed_ino(&mp->m_sb.sb_rbmino, rootino + 1,
>
Darrick J. Wong Jan. 30, 2020, 8:34 p.m. UTC | #2
On Thu, Jan 30, 2020 at 02:18:52PM -0600, Eric Sandeen wrote:
> On 1/23/20 6:17 PM, 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.
> 
> I forget, is there an fstest for this?

https://lore.kernel.org/linux-xfs/20191218041831.GK12765@magnolia/

> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> ...
> 
> > @@ -438,6 +469,20 @@ calc_mkfs(
> >  
> >  	rootino = libxfs_ialloc_calc_rootino(mp, mp->m_sb.sb_unit);
> >  
> > +	/*
> > +	 * 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);
> 
> what would a user do with this warning?  Is there any value in emitting it?
> 
> Otherwise this looks good.

I dunno -- on the one hand, I understand that nobody wants to deal with
the support calls that will erupt from that message.  On the other hand,
it's an indication that this filesystem isn't /quite/ the way we
expected it to be, and that would be a helpful hint if you were
debugging some other weird problem with an xfs filesystem.

What if this were a do_log()?

--D
> 
> 
> > +		rootino = mp->m_sb.sb_rootino;
> > +	}
> > +
> >  	ensure_fixed_ino(&mp->m_sb.sb_rootino, rootino,
> >  			_("root"));
> >  	ensure_fixed_ino(&mp->m_sb.sb_rbmino, rootino + 1,
> >
Eric Sandeen Jan. 30, 2020, 8:41 p.m. UTC | #3
On 1/30/20 2:34 PM, Darrick J. Wong wrote:
> On Thu, Jan 30, 2020 at 02:18:52PM -0600, Eric Sandeen wrote:
>> On 1/23/20 6:17 PM, 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.
>>
>> I forget, is there an fstest for this?
> 
> https://lore.kernel.org/linux-xfs/20191218041831.GK12765@magnolia/

of course :)

...

>>> +	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);
>>
>> what would a user do with this warning?  Is there any value in emitting it?
>>
>> Otherwise this looks good.
> 
> I dunno -- on the one hand, I understand that nobody wants to deal with
> the support calls that will erupt from that message.  On the other hand,
> it's an indication that this filesystem isn't /quite/ the way we
> expected it to be, and that would be a helpful hint if you were
> debugging some other weird problem with an xfs filesystem.
> 
> What if this were a do_log()?

how about something that's less indicative of a problem and more informational,

"sb root inode validated in unaligned location possibly due to sunit change"

-Eric
Darrick J. Wong Jan. 30, 2020, 8:50 p.m. UTC | #4
On Thu, Jan 30, 2020 at 02:41:29PM -0600, Eric Sandeen wrote:
> On 1/30/20 2:34 PM, Darrick J. Wong wrote:
> > On Thu, Jan 30, 2020 at 02:18:52PM -0600, Eric Sandeen wrote:
> >> On 1/23/20 6:17 PM, 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.
> >>
> >> I forget, is there an fstest for this?
> > 
> > https://lore.kernel.org/linux-xfs/20191218041831.GK12765@magnolia/
> 
> of course :)

:D

> ...
> 
> >>> +	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);
> >>
> >> what would a user do with this warning?  Is there any value in emitting it?
> >>
> >> Otherwise this looks good.
> > 
> > I dunno -- on the one hand, I understand that nobody wants to deal with
> > the support calls that will erupt from that message.  On the other hand,
> > it's an indication that this filesystem isn't /quite/ the way we
> > expected it to be, and that would be a helpful hint if you were
> > debugging some other weird problem with an xfs filesystem.
> > 
> > What if this were a do_log()?
> 
> how about something that's less indicative of a problem and more informational,
> 
> "sb root inode validated in unaligned location possibly due to sunit change"

Sounds good to me.

--D

> -Eric

Patch
diff mbox series

diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 53b04dae..372616c4 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.
@@ -438,6 +469,20 @@  calc_mkfs(
 
 	rootino = libxfs_ialloc_calc_rootino(mp, mp->m_sb.sb_unit);
 
+	/*
+	 * 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;
+	}
+
 	ensure_fixed_ino(&mp->m_sb.sb_rootino, rootino,
 			_("root"));
 	ensure_fixed_ino(&mp->m_sb.sb_rbmino, rootino + 1,