[PATCHSET 2/2] xfs: LLM-inspired iunlink repair bug fixes

[PATCHSET 2/2] xfs: LLM-inspired iunlink repair bug fixes

[Darrick J. Wong]

Hi all,

These bugfixes got started as part of a series of assorted bug fixes.
However, in writing a test case to exercise weird corruption scenarios
I discovered a few more fixes, so I decided to collect them all in a
targeted series.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

With a bit of luck, this should all go splendidly.
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=llm-fixes-iunlink
---
Commits in this patchset:
 * xfs: hoist per-bucket unlinked list check to helper
 * xfs: don't livelock in scrub on a circular unlinked list
 * xfs: don't walk off the end of a null sc->sa.agi_bp in AGI repair
 * xfs: load next_agino from the correct xfarray in xrep_iunlink_relink_prev
 * xfs: pass runtime errors from xrep_iunlink_mark_ondisk_rec up to callers
 * xfs: check xfarray iteration errors when committing unlinked inode lists
 * xfs: fix allocated inodes that show up in the unlinked list
 * xfs: fix another iunlink infinite loop bug in online fsck
 * xfs: set the prev pointer when reinserting an inode on the unlinked list
---
 fs/xfs/scrub/trace.h           |    2 +
 fs/xfs/scrub/agheader.c        |   94 ++++++++++++++++++++++++++--------
 fs/xfs/scrub/agheader_repair.c |  110 +++++++++++++++++++++++++++++++++++-----
 3 files changed, 170 insertions(+), 36 deletions(-)

[PATCH 1/9] xfs: hoist per-bucket unlinked list check to helper

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

In the next patch we're going to make this loop more exciting, so hoist
the code to a helper function to reduce clutter in the resulting code.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/scrub/agheader.c |   62 +++++++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 21 deletions(-)


diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index 62ed5eaf08fbc1..cecf034ef989c7 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -932,6 +932,42 @@ xchk_agi_xref(
 	/* scrub teardown will take care of sc->sa for us */
 }
 
