[PATCHSET] xfs: LLM-inspired bug fixes, part 4

[PATCHSET] xfs: LLM-inspired bug fixes, part 4

[Darrick J. Wong]

Hi all,

Here's a fourth batch of xfs fixes resulting from a LLaMma.  Mwa mwa
mwa...

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-4
---
Commits in this patchset:
 * xfs: only check mergeability of bnobt records
 * xfs: don't double-lock when deleting a self-referential directory
 * xfs: don't return EFSCORRUPTED when scrubbing corrupt parent pointers
 * xfs: don't zap the attr fork on repair when there are queued pptr updates
 * xfs: nlink scrub must take IOLOCK before determining ILOCK state
 * xfs: avoid UAF on sc->tempip in xrep_tempfile_create
---
 fs/xfs/scrub/alloc.c          |    5 +++--
 fs/xfs/scrub/attr_repair.c    |    3 ++-
 fs/xfs/scrub/dirtree_repair.c |   22 ++++++++++++++++++----
 fs/xfs/scrub/nlinks.c         |   13 +++++++------
 fs/xfs/scrub/parent.c         |    2 +-
 fs/xfs/scrub/tempfile.c       |    1 +
 6 files changed, 32 insertions(+), 14 deletions(-)

[PATCH 1/6] xfs: only check mergeability of bnobt records

[Darrick J. Wong]

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

In the cntbt (free space by block count) btree, records are not supposed
to be in startblock order.  Hence the mergeability check is pointless.
Remove it, since it does nothing, as LOLLM points out.

