xfs
[Top] [All Lists]

Re: [PATCH 4/9] xfs: add CRC checks to the AGFL

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/9] xfs: add CRC checks to the AGFL
From: Ben Myers <bpm@xxxxxxx>
Date: Wed, 27 Feb 2013 17:32:00 -0600
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130227232045.GE5551@dastard>
References: <1358774760-21841-1-git-send-email-david@xxxxxxxxxxxxx> <1358774760-21841-5-git-send-email-david@xxxxxxxxxxxxx> <20130227223750.GZ22182@xxxxxxx> <20130227232045.GE5551@dastard>
User-agent: Mutt/1.5.20 (2009-06-14)
On Thu, Feb 28, 2013 at 10:20:45AM +1100, Dave Chinner wrote:
> On Wed, Feb 27, 2013 at 04:37:50PM -0600, Ben Myers wrote:
> > Hi Dave,
> > 
> > On Tue, Jan 22, 2013 at 12:25:55AM +1100, Dave Chinner wrote:
> > > From: Christoph Hellwig <hch@xxxxxx>
> > > 
> > > Add CRC checks, location information and a magic number to the AGFL.
> > > Previously the AGFL was just a block containing nothing but the
> > > free block pointers.  The new AGFL has a real header with the usual
> > > boilerplate instead, so that we can verify it's not corrupted and
> > > written into the right place.
> > > 
> > > [dchinner@xxxxxxxxxx] Added LSN field, reworked significantly to fit
> > > into new verifier structure and growfs structure, enabled full
> > > verifier functionality now there is a header to verify and we can
> > > guarantee an initialised AGFL.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > I have a couple comments below.
> .....
> > >   for (i = 0; i < XFS_AGFL_SIZE(mp); i++) {
> > > -         if (be32_to_cpu(agfl->agfl_bno[i]) == NULLAGBLOCK ||
> > > +         if (be32_to_cpu(agfl->agfl_bno[i]) != NULLAGBLOCK &&
> > >               be32_to_cpu(agfl->agfl_bno[i]) >= mp->m_sb.sb_agblocks)
> >                                                <
> 
> No, we are checking for the agbno being out of range here, not in
> range.

Ah.  Good.

> The previous code (which was ifdef'd out) reflected the fact that
> NULLAGBLOCK could not appear in a AGFL (initialised to zero, not
> NULLAGBLOCK), For CRC enabled filesystems - where this check is run,
> we guarantee that unused entries are initialised to NULLAGBLOCK by
> mkfs and growfs, and this change reflects that.
> 
> > Any non NULLAGBLOCK should be less than m_sb.sb_agblocks, correct?
> 
> xfs_agblock_t is an unsigned value, therefore it has a value of
> 0xffffffff. be32-to_cpu() also returns an unsigned value.
> So, no, is it never less than mp->m_sb.sb_agblocks.
> 
> But we don't want to rely on an implicit comparison against
> mp->m_sb.sb_agblocks to detect this, and hence we *always* check
> explicitly for it being a NULLAGBLOCK.
> 
> > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > > index 65c35d5..81d3cc5a 100644
> > > --- a/fs/xfs/xfs_log_recover.c
> > > +++ b/fs/xfs/xfs_log_recover.c
> > > @@ -1961,6 +1961,13 @@ xlog_recover_do_reg_buffer(
> > >           }
> > >           bp->b_ops = &xfs_agf_buf_ops;
> > >           break;
> > > + case XFS_BLF_AGFL_BUF:
> > > +         if (*(__be32 *)bp->b_addr != cpu_to_be32(XFS_AGFL_MAGIC)) {
> > > +                 xfs_warn(mp, "Bad AGFL block magic!");
> > > +                 ASSERT(0);
> > > +         }
> > > +         bp->b_ops = &xfs_agfl_buf_ops;
> > > +         break;
> > 
> > Your changes for v2 in this section look good.
> 
> Actually, the above hunk is broken. The magic number should only be
> checked for CRC enabled filesystems. My current code has this check,
> though I thought I fixed that long before I reposted this series...

I was pointing out that the changes included in v2 of this patch in this
section of code look good.

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