From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 03/13] xfs: have buffer verifier functions report failing address
Date: Tue, 19 Dec 2017 12:26:15 -0800 [thread overview]
Message-ID: <20171219202615.GD11969@magnolia> (raw)
In-Reply-To: <20171219041238.GM4094@dastard>
On Tue, Dec 19, 2017 at 03:12:38PM +1100, Dave Chinner wrote:
> On Wed, Dec 13, 2017 at 03:58:31PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Modify each function that checks the contents of a metadata buffer to
> > return the instruction address of the failing test so that we can report
> > more precise failure errors to the log.
>
>
> > @@ -331,29 +332,29 @@ xfs_allocbt_verify(
> > level = be16_to_cpu(block->bb_level);
> > switch (block->bb_magic) {
> > case cpu_to_be32(XFS_ABTB_CRC_MAGIC):
> > - if (!xfs_btree_sblock_v5hdr_verify(bp))
> > - return false;
> > + if ((fa = xfs_btree_sblock_v5hdr_verify(bp)))
> > + return fa;
>
> I see a few of these.
>
> fa = xfs_btree_sblock_v5hdr_verify(bp);
> if (fa)
> return fa;
Fixed.
> > @@ -4609,7 +4609,7 @@ xfs_btree_sblock_v5hdr_verify(
> > * @bp: buffer containing the btree block
> > * @max_recs: maximum records allowed in this btree node
> > */
> > -bool
> > +xfs_failaddr_t
> > xfs_btree_sblock_verify(
> > struct xfs_buf *bp,
> > unsigned int max_recs)
> > @@ -4619,19 +4619,19 @@ xfs_btree_sblock_verify(
> >
> > /* numrecs verification */
> > if (be16_to_cpu(block->bb_numrecs) > max_recs)
> > - return false;
> > + return __this_address;
> >
> > /* sibling pointer verification */
> > if (!block->bb_u.s.bb_leftsib ||
> > (be32_to_cpu(block->bb_u.s.bb_leftsib) >= mp->m_sb.sb_agblocks &&
> > block->bb_u.s.bb_leftsib != cpu_to_be32(NULLAGBLOCK)))
> > - return false;
> > + return __this_address;
> > if (!block->bb_u.s.bb_rightsib ||
> > (be32_to_cpu(block->bb_u.s.bb_rightsib) >= mp->m_sb.sb_agblocks &&
> > block->bb_u.s.bb_rightsib != cpu_to_be32(NULLAGBLOCK)))
> > - return false;
> > + return __this_address;
>
> Out if scope for this patch, but like th e lblock verifying, I think
> these are candidates for xfs_verify_agbno()?
Yep. Added another patch to fix this.
> > diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > index 27297a6..87565b6 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > @@ -73,13 +73,13 @@ xfs_dir3_leaf1_check(
> > } else if (leafhdr.magic != XFS_DIR2_LEAF1_MAGIC)
> > return false;
> >
> > - return xfs_dir3_leaf_check_int(dp->i_mount, dp, &leafhdr, leaf);
> > + return xfs_dir3_leaf_check_int(dp->i_mount, dp, &leafhdr, leaf) == NULL;
> > }
>
> Hmmmm - shouldn't we make xfs_dir3_leaf_check() dump the failaddr
> so we know what debug check failed? This would improve the debug
> failure reporting in this code, and seeing as we now have the info it
> doesn't really make sense to throw it away without reporting it...
Fixed.
> > diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> > index 682e2bf..00ded0a 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_node.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> > @@ -76,13 +76,13 @@ xfs_dir3_leafn_check(
> > } else if (leafhdr.magic != XFS_DIR2_LEAFN_MAGIC)
> > return false;
> >
> > - return xfs_dir3_leaf_check_int(dp->i_mount, dp, &leafhdr, leaf);
> > + return xfs_dir3_leaf_check_int(dp->i_mount, dp, &leafhdr, leaf) == NULL;
> > }
> > #else
> > #define xfs_dir3_leaf_check(dp, bp)
> > #endif
>
> Same here.
Fixed.
> > @@ -93,21 +93,21 @@ xfs_dir3_free_verify(
> > struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
> >
> > if (hdr3->magic != cpu_to_be32(XFS_DIR3_FREE_MAGIC))
> > - return false;
> > + return __this_address;
> > if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
> > - return false;
> > + return __this_address;
> > if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
> > - return false;
> > + return __this_address;
> > if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
> > - return false;
> > + return __this_address;
> > } else {
> > if (hdr->magic != cpu_to_be32(XFS_DIR2_FREE_MAGIC))
> > - return false;
> > + return __this_address;
> > }
>
> /me wonders if there is opportunity for factoring this header check
> seeing as it's repeated a few times, just with different magic
> numbers...
>
> > diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> > index 45c68d0..c441efb 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> > +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> > @@ -41,7 +41,7 @@ extern int xfs_dir2_leaf_to_block(struct xfs_da_args *args,
> > #ifdef DEBUG
> > #define xfs_dir3_data_check(dp, bp) \
> > do { \
> > - if (!__xfs_dir3_data_check((dp), (bp))) { \
> > + if (__xfs_dir3_data_check((dp), (bp))) { \
> > XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, \
> > (bp)->b_target->bt_mount, (bp)->b_addr); \
> > } \
>
> This should capture the failaddr and dump it as part of the
> corruption error, I think. Otherwise we lose what part of the
> structure was considered corrupt from the error report.
Ok.
> > @@ -507,7 +507,7 @@ xfs_iread(
> > return error;
> >
> > /* even unallocated inodes are verified */
> > - if (!xfs_dinode_verify(mp, ip->i_ino, dip)) {
> > + if (xfs_dinode_verify(mp, ip->i_ino, dip)) {
> > xfs_alert(mp, "%s: validation failed for inode %lld",
> > __func__, ip->i_ino);
>
> Capture the failaddr in the error message?
Ok.
> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> > index a9c97a3..2317511 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.h
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> > @@ -82,7 +82,7 @@ void xfs_inobp_check(struct xfs_mount *, struct xfs_buf *);
> > #define xfs_inobp_check(mp, bp)
> > #endif /* DEBUG */
> >
> > -bool xfs_dinode_verify(struct xfs_mount *mp, xfs_ino_t ino,
> > - struct xfs_dinode *dip);
> > +void *xfs_dinode_verify(struct xfs_mount *mp, xfs_ino_t ino,
> > + struct xfs_dinode *dip);
>
> xfs_failaddr_t?
Fixed.
--D
> Cheers,
>
> Dave.
>
> --
> Dave Chinner
> david@fromorbit.com
> --
> 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
next prev parent reply other threads:[~2017-12-19 20:26 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-13 23:58 [PATCH 00/13] xfs: more and better verifiers Darrick J. Wong
2017-12-13 23:58 ` [PATCH 01/13] xfs: refactor long-format btree header verification routines Darrick J. Wong
2017-12-14 22:06 ` Dave Chinner
2017-12-15 0:12 ` Darrick J. Wong
2017-12-13 23:58 ` [PATCH 02/13] xfs: remove XFS_WANT_CORRUPTED_RETURN from dir3 data verifiers Darrick J. Wong
2017-12-19 3:50 ` Dave Chinner
2017-12-13 23:58 ` [PATCH 03/13] xfs: have buffer verifier functions report failing address Darrick J. Wong
2017-12-19 4:12 ` Dave Chinner
2017-12-19 20:26 ` Darrick J. Wong [this message]
2017-12-13 23:58 ` [PATCH 04/13] xfs: refactor verifier callers to print address of failing check Darrick J. Wong
2017-12-14 22:03 ` Dave Chinner
2017-12-15 0:04 ` Darrick J. Wong
2017-12-15 3:09 ` Dave Chinner
2017-12-19 20:29 ` Darrick J. Wong
2017-12-13 23:58 ` [PATCH 05/13] xfs: verify dinode header first Darrick J. Wong
2017-12-19 4:13 ` Dave Chinner
2017-12-13 23:58 ` [PATCH 06/13] xfs: move inode fork verifiers to xfs_dinode_verify Darrick J. Wong
2017-12-19 5:16 ` Dave Chinner
2017-12-19 20:34 ` Darrick J. Wong
2017-12-19 20:48 ` Dave Chinner
2017-12-13 23:58 ` [PATCH 07/13] xfs: create structure verifier function for shortform xattrs Darrick J. Wong
2017-12-19 5:23 ` Dave Chinner
2017-12-19 20:41 ` Darrick J. Wong
2017-12-19 20:51 ` Dave Chinner
2017-12-19 21:04 ` Darrick J. Wong
2017-12-13 23:59 ` [PATCH 08/13] xfs: create structure verifier function for short form symlinks Darrick J. Wong
2017-12-19 5:27 ` Dave Chinner
2017-12-19 20:45 ` Darrick J. Wong
2017-12-13 23:59 ` [PATCH 09/13] xfs: refactor short form directory structure verifier function Darrick J. Wong
2017-12-19 5:45 ` Dave Chinner
2017-12-13 23:59 ` [PATCH 10/13] xfs: provide a centralized method for verifying inline fork data Darrick J. Wong
2017-12-19 6:06 ` Dave Chinner
2017-12-19 20:50 ` Darrick J. Wong
2017-12-13 23:59 ` [PATCH 11/13] xfs: fail out of xfs_attr3_leaf_lookup_int if it looks corrupt Darrick J. Wong
2017-12-19 6:13 ` Dave Chinner
2017-12-13 23:59 ` [PATCH 12/13] xfs: create a new buf_ops pointer to verify structure metadata Darrick J. Wong
2017-12-19 6:22 ` Dave Chinner
2017-12-19 18:15 ` Darrick J. Wong
2017-12-19 20:53 ` Dave Chinner
2017-12-13 23:59 ` [PATCH 13/13] xfs: scrub in-core metadata Darrick J. Wong
2017-12-19 6:23 ` Dave Chinner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171219202615.GD11969@magnolia \
--to=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox