xfs
[Top] [All Lists]

Re: [PATCH 04/19] xfs: verify superblocks as they are read from disk

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 04/19] xfs: verify superblocks as they are read from disk
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 12 Oct 2012 09:28:08 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20121011214139.GD6346@xxxxxxxxxxxxx>
References: <1349754670-32009-1-git-send-email-david@xxxxxxxxxxxxx> <1349754670-32009-5-git-send-email-david@xxxxxxxxxxxxx> <20121011214139.GD6346@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Oct 11, 2012 at 05:41:39PM -0400, Christoph Hellwig wrote:
> On Tue, Oct 09, 2012 at 02:50:55PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > Add a superblock verify callback function and pass it into the
> > buffer read functions. Remove the now redundant verification code
> > that is currently in use.
> > 
> > Adding verification shows that secondary superblocks never have
> > their "sb_inprogress" flag cleared by mkfs.xfs, so when validating
> > the secondary superblocks during a grow operation we have to avoid
> > checking this field. Even if we fix mkfs, we will still have to
> > ignore this field for verification purposes unless a version of mkfs
> > that does not have this bug was used.
> 
> > @@ -304,9 +304,8 @@ STATIC int
> >  xfs_mount_validate_sb(
> >     xfs_mount_t     *mp,
> >     xfs_sb_t        *sbp,
> > -   int             flags)
> > +   bool            check_inprogress)
> >  {
> > -   int             loud = !(flags & XFS_MFSI_QUIET);
> 
> I don't think removing this is a good idea.  The quiet flag is used
> to silence all filesystem warnings when the kernel is blindly trying
> all filesystem types when searching for the correct root fs type.
> 
> If we always print warnings here people will get annoying messages
> when that happens for a non-XFS rootfs that we're asked to verify.
> 
> I'd rather make check_inprogress another flag here.

It's a little more complex than that - I'd like to have the warnings
emitted on every superblock read (primary or secondary), but only
some of them come through the mount path. e.g. we re-read the
superblock during the log recovery sequence, so even if it is a
silent mount, I want to know that log recovery corrupted the
superblock and what it corrupted....

Indeed, if the kernel is trying random filesystem mounts on a block
device, then we'll fail the magic number check and abort
immediately, so I think that's really the only message we need "loud"
protection on. If you agree that's all we'll need to check, then I
can write a couple of simple wrappers that do:

xfs_sb_read_verify()
{
        /* validate contents of superblock loudly */
}

xfs_sb_quiet_read_verify()
{

        if (!magic num mismatch) {
                /* XFS filesystem, be loud now */
                xfs_sb_read_verify();
                return;
        }
        /* quitely fail now */
        xfs_buf_ioerror(bp, EFSCORRUPTED);
        ....
}

Would that solve the problem?

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>