Cc: <stable@vger.kernel.org> # v6.4
Fixes: d5784ae82778d9 ("xfs: flag free space btree records that could be merged")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/alloc.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/scrub/alloc.c b/fs/xfs/scrub/alloc.c
index 48edaa2cb1e079..c666be69f16481 100644
--- a/fs/xfs/scrub/alloc.c
+++ b/fs/xfs/scrub/alloc.c
@@ -136,7 +136,7 @@ xchk_allocbt_rec(
 	const union xfs_btree_rec	*rec)
 {
 	struct xfs_alloc_rec_incore	irec;
-	struct xchk_alloc	*ca = bs->private;
+	struct xchk_alloc		*ca = bs->private;
 
 	xfs_alloc_btrec_to_irec(rec, &irec);
 	if (xfs_alloc_check_irec(to_perag(bs->cur->bc_group), &irec) != NULL) {
@@ -144,7 +144,8 @@ xchk_allocbt_rec(
 		return 0;
 	}
 
-	xchk_allocbt_mergeable(bs, ca, &irec);
+	if (bs->sc->sm->sm_type == XFS_SCRUB_TYPE_BNOBT)
+		xchk_allocbt_mergeable(bs, ca, &irec);
 	xchk_allocbt_xref(bs->sc, &irec);
 
 	return 0;

[PATCH 2/6] xfs: don't double-lock when deleting a self-referential directory

[Darrick J. Wong]

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

LOLLM notices that the dirtree scrubber can detect a directory that
refers to itself.  In this case, it's not correct for the directory tree
repair code to try to iolock/ilock both sc->ip and dp, because they're
the same inode.  Fix this by detecting that corner case and handling it
appropriately.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: 3f31406aef493b ("xfs: fix corruptions in the directory tree")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/dirtree_repair.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/scrub/dirtree_repair.c b/fs/xfs/scrub/dirtree_repair.c
index 1c0d7ea4a5be6a..bbf6acf6fd400c 100644
--- a/fs/xfs/scrub/dirtree_repair.c
+++ b/fs/xfs/scrub/dirtree_repair.c
@@ -349,6 +349,8 @@ xrep_dirtree_unlink_iolock(
 
 	ASSERT(sc->ilock_flags & XFS_IOLOCK_EXCL);
 
+	if (sc->ip == dp)
+		return 0;
 	if (xfs_ilock_nowait(dp, XFS_IOLOCK_EXCL))
 		return 0;
 
@@ -400,8 +402,18 @@ xrep_dirtree_unlink(
 	 * directory code can handle a reservationless update.
 	 */
 	resblks = xfs_remove_space_res(mp, step->name_len);
-	error = xfs_trans_alloc_dir(dp, &M_RES(mp)->tr_remove, sc->ip,
-			&resblks, &sc->tp, &dontcare);
+	if (sc->ip == dp) {
+again:
+		error = xfs_trans_alloc_inode(dp, &M_RES(mp)->tr_remove,
+				resblks, 0, false, &sc->tp);
+		if ((error == -ENOSPC || error == -EDQUOT) && resblks > 0) {
+			resblks = 0;
+			goto again;
+		}
+	} else {
+		error = xfs_trans_alloc_dir(dp, &M_RES(mp)->tr_remove, sc->ip,
+				&resblks, &sc->tp, &dontcare);
+	}
 	if (error)
 		goto out_iolock;
 
@@ -489,9 +501,11 @@ xrep_dirtree_unlink(
 	xchk_trans_cancel(sc);
 out_ilock:
 	xfs_iunlock(sc->ip, XFS_ILOCK_EXCL);
-	xfs_iunlock(dp, XFS_ILOCK_EXCL);
+	if (dp != sc->ip)
+		xfs_iunlock(dp, XFS_ILOCK_EXCL);
 out_iolock:
-	xfs_iunlock(dp, XFS_IOLOCK_EXCL);
+	if (dp != sc->ip)
+		xfs_iunlock(dp, XFS_IOLOCK_EXCL);
 	return error;
 }
 

[PATCH 3/6] xfs: don't return EFSCORRUPTED when scrubbing corrupt parent pointers

[Darrick J. Wong]

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

LOLLM noticed that scrub sets the CORRUPT flag when xfs_parent_from_attr
thinks it's been given a corrupt parent pointer.  This eliminates the
potential to repair the filesystem because that error code is bubbled up
the call stack.  Fix this by collapsing them all to ECANCELED in
xchk_parent_pptr, which doesn't have that trait.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: 0d29a20fbdba89 ("xfs: scrub parent pointers")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/parent.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c
index a8c4807e1d9464..99b60773b7158d 100644
--- a/fs/xfs/scrub/parent.c
+++ b/fs/xfs/scrub/parent.c
@@ -485,7 +485,7 @@ xchk_parent_scan_attr(
 			valuelen, &parent_ino, NULL);
 	if (error) {
 		xchk_fblock_set_corrupt(sc, XFS_ATTR_FORK, 0);
-		return error;
+		return -ECANCELED;
 	}
 
 	/* No self-referential parent pointers. */

[PATCH 4/6] xfs: don't zap the attr fork on repair when there are queued pptr updates

[Darrick J. Wong]

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

LOLLM noticed that xrep_xattr_rebuild_tree doesn't check for queued
parent pointer updates when it decides that it's going to zap the attr
fork.  This is obviously incorrect, so fix that.  We hold the IOLOCK and
the ILOCK of sc->ip at that point in time, so we can't race with any
/new/ operations.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: e5d7ce0364d8ee ("xfs: replay unlocked parent pointer updates that accrue during xattr repair")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/attr_repair.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/attr_repair.c b/fs/xfs/scrub/attr_repair.c
index be627ab655ad16..fa26328bdeac91 100644
--- a/fs/xfs/scrub/attr_repair.c
+++ b/fs/xfs/scrub/attr_repair.c
@@ -1427,7 +1427,8 @@ xrep_xattr_rebuild_tree(
 	 * If we didn't find any attributes to salvage, repair the file by
 	 * zapping its attr fork.
 	 */
-	if (rx->attrs_found == 0) {
+	if (rx->attrs_found == 0 && (!xfs_has_parent(sc->mp) ||
+				     xfarray_length(rx->pptr_recs) == 0)) {
 		xfs_trans_ijoin(sc->tp, sc->ip, 0);
 		error = xrep_xattr_reset_fork(sc);
 		if (error)

[PATCH 5/6] xfs: nlink scrub must take IOLOCK before determining ILOCK state

[Darrick J. Wong]

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

In xchk_nlinks_ilock_dir, take the IOLOCK before accessing internal
inode state to figure out if we need to take ILOCK shared or exclusive.
That way we can't race with directory updates.  LOLLM pointed out that
the code was initially correct w.r.t. the IOLOCK, but then I broke it.

Cc: <stable@vger.kernel.org> # v6.18
Fixes: f477af0cfa0487 ("xfs: fix locking in xchk_nlinks_collect_dir")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/nlinks.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)


diff --git a/fs/xfs/scrub/nlinks.c b/fs/xfs/scrub/nlinks.c
index 355ab6de23eaba..bcedb8c3e4e608 100644
--- a/fs/xfs/scrub/nlinks.c
+++ b/fs/xfs/scrub/nlinks.c
@@ -382,6 +382,12 @@ xchk_nlinks_ilock_dir(
 {
 	uint			lock_mode = XFS_ILOCK_SHARED;
 
+	/*
+	 * Take the IOLOCK so that other threads cannot start a directory
+	 * update while we're scanning.
+	 */
+	xfs_ilock(ip, XFS_IOLOCK_SHARED);
+
 	/*
 	 * We're going to scan the directory entries, so we must be ready to
 	 * pull the data fork mappings into memory if they aren't already.
@@ -397,13 +403,8 @@ xchk_nlinks_ilock_dir(
 	    xfs_need_iread_extents(&ip->i_af))
 		lock_mode = XFS_ILOCK_EXCL;
 
-	/*
-	 * Take the IOLOCK so that other threads cannot start a directory
-	 * update while we're scanning.
-	 */
-	lock_mode |= XFS_IOLOCK_SHARED;
 	xfs_ilock(ip, lock_mode);
-	return lock_mode;
+	return lock_mode | XFS_IOLOCK_SHARED;
 }
 
 /* Walk a directory to bump the observed link counts of the children. */

[PATCH 6/6] xfs: avoid UAF on sc->tempip in xrep_tempfile_create

[Darrick J. Wong]

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

LOLLM noticed a potential UAF if the tempfile creation code fails after
it set sc->tempip.  Fix that.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: 84c14ee39dd388 ("xfs: create temporary files and directories for online repair")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/tempfile.c |    1 +
 1 file changed, 1 insertion(+)


diff --git a/fs/xfs/scrub/tempfile.c b/fs/xfs/scrub/tempfile.c
index e0c630f888cf6e..98820003b9298f 100644
--- a/fs/xfs/scrub/tempfile.c
+++ b/fs/xfs/scrub/tempfile.c
@@ -174,6 +174,7 @@ xrep_tempfile_create(
 		xfs_iunlock(sc->tempip, XFS_ILOCK_EXCL);
 		xfs_finish_inode_setup(sc->tempip);
 		xchk_irele(sc, sc->tempip);
+		sc->tempip = NULL;
 	}
 out_release_dquots:
 	xfs_qm_dqrele(udqp);

Re: [PATCH 1/6] xfs: only check mergeability of bnobt records

[Christoph Hellwig]

On Thu, Jul 23, 2026 at 11:21:16PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In the cntbt (free space by block count) btree, records are not supposed
> to be in startblock order.  Hence the mergeability check is pointless.
> Remove it, since it does nothing, as LOLLM points out.

Looks good:

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

Re: [PATCH 2/6] xfs: don't double-lock when deleting a self-referential directory

[Christoph Hellwig]

On Thu, Jul 23, 2026 at 11:21:31PM -0700, Darrick J. Wong wrote:
> -	error = xfs_trans_alloc_dir(dp, &M_RES(mp)->tr_remove, sc->ip,
> -			&resblks, &sc->tp, &dontcare);
> +	if (sc->ip == dp) {
> +again:
> +		error = xfs_trans_alloc_inode(dp, &M_RES(mp)->tr_remove,
> +				resblks, 0, false, &sc->tp);
> +		if ((error == -ENOSPC || error == -EDQUOT) && resblks > 0) {
> +			resblks = 0;
> +			goto again;
> +		}
> +	} else {
> +		error = xfs_trans_alloc_dir(dp, &M_RES(mp)->tr_remove, sc->ip,
> +				&resblks, &sc->tp, &dontcare);

Kinda annoying that the resblks handling is so different between the
two, but we'not going to fix this right now:

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

Re: [PATCH 3/6] xfs: don't return EFSCORRUPTED when scrubbing corrupt parent pointers

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 4/6] xfs: don't zap the attr fork on repair when there are queued pptr updates

[Christoph Hellwig]

On Thu, Jul 23, 2026 at 11:22:03PM -0700, Darrick J. Wong wrote:
> -	if (rx->attrs_found == 0) {
> +	if (rx->attrs_found == 0 && (!xfs_has_parent(sc->mp) ||
> +				     xfarray_length(rx->pptr_recs) == 0)) {

Hmm, this formatting looks at least a bit confusing.  Why not:

	if (rx->attrs_found == 0 &&
	    (!xfs_has_parent(sc->mp) || xfarray_length(rx->pptr_recs) == 0)) {

?

Otherwise looks good:

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

Re: [PATCH 5/6] xfs: nlink scrub must take IOLOCK before determining ILOCK state

[Christoph Hellwig]

On Thu, Jul 23, 2026 at 11:22:18PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In xchk_nlinks_ilock_dir, take the IOLOCK before accessing internal
> inode state to figure out if we need to take ILOCK shared or exclusive.
> That way we can't race with directory updates.  LOLLM pointed out that
> the code was initially correct w.r.t. the IOLOCK, but then I broke it.

Looks good:

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

Re: [PATCH 6/6] xfs: avoid UAF on sc->tempip in xrep_tempfile_create

[Christoph Hellwig]

On Thu, Jul 23, 2026 at 11:22:34PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM noticed a potential UAF if the tempfile creation code fails after
> it set sc->tempip.  Fix that.

Looks good:

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