[PATCH 0/3] xfs: improve AGF/AGFL verification

[PATCH 0/3] xfs: improve AGF/AGFL verification

[Dave Chinner]

Hi Folks,

This set of patches is a result of a recent syzkaller bug report on
v4 filesystems. It tripped over a corrupt AGFL count field in the
AGF, but the failure didn't manifest until a crash processing extent
free operations.

It turns out the crash was due to deferred AGFL freeing being passed
a NULLAGBNO as the block to free - this resulted in an invalid
filesystem block value which translated to an invalid AGNO which
then failed a perag lookup in the defered extent freeing code.

While the oops was a result of a change in the 6.4-rc1 merge window,
this is not the cause of the problem - the problem has always been
there for V4 filesystems. However, V5 filesystems would have
detected this specific corruption and would not have failed at all.
There are other failures to detect bad extent ranges being
freed so the v5 code needs improvement, too.

The V5 verification is not done in the AGF/AGFL verifiers.
It is deferred to initialising the perag from the AGF after it has
been read from disk and the verifiers have been run. It is not
documented in the code that the verifiers intentionally don't check
free list indexes, even though they could in most situations.

The verification for v5 filesystems was introduced back when we
discovered that there was an issue with the on-disk sizing of the v5
AGFL header. Code was added to detect the mismatch at runtime
instead of in the verifier so that it could be corrected without
shutting down the filesystem. It does so by resetting the AGFL
indexes and the AGFL in the first transaction that needs to fix the
free list. The old contents of the AGFL are leaked, but the system
continues to function without any further issues.

For some reason, this verification was not extented to v4
filesystems, even though it is still necessary to catch the same
corruptions on V4 filesystems. The first patch in the series simply
runs the existing v5 AGFL index verification on v4 filesystems and
triggers the AGFL reset code in the same way. This is all that is
necessary to "fix" the syzkaller reproducer.

However, one of the things the verification does not do is determine
if the agbno we pull from the AGFL in xfs_alloc_get_freelist() is
valid. The AGFL read verifier for V5 filesystems checks that each
entry is either in range or NULLAGBNO, but that isn't sufficient. V4
filesystems can't even do this, because for a long time mkfs didn't
initialise AGFLs to NULLAGBNO so can contain anything at all.

Hence we have to check the AGBNO we pull from the AGFL as being in a
valid range. NULLAGBNO is not valid - that's what triggered the oops
later on.  If we caught invalid AGBNOs in xfs_alloc_get_freelist()
and returned -EFSCORRUPTED, the system would not have crashed. The
second patch addresses this.

Finally, nothing checked that the fsbno we are adding to xefi for
deferred freeing is actually valid. This fsbno can come from any
other structure in the filesystem, and we magically trust that it is
valid despite many paths into this code not directly verifying the
extent we are asking to be freed is valid. Freeing a bogus extent
will, at best, result in a free space btree with a corrupt record in
it that we trip over later and shut down the filesystem. Worst
case, it will crash the system (as per the syzkaller report).

The third patch in the series hardens the extent freeing code to
reject bogus extent ranges at the time the code attempts to queue
them for freeing. It also adds error handling to all the code that
defers extent free operations so that the filesystem can be shut
down immediately from the context that holds the bad extent, rather
than having it get passes silently to later operations that end up
tripping over it.

IOWs, all three patches are needed to close the holes that lead to
the system crashing rather than detecting AGF/AGFL corruption and
handling it cleanly.

Cheers,

Dave.

[PATCH 1/3] xfs: fix agf/agfl verification on v4 filesystems

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When a v4 filesystem has fl_last - fl_first != fl_count, we do not
not detect the corruption and allow the AGF to be used as it if was
fully valid. On V5 filesystems, we reset the AGFL to empty in these
cases and avoid the corruption at a small cost of leaked blocks.

If we don't catch the corruption on V4 filesystems, bad things
happen later when an allocation attempts to trim the free list
and either double-frees stale entries in the AGFl or tries to free
NULLAGBNO entries.

Either way, this is bad. Prevent this from happening by using the
AGFL_NEED_RESET logic for v4 filesysetms, too.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_alloc.c | 59 ++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 61eb65be17f3..fd3293a8c659 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -628,6 +628,25 @@ xfs_alloc_fixup_trees(
 	return 0;
 }
 
+/*
+ * We do not verify the AGFL contents against AGF-based index counters here,
+ * even though we may have access to the perag that contains shadow copies. We
+ * don't know if the AGF based counters have been checked, and if they have they
+ * still may be inconsistent because they haven't yet been reset on the first
+ * allocation after the AGF has been read in.
+ *
+ * This means we can only check that all agfl entries contain valid or null
+ * values because we can't reliably determine the active range to exclude
+ * NULLAGBNO as a valid value.
+ *
+ * However, we can't even do that for v4 format filesystems because there are
+ * old versions of mkfs out there that does not initialise the AGFL to known,
+ * verifiable values. HEnce we can't tell the difference between a AGFL block
+ * allocated by mkfs and a corrupted AGFL block here on v4 filesystems.
+ *
+ * As a result, we can only fully validate AGFL block numbers when we pull them
+ * from the freelist in xfs_alloc_get_freelist().
+ */
 static xfs_failaddr_t
 xfs_agfl_verify(
 	struct xfs_buf	*bp)
