On Fri, Aug 22, 2025 at 12:55:56PM -0500, Eric Sandeen wrote:
> ENODATA (aka ENOATTR) has a very specific meaning in the xfs xattr code;
> namely, that the requested attribute name could not be found.
>
> However, a medium error from disk may also return ENODATA. At best,
> this medium error may escape to userspace as "attribute not found"
> when in fact it's an IO (disk) error.
>
> At worst, we may oops in xfs_attr_leaf_get() when we do:
>
> error = xfs_attr_leaf_hasname(args, &bp);
> if (error == -ENOATTR) {
> xfs_trans_brelse(args->trans, bp);
> return error;
> }
>
> because an ENODATA/ENOATTR error from disk leaves us with a null bp,
> and the xfs_trans_brelse will then null-deref it.
>
> As discussed on the list, we really need to modify the lower level
> IO functions to trap all disk errors and ensure that we don't let
> unique errors like this leak up into higher xfs functions - many
> like this should be remapped to EIO.
>
> However, this patch directly addresses a reported bug in the xattr
> code, and should be safe to backport to stable kernels. A larger-scope
> patch to handle more unique errors at lower levels can follow later.
>
> (Note, prior to 07120f1abdff we did not oops, but we did return the
> wrong error code to userspace.)
>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> Fixes: 07120f1abdff ("xfs: Add xfs_has_attr and subroutines")
> Cc: <stable@xxxxxxxxxxxxxxx> # v5.9+
Yeah, seems fine to me. Thanks for putting this together.
Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
--D
> ---
>
> V2: Remove the extraneous trap point as pointed out by djwong, oops.
>
> (I get it that sprinkling this around in 2 places might have an ick
> factor but I think it's necessary to make a surgical strike on this bug
> before we address the general problem.)
>
> Thanks,
> -Eric
>
>
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index 4c44ce1c8a64..bff3dc226f81 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -435,6 +435,13 @@ xfs_attr_rmtval_get(
> 0, &bp, &xfs_attr3_rmt_buf_ops);
> if (xfs_metadata_is_sick(error))
> xfs_dirattr_mark_sick(args->dp, XFS_ATTR_FORK);
> + /*
> + * ENODATA from disk implies a disk medium failure;
> + * ENODATA for xattrs means attribute not found, so
> + * disambiguate that here.
> + */
> + if (error == -ENODATA)
> + error = -EIO;
> if (error)
> return error;
>
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 17d9e6154f19..723a0643b838 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -2833,6 +2833,12 @@ xfs_da_read_buf(
> &bp, ops);
> if (xfs_metadata_is_sick(error))
> xfs_dirattr_mark_sick(dp, whichfork);
> + /*
> + * ENODATA from disk implies a disk medium failure; ENODATA for
> + * xattrs means attribute not found, so disambiguate that here.
> + */
> + if (error == -ENODATA && whichfork == XFS_ATTR_FORK)
> + error = -EIO;
> if (error)
> goto out_free;
>
>
>
>
