On Thu, Aug 21, 2025 at 04:36:10PM -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+
> ---
>
> (I get it that sprinkling this around to 3 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_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index fddb55605e0c..9b54cfb0e13d 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -478,6 +478,12 @@ xfs_attr3_leaf_read(
>
> err = xfs_da_read_buf(tp, dp, bno, 0, bpp, XFS_ATTR_FORK,
> &xfs_attr3_leaf_buf_ops);
> + /*
> + * ENODATA from disk implies a disk medium failure; ENODATA for
> + * xattrs means attribute not found, so disambiguate that here.
> + */
> + if (err == -ENODATA)
> + err = -EIO;
I think this first chunk isn't needed since you also changed
xfs_da_read_buf to filter the error code, right?
(Shifting towards the giant reconsideration of ENODATA -> EIO filtering)
Do we now also need to adjust the online fsck code to turn ENODATA into
a corruption report? From __xchk_process_error:
case -EFSBADCRC:
case -EFSCORRUPTED:
/* Note the badness but don't abort. */
sc->sm->sm_flags |= errflag;
*error = 0;
fallthrough;
default:
trace_xchk_op_error(sc, agno, bno, *error, ret_ip);
break;
}
We only flag corruptions for these two error codes, but ENODATA from the
block layer means "critical medium error". I take that to mean the
media has permanently lost whatever was persisted there, right?
--D
> if (err || !(*bpp))
> return err;
>
> 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;
>
>
>
