| Message ID | 20180605025704.GZ10363@dastard (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Jun 05, 2018 at 12:57:04PM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > When looking up the root inode at mount time, we don't actually do > any verification to check that the inode is allocated and accounted > for correctly in the INOBT. Make the checks on the root inode more > robust by making it an untrusted lookup. This forces the inode > lookup to use the inode btree to verify the inode is allocated > and mapped correctly to disk. This will also have the effect of > catching a significant number of AGI/INOBT related corruptions in > AG 0 at mount time. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > --- > fs/xfs/xfs_mount.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 189fa7b615d3..a3378252baa1 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -862,9 +862,12 @@ xfs_mountfs( > * Get and sanity-check the root inode. > * Save the pointer to it in the mount structure. > */ > - error = xfs_iget(mp, NULL, sbp->sb_rootino, 0, XFS_ILOCK_EXCL, &rip); > + error = xfs_iget(mp, NULL, sbp->sb_rootino, XFS_IGET_UNTRUSTED, One little quirk I've noticed with xfs_iget is that a corrupt inode buffer's -EFSCORRUPTED gets turned into -EINVAL on the way out of iget. There's no way to distinguish between "the inode number was crap" vs. "the inode is marked in use but the buffer verifier failed". This particularly roars its head in the (directory) parent pointer scrubber where we try an untrusted iget of the .. entry and if the alleged parent inode is corrupt we lack the ability to distinguish the two (ideally we'd set SCRUB_OFLAG_CORRUPT if the .. entry is junk and SCRUB_OFLAG_XCORRUPT if the inode buffer verifier failed). I was just going to fix the parent pointer scrubber to check the inobt directly, but since this has the same reporting problem I figured I'd just air my dirty laundry on the list. :) --D > + XFS_ILOCK_EXCL, &rip); > if (error) { > - xfs_warn(mp, "failed to read root inode"); > + xfs_warn(mp, > + "Failed to read root inode 0x%llx, error %d", > + sbp->sb_rootino, -error); > goto out_log_dealloc; > } > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 04, 2018 at 09:06:57PM -0700, Darrick J. Wong wrote: > On Tue, Jun 05, 2018 at 12:57:04PM +1000, Dave Chinner wrote: > > > > From: Dave Chinner <dchinner@redhat.com> > > > > When looking up the root inode at mount time, we don't actually do > > any verification to check that the inode is allocated and accounted > > for correctly in the INOBT. Make the checks on the root inode more > > robust by making it an untrusted lookup. This forces the inode > > lookup to use the inode btree to verify the inode is allocated > > and mapped correctly to disk. This will also have the effect of > > catching a significant number of AGI/INOBT related corruptions in > > AG 0 at mount time. > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > > > --- > > fs/xfs/xfs_mount.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > > index 189fa7b615d3..a3378252baa1 100644 > > --- a/fs/xfs/xfs_mount.c > > +++ b/fs/xfs/xfs_mount.c > > @@ -862,9 +862,12 @@ xfs_mountfs( > > * Get and sanity-check the root inode. > > * Save the pointer to it in the mount structure. > > */ > > - error = xfs_iget(mp, NULL, sbp->sb_rootino, 0, XFS_ILOCK_EXCL, &rip); > > + error = xfs_iget(mp, NULL, sbp->sb_rootino, XFS_IGET_UNTRUSTED, > > One little quirk I've noticed with xfs_iget is that a corrupt inode > buffer's -EFSCORRUPTED gets turned into -EINVAL on the way out of iget. That's in xfs_imap_to_bp(), right? And the only place we care about this is xfs_nfs_get_inode() so that we return ESTALE rather than EFSCORRUPTED, yes? Ok, so let's fix that. Cheers, Dave.
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 189fa7b615d3..a3378252baa1 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -862,9 +862,12 @@ xfs_mountfs( * Get and sanity-check the root inode. * Save the pointer to it in the mount structure. */ - error = xfs_iget(mp, NULL, sbp->sb_rootino, 0, XFS_ILOCK_EXCL, &rip); + error = xfs_iget(mp, NULL, sbp->sb_rootino, XFS_IGET_UNTRUSTED, + XFS_ILOCK_EXCL, &rip); if (error) { - xfs_warn(mp, "failed to read root inode"); + xfs_warn(mp, + "Failed to read root inode 0x%llx, error %d", + sbp->sb_rootino, -error); goto out_log_dealloc; }