Re: [PATCH 2/4] xfs: only call xf{array,blob}_destroy if we have a valid pointer

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Feb 05, 2026 at 11:40:27AM -0800, Chris Mason wrote:
> "Darrick J. Wong" <djwong@kernel.org> wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Only call the xfarray and xfblob destructor if we have a valid pointer,
> > and be sure to null out that pointer afterwards.  Note that this patch
> > fixes a large number of commits, most of which were merged between 6.9
> > and 6.10.
> 
> Hi everyone,
> 
> I'm trying out my AI review prompts on a few more trees, and I ran it
> on the fuse-iomap-cache branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=fuse-iomap-cache
> 
> This was the latest email lore could find, apologies if I grabbed the wrong one.
> 
> Hopefully the review below is useful, please let me know if there are places it
> has gone wildly wrong.  I kept the header to make it obvious exactly which
> commit I'd run through.
> 
> I was really going for the fuse patches, but this ended up in my git
> rev-list output, and it looks valid to me:
> 
> commit 1e4e64674c788423e24610612e4981eb1dc9b28b
> Author: Darrick J. Wong <djwong@kernel.org>
> 
> xfs: only call xf{array,blob}_destroy if we have a valid pointer
> 
> This patch adds null pointer checks before calling xfarray_destroy and
> xfblob_destroy functions across multiple XFS scrub/repair teardown
> functions, and nulls out the pointers afterwards to prevent dangling
> pointer issues.
> 
> Cc: r772577952@gmail.com
> Cc: <stable@vger.kernel.org> # v6.12
> Fixes: ab97f4b1c03075 ("xfs: repair AGI unlinked inode bucket lists")
> 
> > diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
> > index a2f6a7f71d839..6e3fef36d6614 100644
> > --- a/fs/xfs/scrub/agheader_repair.c
> > +++ b/fs/xfs/scrub/agheader_repair.c
> 
> [ ... ]
> 
> > diff --git a/fs/xfs/scrub/dir_repair.c b/fs/xfs/scrub/dir_repair.c
> > index 7a21b688a4715..d5a55eabf6801 100644
> > --- a/fs/xfs/scrub/dir_repair.c
> > +++ b/fs/xfs/scrub/dir_repair.c
> > @@ -172,8 +172,12 @@ xrep_dir_teardown(
> >  	struct xrep_dir		*rd = sc->buf;
> >
> >  	xrep_findparent_scan_teardown(&rd->pscan);
> > -	xfblob_destroy(rd->dir_names);
> > -	xfarray_destroy(rd->dir_entries);
> > +	if (rd->dir_names)
> > +		xfblob_destroy(rd->dir_names);
> > +	rd->dir_names = NULL;
> > +	if (rd->dir_entries)
> > +		xfarray_destroy(rd->dir_entries);
> > +	rd->dir_names = NULL;
>                  ^^^^^^^^^^
> 
> In xrep_dir_teardown, after destroying rd->dir_entries, the code sets
> rd->dir_names = NULL instead of rd->dir_entries = NULL. This leaves
> dir_entries as a dangling pointer after the xfarray is freed.
> 
> The commit message states the patch should "be sure to null out that
> pointer afterwards", and the pattern in xrep_agi_buf_cleanup correctly
> nulls both iunlink_prev and iunlink_next pointers. This change should
> null both dir_names (line 177) and dir_entries (line 180), but line 180
> incorrectly nulls dir_names again.
> 
> Shouldn't line 180 be rd->dir_entries = NULL?

Yep.  Will fix this.

> [ ... ]
> 
> > Fixes: ab97f4b1c03075 ("xfs: repair AGI unlinked inode bucket lists")
> 
> The existing Fixes: tag points to commit ab97f4b1c03075, but this is not
> the first commit that introduced the bug. The bug was first introduced by
> commit f1184081ac97 ("xfs: teach scrub to check file nlinks") on
> 2024-02-22, which unconditionally called xfarray_destroy without checking
> for NULL pointers. The commit ab97f4b1c03075 (2024-04-15) is one of
> several later commits that perpetuated the same bug pattern.
> 
> Should the Fixes: tag reference f1184081ac97 instead?

Strictly speaking there should have been separate patches for each of
the files fixed in this patch, but I went with the more recent commit
which was introduced in 6.12 rather than the oldest commit from 6.9
because only 6.12 is receiving fixes anyway.

--D