@@ -637,12 +656,6 @@ xfs_agfl_verify(
 	__be32		*agfl_bno = xfs_buf_to_agfl_bno(bp);
 	int		i;
 
-	/*
-	 * There is no verification of non-crc AGFLs because mkfs does not
-	 * initialise the AGFL to zero or NULL. Hence the only valid part of the
-	 * AGFL is what the AGF says is active. We can't get to the AGF, so we
-	 * can't verify just those entries are valid.
-	 */
 	if (!xfs_has_crc(mp))
 		return NULL;
 
@@ -2321,12 +2334,16 @@ xfs_free_agfl_block(
 }
 
 /*
- * Check the agfl fields of the agf for inconsistency or corruption. The purpose
- * is to detect an agfl header padding mismatch between current and early v5
- * kernels. This problem manifests as a 1-slot size difference between the
- * on-disk flcount and the active [first, last] range of a wrapped agfl. This
- * may also catch variants of agfl count corruption unrelated to padding. Either
- * way, we'll reset the agfl and warn the user.
+ * Check the agfl fields of the agf for inconsistency or corruption.
+ *
+ * The original purpose was to detect an agfl header padding mismatch between
+ * current and early v5 kernels. This problem manifests as a 1-slot size
+ * difference between the on-disk flcount and the active [first, last] range of
+ * a wrapped agfl.
+ *
+ * However, we need to use these same checks to catch agfl count corruptions
+ * unrelated to padding. This could occur on any v4 or v5 filesystem, so either
+ * way, we need to reset the agfl and warn the user.
  *
  * Return true if a reset is required before the agfl can be used, false
  * otherwise.
@@ -2342,10 +2359,6 @@ xfs_agfl_needs_reset(
 	int			agfl_size = xfs_agfl_size(mp);
 	int			active;
 
-	/* no agfl header on v4 supers */
-	if (!xfs_has_crc(mp))
-		return false;
-
 	/*
 	 * The agf read verifier catches severe corruption of these fields.
 	 * Repeat some sanity checks to cover a packed -> unpacked mismatch if
@@ -2889,6 +2902,19 @@ xfs_alloc_put_freelist(
 	return 0;
 }
 
+/*
+ * Verify the AGF is consistent.
+ *
+ * We do not verify the AGFL indexes in the AGF are fully consistent here
+ * because of issues with variable on-disk structure sizes. Instead, we check
+ * the agfl indexes for consistency when we initialise the perag from the AGF
+ * information after a read completes.
+ *
+ * If the index is inconsistent, then we mark the perag as needing an AGFL
+ * reset. The first AGFL update performed then resets the AGFL indexes and
+ * refills the AGFL with known good free blocks, allowing the filesystem to
+ * continue operating normally at the cost of a few leaked free space blocks.
+ */
 static xfs_failaddr_t
 xfs_agf_verify(
 	struct xfs_buf		*bp)
@@ -2962,7 +2988,6 @@ xfs_agf_verify(
 		return __this_address;
 
 	return NULL;
-
 }
 
 static void
-- 
2.40.1

[PATCH 2/3] xfs: validity check agbnos on the AGFL

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

If the agfl or the indexing in the AGF has been corrupted, getting a
block form the AGFL could return an invalid block number. If this
happens, bad things happen. Check the agbno we pull off the AGFL
and return -EFSCORRUPTED if we find somethign bad.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_alloc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index fd3293a8c659..643d17877832 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2780,6 +2780,9 @@ xfs_alloc_get_freelist(
 	 */
 	agfl_bno = xfs_buf_to_agfl_bno(agflbp);
 	bno = be32_to_cpu(agfl_bno[be32_to_cpu(agf->agf_flfirst)]);
+	if (XFS_IS_CORRUPT(tp->t_mountp, !xfs_verify_agbno(pag, bno)))
+		return -EFSCORRUPTED;
+
 	be32_add_cpu(&agf->agf_flfirst, 1);
 	xfs_trans_brelse(tp, agflbp);
 	if (be32_to_cpu(agf->agf_flfirst) == xfs_agfl_size(mp))
-- 
2.40.1

[PATCH 3/3] xfs: validate block number being freed before adding to xefi

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Bad things happen in defered extent freeing operations if it is
passed a bad block number in the xefi. This can come from a bogus
agno/agbno pair from deferred agfl freeing, or just a bad fsbno
being passed to __xfs_free_extent_later(). Either way, it's very
difficult to diagnose where a null perag oops in EFI creation
is coming from when the operation that queued the xefi has already
been completed and there's no longer any trace of it around....

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_ag.c         |  5 ++++-
 fs/xfs/libxfs/xfs_alloc.c      | 16 +++++++++++++---
 fs/xfs/libxfs/xfs_alloc.h      |  6 +++---
 fs/xfs/libxfs/xfs_bmap.c       | 10 ++++++++--
 fs/xfs/libxfs/xfs_bmap_btree.c |  7 +++++--
 fs/xfs/libxfs/xfs_ialloc.c     | 24 ++++++++++++++++--------
 fs/xfs/libxfs/xfs_refcount.c   | 13 ++++++++++---
 fs/xfs/xfs_reflink.c           |  4 +++-
 8 files changed, 62 insertions(+), 23 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index 9b373a0c7aaf..ee84835ebc66 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -984,7 +984,10 @@ xfs_ag_shrink_space(
 		if (err2 != -ENOSPC)
 			goto resv_err;
 
-		__xfs_free_extent_later(*tpp, args.fsbno, delta, NULL, true);
+		err2 = __xfs_free_extent_later(*tpp, args.fsbno, delta, NULL,
+				true);
+		if (err2)
+			goto resv_err;
 
 		/*
 		 * Roll the transaction before trying to re-init the per-ag
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 643d17877832..c20fe99405d8 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2431,7 +2431,7 @@ xfs_agfl_reset(
  * the real allocation can proceed. Deferring the free disconnects freeing up
  * the AGFL slot from freeing the block.
  */
-STATIC void
+static int
 xfs_defer_agfl_block(
 	struct xfs_trans		*tp,
 	xfs_agnumber_t			agno,
@@ -2450,17 +2450,21 @@ xfs_defer_agfl_block(
 	xefi->xefi_blockcount = 1;
 	xefi->xefi_owner = oinfo->oi_owner;
 
+	if (XFS_IS_CORRUPT(mp, !xfs_verify_fsbno(mp, xefi->xefi_startblock)))
+		return -EFSCORRUPTED;
+
 	trace_xfs_agfl_free_defer(mp, agno, 0, agbno, 1);
 
 	xfs_extent_free_get_group(mp, xefi);
 	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_AGFL_FREE, &xefi->xefi_list);
+	return 0;
 }
 
 /*
  * Add the extent to the list of extents to be free at transaction end.
  * The list is maintained sorted (by block number).
  */
-void
+int
 __xfs_free_extent_later(
 	struct xfs_trans		*tp,
 	xfs_fsblock_t			bno,
@@ -2487,6 +2491,9 @@ __xfs_free_extent_later(
 #endif
 	ASSERT(xfs_extfree_item_cache != NULL);
 
+	if (XFS_IS_CORRUPT(mp, !xfs_verify_fsbext(mp, bno, len)))
+		return -EFSCORRUPTED;
+
 	xefi = kmem_cache_zalloc(xfs_extfree_item_cache,
 			       GFP_KERNEL | __GFP_NOFAIL);
 	xefi->xefi_startblock = bno;
@@ -2510,6 +2517,7 @@ __xfs_free_extent_later(
 
 	xfs_extent_free_get_group(mp, xefi);
 	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_FREE, &xefi->xefi_list);
+	return 0;
 }
 
 #ifdef DEBUG
@@ -2670,7 +2678,9 @@ xfs_alloc_fix_freelist(
 			goto out_agbp_relse;
 
 		/* defer agfl frees */
-		xfs_defer_agfl_block(tp, args->agno, bno, &targs.oinfo);
+		error = xfs_defer_agfl_block(tp, args->agno, bno, &targs.oinfo);
+		if (error)
+			goto out_agbp_relse;
 	}
 
 	targs.tp = tp;
diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
index 5dbb25546d0b..85ac470be0da 100644
--- a/fs/xfs/libxfs/xfs_alloc.h
+++ b/fs/xfs/libxfs/xfs_alloc.h
@@ -230,7 +230,7 @@ xfs_buf_to_agfl_bno(
 	return bp->b_addr;
 }
 
-void __xfs_free_extent_later(struct xfs_trans *tp, xfs_fsblock_t bno,
+int __xfs_free_extent_later(struct xfs_trans *tp, xfs_fsblock_t bno,
 		xfs_filblks_t len, const struct xfs_owner_info *oinfo,
 		bool skip_discard);
 
@@ -254,14 +254,14 @@ void xfs_extent_free_get_group(struct xfs_mount *mp,
 #define XFS_EFI_ATTR_FORK	(1U << 1) /* freeing attr fork block */
 #define XFS_EFI_BMBT_BLOCK	(1U << 2) /* freeing bmap btree block */
 
-static inline void
+static inline int
 xfs_free_extent_later(
 	struct xfs_trans		*tp,
 	xfs_fsblock_t			bno,
 	xfs_filblks_t			len,
 	const struct xfs_owner_info	*oinfo)
 {
-	__xfs_free_extent_later(tp, bno, len, oinfo, false);
+	return __xfs_free_extent_later(tp, bno, len, oinfo, false);
 }
 
 
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index cd8870a16fd1..fef35696adb7 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -572,8 +572,12 @@ xfs_bmap_btree_to_extents(
 	cblock = XFS_BUF_TO_BLOCK(cbp);
 	if ((error = xfs_btree_check_block(cur, cblock, 0, cbp)))
 		return error;
+
 	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, whichfork);
-	xfs_free_extent_later(cur->bc_tp, cbno, 1, &oinfo);
+	error = xfs_free_extent_later(cur->bc_tp, cbno, 1, &oinfo);
+	if (error)
+		return error;
+
 	ip->i_nblocks--;
 	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L);
 	xfs_trans_binval(tp, cbp);
@@ -5230,10 +5234,12 @@ xfs_bmap_del_extent_real(
 		if (xfs_is_reflink_inode(ip) && whichfork == XFS_DATA_FORK) {
 			xfs_refcount_decrease_extent(tp, del);
 		} else {
-			__xfs_free_extent_later(tp, del->br_startblock,
+			error = __xfs_free_extent_later(tp, del->br_startblock,
 					del->br_blockcount, NULL,
 					(bflags & XFS_BMAPI_NODISCARD) ||
 					del->br_state == XFS_EXT_UNWRITTEN);
+			if (error)
+				goto done;
 		}
 	}
 
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
index 1b40e5f8b1ec..36564ae3084f 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.c
+++ b/fs/xfs/libxfs/xfs_bmap_btree.c
@@ -268,11 +268,14 @@ xfs_bmbt_free_block(
 	struct xfs_trans	*tp = cur->bc_tp;
 	xfs_fsblock_t		fsbno = XFS_DADDR_TO_FSB(mp, xfs_buf_daddr(bp));
 	struct xfs_owner_info	oinfo;
+	int			error;
 
 	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, cur->bc_ino.whichfork);
-	xfs_free_extent_later(cur->bc_tp, fsbno, 1, &oinfo);
+	error = xfs_free_extent_later(cur->bc_tp, fsbno, 1, &oinfo);
+	if (error)
+		return error;
+
 	ip->i_nblocks--;
-
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L);
 	return 0;
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index a16d5de16933..34600f94c2f4 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1834,7 +1834,7 @@ xfs_dialloc(
  * might be sparse and only free the regions that are allocated as part of the
  * chunk.
  */
-STATIC void
+static int
 xfs_difree_inode_chunk(
 	struct xfs_trans		*tp,
 	xfs_agnumber_t			agno,
@@ -1851,10 +1851,10 @@ xfs_difree_inode_chunk(
 
 	if (!xfs_inobt_issparse(rec->ir_holemask)) {
 		/* not sparse, calculate extent info directly */
-		xfs_free_extent_later(tp, XFS_AGB_TO_FSB(mp, agno, sagbno),
-				  M_IGEO(mp)->ialloc_blks,
-				  &XFS_RMAP_OINFO_INODES);
-		return;
+		return xfs_free_extent_later(tp,
+				XFS_AGB_TO_FSB(mp, agno, sagbno),
+				M_IGEO(mp)->ialloc_blks,
+				&XFS_RMAP_OINFO_INODES);
 	}
 
 	/* holemask is only 16-bits (fits in an unsigned long) */
@@ -1871,6 +1871,8 @@ xfs_difree_inode_chunk(
 						XFS_INOBT_HOLEMASK_BITS);
 	nextbit = startidx + 1;
 	while (startidx < XFS_INOBT_HOLEMASK_BITS) {
+		int error;
+
 		nextbit = find_next_zero_bit(holemask, XFS_INOBT_HOLEMASK_BITS,
 					     nextbit);
 		/*
@@ -1896,8 +1898,11 @@ xfs_difree_inode_chunk(
 
 		ASSERT(agbno % mp->m_sb.sb_spino_align == 0);
 		ASSERT(contigblk % mp->m_sb.sb_spino_align == 0);
-		xfs_free_extent_later(tp, XFS_AGB_TO_FSB(mp, agno, agbno),
-				  contigblk, &XFS_RMAP_OINFO_INODES);
+		error = xfs_free_extent_later(tp,
+				XFS_AGB_TO_FSB(mp, agno, agbno),
+				contigblk, &XFS_RMAP_OINFO_INODES);
+		if (error)
+			return error;
 
 		/* reset range to current bit and carry on... */
 		startidx = endidx = nextbit;
@@ -1905,6 +1910,7 @@ xfs_difree_inode_chunk(
 next:
 		nextbit++;
 	}
+	return 0;
 }
 
 STATIC int
@@ -2003,7 +2009,9 @@ xfs_difree_inobt(
 			goto error0;
 		}
 
-		xfs_difree_inode_chunk(tp, pag->pag_agno, &rec);
+		error = xfs_difree_inode_chunk(tp, pag->pag_agno, &rec);
+		if (error)
+			goto error0;
 	} else {
 		xic->deleted = false;
 
diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index c1c65774dcc2..b6e21433925c 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -1151,8 +1151,10 @@ xfs_refcount_adjust_extents(
 				fsbno = XFS_AGB_TO_FSB(cur->bc_mp,
 						cur->bc_ag.pag->pag_agno,
 						tmp.rc_startblock);
-				xfs_free_extent_later(cur->bc_tp, fsbno,
+				error = xfs_free_extent_later(cur->bc_tp, fsbno,
 						  tmp.rc_blockcount, NULL);
+				if (error)
+					goto out_error;
 			}
 
 			(*agbno) += tmp.rc_blockcount;
@@ -1210,8 +1212,10 @@ xfs_refcount_adjust_extents(
 			fsbno = XFS_AGB_TO_FSB(cur->bc_mp,
 					cur->bc_ag.pag->pag_agno,
 					ext.rc_startblock);
-			xfs_free_extent_later(cur->bc_tp, fsbno,
+			error = xfs_free_extent_later(cur->bc_tp, fsbno,
 					ext.rc_blockcount, NULL);
+			if (error)
+				goto out_error;
 		}
 
 skip:
@@ -1976,7 +1980,10 @@ xfs_refcount_recover_cow_leftovers(
 				rr->rr_rrec.rc_blockcount);
 
 		/* Free the block. */
-		xfs_free_extent_later(tp, fsb, rr->rr_rrec.rc_blockcount, NULL);
+		error = xfs_free_extent_later(tp, fsb,
+				rr->rr_rrec.rc_blockcount, NULL);
+		if (error)
+			goto out_trans;
 
 		error = xfs_trans_commit(tp);
 		if (error)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index f5dc46ce9803..abcc559f3c64 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -616,8 +616,10 @@ xfs_reflink_cancel_cow_blocks(
 			xfs_refcount_free_cow_extent(*tpp, del.br_startblock,
 					del.br_blockcount);
 
-			xfs_free_extent_later(*tpp, del.br_startblock,
+			error = xfs_free_extent_later(*tpp, del.br_startblock,
 					  del.br_blockcount, NULL);
+			if (error)
+				break;
 
 			/* Roll the transaction */
 			error = xfs_defer_finish(tpp);
-- 
2.40.1

Re: [PATCH 1/3] xfs: fix agf/agfl verification on v4 filesystems

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] xfs: validity check agbnos on the AGFL

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/3] xfs: validate block number being freed before adding to xefi

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/3] xfs: fix agf/agfl verification on v4 filesystems

[Darrick J. Wong]

On Mon, May 29, 2023 at 10:08:23AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When a v4 filesystem has fl_last - fl_first != fl_count, we do not
> not detect the corruption and allow the AGF to be used as it if was
> fully valid. On V5 filesystems, we reset the AGFL to empty in these
> cases and avoid the corruption at a small cost of leaked blocks.
> 
> If we don't catch the corruption on V4 filesystems, bad things
> happen later when an allocation attempts to trim the free list
> and either double-frees stale entries in the AGFl or tries to free
> NULLAGBNO entries.
> 
> Either way, this is bad. Prevent this from happening by using the
> AGFL_NEED_RESET logic for v4 filesysetms, too.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Seems logical,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_alloc.c | 59 ++++++++++++++++++++++++++++-----------
>  1 file changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 61eb65be17f3..fd3293a8c659 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -628,6 +628,25 @@ xfs_alloc_fixup_trees(
>  	return 0;
>  }
>  
> +/*
> + * We do not verify the AGFL contents against AGF-based index counters here,
> + * even though we may have access to the perag that contains shadow copies. We
> + * don't know if the AGF based counters have been checked, and if they have they
> + * still may be inconsistent because they haven't yet been reset on the first
> + * allocation after the AGF has been read in.
> + *
> + * This means we can only check that all agfl entries contain valid or null
> + * values because we can't reliably determine the active range to exclude
> + * NULLAGBNO as a valid value.
> + *
> + * However, we can't even do that for v4 format filesystems because there are
> + * old versions of mkfs out there that does not initialise the AGFL to known,
> + * verifiable values. HEnce we can't tell the difference between a AGFL block
> + * allocated by mkfs and a corrupted AGFL block here on v4 filesystems.
> + *
> + * As a result, we can only fully validate AGFL block numbers when we pull them
> + * from the freelist in xfs_alloc_get_freelist().
> + */
>  static xfs_failaddr_t
>  xfs_agfl_verify(
>  	struct xfs_buf	*bp)
> @@ -637,12 +656,6 @@ xfs_agfl_verify(
>  	__be32		*agfl_bno = xfs_buf_to_agfl_bno(bp);
>  	int		i;
>  
> -	/*
> -	 * There is no verification of non-crc AGFLs because mkfs does not
> -	 * initialise the AGFL to zero or NULL. Hence the only valid part of the
> -	 * AGFL is what the AGF says is active. We can't get to the AGF, so we
> -	 * can't verify just those entries are valid.
> -	 */
>  	if (!xfs_has_crc(mp))
>  		return NULL;
>  
> @@ -2321,12 +2334,16 @@ xfs_free_agfl_block(
>  }
>  
>  /*
> - * Check the agfl fields of the agf for inconsistency or corruption. The purpose
> - * is to detect an agfl header padding mismatch between current and early v5
> - * kernels. This problem manifests as a 1-slot size difference between the
> - * on-disk flcount and the active [first, last] range of a wrapped agfl. This
> - * may also catch variants of agfl count corruption unrelated to padding. Either
> - * way, we'll reset the agfl and warn the user.
> + * Check the agfl fields of the agf for inconsistency or corruption.
> + *
> + * The original purpose was to detect an agfl header padding mismatch between
> + * current and early v5 kernels. This problem manifests as a 1-slot size
> + * difference between the on-disk flcount and the active [first, last] range of
> + * a wrapped agfl.
> + *
> + * However, we need to use these same checks to catch agfl count corruptions
> + * unrelated to padding. This could occur on any v4 or v5 filesystem, so either
> + * way, we need to reset the agfl and warn the user.
>   *
>   * Return true if a reset is required before the agfl can be used, false
>   * otherwise.
> @@ -2342,10 +2359,6 @@ xfs_agfl_needs_reset(
>  	int			agfl_size = xfs_agfl_size(mp);
>  	int			active;
>  
> -	/* no agfl header on v4 supers */
> -	if (!xfs_has_crc(mp))
> -		return false;
> -
>  	/*
>  	 * The agf read verifier catches severe corruption of these fields.
>  	 * Repeat some sanity checks to cover a packed -> unpacked mismatch if
> @@ -2889,6 +2902,19 @@ xfs_alloc_put_freelist(
>  	return 0;
>  }
>  
> +/*
> + * Verify the AGF is consistent.
> + *
> + * We do not verify the AGFL indexes in the AGF are fully consistent here
> + * because of issues with variable on-disk structure sizes. Instead, we check
> + * the agfl indexes for consistency when we initialise the perag from the AGF
> + * information after a read completes.
> + *
> + * If the index is inconsistent, then we mark the perag as needing an AGFL
> + * reset. The first AGFL update performed then resets the AGFL indexes and
> + * refills the AGFL with known good free blocks, allowing the filesystem to
> + * continue operating normally at the cost of a few leaked free space blocks.
> + */
>  static xfs_failaddr_t
>  xfs_agf_verify(
>  	struct xfs_buf		*bp)
> @@ -2962,7 +2988,6 @@ xfs_agf_verify(
>  		return __this_address;
>  
>  	return NULL;
> -
>  }
>  
>  static void
> -- 
> 2.40.1
> 

Re: [PATCH 2/3] xfs: validity check agbnos on the AGFL

[Darrick J. Wong]

On Mon, May 29, 2023 at 10:08:24AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> If the agfl or the indexing in the AGF has been corrupted, getting a
> block form the AGFL could return an invalid block number. If this
> happens, bad things happen. Check the agbno we pull off the AGFL
> and return -EFSCORRUPTED if we find somethign bad.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

This looks like a good addition to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_alloc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index fd3293a8c659..643d17877832 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2780,6 +2780,9 @@ xfs_alloc_get_freelist(
>  	 */
>  	agfl_bno = xfs_buf_to_agfl_bno(agflbp);
>  	bno = be32_to_cpu(agfl_bno[be32_to_cpu(agf->agf_flfirst)]);
> +	if (XFS_IS_CORRUPT(tp->t_mountp, !xfs_verify_agbno(pag, bno)))
> +		return -EFSCORRUPTED;
> +
>  	be32_add_cpu(&agf->agf_flfirst, 1);
>  	xfs_trans_brelse(tp, agflbp);
>  	if (be32_to_cpu(agf->agf_flfirst) == xfs_agfl_size(mp))
> -- 
> 2.40.1
> 

Re: [PATCH 3/3] xfs: validate block number being freed before adding to xefi

[Darrick J. Wong]

On Mon, May 29, 2023 at 10:08:25AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Bad things happen in defered extent freeing operations if it is
> passed a bad block number in the xefi. This can come from a bogus
> agno/agbno pair from deferred agfl freeing, or just a bad fsbno
> being passed to __xfs_free_extent_later(). Either way, it's very
> difficult to diagnose where a null perag oops in EFI creation
> is coming from when the operation that queued the xefi has already
> been completed and there's no longer any trace of it around....
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_ag.c         |  5 ++++-
>  fs/xfs/libxfs/xfs_alloc.c      | 16 +++++++++++++---
>  fs/xfs/libxfs/xfs_alloc.h      |  6 +++---
>  fs/xfs/libxfs/xfs_bmap.c       | 10 ++++++++--
>  fs/xfs/libxfs/xfs_bmap_btree.c |  7 +++++--
>  fs/xfs/libxfs/xfs_ialloc.c     | 24 ++++++++++++++++--------
>  fs/xfs/libxfs/xfs_refcount.c   | 13 ++++++++++---
>  fs/xfs/xfs_reflink.c           |  4 +++-
>  8 files changed, 62 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 9b373a0c7aaf..ee84835ebc66 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -984,7 +984,10 @@ xfs_ag_shrink_space(
>  		if (err2 != -ENOSPC)
>  			goto resv_err;
>  
> -		__xfs_free_extent_later(*tpp, args.fsbno, delta, NULL, true);
> +		err2 = __xfs_free_extent_later(*tpp, args.fsbno, delta, NULL,
> +				true);
> +		if (err2)
> +			goto resv_err;
>  
>  		/*
>  		 * Roll the transaction before trying to re-init the per-ag
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 643d17877832..c20fe99405d8 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2431,7 +2431,7 @@ xfs_agfl_reset(
>   * the real allocation can proceed. Deferring the free disconnects freeing up
>   * the AGFL slot from freeing the block.
>   */
> -STATIC void
> +static int
>  xfs_defer_agfl_block(
>  	struct xfs_trans		*tp,
>  	xfs_agnumber_t			agno,
> @@ -2450,17 +2450,21 @@ xfs_defer_agfl_block(
>  	xefi->xefi_blockcount = 1;
>  	xefi->xefi_owner = oinfo->oi_owner;
>  
> +	if (XFS_IS_CORRUPT(mp, !xfs_verify_fsbno(mp, xefi->xefi_startblock)))
> +		return -EFSCORRUPTED;
> +
>  	trace_xfs_agfl_free_defer(mp, agno, 0, agbno, 1);
>  
>  	xfs_extent_free_get_group(mp, xefi);
>  	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_AGFL_FREE, &xefi->xefi_list);
> +	return 0;
>  }
>  
>  /*
>   * Add the extent to the list of extents to be free at transaction end.
>   * The list is maintained sorted (by block number).
>   */
> -void
> +int
>  __xfs_free_extent_later(
>  	struct xfs_trans		*tp,
>  	xfs_fsblock_t			bno,
> @@ -2487,6 +2491,9 @@ __xfs_free_extent_later(
>  #endif
>  	ASSERT(xfs_extfree_item_cache != NULL);
>  
> +	if (XFS_IS_CORRUPT(mp, !xfs_verify_fsbext(mp, bno, len)))
> +		return -EFSCORRUPTED;
> +
>  	xefi = kmem_cache_zalloc(xfs_extfree_item_cache,
>  			       GFP_KERNEL | __GFP_NOFAIL);
>  	xefi->xefi_startblock = bno;
> @@ -2510,6 +2517,7 @@ __xfs_free_extent_later(
>  
>  	xfs_extent_free_get_group(mp, xefi);
>  	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_FREE, &xefi->xefi_list);
> +	return 0;
>  }
>  
>  #ifdef DEBUG
> @@ -2670,7 +2678,9 @@ xfs_alloc_fix_freelist(
>  			goto out_agbp_relse;
>  
>  		/* defer agfl frees */
> -		xfs_defer_agfl_block(tp, args->agno, bno, &targs.oinfo);
> +		error = xfs_defer_agfl_block(tp, args->agno, bno, &targs.oinfo);
> +		if (error)
> +			goto out_agbp_relse;
>  	}
>  
>  	targs.tp = tp;
> diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
> index 5dbb25546d0b..85ac470be0da 100644
> --- a/fs/xfs/libxfs/xfs_alloc.h
> +++ b/fs/xfs/libxfs/xfs_alloc.h
> @@ -230,7 +230,7 @@ xfs_buf_to_agfl_bno(
>  	return bp->b_addr;
>  }
>  
> -void __xfs_free_extent_later(struct xfs_trans *tp, xfs_fsblock_t bno,
> +int __xfs_free_extent_later(struct xfs_trans *tp, xfs_fsblock_t bno,
>  		xfs_filblks_t len, const struct xfs_owner_info *oinfo,
>  		bool skip_discard);
>  
> @@ -254,14 +254,14 @@ void xfs_extent_free_get_group(struct xfs_mount *mp,
>  #define XFS_EFI_ATTR_FORK	(1U << 1) /* freeing attr fork block */
>  #define XFS_EFI_BMBT_BLOCK	(1U << 2) /* freeing bmap btree block */
>  
> -static inline void
> +static inline int
>  xfs_free_extent_later(
>  	struct xfs_trans		*tp,
>  	xfs_fsblock_t			bno,
>  	xfs_filblks_t			len,
>  	const struct xfs_owner_info	*oinfo)
>  {
> -	__xfs_free_extent_later(tp, bno, len, oinfo, false);
> +	return __xfs_free_extent_later(tp, bno, len, oinfo, false);
>  }
>  
>  
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index cd8870a16fd1..fef35696adb7 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -572,8 +572,12 @@ xfs_bmap_btree_to_extents(
>  	cblock = XFS_BUF_TO_BLOCK(cbp);
>  	if ((error = xfs_btree_check_block(cur, cblock, 0, cbp)))
>  		return error;
> +
>  	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, whichfork);
> -	xfs_free_extent_later(cur->bc_tp, cbno, 1, &oinfo);
> +	error = xfs_free_extent_later(cur->bc_tp, cbno, 1, &oinfo);
> +	if (error)
> +		return error;
> +
>  	ip->i_nblocks--;
>  	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L);
>  	xfs_trans_binval(tp, cbp);
> @@ -5230,10 +5234,12 @@ xfs_bmap_del_extent_real(
>  		if (xfs_is_reflink_inode(ip) && whichfork == XFS_DATA_FORK) {
>  			xfs_refcount_decrease_extent(tp, del);
>  		} else {
> -			__xfs_free_extent_later(tp, del->br_startblock,
> +			error = __xfs_free_extent_later(tp, del->br_startblock,
>  					del->br_blockcount, NULL,
>  					(bflags & XFS_BMAPI_NODISCARD) ||
>  					del->br_state == XFS_EXT_UNWRITTEN);
> +			if (error)
> +				goto done;
>  		}
>  	}
>  
> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
> index 1b40e5f8b1ec..36564ae3084f 100644
> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
> @@ -268,11 +268,14 @@ xfs_bmbt_free_block(
>  	struct xfs_trans	*tp = cur->bc_tp;
>  	xfs_fsblock_t		fsbno = XFS_DADDR_TO_FSB(mp, xfs_buf_daddr(bp));
>  	struct xfs_owner_info	oinfo;
> +	int			error;
>  
>  	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, cur->bc_ino.whichfork);
> -	xfs_free_extent_later(cur->bc_tp, fsbno, 1, &oinfo);
> +	error = xfs_free_extent_later(cur->bc_tp, fsbno, 1, &oinfo);
> +	if (error)
> +		return error;
> +
>  	ip->i_nblocks--;
> -
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L);
>  	return 0;
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index a16d5de16933..34600f94c2f4 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -1834,7 +1834,7 @@ xfs_dialloc(
>   * might be sparse and only free the regions that are allocated as part of the
>   * chunk.
>   */
> -STATIC void
> +static int
>  xfs_difree_inode_chunk(
>  	struct xfs_trans		*tp,
>  	xfs_agnumber_t			agno,
> @@ -1851,10 +1851,10 @@ xfs_difree_inode_chunk(
>  
>  	if (!xfs_inobt_issparse(rec->ir_holemask)) {
>  		/* not sparse, calculate extent info directly */
> -		xfs_free_extent_later(tp, XFS_AGB_TO_FSB(mp, agno, sagbno),
> -				  M_IGEO(mp)->ialloc_blks,
> -				  &XFS_RMAP_OINFO_INODES);
> -		return;
> +		return xfs_free_extent_later(tp,
> +				XFS_AGB_TO_FSB(mp, agno, sagbno),
> +				M_IGEO(mp)->ialloc_blks,
> +				&XFS_RMAP_OINFO_INODES);
>  	}
>  
>  	/* holemask is only 16-bits (fits in an unsigned long) */
> @@ -1871,6 +1871,8 @@ xfs_difree_inode_chunk(
>  						XFS_INOBT_HOLEMASK_BITS);
>  	nextbit = startidx + 1;
>  	while (startidx < XFS_INOBT_HOLEMASK_BITS) {
> +		int error;
> +
>  		nextbit = find_next_zero_bit(holemask, XFS_INOBT_HOLEMASK_BITS,
>  					     nextbit);
>  		/*
> @@ -1896,8 +1898,11 @@ xfs_difree_inode_chunk(
>  
>  		ASSERT(agbno % mp->m_sb.sb_spino_align == 0);
>  		ASSERT(contigblk % mp->m_sb.sb_spino_align == 0);
> -		xfs_free_extent_later(tp, XFS_AGB_TO_FSB(mp, agno, agbno),
> -				  contigblk, &XFS_RMAP_OINFO_INODES);
> +		error = xfs_free_extent_later(tp,
> +				XFS_AGB_TO_FSB(mp, agno, agbno),
> +				contigblk, &XFS_RMAP_OINFO_INODES);
> +		if (error)
> +			return error;
>  
>  		/* reset range to current bit and carry on... */
>  		startidx = endidx = nextbit;
> @@ -1905,6 +1910,7 @@ xfs_difree_inode_chunk(
>  next:
>  		nextbit++;
>  	}
> +	return 0;
>  }
>  
>  STATIC int
> @@ -2003,7 +2009,9 @@ xfs_difree_inobt(
>  			goto error0;
>  		}
>  
> -		xfs_difree_inode_chunk(tp, pag->pag_agno, &rec);
> +		error = xfs_difree_inode_chunk(tp, pag->pag_agno, &rec);
> +		if (error)
> +			goto error0;
>  	} else {
>  		xic->deleted = false;
>  
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index c1c65774dcc2..b6e21433925c 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -1151,8 +1151,10 @@ xfs_refcount_adjust_extents(
>  				fsbno = XFS_AGB_TO_FSB(cur->bc_mp,
>  						cur->bc_ag.pag->pag_agno,
>  						tmp.rc_startblock);
> -				xfs_free_extent_later(cur->bc_tp, fsbno,
> +				error = xfs_free_extent_later(cur->bc_tp, fsbno,
>  						  tmp.rc_blockcount, NULL);
> +				if (error)
> +					goto out_error;
>  			}
>  
>  			(*agbno) += tmp.rc_blockcount;
> @@ -1210,8 +1212,10 @@ xfs_refcount_adjust_extents(
>  			fsbno = XFS_AGB_TO_FSB(cur->bc_mp,
>  					cur->bc_ag.pag->pag_agno,
>  					ext.rc_startblock);
> -			xfs_free_extent_later(cur->bc_tp, fsbno,
> +			error = xfs_free_extent_later(cur->bc_tp, fsbno,
>  					ext.rc_blockcount, NULL);
> +			if (error)
> +				goto out_error;
>  		}
>  
>  skip:
> @@ -1976,7 +1980,10 @@ xfs_refcount_recover_cow_leftovers(
>  				rr->rr_rrec.rc_blockcount);
>  
>  		/* Free the block. */
> -		xfs_free_extent_later(tp, fsb, rr->rr_rrec.rc_blockcount, NULL);
> +		error = xfs_free_extent_later(tp, fsb,
> +				rr->rr_rrec.rc_blockcount, NULL);
> +		if (error)
> +			goto out_trans;
>  
>  		error = xfs_trans_commit(tp);
>  		if (error)
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index f5dc46ce9803..abcc559f3c64 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -616,8 +616,10 @@ xfs_reflink_cancel_cow_blocks(
>  			xfs_refcount_free_cow_extent(*tpp, del.br_startblock,
>  					del.br_blockcount);
>  
> -			xfs_free_extent_later(*tpp, del.br_startblock,
> +			error = xfs_free_extent_later(*tpp, del.br_startblock,
>  					  del.br_blockcount, NULL);
> +			if (error)
> +				break;
>  
>  			/* Roll the transaction */
>  			error = xfs_defer_finish(tpp);
> -- 
> 2.40.1
>