+/*
+ * Walk the incore unlinked list for a particular AGI bucket to construct
+ * the unlinked inode bitmap for later reconstruction of the unlinked list.
+ * Returns 1 if we should keep checking, or 0 to stop checking.
+ */
+static int
+xchk_iunlink_bucket(
+	struct xfs_scrub		*sc,
+	unsigned int			bucket,
+	xfs_agino_t			agino)
+{
+	while (agino != NULLAGINO) {
+		struct xfs_inode	*ip;
+
+		if (agino % XFS_AGI_UNLINKED_BUCKETS != bucket) {
+			xchk_block_set_corrupt(sc, sc->sa.agi_bp);
+			return 0;
+		}
+
+		ip = xfs_iunlink_lookup(sc->sa.pag, agino);
+		if (!ip) {
+			xchk_block_set_corrupt(sc, sc->sa.agi_bp);
+			return 0;
+		}
+
+		if (!xfs_inode_on_unlinked_list(ip)) {
+			xchk_block_set_corrupt(sc, sc->sa.agi_bp);
+			return 0;
+		}
+
+		agino = ip->i_next_unlinked;
+	}
+
+	return 1;
+}
+
 /*
  * Check the unlinked buckets for links to bad inodes.  We hold the AGI, so
  * there cannot be any threads updating unlinked list pointers in this AG.
@@ -942,30 +978,14 @@ xchk_iunlink(
 	struct xfs_agi		*agi)
 {
 	unsigned int		i;
-	struct xfs_inode	*ip;
 
 	for (i = 0; i < XFS_AGI_UNLINKED_BUCKETS; i++) {
-		xfs_agino_t	agino = be32_to_cpu(agi->agi_unlinked[i]);
+		int		ret;
 
-		while (agino != NULLAGINO) {
-			if (agino % XFS_AGI_UNLINKED_BUCKETS != i) {
-				xchk_block_set_corrupt(sc, sc->sa.agi_bp);
-				return;
-			}
-
-			ip = xfs_iunlink_lookup(sc->sa.pag, agino);
-			if (!ip) {
-				xchk_block_set_corrupt(sc, sc->sa.agi_bp);
-				return;
-			}
-
-			if (!xfs_inode_on_unlinked_list(ip)) {
-				xchk_block_set_corrupt(sc, sc->sa.agi_bp);
-				return;
-			}
-
-			agino = ip->i_next_unlinked;
-		}
+		ret = xchk_iunlink_bucket(sc, i,
+				be32_to_cpu(agi->agi_unlinked[i]));
+		if (ret < 1)
+			return;
 	}
 }
 

[PATCH 2/9] xfs: don't livelock in scrub on a circular unlinked list

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

LOLLM points out that online fsck can livelock if an unlinked inode list
contains a loop.  Use a bitmap to detect cycles.

Cc: <stable@vger.kernel.org> # v4.15
Fixes: a12890aebb8959 ("xfs: scrub the AGI")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/agheader.c        |   44 +++++++++++++++++++++++++++++++++-------
 fs/xfs/scrub/agheader_repair.c |   17 ++++++++++++++-
 2 files changed, 51 insertions(+), 10 deletions(-)


diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index cecf034ef989c7..1fa66aa68e169f 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -18,6 +18,8 @@
 #include "xfs_inode.h"
 #include "scrub/scrub.h"
 #include "scrub/common.h"
+#include "scrub/bitmap.h"
+#include "scrub/agino_bitmap.h"
 
 int
 xchk_setup_agheader(
@@ -935,7 +937,8 @@ xchk_agi_xref(
 /*
  * Walk the incore unlinked list for a particular AGI bucket to construct
  * the unlinked inode bitmap for later reconstruction of the unlinked list.
- * Returns 1 if we should keep checking, or 0 to stop checking.
+ * Returns 1 if we should keep checking, 0 to stop checking, or a negative
+ * errno.
  */
 static int
 xchk_iunlink_bucket(
@@ -943,36 +946,57 @@ xchk_iunlink_bucket(
 	unsigned int			bucket,
 	xfs_agino_t			agino)
 {
+	struct xagino_bitmap		seen;
+	int				ret;
+
+	xagino_bitmap_init(&seen);
+
 	while (agino != NULLAGINO) {
 		struct xfs_inode	*ip;
+		unsigned int		len = 1;
 
 		if (agino % XFS_AGI_UNLINKED_BUCKETS != bucket) {
 			xchk_block_set_corrupt(sc, sc->sa.agi_bp);
-			return 0;
+			goto bad;
+		}
+
+		if (xagino_bitmap_test(&seen, agino, &len)) {
+			xchk_block_set_corrupt(sc, sc->sa.agi_bp);
+			goto bad;
 		}
 
 		ip = xfs_iunlink_lookup(sc->sa.pag, agino);
 		if (!ip) {
 			xchk_block_set_corrupt(sc, sc->sa.agi_bp);
-			return 0;
+			goto bad;
 		}
 
 		if (!xfs_inode_on_unlinked_list(ip)) {
 			xchk_block_set_corrupt(sc, sc->sa.agi_bp);
-			return 0;
+			goto bad;
 		}
 
+		ret = xagino_bitmap_set(&seen, agino, 1);
+		if (ret)
+			goto out_bitmap;
+
 		agino = ip->i_next_unlinked;
 	}
+	ret = 1;
 
-	return 1;
+out_bitmap:
+	xagino_bitmap_destroy(&seen);
+	return ret;
+bad:
+	ret = 0;
+	goto out_bitmap;
 }
 
 /*
  * Check the unlinked buckets for links to bad inodes.  We hold the AGI, so
  * there cannot be any threads updating unlinked list pointers in this AG.
  */
-STATIC void
+STATIC int
 xchk_iunlink(
 	struct xfs_scrub	*sc,
 	struct xfs_agi		*agi)
@@ -985,8 +1009,10 @@ xchk_iunlink(
 		ret = xchk_iunlink_bucket(sc, i,
 				be32_to_cpu(agi->agi_unlinked[i]));
 		if (ret < 1)
-			return;
+			return ret;
 	}
+
+	return 0;
 }
 
 /* Scrub the AGI. */
