From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 1/4] xfs: detect self referencing btree sibling pointers
Date: Tue, 3 May 2022 15:53:14 -0700 [thread overview]
Message-ID: <20220503225314.GF8265@magnolia> (raw)
In-Reply-To: <20220502082018.1076561-2-david@fromorbit.com>
On Mon, May 02, 2022 at 06:20:15PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> To catch the obvious graph cycle problem and hence potential endless
> looping.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/xfs/libxfs/xfs_btree.c | 137 ++++++++++++++++++++++++++++----------
> 1 file changed, 102 insertions(+), 35 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index a8c79e760d8a..991fae6f500a 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -51,6 +51,50 @@ xfs_btree_magic(
> return magic;
> }
>
> +static xfs_failaddr_t
Minor nit: static inline?
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
> +xfs_btree_check_lblock_siblings(
> + struct xfs_mount *mp,
> + struct xfs_btree_cur *cur,
> + int level,
> + xfs_fsblock_t fsb,
> + xfs_fsblock_t sibling)
> +{
> + if (sibling == NULLFSBLOCK)
> + return NULL;
> + if (sibling == fsb)
> + return __this_address;
> + if (level >= 0) {
> + if (!xfs_btree_check_lptr(cur, sibling, level + 1))
> + return __this_address;
> + } else if (!xfs_verify_fsbno(mp, sibling)) {
> + return __this_address;
> + }
> +
> + return NULL;
> +}
> +
> +static xfs_failaddr_t
> +xfs_btree_check_sblock_siblings(
> + struct xfs_mount *mp,
> + struct xfs_btree_cur *cur,
> + int level,
> + xfs_agnumber_t agno,
> + xfs_agblock_t agbno,
> + xfs_agblock_t sibling)
> +{
> + if (sibling == NULLAGBLOCK)
> + return NULL;
> + if (sibling == agbno)
> + return __this_address;
> + if (level >= 0) {
> + if (!xfs_btree_check_sptr(cur, sibling, level + 1))
> + return __this_address;
> + } else if (!xfs_verify_agbno(mp, agno, sibling)) {
> + return __this_address;
> + }
> + return NULL;
> +}
> +
> /*
> * Check a long btree block header. Return the address of the failing check,
> * or NULL if everything is ok.
> @@ -65,6 +109,8 @@ __xfs_btree_check_lblock(
> struct xfs_mount *mp = cur->bc_mp;
> xfs_btnum_t btnum = cur->bc_btnum;
> int crc = xfs_has_crc(mp);
> + xfs_failaddr_t fa;
> + xfs_fsblock_t fsb = NULLFSBLOCK;
>
> if (crc) {
> if (!uuid_equal(&block->bb_u.l.bb_uuid, &mp->m_sb.sb_meta_uuid))
> @@ -83,16 +129,16 @@ __xfs_btree_check_lblock(
> if (be16_to_cpu(block->bb_numrecs) >
> cur->bc_ops->get_maxrecs(cur, level))
> return __this_address;
> - if (block->bb_u.l.bb_leftsib != cpu_to_be64(NULLFSBLOCK) &&
> - !xfs_btree_check_lptr(cur, be64_to_cpu(block->bb_u.l.bb_leftsib),
> - level + 1))
> - return __this_address;
> - if (block->bb_u.l.bb_rightsib != cpu_to_be64(NULLFSBLOCK) &&
> - !xfs_btree_check_lptr(cur, be64_to_cpu(block->bb_u.l.bb_rightsib),
> - level + 1))
> - return __this_address;
>
> - return NULL;
> + if (bp)
> + fsb = XFS_DADDR_TO_FSB(mp, xfs_buf_daddr(bp));
> +
> + fa = xfs_btree_check_lblock_siblings(mp, cur, level, fsb,
> + be64_to_cpu(block->bb_u.l.bb_leftsib));
> + if (!fa)
> + fa = xfs_btree_check_lblock_siblings(mp, cur, level, fsb,
> + be64_to_cpu(block->bb_u.l.bb_rightsib));
> + return fa;
> }
>
> /* Check a long btree block header. */
> @@ -130,6 +176,9 @@ __xfs_btree_check_sblock(
> struct xfs_mount *mp = cur->bc_mp;
> xfs_btnum_t btnum = cur->bc_btnum;
> int crc = xfs_has_crc(mp);
> + xfs_failaddr_t fa;
> + xfs_agblock_t agbno = NULLAGBLOCK;
> + xfs_agnumber_t agno = NULLAGNUMBER;
>
> if (crc) {
> if (!uuid_equal(&block->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid))
> @@ -146,16 +195,18 @@ __xfs_btree_check_sblock(
> if (be16_to_cpu(block->bb_numrecs) >
> cur->bc_ops->get_maxrecs(cur, level))
> return __this_address;
> - if (block->bb_u.s.bb_leftsib != cpu_to_be32(NULLAGBLOCK) &&
> - !xfs_btree_check_sptr(cur, be32_to_cpu(block->bb_u.s.bb_leftsib),
> - level + 1))
> - return __this_address;
> - if (block->bb_u.s.bb_rightsib != cpu_to_be32(NULLAGBLOCK) &&
> - !xfs_btree_check_sptr(cur, be32_to_cpu(block->bb_u.s.bb_rightsib),
> - level + 1))
> - return __this_address;
>
> - return NULL;
> + if (bp) {
> + agbno = xfs_daddr_to_agbno(mp, xfs_buf_daddr(bp));
> + agno = xfs_daddr_to_agno(mp, xfs_buf_daddr(bp));
> + }
> +
> + fa = xfs_btree_check_sblock_siblings(mp, cur, level, agno, agbno,
> + be32_to_cpu(block->bb_u.s.bb_leftsib));
> + if (!fa)
> + fa = xfs_btree_check_sblock_siblings(mp, cur, level, agno,
> + agbno, be32_to_cpu(block->bb_u.s.bb_rightsib));
> + return fa;
> }
>
> /* Check a short btree block header. */
> @@ -4271,6 +4322,20 @@ xfs_btree_visit_block(
> if (xfs_btree_ptr_is_null(cur, &rptr))
> return -ENOENT;
>
> + /*
> + * We only visit blocks once in this walk, so we have to avoid the
> + * internal xfs_btree_lookup_get_block() optimisation where it will
> + * return the same block without checking if the right sibling points
> + * back to us and creates a cyclic reference in the btree.
> + */
> + if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
> + if (be64_to_cpu(rptr.l) ==
> + XFS_DADDR_TO_FSB(cur->bc_mp, xfs_buf_daddr(bp)))
> + return -EFSCORRUPTED;
> + } else if (be32_to_cpu(rptr.s) ==
> + xfs_daddr_to_agbno(cur->bc_mp, xfs_buf_daddr(bp))) {
> + return -EFSCORRUPTED;
> + }
> return xfs_btree_lookup_get_block(cur, level, &rptr, &block);
> }
>
> @@ -4445,20 +4510,21 @@ xfs_btree_lblock_verify(
> {
> struct xfs_mount *mp = bp->b_mount;
> struct xfs_btree_block *block = XFS_BUF_TO_BLOCK(bp);
> + xfs_fsblock_t fsb;
> + xfs_failaddr_t fa;
>
> /* numrecs verification */
> if (be16_to_cpu(block->bb_numrecs) > max_recs)
> return __this_address;
>
> /* sibling pointer verification */
> - if (block->bb_u.l.bb_leftsib != cpu_to_be64(NULLFSBLOCK) &&
> - !xfs_verify_fsbno(mp, be64_to_cpu(block->bb_u.l.bb_leftsib)))
> - return __this_address;
> - if (block->bb_u.l.bb_rightsib != cpu_to_be64(NULLFSBLOCK) &&
> - !xfs_verify_fsbno(mp, be64_to_cpu(block->bb_u.l.bb_rightsib)))
> - return __this_address;
> -
> - return NULL;
> + fsb = XFS_DADDR_TO_FSB(mp, xfs_buf_daddr(bp));
> + fa = xfs_btree_check_lblock_siblings(mp, NULL, -1, fsb,
> + be64_to_cpu(block->bb_u.l.bb_leftsib));
> + if (!fa)
> + fa = xfs_btree_check_lblock_siblings(mp, NULL, -1, fsb,
> + be64_to_cpu(block->bb_u.l.bb_rightsib));
> + return fa;
> }
>
> /**
> @@ -4499,7 +4565,9 @@ xfs_btree_sblock_verify(
> {
> struct xfs_mount *mp = bp->b_mount;
> struct xfs_btree_block *block = XFS_BUF_TO_BLOCK(bp);
> - xfs_agblock_t agno;
> + xfs_agnumber_t agno;
> + xfs_agblock_t agbno;
> + xfs_failaddr_t fa;
>
> /* numrecs verification */
> if (be16_to_cpu(block->bb_numrecs) > max_recs)
> @@ -4507,14 +4575,13 @@ xfs_btree_sblock_verify(
>
> /* sibling pointer verification */
> agno = xfs_daddr_to_agno(mp, xfs_buf_daddr(bp));
> - if (block->bb_u.s.bb_leftsib != cpu_to_be32(NULLAGBLOCK) &&
> - !xfs_verify_agbno(mp, agno, be32_to_cpu(block->bb_u.s.bb_leftsib)))
> - return __this_address;
> - if (block->bb_u.s.bb_rightsib != cpu_to_be32(NULLAGBLOCK) &&
> - !xfs_verify_agbno(mp, agno, be32_to_cpu(block->bb_u.s.bb_rightsib)))
> - return __this_address;
> -
> - return NULL;
> + agbno = xfs_daddr_to_agbno(mp, xfs_buf_daddr(bp));
> + fa = xfs_btree_check_sblock_siblings(mp, NULL, -1, agno, agbno,
> + be32_to_cpu(block->bb_u.s.bb_leftsib));
> + if (!fa)
> + fa = xfs_btree_check_sblock_siblings(mp, NULL, -1, agno, agbno,
> + be32_to_cpu(block->bb_u.s.bb_rightsib));
> + return fa;
> }
>
> /*
> --
> 2.35.1
>
next prev parent reply other threads:[~2022-05-03 22:54 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-02 8:20 [PATCH 0/4] xfs: fix random format verification issues Dave Chinner
2022-05-02 8:20 ` [PATCH 1/4] xfs: detect self referencing btree sibling pointers Dave Chinner
2022-05-03 14:53 ` Christoph Hellwig
2022-05-03 21:27 ` Dave Chinner
2022-05-03 22:53 ` Darrick J. Wong [this message]
2022-05-03 23:13 ` Dave Chinner
2022-05-06 9:22 ` [xfs] 32678f1513: aim7.jobs-per-min -5.6% regression kernel test robot
2022-05-06 21:29 ` Dave Chinner
2022-05-07 11:09 ` [LKP] " Carel Si
2022-05-09 0:03 ` Dave Chinner
2022-05-02 8:20 ` [PATCH 2/4] xfs: validate inode fork size against fork format Dave Chinner
2022-05-03 14:55 ` Christoph Hellwig
2022-05-03 22:55 ` Darrick J. Wong
2022-05-02 8:20 ` [PATCH 3/4] xfs: set XFS_FEAT_NLINK correctly Dave Chinner
2022-05-03 14:56 ` Christoph Hellwig
2022-05-03 22:55 ` Darrick J. Wong
2022-05-02 8:20 ` [PATCH 4/4] xfs: validate v5 feature fields Dave Chinner
2022-05-02 9:44 ` kernel test robot
2022-05-02 12:37 ` kernel test robot
2022-05-03 15:00 ` Christoph Hellwig
2022-05-03 21:26 ` Dave Chinner
2022-05-03 22:59 ` Darrick J. Wong
2022-05-03 23:18 ` Dave Chinner
2022-05-03 23:28 ` Darrick J. Wong
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=20220503225314.GF8265@magnolia \
--to=djwong@kernel.org \
--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