Re: [PATCH 1/3] xfs: empty xattr leaf header blocks are not corruption

[Dave Chinner <david@fromorbit.com>]

On Mon, Jun 27, 2022 at 02:35:27PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> TLDR: Revert commit 51e6104fdb95 ("xfs: detect empty attr leaf blocks in
> xfs_attr3_leaf_verify") because it was wrong.
....
> 
> Original-bug: 517c22207b04 ("xfs: add CRCs to attr leaf blocks")
> Still-not-fixed-by: 2e1d23370e75 ("xfs: ignore leaf attr ichdr.count in verifier during log replay")
> Removed-in: f28cef9e4dac ("xfs: don't fail verifier on empty attr3 leaf block")
> Fixes: 51e6104fdb95 ("xfs: detect empty attr leaf blocks in xfs_attr3_leaf_verify")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c |   26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 37e7c33f6283..f6629960e17b 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -289,6 +289,23 @@ xfs_attr3_leaf_verify_entry(
>  	return NULL;
>  }
>  
> +/*
> + * Validate an attribute leaf block.
> + *
> + * Empty leaf blocks can occur under the following circumstances:
> + *
> + * 1. setxattr adds a new extended attribute to a file;
> + * 2. The file has zero existing attributes;
> + * 3. The attribute is too large to fit in the attribute fork;
> + * 4. The attribute is small enough to fit in a leaf block;
> + * 5. A log flush occurs after committing the transaction that creates
> + *    the (empty) leaf block; and
> + * 6. The filesystem goes down after the log flush but before the new
> + *    attribute can be committed to the leaf block.
> + *
> + * Hence we need to ensure that we don't fail the validation purely
> + * because the leaf is empty.
> + */
>  static xfs_failaddr_t
>  xfs_attr3_leaf_verify(
>  	struct xfs_buf			*bp)
> @@ -310,15 +327,6 @@ xfs_attr3_leaf_verify(
>  	if (fa)
>  		return fa;
>  
> -	/*
> -	 * Empty leaf blocks should never occur;  they imply the existence of a
> -	 * software bug that needs fixing. xfs_repair also flags them as a
> -	 * corruption that needs fixing, so we should never let these go to
> -	 * disk.
> -	 */
> -	if (ichdr.count == 0)
> -		return __this_address;
> -
>  	/*
>  	 * firstused is the block offset of the first name info structure.
>  	 * Make sure it doesn't go off the block or crash into the header.

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com