@@ -1073,7 +1099,9 @@ xchk_agi(
 	if (pag->pagi_freecount != be32_to_cpu(agi->agi_freecount))
 		xchk_block_set_corrupt(sc, sc->sa.agi_bp);
 
-	xchk_iunlink(sc, agi);
+	error = xchk_iunlink(sc, agi);
+	if (error)
+		goto out;
 
 	xchk_agi_xref(sc);
 out:
diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 2554494847ff1b..13074d5e319cc0 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -1080,18 +1080,22 @@ xrep_iunlink_walk_ondisk_bucket(
 	struct xrep_agi		*ragi,
 	unsigned int		bucket)
 {
+	struct xagino_bitmap	seen;
 	struct xfs_scrub	*sc = ragi->sc;
 	struct xfs_agi		*agi = sc->sa.agi_bp->b_addr;
 	xfs_agino_t		prev_agino = NULLAGINO;
 	xfs_agino_t		next_agino;
 	int			error = 0;
 
+	xagino_bitmap_init(&seen);
+
 	next_agino = be32_to_cpu(agi->agi_unlinked[bucket]);
 	while (next_agino != NULLAGINO) {
 		xfs_agino_t	agino = next_agino;
+		unsigned int	len = 1;
 
 		if (xchk_should_terminate(ragi->sc, &error))
-			return error;
+			goto out_bitmap;
 
 		trace_xrep_iunlink_walk_ondisk_bucket(sc->sa.pag, bucket,
 				prev_agino, agino);
@@ -1099,15 +1103,24 @@ xrep_iunlink_walk_ondisk_bucket(
 		if (bucket != agino % XFS_AGI_UNLINKED_BUCKETS)
 			break;
 
+		if (xagino_bitmap_test(&seen, agino, &len))
+			break;
+
 		next_agino = xrep_iunlink_next(sc, agino);
 		if (!next_agino)
 			next_agino = xrep_iunlink_reload_next(ragi, prev_agino,
 					agino);
 
+		error = xagino_bitmap_set(&seen, agino, 1);
+		if (error)
+			goto out_bitmap;
+
 		prev_agino = agino;
 	}
 
-	return 0;
+out_bitmap:
+	xagino_bitmap_destroy(&seen);
+	return error;
 }
 
 /* Decide if this is an unlinked inode in this AG. */

[PATCH 3/9] xfs: don't walk off the end of a null sc->sa.agi_bp in AGI repair

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

LOLLM noticed a longstanding bug where xrep_iunlink_walk_ondisk_bucket
tries to walk ragi->sc->sa.agi_bp to rebuild the unlinked inode lists.
Unfortunately, it's possible for agi_bp to be null if the buffer
verifier fails, so we have to use ragi->agi_bp (which skips verifier
checks) instead.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: ab97f4b1c03075 ("xfs: repair AGI unlinked inode bucket lists")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/agheader_repair.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 13074d5e319cc0..39468b8fa9863d 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -1082,7 +1082,7 @@ xrep_iunlink_walk_ondisk_bucket(
 {
 	struct xagino_bitmap	seen;
 	struct xfs_scrub	*sc = ragi->sc;
-	struct xfs_agi		*agi = sc->sa.agi_bp->b_addr;
+	struct xfs_agi		*agi = ragi->agi_bp->b_addr;
 	xfs_agino_t		prev_agino = NULLAGINO;
 	xfs_agino_t		next_agino;
 	int			error = 0;

[PATCH 4/9] xfs: load next_agino from the correct xfarray in xrep_iunlink_relink_prev

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

LOLLM notices that xrep_iunlink_relink_prev has the comment "set the
forward pointer..." but then loads the value from the xfarray that
stores pointers to the previous inode in the unlinked list.  That's
wrong, so fix the variable access.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: ab97f4b1c03075 ("xfs: repair AGI unlinked inode bucket lists")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/agheader_repair.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 39468b8fa9863d..5204eab09c6450 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -1619,7 +1619,7 @@ xrep_iunlink_relink_prev(
 		want_rele = true;
 
 		/* Set the forward pointer since this just came off disk. */
-		error = xfarray_load(ragi->iunlink_prev, agino, &next_agino);
+		error = xfarray_load(ragi->iunlink_next, agino, &next_agino);
 		if (error)
 			goto out_rele;
 

[PATCH 5/9] xfs: pass runtime errors from xrep_iunlink_mark_ondisk_rec up to callers

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

LOLLM points out that the only error that xrep_iunlink_mark_ondisk_rec
returns is ENOMEM, but we ignore that, and can end up writing a garbage
AGI based on incomplete information.  We shouldn't do that, though here
we must be screen out EFSCORRUPTED/EFSBASDCRC because we haven't
checked the inobt yet.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: ab97f4b1c03075 ("xfs: repair AGI unlinked inode bucket lists")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/agheader_repair.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 5204eab09c6450..7f19554843038d 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -1309,7 +1309,7 @@ xrep_iunlink_mark_ondisk_rec(
  * iunlink_bmp.   We haven't checked the inobt yet, so we don't error out if
  * the btree is corrupt.
  */
-STATIC void
+STATIC int
 xrep_iunlink_mark_ondisk(
 	struct xrep_agi		*ragi)
 {
@@ -1321,6 +1321,14 @@ xrep_iunlink_mark_ondisk(
 	cur = xfs_inobt_init_cursor(sc->sa.pag, sc->tp, agi_bp);
 	error = xfs_btree_query_all(cur, xrep_iunlink_mark_ondisk_rec, ragi);
 	xfs_btree_del_cursor(cur, error);
+
+	/*
+	 * Don't proceed if we couldn't set a bit in the bitmap.  All other
+	 * errors we ignore because we haven't actually checked the inobt yet.
+	 */
+	if (error == -ENOMEM)
+		return -ENOMEM;
+	return 0;
 }
 
 /*
@@ -1508,7 +1516,9 @@ xrep_iunlink_rebuild_buckets(
 	 * If there are ondisk inodes that are unlinked and are not been loaded
 	 * into cache, record them in iunlink_bmp.
 	 */
-	xrep_iunlink_mark_ondisk(ragi);
+	error = xrep_iunlink_mark_ondisk(ragi);
+	if (error)
+		return error;
 
 	/*
 	 * Walk each iunlink bucket to (re)construct as much of the incore list

[PATCH 6/9] xfs: check xfarray iteration errors when committing unlinked inode lists

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

LOLLM noticed that we neglect to check for xfarray_iter itself returning
errors when writing a new AGI.  Fix that.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: ab97f4b1c03075 ("xfs: repair AGI unlinked inode bucket lists")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/agheader_repair.c |    4 ++++
 1 file changed, 4 insertions(+)


diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 7f19554843038d..8514d2d7e3cea3 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -1675,6 +1675,8 @@ xrep_iunlink_commit(
 		if (error)
 			return error;
 	}
+	if (error < 0)
+		return error;
 
 	/* Fix all the back links */
 	idx = XFARRAY_CURSOR_INIT;
@@ -1683,6 +1685,8 @@ xrep_iunlink_commit(
 		if (error)
 			return error;
 	}
+	if (error < 0)
+		return error;
 
 	/* Copy the staged iunlink buckets to the new AGI. */
 	for (i = 0; i < XFS_AGI_UNLINKED_BUCKETS; i++) {

[PATCH 7/9] xfs: fix allocated inodes that show up in the unlinked list

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If an allocated inode shows up in the unlinked list, we need to get it
completely off the list.  Set the corrected next/prev pointers such that
the inode will not look like it should be on an unlinked list at all.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: ab97f4b1c03075 ("xfs: repair AGI unlinked inode bucket lists")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/scrub/trace.h           |    1 +
 fs/xfs/scrub/agheader_repair.c |   57 +++++++++++++++++++++++++++++++++++-----
 2 files changed, 51 insertions(+), 7 deletions(-)


diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
index d5d39d82749e5c..00fbe1b9c2354f 100644
--- a/fs/xfs/scrub/trace.h
+++ b/fs/xfs/scrub/trace.h
@@ -3542,6 +3542,7 @@ DEFINE_REPAIR_IUNLINK_RESOLVE_EVENT(xrep_iunlink_resolve_uncached);
 DEFINE_REPAIR_IUNLINK_RESOLVE_EVENT(xrep_iunlink_resolve_wronglist);
 DEFINE_REPAIR_IUNLINK_RESOLVE_EVENT(xrep_iunlink_resolve_nolist);
 DEFINE_REPAIR_IUNLINK_RESOLVE_EVENT(xrep_iunlink_resolve_ok);
+DEFINE_REPAIR_IUNLINK_RESOLVE_EVENT(xrep_iunlink_resolve_allocated);
 
 TRACE_EVENT(xrep_iunlink_relink_next,
 	TP_PROTO(struct xfs_inode *ip, xfs_agino_t next_agino),
diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 8514d2d7e3cea3..edd406a97b7ce9 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -979,6 +979,13 @@ xrep_agi_calc_from_btrees(
 	return error;
 }
 
+/*
+ * Magic value that means "not unlinked" because xfarrays don't support storing
+ * totally zeroed elements.  There can't be a cluster that starts in daddr 0 so
+ * there can't be an inode #1 either.
+ */
+#define LINKED_AGINO	(0x1)
+
 /*
  * Record a forwards unlinked chain pointer from agino -> next_agino in our
  * staging information.
@@ -1362,6 +1369,29 @@ xrep_iunlink_resolve_bucket(
 			break;
 		}
 
+		if (VFS_I(ip)->i_nlink != 0) {
+			/*
+			 * Inode is allocated!  Blow out both unlinked list
+			 * pointers, advance the list, and pretend we didn't
+			 * see this inode.
+			 */
+			trace_xrep_iunlink_resolve_allocated(sc->sa.pag,
+					bucket, prev_agino, next_agino);
+
+			error = xrep_iunlink_store_next(ragi, next_agino,
+					NULLAGINO);
+			if (error)
+				return error;
+
+			error = xrep_iunlink_store_prev(ragi, next_agino,
+					LINKED_AGINO);
+			if (error)
+				return error;
+
+			next_agino = ip->i_next_unlinked;
+			continue;
+		}
+
 		if (next_agino % XFS_AGI_UNLINKED_BUCKETS != bucket) {
 			/*
 			 * Inode is in the wrong bucket.  Advance the list,
@@ -1540,6 +1570,24 @@ xrep_iunlink_rebuild_buckets(
 			xrep_iunlink_add_lost_inodes, ragi);
 }
 
+static inline void
+set_inode_prev_unlinked(
+	struct xfs_inode	*ip,
+	xfs_agino_t		prev_agino)
+{
+	/*
+	 * Magic value that means "not unlinked" because xfarrays don't support
+	 * storing totally zeroed elements.
+	 */
+	if (prev_agino == LINKED_AGINO)
+		prev_agino = 0;
+
+	if (ip->i_prev_unlinked != prev_agino) {
+		trace_xrep_iunlink_relink_prev(ip, prev_agino);
+		ip->i_prev_unlinked = prev_agino;
+	}
+}
+
 /* Update i_next_iunlinked for the inode @agino. */
 STATIC int
 xrep_iunlink_relink_next(
@@ -1573,8 +1621,7 @@ xrep_iunlink_relink_next(
 		if (error)
 			goto out_rele;
 
-		trace_xrep_iunlink_relink_prev(ip, prev_agino);
-		ip->i_prev_unlinked = prev_agino;
+		set_inode_prev_unlinked(ip, prev_agino);
 	}
 
 	/* Update the forward pointer. */
@@ -1641,11 +1688,7 @@ xrep_iunlink_relink_prev(
 		ip->i_next_unlinked = next_agino;
 	}
 
-	/* Update the backward pointer. */
-	if (ip->i_prev_unlinked != prev_agino) {
-		trace_xrep_iunlink_relink_prev(ip, prev_agino);
-		ip->i_prev_unlinked = prev_agino;
-	}
+	set_inode_prev_unlinked(ip, prev_agino);
 
 out_rele:
 	/*

[PATCH 8/9] xfs: fix another iunlink infinite loop bug in online fsck

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

xrep_iunlink_resolve_bucket is supposed to reconstruct as much of the
incore prev and next unlinked list pointers based on what it finds on
disk and in memory before we move on to relinking the truly lost inodes
back into the unlinked list.  However, it's still vulnerable to infinite
loops that come in via the next_unlinked pointers.

Fix this problem by checking iunlink_bmp, which at that point in
execution represents truly unlinked inodes that haven't yet been
processed.  If a bit is already unset, either this is a loop or the
inode has nonzero link count.  We'll deal with the second case in a
subsequent patch.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: ab97f4b1c03075 ("xfs: repair AGI unlinked inode bucket lists")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/scrub/trace.h           |    1 +
 fs/xfs/scrub/agheader_repair.c |   10 ++++++++++
 2 files changed, 11 insertions(+)


diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
index 00fbe1b9c2354f..14aa0ec1f09e4a 100644
--- a/fs/xfs/scrub/trace.h
+++ b/fs/xfs/scrub/trace.h
@@ -3538,6 +3538,7 @@ DEFINE_EVENT(xrep_iunlink_resolve_class, name, \
 	TP_PROTO(const struct xfs_perag *pag, unsigned int bucket, \
 		 xfs_agino_t prev_agino, xfs_agino_t next_agino), \
 	TP_ARGS(pag, bucket, prev_agino, next_agino))
+DEFINE_REPAIR_IUNLINK_RESOLVE_EVENT(xrep_iunlink_resolve_infinite_loop);
 DEFINE_REPAIR_IUNLINK_RESOLVE_EVENT(xrep_iunlink_resolve_uncached);
 DEFINE_REPAIR_IUNLINK_RESOLVE_EVENT(xrep_iunlink_resolve_wronglist);
 DEFINE_REPAIR_IUNLINK_RESOLVE_EVENT(xrep_iunlink_resolve_nolist);
diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index edd406a97b7ce9..da2265678d17f5 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -1355,6 +1355,8 @@ xrep_iunlink_resolve_bucket(
 	int			error = 0;
 
 	while (next_agino != NULLAGINO) {
+		unsigned int len = 1;
+
 		if (xchk_should_terminate(ragi->sc, &error))
 			return error;
 
@@ -1417,6 +1419,14 @@ xrep_iunlink_resolve_bucket(
 			continue;
 		}
 
+		/* Inode already seen?  We're stuck in a loop */
+		if (!xagino_bitmap_test(&ragi->iunlink_bmp, next_agino, &len)) {
+			trace_xrep_iunlink_resolve_infinite_loop(sc->sa.pag,
+					bucket, prev_agino, next_agino);
+			next_agino = NULLAGINO;
+			break;
+		}
+
 		trace_xrep_iunlink_resolve_ok(sc->sa.pag, bucket, prev_agino,
 				next_agino);
 

[PATCH 9/9] xfs: set the prev pointer when reinserting an inode on the unlinked list

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If we find a rogue free inode and decide to reinsert it into the
unlinked list, we need to set the prev pointer to NULLAGINO so that the
incore list gets updated.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: ab97f4b1c03075 ("xfs: repair AGI unlinked inode bucket lists")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/scrub/agheader_repair.c |    4 ++++
 1 file changed, 4 insertions(+)


diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index da2265678d17f5..f75bb8f27b1215 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -1489,6 +1489,10 @@ xrep_iunlink_add_to_bucket(
 	if (error)
 		return error;
 
+	error = xrep_iunlink_store_prev(ragi, agino, NULLAGINO);
+	if (error)
+		return error;
+
 	/* Remember the head inode's previous pointer. */
 	if (current_head != NULLAGINO) {
 		error = xrep_iunlink_store_prev(ragi, current_head, agino);

[RFC PATCH] xfs: test unlinked inode list checking and repair with loops

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Simple test of various weird ways we can screw up unlinked inode list
reconstruction.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 tests/xfs/1907     |  212 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/1907.out |   17 ++++
 2 files changed, 229 insertions(+)
 create mode 100755 tests/xfs/1907
 create mode 100644 tests/xfs/1907.out

diff --git a/tests/xfs/1907 b/tests/xfs/1907
new file mode 100755
index 00000000000000..3b8634f3f16b3c
--- /dev/null
+++ b/tests/xfs/1907
@@ -0,0 +1,212 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2026 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 1907
+#
+# Test using online fsck code to fix unlinked inodes on a clean filesystem that
+# never got cleaned up.
+#
+. ./common/preamble
+_begin_fstest auto quick unlink
+
+. ./common/filter
+. ./common/fuzzy
+. ./common/quota
+
+_require_command "$TIMEOUT_PROG" timeout
+_require_xfs_db_command iunlink
+_require_scratch_nocheck	# we'll run repair ourselves
+
+_scratch_mount
+$XFS_IO_PROG -x -c 'repair -R agi 0' $SCRATCH_MNT 2>&1 | \
+	grep -q 'Operation not supported' && \
+	_notrun "cannot repair agi"
+_scratch_unmount
+
+# From the AGI definition
+XFS_AGI_UNLINKED_BUCKETS=64
+NULLAGINO="0xffffffff"
+
+# Try to make each iunlink bucket have this many inodes in it.
+IUNLINK_BUCKETLEN=5
+
+# Disable quota since quotacheck will break this test
+orig_mount_options="$MOUNT_OPTIONS"
+_qmount_option 'noquota'
+
+format_scratch() {
+	_scratch_mkfs -d agcount=1 | _filter_mkfs 2> "${tmp}.mkfs" >> $seqres.full
+	source "${tmp}.mkfs"
+	test "${agcount}" -eq 1 || _notrun "test requires 1 AG for error injection"
+
+	local nr_iunlinks="$((IUNLINK_BUCKETLEN * XFS_AGI_UNLINKED_BUCKETS))"
+	readarray -t BADINODES < <(_scratch_xfs_db -x -c "iunlink -n $nr_iunlinks" | awk '{print $4}')
+
+	ROOTINO="$(_scratch_xfs_db -c 'sb' -c 'print rootino' | cut -d ' ' -f 3)"
+
+	BUCKET_23=()
+	BUCKET_24=()
+	for badinode in "${BADINODES[@]}"; do
+		(( (badinode % 64) == 23 )) && BUCKET_23+=("${badinode}")
+		(( (badinode % 64) == 24 )) && BUCKET_24+=("${badinode}")
+	done
+	test "${#BUCKET_23[@]}" -ge "$IUNLINK_BUCKETLEN" || \
+		echo "bucket 23 should have at least $IUNLINK_BUCKETLEN inodes, has ${#BUCKET_23[@]}"
+	test "${#BUCKET_24[@]}" -ge "$IUNLINK_BUCKETLEN" || \
+		echo "bucket 24 should have at least $IUNLINK_BUCKETLEN inodes, has ${#BUCKET_23[@]}"
+
+	# Log what we think the bucket 23 unlinked list will look like
+	printf "ROOTINO 0x%x\n" "$ROOTINO" >> $seqres.full
+	for badinode in "${BUCKET_23[@]}"; do
+		printf "0x%x <- " "${badinode}" >> $seqres.full
+	done
+	echo " AGI.iunlinked[23]" >> $seqres.full
+	for badinode in "${BUCKET_24[@]}"; do
+		printf "0x%x <- " "${badinode}" >> $seqres.full
+	done
+	echo " AGI.iunlinked[24]" >> $seqres.full
+
+	# Log what the actual bucket 23 unlinked list ended up looking like
+	local subcommands=()
+	for badinode in "${BUCKET_23[@]}"; do
+		subcommands+=(-c "inode ${badinode}" -c "print next_unlinked")
+	done
+	_scratch_xfs_db -x "${subcommands[@]}" >> $seqres.full
+}
+
+__repair_check_scratch() {
+	_scratch_xfs_repair -o force_geometry -n 2>&1 | \
+		tee -a $seqres.full | \
+		grep -E '(disconnected inode.*would move|next_unlinked in inode|unlinked bucket.*is.*in ag)'
+	return "${PIPESTATUS[0]}"
+}
+
+exercise_scratch() {
+	# Create a bunch of files...
+	declare -A inums
+	for ((i = 0; i < (XFS_AGI_UNLINKED_BUCKETS * 2); i++)); do
+		touch "${SCRATCH_MNT}/${i}" || break
+		inums["${i}"]="$(stat -c %i "${SCRATCH_MNT}/${i}")"
+	done
+
+	# ...then delete them to exercise the unlinked buckets
+	for ((i = 0; i < (XFS_AGI_UNLINKED_BUCKETS * 2); i++)); do
+		if ! rm -f "${SCRATCH_MNT}/${i}"; then
+			echo "rm failed on inum ${inums[$i]}"
+			break
+		fi
+	done
+}
+
+test_body() {
+	_scratch_mount
+	timeout 30s $XFS_IO_PROG -x -c 'scrub agi 0' -c 'repair agi 0' $SCRATCH_MNT
+	$XFS_IO_PROG -x -c 'repair fscounters' $SCRATCH_MNT >> $seqres.full
+	exercise_scratch
+	_scratch_unmount
+	final_check_scratch
+}
+
+# Offline repair should not find anything
+final_check_scratch() {
+	__repair_check_scratch
+	res=$?
+	if [ $res -eq 2 ]; then
+		echo "scratch fs went offline?"
+		_scratch_mount
+		_scratch_unmount
+		__repair_check_scratch
+	fi
+	test "$res" -ne 0 && echo "repair returned $res?"
+}
+
+echo "+ Part 1: Fix a correct unlinked list" | tee -a $seqres.full
+_kernlog "part 1"
+format_scratch
+test_body
+
+echo "+ Part 2: Fix a loop between 1 and 3" | tee -a $seqres.full
+_kernlog "part 2"
+format_scratch
+# BUCKET_23 is in reverse order of the ondisk list, so we make
+# inode 1 point back to inode 3.
+_scratch_xfs_db -x \
+	-c "inode ${BUCKET_23[1]}" \
+	-c "print next_unlinked" \
+	-c "write -d next_unlinked ${BUCKET_23[3]}" \
+	>> $seqres.full
+test_body
+
+echo "+ Part 3: Fix a truncated bucket" | tee -a $seqres.full
+_kernlog "part 3"
+format_scratch
+_scratch_xfs_db -x \
+	-c "agi 0" \
+	-c "print" \
+	-c "write -d unlinked[23] ${NULLAGINO}" \
+	>> $seqres.full
+test_body
+
+echo "+ Part 4: Fix a loop at the end" | tee -a $seqres.full
+_kernlog "part 4"
+format_scratch
+# BUCKET_23 is in reverse order of the ondisk list, so we make
+# inode 0 point back to inode 0.
+_scratch_xfs_db -x \
+	-c "inode ${BUCKET_23[0]}" \
+	-c "print next_unlinked" \
+	-c "write -d next_unlinked ${BUCKET_23[0]}" \
+	>> $seqres.full
+test_body
+
+echo "+ Part 5: Fix an inode in the wrong bucket" | tee -a $seqres.full
+_kernlog "part 5"
+format_scratch
+_scratch_xfs_db -x \
+	-c "inode ${BUCKET_23[0]}" \
+	-c "print next_unlinked" \
+	-c "write -d next_unlinked ${BUCKET_24[1]}" \
+	-c "inode ${BUCKET_24[2]}" \
+	-c "print next_unlinked" \
+	-c "write -d next_unlinked ${NULLAGINO}" \
+	>> $seqres.full
+test_body
+
+echo "+ Part 6: Fix an inode that isn't free and truncates list" | tee -a $seqres.full
+_kernlog "part 6"
+format_scratch
+_scratch_xfs_db -x \
+	-c "inode ${BUCKET_23[2]}" \
+	-c "print next_unlinked" \
+	-c "write -d next_unlinked ${ROOTINO}" \
+	>> $seqres.full
+test_body
+
+echo "+ Part 7: Fix an inode that isn't free" | tee -a $seqres.full
+_kernlog "part 7"
+format_scratch
+_scratch_xfs_db -x \
+	-c "inode ${BUCKET_23[2]}" \
+	-c "print next_unlinked" \
+	-c "write -d next_unlinked ${ROOTINO}" \
+	-c "inode ${ROOTINO}" \
+	-c "print next_unlinked" \
+	-c "write -d next_unlinked ${BUCKET_23[1]}" \
+	>> $seqres.full
+test_body
+
+echo "+ Part 8: Fix an totally unallocated inode" | tee -a $seqres.full
+_kernlog "part 8"
+format_scratch
+target=$(( BADINODES[-1] + 256 ))
+_scratch_xfs_db -x \
+	-c "inode ${BUCKET_23[2]}" \
+	-c "print next_unlinked" \
+	-c "write -d next_unlinked $target" \
+	>> $seqres.full
+test_body
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/1907.out b/tests/xfs/1907.out
new file mode 100644
index 00000000000000..83746ad602b21d
--- /dev/null
+++ b/tests/xfs/1907.out
@@ -0,0 +1,17 @@
+QA output created by 1907
++ Part 1: Fix a correct unlinked list
+Corruption detected.
++ Part 2: Fix a loop between 1 and 3
+Corruption detected.
++ Part 3: Fix a truncated bucket
+Corruption detected.
++ Part 4: Fix a loop at the end
+Corruption detected.
++ Part 5: Fix an inode in the wrong bucket
+Corruption detected.
++ Part 6: Fix an inode that isn't free and truncates list
+Corruption detected.
++ Part 7: Fix an inode that isn't free
+Corruption detected.
++ Part 8: Fix an totally unallocated inode
+Corruption detected.

Re: [PATCH 1/9] xfs: hoist per-bucket unlinked list check to helper

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 4/9] xfs: load next_agino from the correct xfarray in xrep_iunlink_relink_prev

[Christoph Hellwig]

On Mon, Jul 20, 2026 at 08:25:51PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM notices that xrep_iunlink_relink_prev has the comment "set the
> forward pointer..." but then loads the value from the xfarray that
> stores pointers to the previous inode in the unlinked list.  That's
> wrong, so fix the variable access.

Looks good:

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

Re: [PATCH 8/9] xfs: fix another iunlink infinite loop bug in online fsck

[Christoph Hellwig]

Looks good:

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

Re: [RFC PATCH] xfs: test unlinked inode list checking and repair with loops

[Christoph Hellwig]

On Mon, Jul 20, 2026 at 08:41:05PM -0700, Darrick J. Wong wrote:
> +# From the AGI definition
> +XFS_AGI_UNLINKED_BUCKETS=64
> +NULLAGINO="0xffffffff"

Oh fun, on-disk formats in bash :)

The tests themselves looks good to me, but by now everyone should
know I am not to be trusted on advanced bash code..

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

Re: [PATCH 9/9] xfs: set the prev pointer when reinserting an inode on the unlinked list

[Christoph Hellwig]

Looks good:

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