Re: [PATCH V2] xfs: do not propagate ENODATA disk errors into xattr code

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

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@redhat.com>
> Fixes: 07120f1abdff ("xfs: Add xfs_has_attr and subroutines")
> Cc: <stable@vger.kernel.org> # v5.9+

Yeah, seems fine to me.  Thanks for putting this together.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--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;
>  
> 
> 
>