diff mbox series

[10/12] xfs: fix agblocks check in the cow leftover recovery function

Message ID 166689089944.3788582.6885104145014798058.stgit@magnolia (mailing list archive)
State Superseded, archived
Headers show
Series xfs: improve runtime refcountbt corruption detection | expand

Commit Message

Darrick J. Wong Oct. 27, 2022, 5:14 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

As we've seen, refcount records use the upper bit of the rc_startblock
field to ensure that all the refcount records are at the right side of
the refcount btree.  This works because an AG is never allowed to have
more than (1U << 31) blocks in it.  If we ever encounter a filesystem
claiming to have that many blocks, we absolutely do not want reflink
touching it at all.

However, this test at the start of xfs_refcount_recover_cow_leftovers is
slightly incorrect -- it /should/ be checking that agblocks isn't larger
than the XFS_MAX_CRC_AG_BLOCKS constant, and it should check that the
constant is never large enough to conflict with that CoW flag.

Note that the V5 superblock verifier has not historically rejected
filesystems where agblocks <= XFS_MAX_CRC_AG_BLOCKS, which is why this
ended up in the COW recovery routine.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_refcount.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Dave Chinner Oct. 27, 2022, 9:22 p.m. UTC | #1 
On Thu, Oct 27, 2022 at 10:14:59AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> As we've seen, refcount records use the upper bit of the rc_startblock
> field to ensure that all the refcount records are at the right side of
> the refcount btree.  This works because an AG is never allowed to have
> more than (1U << 31) blocks in it.  If we ever encounter a filesystem
> claiming to have that many blocks, we absolutely do not want reflink
> touching it at all.
> 
> However, this test at the start of xfs_refcount_recover_cow_leftovers is
> slightly incorrect -- it /should/ be checking that agblocks isn't larger
> than the XFS_MAX_CRC_AG_BLOCKS constant, and it should check that the
> constant is never large enough to conflict with that CoW flag.
> 
> Note that the V5 superblock verifier has not historically rejected
> filesystems where agblocks <= XFS_MAX_CRC_AG_BLOCKS, which is why this
ITYM                         >=

> ended up in the COW recovery routine.

I think we should probably fix that - I didn't realise we had this
superblock geometry check buried deep in the reflink recovery code.

That said, for the moment adding an extra check to the reflink
recovery code is fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index 608a122eef16..529968b3b9c3 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -1811,7 +1811,9 @@  xfs_refcount_recover_cow_leftovers(
 	xfs_fsblock_t			fsb;
 	int				error;
 
-	if (mp->m_sb.sb_agblocks >= XFS_REFC_COW_START)
+	/* reflink filesystems mustn't have AGs larger than 2^31-1 blocks */
+	BUILD_BUG_ON(XFS_MAX_CRC_AG_BLOCKS >= XFS_REFC_COW_START);
+	if (mp->m_sb.sb_agblocks > XFS_MAX_CRC_AG_BLOCKS)
 		return -EOPNOTSUPP;
 
 	INIT_LIST_HEAD(&debris);