On Mon, Jun 27, 2022 at 02:35:27PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
--
Dave Chinner
david@xxxxxxxxxxxxx
