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

[Chris Mason <clm@meta.com>]

"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?

[ ... ]

> 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?