Re: [PATCH 1/3] xfs: fix agf/agfl verification on v4 filesystems

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

On Mon, May 29, 2023 at 10:08:23AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When a v4 filesystem has fl_last - fl_first != fl_count, we do not
> not detect the corruption and allow the AGF to be used as it if was
> fully valid. On V5 filesystems, we reset the AGFL to empty in these
> cases and avoid the corruption at a small cost of leaked blocks.
> 
> If we don't catch the corruption on V4 filesystems, bad things
> happen later when an allocation attempts to trim the free list
> and either double-frees stale entries in the AGFl or tries to free
> NULLAGBNO entries.
> 
> Either way, this is bad. Prevent this from happening by using the
> AGFL_NEED_RESET logic for v4 filesysetms, too.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Seems logical,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_alloc.c | 59 ++++++++++++++++++++++++++++-----------
>  1 file changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 61eb65be17f3..fd3293a8c659 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -628,6 +628,25 @@ xfs_alloc_fixup_trees(
>  	return 0;
>  }
>  
> +/*
> + * We do not verify the AGFL contents against AGF-based index counters here,
> + * even though we may have access to the perag that contains shadow copies. We
> + * don't know if the AGF based counters have been checked, and if they have they
> + * still may be inconsistent because they haven't yet been reset on the first
> + * allocation after the AGF has been read in.
> + *
> + * This means we can only check that all agfl entries contain valid or null
> + * values because we can't reliably determine the active range to exclude
> + * NULLAGBNO as a valid value.
> + *
> + * However, we can't even do that for v4 format filesystems because there are
> + * old versions of mkfs out there that does not initialise the AGFL to known,
> + * verifiable values. HEnce we can't tell the difference between a AGFL block
> + * allocated by mkfs and a corrupted AGFL block here on v4 filesystems.
> + *
> + * As a result, we can only fully validate AGFL block numbers when we pull them
> + * from the freelist in xfs_alloc_get_freelist().
> + */
>  static xfs_failaddr_t
>  xfs_agfl_verify(
>  	struct xfs_buf	*bp)
> @@ -637,12 +656,6 @@ xfs_agfl_verify(
>  	__be32		*agfl_bno = xfs_buf_to_agfl_bno(bp);
>  	int		i;
>  
> -	/*
> -	 * There is no verification of non-crc AGFLs because mkfs does not
> -	 * initialise the AGFL to zero or NULL. Hence the only valid part of the
> -	 * AGFL is what the AGF says is active. We can't get to the AGF, so we
> -	 * can't verify just those entries are valid.
> -	 */
>  	if (!xfs_has_crc(mp))
>  		return NULL;
>  
> @@ -2321,12 +2334,16 @@ xfs_free_agfl_block(
>  }
>  
>  /*
> - * Check the agfl fields of the agf for inconsistency or corruption. The purpose
> - * is to detect an agfl header padding mismatch between current and early v5
> - * kernels. This problem manifests as a 1-slot size difference between the
> - * on-disk flcount and the active [first, last] range of a wrapped agfl. This
> - * may also catch variants of agfl count corruption unrelated to padding. Either
> - * way, we'll reset the agfl and warn the user.
> + * Check the agfl fields of the agf for inconsistency or corruption.
> + *
> + * The original purpose was to detect an agfl header padding mismatch between
> + * current and early v5 kernels. This problem manifests as a 1-slot size
> + * difference between the on-disk flcount and the active [first, last] range of
> + * a wrapped agfl.
> + *
> + * However, we need to use these same checks to catch agfl count corruptions
> + * unrelated to padding. This could occur on any v4 or v5 filesystem, so either
> + * way, we need to reset the agfl and warn the user.
>   *
>   * Return true if a reset is required before the agfl can be used, false
>   * otherwise.
> @@ -2342,10 +2359,6 @@ xfs_agfl_needs_reset(
>  	int			agfl_size = xfs_agfl_size(mp);
>  	int			active;
>  
> -	/* no agfl header on v4 supers */
> -	if (!xfs_has_crc(mp))
> -		return false;
> -
>  	/*
>  	 * The agf read verifier catches severe corruption of these fields.
>  	 * Repeat some sanity checks to cover a packed -> unpacked mismatch if
> @@ -2889,6 +2902,19 @@ xfs_alloc_put_freelist(
>  	return 0;
>  }
>  
> +/*
> + * Verify the AGF is consistent.
> + *
> + * We do not verify the AGFL indexes in the AGF are fully consistent here
> + * because of issues with variable on-disk structure sizes. Instead, we check
> + * the agfl indexes for consistency when we initialise the perag from the AGF
> + * information after a read completes.
> + *
> + * If the index is inconsistent, then we mark the perag as needing an AGFL
> + * reset. The first AGFL update performed then resets the AGFL indexes and
> + * refills the AGFL with known good free blocks, allowing the filesystem to
> + * continue operating normally at the cost of a few leaked free space blocks.
> + */
>  static xfs_failaddr_t
>  xfs_agf_verify(
>  	struct xfs_buf		*bp)
> @@ -2962,7 +2988,6 @@ xfs_agf_verify(
>  		return __this_address;
>  
>  	return NULL;
> -
>  }
>  
>  static void
> -- 
> 2.40.1
>