Re: [PATCH 12/14] xfs: don't allow sorting sparse arrays

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

On Sun, Sep 20, 2026 at 11:18:06PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Now that we've reduced the functionality of xfarray_unset, let's add a
> new safeguard: no sorting of xfarrays with sparse holes in them.  It's
> not clear what that even means, and nobody actually does this, so we're
> really just eliminating subtle logic bombs.
> 
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
>  fs/xfs/scrub/xfarray.h |    3 +++
>  fs/xfs/scrub/xfarray.c |   11 +++++++++++
>  2 files changed, 14 insertions(+)
> 
> 
> diff --git a/fs/xfs/scrub/xfarray.h b/fs/xfs/scrub/xfarray.h
> index d55225c7885b25..05ff65b09fcf41 100644
> --- a/fs/xfs/scrub/xfarray.h
> +++ b/fs/xfs/scrub/xfarray.h
> @@ -32,6 +32,9 @@ struct xfarray {
>  
>  	/* log2 of array element size, if possible. */
>  	int		obj_size_log;
> +
> +	/* Might there be sparse holes in this array? */
> +	bool		possibly_sparse;
>  };
>  
>  int xfarray_create(const char *descr, unsigned long long required_capacity,
> diff --git a/fs/xfs/scrub/xfarray.c b/fs/xfs/scrub/xfarray.c
> index ee3f0bf87433f1..78274ad53097e1 100644
> --- a/fs/xfs/scrub/xfarray.c
> +++ b/fs/xfs/scrub/xfarray.c
> @@ -193,6 +193,8 @@ xfarray_store(
>  	if (ret)
>  		return ret;
>  
> +	if (idx > array->nr)
> +		array->possibly_sparse = true;
>  	array->nr = max(array->nr, idx + 1);
>  	return 0;
>  }
> @@ -844,6 +846,14 @@ xfarray_sort(
>  		return 0;
>  	if (array->nr >= QSORT_MAX_RECS)
>  		return -E2BIG;
> +	if (array->possibly_sparse) {
> +		/*
> +		 * What does it mean to sort an array with holes in it?
> +		 * Currently none of the users need this ability.
> +		 */
> +		ASSERT(array->possibly_sparse);

Shashiko says:

"Will this assertion always evaluate to true?
Since it is located inside the if (array->possibly_sparse) block, it acts
as a no-op instead of catching sparse array sorting in debug builds. Should
this be ASSERT(!array->possibly_sparse) before the condition, or ASSERT(0)
inside it?"

Yes.  The logic is backwards.  Will fix.

> +		return -EINVAL;
> +	}
>  
>  	error = xfarray_sortinfo_alloc(array, cmp_fn, flags, &si);
>  	if (error)
> @@ -997,4 +1007,5 @@ xfarray_truncate(
>  {
>  	xfile_discard(array->xfile, 0, MAX_LFS_FILESIZE);
>  	array->nr = 0;
> +	array->possibly_sparse = false;
>  }

"Since the new possibly_sparse flag is cleared here when the array is
truncated to 0 elements, does xfarray_trim() need a similar update?
Looking at fs/xfs/scrub/xfarray.c:xfarray_trim(), it can also reduce
the array to 0 elements:
int
xfarray_trim(
	struct xfarray		*array,
	unsigned long long	nr)
{
	if (nr > array->nr)
		return -ENODATA;
	array->nr -= nr;
	xfile_discard(array->xfile, xfarray_pos(array, array->nr),
			MAX_LFS_FILESIZE);
	return 0;
}
If an array is sparse, completely trimmed to 0 elements via
xfarray_trim(array, array->nr), and then populated densely, it appears
the possibly_sparse flag would remain true. Could this cause future
calls to xfarray_sort() on the dense array to fail with a spurious
-EINVAL?"

Yes, the trim function needs it too.  Nobody does that, but I agree not
to leave a logic bomb.

--D