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

From: Dave Chinner <david@fromorbit.com>
To: Ben Myers <bpm@sgi.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 4/9] xfs: add CRC checks to the AGFL
Date: Thu, 28 Feb 2013 10:20:45 +1100	[thread overview]
Message-ID: <20130227232045.GE5551@dastard> (raw)
In-Reply-To: <20130227223750.GZ22182@sgi.com>

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@lst.de>
> > 
> > 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@redhat.com] 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@lst.de>
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> 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.

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...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs