xfs
[Top] [All Lists]

Re: [PATCH 12/21] xfs: add CRC checking to dir2 free blocks

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [PATCH 12/21] xfs: add CRC checking to dir2 free blocks
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 29 Mar 2013 14:13:31 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130328234023.GW22182@xxxxxxx>
References: <1363091454-8852-1-git-send-email-david@xxxxxxxxxxxxx> <1363091454-8852-13-git-send-email-david@xxxxxxxxxxxxx> <20130328234023.GW22182@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Mar 28, 2013 at 06:40:23PM -0500, Ben Myers wrote:
> Hey Dave,
> 
> On Tue, Mar 12, 2013 at 11:30:45PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > This addition follows the same pattern as the dir2 block CRCs, but
> > with a few differences. The main difference is that the free block
> > header is different between the v2 and v3 formats, so an "in-core"
> > free block header has been added and _todisk/_from_disk functions
> > used to abstract the differences in structure format from the code.
> > This is similar to the on-disk superblock versus the in-core
> > superblock setup. The in-core strucutre is populated when the buffer
> > is read from disk, all the in memory checks and modifications are
> > done on the in-core version of the structure which is written back
> > to the buffer before the buffer is logged.
> > 
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> Just a few nits below.
> 
> > -static void
> > -xfs_dir2_free_verify(
> > +static bool
> > +xfs_dir3_free_verify(
> >     struct xfs_buf          *bp)
> >  {
> >     struct xfs_mount        *mp = bp->b_target->bt_mount;
> >     struct xfs_dir2_free_hdr *hdr = bp->b_addr;
> > -   int                     block_ok = 0;
> >  
> > -   block_ok = hdr->magic == cpu_to_be32(XFS_DIR2_FREE_MAGIC);
> > -   if (!block_ok) {
> > -           XFS_CORRUPTION_ERROR("xfs_dir2_free_verify magic",
> > -                                XFS_ERRLEVEL_LOW, mp, hdr);
> > -           xfs_buf_ioerror(bp, EFSCORRUPTED);
> > +   if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > +           struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
> > +
> > +           if (hdr3->magic != be32_to_cpu(XFS_DIR3_FREE_MAGIC))
> > +                   return false;
> > +           if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_uuid))
> > +                   return false;
> > +           if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
> > +                   return false;
> > +   } else {
> > +           if (hdr->magic != be32_to_cpu(XFS_DIR2_FREE_MAGIC))
> > +                   return false;
> >     }
> > +
> > +   /* XXX: should bounds check the xfs_dir3_icfree_hdr here */
> 
> What did you have in mind?

Checking that the header fields:
        a) are within valid bounds; and
        b) match what is in the bests array.

> > -   if (!hdr) {
> > -           /* One less used entry in the free table.  */
> > -           be32_add_cpu(&free->hdr.nused, -1);
> > -           xfs_dir2_free_log_header(tp, fbp);
> >  
> > -           /*
> > -            * If this was the last entry in the table, we can trim the
> > -            * table size back.  There might be other entries at the end
> > -            * referring to non-existent data blocks, get those too.
> > -            */
> 
> You dropped this comment.  Was there something wrong with it?

No, just an oversight when rearranging the code.

> >     /* Log the free entry that changed, unless we got rid of it.  */
> >     if (logfree)
> >             xfs_dir2_free_log_bests(tp, fbp, findex, findex);
> > @@ -1062,10 +1221,15 @@ xfs_dir2_leafn_remove(
> >             if (error)
> >                     return error;
> >             free = fbp->b_addr;
> > -           ASSERT(free->hdr.magic == cpu_to_be32(XFS_DIR2_FREE_MAGIC));
> > -           ASSERT(be32_to_cpu(free->hdr.firstdb) ==
> > -                  xfs_dir2_free_max_bests(mp) *
> > -                  (fdb - XFS_DIR2_FREE_FIRSTDB(mp)));
> > +#ifdef DEBUG
> > +   {
> > +           struct xfs_dir3_icfree_hdr freehdr;
> > +           xfs_dir3_free_hdr_from_disk(&freehdr, free);
> > +           ASSERT(be32_to_cpu(freehdr.firstdb) ==
> 
> I think freehdr.firstdb is already in cpu endianness.

Right. Well spotted. I've already posted a fix for it - sparse
endian checking picked this up. My current series folds those fixes
back into the original patches....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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